[v1,3/5] ALSA: hda - Add DP-MST conn list support
diff mbox series

Message ID 20191114033704.18171-4-nmahale@nvidia.com
State New
Headers show
Series
  • ALSA: hda - Add DP-MST support for NVIDIA codecs
Related show

Commit Message

Nikhil Mahale Nov. 14, 2019, 3:37 a.m. UTC
Document change notification HDA040-A for the Intel High Definition
Audio 1.0a specification introduces a Device Select verb for Digital
Display Pin Widgets that are multi-stream capable. This verb selects
a Device Entry that is used by subsequent Pin Widget verbs,
including the Get Connection List Entry verb.

This patch queries the current Device Select value, associates it
with the connection list, and updates the connection list management
code to consider dev_id along with nid.

Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
---
 sound/pci/hda/hda_codec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Takashi Iwai Nov. 14, 2019, 10:57 a.m. UTC | #1
On Thu, 14 Nov 2019 04:37:02 +0100,
Nikhil Mahale wrote:
> 
> Document change notification HDA040-A for the Intel High Definition
> Audio 1.0a specification introduces a Device Select verb for Digital
> Display Pin Widgets that are multi-stream capable. This verb selects
> a Device Entry that is used by subsequent Pin Widget verbs,
> including the Get Connection List Entry verb.
> 
> This patch queries the current Device Select value, associates it
> with the connection list, and updates the connection list management
> code to consider dev_id along with nid.

I don't get why this is needed.  This is the list that is added only
from snd_hda_override_conn_list(), and you don't change that call
pattern, so dev_id is always zero.


thanks,

Takashi

