Robustness improvements to HDA codec probe by kv2019i · Pull Request #1612 · thesofproject/linux

kv2019i · 2019-12-10T12:30:56Z

Here's a set of patches to expand usage of the generic HDA driver and make the SOF HDA stack handling of codec probe errors more robust/flexible.

Fixes: #1558
Fixes: #1367 (updated, I had wrong bug number here)

The main idea is that the generic HDA driver is the last resort and only tried in the case more specific machine driver is not found. With current code, if more codecs are found than what the generic driver can handle, probe error is returned and user is left with no audio. The patchset loosens the logic such that the generic driver is used to enable a subset of hardware, and only a warning is emitted. If user wants to enable the full hardware capabilities, a more specific machine driver needs to be written/enabled. It is expected these cases to be very rare.

plbossart

not fully on board @kv2019i, can you take a look at the comments below

plbossart · 2019-12-10T15:04:44Z

sound/soc/intel/boards/skl_hda_dsp_generic.c

 	if (!mach)
 		return -EINVAL;

+	hda_soc_card.dev = &pdev->dev;


is this not a different issue? or is this even required to move this here?

or for that matter required at all. devm_snd_soc_register_card() might actually handle this already?

is this not a different issue? or is this even required to move this here?

@plbossart This is a bit out-of-place, but I wanted to use dev_warn() in skl_hda_fill_card_info and easiest way to do this was to assign hda_soc_card.dev early.

sound/soc/sof/intel/hda-codec.c

plbossart · 2019-12-10T15:11:25Z

sound/soc/intel/boards/skl_hda_dsp_generic.c

+		dev_warn(card->dev, "Only max two codecs supported!\n");
+		codec_count = 2;
+	}
+


how would this work? You have 3 codecs, how would you select two out of three.

@kv2019i instead of manipulating the codec count, why not change the else if down below on line 143 to handle the case count >=2 instead of count == 2?

@plbossart Uhm, you have a point. I was assuming we enumerate the codecs in deterministic order, and we select the codec with lowest address bit first, so SOF PCM pipe is set up, the first HDA codec will be included in the pipeline. But, but, going through the code again, I'm not 100% sure the first codec is selected. Hmm, I'll see if I can guarantee this somehow. If not, I can reduce the patchset so that we only ignore probe failures.

Ok, I think this actually does work, I double-checked it now. We hardcode the bus address in machine drivers (the "ehdaudioXDy" names), so it is guaranteed we will set up the right codec -- we always enable addr=0 as the external HDA codec, no matter if other codecs are found.

But, but, I admit this is less important. I'll split it out for now and put the multi-codec case to the enhancement list for now.

ranj063 · 2019-12-10T17:09:16Z

sound/soc/sof/intel/hda.c

+		 * hda machine driver if at least the HDMI codec is present
 		 */
-		if (!pdata->machine && codec_num <= 2 &&
-		    HDA_IDISP_CODEC(bus->codec_mask)) {


@kv2019i will this case ever be true that codec_num will be > 2? In the previous patch, you'd have adjusted the codec_mask to drop the codec that failed to probe isnt it?

@ranj063 There are two different scenarios tackled. First is that we have a faulty external codec. In this case, we may go from iDisp+Ext=>IDisp_only, or from IDisp+ext+ext2=>iDisp+ext1.

Second case we have a system that has multiple working external HDA codecs attached on the bus. In this case the probes will succeed for all codecs, but we do not have a machine driver (and related infra) to tackle this configuration. What we can do is to still enable at least the first external HDA codec.

But, but, while responding to @plbossart previous comment, I realized guaranteeing we can pick configure the right codec, might be more complex than what I have here. If I can't solve that, then I have to revert to just address the first scenario (i.e. faulty codec probes).

Second case we have a system that has multiple working external HDA codecs attached on the bus. In this case the probes will succeed for all codecs, but we do not have a machine driver (and related infra) to tackle this configuration. What we can do is to still enable at least the first external HDA codec.

@kv2019i this would be a flawed strategy isnt it? If there is a working codec, we need to provide the right machine driver that manages it?

@ranj063 Well, the generic HDA driver is a fallback in any case. Yes we should have a more specific machine driver, but if we reach this point in code, search for that machine driver already failed. So the question is then that should we at least enable HDMI audio, and maybe a single HDA codec , which we do have machine driver, or leave the end user with no audio at all.

But yeah, not so clear cut. Currently I anyways have pieces missing to tackle this, so I need to do some more homework for this.

@kv2019i with my multi-client stuff, I do plan to separate out the HDMI codec into a separate client. With that change, you'd probably have the fallback with just the HDMI codecs working if they are probed successfully.

kv2019i · 2019-12-11T19:14:50Z

@plbossart @ranj063 Dropped patches 1+3 and modified PR to only address the case of a faulty/unresponsive codec. I added the "handle multiple codecs on the bus" as an enhancement to #990 and will return to this later. Patch 2 addresses the two open bugs and should be less controversial change to take in.

ranj063

LGTM

plbossart

the error case needs more work, no?

plbossart · 2019-12-11T20:47:48Z

sound/soc/sof/intel/hda-codec.c


+	if (!bus->codec_mask)
+		return -ENODEV;
+


/* create codec instances */
hda_codec_probe_bus(sdev, hda_codec_use_common_hdmi);

so that -ENODEV would have no impact...

@plbossart That matches the existing implementation, but you are right the errors (if passed) should be handled. I now updated the patch and made the probe function return void. I did also try a version that returns the error and passes it up from the caller, but we actually get more understandable error messages if we let this pass and get an error message from machine_check(). Who knows, maybe there's some case where DSP will be loaded despite no codecs found.

In case a HDA codec probe fails, do not raise error immediately, but instead remove the codec from bus->codec_mask and continue probe for other codecs. This allows for more robust behaviour in cases where one codec in the system is faulty. SOF driver load can still proceed with the codecs that can be probed successfully. Probe may still fail if suitable machine driver is not found, but in many cases the generic HDA machine driver can operate with a subset of codecs. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

kv2019i requested review from keyonjie, libinyang, lyakh, plbossart, ranj063 and xiulipan December 10, 2019 12:30

kv2019i requested a review from lgirdwood as a code owner December 10, 2019 12:30

kv2019i force-pushed the topic/hda-robust-codec-probe branch from 58f2640 to 9a7aca8 Compare December 10, 2019 12:35

plbossart reviewed Dec 10, 2019

View reviewed changes

ranj063 reviewed Dec 10, 2019

View reviewed changes

wenqingfu mentioned this pull request Dec 11, 2019

Enhance HDA support in SOF #990

Closed

15 tasks

kv2019i force-pushed the topic/hda-robust-codec-probe branch from 9a7aca8 to 36ff49b Compare December 11, 2019 19:13

kv2019i requested review from plbossart and ranj063 December 11, 2019 19:14

ranj063 previously approved these changes Dec 11, 2019

View reviewed changes

plbossart requested changes Dec 11, 2019

View reviewed changes

kv2019i dismissed ranj063’s stale review via be8a199 December 12, 2019 15:17

kv2019i force-pushed the topic/hda-robust-codec-probe branch from 36ff49b to be8a199 Compare December 12, 2019 15:17

kv2019i requested review from plbossart and ranj063 December 12, 2019 15:52

ranj063 approved these changes Dec 12, 2019

View reviewed changes

plbossart approved these changes Dec 12, 2019

View reviewed changes

plbossart merged commit 9c0401c into thesofproject:topic/sof-dev Dec 12, 2019

Conversation

kv2019i commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Dec 11, 2019

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kv2019i commented Dec 10, 2019 •

edited

Loading