Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, October 24, 2016 4:41 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 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@suse.de] Sent: Friday, October 21, 2016 8:33 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 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