Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sdw rework #3

Closed
wants to merge 4 commits into from
Closed

Sdw rework #3

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
343e424
ASoC: codec: rt700: Add prepare support in rt700
RanderWang Aug 13, 2019
835b6a8
ASoC: SOF: sdw: revert change for sdw dai in sof driver
RanderWang Aug 13, 2019
7c0809b
soundwire: intel: add more features for sdw dai driver
RanderWang Aug 13, 2019
847e2a0
ASoC: SOF: sdw: add a callback function to free resource in FW
RanderWang Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions drivers/soundwire/intel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,17 @@ static int intel_config_stream(struct sdw_intel *sdw,
return -EIO;
}

static int intel_free_stream(struct sdw_intel *sdw,
struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
if (sdw->res->ops && sdw->res->ops->free_stream && sdw->res->arg)
return sdw->res->ops->free_stream(sdw->res->arg,
substream, dai);

return 0;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be separate patch

/*
* bank switch routines
*/
Expand Down Expand Up @@ -738,6 +749,62 @@ static int intel_startup(struct snd_pcm_substream *substream,
return ret;
}

static int sdw_stream_setup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct sdw_stream_runtime *sdw_stream = NULL;
char *name;
int i, ret = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous initialisation?


name = kzalloc(32, GFP_KERNEL);
if (!name)
return -ENOMEM;

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
snprintf(name, 32, "%s-Playback", dai->name);
else
snprintf(name, 32, "%s-Capture", dai->name);

sdw_stream = sdw_alloc_stream(name);
if (!sdw_stream) {
dev_err(dai->dev,
"alloc stream failed for DAI %s: %d",
dai->name, ret);
ret = -ENOMEM;
goto error;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you only kfree(name) here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is wrong. The concept of 'stream' means we can use multiple DAIs (cpu and codec), so doing a sdw_alloc_stream here is just not quite right. see comments

/**
 * sdw_alloc_stream() - Allocate and return stream runtime
 *
 * @stream_name: SoundWire stream name
 *
 * Allocates a SoundWire stream runtime instance.
 * sdw_alloc_stream should be called only once per stream. Typically
 * invoked from ALSA/ASoC machine/platform driver.
 */

}

/* Set stream pointer on CPU DAI */
ret = snd_soc_dai_set_sdw_stream(dai,
sdw_stream,
substream->stream);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang please use one line if possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks！I get it.

if (ret < 0) {
dev_err(dai->dev, "failed to set stream pointer on cpu dai %s",
dai->name);
return ret;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memory leak, use goto error

}

/* Set stream pointer on all CODEC DAIs */
for (i = 0; i < rtd->num_codecs; i++) {
ret = snd_soc_dai_set_sdw_stream(rtd->codec_dais[i],
sdw_stream,
substream->stream);
if (ret < 0) {
dev_err(dai->dev, "failed to set stream pointer on codec dai %s",
rtd->codec_dais[i]->name);
return ret;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memory leak here too?

}
}

return 0;

error:
kfree(name);
sdw_release_stream(sdw_stream);
return ret;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not done in the right order, you need to revert the kfree and sdw_release_stream

}

static int intel_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
Expand All @@ -750,6 +817,12 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
int ret, i, ch, dir;
bool pcm = true;

ret = sdw_stream_setup(substream, dai);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do sdw_stream_step() in hw_params().I think it should be done in startup() to keep it in line with what is there already

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be done in a dai operation, the notion of stream is larger than a dai.
For example the same stereo 'stream' can be rendered by using link0-dai0 and link1-dai4, going to two separate amplifiers. This setup should be done in a dailink operation where we will have the list of all cpu and codec dais handled.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still struggling here, in the initial proposal there was no notion of DAI,

Initially we did all the soundwire stream handling in pcm_ops callbacks.

Here everything is done as part of dai_ops callbacks.

That's very very different!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart but all the dai_ops are called from the pcm ops context anyway isnt it? like for example:
https://github.com/thesofproject/linux/blob/b45315d57e2ab6fce1de8bd6391d925143b4fbda/sound/soc/soc-pcm.c#L806

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ranj063, I don't get your point. I never fully understood how DPCM works so not sure if the two models are was referring to are equivalent.

