Skip to contentSkip to conversation

ALSA: hda - hdmi begin to support dynamic PCM assignment

RFC PATCH In review [email protected] 3 participants 5 messages Nov 13, 2015 – Nov 14, 2015 Sound

lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 ALSA: hda - hdmi begin to support dynamic PCM assignment

From: Libin Yang <[email protected]>

Begin to support dynamic PCM assignment to pin in hdmi audio driver.

This means PCM will not be statically bound with pin. When there is a monitor connected, the corresponding pin will try to find a proper PCM to bind. When the monitor is disconnected, the corresponding pin will unbind the PCM. This helps to reduce the PCM number when there are many pins (device entries in DP MST mode) and only a few of them work at the same time.

This patch adds the pcm member in struct hdmi_spec_per_pin. When PCM is dynamically bound to the pin, the member pcm will pointer to the corresponding pcm_rec[] in hdmi_spec, which means the hda_pcm is bound to the pin.

---
 sound/pci/hda/patch_hdmi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
sound/pci/hda/patch_hdmi.c +21 -5
index f503a88..f3363b8 100644
@@ -82,6 +82,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] dynamically*/
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -140,7 +141,8 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
-
+	bool dyn_pcm_assign;
+	struct mutex pcm_lock;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -378,13 +380,26 @@ static int hinfo_to_pin_index(struct hda_codec *codec,
 			      struct hda_pcm_stream *hinfo)
 {
 	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
-			return pin_idx;
+	if (!spec->dyn_pcm_assign) {
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
+			if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
+				return pin_idx;
+	} else {
+		mutex_lock(&spec->pcm_lock);
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+			per_pin = get_pin(spec, pin_idx);
+			if (per_pin->pcm && per_pin->pcm->stream == hinfo) {
+				mutex_unlock(&spec->pcm_lock);
+				return pin_idx;
+			}
+		}
+		mutex_unlock(&spec->pcm_lock);
+	}
 
-	codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo);
+	codec_dbg(codec, "HDMI: hinfo %p not registered\n", hinfo);
 	return -EINVAL;
 }
 
@@ -2360,6 +2375,7 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		return -ENOMEM;
 
 	spec->ops = generic_standard_hdmi_ops;
+	mutex_init(&spec->pcm_lock);
 	codec->spec = spec;
 	hdmi_array_init(spec, 4);
 
Signature · 1 line
1.9.1
#1 lore raw
Quoted 60 lines · On Fri, 13 Nov 2015 09:56:30 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

Begin to support dynamic PCM assignment to pin in hdmi audio driver.

This means PCM will not be statically bound with pin. When there is a monitor connected, the corresponding pin will try to find a proper PCM to bind. When the monitor is disconnected, the corresponding pin will unbind the PCM. This helps to reduce the PCM number when there are many pins (device entries in DP MST mode) and only a few of them work at the same time.

This patch adds the pcm member in struct hdmi_spec_per_pin. When PCM is dynamically bound to the pin, the member pcm will pointer to the corresponding pcm_rec[] in hdmi_spec, which means the hda_pcm is bound to the pin.

---
 sound/pci/hda/patch_hdmi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
