[alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support
Yang, Libin
libin.yang at intel.com
Fri Oct 21 10:24:26 CEST 2016
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.
>
> 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.
Regards,
Libin
>
> Takashi
>
> > }
> > }
> >
> > /* A wrapper of intel_not_share_asigned_cvt() */ static void
> > intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
> > - hda_nid_t pin_nid, hda_nid_t cvt_nid)
> > + hda_nid_t pin_nid, int dev_id, hda_nid_t cvt_nid)
> > {
> > int mux_idx;
> > struct hdmi_spec *spec = codec->spec; @@ -1025,7 +1096,7 @@
> static
> > void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
> > */
> > mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
> > if (mux_idx >= 0)
> > - intel_not_share_assigned_cvt(codec, pin_nid, mux_idx);
> > + intel_not_share_assigned_cvt(codec, pin_nid, dev_id,
> mux_idx);
> > }
> >
> > /* skeleton caller of pin_cvt_fixup ops */ @@ -1140,6 +1211,7 @@
> > static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
> > per_pin->cvt_nid = per_cvt->cvt_nid;
> > hinfo->nid = per_cvt->cvt_nid;
> >
More information about the Alsa-devel
mailing list