New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASoC: SOF: Intel: hda: allow operation without i915 #1731
ASoC: SOF: Intel: hda: allow operation without i915 #1731
Conversation
Draft only, I didn't manage to clean up fully yet, but this looks very promising. We can extend the HDA generic driver to these cases without needing a separate topology. Plus a big bonus, transient faults with i915 hw (like the recent afg problems), won't bring down the whole SOF stack, but only the HDMI part. |
skl_hda_be_dai_links[i].codecs->name = | ||
"snd-soc-dummy"; | ||
skl_hda_be_dai_links[i].codecs->dai_name = | ||
"snd-soc-dummy-dai"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, skl_hda_be_dai_links[i].codecs
is actually an array of skl_hda_be_dai_links[i].num_codecs
elements. So, the above actually means skl_hda_be_dai_links[i].codecs[0]
and it assumes that .num_codecs
is at least 1. Maybe indeed this is a valid assumption, but it would be better to use an array representation instead of just a pointer and maybe add a comment, saying that array is guaranteed to have at least one element in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh Ack. I changed this for the "production version" and used the common macros instead.
a2246bb
to
83a99d4
Compare
Ready for review folks! I tested all the error scenarios with specially crafted error injection code. |
if (!hda->i915_codec_enabled) { | ||
dev_dbg(sdev->dev, | ||
"iDisp hw present but no driver, ignoring\n"); | ||
return -ENOENT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I wondered about is should the device be unregistered here immediately in the error case. But I ended up not doing that. snd_hdac_ext_bus_device_remove() will clean up this device as well.
There's one checkpatch warning (shows up as a failure from Travis).
... but this is matching with existing usage of SND_SOC_DAILINK_DEF in ASoC, so kept it like this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments below
sound/soc/sof/intel/hda-codec.c
Outdated
if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) { | ||
if (!hda->i915_codec_enabled) { | ||
dev_dbg(sdev->dev, | ||
"iDisp hw present but no driver, ignoring\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's more 'will be ignored'. There is nothing in this function that ignores the codec, you just return a negative value that will be handled in hda_codec_probe_bus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart hmm, I'll change this. but we do skip the codec driver probe later in the function, so first part of "ignoring" is already done in this function. but yeah, the info needs to be passed to controller side as well and that is done in the parent, so point taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart "will be ignored" didn't fit 80-chars, so I left ", ignoring" part out.. not adding value
83a99d4
to
a6da130
Compare
v3 uploaded: addressed Pierre's feedback to code comments, no other changes compared to v2 |
@@ -135,11 +138,20 @@ static int skl_hda_fill_card_info(struct snd_soc_acpi_mach_params *mach_params) | |||
skl_hda_be_dai_links[IDISP_DAI_COUNT + | |||
HDAC_DAI_COUNT + i]; | |||
} | |||
} else if (codec_count == 2 && codec_mask & IDISP_CODEC_MASK) { | |||
} else if (codec_count <= 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor - why do we <= 2 codecs important here ?
@@ -987,7 +989,7 @@ static int hda_generic_machine_select(struct snd_sof_dev *sdev) | |||
dev_info(bus->dev, "using HDA machine driver %s now\n", | |||
hda_mach->drv_name); | |||
|
|||
if (codec_num == 1) | |||
if (codec_num == 1 && HDA_IDISP_CODEC(bus->codec_mask)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor again, not following here why codec count is important. previous author should have commented this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change suggested so that the cases we support in the machine driver are crystal-clear
Thanks @kv2019i
I will let #1764 go in first and I will then rebase this one. |
I've addressed the comments in my local branch, but will wait and not upload until #1767 is merged via upstream (there's a conflict). |
One remaining issues with SOF module load/unload with this patchset and latest sof-dev, will debug this today. |
a6da130
to
378af3e
Compare
@lgirdwood @plbossart Now updated to address your comments. |
378af3e
to
6574cb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something's fishy with the mask/count code?
|
||
if (codec_count == 1 && codec_mask & IDISP_CODEC_MASK) { | ||
if (codec_count == idisp_count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm, confused...
idisp_count is either 0 or 0x4.
did you mean idisp_count = hweight_long(codec_mask & IDISP_CODEC_MASK) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart Gah :(, thanks Pierre for catching this. I also messed up in testing as I thought I had covered this case, but I didn't as this is obviously wrong. Going back, the checks still looked too complicated as well. I now updated the 3rd try with s/disp_count/idisp_mask/ and added more comments to further clarify that we are handling two separate cases of topologies here. Hopefully now clear. Tested scenarios on CFL as well.
Extend the generic HDA driver to support systems where iDisp/HDMI audio codecs are disabled for some reason. Switch codecs to SoC dummy in the affected DAI links. This allows to reuse existing topologies for this case. BugLink: thesofproject#1658 BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1163677 BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206085 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Add support to configure the HDA controller with an external HDA codec even if iDisp codec in i915 is not available. This can happen for multiple reasons: - internal graphics is disabled on the system - i915 driver is not enabled in kernel or it fails to init - i915 codec reports error in HDA codec probe - HDA codec driver probe fails Address all these scenarios, but keep using the existing topology. In case failures occur, HDMI PCM nodes are created, but they will report error if application tries to use them. No ALSA mixer controls are created. If the external HDA codec init fails as well, SOF probe will return error as before. BugLink: thesofproject#1658 BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1163677 BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=206085 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
6574cb7
to
ba0aba7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kv2019i
Should
a) merge this
b) send upstream directly
c) merge upstream stuff then resolve conflicts then send upstream?
@plbossart wrote:
Let's take (c). I'll add your reviewed-by tags and send directly. This patch is not critical to have in sof-dev (but is critical for people with affected hw) and I don't have any follow-up patches to these parts of the code, so might as well sent to the list. |
Sent to upstream, let's merge this via upstream. Keeping open until merged upstream. |
Merged upstream and in sof-dev as well (via rebase). |
Add support to configure HDA controller with external codec even if iDisp codec in i915 is not available.
This can happen for multiple reasons:
- internal graphics is disabled on the system
- i915 driver is not enabled in kernel or it fails to init
- i915 codec reports error in HDA codec probe
- HDA codec driver probe fails
Address all these scenarios, but keep using the same topology configuration still. In case failures occur, HDMI PCM nodes are created, but they will report error at open. No ALSA mixer controls are created.
Fixes: #1658
Note to @plbossart : this will conflict with "ASoC: SOF: Intel: refactor i915_get/put functions" that was sent to upstream last week. In case merging get delayed, you'll see this.