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?
