Re: [alsa-devel] [RFC]Connection list of hda widget
At Thu, 07 Feb 2013 16:09:35 +0100, David Henningsson wrote:
On 02/07/2013 06:38 AM, Raymond Yau wrote:
Treat vendor defined widget different from audio selector/audio input widget since hda_proc.c query the connection list of vendor defined widget
ADI codecs have vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
wide_type != AC_WID_VENDOR &&curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
I think there are some Sigmatel/IDT (or was it Analog Devices?) codecs that actually have sensible connections for their vendor specific nodes, so I would prefer not to change this.
Right. The widget connection longer than 32 itself is OK. It's just ugly. That being said, it's basically a bug that we restrict the max connection size in the core code.
So I applied the patch below. This removes the limit while we still keep the efficiency (except for proc output, which isn't too critical). The patch became bigger than I wanted originally, but it's life.
thanks,
Takashi
--- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda - Remove limit of widget connections
Currently we set the max number of connections to be 32, but there seems codec that gives longer connection lists like AD1988, and we see errors in proc output and else. (Though, in the case of AD1988, it's a list of all codecs connected to a single vendor widget, so this must be something fishy, but it's still valid from the h/w design POV.)
This patch tries to remove this restriction. For efficiency, we still use the fixed size array in the parser, but takes a dynamic array when the size is reported to be greater than that.
Now the fixed array size is found only in patch_hdmi.c, but it should be fine, as the codec itself can't support so many pins.
Reported-by: Raymond Yau superquad.vortex2@gmail.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 62 ++++++++++++++++++++++++++++++---------------- sound/pci/hda/hda_codec.h | 4 +-- sound/pci/hda/hda_proc.c | 19 +++++++++++--- sound/pci/hda/patch_hdmi.c | 3 +++ 4 files changed, 60 insertions(+), 28 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index f82a64d..3c92514 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -382,13 +382,23 @@ static void remove_conn_list(struct hda_codec *codec) /* read the connection and add to the cache */ static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid) { - hda_nid_t list[HDA_MAX_CONNECTIONS]; + hda_nid_t list[32]; + hda_nid_t *result = list; int len;
len = snd_hda_get_raw_connections(codec, nid, list, ARRAY_SIZE(list)); - if (len < 0) - return len; - return snd_hda_override_conn_list(codec, nid, len, list); + if (len == -ENOSPC) { + len = snd_hda_get_num_raw_conns(codec, nid); + result = kmalloc(sizeof(hda_nid_t) * len, GFP_KERNEL); + if (!result) + return -ENOMEM; + len = snd_hda_get_raw_connections(codec, nid, result, len); + } + if (len >= 0) + len = snd_hda_override_conn_list(codec, nid, len, result); + if (result != list) + kfree(result); + return len; }
/** @@ -466,6 +476,27 @@ int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid, } EXPORT_SYMBOL_HDA(snd_hda_get_connections);
+/* return CONNLIST_LEN parameter of the given widget */ +static unsigned int get_num_conns(struct hda_codec *codec, hda_nid_t nid) +{ + unsigned int wcaps = get_wcaps(codec, nid); + unsigned int parm; + + if (!(wcaps & AC_WCAP_CONN_LIST) && + get_wcaps_type(wcaps) != AC_WID_VOL_KNB) + return 0; + + parm = snd_hda_param_read(codec, nid, AC_PAR_CONNLIST_LEN); + if (parm == -1) + parm = 0; + return parm; +} + +int snd_hda_get_num_raw_conns(struct hda_codec *codec, hda_nid_t nid) +{ + return get_num_conns(codec, nid) & AC_CLIST_LENGTH; +} + /** * snd_hda_get_raw_connections - copy connection list without cache * @codec: the HDA codec @@ -483,19 +514,16 @@ int snd_hda_get_raw_connections(struct hda_codec *codec, hda_nid_t nid, unsigned int parm; int i, conn_len, conns; unsigned int shift, num_elems, mask; - unsigned int wcaps; hda_nid_t prev_nid; int null_count = 0;
if (snd_BUG_ON(!conn_list || max_conns <= 0)) return -EINVAL;
- wcaps = get_wcaps(codec, nid); - if (!(wcaps & AC_WCAP_CONN_LIST) && - get_wcaps_type(wcaps) != AC_WID_VOL_KNB) + parm = get_num_conns(codec, nid); + if (!parm) return 0;
- parm = snd_hda_param_read(codec, nid, AC_PAR_CONNLIST_LEN); if (parm & AC_CLIST_LONG) { /* long form */ shift = 16; @@ -552,21 +580,13 @@ int snd_hda_get_raw_connections(struct hda_codec *codec, hda_nid_t nid, continue; } for (n = prev_nid + 1; n <= val; n++) { - if (conns >= max_conns) { - snd_printk(KERN_ERR "hda_codec: " - "Too many connections %d for NID 0x%x\n", - conns, nid); - return -EINVAL; - } + if (conns >= max_conns) + return -ENOSPC; conn_list[conns++] = n; } } else { - if (conns >= max_conns) { - snd_printk(KERN_ERR "hda_codec: " - "Too many connections %d for NID 0x%x\n", - conns, nid); - return -EINVAL; - } + if (conns >= max_conns) + return -ENOSPC; conn_list[conns++] = val; } prev_nid = val; diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 6c59272..0be1826 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -551,9 +551,6 @@ enum { AC_JACK_PORT_BOTH, };
-/* max. connections to a widget */ -#define HDA_MAX_CONNECTIONS 32 - /* max. codec address */ #define HDA_MAX_CODEC_ADDRESS 0x0f
@@ -958,6 +955,7 @@ snd_hda_get_num_conns(struct hda_codec *codec, hda_nid_t nid) { return snd_hda_get_connections(codec, nid, NULL, 0); } +int snd_hda_get_num_raw_conns(struct hda_codec *codec, hda_nid_t nid); int snd_hda_get_raw_connections(struct hda_codec *codec, hda_nid_t nid, hda_nid_t *conn_list, int max_conns); int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid, diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c index 5e02f26..97cdbef 100644 --- a/sound/pci/hda/hda_proc.c +++ b/sound/pci/hda/hda_proc.c @@ -623,7 +623,7 @@ static void print_codec_info(struct snd_info_entry *entry, snd_hda_param_read(codec, nid, AC_PAR_AUDIO_WIDGET_CAP); unsigned int wid_type = get_wcaps_type(wid_caps); - hda_nid_t conn[HDA_MAX_CONNECTIONS]; + hda_nid_t *conn = NULL; int conn_len = 0;
snd_iprintf(buffer, "Node 0x%02x [%s] wcaps 0x%x:", nid, @@ -660,9 +660,18 @@ static void print_codec_info(struct snd_info_entry *entry, if (wid_type == AC_WID_VOL_KNB) wid_caps |= AC_WCAP_CONN_LIST;
- if (wid_caps & AC_WCAP_CONN_LIST) - conn_len = snd_hda_get_raw_connections(codec, nid, conn, - HDA_MAX_CONNECTIONS); + if (wid_caps & AC_WCAP_CONN_LIST) { + conn_len = snd_hda_get_num_raw_conns(codec, nid); + if (conn_len > 0) { + conn = kmalloc(sizeof(hda_nid_t) * conn_len, + GFP_KERNEL); + if (!conn) + return; + if (snd_hda_get_raw_connections(codec, nid, conn, + conn_len) < 0) + conn_len = 0; + } + }
if (wid_caps & AC_WCAP_IN_AMP) { snd_iprintf(buffer, " Amp-In caps: "); @@ -735,6 +744,8 @@ static void print_codec_info(struct snd_info_entry *entry,
if (codec->proc_widget_hook) codec->proc_widget_hook(buffer, codec, nid); + + kfree(conn); } snd_hda_power_down(codec); } diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 85236da..899c4fb 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -64,6 +64,9 @@ struct hdmi_spec_per_cvt { unsigned int maxbps; };
+/* max. connections to a widget */ +#define HDA_MAX_CONNECTIONS 32 + struct hdmi_spec_per_pin { hda_nid_t pin_nid; int num_mux_nids;
Treat vendor defined widget different from audio selector/audio input widget since hda_proc.c query the connection list of vendor defined
widget
ADI codecs have vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
wide_type != AC_WID_VENDOR &&curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
I think there are some Sigmatel/IDT (or was it Analog Devices?) codecs that actually have sensible connections for their vendor specific nodes, so I would prefer not to change this.
Do you mean other codecs have vendor defined widgets which support AC_VERB_GET_CONNECT_SEL ?
7.1.2 Node Addressing
Each entry in the connection list is one NID; it may be an independent NID, indicating a single node, or may be part of a 2-tuple of NIDs delineating a continuous range of nodes. The high order bit of each entry indicates how it is to be interpreted (see Figure 51). If the range indicator is set, that list entry forms a range with the previous list entry; i.e., if the range bit were set on the third list entry, then the second and third entries form a range, and the first entry is an independent NID. The range indicator may not be set in the first entry of a connection list nor may it be set on any two sequential list entries. The connection list thus provides a set of optional inputs to the node. A connection selector (control) allows run-time control over which input is used
Right. The widget connection longer than 32 itself is OK. It's just ugly. That being said, it's basically a bug that we restrict the max connection size in the core code.
Node 0x23 [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 18 0x11* 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x24 0x25 0x38 0x39 0x3a 0x3b 0x3c 0x3d 0x20 0x21
The length is 8 and the number of nodes in the connection list is 18
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since there is no way to know the vendor defined widget support the verb GET_CONNECT_SELECT
Node 0x19 [Power Widget] wcaps 0x500500: Mono Power: 0x0 Connection: 2 0x20* 0x21
At Fri, 8 Feb 2013 06:33:46 +0800, Raymond Yau wrote:
Treat vendor defined widget different from audio selector/audio input widget since hda_proc.c query the connection list of vendor defined
widget
ADI codecs have vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
wide_type != AC_WID_VENDOR &&curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
I think there are some Sigmatel/IDT (or was it Analog Devices?) codecs that actually have sensible connections for their vendor specific nodes, so I would prefer not to change this.
Do you mean other codecs have vendor defined widgets which support AC_VERB_GET_CONNECT_SEL ?
Possibly, who knows. It's a vendor-specific widget, after all.
7.1.2 Node Addressing
Each entry in the connection list is one NID; it may be an independent NID, indicating a single node, or may be part of a 2-tuple of NIDs delineating a continuous range of nodes. The high order bit of each entry indicates how it is to be interpreted (see Figure 51). If the range indicator is set, that list entry forms a range with the previous list entry; i.e., if the range bit were set on the third list entry, then the second and third entries form a range, and the first entry is an independent NID. The range indicator may not be set in the first entry of a connection list nor may it be set on any two sequential list entries. The connection list thus provides a set of optional inputs to the node. A connection selector (control) allows run-time control over which input is used
Right. The widget connection longer than 32 itself is OK. It's just ugly. That being said, it's basically a bug that we restrict the max connection size in the core code.
Node 0x23 [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 18 0x11* 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x24 0x25 0x38 0x39 0x3a 0x3b 0x3c 0x3d 0x20 0x21
The length is 8 and the number of nodes in the connection list is 18
Do you mean NID 0x23 returns AC_PAR_CONNLIST_LEN = 8, but contains more nodes because of range definitions? Or what do you mean exactly?
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since there is no way to know the vendor defined widget support the verb GET_CONNECT_SELECT
You should have written _this_ text at the first place! The biggest problem is that your description doesn't match with what you write in your patch.
But still, it's still an open question. In the specification, the use of selector isn't prohibited for vendor-spec widgets. So, some crazy h/w designer may think of it.
thanks,
Takashi
Node 0x19 [Power Widget] wcaps 0x500500: Mono Power: 0x0 Connection: 2 0x20* 0x21
Treat vendor defined widget different from audio selector/audio
input
widget since hda_proc.c query the connection list of vendor
defined
widget
ADI codecs have vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
wide_type != AC_WID_VENDOR &&curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since there
is
no way to know the vendor defined widget support the verb
GET_CONNECT_SELECT
You should have written _this_ text at the first place! The biggest problem is that your description doesn't match with what you write in your patch.
But still, it's still an open question. In the specification, the use of selector isn't prohibited for vendor-spec widgets. So, some crazy h/w designer may think of it.
Yes, it depend on how you interept "Other"
7.3.3.2 Connection Select Control Applies to: Input Converter Selector Widget Pin Complex Other
I think your patch is not necessary, what is missing are the codec responses when error occur on ad1882
-static const char * const ad1882_models[AD1986A_MODELS] = { +static const char * const ad1882_models[AD19882_MODELS] = { [AD1882_AUTO] = "auto", [AD1882_3STACK] = "3stack", [AD1882_6STACK] = "6stack", [AD1882_3STACK_AUTOMUTE] = "3stack-automute", };
At Fri, 8 Feb 2013 16:37:51 +0800, Raymond Yau wrote:
Treat vendor defined widget different from audio selector/audio
input
widget since hda_proc.c query the connection list of vendor
defined
widget
ADI codecs have vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
wide_type != AC_WID_VENDOR &&curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since there
is
no way to know the vendor defined widget support the verb
GET_CONNECT_SELECT
You should have written _this_ text at the first place! The biggest problem is that your description doesn't match with what you write in your patch.
But still, it's still an open question. In the specification, the use of selector isn't prohibited for vendor-spec widgets. So, some crazy h/w designer may think of it.
Yes, it depend on how you interept "Other"
7.3.3.2 Connection Select Control Applies to: Input Converter Selector Widget Pin Complex Other
Heh, and obviously h/w manufacturers thought vendor-spec widget belongs to "Other".
I think your patch is not necessary, what is missing are the codec responses when error occur on ad1882
-static const char * const ad1882_models[AD1986A_MODELS] = { +static const char * const ad1882_models[AD19882_MODELS] = { [AD1882_AUTO] = "auto", [AD1882_3STACK] = "3stack", [AD1882_6STACK] = "6stack", [AD1882_3STACK_AUTOMUTE] = "3stack-automute", };
More context please. Which response is missing?
Takashi
> > ADI codecs have vendor defined widgets ( BIAS Power-Down and
VREF
> Power-Down) which have connection list
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since
there
is
no way to know the vendor defined widget support the verb
GET_CONNECT_SELECT
You should have written _this_ text at the first place! The biggest problem is that your description doesn't match with what you write in your patch.
But still, it's still an open question. In the specification, the use of selector isn't prohibited for vendor-spec widgets. So, some crazy h/w designer may think of it.
Yes, it depend on how you interept "Other"
7.3.3.2 Connection Select Control Applies to: Input Converter Selector Widget Pin Complex Other
Heh, and obviously h/w manufacturers thought vendor-spec widget belongs to "Other".
http://git.kernel.org/?p=linux/kernel/git/tiwai/sound.git;a=commit;h=5fdaecd...
do you have the connection list in node 0x12 of ad1986a ?
If you prefer the patch to blacklist specific widgets similar to power widget
Refer to Table 13.Connection List of AD1882 dartasheet
23 A2209811 BC30AE24 NID 11 1 18 20 1 22 24 1 2E 30 1 3C 2F 00171514 NID 14 15 17
Refer to Table 13.Connection List of AD1988 dartasheet
23 25249811 2120BD38 Length 8 NID 11 1 18 24 25 38 1 3D 20 21 2F 15141211 00001716 Length 6 NID 11 12 14 15 16 17
Blacklist ADI codec's vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list for not sending AC_VERB_GET_CONNECT_SEL
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB && + (wide_type != AC_WID _VENDOR && ((codec->vendor_id >> 16) != 0x11d4)) && wid_type != AC_WID_POWER) curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
At Wed, 13 Feb 2013 11:16:15 +0800, Raymond Yau wrote:
[1 <text/plain; UTF-8 (7bit)>]
> > > > ADI codecs have vendor defined widgets ( BIAS Power-Down and
VREF
> > Power-Down) which have connection list
Node 0x2f [Vendor Defined Widget] wcaps 0xf00100: Mono Connection: 6 0x11* 0x12 0x14 0x15 0x16 0x17
My proposed patch just remove "*" from the connection list since
there
is
no way to know the vendor defined widget support the verb
GET_CONNECT_SELECT
You should have written _this_ text at the first place! The biggest problem is that your description doesn't match with what you write in your patch.
But still, it's still an open question. In the specification, the use of selector isn't prohibited for vendor-spec widgets. So, some crazy h/w designer may think of it.
Yes, it depend on how you interept "Other"
7.3.3.2 Connection Select Control Applies to: Input Converter Selector Widget Pin Complex Other
Heh, and obviously h/w manufacturers thought vendor-spec widget belongs to "Other".
http://git.kernel.org/?p=linux/kernel/git/tiwai/sound.git;a=commit;h=5fdaecd...
do you have the connection list in node 0x12 of ad1986a ?
If you prefer the patch to blacklist specific widgets similar to power widget
Refer to Table 13.Connection List of AD1882 dartasheet
23 A2209811 BC30AE24 NID 11 1 18 20 1 22 24 1 2E 30 1 3C 2F 00171514 NID 14 15 17
Refer to Table 13.Connection List of AD1988 dartasheet
23 25249811 2120BD38 Length 8 NID 11 1 18 24 25 38 1 3D 20 21 2F 15141211 00001716 Length 6 NID 11 12 14 15 16 17
Blacklist ADI codec's vendor defined widgets ( BIAS Power-Down and VREF Power-Down) which have connection list for not sending AC_VERB_GET_CONNECT_SEL
static void print_conn_list(struct snd_info_buffer *buffer, struct hda_codec *codec, hda_nid_t nid, unsigned int wid_type, hda_nid_t *conn, int conn_len) { int c, curr = -1;
if (conn_len > 1 && wid_type != AC_WID_AUD_MIX && wid_type != AC_WID_VOL_KNB &&
(wide_type != AC_WID _VENDOR && ((codec->vendor_id >> 16) != 0x11d4))&& wid_type != AC_WID_POWER) curr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONNECT_SEL, 0); snd_iprintf(buffer, " Connection: %d\n", conn_len);
Care to send a proper patch please?
thanks,
Takashi
participants (2)
-
Raymond Yau -
Takashi Iwai