[alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb support

[Takashi Iwai]

On Fri, 25 Mar 2016 03:26:06 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Thursday, March 24, 2016 5:19 PM
> > To: Yang, Libin
> > Cc: libin.yang at linux.intel.com; alsa-devel at 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 at suse.de]
> > > > Sent: Tuesday, March 22, 2016 7:17 PM
> > > > To: libin.yang at linux.intel.com
> > > > Cc: alsa-devel at 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 at linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang at 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 at 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