[alsa-devel] [RFC PATCH 0/3] support DP MST audio

[Takashi Iwai]

On Tue, 27 Sep 2016 10:18:13 +0200,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, September 27, 2016 3:59 PM
> > To: Yang, Libin <libin.yang at intel.com>
> > Cc: libin.yang at linux.intel.com; alsa-devel at alsa-project.org; Lin, Mengdong
> > <mengdong.lin at intel.com>
> > Subject: Re: [alsa-devel] [RFC PATCH 0/3] support DP MST audio
> > 
> > On Tue, 27 Sep 2016 09:47:38 +0200,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > > Sent: Tuesday, September 27, 2016 1:51 AM
> > > > To: Yang, Libin <libin.yang at intel.com>
> > > > Cc: libin.yang at linux.intel.com; alsa-devel at alsa-project.org; Lin,
> > > > Mengdong <mengdong.lin at intel.com>
> > > > Subject: Re: [alsa-devel] [RFC PATCH 0/3] support DP MST audio
> > > >
> > > > On Mon, 26 Sep 2016 18:07:51 +0200,
> > > > Yang, Libin wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > > > > Sent: Monday, September 26, 2016 11:11 PM
> > > > > > To: libin.yang at linux.intel.com
> > > > > > Cc: alsa-devel at alsa-project.org; Lin, Mengdong
> > > > > > <mengdong.lin at intel.com>; Yang, Libin <libin.yang at intel.com>
> > > > > > Subject: Re: [alsa-devel] [RFC PATCH 0/3] support DP MST audio
> > > > > >
> > > > > > On Mon, 26 Sep 2016 10:35:35 +0200, libin.yang at linux.intel.com
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Libin Yang <libin.yang at linux.intel.com>
> > > > > > >
> > > > > > > This patchset starts to support DP MST audio.
> > > > > > >
> > > > > > > This patchset is based on drm-intel-nightly on
> > > > > > > git://anongit.freedesktop.org/drm-intel
> > > > > > >
> > > > > > > Libin Yang (3):
> > > > > > >   ALSA: hda - codec add DP MST support for connection list
> > > > > > >   ALSA: hda - add DP mst verb support
> > > > > > >   ALSA: hda - add DP mst audio support
> > > > > >
> > > > > > I read the patchset again, and I'm still not convinced enough by
> > > > > > this
> > > > change.
> > > > >
> > > > > Yes and I'm sorry that this patchset is delayed for a long time.
> > > > > Our gfx driver has been redesigned and some APIs have been changed.
> > > > > The change is done in Pandiyan, Dhinakaran's patches:
> > > > > Patchset: Prep. for DP audio MST support
> > > > >   drm/i915: Standardize port type for DVO encoders
> > > > >   drm/i915: Store port enum in intel_encoder
> > > > >   drm/i915: Switch to using port stored in intel_encoder
> > > > >   drm/i915: Move audio_connector to intel_encoder
> > > > >   drm/i915: start adding dp mst audio
> > > > > Patch: drm/i915/dp: DP audio API changes for MST
> > > > >
> > > > > >
> > > > > > The connection list is coded in the current way under the
> > > > > > assumption that connections are more or less static.  The
> > > > > > connections are cached in snd_array, which is only for growing
> > > > > > up, and not suitable for the entries that might be frequently
> > > > > > removed.  The removal is done only at overriding, and it happens only
> > once at boot.
> > > > > >
> > > > > > OTOH, with DP-MST case, the connection list is basically dynamic.
> > > > > > It may increase or decrease depending on the connected monitors...
> > > > > > Is my understanding correct?  Or is the DP-MST connection list
> > > > > > is static, too?  Then I do wonder how it covers the whole
> > > > > > connections with
> > > > arbitrary device indices.
> > > > >
> > > > > For the connection list, the driver will setup all the connection
> > > > > list at the beginning for each device entry, even it is non-MST.
> > > > > We statically allocate the connection list and will never remove them.
> > > >
> > > > Hrm, so how will it actually be?  Are the device indices fixed
> > > > through the whole operations, no matter which monitor is connected?
> > > > How many will they be?
> > > >
> > > > It'd be helpful if you give an example of the actual typical setup.
> > > > Then we can judge whether the proposed implementation is suitable.
> > >
> > > Even it is in non-MST mode, the connection list is setup with the
> > > per_pin initialization.  And as the device entries under the same pin
> > > have the same connection list, I use the device entry 0's connection
> > > list for other device entries. This avoids connection list is invalid
> > > for other device entries in non-MST. After initialization, the
> > > connection list will never change even the non-MST/MST mode is changed.
> > 
> > The devid=0 case is fine.  This is identical with the normal widget connections.
> > But you wrote that these connections are static even for devid!=0.  Is it
> > correct?
> > 
> > If so, now my question again: for devid!=0, which connections are given
> > statically?  In other words, how many devid would be passed?
> 
> For the connection list, it is static. It will be initialized at bootup.

