[PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Hui Wang]

If the platform set the dyn_pcm_assign to true, it will call
hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is
connected and need to create a pcm.

So far only intel_hsw_common_init() and patch_nvhdmi() set the
dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot
dynamically first, if the driver runs for a period of time and there
is no regression reported, we could set no_fixed_assgin to true in
the intel_hsw_common_init(), and then set it to true in the
patch_nvhdmi().

This change comes from the discussion between Takashi and
Kai Vehmanen. Please refer to:
https://github.com/alsa-project/alsa-lib/pull/118

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 sound/pci/hda/patch_hdmi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index e405be7929e3..379a4d786b3b 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -157,6 +157,7 @@ struct hdmi_spec {
 
 	bool dyn_pin_out;
 	bool dyn_pcm_assign;
+	bool no_fixed_assign;
 	bool intel_hsw_fixup;	/* apply Intel platform-specific fixups */
 	/*
 	 * Non-generic VIA/NVIDIA specific
@@ -1345,6 +1346,12 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
 {
 	int i;
 
+	/* on the new machines, try to assign the pcm slot dynamically,
+	 * not use the preferred fixed map anymore.
+	 */
+	if (spec->no_fixed_assign)
+		goto last_try;
+
 	/*
 	 * generic_hdmi_build_pcms() may allocate extra PCMs on some
 	 * platforms (with maximum of 'num_nids + dev_num - 1')
@@ -1374,6 +1381,7 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
 			return i;
 	}
 
+ last_try:
 	/* the last try; check the empty slots in pins */
 	for (i = 0; i < spec->num_nids; i++) {
 		if (!test_bit(i, &spec->pcm_bitmap))
@@ -2937,6 +2945,9 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid,
 	spec->port_num = port_num;
 	spec->intel_hsw_fixup = true;
 
+	if (port_num > 6)
+		spec->no_fixed_assign = true;
+
 	intel_haswell_enable_all_pins(codec, true);
 	intel_haswell_fixup_enable_dp12(codec);
 
-- 
2.25.1

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Takashi Iwai]

On Tue, 23 Feb 2021 13:22:05 +0100,
Hui Wang wrote:
> 
> If the platform set the dyn_pcm_assign to true, it will call
> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is
> connected and need to create a pcm.
> 
> So far only intel_hsw_common_init() and patch_nvhdmi() set the
> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot
> dynamically first, if the driver runs for a period of time and there
> is no regression reported, we could set no_fixed_assgin to true in
> the intel_hsw_common_init(), and then set it to true in the
> patch_nvhdmi().
> 
> This change comes from the discussion between Takashi and
> Kai Vehmanen. Please refer to:
> https://github.com/alsa-project/alsa-lib/pull/118
> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

As this would "fix" actual use cases, I'd love to merge this for 5.12,
but of course it needs to be verified beforehand.

So this was actually tested in your side, right?


thanks,

Takashi

> ---
>  sound/pci/hda/patch_hdmi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index e405be7929e3..379a4d786b3b 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -157,6 +157,7 @@ struct hdmi_spec {
>  
>  	bool dyn_pin_out;
>  	bool dyn_pcm_assign;
> +	bool no_fixed_assign;
>  	bool intel_hsw_fixup;	/* apply Intel platform-specific fixups */
>  	/*
>  	 * Non-generic VIA/NVIDIA specific
> @@ -1345,6 +1346,12 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
>  {
>  	int i;
>  
> +	/* on the new machines, try to assign the pcm slot dynamically,
> +	 * not use the preferred fixed map anymore.
> +	 */
> +	if (spec->no_fixed_assign)
> +		goto last_try;
> +
>  	/*
>  	 * generic_hdmi_build_pcms() may allocate extra PCMs on some
>  	 * platforms (with maximum of 'num_nids + dev_num - 1')
> @@ -1374,6 +1381,7 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
>  			return i;
>  	}
>  
> + last_try:
>  	/* the last try; check the empty slots in pins */
>  	for (i = 0; i < spec->num_nids; i++) {
>  		if (!test_bit(i, &spec->pcm_bitmap))
> @@ -2937,6 +2945,9 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid,
>  	spec->port_num = port_num;
>  	spec->intel_hsw_fixup = true;
>  
> +	if (port_num > 6)
> +		spec->no_fixed_assign = true;
> +
>  	intel_haswell_enable_all_pins(codec, true);
>  	intel_haswell_fixup_enable_dp12(codec);
>  
> -- 
> 2.25.1
> 

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Hui Wang]

On 2/23/21 9:09 PM, Takashi Iwai wrote:
> On Tue, 23 Feb 2021 13:22:05 +0100,
> Hui Wang wrote:
>> If the platform set the dyn_pcm_assign to true, it will call
>> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is
>> connected and need to create a pcm.
>>
>> So far only intel_hsw_common_init() and patch_nvhdmi() set the
>> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot
>> dynamically first, if the driver runs for a period of time and there
>> is no regression reported, we could set no_fixed_assgin to true in
>> the intel_hsw_common_init(), and then set it to true in the
>> patch_nvhdmi().
>>
>> This change comes from the discussion between Takashi and
>> Kai Vehmanen. Please refer to:
>> https://github.com/alsa-project/alsa-lib/pull/118
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> As this would "fix" actual use cases, I'd love to merge this for 5.12,
> but of course it needs to be verified beforehand.
>
> So this was actually tested in your side, right?
>
I tested it on TGL machines with Ubuntu,  both sof and legacy hda, so 
far there is no regression.

Regards,

Hui.

> thanks,
>
> Takashi
>

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Kai Vehmanen]

Hi,

thanks for the patch! 

On Tue, 23 Feb 2021, Hui Wang wrote:

> If the platform set the dyn_pcm_assign to true, it will call
> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is
> connected and need to create a pcm.
[...]
> This change comes from the discussion between Takashi and
> Kai Vehmanen. Please refer to:
> https://github.com/alsa-project/alsa-lib/pull/118

I did propose to merge the alsa-lib change to give us a bit more time to 
think about how this should be handled in kernel.

While this patch certainly solves the problem of kernel picking ALSA PCMs, 
which current alsa-lib cannot handle, it leaves us a bit halfway. We'd 
create many PCMs that will never be used. And this change is a bit more 
involved.

> So far only intel_hsw_common_init() and patch_nvhdmi() set the
> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot
> dynamically first, if the driver runs for a period of time and there
> is no regression reported, we could set no_fixed_assgin to true in
> the intel_hsw_common_init(), and then set it to true in the
> patch_nvhdmi().

Staged plan sounds good here, although I'd be fairly cautious with this. 
People using Pulseaudio/Pipewire+UCM won't notice a thing, but I'm sure 
there are people out there assuming a fixed "physical connector -> ALSA 
PCM" mapping and not using UCM. Probably at least some way to opt-out 
would be needed for older platforms.

> +	if (port_num > 6)
> +		spec->no_fixed_assign = true;

I think this is magic enough of a number to be defined separately along 
with some documentation. So basicly user-space has a max limit of 8 now
and two PCMs are reserved for DP-MST, so that brings us to six, right?

This is somewhat arbitrary still. If we simply want to enable the mode for 
TGL only, easier and cleaned would be to set this flag in 
patch_i915_tgl_hdmi() directly.

Br, Kai

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Jaroslav Kysela]

