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

[Takashi Iwai]

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.


> > 
> > 	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