[alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support
Takashi Iwai
tiwai at suse.de
Fri Oct 21 11:05:06 CEST 2016
On Fri, 21 Oct 2016 10:56:24 +0200,
Yang, Libin wrote:
>
>
>
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Friday, October 21, 2016 4:44 PM
> > To: Yang, Libin <libin.yang at intel.com>
> > Cc: libin.yang at linux.intel.com; alsa-devel at alsa-project.org; Lin, Mengdong
> > <mengdong.lin at intel.com>
> > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> > support
> >
> > On Fri, 21 Oct 2016 10:24:26 +0200,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > > Sent: Thursday, October 20, 2016 11:34 PM
> > > > To: libin.yang at linux.intel.com
> > > > Cc: alsa-devel at alsa-project.org; Yang, Libin <libin.yang at intel.com>;
> > > > Lin, Mengdong <mengdong.lin at intel.com>
> > > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst
> > > > audio support
> > > >
> > > > On Thu, 13 Oct 2016 09:49:36 +0200,
> > > > libin.yang at linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang at linux.intel.com>
> > > > >
> > > > > This patch adds the DP mst audio support.
> > > >
> > > > A bit more useful texts here please. I know you'll give more
> > > > explanations in the document in the later patch, but still something
> > > > better should be mentioned here.
> > >
> > > Yes, I will add more description here.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
> > > > > ---
> > > > > sound/pci/hda/hda_codec.c | 4 +
> > > > > sound/pci/hda/patch_hdmi.c | 243
> > > > > +++++++++++++++++++++++++++++++++++----------
> > > > > 2 files changed, 196 insertions(+), 51 deletions(-)
> > > > >
> > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > > > index caa2c9d..8a6b0a2 100644
> > > > > --- a/sound/pci/hda/hda_codec.c
> > > > > +++ b/sound/pci/hda/hda_codec.c
> > > > > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec
> > > > *codec)
> > > > > pin->nid = nid;
> > >
> > > ...
> > >
> > > > > +
> > > > >
> > > > > /* choose an unassigned converter. The conveters in the
> > > > > * connection list are in the same order as in the codec.
> > > > > @@ -1008,12 +1078,13 @@ static void
> > > > > intel_not_share_assigned_cvt(struct
> > > > hda_codec *codec,
> > > > > break;
> > > > > }
> > > > > }
> > > > > + snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > >
> > > > I guess this revert won't be reached when you break or continue the
> > > > loop beforehand?
> > >
> > > To make it easier to explain, please let me attach the full patched code here:
> > >
> > > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > > int dev_id_saved;
> > > int dev_num;
> > >
> > > per_pin = get_pin(spec, pin_idx);
> > > /*
> > > * pin not connected to monitor
> > > * no need to operate on it
> > > */
> > > if (!per_pin->pcm)
> > > continue;
> > >
> > > if ((per_pin->pin_nid == pin_nid) &&
> > > (per_pin->dev_id == dev_id))
> > > continue;
> > >
> > > /*
> > > * if per_pin->dev_id >= dev_num,
> > > * snd_hda_get_dev_select() will fail,
> > > * and the following operation is unpredictable.
> > > * So skip this situation.
> > > */
> > > dev_num = snd_hda_get_num_devices(codec, per_pin-
> > >pin_nid) + 1;
> > > if (per_pin->dev_id >= dev_num)
> > > continue;
> > >
> > > nid = per_pin->pin_nid;
> > >
> > > /*
> > > * Calling this function should not impact
> > > * on the device entry selection
> > > * So let's save the dev id for each pin,
> > > * and restore it when return
> > > */
> > > dev_id_saved = snd_hda_get_dev_select(codec, nid);
> > > tag 1 snd_hda_set_dev_select(codec, nid, per_pin->dev_id);
> > > curr = snd_hda_codec_read(codec, nid, 0,
> > > AC_VERB_GET_CONNECT_SEL, 0);
> > > if (curr != mux_idx) {
> > > tag 2 snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > continue;
> > > }
> > >
> > >
> > > /* choose an unassigned converter. The conveters in the
> > > * connection list are in the same order as in the codec.
> > > */
> > > for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> > > per_cvt = get_cvt(spec, cvt_idx);
> > > if (!per_cvt->assigned) {
> > > codec_dbg(codec,
> > > "choose cvt %d for pin nid %d\n",
> > > cvt_idx, nid);
> > > snd_hda_codec_write_cache(codec, nid, 0,
> > > AC_VERB_SET_CONNECT_SEL,
> > > cvt_idx);
> > > tag 3 break;
> > > }
> > > }
> > > snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > }
> > >
> > > We need revert dev_id only after we change the dev_id setting before.
> > > This is done by: snd_hda_set_dev_select(codec, nid, per_pn->dev_id)
> > > (tag1) There is one "continue" and we have reverted dev_id before
> > > continue (tag 2) There is one "break" (tag 3). However, this is for another
> > for loop.
> >
> > OK, fair enough.
> >
> >
> > > > 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.
Takashi
More information about the Alsa-devel
mailing list