Re: [PATCH] sound: sof: ioc4-topology: avoid extra dai_params copy
On Wed, Aug 7, 2024, at 17:09, Mark Brown wrote:
On Wed, Aug 07, 2024 at 10:02:27AM +0200, Arnd Bergmann wrote:
From what I can tell, this was unintentional, as both sof_ipc4_prepare_dai_copier() and sof_ipc4_prepare_copier_module() make a copy for the same purpose, but copying it once has the exact same effect.
Remove the extra copy and change the direct struct assignment to an explicit memcpy() call to make it clearer to the reader that this is what happens. Note that gcc treats struct assignment as a memcpy() that may be inlined anyway, so the resulting object code is the same.
The effect of the copy is to ensure that if the function fails the argument is unmodified - did you do the analysis to check that it's OK to modify on error? Your commit log says "the same purpose" but never specifies what that purpose is.
There is always a chance that I misunderstood the code, but yes, I did understand that the idea is to not modify the parameters inside of sof_ipc4_prepare_dai_copier.
The only caller is in sof_ipc4_prepare_copier_module(), which achieves the exact same bit by doing the same:
/* * Use the fe_params as a base for the copier configuration. * The ref_params might get updated to reflect what format is * supported by the copier on the DAI side. * * In case of capture the ref_params returned will be used to * find the input configuration of the copier. */ memcpy(&ref_params, fe_params, sizeof(ref_params)); ret = sof_ipc4_prepare_dai_copier(sdev, dai, &ref_params, dir); if (ret < 0) return ret;
So when sof_ipc4_prepare_dai_copier() fails, the caller's local 'ref_params' structure is no longer used anywhere.
Arnd
On Wed, 2024-08-07 at 17:18 +0200, Arnd Bergmann wrote:
On Wed, Aug 7, 2024, at 17:09, Mark Brown wrote:
On Wed, Aug 07, 2024 at 10:02:27AM +0200, Arnd Bergmann wrote:
From what I can tell, this was unintentional, as both sof_ipc4_prepare_dai_copier() and sof_ipc4_prepare_copier_module() make a copy for the same purpose, but copying it once has the exact same effect.
Remove the extra copy and change the direct struct assignment to an explicit memcpy() call to make it clearer to the reader that this is what happens. Note that gcc treats struct assignment as a memcpy() that may be inlined anyway, so the resulting object code is the same.
The effect of the copy is to ensure that if the function fails the argument is unmodified - did you do the analysis to check that it's OK to modify on error? Your commit log says "the same purpose" but never specifies what that purpose is.
There is always a chance that I misunderstood the code, but yes, I did understand that the idea is to not modify the parameters inside of sof_ipc4_prepare_dai_copier.
Hi Arnd,
The idea behind the local copy is that the DAI widget needs to handle its audio formats in topology differently from the other widgets in the pipeline. So, locally the sof_ipc4_prepare_dai_copier() modifies the params to make sure the right NHLT blobs are chosen based on what's available in topology and the information passed in the params. But when the params variable is passed on to the next widget in the pipeline, any local modifications done by the DAI widget should be carried forward.
For your reference, this is code that does the propagation of the prepare callback for each widget in the playback/capture path in the pipeline. https://github.com/thesofproject/linux/blob/bc47b82db6e03d540061964d4540a371...
Thanks, Ranjani
participants (2)
-
Arnd Bergmann -
Ranjani Sridharan