Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, September 27, 2016 4:46 PM 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 Tue, 27 Sep 2016 10:18:13 +0200, Yang, Libin wrote:
Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, September 27, 2016 3:59 PM 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 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?
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