[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: 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 (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