Does anyone have a sequence diagram showing how those rtd, pcm_ops and cpu_dai and codec_dai ops are called?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check the ASOC and give a process flow picture

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be done in a dai operation, the notion of stream is larger than a dai.
For example the same stereo 'stream' can be rendered by using link0-dai0 and link1-dai4, going to two separate amplifiers. This setup should be done in a dailink operation where we will have the list of all cpu and codec dais handled.

I agree with @plbossart the operation can be done in dailink. Actually I think intel_hw_params() will be invoked twice if there are two sdw components in the same dailink. @RanderWang Can we allocate and release stream in machine driver? Maybe we can use startup and shutdown ops?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drawing1

Copy link
Author

@RanderWang RanderWang Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are component pcm driver and dai driver in ASoC. they all have prepare, hw_params, and trigger functions. In the following picture, dai hw_params is for dai driver , and the only hw_params is for component pcm driver. Now there is only component pcm driver in SOF for SDW, and there is only dai driver in SDW component and codec comopnent.

All the components pcm driver are invoked in both FE and BE dai, so you will find some functions are invoke twice. Although there is no component pcm driver in SDW and codec component, I still marked some functions in in picture to reflect the process flow in ASoC

Drawing2

if (ret) {
dev_err(cdns->dev, "stream setup failed:%d\n", ret);
return -EIO;
}

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
return -EIO;
Expand Down Expand Up @@ -835,24 +908,96 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
return ret;
}

static int intel_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct sdw_cdns_dma_data *dma;
int ret;

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
dev_err(dai->dev, "failed to get dma data in %s",
__func__);
return -EIO;
}

ret = sdw_prepare_stream(dma->stream);
return ret;
}

static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
struct sdw_cdns_dma_data *dma;
int ret;

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma) {
dev_err(dai->dev, "failed to get dma data in %s", __func__);
return -EIO;
}

switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
case SNDRV_PCM_TRIGGER_RESUME:
ret = sdw_enable_stream(dma->stream);
if (ret) {
dev_err(dai->dev,
"%s trigger %d failed: %d",
__func__, cmd, ret);
return ret;
}
break;

case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
ret = sdw_disable_stream(dma->stream);
if (ret) {
dev_err(dai->dev,
"%s trigger %d failed: %d",
__func__, cmd, ret);
return ret;
}
break;

default:
return -EINVAL;
}

return 0;
}

static int
intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
{
struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
struct sdw_intel *sdw = cdns_to_intel(cdns);
struct sdw_cdns_dma_data *dma;
int ret;

dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
return -EIO;

ret = sdw_deprepare_stream(dma->stream);
if (ret) {
dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
return ret;
}

ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
if (ret < 0)
dev_err(dai->dev, "remove master from stream %s failed: %d\n",
dma->stream->name, ret);

intel_port_cleanup(dma);
kfree(dma->port);

ret = intel_free_stream(sdw, substream, dai);
sdw_release_stream(dma->stream);

return ret;
}

Expand Down Expand Up @@ -898,13 +1043,17 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
.hw_free = intel_hw_free,
.shutdown = intel_shutdown,
.set_sdw_stream = intel_pcm_set_sdw_stream,
.prepare = intel_prepare,
.trigger = intel_trigger,
};

static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
.hw_params = intel_hw_params,
.hw_free = intel_hw_free,
.shutdown = intel_shutdown,
.set_sdw_stream = intel_pdm_set_sdw_stream,
.prepare = intel_prepare,
.trigger = intel_trigger,
};

