| Message ID | 1458549476-77040-2-git-send-email-libin.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 21 Mar 2016 09:37:54 +0100, libin.yang@linux.intel.com wrote: > > From: Libin Yang <libin.yang@linux.intel.com> > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() functions > for DP MST audio support. > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > --- > sound/pci/hda/hda_codec.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--- > sound/pci/hda/hda_codec.h | 3 ++ > 2 files changed, 82 insertions(+), 4 deletions(-) > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > index d17e38e..db94a66 100644 > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux, > } > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > - > -/* return DEVLIST_LEN parameter of the given widget */ > -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) > +/** > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the given widget > + * @codec: the HDA codec > + * @nid: NID of the pin to parse > + * > + * Get the device entry number on the given widget. > + * This is a feature of DP MST audio. Each pin can > + * have several device entries in it. > + */ > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid) > { > unsigned int wcaps = get_wcaps(codec, nid); > unsigned int parm; > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) > parm = 0; > return parm & AC_DEV_LIST_LEN_MASK; > } > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > /** > * snd_hda_get_devices - copy device list without cache > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, > unsigned int parm; > int i, dev_len, devices; > > - parm = get_num_devices(codec, nid); > + parm = snd_hda_get_num_devices(codec, nid); > if (!parm) /* not multi-stream capable */ > return 0; > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, > return devices; > } > > +/** > + * snd_hda_get_dev_select - get device entry select on the pin > + * @codec: the HDA codec > + * @nid: NID of the pin to get device entry select > + * > + * Get the devcie entry select on the pin. Return the device entry > + * id selected on the pin. Return 0 means the first device entry > + * is selected or MST is not supported. > + */ > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid) > +{ > + /* not support dp_mst will always return 0, using first dev_entry */ > + if (!codec->dp_mst) > + return 0; > + > + return snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_DEVICE_SEL, 0); > +} > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > + > +/** > + * snd_hda_set_dev_select - set device entry select on the pin > + * @codec: the HDA codec > + * @nid: NID of the pin to set device entry select > + * @dev_id: device entry id to be set > + * > + * Set the device entry select on the pin nid. > + */ > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id) > +{ > + int ret, dev, num_devices; > + unsigned long timeout; > + > + /* not support dp_mst will always return 0, using first dev_entry */ > + if (!codec->dp_mst) > + return 0; > + > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > + num_devices = snd_hda_get_num_devices(codec, nid); > + /* If Device List Length is 0, the pin is not multi stream capable. > + */ > + if (num_devices == 0) > + return 0; > + > + /* Behavior of setting index being equal to or greater than > + * Device List Length is not predictable > + */ > + if (num_devices + 1 <= dev_id) > + return 0; Is this correct? Doesn't num_devices=1 mean that there is only a single device? If dev_id=1 must be invalid, but the check above passes. The standard form of such index check is like if (dev_id >= num_devices) error; And this also leads to the question. Shouldn't we return an error if an invalid dev_id is given? > + > + timeout = jiffies + msecs_to_jiffies(500); > + > + /* make sure the device entry selection takes effect */ > + do { > + ret = snd_hda_codec_write(codec, nid, 0, > + AC_VERB_SET_DEVICE_SEL, dev_id); > + udelay(20); > + > + dev = snd_hda_get_dev_select(codec, nid); > + if (dev == dev_id) > + break; > + msleep(20); > + } while (time_before(jiffies, timeout)); Is this loop needed? Most verbs take effect immediately, and so far, the only exception is the power state. Takashi
Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Tuesday, March 22, 2016 7:17 PM > To: libin.yang@linux.intel.com > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb > support > > On Mon, 21 Mar 2016 09:37:54 +0100, > libin.yang@linux.intel.com wrote: > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() > functions > > for DP MST audio support. > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > --- > > sound/pci/hda/hda_codec.c | 83 > ++++++++++++++++++++++++++++++++++++++++++++--- > > sound/pci/hda/hda_codec.h | 3 ++ > > 2 files changed, 82 insertions(+), 4 deletions(-) > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > > index d17e38e..db94a66 100644 > > --- a/sound/pci/hda/hda_codec.c > > +++ b/sound/pci/hda/hda_codec.c > > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct > hda_codec *codec, hda_nid_t mux, > > } > > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > > > - > > -/* return DEVLIST_LEN parameter of the given widget */ > > -static unsigned int get_num_devices(struct hda_codec *codec, > hda_nid_t nid) > > +/** > > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the > given widget > > + * @codec: the HDA codec > > + * @nid: NID of the pin to parse > > + * > > + * Get the device entry number on the given widget. > > + * This is a feature of DP MST audio. Each pin can > > + * have several device entries in it. > > + */ > > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, > hda_nid_t nid) > > { > > unsigned int wcaps = get_wcaps(codec, nid); > > unsigned int parm; > > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct > hda_codec *codec, hda_nid_t nid) > > parm = 0; > > return parm & AC_DEV_LIST_LEN_MASK; > > } > > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > > > /** > > * snd_hda_get_devices - copy device list without cache > > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec > *codec, hda_nid_t nid, > > unsigned int parm; > > int i, dev_len, devices; > > > > - parm = get_num_devices(codec, nid); > > + parm = snd_hda_get_num_devices(codec, nid); > > if (!parm) /* not multi-stream capable */ > > return 0; > > > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct hda_codec > *codec, hda_nid_t nid, > > return devices; > > } > > > > +/** > > + * snd_hda_get_dev_select - get device entry select on the pin > > + * @codec: the HDA codec > > + * @nid: NID of the pin to get device entry select > > + * > > + * Get the devcie entry select on the pin. Return the device entry > > + * id selected on the pin. Return 0 means the first device entry > > + * is selected or MST is not supported. > > + */ > > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid) > > +{ > > + /* not support dp_mst will always return 0, using first dev_entry > */ > > + if (!codec->dp_mst) > > + return 0; > > + > > + return snd_hda_codec_read(codec, nid, 0, > AC_VERB_GET_DEVICE_SEL, 0); > > +} > > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > > + > > +/** > > + * snd_hda_set_dev_select - set device entry select on the pin > > + * @codec: the HDA codec > > + * @nid: NID of the pin to set device entry select > > + * @dev_id: device entry id to be set > > + * > > + * Set the device entry select on the pin nid. > > + */ > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, > int dev_id) > > +{ > > + int ret, dev, num_devices; > > + unsigned long timeout; > > + > > + /* not support dp_mst will always return 0, using first dev_entry > */ > > + if (!codec->dp_mst) > > + return 0; > > + > > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > > + num_devices = snd_hda_get_num_devices(codec, nid); > > + /* If Device List Length is 0, the pin is not multi stream capable. > > + */ > > + if (num_devices == 0) > > + return 0; > > + > > + /* Behavior of setting index being equal to or greater than > > + * Device List Length is not predictable > > + */ > > + if (num_devices + 1 <= dev_id) > > + return 0; > > Is this correct? Doesn't num_devices=1 mean that there is only a > single device? If dev_id=1 must be invalid, but the check above > passes. The standard form of such index check is like num_device is the return value of the verb AC_PAR_DEVLIST_LEN. device number is AC_PAR_DEVLIST_LEN + 1. From the spec: Device List Length is a 0 based integer value indicating the number of sink device that a multi stream capable Digital Display Pin Widget can support. If Device List Length is value is 0, there is only one sink device connection possible indicating the Pin Widget is not multi stream capable, and there is no Device Select control And dev_id is 0 based. If dev_id = 1, it means set the second device entry, and num_device + 1 (the real number of device entries) must be equal to or greater than 2. (num_device + 1 > dev_id ) So (num_device + 1 <= dev_id) is wrong situation. > > if (dev_id >= num_devices) > error; > > And this also leads to the question. Shouldn't we return an error if > an invalid dev_id is given? I'm thinking if the wrong dev_id is given, we just leave as it is before. I will return the -EINVAL in next version. > > > + > > + timeout = jiffies + msecs_to_jiffies(500); > > + > > + /* make sure the device entry selection takes effect */ > > + do { > > + ret = snd_hda_codec_write(codec, nid, 0, > > + AC_VERB_SET_DEVICE_SEL, dev_id); > > + udelay(20); > > + > > + dev = snd_hda_get_dev_select(codec, nid); > > + if (dev == dev_id) > > + break; > > + msleep(20); > > + } while (time_before(jiffies, timeout)); > > Is this loop needed? Most verbs take effect immediately, and so far, > the only exception is the power state. OK, I will remove the loop. Regards, Libin > > > Takashi
On Thu, 24 Mar 2016 09:44:32 +0100, Yang, Libin wrote: > > Hi Takashi, > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Tuesday, March 22, 2016 7:17 PM > > To: libin.yang@linux.intel.com > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb > > support > > > > On Mon, 21 Mar 2016 09:37:54 +0100, > > libin.yang@linux.intel.com wrote: > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() > > functions > > > for DP MST audio support. > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > --- > > > sound/pci/hda/hda_codec.c | 83 > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > sound/pci/hda/hda_codec.h | 3 ++ > > > 2 files changed, 82 insertions(+), 4 deletions(-) > > > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > > > index d17e38e..db94a66 100644 > > > --- a/sound/pci/hda/hda_codec.c > > > +++ b/sound/pci/hda/hda_codec.c > > > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct > > hda_codec *codec, hda_nid_t mux, > > > } > > > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > > > > > - > > > -/* return DEVLIST_LEN parameter of the given widget */ > > > -static unsigned int get_num_devices(struct hda_codec *codec, > > hda_nid_t nid) > > > +/** > > > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the > > given widget > > > + * @codec: the HDA codec > > > + * @nid: NID of the pin to parse > > > + * > > > + * Get the device entry number on the given widget. > > > + * This is a feature of DP MST audio. Each pin can > > > + * have several device entries in it. > > > + */ > > > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, > > hda_nid_t nid) > > > { > > > unsigned int wcaps = get_wcaps(codec, nid); > > > unsigned int parm; > > > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct > > hda_codec *codec, hda_nid_t nid) > > > parm = 0; > > > return parm & AC_DEV_LIST_LEN_MASK; > > > } > > > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > > > > > /** > > > * snd_hda_get_devices - copy device list without cache > > > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec > > *codec, hda_nid_t nid, > > > unsigned int parm; > > > int i, dev_len, devices; > > > > > > - parm = get_num_devices(codec, nid); > > > + parm = snd_hda_get_num_devices(codec, nid); > > > if (!parm) /* not multi-stream capable */ > > > return 0; > > > > > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct hda_codec > > *codec, hda_nid_t nid, > > > return devices; > > > } > > > > > > +/** > > > + * snd_hda_get_dev_select - get device entry select on the pin > > > + * @codec: the HDA codec > > > + * @nid: NID of the pin to get device entry select > > > + * > > > + * Get the devcie entry select on the pin. Return the device entry > > > + * id selected on the pin. Return 0 means the first device entry > > > + * is selected or MST is not supported. > > > + */ > > > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid) > > > +{ > > > + /* not support dp_mst will always return 0, using first dev_entry > > */ > > > + if (!codec->dp_mst) > > > + return 0; > > > + > > > + return snd_hda_codec_read(codec, nid, 0, > > AC_VERB_GET_DEVICE_SEL, 0); > > > +} > > > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > > > + > > > +/** > > > + * snd_hda_set_dev_select - set device entry select on the pin > > > + * @codec: the HDA codec > > > + * @nid: NID of the pin to set device entry select > > > + * @dev_id: device entry id to be set > > > + * > > > + * Set the device entry select on the pin nid. > > > + */ > > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, > > int dev_id) > > > +{ > > > + int ret, dev, num_devices; > > > + unsigned long timeout; > > > + > > > + /* not support dp_mst will always return 0, using first dev_entry > > */ > > > + if (!codec->dp_mst) > > > + return 0; > > > + > > > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > > > + num_devices = snd_hda_get_num_devices(codec, nid); > > > + /* If Device List Length is 0, the pin is not multi stream capable. > > > + */ > > > + if (num_devices == 0) > > > + return 0; > > > + > > > + /* Behavior of setting index being equal to or greater than > > > + * Device List Length is not predictable > > > + */ > > > + if (num_devices + 1 <= dev_id) > > > + return 0; > > > > Is this correct? Doesn't num_devices=1 mean that there is only a > > single device? If dev_id=1 must be invalid, but the check above > > passes. The standard form of such index check is like > > num_device is the return value of the verb AC_PAR_DEVLIST_LEN. > device number is AC_PAR_DEVLIST_LEN + 1. > > >From the spec: > Device List Length is a 0 based integer value indicating the number > of sink device that a multi stream capable Digital Display Pin > Widget can support. If Device List Length is value is 0, there is > only one sink device connection possible indicating the Pin Widget > is not multi stream capable, and there is no Device Select control > > And dev_id is 0 based. > > If dev_id = 1, it means set the second device entry, and > num_device + 1 (the real number of device entries) > must be equal to or greater than 2. > (num_device + 1 > dev_id ) > > So (num_device + 1 <= dev_id) is wrong situation. If so, the term "num_devices" is counter-intuitive. It should be corrected to the natural value, i.e. based on 1, or renamed to a different one. Readers would assume that the number of devices is 1 if there is a single element, not 0. If this isn't true, it just confuses readers. > > > > if (dev_id >= num_devices) > > error; > > > > And this also leads to the question. Shouldn't we return an error if > > an invalid dev_id is given? > > I'm thinking if the wrong dev_id is given, we just leave as it is before. > I will return the -EINVAL in next version. IMO, it's safer to return an error. Since non-zero dev_id is only for MST, the code path that passes such a value must handle MST properly, i.e. not through the default code path. > > > + > > > + timeout = jiffies + msecs_to_jiffies(500); > > > + > > > + /* make sure the device entry selection takes effect */ > > > + do { > > > + ret = snd_hda_codec_write(codec, nid, 0, > > > + AC_VERB_SET_DEVICE_SEL, dev_id); > > > + udelay(20); > > > + > > > + dev = snd_hda_get_dev_select(codec, nid); > > > + if (dev == dev_id) > > > + break; > > > + msleep(20); > > > + } while (time_before(jiffies, timeout)); > > > > Is this loop needed? Most verbs take effect immediately, and so far, > > the only exception is the power state. > > OK, I will remove the loop. Well, I don't mean to remove forcibly. Just want to be sure. If such a loop is needed practically, we must have it, of course. Let's check. thanks, Takashi
Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Thursday, March 24, 2016 5:19 PM > To: Yang, Libin > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, > Mengdong > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb > support > > On Thu, 24 Mar 2016 09:44:32 +0100, > Yang, Libin wrote: > > > > Hi Takashi, > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Tuesday, March 22, 2016 7:17 PM > > > To: libin.yang@linux.intel.com > > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin > > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst > verb > > > support > > > > > > On Mon, 21 Mar 2016 09:37:54 +0100, > > > libin.yang@linux.intel.com wrote: > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() > > > functions > > > > for DP MST audio support. > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > > --- > > > > sound/pci/hda/hda_codec.c | 83 > > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > > sound/pci/hda/hda_codec.h | 3 ++ > > > > 2 files changed, 82 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/sound/pci/hda/hda_codec.c > b/sound/pci/hda/hda_codec.c > > > > index d17e38e..db94a66 100644 > > > > --- a/sound/pci/hda/hda_codec.c > > > > +++ b/sound/pci/hda/hda_codec.c > > > > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct > > > hda_codec *codec, hda_nid_t mux, > > > > } > > > > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > > > > > > > - > > > > -/* return DEVLIST_LEN parameter of the given widget */ > > > > -static unsigned int get_num_devices(struct hda_codec *codec, > > > hda_nid_t nid) > > > > +/** > > > > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the > > > given widget > > > > + * @codec: the HDA codec > > > > + * @nid: NID of the pin to parse > > > > + * > > > > + * Get the device entry number on the given widget. > > > > + * This is a feature of DP MST audio. Each pin can > > > > + * have several device entries in it. > > > > + */ > > > > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, > > > hda_nid_t nid) > > > > { > > > > unsigned int wcaps = get_wcaps(codec, nid); > > > > unsigned int parm; > > > > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct > > > hda_codec *codec, hda_nid_t nid) > > > > parm = 0; > > > > return parm & AC_DEV_LIST_LEN_MASK; > > > > } > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > > > > > > > /** > > > > * snd_hda_get_devices - copy device list without cache > > > > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec > > > *codec, hda_nid_t nid, > > > > unsigned int parm; > > > > int i, dev_len, devices; > > > > > > > > - parm = get_num_devices(codec, nid); > > > > + parm = snd_hda_get_num_devices(codec, nid); > > > > if (!parm) /* not multi-stream capable */ > > > > return 0; > > > > > > > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct > hda_codec > > > *codec, hda_nid_t nid, > > > > return devices; > > > > } > > > > > > > > +/** > > > > + * snd_hda_get_dev_select - get device entry select on the pin > > > > + * @codec: the HDA codec > > > > + * @nid: NID of the pin to get device entry select > > > > + * > > > > + * Get the devcie entry select on the pin. Return the device entry > > > > + * id selected on the pin. Return 0 means the first device entry > > > > + * is selected or MST is not supported. > > > > + */ > > > > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t > nid) > > > > +{ > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > */ > > > > + if (!codec->dp_mst) > > > > + return 0; > > > > + > > > > + return snd_hda_codec_read(codec, nid, 0, > > > AC_VERB_GET_DEVICE_SEL, 0); > > > > +} > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > > > > + > > > > +/** > > > > + * snd_hda_set_dev_select - set device entry select on the pin > > > > + * @codec: the HDA codec > > > > + * @nid: NID of the pin to set device entry select > > > > + * @dev_id: device entry id to be set > > > > + * > > > > + * Set the device entry select on the pin nid. > > > > + */ > > > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t > nid, > > > int dev_id) > > > > +{ > > > > + int ret, dev, num_devices; > > > > + unsigned long timeout; > > > > + > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > */ > > > > + if (!codec->dp_mst) > > > > + return 0; > > > > + > > > > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > > > > + num_devices = snd_hda_get_num_devices(codec, nid); > > > > + /* If Device List Length is 0, the pin is not multi stream capable. > > > > + */ > > > > + if (num_devices == 0) > > > > + return 0; > > > > + > > > > + /* Behavior of setting index being equal to or greater than > > > > + * Device List Length is not predictable > > > > + */ > > > > + if (num_devices + 1 <= dev_id) > > > > + return 0; > > > > > > Is this correct? Doesn't num_devices=1 mean that there is only a > > > single device? If dev_id=1 must be invalid, but the check above > > > passes. The standard form of such index check is like > > > > num_device is the return value of the verb AC_PAR_DEVLIST_LEN. > > device number is AC_PAR_DEVLIST_LEN + 1. > > > > >From the spec: > > Device List Length is a 0 based integer value indicating the number > > of sink device that a multi stream capable Digital Display Pin > > Widget can support. If Device List Length is value is 0, there is > > only one sink device connection possible indicating the Pin Widget > > is not multi stream capable, and there is no Device Select control > > > > And dev_id is 0 based. > > > > If dev_id = 1, it means set the second device entry, and > > num_device + 1 (the real number of device entries) > > must be equal to or greater than 2. > > (num_device + 1 > dev_id ) > > > > So (num_device + 1 <= dev_id) is wrong situation. > > If so, the term "num_devices" is counter-intuitive. It should be > corrected to the natural value, i.e. based on 1, or renamed to a > different one. Readers would assume that the number of devices is 1 > if there is a single element, not 0. If this isn't true, it just > confuses readers. Get it. So what do you think if using: if (dev_id > num_device) //both is 0 based. Regards, Libin > > > > > > > > if (dev_id >= num_devices) > > > error; > > > > > > And this also leads to the question. Shouldn't we return an error if > > > an invalid dev_id is given? > > > > I'm thinking if the wrong dev_id is given, we just leave as it is before. > > I will return the -EINVAL in next version. > > IMO, it's safer to return an error. Since non-zero dev_id is only for > MST, the code path that passes such a value must handle MST properly, > i.e. not through the default code path. > > > > > > + > > > > + timeout = jiffies + msecs_to_jiffies(500); > > > > + > > > > + /* make sure the device entry selection takes effect */ > > > > + do { > > > > + ret = snd_hda_codec_write(codec, nid, 0, > > > > + AC_VERB_SET_DEVICE_SEL, dev_id); > > > > + udelay(20); > > > > + > > > > + dev = snd_hda_get_dev_select(codec, nid); > > > > + if (dev == dev_id) > > > > + break; > > > > + msleep(20); > > > > + } while (time_before(jiffies, timeout)); > > > > > > Is this loop needed? Most verbs take effect immediately, and so far, > > > the only exception is the power state. > > > > OK, I will remove the loop. > > Well, I don't mean to remove forcibly. Just want to be sure. > If such a loop is needed practically, we must have it, of course. > Let's check. > > > thanks, > > Takashi
On Fri, 25 Mar 2016 03:26:06 +0100, Yang, Libin wrote: > > Hi Takashi, > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Thursday, March 24, 2016 5:19 PM > > To: Yang, Libin > > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, > > Mengdong > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb > > support > > > > On Thu, 24 Mar 2016 09:44:32 +0100, > > Yang, Libin wrote: > > > > > > Hi Takashi, > > > > > > > -----Original Message----- > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > Sent: Tuesday, March 22, 2016 7:17 PM > > > > To: libin.yang@linux.intel.com > > > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin > > > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst > > verb > > > > support > > > > > > > > On Mon, 21 Mar 2016 09:37:54 +0100, > > > > libin.yang@linux.intel.com wrote: > > > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() > > > > functions > > > > > for DP MST audio support. > > > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > > > --- > > > > > sound/pci/hda/hda_codec.c | 83 > > > > ++++++++++++++++++++++++++++++++++++++++++++--- > > > > > sound/pci/hda/hda_codec.h | 3 ++ > > > > > 2 files changed, 82 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/sound/pci/hda/hda_codec.c > > b/sound/pci/hda/hda_codec.c > > > > > index d17e38e..db94a66 100644 > > > > > --- a/sound/pci/hda/hda_codec.c > > > > > +++ b/sound/pci/hda/hda_codec.c > > > > > @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct > > > > hda_codec *codec, hda_nid_t mux, > > > > > } > > > > > EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); > > > > > > > > > > - > > > > > -/* return DEVLIST_LEN parameter of the given widget */ > > > > > -static unsigned int get_num_devices(struct hda_codec *codec, > > > > hda_nid_t nid) > > > > > +/** > > > > > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the > > > > given widget > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to parse > > > > > + * > > > > > + * Get the device entry number on the given widget. > > > > > + * This is a feature of DP MST audio. Each pin can > > > > > + * have several device entries in it. > > > > > + */ > > > > > +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, > > > > hda_nid_t nid) > > > > > { > > > > > unsigned int wcaps = get_wcaps(codec, nid); > > > > > unsigned int parm; > > > > > @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct > > > > hda_codec *codec, hda_nid_t nid) > > > > > parm = 0; > > > > > return parm & AC_DEV_LIST_LEN_MASK; > > > > > } > > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); > > > > > > > > > > /** > > > > > * snd_hda_get_devices - copy device list without cache > > > > > @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec > > > > *codec, hda_nid_t nid, > > > > > unsigned int parm; > > > > > int i, dev_len, devices; > > > > > > > > > > - parm = get_num_devices(codec, nid); > > > > > + parm = snd_hda_get_num_devices(codec, nid); > > > > > if (!parm) /* not multi-stream capable */ > > > > > return 0; > > > > > > > > > > @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct > > hda_codec > > > > *codec, hda_nid_t nid, > > > > > return devices; > > > > > } > > > > > > > > > > +/** > > > > > + * snd_hda_get_dev_select - get device entry select on the pin > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to get device entry select > > > > > + * > > > > > + * Get the devcie entry select on the pin. Return the device entry > > > > > + * id selected on the pin. Return 0 means the first device entry > > > > > + * is selected or MST is not supported. > > > > > + */ > > > > > +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t > > nid) > > > > > +{ > > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > > */ > > > > > + if (!codec->dp_mst) > > > > > + return 0; > > > > > + > > > > > + return snd_hda_codec_read(codec, nid, 0, > > > > AC_VERB_GET_DEVICE_SEL, 0); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); > > > > > + > > > > > +/** > > > > > + * snd_hda_set_dev_select - set device entry select on the pin > > > > > + * @codec: the HDA codec > > > > > + * @nid: NID of the pin to set device entry select > > > > > + * @dev_id: device entry id to be set > > > > > + * > > > > > + * Set the device entry select on the pin nid. > > > > > + */ > > > > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t > > nid, > > > > int dev_id) > > > > > +{ > > > > > + int ret, dev, num_devices; > > > > > + unsigned long timeout; > > > > > + > > > > > + /* not support dp_mst will always return 0, using first dev_entry > > > > */ > > > > > + if (!codec->dp_mst) > > > > > + return 0; > > > > > + > > > > > + /* AC_PAR_DEVLIST_LEN is 0 based. */ > > > > > + num_devices = snd_hda_get_num_devices(codec, nid); > > > > > + /* If Device List Length is 0, the pin is not multi stream capable. > > > > > + */ > > > > > + if (num_devices == 0) > > > > > + return 0; > > > > > + > > > > > + /* Behavior of setting index being equal to or greater than > > > > > + * Device List Length is not predictable > > > > > + */ > > > > > + if (num_devices + 1 <= dev_id) > > > > > + return 0; > > > > > > > > Is this correct? Doesn't num_devices=1 mean that there is only a > > > > single device? If dev_id=1 must be invalid, but the check above > > > > passes. The standard form of such index check is like > > > > > > num_device is the return value of the verb AC_PAR_DEVLIST_LEN. > > > device number is AC_PAR_DEVLIST_LEN + 1. > > > > > > >From the spec: > > > Device List Length is a 0 based integer value indicating the number > > > of sink device that a multi stream capable Digital Display Pin > > > Widget can support. If Device List Length is value is 0, there is > > > only one sink device connection possible indicating the Pin Widget > > > is not multi stream capable, and there is no Device Select control > > > > > > And dev_id is 0 based. > > > > > > If dev_id = 1, it means set the second device entry, and > > > num_device + 1 (the real number of device entries) > > > must be equal to or greater than 2. > > > (num_device + 1 > dev_id ) > > > > > > So (num_device + 1 <= dev_id) is wrong situation. > > > > If so, the term "num_devices" is counter-intuitive. It should be > > corrected to the natural value, i.e. based on 1, or renamed to a > > different one. Readers would assume that the number of devices is 1 > > if there is a single element, not 0. If this isn't true, it just > > confuses readers. > > Get it. So what do you think if using: > > if (dev_id > num_device) //both is 0 based. num_device is still a confusing name. If you really want to keep it zero-based, use max_device_id or such. Or, just return +1 from *_get_num_devices() instead. Takashi
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index d17e38e..db94a66 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -325,9 +325,16 @@ int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux, } EXPORT_SYMBOL_GPL(snd_hda_get_conn_index); - -/* return DEVLIST_LEN parameter of the given widget */ -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) +/** + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the given widget + * @codec: the HDA codec + * @nid: NID of the pin to parse + * + * Get the device entry number on the given widget. + * This is a feature of DP MST audio. Each pin can + * have several device entries in it. + */ +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid) { unsigned int wcaps = get_wcaps(codec, nid); unsigned int parm; @@ -341,6 +348,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) parm = 0; return parm & AC_DEV_LIST_LEN_MASK; } +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices); /** * snd_hda_get_devices - copy device list without cache @@ -358,7 +366,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, unsigned int parm; int i, dev_len, devices; - parm = get_num_devices(codec, nid); + parm = snd_hda_get_num_devices(codec, nid); if (!parm) /* not multi-stream capable */ return 0; @@ -382,6 +390,73 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, return devices; } +/** + * snd_hda_get_dev_select - get device entry select on the pin + * @codec: the HDA codec + * @nid: NID of the pin to get device entry select + * + * Get the devcie entry select on the pin. Return the device entry + * id selected on the pin. Return 0 means the first device entry + * is selected or MST is not supported. + */ +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid) +{ + /* not support dp_mst will always return 0, using first dev_entry */ + if (!codec->dp_mst) + return 0; + + return snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_DEVICE_SEL, 0); +} +EXPORT_SYMBOL_GPL(snd_hda_get_dev_select); + +/** + * snd_hda_set_dev_select - set device entry select on the pin + * @codec: the HDA codec + * @nid: NID of the pin to set device entry select + * @dev_id: device entry id to be set + * + * Set the device entry select on the pin nid. + */ +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id) +{ + int ret, dev, num_devices; + unsigned long timeout; + + /* not support dp_mst will always return 0, using first dev_entry */ + if (!codec->dp_mst) + return 0; + + /* AC_PAR_DEVLIST_LEN is 0 based. */ + num_devices = snd_hda_get_num_devices(codec, nid); + /* If Device List Length is 0, the pin is not multi stream capable. + */ + if (num_devices == 0) + return 0; + + /* Behavior of setting index being equal to or greater than + * Device List Length is not predictable + */ + if (num_devices + 1 <= dev_id) + return 0; + + timeout = jiffies + msecs_to_jiffies(500); + + /* make sure the device entry selection takes effect */ + do { + ret = snd_hda_codec_write(codec, nid, 0, + AC_VERB_SET_DEVICE_SEL, dev_id); + udelay(20); + + dev = snd_hda_get_dev_select(codec, nid); + if (dev == dev_id) + break; + msleep(20); + } while (time_before(jiffies, timeout)); + + return ret; +} +EXPORT_SYMBOL_GPL(snd_hda_set_dev_select); + /* * read widget caps for each widget and store in cache */ diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 4852c1a..57d1659 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -348,8 +348,11 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int dev_id, int nums, const hda_nid_t *list); int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux, hda_nid_t nid, int dev_id, int recursive); +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid); int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, u8 *dev_list, int max_devices); +int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid); +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id); struct hda_verb { hda_nid_t nid;