Dne 23. 02. 21 v 15:14 Kai Vehmanen napsal(a):

> This is somewhat arbitrary still. If we simply want to enable the mode for 
> TGL only, easier and cleaned would be to set this flag in 
> patch_i915_tgl_hdmi() directly.

A dumb question: Does TGL really support up to 11 separate displays or it's
just to handle 11 connections and the number of maximal simultanenous
connected displays is lower? In the later case, the dynamic allocation makes
sense a lot.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Takashi Iwai]

On Tue, 23 Feb 2021 17:25:23 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 02. 21 v 15:14 Kai Vehmanen napsal(a):
> 
> > This is somewhat arbitrary still. If we simply want to enable the mode for 
> > TGL only, easier and cleaned would be to set this flag in 
> > patch_i915_tgl_hdmi() directly.
> 
> A dumb question: Does TGL really support up to 11 separate displays or it's
> just to handle 11 connections and the number of maximal simultanenous
> connected displays is lower? In the later case, the dynamic allocation makes
> sense a lot.

That's the latter.  And, the fixed assignment was merely for
compatibility with legacy usage, and supposed to be 3 fixed ones or
so.  Extending to that high number wasn't intended when the mechanism
was introduced.  We should have noticed it at ICL support (which has
up to 6 devices).

About the number of devices: from the implementation POV, number 4
makes sense.  In the current implementation, with more than 4 HDMI
devices, the PCM device number itself is assigned dynamically, hence
you can't enumerate fully statically, in anyway.