sound/pci/hda/patch_hdmi.c +8 -4
index f503a88..f3363b8 100644
@@ -82,6 +82,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] dynamically*/
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -140,7 +141,8 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
-
+	bool dyn_pcm_assign;
+	struct mutex pcm_lock;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -378,13 +380,26 @@ static int hinfo_to_pin_index(struct hda_codec *codec,
 			      struct hda_pcm_stream *hinfo)
 {
 	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
-			return pin_idx;
+	if (!spec->dyn_pcm_assign) {
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
+			if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
+				return pin_idx;

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec, · line 399

Quoted 10 lines
sound/pci/hda/patch_hdmi.c +10 -0
+	} else {
+		mutex_lock(&spec->pcm_lock);
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+			per_pin = get_pin(spec, pin_idx);
+			if (per_pin->pcm && per_pin->pcm->stream == hinfo) {
+				mutex_unlock(&spec->pcm_lock);
+				return pin_idx;
+			}
+		}
+		mutex_unlock(&spec->pcm_lock);

You can assign per_pin->pcm statically at init time when dyn_pcm_assign=false. Then the same code is used in the above.

The need of spec->pcm_lock isn't clear at this moment. I guess you're trying to avoid the race of dynamic pin assignment and release. But then the code above doesn't protect anything because you return the pin_idx as an available value after the unlock. Then the pin release can happen after this point while you're accessing with the pin_idx.

So, better to consider the implementation of lock or other mechanism once when the whole picture is ready.

Takashi

#2 lore raw
Quoted 81 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, November 13, 2015 5:21 PM
To: [email protected]
Cc: [email protected]; [email protected]; Yang, Libin
Subject: Re: [alsa-devel] [RFC PATCH] ALSA: hda - hdmi begin to support dynamic PCM assignment

Quoted 19 lines · On Fri, 13 Nov 2015 09:56:30 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

Begin to support dynamic PCM assignment to pin in hdmi audio driver.

This means PCM will not be statically bound with pin. When there is a monitor connected, the corresponding pin will try to find a proper PCM to bind. When the monitor is disconnected, the corresponding pin will unbind the PCM. This helps to reduce the PCM number when there are many pins (device entries in DP MST mode) and only a few of them work at the same time.

This patch adds the pcm member in struct hdmi_spec_per_pin. When PCM is dynamically bound to the pin, the member pcm will pointer to the corresponding pcm_rec[] in hdmi_spec, which means the hda_pcm is bound to the pin.

Review

Quoted 6 lines
---
 sound/pci/hda/patch_hdmi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
(quoted diff) +0 -0

b/sound/pci/hda/patch_hdmi.c

sound/pci/hda/patch_hdmi.c struct hdmi_spec_per_pin { · line 85

Quoted 8 lines

index f503a88..f3363b8 100644

sound/pci/hda/patch_hdmi.c +1 -0
@@ -82,6 +82,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n]

dynamically*/

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct

Quoted 14 lines
sound/pci/hda/patch_hdmi.c +2 -1
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -140,7 +141,8 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
-
+	bool dyn_pcm_assign;
+	struct mutex pcm_lock;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -378,13 +380,26 @@ static int hinfo_to_pin_index(struct

hda_codec *codec,

Quoted 8 lines
sound/pci/hda/patch_hdmi.c +1 -2
 			      struct hda_pcm_stream *hinfo)
 {
 	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pcm_rec(spec, pin_idx)->stream == hinfo)

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec,

Quoted 10 lines
sound/pci/hda/patch_hdmi.c +9 -1
-			return pin_idx;
+	if (!spec->dyn_pcm_assign) {
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
+			if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
+				return pin_idx;
+	} else {
+		mutex_lock(&spec->pcm_lock);
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+			per_pin = get_pin(spec, pin_idx);
+			if (per_pin->pcm && per_pin->pcm->stream ==

hinfo) {

Quoted 8 lines

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec, · line 399

Quoted 5 lines
sound/pci/hda/patch_hdmi.c +5 -0
+				mutex_unlock(&spec->pcm_lock);
+				return pin_idx;
+			}
+		}
+		mutex_unlock(&spec->pcm_lock);

You can assign per_pin->pcm statically at init time when dyn_pcm_assign=false. Then the same code is used in the above.

Yes, I will change it.

Quoted 9 lines

The need of spec->pcm_lock isn't clear at this moment. I guess you're trying to avoid the race of dynamic pin assignment and release. But then the code above doesn't protect anything because you return the pin_idx as an available value after the unlock. Then the pin release can happen after this point while you're accessing with the pin_idx.

So, better to consider the implementation of lock or other mechanism once when the whole picture is ready.

I add the lock to avoid:
if (per_pin->pcm && per_pin->pcm->stream == hinfo)
per_pin->pcm is not null at first and when judge "per_pin->pcm->stream == hinfo" per_pin->pcm has been null.

I found it's very hard to protect the pin_idx in the whole process. For example, pin is connected when open, but disconnected when prepare or close, or vise verse. Or open is called twice at the same time, but pin is assigned to 2 different PCMs (hotplug happens).

Maybe we can keep the lock now, and will refine it later? This lock can make sure when per_pin->pcm->stream == hinfo, it will never access invalid memory.

Regards,
Libin

Quoted 3 lines

Takashi

#3 lore raw
Quoted 108 lines · Libin
Quoted 82 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, November 13, 2015 5:21 PM
To: [email protected]
Cc: [email protected]; [email protected]; Yang, Libin
Subject: Re: [alsa-devel] [RFC PATCH] ALSA: hda - hdmi begin to support dynamic PCM assignment

Quoted 19 lines · On Fri, 13 Nov 2015 09:56:30 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

Begin to support dynamic PCM assignment to pin in hdmi audio driver.

This means PCM will not be statically bound with pin. When there is a monitor connected, the corresponding pin will try to find a proper PCM to bind. When the monitor is disconnected, the corresponding pin will unbind the PCM. This helps to reduce the PCM number when there are many pins (device entries in DP MST mode) and only a few of them work at the same time.

This patch adds the pcm member in struct hdmi_spec_per_pin. When PCM is dynamically bound to the pin, the member pcm will pointer to the corresponding pcm_rec[] in hdmi_spec, which means the hda_pcm is bound to the pin.

Review

Quoted 6 lines
---
 sound/pci/hda/patch_hdmi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
(quoted diff) +0 -0

b/sound/pci/hda/patch_hdmi.c

sound/pci/hda/patch_hdmi.c struct hdmi_spec_per_pin { · line 85

Quoted 8 lines

index f503a88..f3363b8 100644

sound/pci/hda/patch_hdmi.c +1 -0
@@ -82,6 +82,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n]

dynamically*/

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct

Quoted 14 lines
sound/pci/hda/patch_hdmi.c +2 -1
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -140,7 +141,8 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
-
+	bool dyn_pcm_assign;
+	struct mutex pcm_lock;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -378,13 +380,26 @@ static int hinfo_to_pin_index(struct

hda_codec *codec,

Quoted 8 lines
sound/pci/hda/patch_hdmi.c +1 -2
 			      struct hda_pcm_stream *hinfo)
 {
 	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pcm_rec(spec, pin_idx)->stream == hinfo)

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec,

Quoted 10 lines
sound/pci/hda/patch_hdmi.c +9 -1
-			return pin_idx;
+	if (!spec->dyn_pcm_assign) {
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
+			if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
+				return pin_idx;
+	} else {
+		mutex_lock(&spec->pcm_lock);
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+			per_pin = get_pin(spec, pin_idx);
+			if (per_pin->pcm && per_pin->pcm->stream ==

hinfo) {

Quoted 8 lines

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec, · line 399

Quoted 5 lines
sound/pci/hda/patch_hdmi.c +5 -0
+				mutex_unlock(&spec->pcm_lock);
+				return pin_idx;
+			}
+		}
+		mutex_unlock(&spec->pcm_lock);

You can assign per_pin->pcm statically at init time when dyn_pcm_assign=false. Then the same code is used in the above.

Yes, I will change it.

Quoted 9 lines

The need of spec->pcm_lock isn't clear at this moment. I guess you're trying to avoid the race of dynamic pin assignment and release. But then the code above doesn't protect anything because you return the pin_idx as an available value after the unlock. Then the pin release can happen after this point while you're accessing with the pin_idx.

So, better to consider the implementation of lock or other mechanism once when the whole picture is ready.

I add the lock to avoid:
if (per_pin->pcm && per_pin->pcm->stream == hinfo)
per_pin->pcm is not null at first and when judge "per_pin->pcm->stream == hinfo" per_pin->pcm has been null.

Quoted 4 lines

I found it's very hard to protect the pin_idx in the whole process. For example, pin is connected when open, but disconnected when prepare or close, or vise verse. Or open is called twice at the same time, but pin is assigned to 2 different PCMs (hotplug happens).

I think it's feasible. We need to protect it only in each PCM operation, not through the whole PCM open time, i.e. only in each prepare(), cleanup() or whatever call. For example, make a rw semaphore and do read lock in each operation. When the PCM assign or release happens, it takes write lock. So it waits until the PCM operation finishes and blocks it while switching.

Quoted 3 lines

Maybe we can keep the lock now, and will refine it later? This lock can make sure when per_pin->pcm->stream == hinfo, it will never access invalid memory.

As I wrote, there is no reason to introduce the lock in *this* patch, as there is no race at all here. There will be a race in future, but then let's think the best way at that point.

Also, the mapping between hinfo and per_pin can be done in a different way instead of loop search. We may still need some lock, but then the required lock would be different, too.

Takashi

#4 lore raw
Quoted 126 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, November 13, 2015 11:56 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [alsa-devel] [RFC PATCH] ALSA: hda - hdmi begin to support dynamic PCM assignment

Quoted 9 lines · Libin
Quoted 9 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, November 13, 2015 5:21 PM
To: [email protected]
Cc: [email protected]; [email protected]; Yang, Libin
Subject: Re: [alsa-devel] [RFC PATCH] ALSA: hda - hdmi begin to

support

Quoted 99 lines
Quoted 73 lines

dynamic PCM assignment

Quoted 19 lines · On Fri, 13 Nov 2015 09:56:30 +0100, [email protected] wrote:

From: Libin Yang <[email protected]>

Begin to support dynamic PCM assignment to pin in hdmi audio driver.

This means PCM will not be statically bound with pin. When there is a monitor connected, the corresponding pin will try to find a proper PCM to bind. When the monitor is disconnected, the corresponding pin will unbind the PCM. This helps to reduce the PCM number when there are many pins (device entries in DP MST mode) and only a few of them work at the same time.

This patch adds the pcm member in struct hdmi_spec_per_pin. When PCM is dynamically bound to the pin, the member pcm will pointer to the corresponding pcm_rec[] in hdmi_spec, which means the hda_pcm is bound to the pin.

Review

Quoted 6 lines
---
 sound/pci/hda/patch_hdmi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
(quoted diff) +0 -0

b/sound/pci/hda/patch_hdmi.c

sound/pci/hda/patch_hdmi.c struct hdmi_spec_per_pin { · line 85

Quoted 8 lines

index f503a88..f3363b8 100644

sound/pci/hda/patch_hdmi.c +1 -0
@@ -82,6 +82,7 @@ struct hdmi_spec_per_pin {
 	struct mutex lock;
 	struct delayed_work work;
 	struct snd_kcontrol *eld_ctl;
+	struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n]

dynamically*/

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct

Quoted 14 lines
sound/pci/hda/patch_hdmi.c +2 -1
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
 	int channels; /* current number of channels */
@@ -140,7 +141,8 @@ struct hdmi_spec {
 	struct hdmi_ops ops;
 
 	bool dyn_pin_out;
-
+	bool dyn_pcm_assign;
+	struct mutex pcm_lock;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -378,13 +380,26 @@ static int hinfo_to_pin_index(struct

hda_codec *codec,

Quoted 8 lines
sound/pci/hda/patch_hdmi.c +1 -2
 			      struct hda_pcm_stream *hinfo)
 {
 	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
 	int pin_idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pcm_rec(spec, pin_idx)->stream == hinfo)

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec,

Quoted 10 lines
sound/pci/hda/patch_hdmi.c +9 -1
-			return pin_idx;
+	if (!spec->dyn_pcm_assign) {
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
+			if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
+				return pin_idx;
+	} else {
+		mutex_lock(&spec->pcm_lock);
+		for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+			per_pin = get_pin(spec, pin_idx);
+			if (per_pin->pcm && per_pin->pcm->stream ==

hinfo) {

Quoted 8 lines

sound/pci/hda/patch_hdmi.c static int hinfo_to_pin_index(struct hda_codec *codec, · line 399

Quoted 5 lines
sound/pci/hda/patch_hdmi.c +5 -0
+				mutex_unlock(&spec->pcm_lock);
+				return pin_idx;
+			}
+		}
+		mutex_unlock(&spec->pcm_lock);

You can assign per_pin->pcm statically at init time when dyn_pcm_assign=false. Then the same code is used in the above.

Yes, I will change it.

Quoted 9 lines

The need of spec->pcm_lock isn't clear at this moment. I guess you're trying to avoid the race of dynamic pin assignment and release. But then the code above doesn't protect anything because you return the pin_idx as an available value after the unlock. Then the pin release can happen after this point while you're accessing with the pin_idx.

So, better to consider the implementation of lock or other mechanism once when the whole picture is ready.

I add the lock to avoid:
if (per_pin->pcm && per_pin->pcm->stream == hinfo)
per_pin->pcm is not null at first and when judge "per_pin->pcm->stream == hinfo" per_pin->pcm has been null.

Quoted 4 lines

I found it's very hard to protect the pin_idx in the whole process. For example, pin is connected when open, but disconnected when prepare or close, or vise verse. Or open is called twice at the same time, but pin is assigned to 2 different PCMs (hotplug happens).

Quoted 6 lines

I think it's feasible. We need to protect it only in each PCM operation, not through the whole PCM open time, i.e. only in each prepare(), cleanup() or whatever call. For example, make a rw semaphore and do read lock in each operation. When the PCM assign or release happens, it takes write lock. So it waits until the PCM operation finishes and blocks it while switching.

Get it. Thanks.

Quoted 8 lines
Quoted 4 lines

Maybe we can keep the lock now, and will refine it later? This lock can make sure when per_pin->pcm->stream == hinfo, it will never access invalid memory.

As I wrote, there is no reason to introduce the lock in *this* patch, as there is no race at all here. There will be a race in future, but then let's think the best way at that point.

Get it. I will remove the lock.

Regards,
Libin

Quoted 7 lines

Also, the mapping between hinfo and per_pin can be done in a different way instead of loop search. We may still need some lock, but then the required lock would be different, too.

Takashi