So this is only about devid=0, right?  For devid!=0, there wouldn't be
anything else than pin/cvt connections, correct?

> For the pin and cvt actual connection, it is dynamically assigned.

... but you still want to cache this to an (ever-growing) array for
the standard connection lists.  That's what I was concerned.

> In hdmi_pcm_open(), it will call hdmi_choose_cvt() to pick up
> a cvt (based on the connection list and the cvt is used or not).
> Actually, I think the connection list is only useful when picking
> up the cvt (or other nodes). Please correct me if I'm wrong.

Yes, since HDMI/DP codec has only pretty simple connections.

> When we try to connect device entry 2, pin 1 to cvt A, for example,
> we will call snd_hda_set_dev_select(pin_nid, dev_id) to select the
> device entry on the pin. And then we will use the verb
> AC_VERB_SET_CONNECT_SEL to setup the connection as before.

OK, that sounds straightforward.  So we just need to set up these
verbs at opening the device (and likely at suspend/resume, too).


> > > > > > So, the connection cache management is one thing.  Another thing
> > > > > > is that the patchset doesn't consider about the pin ELD notification.
> > > > > > Basically we switched to ELD notification instead of the pin
> > > > > > unsol event for Intel chips.  How does it fit?
> > > > >
> > > > > For the ELD notification, in the patch 0003, if there is monitor
> > > > > hotplug, gfx driver will notify audio driver with acomp. Gfx
> > > > > driver will tell which port (pin), pipe (device entry) occurs the
> > > > > hotplug event. And then audio driver will call
> > > > > snd_hdac_acomp_get_eld() to get the
> > > > eld information.
> > > >
> > > > OK, then drop the patch 1 and try to implement without messing
> > > > around the connection list.  The patch 1 is what I really don't like
> > > > from the beginning.  It makes things complicated.
> > >
> > > As the connection list is the same for the device entries under same
> > > pin, what do you think if we use the original APIs, ignoring the
> > > device id. This will not change the connection list code or be a very small
> > change.
> > >
> > > If you agree on that, I will have a try on it.
> > >
> > > On the same time, I will check with our silicon if my understanding
> > > "the connection list is the same for the device entries under same pin"
> > > is right.
> > >
> > > > Is there anything else than haswell fixup that touches the
> > > > connection list with the dev id?  If it's the only place, there can be an
> > alternative way to hack it.
> > >
> > > If we always use the device entry 0 as all the device entries under
> > > the same pin, I think we can ignore the dev id now.
> > 
> > Well, it is only about the pin/cvt connection inside HDMI/DP codec, and it
> > should be easily covered in a different way even with devid!=0, I suppose.
> > That said, the patchset looks way too complex than necessary.
> > 
> > Though, it might be a good idea to start with devid=0 assumption if it makes
> > the changes simpler.
> 
> Yes, the patches are too complex for this simple usage. I will try to see if it 
> works without touching the connection list core.

Yes, please.  Let's keep KISS principle.


thanks,

Takashi