ASoC: Intel: sof_pcm512x: make HDMI optional for all platforms - #1777
Conversation
|
|
||
| static int sof_audio_probe(struct platform_device *pdev) | ||
| { | ||
| struct snd_soc_acpi_mach *mach = (&pdev->dev)->platform_data; |
| if (!mach->mach_params.common_hdmi_codec_drv) { | ||
| dev_err(&pdev->dev, "HDMI codec driver error\n"); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
what if you build a kernel with hdac-hdmi disabled, then you shouldn't need to specify use_common_hdmi (not sure it would work in fact, have to check). But this way you fail?
There was a problem hiding this comment.
@lyakh that's perfectly ok. Use_common_hdmi is always set if we try to load snd-hda-codec-hdmi driver from SOF. There is no relation to hdac-hdmi, so whether it is enabled/disabled has no impact. Only thing that matters is thet sof_pcm512 only works with snd-hda-codec-hdmi and if you disable loading it, the probe will fail.
I don't think we can get rid of use_common_hdmi any time soon.
There was a problem hiding this comment.
@kv2019i you mean if you disable SND_SOC_HDAC_HDMI and enable CONFIG_SND_HDA_CODEC_HDMI this still isn't sufficient to use the common HDMI driver? We still have to specify use_common_hdmi? This doesn't seem very logical to me - if out of two alternatives one isn't available, it seems we should be trying the other one anyway, regardless whether it's explicitly requested or not?
There was a problem hiding this comment.
@lyakh By default things go ok. If you want to select sof_pcm512, CONFIG_SND_HDA_CODEC_HDMI is selected as well (via Kconfig rule) and then the default value of USE_COMMON_HDMI will be yes as well (via Kconfig rule), so the right thing will happen.
But you can break this is if you explictly pass an option use_common=hdmi=0 to the kernel driver. SOF code doesn't know what the machine driver supports, so it can't block this.
It's true we could have an additional check that if SND_SOC_HDAC_HDMI is not even built, then we can just set use_common_hdmi to 1. That is not done currently and could be easily added. Or maybe try to improve the loader code more in hda.c.
There was a problem hiding this comment.
...also: do we really want to fail probing completely in such a case or rather just disable HDMI / set hdmi_num = 0?
There was a problem hiding this comment.
@kv2019i just wondering is there a way to make the module param to be mutually incompatible with the HDA_COMMON_HDMI_CODEC kconfig selection? If we allow setting use_common_hdmi to 0, then we should make it work instead of reporting an error here, no?
There was a problem hiding this comment.
@kv2019i @ranj063 I don't think playing with SND_SOC_HDAC_HDMI can help. Think about distro kernels. They'll have support for all platforms, so, both those HDA HDMI codec options will be selected. And that same kernel will have to run on different platforms - on some it will use the common HDMI driver, on others it will not. So, this has to be decided at run-time. Currently you do that with a kernel parameter. Which is also suboptimal, because how would an average user know, that they have to set that? Unfortunately with the current control flow this cannot be (easily) done from the machine initialisation - ACPI parsing takes place after HDMI initialisation. But I think we really want that - an automatic decision making. I think the proper solution would be postponing the HDMI initialisation after ACPI parsing. Are there any principal objections against that?
There was a problem hiding this comment.
@lyakh @ranj063 The whole thing was specifically added for distro kernels, and to support runtime selection from user-space. The kernel will typically have both drivers built-in. As hdac-hdmi registers directly to HDA core, we don't really know until fairly late whether any driver was found, so changing the ACPI parsing timing won't solve this completely. So drivers are typically both there, but selection of HDMI codec driver depends on which UCM config files are installed. Kernell cannot know this. If user-space updates UCM files to common HDMI driver, then they can switch use_common_hdmi kernel module parameter at the same time. This allows to decouple kernel and user-space updates (so moving to new kernel is not blocked by missing some UCM file).
This is just a transition mechanism that will go away after a few kernel releases after all known users have changed. UCM2 helps in this transition, so once we get UCM2 get to major distros, then this doesn't matter as much anymore. We can just install UCM files that work with both and we can just default on common driver always. Chrome transition is also progressing well, but is not done.
The other cases are really very niche developer cases. I'm now working on the no-i915 patches (#1731 ) and after that, if a developer gets the Kconfig options wrong (e.g. forced hdac-hdmi but uses machine driver or platform that doesn't support it), probe will not fail anymore, but you just don't get HDMI functionality. This seems pretty reasonable, right?
There was a problem hiding this comment.
...also: do we really want to fail probing completely in such a case or rather just disable HDMI / set
hdmi_num = 0?
Ack, I'll fix this. You need to do a bit more work to disable the topology nodes, but why not.
68a1f0b to
1f1adbe
Compare
1f1adbe to
7500ff0
Compare
| if (list_empty(&ctx->hdmi_pcm_list)) | ||
| return -EINVAL; | ||
|
|
||
| if (!ctx->idisp_codec) |
There was a problem hiding this comment.
unrelated to this patch: the above "return -EINVAL;" can only be triggered if for some reason .init() wasn't called for any of the three iDisp interfaces. A comment would be nice
There was a problem hiding this comment.
@lyakh yeah, this is in all drivers and has been carried over. I think this would be triggered by an invalid topology file, but not 100% sure... would need some investigation to write a good comment.
|
|
||
| if (!ctx->idisp_codec) | ||
| return 0; | ||
|
|
There was a problem hiding this comment.
also unrelated: it isn't very obvious either why just taking the first member in the list is good. That means, that hda_dsp_hdmi_build_controls() can be called with any codec DAI component? Looks a bit confusing to me.
There was a problem hiding this comment.
@lyakh This is based on the assumption that all HDMI PCMs are handled by the same codec, so we look up the driver via the codec_dai->component of the first PCM. I guess a comment would not hurt.
kv2019i
changed the title
Make HDMI optional for APL and later platforms. If no HDMI codec is found on the HDA bus, the graphics side driver is missing or correct codec driver is not part of kernel build, codec_mask reflects this and HDMI is disabled. The DSP topology will still have the links for HDMI, so connect these to dummy codec to avoid failures in topology loading. This change also fixes a kernel oops that was triggered if sof_pcm512x was used with SOF configured to use hdac-hdmi (can be done via "use_common_hdmi=0" or by selecting CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC=n). This is not a supported configuration. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
7500ff0 to
7f48610
Compare
|
SOFCI TEST |
|
@plbossart there are some CI failures for APL nocodec and those are unrelated to this PR. Is it OK to merge? |
I ignored those issues when merging the upstream code, but we'd need to find out what's going on. |
I meant let's merge and recheck apl-nocodec manually. |
4 participants
If user explicitly selects hdac-hdmi as the SOF HDA HDMI codec
driver, either by passing "use_common_hdmi=0" or by selecting
CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC=n, this leads to
a kernel oops in hda_dsp_hdmi_build_control() with sof_pcm512x.
This machine driver only supports snd-hda-codec-hdmi, so
this is an invalid configuration.
Kernel oops happens as hda_dsp_hdmi_build_control() expects
component drvdata to be of type "struct hdac_hda_priv". This is
not the case if the codec driver is hdac-hdmi. To avoid
this problem, add an explicit check to sof_pcm512x probe.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com