@libinyang I dont get this. Previously we set the input params for the next module connected to the copier based on the output format set for Pin 0. Now you will keep overwriting the params with the output formats and what will stick is the last one, right? Am I understanding this correctly?

We don't support different output formats currently. The outputs of one widget should be the same. I heard Chao is working on different output formats. At that time we will support this feature.

This patch is just to find a general point to change the parameter. Copier may change the hw_params, and other modules may change the hw_params also, such as micsel, src. With this patch, we don't need to add a patch each time we support a new module.

@libinyang can you please coordinate with @aiChaoSONG and come up with one fix?

@libinyang can you please coordinate with @aiChaoSONG and come up with one fix?

@ranj063 Thanks for the suggestion. We discussed it today and decide combine the 2 PR into one.

@ranj063 Thanks for the suggestion. We discussed it today and decide combine the 2 PR into one.

Synced with Chao that he will not submit the hw_params update patch as SmartAMP doesn't need such feature. So I reopen this PR.

what do the last 3 lines above do and what are they needed for?

openmin and openmax means whether the min value and max value is included in the set or not. If open is set, the value is not included in the set. If we set min and max to the same, but they are open, this will be wrong. So we must set they are not open.

I think i->integer means this is a integer or not. Maybe we don't need to set i->integer as this should be set already. However I refer snd_interval_refine() at

@libinyang this sort of code doesn't seem be used in most drivers, i.e. there are exactly two occurrences of such code in the entire sound/soc tree. I would assert that this is needlessly complicated?

sound/soc$ git grep openmax
atmel/atmel_ssc_dai.c:          t.openmin = t.openmax = 0;
sti/sti_uniperif.c:     t.openmax = 0;

@plbossart The integer in atmel and sti belongs the local variable struct snd_interval t. The snd_interval_refine() will be called immediately after this setting. And the set of t.integer = 0 is just letting the integer in the local struct snd_interval t not impact integer in the hw_param's snd_interval i.
However I think integer may be not necessary to set in our case as the integer should already set before and don't need to be changed.

My point @libinyang is that virtually no one else has played with this sort of constraints. That makes me question if this is a wise direction. This is a very very obscure part of ALSA, do we even want to look into this?

@plbossart Yes. I agree we do not touch integer. Setting min, max, openmin and openmax is enough.

you're not getting the point @libinyang. Even openmin and openmax are not used very often, and not at all outside of codecs, so it's unclear to me why the SOF driver would need to be the first one to use these fields.

you're not getting the point @libinyang. Even openmin and openmax are not used very often, and not at all outside of codecs, so it's unclear to me why the SOF driver would need to be the first one to use these fields.

I removed the openmin, openmax and integer updating in the new version as it is supposed that such setting should already be set correctly when open.

i'd return in the if {} block and remove the else if

OK. I will update it in the next version

@libinyang how was this tested? AFAICT i is uninitialized at this point. So what is available_fmt->out_audio_fmt[i]?

@libinyang &available_fmt->out_audio_fmt[i] should be &process->output_format, right? IIRC, the origin PR updated pipeline_params in sof_ipc4_init_audio_fmt and it can cover all copier widgets. But now you update pipeline_params here. It means @RanderWang need to update pipeline_params in sof_ipc4_prepare_copier_module since he need to convert the format in a buffer type copier.

@libinyang how was this tested? AFAICT i is uninitialized at this point. So what is available_fmt->out_audio_fmt[i]?

@ranj063 Thanks for pointing it out. I copied the code from my origin code and didn't do the corresponding changing.

&available_fmt->out_audio_fmt[i] should be &process->output_format, right?

Yes, Thanks for suggestion @bardliao.

this should be sof_ipc4_update_hw_params() but it is actually forcing a single configuration to the pointed hw_params.

Thanks for the suggestion. I will rename the function. It updates the snd_pcm_hw_params based on the module instance output.

do you really need this function? Can we just set these in the function below directly?

Sure, I will do it.

I dont quite understand what this statemeant means "if params is not snd_mask, then it is snd_interval". "params" is obviously not snd_mask. it is snd_pcm_ha_params.

Oh, I mean snd_pcm_hw_param_t var to determine whether it is a snd_mask or snd_interval. Anyway, I will merge this function into sof_update_hw_params() as you suggested. So we don't need this function.

@libinyang IIUC this flag is only applicable for modules that are built into the base FW. Why do we only update the pipeline params only for base FW modules and not all external modules?

@ranj063, I don't get why we are locking module features to their position (in or out of basefw). When loaded to the DSP, it is there, does not matter if it was loaded as part of the base library or as an external library.

This flag is telling that the given module's MOD_INIT_INSTANCE message should also have this extra output format section, it is irrelevant how that module got into the RAM of the DSP.

imho.

@ujfalusi you are right once it is in the DSP, it is there. But this part is before it is initialized in the DSP and how we initialize them is very module implementation specific. Unfortunately for us, there is a difference between how a module in the base FW in initialized and how external module is initialized.

But to clarify my comment, my point was that updating the output params in the pipeline params should be independent of whether the module is built-in or not. And thie flags "payload_with_output_format) is only applicable to the built-in ones. So the check is restrictive and it shouldnt be.

@ranj063, I don't think so. The library loading is a separate step and after that is done the module is in the DSP. Only modules which are already loaded can be initialized. loading a module to firmware and the module initialization is completely different thing, or it is supposed to be.

I think @ranj063 is talking the IADK modules, which use different payload format with base FW module. I didn't support IADK modules in this PR, because they are using different payload format and SmartAMP and WoV doesn't need to update the params for IADK modules and don't have any scenarios to test the feature. We will add the IADK support when it is needed.

@libinyang I think we should update the pipeline params based on the output format for all process modules unconditionally. It doesn't make sense to restrict it to only those that have the output format in their init module instance ipc payload.

@libinyang I think we should update the pipeline params based on the output format for all process modules unconditionally. It doesn't make sense to restrict it to only those that have the output format in their init module instance ipc payload.

Yes, this is our final goal, to support all process modules. However, we currently don't have any user case to test it. I think it should be easy to add such support. @aiChaoSONG had a patch before. The 2 types of modules has different implmentation to support this feature. I think when we need to add this feature for IADK module, we can submit another PR for it based on this PR. What do you think?