> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Wednesday, November 18, 2015 7:31 PM > To: libin.yang@xxxxxxxxxxxxxxx > Cc: alsa-devel@xxxxxxxxxxxxxxxx; mengdong.lin@xxxxxxxxxxxxxxx; Yang, > Libin > Subject: Re: [RFC PATCH 2/2] ALSA: hda - hdmi dynamically > bind PCM to pin when monitor hotplug > > On Wed, 18 Nov 2015 09:46:59 +0100, > libin.yang@xxxxxxxxxxxxxxx wrote: > > > > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > Dynamically bind/unbind the PCM to pin when HDMI/DP monitor > hotplug. > > > > When monitor is connected, find a proper PCM for the monitor. > > If PCM is already open, set the correct configuration for the pin. > > > > When monitor is disconnected, unbind the PCM from the pin if > > PCM is not open. Otherwise unbind the PCM when pcm closes. > > Well, there are still too many logical changes in a single patch > although the code size itself isn't too big. > > Let's see: > > > > > Signed-off-by: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > --- > > sound/pci/hda/hda_codec.h | 1 + > > sound/pci/hda/patch_hdmi.c | 197 > ++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 188 insertions(+), 10 deletions(-) > > > > diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h > > index 373fcad..ee97401 100644 > > --- a/sound/pci/hda/hda_codec.h > > +++ b/sound/pci/hda/hda_codec.h > > @@ -167,6 +167,7 @@ enum { > > /* for PCM creation */ > > struct hda_pcm { > > char *name; > > + bool in_use; > > Now here is a new flag... > > > struct hda_pcm_stream stream[2]; > > unsigned int pcm_type; /* HDA_PCM_TYPE_XXX */ > > int device; /* device number to assign */ > > diff --git a/sound/pci/hda/patch_hdmi.c > b/sound/pci/hda/patch_hdmi.c > > index 12fc506..77d6701 100644 > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -72,6 +72,8 @@ struct hdmi_spec_per_cvt { > > > > struct hdmi_spec_per_pin { > > hda_nid_t pin_nid; > > + /* pin idx, different device entries on the same pin use the same > idx */ > > + int pin_nid_idx; > > And a new field... > > > int num_mux_nids; > > hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; > > int mux_idx; > > @@ -83,6 +85,7 @@ struct hdmi_spec_per_pin { > > struct delayed_work work; > > struct snd_kcontrol *eld_ctl; > > struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] > dynamically*/ > > + int pcm_idx; /* which pcm is attached. -1 means no pcm is > attached */ > > Yet a new one... > > > int repoll_count; > > bool setup; /* the stream has been set up by prepare callback */ > > int channels; /* current number of channels */ > > @@ -136,6 +139,8 @@ struct hdmi_spec { > > struct snd_array pins; /* struct hdmi_spec_per_pin */ > > struct hda_pcm *pcm_rec[16]; > > struct mutex pcm_lock; > > + unsigned long pcm_bitmap; > > + int pcm_used; /* counter of pcm_rec[] */ > > Two new fields. This already looks suspicious. Basically these > additions imply: > - the patch introduces a new usage flag per pin, > - the patch introduces a new index number for MST dev, > - the patch introduces a new index number for a PCM per pin, > - the patch introduces the new concept of PCM numbers, and > - the patch introduces some bitmap management. > > This makes hard to understand what this patch does; simply because it > does so many different things. > > The whole changes look reasonable to me in a quick glance, so we can > go in that direction. But let's organize the patches better. Thanks for review. I will split the patch into several ones. Regards, Libin > > > thanks, > > Takashi > > > unsigned int channels_max; /* max over all cvts */ > > > > struct hdmi_eld temp_eld; > > @@ -376,6 +381,20 @@ static int pin_nid_to_pin_index(struct > hda_codec *codec, hda_nid_t pin_nid) > > return -EINVAL; > > } > > > > +static int hinfo_to_pcm_index(struct hda_codec *codec, > > + struct hda_pcm_stream *hinfo) > > +{ > > + struct hdmi_spec *spec = codec->spec; > > + int pcm_idx; > > + > > + for (pcm_idx = 0; pcm_idx < spec->pcm_used; pcm_idx++) > > + if (get_pcm_rec(spec, pcm_idx)->stream == hinfo) > > + return pcm_idx; > > + > > + codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo); > > + return -EINVAL; > > +} > > + > > static int hinfo_to_pin_index(struct hda_codec *codec, > > struct hda_pcm_stream *hinfo) > > { > > @@ -1486,10 +1505,14 @@ static int hdmi_pcm_open_no_pin(struct > hda_pcm_stream *hinfo, > > { > > struct hdmi_spec *spec = codec->spec; > > struct snd_pcm_runtime *runtime = substream->runtime; > > - int cvt_idx, mux_idx; > > + int cvt_idx, pcm_idx, mux_idx; > > struct hdmi_spec_per_cvt *per_cvt = NULL; > > int err; > > > > + pcm_idx = hinfo_to_pcm_index(codec, hinfo); > > + if (pcm_idx < 0) > > + return -EINVAL; > > + > > err = hdmi_find_available_cvt(codec, &cvt_idx); > > if (err) > > return err; > > @@ -1497,11 +1520,11 @@ static int hdmi_pcm_open_no_pin(struct > hda_pcm_stream *hinfo, > > per_cvt = get_cvt(spec, cvt_idx); > > per_cvt->assigned = 1; > > hinfo->nid = per_cvt->cvt_nid; > > - > > mux_idx = intel_cvt_id_to_mux_idx(spec, per_cvt->cvt_nid); > > /* unshare converter from all pins */ > > intel_not_share_assigned_cvt(codec, 0, mux_idx); > > > > + spec->pcm_rec[pcm_idx]->in_use = true; > > /* todo: setup spdif ctls assign */ > > > > /* Initially set the converter's capabilities */ > > @@ -1531,13 +1554,17 @@ static int hdmi_pcm_open(struct > hda_pcm_stream *hinfo, > > { > > struct hdmi_spec *spec = codec->spec; > > struct snd_pcm_runtime *runtime = substream->runtime; > > - int pin_idx, cvt_idx, mux_idx = 0; > > + int pin_idx, cvt_idx, pcm_idx, mux_idx = 0; > > struct hdmi_spec_per_pin *per_pin; > > struct hdmi_eld *eld; > > struct hdmi_spec_per_cvt *per_cvt = NULL; > > int err; > > > > /* Validate hinfo */ > > + pcm_idx = hinfo_to_pcm_index(codec, hinfo); > > + if (pcm_idx < 0) > > + return -EINVAL; > > + > > mutex_lock(&spec->pcm_lock); > > pin_idx = hinfo_to_pin_index(codec, hinfo); > > if (!spec->dyn_pcm_assign) { > > @@ -1566,7 +1593,7 @@ static int hdmi_pcm_open(struct > hda_pcm_stream *hinfo, > > /* Claim converter */ > > per_cvt->assigned = 1; > > > > - > > + spec->pcm_rec[pcm_idx]->in_use = true; > > per_pin = get_pin(spec, pin_idx); > > per_pin->cvt_nid = per_cvt->cvt_nid; > > hinfo->nid = per_cvt->cvt_nid; > > @@ -1579,7 +1606,7 @@ static int hdmi_pcm_open(struct > hda_pcm_stream *hinfo, > > if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > > intel_not_share_assigned_cvt(codec, per_pin->pin_nid, > mux_idx); > > > > - snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); > > + snd_hda_spdif_ctls_assign(codec, pcm_idx, per_cvt->cvt_nid); > > > > /* Initially set the converter's capabilities */ > > hinfo->channels_min = per_cvt->channels_min; > > @@ -1596,7 +1623,7 @@ static int hdmi_pcm_open(struct > hda_pcm_stream *hinfo, > > !hinfo->rates || !hinfo->formats) { > > per_cvt->assigned = 0; > > hinfo->nid = 0; > > - snd_hda_spdif_ctls_unassign(codec, pin_idx); > > + snd_hda_spdif_ctls_unassign(codec, pcm_idx); > > mutex_unlock(&spec->pcm_lock); > > return -ENODEV; > > } > > @@ -1637,6 +1664,135 @@ static int hdmi_read_pin_conn(struct > hda_codec *codec, int pin_idx) > > return 0; > > } > > > > +static int get_preferred_pcm_slot(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + if ((spec->pcm_bitmap & (1 << per_pin->pin_nid_idx)) == 0) > > + return per_pin->pin_nid_idx; > > + return -1; > > +} > > + > > +static int hdmi_find_pcm_slot(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + int slot; > > + int i; > > + > > + /* try the prefer PCM */ > > + slot = get_preferred_pcm_slot(spec, per_pin); > > + if (slot != -1) > > + return slot; > > + > > + /* have a second try */ > > + for (i = spec->num_pins; i < spec->pcm_used; i++) { > > + if ((spec->pcm_bitmap & (1 << i)) == 0) > > + return i; > > + } > > + > > + /* the last try */ > > + for (i = 0; i < spec->pcm_used; i++) { > > + if ((spec->pcm_bitmap & (1 << i)) == 0) > > + return i; > > + } > > + return -ENODEV; > > +} > > + > > +static void hdmi_attach_hda_pcm(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + int idx; > > + > > + /* pcm already be attached to the pin */ > > + if (per_pin->pcm) > > + return; > > + idx = hdmi_find_pcm_slot(spec, per_pin); > > + if (idx == -ENODEV) > > + return; > > + per_pin->pcm_idx = idx; > > + per_pin->pcm = spec->pcm_rec[idx]; > > + set_bit(idx, &spec->pcm_bitmap); > > +} > > + > > +static void hdmi_detach_hda_pcm(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + int idx; > > + > > + /* pcm already be detached from the pin */ > > + if (!per_pin->pcm) > > + return; > > + idx = per_pin->pcm_idx; > > + per_pin->pcm_idx = -1; > > + per_pin->pcm = NULL; > > + if (idx >= 0 && idx < spec->pcm_used) > > + clear_bit(idx, &spec->pcm_bitmap); > > +} > > + > > +static int hdmi_get_pin_cvt_mux(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin, hda_nid_t cvt_nid) > > +{ > > + int mux_idx; > > + > > + for (mux_idx = 0; mux_idx < per_pin->num_mux_nids; > mux_idx++) > > + if (per_pin->mux_nids[mux_idx] == cvt_nid) > > + break; > > + return mux_idx; > > +} > > + > > +static bool check_non_pcm_per_cvt(struct hda_codec *codec, > hda_nid_t cvt_nid); > > +static void hdmi_pcm_setup_pin(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + struct hda_codec *codec = per_pin->codec; > > + struct hda_pcm *pcm; > > + struct hda_pcm_stream *hinfo; > > + struct snd_pcm_substream *substream; > > + > > + int mux_idx; > > + bool non_pcm; > > + > > + if (per_pin->pcm_idx >= 0 && per_pin->pcm_idx < spec- > >pcm_used) > > + pcm = spec->pcm_rec[per_pin->pcm_idx]; > > + else > > + return; > > + if (!pcm->in_use) > > + return; > > + > > + /* hdmi audio only uses playback and one substream */ > > + hinfo = pcm->stream; > > + substream = pcm->pcm->streams[0].substream; > > + > > + per_pin->cvt_nid = hinfo->nid; > > + > > + mux_idx = hdmi_get_pin_cvt_mux(spec, per_pin, hinfo->nid); > > + if (mux_idx < per_pin->num_mux_nids) > > + snd_hda_codec_write_cache(codec, per_pin->pin_nid, > 0, > > + AC_VERB_SET_CONNECT_SEL, > > + mux_idx); > > + snd_hda_spdif_ctls_assign(codec, per_pin->pcm_idx, hinfo->nid); > > + > > + non_pcm = check_non_pcm_per_cvt(codec, hinfo->nid); > > + if (substream->runtime) > > + per_pin->channels = substream->runtime->channels; > > + per_pin->setup = true; > > + per_pin->mux_idx = mux_idx; > > + > > + hdmi_setup_audio_infoframe(codec, per_pin, non_pcm); > > +} > > + > > +static void hdmi_pcm_reset_pin(struct hdmi_spec *spec, > > + struct hdmi_spec_per_pin *per_pin) > > +{ > > + if (per_pin->pcm_idx >= 0 && per_pin->pcm_idx < spec- > >pcm_used) > > + snd_hda_spdif_ctls_unassign(per_pin->codec, per_pin- > >pcm_idx); > > + > > + per_pin->chmap_set = false; > > + memset(per_pin->chmap, 0, sizeof(per_pin->chmap)); > > + > > + per_pin->setup = false; > > + per_pin->channels = 0; > > +} > > + > > static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, > int repoll) > > { > > struct hda_jack_tbl *jack; > > @@ -1661,6 +1817,7 @@ static bool hdmi_present_sense(struct > hdmi_spec_per_pin *per_pin, int repoll) > > snd_hda_power_up_pm(codec); > > present = snd_hda_pin_sense(codec, pin_nid); > > > > + mutex_lock(&spec->pcm_lock); > > mutex_lock(&per_pin->lock); > > pin_eld->monitor_present = !!(present & > AC_PINSENSE_PRESENCE); > > if (pin_eld->monitor_present) > > @@ -1694,6 +1851,16 @@ static bool hdmi_present_sense(struct > hdmi_spec_per_pin *per_pin, int repoll) > > } > > } > > > > + if (spec->dyn_pcm_assign) { > > + if (eld->eld_valid) { > > + hdmi_attach_hda_pcm(spec, per_pin); > > + hdmi_pcm_setup_pin(spec, per_pin); > > + } else { > > + hdmi_pcm_reset_pin(spec, per_pin); > > + hdmi_detach_hda_pcm(spec, per_pin); > > + } > > + } > > + > > if (pin_eld->eld_valid != eld->eld_valid) > > eld_changed = true; > > > > @@ -1744,6 +1911,7 @@ static bool hdmi_present_sense(struct > hdmi_spec_per_pin *per_pin, int repoll) > > jack->block_report = !ret; > > > > mutex_unlock(&per_pin->lock); > > + mutex_unlock(&spec->pcm_lock); > > snd_hda_power_down_pm(codec); > > return ret; > > } > > @@ -1789,6 +1957,11 @@ static int hdmi_add_pin(struct hda_codec > *codec, hda_nid_t pin_nid) > > > > per_pin->pin_nid = pin_nid; > > per_pin->non_pcm = false; > > + if (spec->dyn_pcm_assign) > > + per_pin->pcm_idx = -1; > > + else > > + per_pin->pcm_idx = pin_idx; > > + per_pin->pin_nid_idx = pin_idx; > > > > err = hdmi_read_pin_conn(codec, pin_idx); > > if (err < 0) > > @@ -1994,22 +2167,25 @@ static int hdmi_pcm_close(struct > hda_pcm_stream *hinfo, > > struct snd_pcm_substream *substream) > > { > > struct hdmi_spec *spec = codec->spec; > > - int cvt_idx, pin_idx; > > + int cvt_idx, pin_idx, pcm_idx; > > struct hdmi_spec_per_cvt *per_cvt; > > struct hdmi_spec_per_pin *per_pin; > > int pinctl; > > > > if (hinfo->nid) { > > + pcm_idx = hinfo_to_pcm_index(codec, hinfo); > > + if (snd_BUG_ON(pcm_idx < 0)) > > + return -EINVAL; > > cvt_idx = cvt_nid_to_cvt_index(codec, hinfo->nid); > > if (snd_BUG_ON(cvt_idx < 0)) > > return -EINVAL; > > per_cvt = get_cvt(spec, cvt_idx); > > - > > snd_BUG_ON(!per_cvt->assigned); > > per_cvt->assigned = 0; > > hinfo->nid = 0; > > > > mutex_lock(&spec->pcm_lock); > > + spec->pcm_rec[pcm_idx]->in_use = false; > > pin_idx = hinfo_to_pin_index(codec, hinfo); > > if (spec->dyn_pcm_assign && pin_idx < 0) { > > mutex_unlock(&spec->pcm_lock); > > @@ -2021,7 +2197,6 @@ static int hdmi_pcm_close(struct > hda_pcm_stream *hinfo, > > return -EINVAL; > > } > > per_pin = get_pin(spec, pin_idx); > > - > > if (spec->dyn_pin_out) { > > pinctl = snd_hda_codec_read(codec, per_pin- > >pin_nid, 0, > > > AC_VERB_GET_PIN_WIDGET_CONTROL, 0); > > @@ -2030,7 +2205,7 @@ static int hdmi_pcm_close(struct > hda_pcm_stream *hinfo, > > pinctl & ~PIN_OUT); > > } > > > > - snd_hda_spdif_ctls_unassign(codec, pin_idx); > > + snd_hda_spdif_ctls_unassign(codec, pcm_idx); > > > > mutex_lock(&per_pin->lock); > > per_pin->chmap_set = false; > > @@ -2228,6 +2403,7 @@ static int generic_hdmi_build_pcms(struct > hda_codec *codec) > > per_pin->pcm = info; > > } > > spec->pcm_rec[pin_idx] = info; > > + spec->pcm_used++; > > info->pcm_type = HDA_PCM_TYPE_HDMI; > > info->own_chmap = true; > > > > @@ -2275,6 +2451,7 @@ static int > generic_hdmi_build_controls(struct hda_codec *codec) > > > HDA_PCM_TYPE_HDMI); > > if (err < 0) > > return err; > > + /* pin number is the same with pcm number so far */ > > snd_hda_spdif_ctls_unassign(codec, pin_idx); > > > > /* add control for ELD Bytes */ > > -- > > 1.9.1 > > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