Takashi

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Kai Vehmanen]

Hi,

On Tue, 23 Feb 2021, Takashi Iwai wrote:

> On Tue, 23 Feb 2021 17:25:23 +0100, Jaroslav Kysela wrote:
> > A dumb question: Does TGL really support up to 11 separate displays or it's
> > just to handle 11 connections and the number of maximal simultanenous
> > connected displays is lower? In the later case, the dynamic allocation makes
> 
> That's the latter.  And, the fixed assignment was merely for
> compatibility with legacy usage, and supposed to be 3 fixed ones or
> so.  Extending to that high number wasn't intended when the mechanism
> was introduced.  We should have noticed it at ICL support (which has
> up to 6 devices).

yes, exactly. The pins relate to physical ports. There are more pins now 
to cater for various DP-over-USB-C topologies (versus just native HDMI and 
DP ports). Most product have less physical ports connected, but on the HDA 
interface all pins are exposed. Each pin does provide functionality to 
query whether a display is connected to it, and whether the connected 
display has audio capability.

The maximum number of concurrent displays is described as converters.
On TGL this is 4.

With SOF, we didn't have legacy userspace, so the HDMI/DP PCMs are already 
exposed differently and only a small number (3 or 4) of PCMs are created 
depending on hardware generation. Now the question is how we move 
snd-hda-intel to similar model with minimal distraction to existing 
user-space.

Br, Kai

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Hui Wang]

On 2/24/21 1:51 AM, Kai Vehmanen wrote:
> Hi,
>
> On Tue, 23 Feb 2021, Takashi Iwai wrote:
>
>> On Tue, 23 Feb 2021 17:25:23 +0100, Jaroslav Kysela wrote:
>>> A dumb question: Does TGL really support up to 11 separate displays or it's
>>> just to handle 11 connections and the number of maximal simultanenous
>>> connected displays is lower? In the later case, the dynamic allocation makes
>> That's the latter.  And, the fixed assignment was merely for
>> compatibility with legacy usage, and supposed to be 3 fixed ones or
>> so.  Extending to that high number wasn't intended when the mechanism
>> was introduced.  We should have noticed it at ICL support (which has
>> up to 6 devices).
> yes, exactly. The pins relate to physical ports. There are more pins now
> to cater for various DP-over-USB-C topologies (versus just native HDMI and
> DP ports). Most product have less physical ports connected, but on the HDA
> interface all pins are exposed. Each pin does provide functionality to
> query whether a display is connected to it, and whether the connected
> display has audio capability.
>
> The maximum number of concurrent displays is described as converters.
> On TGL this is 4.
If a physical port supports DP-MST, does the 3 connections on this 
physical port share a single converter? And each connection has an 
independent pcm,  maybe the driver should create pcm pool according to 
num_converter * 3.
>
> With SOF, we didn't have legacy userspace, so the HDMI/DP PCMs are already
> exposed differently and only a small number (3 or 4) of PCMs are created
> depending on hardware generation. Now the question is how we move
> snd-hda-intel to similar model with minimal distraction to existing
> user-space.

Let's do some change on TGL first, let us see if it will break some 
user-space apps, then we could evaluate how to handle it.

Thanks,

Hui.

> Br, Kai

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Kai Vehmanen]

Hey,

On Wed, 24 Feb 2021, Hui Wang wrote:

> On 2/24/21 1:51 AM, Kai Vehmanen wrote:
> > interface all pins are exposed. Each pin does provide functionality to
> > query whether a display is connected to it, and whether the connected
> > display has audio capability.
> > 
> > The maximum number of concurrent displays is described as converters.
> > On TGL this is 4.
> If a physical port supports DP-MST, does the 3 connections on this physical
> port share a single converter? And each connection has an independent pcm, 
> maybe the driver should create pcm pool according to num_converter * 3.

DP-MST is is reported per-pin, so basicly the interface can report display 
connection status for "numpins*3" endpoints so that would be 9*3 on 
Intel TGL systems.

However, this doesn't affect the converters. There is still four 
converters, so 4 PCMs are enough to cover all possible combination of 
plain DP/HDMI and DP-MST. User-space can query the ELD information to 
learn the mapping from a PCM to a specific display (like e.g. Pulseaudio 
does).

