[RanderWang]

Hw params may be changed by be_hw_params_fixup, so it is
not correct to calculate format based on runtime information.
This makes arecord get only zero data on hda platforms.

In ASoC, hw params would be applied to be cpu dai and codec
dai by hw_params ops after it is changed by be_hw_params_fixup.
Now calculate format value in hw_params in hdac like hdac_hdmi
and other codec drivers.

Signed-off-by: Rander Wang rander.wang@linux.intel.com

[plbossart]

Sorry, I don't quite get the issue here nor if your fix is correct
In the prepare step, you configure the codec with a call to snd_hda_codec_prepare. If the fixup changes the format, and you don't reconfigure the codec, then something is very wrong.

Can you please clarify which fixups you are using for capture and how come they change the format?

[RanderWang]

ASoC flow:
dpcm_be_dai_hw_params ()
{
    ...
    be_hw_params_fixup(params);
    ...
   soc_pcm_hw_params(params) 
   {
       codec->hw_params(params);
        ...
        be dai->hw_params(params);
        ...
    }
}

SOF provide ops function: sof_pcm_dai_link_fixup for be_hw_params_fixup.

snd_mask_none(fmt)
switch(frame_fmt) {
case S16LE
        snd_mask_set(S16LE)
case ...
}

If our BE format is 24bit, hw params would be changed to 24bit in sof_pcm_dai_link_fixup even stream format is 16bit. For I2S case, all the codec driver in ASoC provide ops for hw_params, so the format changed by fixup function could be set to codec correctly by hw_params following fixup function.

For hda case, hdac_hda doesn't provide hw_params function and only get format from runtime. For BE format of 24 bits, if a 16 bits stream is played, SOF would set hda registers in intel IP to 24bit, but codec is set to 16 bits, then only zero data is gotten by arecord. Actually hdac_hdmi provides hw_params function and gets correct format.

[plbossart]

@RanderWang I appreciate the explanations, so now I get the SOF-specific part. I still don't get how this works if you configure the codec in .prepare and then change the format in .hw_params without reconfiguring the codec after the fixup. Something's not adding up.

[RanderWang]

@RanderWang I appreciate the explanations, so now I get the SOF-specific part. I still don't get how this works if you configure the codec in .prepare and then change the format in .hw_params without reconfiguring the codec after the fixup. Something's not adding up.

@plbossart The function hw_params and prepare are called by user mode application by IOCTL.
Hw_params should be called before prepare. First, I tried to configure codec in hw_params and found
stream_tag was not initialized at this time, so I gave up this.

https://vovkos.github.io/doxyrest/samples/alsa/page_pcm.html

The PCM device is in the open state. After the snd_pcm_open() open call, the device is
in this state. Also, when snd_pcm_hw_params() call fails, then this state is entered to
force application calling snd_pcm_hw_params() function to set right communication parameters.

The PCM device has accepted communication parameters and it is waiting for snd_pcm_prepare()
call to prepare the hardware for selected operation (playback or capture).

The PCM device is prepared for operation. Application can use snd_pcm_start() call, write or
read data to start the operation.

[plbossart]

Sorry, this is still very unclear.

I now get the point that using the information in the PCM runtime is not good enough, the hw_params needs to be invoked.

I don't get at all why you need a .prepare and why it necessary to keep this two-level solution with information saved during the .hw_params reused in the .prepare. the stream_tag is allocated on an open, so this feels like a work-around due to an SOF issue.

[RanderWang]

Sorry, this is still very unclear.

I now get the point that using the information in the PCM runtime is not good enough, the hw_params needs to be invoked.

I don't get at all why you need a .prepare and why it necessary to keep this two-level solution with information saved during the .hw_params reused in the .prepare. the stream_tag is allocated on an open, so this feels like a work-around due to an SOF issue.

I get your idea: you don't want .prepare function, just use .hw_params? right ?
I will debug stream_tag which is not allocated on an open.

[RanderWang]

@plbossart steam_tag is allocated by hdac_hda_dai_set_tdm_slot.
The control flow is

soc_pcm_hw_params ( params)
{
     soc_dai_hw_params(codec_dai);  \\ stream_tag is not initialized here
     ...
     component->driver->ops->hw_params() \\ hda_link_hw_params in sof
                                          \\ skl_link_hw_params in sst

          ---  snd_soc_dai_set_tdm_slot() \\ set stream_tag to 1 e.g.
      ...
}
...
soc_pcm_prepare()
{
       codec_prepare();  \\ stream_tag = 1;
}

So now I understand why author didn't do it in hw_params as usually, it is for stream_tag. And
hdac_hdmi (hdac_hdmi_set_hw_params) also done like my PR.

And I prepare another solution for this: RanderWang@a5fd59f.
Could you help to review it and give some advice about which one is better ? Thanks!

[plbossart]

@RanderWang I am still not clear, sorry. I get the flow and the fact that the stream tag isn't allocated, but why is the current solution an issue only on capture? It feels like even the playback should not work without this fix but it does, so there is still something magical about the stream management.
Also you would need to double-check that this change still works with the cAVS/SST driver.

[RanderWang]

@RanderWang I am still not clear, sorry. I get the flow and the fact that the stream tag isn't allocated, but why is the current solution an issue only on capture? It feels like even the playback should not work without this fix but it does, so there is still something magical about the stream management.
Also you would need to double-check that this change still works with the cAVS/SST driver.

Yes, I checked it on SST driver, no problem.

And The PR also improves playback. Without it, there is some faint noise in one side of headphone for the music file I often use. First, I think this is caused by my board. And I change another music file, I get great noise in one side of headphone. With this PR, all noise disappears. I will update my commit message for it.

[plbossart]

Please update the commit message along the following lines. You've got to be more direct on

1. what is the issue
2. what is the root cause
3. how it's fixed and why you selected this implementation over alternatives
4. how it's been tested.

e.g.

The current implementation of the hdac_hda codec results in zero-valued samples on capture and noise with headset playback when SOF is used on platforms with an on-board HDaudio codec. This is root-caused to SOF using be_hw_params_fixup, and the prepare() call using invalid runtime fields to determine the format.

This patch moves the format handling to the hw_params() callback, as done already for hdac_hdmi, to make sure the fixed-up information is taken into account but keeps the codec initialization in prepare() as the stream_tag is only available at that time. Moving everything in the prepare() callback is possible but the code is less elegant so this two-step solution was chosen.

The solution was tested with the SST driver with no regressions, and all the issues with SOF playback and capture are solved.

[RanderWang]

@plbossart Thanks! I study your commit message and it is a good example for me.

[plbossart]

Thanks @RanderWang, please send to alsa-devel.

[RanderWang]

Thanks! I will do it next week for it is slow to update kernel git in china.

[RanderWang]

@plbossart Done! thanks!