> 
> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
> ---
>  sound/pci/hda/hda_codec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index a2fb19129219..8f32cab8f4a4 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -88,6 +88,7 @@ struct hda_conn_list {
>  	struct list_head list;
>  	int len;
>  	hda_nid_t nid;
> +	int dev_id;
>  	hda_nid_t conns[0];
>  };
>  
> @@ -96,8 +97,9 @@ static struct hda_conn_list *
>  lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
>  {
>  	struct hda_conn_list *p;
> +	int dev_id = snd_hda_get_dev_select(codec, nid);
>  	list_for_each_entry(p, &codec->conn_list, list) {
> -		if (p->nid == nid)
> +		if (p->nid == nid && p->dev_id == dev_id)
>  			return p;
>  	}
>  	return NULL;
> @@ -113,6 +115,7 @@ static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
>  		return -ENOMEM;
>  	p->len = len;
>  	p->nid = nid;
> +	p->dev_id = snd_hda_get_dev_select(codec, nid);
>  	memcpy(p->conns, list, len * sizeof(hda_nid_t));
>  	list_add(&p->list, &codec->conn_list);
>  	return 0;
> -- 
> 2.16.4
>
Nikhil Mahale Nov. 14, 2019, 11:47 a.m. UTC | #2
On 11/14/19 4:27 PM, Takashi Iwai wrote:
> On Thu, 14 Nov 2019 04:37:02 +0100,
> Nikhil Mahale wrote:
>>
>> Document change notification HDA040-A for the Intel High Definition
>> Audio 1.0a specification introduces a Device Select verb for Digital
>> Display Pin Widgets that are multi-stream capable. This verb selects
>> a Device Entry that is used by subsequent Pin Widget verbs,
>> including the Get Connection List Entry verb.
>>
>> This patch queries the current Device Select value, associates it
>> with the connection list, and updates the connection list management
>> code to consider dev_id along with nid.
> 
> I don't get why this is needed.  This is the list that is added only
> from snd_hda_override_conn_list(), and you don't change that call
> pattern, so dev_id is always zero.

In follow-on patch "ALSA: hda - Add DP-MST support for non-acomp codecs",
hdmi_read_pin_conn() calls into snd_hda_set_dev_select() before
running into code path -
  
  snd_hda_get_connections()
   |-> snd_hda_get_conn_list()
        |-> read_and_add_raw_conns()
             |-> snd_hda_override_conn_list()
                  |-> add_conn_list()

If I understand spec correctly, conn_list need to read/write/track
along with device entry set into AC_VERB_SET_DEVICE_SEL verb, right?
This patch address that limited part here.

Thanks,
Nikhil Mahale

> thanks,
> 
> Takashi
> 
>>
>> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
>> Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
>> ---
>>  sound/pci/hda/hda_codec.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index a2fb19129219..8f32cab8f4a4 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -88,6 +88,7 @@ struct hda_conn_list {
>>  	struct list_head list;
>>  	int len;
>>  	hda_nid_t nid;
>> +	int dev_id;
>>  	hda_nid_t conns[0];
>>  };
>>  
>> @@ -96,8 +97,9 @@ static struct hda_conn_list *
>>  lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
>>  {
>>  	struct hda_conn_list *p;
>> +	int dev_id = snd_hda_get_dev_select(codec, nid);
>>  	list_for_each_entry(p, &codec->conn_list, list) {
>> -		if (p->nid == nid)
>> +		if (p->nid == nid && p->dev_id == dev_id)
>>  			return p;
>>  	}
>>  	return NULL;
>> @@ -113,6 +115,7 @@ static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
>>  		return -ENOMEM;
>>  	p->len = len;
>>  	p->nid = nid;
>> +	p->dev_id = snd_hda_get_dev_select(codec, nid);
>>  	memcpy(p->conns, list, len * sizeof(hda_nid_t));
>>  	list_add(&p->list, &codec->conn_list);
>>  	return 0;
>> -- 
>> 2.16.4
>>

-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------
Takashi Iwai Nov. 14, 2019, 1:14 p.m. UTC | #3
On Thu, 14 Nov 2019 12:47:36 +0100,
Nikhil Mahale wrote:
> 
> On 11/14/19 4:27 PM, Takashi Iwai wrote:
> > On Thu, 14 Nov 2019 04:37:02 +0100,
> > Nikhil Mahale wrote:
> >>
> >> Document change notification HDA040-A for the Intel High Definition
> >> Audio 1.0a specification introduces a Device Select verb for Digital
> >> Display Pin Widgets that are multi-stream capable. This verb selects
> >> a Device Entry that is used by subsequent Pin Widget verbs,
> >> including the Get Connection List Entry verb.
> >>
> >> This patch queries the current Device Select value, associates it
> >> with the connection list, and updates the connection list management
> >> code to consider dev_id along with nid.
> > 
> > I don't get why this is needed.  This is the list that is added only
> > from snd_hda_override_conn_list(), and you don't change that call
> > pattern, so dev_id is always zero.
> 
> In follow-on patch "ALSA: hda - Add DP-MST support for non-acomp codecs",
> hdmi_read_pin_conn() calls into snd_hda_set_dev_select() before
> running into code path -
>   
>   snd_hda_get_connections()
>    |-> snd_hda_get_conn_list()
>         |-> read_and_add_raw_conns()
>              |-> snd_hda_override_conn_list()
>                   |-> add_conn_list()
> 
> If I understand spec correctly, conn_list need to read/write/track
> along with device entry set into AC_VERB_SET_DEVICE_SEL verb, right?

AFAIK, the device connection list itself is same no matter which
device entry is.  The pin widget may choose the route per device
entry, but the available routes should be same.


Takashi
Nikhil Mahale Nov. 15, 2019, 9:52 a.m. UTC | #4
On 11/14/19 6:44 PM, Takashi Iwai wrote:
> On Thu, 14 Nov 2019 12:47:36 +0100,
> Nikhil Mahale wrote:
>>
>> On 11/14/19 4:27 PM, Takashi Iwai wrote:
>>> On Thu, 14 Nov 2019 04:37:02 +0100,
>>> Nikhil Mahale wrote:
>>>>
>>>> Document change notification HDA040-A for the Intel High Definition
>>>> Audio 1.0a specification introduces a Device Select verb for Digital
>>>> Display Pin Widgets that are multi-stream capable. This verb selects
>>>> a Device Entry that is used by subsequent Pin Widget verbs,
>>>> including the Get Connection List Entry verb.
>>>>
>>>> This patch queries the current Device Select value, associates it
>>>> with the connection list, and updates the connection list management
>>>> code to consider dev_id along with nid.
>>>
>>> I don't get why this is needed.  This is the list that is added only
>>> from snd_hda_override_conn_list(), and you don't change that call
>>> pattern, so dev_id is always zero.
>>
>> In follow-on patch "ALSA: hda - Add DP-MST support for non-acomp codecs",
>> hdmi_read_pin_conn() calls into snd_hda_set_dev_select() before
>> running into code path -
>>   
>>   snd_hda_get_connections()
>>    |-> snd_hda_get_conn_list()
>>         |-> read_and_add_raw_conns()
>>              |-> snd_hda_override_conn_list()
>>                   |-> add_conn_list()
>>
>> If I understand spec correctly, conn_list need to read/write/track
>> along with device entry set into AC_VERB_SET_DEVICE_SEL verb, right?
> 
> AFAIK, the device connection list itself is same no matter which
> device entry is.  The pin widget may choose the route per device
> entry, but the available routes should be same.

Section 7.3.3.42 of https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/high-definition-audio-multi-stream.pdf says that 'Get Connection List Entry' control should be per-device!

Thanks,
Nikhil Mahale

> 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.
-----------------------------------------------------------------------------------
Takashi Iwai Nov. 15, 2019, 10:30 a.m. UTC | #5
On Fri, 15 Nov 2019 10:52:30 +0100,
Nikhil Mahale wrote:
> 
> On 11/14/19 6:44 PM, Takashi Iwai wrote:
> > On Thu, 14 Nov 2019 12:47:36 +0100,
> > Nikhil Mahale wrote:
> >>
> >> On 11/14/19 4:27 PM, Takashi Iwai wrote:
> >>> On Thu, 14 Nov 2019 04:37:02 +0100,
> >>> Nikhil Mahale wrote:
> >>>>
> >>>> Document change notification HDA040-A for the Intel High Definition
> >>>> Audio 1.0a specification introduces a Device Select verb for Digital
> >>>> Display Pin Widgets that are multi-stream capable. This verb selects
> >>>> a Device Entry that is used by subsequent Pin Widget verbs,
> >>>> including the Get Connection List Entry verb.
> >>>>
> >>>> This patch queries the current Device Select value, associates it
> >>>> with the connection list, and updates the connection list management
> >>>> code to consider dev_id along with nid.
> >>>
> >>> I don't get why this is needed.  This is the list that is added only
> >>> from snd_hda_override_conn_list(), and you don't change that call
> >>> pattern, so dev_id is always zero.
> >>
> >> In follow-on patch "ALSA: hda - Add DP-MST support for non-acomp codecs",
> >> hdmi_read_pin_conn() calls into snd_hda_set_dev_select() before
> >> running into code path -
> >>   
> >>   snd_hda_get_connections()
> >>    |-> snd_hda_get_conn_list()
> >>         |-> read_and_add_raw_conns()
> >>              |-> snd_hda_override_conn_list()
> >>                   |-> add_conn_list()
> >>
> >> If I understand spec correctly, conn_list need to read/write/track
> >> along with device entry set into AC_VERB_SET_DEVICE_SEL verb, right?
> > 
> > AFAIK, the device connection list itself is same no matter which
> > device entry is.  The pin widget may choose the route per device
> > entry, but the available routes should be same.
> 
> Section 7.3.3.42 of https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/high-definition-audio-multi-stream.pdf says that 'Get Connection List Entry' control should be per-device!

Hrm, right.  It's a bit concern that this may lead to the unlimited
number of data, though.

Since the necessity is only for HDMI codec, if that's the only place
we need the connection list, the call there can be replaced with
snd_hda_get_raw_connections() so that the result won't be cached in
HD-audio core side.


Takashi
Nikhil Mahale Nov. 19, 2019, 8:43 a.m. UTC | #6
Thanks,
Nikhil Mahale

On 11/15/19 4:00 PM, Takashi Iwai wrote:
> On Fri, 15 Nov 2019 10:52:30 +0100,
> Nikhil Mahale wrote:
>>
>> On 11/14/19 6:44 PM, Takashi Iwai wrote:
>>> On Thu, 14 Nov 2019 12:47:36 +0100,
>>> Nikhil Mahale wrote:
>>>>
>>>> On 11/14/19 4:27 PM, Takashi Iwai wrote:
>>>>> On Thu, 14 Nov 2019 04:37:02 +0100,
>>>>> Nikhil Mahale wrote:
>>>>>>
>>>>>> Document change notification HDA040-A for the Intel High Definition
>>>>>> Audio 1.0a specification introduces a Device Select verb for Digital
>>>>>> Display Pin Widgets that are multi-stream capable. This verb selects
>>>>>> a Device Entry that is used by subsequent Pin Widget verbs,
>>>>>> including the Get Connection List Entry verb.
>>>>>>
>>>>>> This patch queries the current Device Select value, associates it
>>>>>> with the connection list, and updates the connection list management
>>>>>> code to consider dev_id along with nid.
>>>>>
>>>>> I don't get why this is needed.  This is the list that is added only
>>>>> from snd_hda_override_conn_list(), and you don't change that call
>>>>> pattern, so dev_id is always zero.
>>>>
>>>> In follow-on patch "ALSA: hda - Add DP-MST support for non-acomp codecs",
>>>> hdmi_read_pin_conn() calls into snd_hda_set_dev_select() before
>>>> running into code path -
>>>>   
>>>>   snd_hda_get_connections()
>>>>    |-> snd_hda_get_conn_list()
>>>>         |-> read_and_add_raw_conns()
>>>>              |-> snd_hda_override_conn_list()
>>>>                   |-> add_conn_list()
>>>>
>>>> If I understand spec correctly, conn_list need to read/write/track
>>>> along with device entry set into AC_VERB_SET_DEVICE_SEL verb, right?
>>>
>>> AFAIK, the device connection list itself is same no matter which
>>> device entry is.  The pin widget may choose the route per device
>>> entry, but the available routes should be same.
>>
>> Section 7.3.3.42 of https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/high-definition-audio-multi-stream.pdf says that 'Get Connection List Entry' control should be per-device!
> 
> Hrm, right.  It's a bit concern that this may lead to the unlimited
> number of data, though.
> 
> Since the necessity is only for HDMI codec, if that's the only place
> we need the connection list, the call there can be replaced with
> snd_hda_get_raw_connections() so that the result won't be cached in
> HD-audio core side.
Ok, I am addressing this in 3rd version of patch set.

Thanks,
Nikhil Mahale

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

Patch
diff mbox series

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index a2fb19129219..8f32cab8f4a4 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -88,6 +88,7 @@  struct hda_conn_list {
 	struct list_head list;
 	int len;
 	hda_nid_t nid;
+	int dev_id;
 	hda_nid_t conns[0];
 };
 
@@ -96,8 +97,9 @@  static struct hda_conn_list *
 lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
 {
 	struct hda_conn_list *p;
+	int dev_id = snd_hda_get_dev_select(codec, nid);
 	list_for_each_entry(p, &codec->conn_list, list) {
-		if (p->nid == nid)
+		if (p->nid == nid && p->dev_id == dev_id)
 			return p;
 	}
 	return NULL;
@@ -113,6 +115,7 @@  static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
 		return -ENOMEM;
 	p->len = len;
 	p->nid = nid;
+	p->dev_id = snd_hda_get_dev_select(codec, nid);
 	memcpy(p->conns, list, len * sizeof(hda_nid_t));
 	list_add(&p->list, &codec->conn_list);
 	return 0;