On 19.05.2021 17:15, Takashi Iwai wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 19 May 2021 12:48:36 +0200, > Codrin Ciubotariu wrote: >> >> This patchset adds a different snd_pcm_runtime in the BE's substream, >> replacing the FE's snd_pcm_runtime. With a different structure, the BE >> HW capabilities and constraints will no longer merge with the FE ones. >> This allows for error detection if the be_hw_params_fixup() applies HW >> parameters not supported by the BE DAIs. Also, it calculates values >> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and >> period size, if needed. >> >> The first 4 patches are preparatory patches, that just group and export >> functions used to allocate and initialize the snd_pcm_runtime. Also, the >> functions that set and apply the HW constraints are exported. >> The 5th patch does (almost) everything need to create the new snd_pcm_runtime >> for BEs, which includes allocation, initializing the HW capabilities, >> HW constraints and HW parameters. The BE HW parameters are no longer >> copied from the FE. They are recalculated, based on HW capabilities, >> constraints and the be_hw_params_fixup() callback. >> The 6th and last patch basically adds support for the PCM generic >> dmaengine to be used as a platform driver for BE DAI links. It allocates >> a buffer, needed by the DMA transfers that do not support dev-to-dev >> transfers between FE and BE DAIs. >> >> This is a superset of >> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html >> which only handles the BE HW constraints. This patchset aims to be more >> complete, defining a a snd_pcm_runtime between each FE and BE and can >> be used between any DAI link connection. I am sure I am not handling all >> the needed members of snd_pcm_runtime (such as handling >> struct snd_pcm_mmap_status *status), but I would like to have your >> feedback regarding this idea. > > I'm also concerned about the handling of other fields in runtime > object, maybe allocating a complete runtime object for each BE is an > overkill and fragile. Could it be rather only hw_constraints to be > unique for each BE, instead? I tried with only the hw constraints in the previous patchset and it's difficult to handle the snd_pcm_hw_rule_add() calls, without changing the function's declaration. This solution requires no changes to constraints API, nor to their 'clients'. I agree that handling all the runtime fields might be over-complicated. From what I see, the scary ones are used to describe the buffer and the status of the transfers. I do not think there are BEs that use these values at this moment (the FE ones). I think that the HW params, private section, hardware description and maybe DMA members (at least in my case) are mostly needed by BEs. > Also, the last patch allows only IRAM type, which sounds already > doubtful. The dmaengine code should be generic. dmaengine, when used with normal PCM, preallocates only IRAM buffers [1]. This BE buffer would only be needed if DMA dev-to-mem or mem-to-dev transfers are needed, between FE and BE. I agree that it could be handled differently, I added it here mostly to express my goal, which is to use the generic dmaengine for BEs. My DMA has no dev-to-dev DMA capability, so I need a buffer to move the data between FE and BE. > > Last but not least, one minor nitpick: please use EXPORT_SYMBOL_GPL() > for the newly introduced symbols. Sure, this is an oversight on my side. I will make all of them EXPORT_SYMBOL_GPL. Thank you very much for your review! Best regards, Codrin [1] https://elixir.bootlin.com/linux/v5.13-rc2/source/sound/soc/soc-generic-dmaengine-pcm.c#L266
