Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Tuesday, September 27, 2016 4:46 PM > To: Yang, Libin <libin.yang@xxxxxxxxx> > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Lin, Mengdong > <mengdong.lin@xxxxxxxxx> > Subject: Re: [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@xxxxxxx] > > > Sent: Tuesday, September 27, 2016 3:59 PM > > > To: Yang, Libin <libin.yang@xxxxxxxxx> > > > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Lin, > > > Mengdong <mengdong.lin@xxxxxxxxx> > > > Subject: Re: [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@xxxxxxx] > > > > > Sent: Tuesday, September 27, 2016 1:51 AM > > > > > To: Yang, Libin <libin.yang@xxxxxxxxx> > > > > > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > > > > > Lin, Mengdong <mengdong.lin@xxxxxxxxx> > > > > > Subject: Re: [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@xxxxxxx] > > > > > > > Sent: Monday, September 26, 2016 11:11 PM > > > > > > > To: libin.yang@xxxxxxxxxxxxxxx > > > > > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Lin, Mengdong > > > > > > > <mengdong.lin@xxxxxxxxx>; Yang, Libin <libin.yang@xxxxxxxxx> > > > > > > > Subject: Re: [RFC PATCH 0/3] support DP MST > > > > > > > audio > > > > > > > > > > > > > > On Mon, 26 Sep 2016 10:35:35 +0200, > > > > > > > libin.yang@xxxxxxxxxxxxxxx > > > > > > > wrote: > > > > > > > > > > > > > > > > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > > > > > > > > > > > > > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
