Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Monday, October 24, 2016 4:41 PM > To: Yang, Libin <libin.yang@xxxxxxxxx> > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Lin, Mengdong > <mengdong.lin@xxxxxxxxx> > Subject: Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio > support > > On Mon, 24 Oct 2016 09:09:07 +0200, > Yang, Libin wrote: > > > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > > > Sent: Friday, October 21, 2016 8:33 PM > > > To: Yang, Libin <libin.yang@xxxxxxxxx> > > > Cc: libin.yang@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Lin, > > > Mengdong <mengdong.lin@xxxxxxxxx> > > > Subject: Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst > > > audio support > > > > > > On Fri, 21 Oct 2016 14:25:28 +0200, > > > Yang, Libin wrote: > > > > > > > > > > > > > Also, this rewrite changes the loop completely, so we > > > > > > > > > you should update the comment appropriately. It's no > > > > > > > > > longer scanning the all pins (including unused ones). > > > > > > > > > And I'm not 100% sure whether it's OK to change in that > > > > > > > > > way; the function was written to do that (touching the > > > > > > > > > unused pin as well) > > > on purpose, IIRC. > > > > > > > > > > > > > > > > > > Hm, or is it OK because we are listing always the three pins? > > > > > > > > > More explanation please, either in comments or in the > changelog. > > > > > > > > > > > > > > > > > > > > > > > > > This function is a little complex now. Please let me explain it in > detail. > > > > > > > > > > > > > > > > As you know, we need this function because we need fix: > > > > > > > > same cvt is connecting to several pins. In this situation, > > > > > > > > playback on one pin will also impact on other pins. Let's > > > > > > > > assume cvt 1 are connecting to pin 1, 2, > > > > > > > 3. > > > > > > > > When we playback on pin1, we need call this function to > > > > > > > > make sure pin 2, 3 is connected to other cvts. > > > > > > > > > > > > > > > > In MST mode, we are using dynamic pcm. If no pcm is > > > > > > > > attached to the pin, it means there is no monitor connected to > the pin. > > > > > > > > So we don't need to set the cvt. Let's still assume the > > > > > > > > cvt 1 are connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 > > > > > > > > (here pin is the virtual > > > pin). > > > > > > > > > > > > > > > > Let's assume playback on pin 1 (which is connected to cvt 1). > > > > > > > > So we should make sure all others pins are not connected to cvt1. > > > > > > > > > > > > > > > > 1) For other pins: > > > > > > > > if (!per_pin->pcm) > > > > > > > > continue; > > > > > > > > This means no monitors is connected to the pin. So we > > > > > > > > don't care the cvt connection because the pin doesn't > > > > > > > > output sound to > > > monitor. > > > > > > > > (when the virtual pin is connected to monitor later, this > > > > > > > > function will be called in present_sense(), which will > > > > > > > > make sure the cvt connected to the virtual pin is > > > > > > > > different from the cvt connected to pin 1) > > > > > > > > > > > > > > > > 2) For the following situation: > > > > > > > > dev_num = snd_hda_get_num_devices(codec, > > > > > > > > per_pin->pin_nid) > > > + 1; > > > > > > > > if (per_pin->dev_id >= dev_num) > > > > > > > > continue; > > > > > > > > The per_pin->dev_id is greater than dev_num. This means no > > > > > > > > such virtual pin. We should skip this situation. > > > > > > > > > > > > > > > > 3) For the following situation: > > > > > > > > if (curr != mux_idx) { > > > > > > > > snd_hda_set_dev_select(codec, nid, dev_id_saved); > > > > > > > > continue; > > > > > > > > } > > > > > > > > It means the cvt connected to the virtual pin is already > > > > > > > > different to the cvt connected to the pin 1. So we don't > > > > > > > > need to do > > > anything. > > > > > > > > > > > > > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the > cvt. > > > > > > > > > > > > > > > > I will add more description for this function in the next version > patch. > > > > > > > > > > > > > > One thing that is still unclear to me is how to deal with > > > > > > > the unconfigured > > > > > pins. > > > > > > > I thought that the pins with port=non pincfg are still > > > > > > > ignored in > > > > > > > hdmi_add_pin() even after your patch. So they won't be > > > > > > > listed in the pins / nids arrays. > > > > > > > > > > > > Actually, MST code doesn't touch port=non code. It exists before. > > > > > > > > > > > > My understanding of this code is that port=non is HW > > > > > > connection related. If this pin is not connected to any jack, > > > > > > it means it is not lined out and the pin is useless for the special > machine. > > > > > > > > > > > > > > > > > > > > Now, your new code only loops over the enumerated pins, i.e. > > > > > > > these unconfigured pins are ignored. What if these pins are > > > > > > > connected to the active converters? I thought excluding > > > > > > > such a connection is one of the purposes of this function, > > > > > > > judging from the > > > function description. > > > > > > > > > > > > If the pin is not lined out, no monitor will be connected to the pin. > > > > > > So even the cvt is connected to the pin, it will impact nothing. > > > > > > For MST mode, if it is not lined out, there will be never pcm > > > > > > attached to the pin. This means you will never have chance to > > > > > > operate the cvt > > > > > connected to the pin. > > > > > > Besides, I found if there is no monitor connected to the pin, > > > > > > the connection list is not actually connecting to any cvt for the pin. > > > > > > > > > > But why the current function explicitly states that it does care > > > > > about the unused pins? There must be a reason to do so. I > > > > > vaguely remember that it was mentioned like that connecting to > > > > > the unused pin causing some unexpected behavior. > > > > > > > > I don't know the story. Maybe in this situation, mute/unmute will > > > > be messed. I will check with our internal team to see if someone > > > > knows this reason. > > > > > > Mengdong? > > > > The patch of checking AC_JACK_PORT_NONE is from Stephen Warren, who > is > > from nvidia. I have checked with Mengdong, and she didn't know the > > patch. But it seems to be similar with intel_not_share_xxx() function. > > Well, ignoring the unused port there is basically irrelevant with the behavior > in intel_not_share_*(). It was introduced in commit > 116dcde638065a6b94344ca570e1e79c8e857826 > ALSA: HDA: Remove unconnected PCM devices for Intel HDMI > > Meanwhile intel_not_share_*() stuff was introduced much later in commit > 7ef166b831237e67b2ea83ce0c933c46ddd6eb26 > ALSA: hda - Avoid choose same converter for unused pins Get it. It is used to reduce the user confusion. Thanks. Regards, Libin > > > Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
