[alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx
Add additional check in hdmi_find_pcm_slot() to not return a pcm index that points to unallocated pcm. This could happen if codec driver is set up in codec->mst_no_extra_pcms mode. On some platforms, this leads to a kernel oops in snd_ctl_notify(), called via update_eld().
BugLink: https://github.com/thesofproject/linux/issues/1536 Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 373ca99b7a2f..c3940c197122 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1335,24 +1335,24 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, int i;
/* - * generic_hdmi_build_pcms() allocates (num_nids + dev_num - 1) - * number of pcms. + * generic_hdmi_build_pcms() may allocate extra PCMs on some + * platforms (with maximum of 'num_nids + dev_num - 1') * * The per_pin of pin_nid_idx=n and dev_id=m prefers to get pcm-n * if m==0. This guarantees that dynamic pcm assignments are compatible - * with the legacy static per_pin-pmc assignment that existed in the + * with the legacy static per_pin-pcm assignment that existed in the * days before DP-MST. * * per_pin of m!=0 prefers to get pcm=(num_nids + (m - 1)). */ - if (per_pin->dev_id == 0 && - !test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap)) - return per_pin->pin_nid_idx; - - if (per_pin->dev_id != 0 && - !(test_bit(spec->num_nids + (per_pin->dev_id - 1), - &spec->pcm_bitmap))) { - return spec->num_nids + (per_pin->dev_id - 1); + + if (per_pin->dev_id == 0) { + if (!test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap)) + return per_pin->pin_nid_idx; + } else { + i = spec->num_nids + (per_pin->dev_id - 1); + if (i < spec->pcm_used && !(test_bit(i, &spec->pcm_bitmap))) + return i; }
/* have a second try; check the area over num_nids */
Commit 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs") introduced a slight change of behaviour how non-MST monitors are assigned to PCMs on Intel platforms.
In the drm_audio_component.h interface, the third parameter to pin_eld_notify() is pipe number. On Intel platforms, this value is -1 for MST. On other platforms, a non-zero pipe id is used to signal MST use.
This difference leads to some subtle differences in hdmi_find_pcm_slot() with regards to how non-MST monitors are assigned to PCMs. This patch restores the original behaviour on Intel platforms while keeping the new allocation policy on other platforms.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/pci/hda/patch_hdmi.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index c3940c197122..1dd4c92254a4 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1353,6 +1353,11 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, i = spec->num_nids + (per_pin->dev_id - 1); if (i < spec->pcm_used && !(test_bit(i, &spec->pcm_bitmap))) return i; + + /* keep legacy assignment for dev_id>0 on Intel platforms */ + if (spec->intel_hsw_fixup) + if (!test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap)) + return per_pin->pin_nid_idx; }
/* have a second try; check the area over num_nids */
On Fri, 29 Nov 2019 15:37:56 +0100, Kai Vehmanen wrote:
Commit 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs") introduced a slight change of behaviour how non-MST monitors are assigned to PCMs on Intel platforms.
In the drm_audio_component.h interface, the third parameter to pin_eld_notify() is pipe number. On Intel platforms, this value is -1 for MST. On other platforms, a non-zero pipe id is used to signal MST use.
This difference leads to some subtle differences in hdmi_find_pcm_slot() with regards to how non-MST monitors are assigned to PCMs. This patch restores the original behaviour on Intel platforms while keeping the new allocation policy on other platforms.
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
I believe this is the right fix. I thought of a similar possibility, but didn't take it seriously whether it really matters.
So applied it now for the next 5.5-rc1 pull request.
thanks,
Takashi
Hi Takashi, Nikil and others,
On Fri, 29 Nov 2019, Kai Vehmanen wrote:
This difference leads to some subtle differences in hdmi_find_pcm_slot() with regards to how non-MST monitors are assigned to PCMs. This patch restores the original behaviour on Intel platforms while keeping the new allocation policy on other platforms.
hmm, there seems a couple of more issues. The first patch is a clear bug that leads to segfault with SOF+patch_hdmi on some platforms (pipe B used for single monitor HDMI case -> dev_id=1 -> non-existant pcm selected and eventual kernel oops).
This second patch is however trickier. Nikhil your patch changed the default allocation a bit, so the routing might be difference also with snd-hda-intel (i.e. not SOF) for existing platforms and this may surprise users.
Digging deeper, we seem to have a slight semantics difference in how intel_pin_eld_notify() and generic_acomp_pin_eld_notify() handle the third pipe/dev_id parameter.
Any thoughts how to solve? I first I thought making separate functions for hdmi_find_pcm_slot() and allow platforms to define an alternative implementation, but in this RFC patch I opted for a simpler quirk in the function. This is becoming fairly messy I must say -- the amount of code commentary needed is a good indication some simplifaction would be in order.
PS I did not have time to fully test the RFC patch, so this is just for discussion now...
Br, Kai
On Fri, 29 Nov 2019 15:47:11 +0100, Kai Vehmanen wrote:
Hi Takashi, Nikil and others,
On Fri, 29 Nov 2019, Kai Vehmanen wrote:
This difference leads to some subtle differences in hdmi_find_pcm_slot() with regards to how non-MST monitors are assigned to PCMs. This patch restores the original behaviour on Intel platforms while keeping the new allocation policy on other platforms.
hmm, there seems a couple of more issues. The first patch is a clear bug that leads to segfault with SOF+patch_hdmi on some platforms (pipe B used for single monitor HDMI case -> dev_id=1 -> non-existant pcm selected and eventual kernel oops).
This second patch is however trickier. Nikhil your patch changed the default allocation a bit, so the routing might be difference also with snd-hda-intel (i.e. not SOF) for existing platforms and this may surprise users.
Well, but the allocation itself is dynamic for DP-MST, even on Intel, so user can't expect the completely persistent index assignment. That's the reason I took Nikhil's patch (even I suggested to simplify in that way).
We had a trick to assign the primary index. It still works, right? That should be the only concern.
Digging deeper, we seem to have a slight semantics difference in how intel_pin_eld_notify() and generic_acomp_pin_eld_notify() handle the third pipe/dev_id parameter.
This is a platform-specific part, and on Intel, the assumption has been that pipe is equivalent with dev_id. If this changed, of course, we must reconsider the whole picture.
For generic_acomp_pin_eld_notify(), it's gfx-driver specific, too. And currently dev_id = -1 in AMDGPU, so we don't think too much about the behavior compatibility.
Any thoughts how to solve? I first I thought making separate functions for hdmi_find_pcm_slot() and allow platforms to define an alternative implementation, but in this RFC patch I opted for a simpler quirk in the function. This is becoming fairly messy I must say -- the amount of code commentary needed is a good indication some simplifaction would be in order.
Yeah, that's a bit messy. The only expectation is the primary slot assignment -- i.e. the case only one monitor is connected. As long as this behavior is kept, I don't think any big problem with the dynamic assignment.
PS I did not have time to fully test the RFC patch, so this is just for discussion now...
Since the assignment should work with your patch somehow, I already applied it. Let's do fine tune-up during 5.5 rc cycles, if any.
thanks,
Takashi
Hey,
On Fri, 29 Nov 2019, Takashi Iwai wrote:
On Fri, 29 Nov 2019 15:47:11 +0100, Kai Vehmanen wrote:
This second patch is however trickier. Nikhil your patch changed the default allocation a bit, so the routing might be difference also with snd-hda-intel (i.e. not SOF) for existing platforms and this may surprise users.
Well, but the allocation itself is dynamic for DP-MST, even on Intel, so user can't expect the completely persistent index assignment. That's the reason I took Nikhil's patch (even I suggested to simplify in that way).
[...]
We had a trick to assign the primary index. It still works, right? That should be the only concern.
[...]
This is a platform-specific part, and on Intel, the assumption has been that pipe is equivalent with dev_id. If this changed, of course, we must reconsider the whole picture.
hmm, the pipe equivalency should actually still hold. Looking at the code more, this could also be a lurking bug on graphics driver that had new side-effects with the recent ALSA side changes. E.g. I've received logs where dev_id=1 is for a single connected HDMI monitor. I need to investigate more whether this is an expected behaviour or a bug. :)
PS I did not have time to fully test the RFC patch, so this is just for discussion now...
Since the assignment should work with your patch somehow, I already applied it. Let's do fine tune-up during 5.5 rc cycles, if any.
Ack, ok. My commit message is a bit confusing (the wording about MST is not correct) but the actual code restores original behaviour so this should be good to apply. I'll continue testing and also dig a bit deeper into the bugreports w.r.t. what happens in the problematic non-MST cases. Thanks for the quick reviews!
Br, Kai
Thanks Kai, see inline -
On 11/29/19 8:07 PM, Kai Vehmanen wrote:
Commit 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs") introduced a slight change of behaviour how non-MST monitors are assigned to PCMs on Intel platforms.
In the drm_audio_component.h interface, the third parameter to pin_eld_notify() is pipe number. On Intel platforms, this value is -1 for MST. On other platforms, a non-zero pipe id is used to signal MST use.
Do you mean "on Intel platforms, this value is -1 for non-MST"?
I am looking into functions intel_audio_codec_enable/intel_audio_codec_disable, they sets pipe = -1 for non-MST cases, right?
This difference leads to some subtle differences in hdmi_find_pcm_slot() with regards to how non-MST monitors are assigned to PCMs. This patch restores the original behaviour on Intel platforms while keeping the new allocation policy on other platforms.
What exact change commit 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs") made in behaviour on Intel platform? Sorry, it is not clear to me from your reply on this thread.
For non-MST monitors, pipe = -1 is getting passed to intel_pin_eld_notify(). check_presence_and_report -> pin_id_to_pin_index changes value of dev_id from -1 to 0, comment there says "(dev_id == -1) means it is NON-MST pin return the first virtual pin on this port". If this is the case, non-MST monitor should get PCM of index 'pin_nid_idx' like it was happening before commit 5398e94fb753. Isn't it?
Thanks, Nikhil Mahale
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
sound/pci/hda/patch_hdmi.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index c3940c197122..1dd4c92254a4 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1353,6 +1353,11 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, i = spec->num_nids + (per_pin->dev_id - 1); if (i < spec->pcm_used && !(test_bit(i, &spec->pcm_bitmap))) return i;
/* keep legacy assignment for dev_id>0 on Intel platforms */if (spec->intel_hsw_fixup)if (!test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap))return per_pin->pin_nid_idx;}
/* have a second try; check the area over num_nids */
Hey,
On Mon, 2 Dec 2019, Nikhil Mahale wrote:
On 11/29/19 8:07 PM, Kai Vehmanen wrote:
In the drm_audio_component.h interface, the third parameter to pin_eld_notify() is pipe number. On Intel platforms, this value is -1 for MST. On other platforms, a non-zero pipe id is used to
Do you mean "on Intel platforms, this value is -1 for non-MST"?
[...]
I am looking into functions intel_audio_codec_enable/intel_audio_codec_disable, they sets pipe = -1 for non-MST cases, right?
yup, -1 for non-MST, that was just plain wrong in my Friday mail. The problem seems to be pipe id is positive in some non-MST cases (like https://github.com/thesofproject/linux/issues/1536 ), which is very suprising looking at e.g. intel_audio_codec_enable(), but that seems to be happening nevertheless.
This would seem like a lurking bug on the i915 graphics driver side. Your patch changed the behaviour in these cases on ALSA side (PCM was selected based on per_pin->pin_nid_idx, ignoring dev_id), but one can argue whether this is worth preserving (or just drop/revert the RFC patch). I'll try to get a bit more info on the rootcause and how common case this is.
Br, Kai
Hi Takashi and Nikhil,
On Mon, 2 Dec 2019, Kai Vehmanen wrote:
yup, -1 for non-MST, that was just plain wrong in my Friday mail. The problem seems to be pipe id is positive in some non-MST cases (like https://github.com/thesofproject/linux/issues/1536 ), which is very suprising looking at e.g. intel_audio_codec_enable(), but that seems to be happening nevertheless.
ok, got graphics driver log from this case now, and this turns out to be a system with an unusual converter setup for HDMI output. I.e. from user point of view it's HDMI, but for graphics driver it looks like a DP connection (with MST enabled which is not so common but apparently is the case). So i915 driver is working as expected and in patch_hdmi we should handle it as a normal MST case.
Takashi, considering the commit message is wrong, I think it's better to revert the "ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms" patch. Should I send a revert?
It also doesn't fully preseve the old routing as in the case of a single DP-MST monitor (with dev_id=1), hdmi_find_pcm_slot() will not pick the preferred PCM "per_pin->pin_nid_idx", but will choose "spec->num_nids + (per_pin->dev_id - 1)". So seems better to just drop it.
Br, Kai
On Tue, 03 Dec 2019 14:48:10 +0100, Kai Vehmanen wrote:
Hi Takashi and Nikhil,
On Mon, 2 Dec 2019, Kai Vehmanen wrote:
yup, -1 for non-MST, that was just plain wrong in my Friday mail. The problem seems to be pipe id is positive in some non-MST cases (like https://github.com/thesofproject/linux/issues/1536 ), which is very suprising looking at e.g. intel_audio_codec_enable(), but that seems to be happening nevertheless.
ok, got graphics driver log from this case now, and this turns out to be a system with an unusual converter setup for HDMI output. I.e. from user point of view it's HDMI, but for graphics driver it looks like a DP connection (with MST enabled which is not so common but apparently is the case). So i915 driver is working as expected and in patch_hdmi we should handle it as a normal MST case.
Takashi, considering the commit message is wrong, I think it's better to revert the "ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms" patch. Should I send a revert?
It also doesn't fully preseve the old routing as in the case of a single DP-MST monitor (with dev_id=1), hdmi_find_pcm_slot() will not pick the preferred PCM "per_pin->pin_nid_idx", but will choose "spec->num_nids + (per_pin->dev_id - 1)". So seems better to just drop it.
Well, if we want to keep the old behavior for compatibility just to be sure, how about a patch like below?
Takashi
--- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1348,21 +1348,18 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, * with the legacy static per_pin-pcm assignment that existed in the * days before DP-MST. * + * Intel DP-MST prefers this legacy behavior for compatibility, too. + * * per_pin of m!=0 prefers to get pcm=(num_nids + (m - 1)). */
- if (per_pin->dev_id == 0) { + if (per_pin->dev_id == 0 || spec->intel_hsw_fixup) { if (!test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap)) return per_pin->pin_nid_idx; } else { i = spec->num_nids + (per_pin->dev_id - 1); if (i < spec->pcm_used && !(test_bit(i, &spec->pcm_bitmap))) return i; - - /* keep legacy assignment for dev_id>0 on Intel platforms */ - if (spec->intel_hsw_fixup) - if (!test_bit(per_pin->pin_nid_idx, &spec->pcm_bitmap)) - return per_pin->pin_nid_idx; }
/* have a second try; check the area over num_nids */
Hey,
On Tue, 3 Dec 2019, Takashi Iwai wrote:
Well, if we want to keep the old behavior for compatibility just to be sure, how about a patch like below?
[...]
- if (per_pin->dev_id == 0) {
- if (per_pin->dev_id == 0 || spec->intel_hsw_fixup) {
that would work. spec->intel_hsw_fixup starts to be a bit overloaded, but better than branching off the whole pcm selection, so ok for me.
Br, Kai
On Fri, 29 Nov 2019 15:37:55 +0100, Kai Vehmanen wrote:
Add additional check in hdmi_find_pcm_slot() to not return a pcm index that points to unallocated pcm. This could happen if codec driver is set up in codec->mst_no_extra_pcms mode. On some platforms, this leads to a kernel oops in snd_ctl_notify(), called via update_eld().
BugLink: https://github.com/thesofproject/linux/issues/1536 Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
Applied, thanks.
Takashi
Oh sorry again for this regression, Kai.
Originally my patches were developed with slightly older code which doesn't have your commit 2a2edfbbfee4 (ALSA: hda/hdmi - implement mst_no_extra_pcms flag), when I merge them with tot I did not notice codec->mst_no_extra_pcms mode.
This patch looks good to me.
Thanks, Nikhil Mahale
On 11/29/19 8:13 PM, Takashi Iwai wrote:
On Fri, 29 Nov 2019 15:37:55 +0100, Kai Vehmanen wrote:
Add additional check in hdmi_find_pcm_slot() to not return a pcm index that points to unallocated pcm. This could happen if codec driver is set up in codec->mst_no_extra_pcms mode. On some platforms, this leads to a kernel oops in snd_ctl_notify(), called via update_eld().
BugLink: https://github.com/thesofproject/linux/issues/1536 Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com
Applied, thanks.
Takashi
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
participants (3)
-
Kai Vehmanen -
Nikhil Mahale -
Takashi Iwai