alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx
@ 2019-11-29 14:37 Kai Vehmanen
  2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
  2019-11-29 14:43 ` [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Takashi Iwai
  0 siblings, 2 replies; 13+ messages in thread
From: Kai Vehmanen @ 2019-11-29 14:37 UTC (permalink / raw)
  To: alsa-devel, tiwai, Nikhil Mahale; +Cc: Kai Vehmanen

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 */
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 14:37 [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Kai Vehmanen
@ 2019-11-29 14:37 ` Kai Vehmanen
  2019-11-29 14:44   ` Takashi Iwai
                     ` (2 more replies)
  2019-11-29 14:43 ` [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Takashi Iwai
  1 sibling, 3 replies; 13+ messages in thread
From: Kai Vehmanen @ 2019-11-29 14:37 UTC (permalink / raw)
  To: alsa-devel, tiwai, Nikhil Mahale; +Cc: Kai Vehmanen

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 */
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx
  2019-11-29 14:37 [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Kai Vehmanen
  2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
@ 2019-11-29 14:43 ` Takashi Iwai
  2019-12-02  4:44   ` Nikhil Mahale
  1 sibling, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2019-11-29 14:43 UTC (permalink / raw)
  To: Kai Vehmanen; +Cc: alsa-devel, Nikhil Mahale

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
@ 2019-11-29 14:44   ` Takashi Iwai
  2019-11-29 14:47   ` Kai Vehmanen
  2019-12-02  5:07   ` Nikhil Mahale
  2 siblings, 0 replies; 13+ messages in thread
From: Takashi Iwai @ 2019-11-29 14:44 UTC (permalink / raw)
  To: Kai Vehmanen; +Cc: alsa-devel, Nikhil Mahale

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
  2019-11-29 14:44   ` Takashi Iwai
@ 2019-11-29 14:47   ` Kai Vehmanen
  2019-11-29 15:08     ` Takashi Iwai
  2019-12-02  5:07   ` Nikhil Mahale
  2 siblings, 1 reply; 13+ messages in thread
From: Kai Vehmanen @ 2019-11-29 14:47 UTC (permalink / raw)
  To: Kai Vehmanen; +Cc: tiwai, alsa-devel, Nikhil Mahale

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 14:47   ` Kai Vehmanen
@ 2019-11-29 15:08     ` Takashi Iwai
  2019-11-29 16:36       ` Kai Vehmanen
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2019-11-29 15:08 UTC (permalink / raw)
  To: Kai Vehmanen; +Cc: alsa-devel, Nikhil Mahale

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 15:08     ` Takashi Iwai
@ 2019-11-29 16:36       ` Kai Vehmanen
  0 siblings, 0 replies; 13+ messages in thread
From: Kai Vehmanen @ 2019-11-29 16:36 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Nikhil Mahale, Kai Vehmanen

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx
  2019-11-29 14:43 ` [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Takashi Iwai
@ 2019-12-02  4:44   ` Nikhil Mahale
  0 siblings, 0 replies; 13+ messages in thread
From: Nikhil Mahale @ 2019-12-02  4:44 UTC (permalink / raw)
  To: Takashi Iwai, Kai Vehmanen; +Cc: alsa-devel

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.
-----------------------------------------------------------------------------------
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
  2019-11-29 14:44   ` Takashi Iwai
  2019-11-29 14:47   ` Kai Vehmanen
@ 2019-12-02  5:07   ` Nikhil Mahale
  2019-12-02 11:18     ` Kai Vehmanen
  2 siblings, 1 reply; 13+ messages in thread
From: Nikhil Mahale @ 2019-12-02  5:07 UTC (permalink / raw)
  To: Kai Vehmanen, alsa-devel, tiwai

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 */
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-12-02  5:07   ` Nikhil Mahale
@ 2019-12-02 11:18     ` Kai Vehmanen
  2019-12-03 13:48       ` Kai Vehmanen
  0 siblings, 1 reply; 13+ messages in thread
From: Kai Vehmanen @ 2019-12-02 11:18 UTC (permalink / raw)
  To: Nikhil Mahale; +Cc: tiwai, alsa-devel, Kai Vehmanen

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-12-02 11:18     ` Kai Vehmanen
@ 2019-12-03 13:48       ` Kai Vehmanen
  2019-12-03 14:05         ` Takashi Iwai
  0 siblings, 1 reply; 13+ messages in thread
From: Kai Vehmanen @ 2019-12-03 13:48 UTC (permalink / raw)
  To: tiwai, Nikhil Mahale; +Cc: alsa-devel, Kai Vehmanen

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-12-03 13:48       ` Kai Vehmanen
@ 2019-12-03 14:05         ` Takashi Iwai
  2019-12-03 14:35           ` Kai Vehmanen
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2019-12-03 14:05 UTC (permalink / raw)
  To: Kai Vehmanen; +Cc: Nikhil Mahale, alsa-devel

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 */
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms
  2019-12-03 14:05         ` Takashi Iwai
@ 2019-12-03 14:35           ` Kai Vehmanen
  0 siblings, 0 replies; 13+ messages in thread
From: Kai Vehmanen @ 2019-12-03 14:35 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Nikhil Mahale, alsa-devel, Kai Vehmanen

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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-12-03 14:36 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 14:37 [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Kai Vehmanen
2019-11-29 14:37 ` [alsa-devel] [PATCH 2/2] [RFC] ALSA: hda: hdmi - preserve non-MST PCM routing for Intel platforms Kai Vehmanen
2019-11-29 14:44   ` Takashi Iwai
2019-11-29 14:47   ` Kai Vehmanen
2019-11-29 15:08     ` Takashi Iwai
2019-11-29 16:36       ` Kai Vehmanen
2019-12-02  5:07   ` Nikhil Mahale
2019-12-02 11:18     ` Kai Vehmanen
2019-12-03 13:48       ` Kai Vehmanen
2019-12-03 14:05         ` Takashi Iwai
2019-12-03 14:35           ` Kai Vehmanen
2019-11-29 14:43 ` [alsa-devel] [PATCH 1/2] ALSA: hda: hdmi - fix kernel oops caused by invalid PCM idx Takashi Iwai
2019-12-02  4:44   ` Nikhil Mahale

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).