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? > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
