| Message ID | 20190725234032.21152-32-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: > +void intel_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct sdw_cdns_dma_data *dma; > + > + dma = snd_soc_dai_get_dma_data(dai, substream); > + if (!dma) > + return; > + > + snd_soc_dai_set_dma_data(dai, substream, NULL); > + kfree(dma); > +} Correct me if I'm wrong, but do we really need to _get_dma_ here? _set_dma_ seems bulletproof, same for kfree.
On 7/26/19 5:38 AM, Cezary Rojewski wrote: > On 2019-07-26 01:40, Pierre-Louis Bossart wrote: >> +void intel_shutdown(struct snd_pcm_substream *substream, >> + struct snd_soc_dai *dai) >> +{ >> + struct sdw_cdns_dma_data *dma; >> + >> + dma = snd_soc_dai_get_dma_data(dai, substream); >> + if (!dma) >> + return; >> + >> + snd_soc_dai_set_dma_data(dai, substream, NULL); >> + kfree(dma); >> +} > > Correct me if I'm wrong, but do we really need to _get_dma_ here? > _set_dma_ seems bulletproof, same for kfree. I must admit I have no idea why we have a reference to DMAs here, this looks like an abuse to store a dai-specific context, and the initial test looks like copy-paste to detect invalid configs, as done in other callbacks. Vinod and Sanyog might have more history than me here.
On 26-07-19, 09:46, Pierre-Louis Bossart wrote: > > > On 7/26/19 5:38 AM, Cezary Rojewski wrote: > > On 2019-07-26 01:40, Pierre-Louis Bossart wrote: > > > +void intel_shutdown(struct snd_pcm_substream *substream, > > > + struct snd_soc_dai *dai) > > > +{ > > > + struct sdw_cdns_dma_data *dma; > > > + > > > + dma = snd_soc_dai_get_dma_data(dai, substream); > > > + if (!dma) > > > + return; > > > + > > > + snd_soc_dai_set_dma_data(dai, substream, NULL); > > > + kfree(dma); > > > +} > > > > Correct me if I'm wrong, but do we really need to _get_dma_ here? > > _set_dma_ seems bulletproof, same for kfree. > > I must admit I have no idea why we have a reference to DMAs here, this looks > like an abuse to store a dai-specific context, and the initial test looks > like copy-paste to detect invalid configs, as done in other callbacks. Vinod > and Sanyog might have more history than me here. I dont see snd_soc_dai_set_dma_data() call for sdw_cdns_dma_data so somthing is missing (at least in upstream code) IIRC we should have a snd_soc_dai_set_dma_data() in alloc or some initialization routine and we free it here.. Sanyog?
On 8/2/19 12:28 PM, Vinod Koul wrote: > On 26-07-19, 09:46, Pierre-Louis Bossart wrote: >> >> >> On 7/26/19 5:38 AM, Cezary Rojewski wrote: >>> On 2019-07-26 01:40, Pierre-Louis Bossart wrote: >>>> +void intel_shutdown(struct snd_pcm_substream *substream, >>>> + struct snd_soc_dai *dai) >>>> +{ >>>> + struct sdw_cdns_dma_data *dma; >>>> + >>>> + dma = snd_soc_dai_get_dma_data(dai, substream); >>>> + if (!dma) >>>> + return; >>>> + >>>> + snd_soc_dai_set_dma_data(dai, substream, NULL); >>>> + kfree(dma); >>>> +} >>> >>> Correct me if I'm wrong, but do we really need to _get_dma_ here? >>> _set_dma_ seems bulletproof, same for kfree. >> >> I must admit I have no idea why we have a reference to DMAs here, this looks >> like an abuse to store a dai-specific context, and the initial test looks >> like copy-paste to detect invalid configs, as done in other callbacks. Vinod >> and Sanyog might have more history than me here. > > I dont see snd_soc_dai_set_dma_data() call for > sdw_cdns_dma_data so somthing is missing (at least in upstream code) > > IIRC we should have a snd_soc_dai_set_dma_data() in alloc or some > initialization routine and we free it here.. Sanyog? the code does a bunch of get_dma_data() and this seems to work, but indeed I don't see where the _set_dma_data() is done. magic.
>>>> +void intel_shutdown(struct snd_pcm_substream *substream, >>>> + struct snd_soc_dai *dai) >>>> +{ >>>> + struct sdw_cdns_dma_data *dma; >>>> + >>>> + dma = snd_soc_dai_get_dma_data(dai, substream); >>>> + if (!dma) >>>> + return; >>>> + >>>> + snd_soc_dai_set_dma_data(dai, substream, NULL); >>>> + kfree(dma); >>>> +} >>> >>> Correct me if I'm wrong, but do we really need to _get_dma_ here? >>> _set_dma_ seems bulletproof, same for kfree. >> >> I must admit I have no idea why we have a reference to DMAs here, this looks >> like an abuse to store a dai-specific context, and the initial test looks >> like copy-paste to detect invalid configs, as done in other callbacks. Vinod >> and Sanyog might have more history than me here. > > I dont see snd_soc_dai_set_dma_data() call for > sdw_cdns_dma_data so somthing is missing (at least in upstream code) > > IIRC we should have a snd_soc_dai_set_dma_data() in alloc or some > initialization routine and we free it here.. Sanyog? Vinod, I double-checked that we do not indeed have a call to snd_soc_dai_dma_data(), but there is code in cdns_set_stream() that sets the relevant dai->playback/capture_dma_data, see below I am not a big fan of this code, touching the ASoC core internal fields isn't a good idea in general. Also not sure why for a DAI we need both _drvdata and _dma_data (especially for this case where the information stored has absolutely nothing to do with DMAs). If the idea was to keep a context that is direction-dependent, that's likely unnecessary. For the Intel/Cadence case the interfaces can be configured as playback OR capture, not both concurrently, so the "dma" information could have been stored in the generic DAI _drvdata. I have other things to look into for now but this code will likely need to be cleaned-up at some point to remove unnecessary parts. int cdns_set_sdw_stream(struct snd_soc_dai *dai, void *stream, bool pcm, int direction) { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_cdns_dma_data *dma; dma = kzalloc(sizeof(*dma), GFP_KERNEL); if (!dma) return -ENOMEM; if (pcm) dma->stream_type = SDW_STREAM_PCM; else dma->stream_type = SDW_STREAM_PDM; dma->bus = &cdns->bus; dma->link_id = cdns->instance; dma->stream = stream; >>> this is equivalent to snd_soc_dai_dma_data() if (direction == SNDRV_PCM_STREAM_PLAYBACK) dai->playback_dma_data = dma; else dai->capture_dma_data = dma; <<<< return 0; } EXPORT_SYMBOL(cdns_set_sdw_stream);
On 14-08-19, 14:31, Pierre-Louis Bossart wrote: > > > > > > > +void intel_shutdown(struct snd_pcm_substream *substream, > > > > > + struct snd_soc_dai *dai) > > > > > +{ > > > > > + struct sdw_cdns_dma_data *dma; > > > > > + > > > > > + dma = snd_soc_dai_get_dma_data(dai, substream); > > > > > + if (!dma) > > > > > + return; > > > > > + > > > > > + snd_soc_dai_set_dma_data(dai, substream, NULL); > > > > > + kfree(dma); > > > > > +} > > > > > > > > Correct me if I'm wrong, but do we really need to _get_dma_ here? > > > > _set_dma_ seems bulletproof, same for kfree. > > > > > > I must admit I have no idea why we have a reference to DMAs here, this looks > > > like an abuse to store a dai-specific context, and the initial test looks > > > like copy-paste to detect invalid configs, as done in other callbacks. Vinod > > > and Sanyog might have more history than me here. > > > > I dont see snd_soc_dai_set_dma_data() call for > > sdw_cdns_dma_data so somthing is missing (at least in upstream code) > > > > IIRC we should have a snd_soc_dai_set_dma_data() in alloc or some > > initialization routine and we free it here.. Sanyog? > > Vinod, I double-checked that we do not indeed have a call to > snd_soc_dai_dma_data(), but there is code in cdns_set_stream() that sets the > relevant dai->playback/capture_dma_data, see below > > I am not a big fan of this code, touching the ASoC core internal fields > isn't a good idea in general. IIRC as long as you stick to single link I do not see this required. The question comes into picture when we have multi links as you would need to allocate a soundwire stream and set that for all the sdw DAIs So, what is the current model of soundwire stream, which entity allocates that and do you still care about multi-link? is there any machine driver with soundwire upstream yet? > Also not sure why for a DAI we need both _drvdata and _dma_data (especially _drvdata is global for driver whereas _dma_data is typically used per DAI > for this case where the information stored has absolutely nothing to do with > DMAs). > > If the idea was to keep a context that is direction-dependent, that's likely > unnecessary. For the Intel/Cadence case the interfaces can be configured as > playback OR capture, not both concurrently, so the "dma" information could > have been stored in the generic DAI _drvdata. > > I have other things to look into for now but this code will likely need to > be cleaned-up at some point to remove unnecessary parts. Sure please go ahead and do the cleanup. > > int cdns_set_sdw_stream(struct snd_soc_dai *dai, > void *stream, bool pcm, int direction) > { > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); > struct sdw_cdns_dma_data *dma; > > dma = kzalloc(sizeof(*dma), GFP_KERNEL); > if (!dma) > return -ENOMEM; > > if (pcm) > dma->stream_type = SDW_STREAM_PCM; > else > dma->stream_type = SDW_STREAM_PDM; > > dma->bus = &cdns->bus; > dma->link_id = cdns->instance; > > dma->stream = stream; > > >>> this is equivalent to snd_soc_dai_dma_data() > > if (direction == SNDRV_PCM_STREAM_PLAYBACK) > dai->playback_dma_data = dma; > else > dai->capture_dma_data = dma; > <<<< > return 0; > } > EXPORT_SYMBOL(cdns_set_sdw_stream);
>>>>>> +void intel_shutdown(struct snd_pcm_substream *substream, >>>>>> + struct snd_soc_dai *dai) >>>>>> +{ >>>>>> + struct sdw_cdns_dma_data *dma; >>>>>> + >>>>>> + dma = snd_soc_dai_get_dma_data(dai, substream); >>>>>> + if (!dma) >>>>>> + return; >>>>>> + >>>>>> + snd_soc_dai_set_dma_data(dai, substream, NULL); >>>>>> + kfree(dma); >>>>>> +} >>>>> >>>>> Correct me if I'm wrong, but do we really need to _get_dma_ here? >>>>> _set_dma_ seems bulletproof, same for kfree. >>>> >>>> I must admit I have no idea why we have a reference to DMAs here, this looks >>>> like an abuse to store a dai-specific context, and the initial test looks >>>> like copy-paste to detect invalid configs, as done in other callbacks. Vinod >>>> and Sanyog might have more history than me here. >>> >>> I dont see snd_soc_dai_set_dma_data() call for >>> sdw_cdns_dma_data so somthing is missing (at least in upstream code) >>> >>> IIRC we should have a snd_soc_dai_set_dma_data() in alloc or some >>> initialization routine and we free it here.. Sanyog? >> >> Vinod, I double-checked that we do not indeed have a call to >> snd_soc_dai_dma_data(), but there is code in cdns_set_stream() that sets the >> relevant dai->playback/capture_dma_data, see below >> >> I am not a big fan of this code, touching the ASoC core internal fields >> isn't a good idea in general. > > IIRC as long as you stick to single link I do not see this required. The > question comes into picture when we have multi links as you would need > to allocate a soundwire stream and set that for all the sdw DAIs > > So, what is the current model of soundwire stream, which entity allocates > that and do you still care about multi-link? is there any machine driver > with soundwire upstream yet? yes, multi-link is definitively required and one of the main appeals of SoundWire. We have a platform with 2 amplifiers on separate links and they need to be synchronized and handled with the stream concept. The tentative plan would be to move the stream allocation to the dailink .init (or equivalent), and make sure each DAI in that link used the same stream information. There are dependencies on the multi-cpu concept that Morimoto-san wanted to push, so we'll likely be the first users. For the DAI trigger, we will need to change the existing API so that a sdw_stream_enable() can be called multiple times, but only takes effect when the .trigger of the first DAI in the stream is invoked. This is a similar behavior than with HDaudio .trigger operations when the SYNC bits are used. We will do this when we have a first pass working with all codec drivers upstream and a basic machine driver upstream with all 4 links working independently. Everything is done in public btw, you can track our WIP solutions here: https://github.com/thesofproject/linux/pull/1140 https://github.com/thesofproject/linux/pull/1141 https://github.com/thesofproject/linux/pull/1142
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index dede55072191..4a189e487830 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1381,19 +1381,5 @@ int sdw_cdns_alloc_stream(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_alloc_stream); -void sdw_cdns_shutdown(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - struct sdw_cdns_dma_data *dma; - - dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) - return; - - snd_soc_dai_set_dma_data(dai, substream, NULL); - kfree(dma); -} -EXPORT_SYMBOL(sdw_cdns_shutdown); - MODULE_LICENSE("Dual BSD/GPL"); MODULE_DESCRIPTION("Cadence Soundwire Library"); diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index d375bbfead18..de97bc22acb7 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -177,8 +177,6 @@ int sdw_cdns_alloc_stream(struct sdw_cdns *cdns, void sdw_cdns_config_stream(struct sdw_cdns *cdns, struct sdw_cdns_port *port, u32 ch, u32 dir, struct sdw_cdns_pdi *pdi); -void sdw_cdns_shutdown(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai); int sdw_cdns_pcm_set_stream(struct snd_soc_dai *dai, void *stream, int direction); int sdw_cdns_pdm_set_stream(struct snd_soc_dai *dai, diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 5947fa8e840b..c40ab443e723 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -772,6 +772,19 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; } +void intel_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct sdw_cdns_dma_data *dma; + + dma = snd_soc_dai_get_dma_data(dai, substream); + if (!dma) + return; + + snd_soc_dai_set_dma_data(dai, substream, NULL); + kfree(dma); +} + static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction) { @@ -787,14 +800,14 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai, static const struct snd_soc_dai_ops intel_pcm_dai_ops = { .hw_params = intel_hw_params, .hw_free = intel_hw_free, - .shutdown = sdw_cdns_shutdown, + .shutdown = intel_shutdown, .set_sdw_stream = intel_pcm_set_sdw_stream, }; static const struct snd_soc_dai_ops intel_pdm_dai_ops = { .hw_params = intel_hw_params, .hw_free = intel_hw_free, - .shutdown = sdw_cdns_shutdown, + .shutdown = intel_shutdown, .set_sdw_stream = intel_pdm_set_sdw_stream, };
All DAI callbacks are in intel.c except for shutdown. Move and remove export symbol Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 2 -- drivers/soundwire/intel.c | 17 +++++++++++++++-- 3 files changed, 15 insertions(+), 18 deletions(-)