I sent a patch "ALSA: hda/proc - print DP-MST connections" to visualize 
these a bit better in procfs output. I put example output in the commit:
https://lore.kernel.org/r/20201208185736.2877541-1-kai.vehmanen@linux.intel.com

The existing driver provides a PCM for each pin, plus reserve two extra 
PCMs for DP-MST. There's merit to this design as well, but arguably 
the SOF approach is easier to understand on systems like TGL and ICL.

Br, Kai

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Hui Wang]

On 2/24/21 6:17 PM, Kai Vehmanen wrote:
> Hey,
>
> On Wed, 24 Feb 2021, Hui Wang wrote:
>
>> On 2/24/21 1:51 AM, Kai Vehmanen wrote:
>>> interface all pins are exposed. Each pin does provide functionality to
>>> query whether a display is connected to it, and whether the connected
>>> display has audio capability.
>>>
>>> The maximum number of concurrent displays is described as converters.
>>> On TGL this is 4.
>> If a physical port supports DP-MST, does the 3 connections on this physical
>> port share a single converter? And each connection has an independent pcm,
>> maybe the driver should create pcm pool according to num_converter * 3.
> DP-MST is is reported per-pin, so basicly the interface can report display
> connection status for "numpins*3" endpoints so that would be 9*3 on
> Intel TGL systems.
>
> However, this doesn't affect the converters. There is still four
> converters, so 4 PCMs are enough to cover all possible combination of
> plain DP/HDMI and DP-MST. User-space can query the ELD information to
> learn the mapping from a PCM to a specific display (like e.g. Pulseaudio
> does).
>
> I sent a patch "ALSA: hda/proc - print DP-MST connections" to visualize
> these a bit better in procfs output. I put example output in the commit:
> https://lore.kernel.org/r/20201208185736.2877541-1-kai.vehmanen@linux.intel.com

Indeed, today I found a thunderbolt dock station which plays DP-MST hub 
and I connected 2 monitors on it, they are 2 sub-devices belong to a 
physical pin, and they are assigned different converters. And the procfs 
output is very clear to show it.

Thanks,

Hui.

>
> The existing driver provides a PCM for each pin, plus reserve two extra
> PCMs for DP-MST. There's merit to this design as well, but arguably
> the SOF approach is easier to understand on systems like TGL and ICL.
>
> Br, Kai

Re: [PATCH] ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically

[Hui Wang]

On 2/23/21 10:14 PM, Kai Vehmanen wrote:
> Hi,
>
> thanks for the patch!
>
> On Tue, 23 Feb 2021, Hui Wang wrote:
>
>> If the platform set the dyn_pcm_assign to true, it will call
>> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is
>> connected and need to create a pcm.
> [...]
>> This change comes from the discussion between Takashi and
>> Kai Vehmanen. Please refer to:
>> https://github.com/alsa-project/alsa-lib/pull/118
> I did propose to merge the alsa-lib change to give us a bit more time to
> think about how this should be handled in kernel.
That is not merged yet, and my PM was pushing me to find a solution, 
then I wrote this patch.
>
> While this patch certainly solves the problem of kernel picking ALSA PCMs,
> which current alsa-lib cannot handle, it leaves us a bit halfway. We'd
> create many PCMs that will never be used. And this change is a bit more
> involved.
>
>> So far only intel_hsw_common_init() and patch_nvhdmi() set the
>> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot
>> dynamically first, if the driver runs for a period of time and there
>> is no regression reported, we could set no_fixed_assgin to true in
>> the intel_hsw_common_init(), and then set it to true in the
>> patch_nvhdmi().
> Staged plan sounds good here, although I'd be fairly cautious with this.
> People using Pulseaudio/Pipewire+UCM won't notice a thing, but I'm sure
> there are people out there assuming a fixed "physical connector -> ALSA
> PCM" mapping and not using UCM. Probably at least some way to opt-out
> would be needed for older platforms.
>
>> +	if (port_num > 6)
>> +		spec->no_fixed_assign = true;
> I think this is magic enough of a number to be defined separately along
> with some documentation. So basicly user-space has a max limit of 8 now
> and two PCMs are reserved for DP-MST, so that brings us to six, right?
Yes, and also before the TGL, the max number of physical pins is 6 (icl).
>
> This is somewhat arbitrary still. If we simply want to enable the mode for
> TGL only, easier and cleaned would be to set this flag in
> patch_i915_tgl_hdmi() directly.

OK, will consider it.

Thanks,

Hui.

> Br, Kai