Re: [PATCH 02/14] ASoC: codecs: Add HD-Audio codec driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 5/6/22 10:28, Cezary Rojewski wrote:
> On 2022-05-06 4:56 PM, Pierre-Louis Bossart wrote:
>> On 5/6/22 08:39, Cezary Rojewski wrote:
> 
> ...
> 
>>> Sorry for the late response, did not realize there is an unanswered
>>> comment here.
>>>
>>> So, the rough list goes as:
>>> - hdac_hda.c hardcodes codec capabilities rather than aligning with what
>>> sound/pci/hda/ code does
>>> - merges HDMI (i.e. Intel i915 audio component) and HDA DAIs together
>>> whereas these are two separate devices
>>> - because of above, implements custom search/matching mechanism for
>>> PCM/DAI
>>> - cont. because of above, its header hosts private data struct,
>>> unnecessary complication
>>> - follows HDA_DEV_ASOC convention rather than HDA_DEV_LEGACY causing
>>> misalignments between sound/pci/hda and sound/soc/ behaviour
>>> - has basic PM runtime support and does not survive scenarios where
>>> resume/suspend + denylist + rmmod/modprobe are mixed together or invoked
>>> in unordered fashion between this module and several others in the audio
>>> stack
>>>
>>> My suggestion is different: have all HD-Audio ASoC users switch to this
>>> implementation when possible and remove the existing code along with
>>> skylake-driver.
>>
>> I am not against change and will agree that HDaudio support is far from
>> perfect, but it's been released for multiple generations from dozens of
>> OEMs and mostly works. All the issues reported to us are related to
>> codec configurations that also don't work with the legacy HDaudio
>> driver, DMIC configurations, CSME authentication or system hangs that
>> have not been root-caused [1]. HDaudio/ASoC interfaces are not on our
>> radar as problematic.
> 
> 
> That's why aligning with sound/pci/hda behavior is better for both, ALSA
> and ASoC users -> one place to fix the problems, both clients happy.
> 
>> Disrupting basic HDaudio support to do things better has to be handled
>> with extreme caution and a ton of testing involving distro maintainers
>> and community members, so we are talking about an opt-in transition, not
>> an immediate switch. We've done a similar transition in the past to stop
>> using a dedicated hdac_hdmi.c codec, see all references to the
>> 'use_common_hdmi' parameter in the SOF code. That transition seems to go
>> exactly against your second point above on HDMI and HDA being different
>> devices, so this could be an interesting debate.
>>
>> Changes to the HDAudio/ASoC support would need to be handled with a
>> separate patchset anyways, and the SOF side changes done after we are
>> finished with the IPC4 and MeteorLake upstreaming. No one in our team
>> has any bandwidth to help with reviews or tests on this topic at the
>> moment.
> 
> 
> Agree. This won't be forced on anyone and that's why separate
> implementation needed to be provided. There is too much to cover if we
> were to refactor hdac_hda.c
> 
>> I will also re-state that the removal of the skylake driver can only
>> happen after a long period of deprecation, when firmware and topologies
>> have been picked by distributions and all users are known to have
>> switched, so it's very likely that any alignment between "all HD-Audio
>> ASoC users" mentioned above does include the Skylake driver, doesn't it?
> 
> 
> Nah, I don't believe we need to be saving skylake-driver here. By "all
> HD-Audio ASoC users" I meant sof-driver as it isn't going anywhere -
> what cannot be said about the skylake-driver :)
> 
>> So to circle back: is there anything preventing the use of the existing
>> hdac_hda.c codec in this "ASoC: Intel: avs: Machine boards and HDA codec
>> support" series and can the HDaudio codec change be done "later" in a
>> more organized way?
> 
> 
> Yeah, all the pm scenarios will fail when paired with the avs-driver.
> The expectations are different. We'd have to fix probe() and remove()
> (and related) sequences for the hdac_hda.c, and given that its users did
> not notice prompts further problems with the refactor. This is very
> similar to the skylake-driver vs avs-driver case. We could have applied
> ~300 patches we had internally that prepare skylake-driver to be
> re-modeled and then apply patchsets which look more or less like the
> avs-driver series instead of providing a parallel driver.
> 
> But the reality shows that such approach applies too much pressure on
> the reviewers and leaves no fallback option for the end users if
> anything fails along the way.

I will stop commenting here to let others chime in, I don't have the
background to provide useful technical feedback on this complicated
HDaudio/ASoC interface.

I am however concerned about the lack of long-term plans and confusion
coming having 3 different ways of dealing with HDaudio codecs on the
same hardware platform (legacy, ASoC/SOF-Skylake, ASoC-AVS). 2 was
already bad, I don't see how 3 is better?