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

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, September 27, 2016 4:46 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 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?

Devid != 0 is the same as devid = 0. And there is only pin/cvt connections.

My basic idea is:
1. Every device entry use a virtual pin
2. Each device entry will be initialized at bootup even it is not in MST mode.
    This means after bootup, mode change will not add/remove the per_pin.
3. All device entries under the same pin will use the same connection list.
    And the connection list is initialized at bootup. It will be saved in
    per_pin->mux_nids. This will never be changed after bootup.
    (However the pin-cvt connection is dynamically assigned when necessary.)

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

Do you mean codec->conn_list? If we don't use the dev_id and all
the device entries under the same pin use the same one, it will be
exactly the same as before.

Or do you mean the array of spec->pins? spec->pins is setup at bootup
and will not be changed later.

Regards,
Libin

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