On Fri, 21 Oct 2016 10:24:26 +0200, Yang, Libin wrote: > > Hi Takashi, > > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > > Sent: Thursday, October 20, 2016 11:34 PM > > To: libin.yang@xxxxxxxxxxxxxxx > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; Yang, Libin <libin.yang@xxxxxxxxx>; Lin, > > Mengdong <mengdong.lin@xxxxxxxxx> > > Subject: Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio > > support > > > > On Thu, 13 Oct 2016 09:49:36 +0200, > > libin.yang@xxxxxxxxxxxxxxx wrote: > > > > > > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx> > > > --- > > > 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. 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. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
