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

[Yang, Libin]

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.

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