static const struct snd_soc_component_driver dai_component = {
Expand Down
1 change: 1 addition & 0 deletions include/linux/soundwire/sdw_intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
struct sdw_intel_ops {
int (*config_stream)(void *arg, void *substream,
void *dai, void *hw_params, int stream_num);
int (*free_stream)(void *arg, void *substream, void *dai);
};

/**
Expand Down
64 changes: 39 additions & 25 deletions sound/soc/codecs/rt700.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,20 +1232,14 @@ static int rt700_pcm_hw_params(struct snd_pcm_substream *substream,
{
struct snd_soc_component *component = dai->component;
struct rt700_priv *rt700 = snd_soc_component_get_drvdata(component);
struct sdw_stream_config stream_config;
struct sdw_port_config port_config;
struct sdw_stream_config *stream_config;
struct sdw_port_config *port_config;
enum sdw_data_direction direction;
struct sdw_stream_data *stream;
int retval, port, num_channels;
int port, num_channels;
unsigned int val = 0;

dev_err(dai->dev, "%s %s", __func__, dai->name);
stream = snd_soc_dai_get_dma_data(dai, substream);

if (!stream)
return -ENOMEM;

dev_err(dai->dev, "1 %s %s", __func__, dai->name);
if (!rt700->slave)
return 0;

Expand All @@ -1269,24 +1263,17 @@ static int rt700_pcm_hw_params(struct snd_pcm_substream *substream,
dev_err(component->dev, "Invalid DAI id %d\n", dai->id);
return -EINVAL;
}
dev_err(dai->dev, "2 %s %s", __func__, dai->name);

stream_config.frame_rate = params_rate(params);
stream_config.ch_count = params_channels(params);
stream_config.bps = snd_pcm_format_width(params_format(params));
stream_config.direction = direction;
stream_config = &rt700->stream_config;
stream_config->frame_rate = params_rate(params);
stream_config->ch_count = params_channels(params);
stream_config->bps = snd_pcm_format_width(params_format(params));
stream_config->direction = direction;

dev_err(dai->dev, "3 %s %s", __func__, dai->name);
num_channels = params_channels(params);
port_config.ch_mask = (1 << (num_channels)) - 1;
port_config.num = port;

retval = sdw_stream_add_slave(rt700->slave, &stream_config,
&port_config, 1, stream->sdw_stream);
if (retval) {
dev_err(dai->dev, "Unable to configure port\n");
return retval;
}
port_config = &rt700->port_config;
port_config->ch_mask = (1 << (num_channels)) - 1;
port_config->num = port;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

port is not initialized?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you need those two structures? Is this to memorize information that will be used in prepare? If yes, maybe you can add comments to help the reviewer, there are none in your changes...


dev_err(dai->dev, "4 %s %s", __func__, dai->name);
switch (params_rate(params)) {
Expand Down Expand Up @@ -1339,7 +1326,33 @@ static int rt700_pcm_hw_params(struct snd_pcm_substream *substream,
snd_soc_component_write(component, RT700_ADC_FORMAT_L, val);

dev_err(dai->dev, "5 %s %s", __func__, dai->name);
return retval;
return 0;
}

static int rt700_prepare_stream(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct snd_soc_component *component = dai->component;
struct rt700_priv *rt700 = snd_soc_component_get_drvdata(component);
struct sdw_stream_data *stream;
int retval;

stream = snd_soc_dai_get_dma_data(dai, substream);

if (!stream) {
dev_err(dai->dev, "no dma data found in dai %s", dai->name);
return -ENOMEM;
}

retval = sdw_stream_add_slave(rt700->slave, &rt700->stream_config,
&rt700->port_config, 1,
stream->sdw_stream);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully get this code, is this going to initialize the 'stream'?
Again please add comments to explain what you are doing, the code is not self-explanatory.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang yeah same here. I dont understand why we need to move this to prepare. AFAICT, sdw_stream_add_slave() need the stream_config to be set, particularly the frame_rate.

if (retval) {
dev_err(dai->dev, "Unable to configure port\n");
return retval;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang just return retval outside the if. No need for 2 separate return statements here

}

return 0;
}

static int rt700_pcm_hw_free(struct snd_pcm_substream *substream,
Expand All @@ -1365,6 +1378,7 @@ static struct snd_soc_dai_ops rt700_ops = {
.hw_params = rt700_pcm_hw_params,
.hw_free = rt700_pcm_hw_free,
.set_sdw_stream = rt700_set_sdw_stream,
.prepare = rt700_prepare_stream,
.shutdown = rt700_shutdown,
};

Expand Down
2 changes: 2 additions & 0 deletions sound/soc/codecs/rt700.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ struct rt700_priv {
int dbg_payload;
enum sdw_slave_status status;
struct sdw_bus_params params;
struct sdw_stream_config stream_config;
struct sdw_port_config port_config;
bool hw_init;
};

Expand Down
1 change: 0 additions & 1 deletion sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,6 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.pcm_hw_free = hda_dsp_stream_hw_free,
.pcm_trigger = hda_dsp_pcm_trigger,
.pcm_pointer = hda_dsp_pcm_pointer,
.pcm_prepare = sdw_pcm_prepare,

/* firmware loading */
.load_firmware = snd_sof_load_firmware_raw,
Expand Down
Loading