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

[PATCH v2] 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>
---
Did the test with this patch on 2 TGL machines, and with both legacy
HDA driver and sof driver, plug 1 monitor into the machine, plug 2
monitors into the machine and plug 2 monitors into a dp-mst hub, they
all worked as expcted, the first 2 pcm slots was assigned to monitors,
and playback worked well.

 sound/pci/hda/patch_hdmi.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 97adff0cbcab..1aca815b920d 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))
@@ -2988,8 +2996,16 @@ static int patch_i915_tgl_hdmi(struct hda_codec *codec)
 	 * the index indicate the port number.
 	 */
 	static const int map[] = {0x4, 0x6, 0x8, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf};
+	int ret;
 
-	return intel_hsw_common_init(codec, 0x02, map, ARRAY_SIZE(map));
+	ret = intel_hsw_common_init(codec, 0x02, map, ARRAY_SIZE(map));
+	if (!ret) {
+		struct hdmi_spec *spec = codec->spec;
+
+		spec->no_fixed_assign = true;
+	}
+
+	return ret;
 }
 
 /* Intel Baytrail and Braswell; with eld notifier */
-- 
2.25.1

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

[Kai Vehmanen]

Hi,

thanks! I tested this on a few machines and seems good, no functional 
issues.

On Thu, 25 Feb 2021, Hui Wang wrote:

> --- 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;

hmm, I wonder if we could come up with a better name. "dyn_pcm_assign"
and "no_fixed_assign" seem a bit at odds.

How about:
   bool dyn_pcm_assign; 
+  bool dyn_pcm_no_legacy; 

Then it's more clear this is still the same dynamic PCM assign policy, but 
we just drop the legacy exception.

Takashi, do you think we can go with this without any method (a module 
option) to opt-out?

I'm ok to go with this, given all users should be using kcontrol status to 
select the PCM for HDMI/DP output. Any other method is not guaranteed 
anyways to work in all cases as the assignment is dynamic in the end.. 

Br, Kai

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

[Takashi Iwai]

On Fri, 26 Feb 2021 19:14:32 +0100,
Kai Vehmanen wrote:
> 
> Takashi, do you think we can go with this without any method (a module 
> option) to opt-out?

Yes, let's try without a module option at first.  I bet no one would
be annoyed by this change.

> I'm ok to go with this, given all users should be using kcontrol status to 
> select the PCM for HDMI/DP output. Any other method is not guaranteed 
> anyways to work in all cases as the assignment is dynamic in the end.. 

Right.  Let's hope that all modern systems already deal with the
situation better than ago.


thanks,

Takashi

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

[Hui Wang]

On 2/27/21 2:14 AM, Kai Vehmanen wrote:
> Hi,
>
> thanks! I tested this on a few machines and seems good, no functional
> issues.
>
> On Thu, 25 Feb 2021, Hui Wang wrote:
>
>> --- 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;
> hmm, I wonder if we could come up with a better name. "dyn_pcm_assign"
> and "no_fixed_assign" seem a bit at odds.
>
> How about:
>     bool dyn_pcm_assign;
> +  bool dyn_pcm_no_legacy;
>
> Then it's more clear this is still the same dynamic PCM assign policy, but
> we just drop the legacy exception.

OK, will change to it.

Thanks.

> Takashi, do you think we can go with this without any method (a module
> option) to opt-out?
>
> I'm ok to go with this, given all users should be using kcontrol status to
> select the PCM for HDMI/DP output. Any other method is not guaranteed
> anyways to work in all cases as the assignment is dynamic in the end..
>
> Br, Kai