Re: [RFC PATCH 0/3] support DP MST audio

[Takashi Iwai <tiwai@suse.de>]

On Tue, 27 Sep 2016 09:47:38 +0200,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, September 27, 2016 1:51 AM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> > <mengdong.lin@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@suse.de]
> > > > Sent: Monday, September 26, 2016 11:11 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong
> > > > <mengdong.lin@intel.com>; Yang, Libin <libin.yang@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@linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang@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?

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


Takashi