[alsa-devel] [RFC PATCH 2/2] ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug
Yang, Libin
libin.yang at intel.com
Wed Nov 18 15:43:59 CET 2015
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Wednesday, November 18, 2015 7:31 PM
> To: libin.yang at linux.intel.com
> Cc: alsa-devel at alsa-project.org; mengdong.lin at linux.intel.com; Yang,
> Libin
> Subject: Re: [alsa-devel] [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 at linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang at linux.intel.com>
> >
> > 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 at linux.intel.com>
> > ---
> > 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
> >
More information about the Alsa-devel
mailing list