ASoC: SOF: Intel: hda: remove SND_SOC_SOF_HDA_COMMON_HDMI_CODEC - #1780
Conversation
|
Discussing #1777 with @lyakh today, I had to accept that my decision to add the Kconfig option for common-hdmi was not a good one in retrospective. Basicly we have less control on how the kconfig options get chosen and if some distro ends up with COMMON_HDMI_CODEC=n in their defconfigs (and by oversight, i.e. they specifically do not want it due to some UCM dependencies), it will create lots of work for us. We can't take that decision back, so I started looking for other options. Currently, Linux desktop side has pretty much moved to common codec:
So it seems desktop side is ok. For Chrome, there are ~10 boards that have UCM files for hdac-hdmi still, as can be seen from: @yongzhi1 @sathya-nujella @sathya-nujella Do you think it's ok to move ahead still? For Chrome, the main risk is if you want to use an upstream kernel with an older board. To keep the UCM files functional, you would then have to pass "use_common_hdmi" kernel parameter, or disable SND_HDA_CODEC_HDMI in the kernel build. For newer boards (anything newer than ICL), this is not an issue as only snd-hda-codec-hdmi supports the hw. So all in all, it seems we ready to take this step, please review. |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
I'm still not quite clear about this. My first question is: what would happen if a generic distro boots on hardware, that doesn't support common HDMI? HDMI won't work unless the user does their research and sets the module parameter, right? Would it not be possible to move ACPI parsing before HDMI initialisation, let the machine driver set a flag in machine driver, if it cannot support common HDMI and then have the HDMI initialisation code check that flag? Yes, I understand that this would be a larger change, so, it depends how much we care about that legacy hardware. Or maybe it would be possible to port all machine drivers to the common HDMI driver?
| IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI) | ||
|
|
||
| if (hda_codec_use_common_hdmi) | ||
| return false; |
There was a problem hiding this comment.
let me try to understand: if Kconfig is right for this code to enable, it would mean: if hda_codec_use_common_hdmi is true, we return false. If it's false, we fall through and return its value below, which is false. So, we return false in both cases. I think it would be much clearer to write this
#if !IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) && \
IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
return false;
#else
return hda_codec_use_common_hdmi;
#endif
What if none of those CONFIG_* options is enabled? Don't we have to return false too? Even if it doesn't make any difference, that seems a more logical definition to me - if CONFIG_SND_HDA_CODEC_HDMI isn't enabled, return false. So, maybe you actually can just do return IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI) && hda_codec_use_common_hdmi;
There was a problem hiding this comment.
@lyakh Ack, agreed, returning false right away is better.
I do want to return true in the case neither is selected. The common hda driver path does an explicit driver probe&attach and we can report errors immediately in hda_codec_probe(). And more importantly, we can modify the codec_mask and disable HDMI and continue SOF load. With hdac-hdmi, we find the missing codec much later and then we just fail in topology load and user is left without sound. So basicly this should return false only and only if user really, really wants to use hdac-hdmi.
There was a problem hiding this comment.
@kv2019i mach_params->common_hdmi_codec_drv i set to hda_codec_use_common_hdmi, should this be set to hda_use_common_hdmi() instead?
There was a problem hiding this comment.
@ranj063 Thanks, good catch. I had overlooked that, needs to be changed as well.
|
@lyakh wrote:
The common (snd-hda-codec-hdmi) driver is much older driver and it supports all hardware hdac-hdmi does. It actually supports much more hardware (both old and new), so this can't really happen. |
@kv2019i then why do we keep hdac_hdmi at all? |
|
@kv2019i there are multiple PR's on this topic and its all too confusing for me. So can I try and summarize the idea here? There are 2 kconfig options and 1 module param under scrutiny: CONFIG_SND_HDA_CODEC_HDMI, CONFIG_SND_SOC_HDAC_HDMI and use_common_hdmi. So, looking into the possibilities:
|
|
@lyakh wrote:
Because the user-space interface is different and we shipped SOF with linux-5.4 with hdac-hdmi as the only option. Any users (e.g. GLK Chromebooks) that have user-space built on top of hdac-hdmi, will rely on us to provide hdac-hdmi support in order for them to upgrade to a new kernel. I checked the Chrome tree just today and there are still UCM files that are tried to SOF and hdac-hdmi. But yeah, this should be the only reason. |
|
@ranj063 wrote:
Sure thing. The other PRs are not directly related, so you can consider this as a standalone change to consider.
Correct.
Then hdac-hdmi is used. Really only reason to use this is if your user-space is dependent on hdac-hdmi and you want to use a newer kernel. E.g. Chrome git master (see links above) still has hdac-hdmi based UCM files. Requiring user-space and kernel to be updated in sync, is not a requirement distros appreciate. In Chrome this is somewhat more acceptable as full images are built (versus deb/rpm type of package distribution in Linux distros), but still causes some additional work and constraints. And I'm not proposing to remove this. Just limit this to kernel module parameter only.
Correct.
The SOF card probe will fail as the codec entries in iDisp ASoC topology, will not exist, and card registration will fail. |
@kv2019i ok, I see now, thanks! It seems to me then, that we want to discourage basing new user-space builds on top of hdac_hdmi as much as we can. And the only existing users are Chrome machines. Do Chrome builds have different kernel configurations for each hardware type? In such a case we could even remove the module parameter and make it a really scary kernel configuration option like "legacy-deprecated-yes-I-really-know-what-I-am-doing" (not literally, but in that direction). But if they're already relying on that parameter, then we probably cannot remove it for now. Then yes, making it unconditionally "yes" on default seems good. |
To help user-space with HDMI codec driver transition, both a kernel module parameter and a kernel option were initially provided to configure default behaviour of SOF on Intel hardware with commit 139c7fe ("ASoC: SOF: Intel: add support for snd-hda-codec-hdmi"). As hdac-hdmi is already now lagging in features compared to snd-hda-codec-hdmi, move ahead with the transition and remove the build option to select between the two, and instead default to snd-hda-codec-hdmi if it is enabled in kernel build. The old behaviour of using hdac-hdmi driver can still be forced via the kernel module parameter. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
7797e42 to
90f9b7e
Compare
|
@lyakh @ranj063 Thanks for the reviews! The intention is to reduce confusion, so your input is really helpful. I further simplified the patch now and removed the helper function. Now basicly we just remove the option to change the parameter default (but also remove the possibility to change it by accident like happened for @lyakh and we have seen in some bugs like thesofproject/sof#2265 (comment) https://github.com/thesofproject/sof/files/4049160/default.txt ). So let's keep it as simple as possible. I also improved the commit message to clearly mention that snd-hda-codec-hdmi has more features, so it should be used by default. I would like to get an ack from Chrome folks before we merge this (ping @yongzhi1 @sathya-nujella ). |
|
Hi, @kv2019i , We have and also will have Chrome products using DSP and not using DSP (legacy HDA, without using SOF). For legacy HDA usage (without using SOF), if this hda_codec_use_common_hdmi is set.. will there be any impact? |
|
@sathya-nujella wrote:
No, no impact. This setting only affects SOF, so no impact to legacy HDA (snd-hda-intel) driver or to SST. |
ok, thank you for that confirmation @kv2019i . |
|
@lyakh waiting for your updated review, thanks |
|
@kv2019i I discover |
5 participants
To help user-space with HDMI codec driver transition, both
a kernel module parameter and a kernel option were initially
provided to configure default behaviour of SOF on Intel hardware
with commit 139c7fe ("ASoC: SOF: Intel: add support for
snd-hda-codec-hdmi").
As the build option has created some new problems, move
forward with the transition by removing the build option
altogether. The old behaviour of using hdac-hdmi driver can
still be forced via the kernel module parameter, and it is
also used if snd-hda-codec-hdmi is not enabled in kernel build.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com