[alsa-devel] [RFC PATCH v2 3/3] ALSA - hda: add DP MST support
Yang, Libin
libin.yang at intel.com
Thu Mar 24 09:57:05 CET 2016
Hi Takashi,
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, March 22, 2016 7:28 PM
> To: libin.yang at linux.intel.com
> Cc: alsa-devel at alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [RFC PATCH v2 3/3] ALSA - hda: add DP MST
> support
>
> On Mon, 21 Mar 2016 09:37:56 +0100,
> libin.yang at linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang at linux.intel.com>
> >
> > This patch adds the DP MST support in hdmi audio driver.
> > ---
> > sound/pci/hda/hda_codec.c | 3 +
> > sound/pci/hda/patch_hdmi.c | 179
> ++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 133 insertions(+), 49 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index db94a66..d4be769 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -492,6 +492,9 @@ static int read_pin_defaults(struct hda_codec
> *codec)
> > pin->nid = nid;
> > pin->cfg = snd_hda_codec_read(codec, nid, 0,
> >
> AC_VERB_GET_CONFIG_DEFAULT, 0);
> > + /* all device entries are the same widget control so far
> > + * fixme: if any codec is different, need fix here
> > + */
> > pin->ctrl = snd_hda_codec_read(codec, nid, 0,
> >
> AC_VERB_GET_PIN_WIDGET_CONTROL,
> > 0);
> > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > index 5987a31..67e191c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -144,7 +144,20 @@ struct hdmi_spec {
> > struct snd_array cvts; /* struct hdmi_spec_per_cvt */
> > hda_nid_t cvt_nids[4]; /* only for haswell fix */
> >
> > + /* num_pins is the number of virtual pins
> > + * for example, there are 3 pins, and each pin
> > + * has 4 device entries, then the num_pins is 12
> > + */
> > int num_pins;
> > + /* num_nids is the number of real pins
> > + * In the above example, num_nids is 3
> > + */
> > + int num_nids;
> > + /* dev_num is the number of device entries
> > + * on each pin.
> > + * In the above example, dev_num is 4
> > + */
> > + int dev_num;
> > struct snd_array pins; /* struct hdmi_spec_per_pin */
> > struct hdmi_pcm pcm_rec[16];
> > struct mutex pcm_lock;
> > @@ -1248,6 +1261,10 @@ static void
> hdmi_setup_audio_infoframe(struct hda_codec *codec,
> >
> > static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin,
> int repoll);
> >
> > +/* todo:
> > + * To fully support DP MST, check_presence_and_report need param
> dev_id
> > + * to tell which device entry occurs monitor connection event
> > + */
> > static void check_presence_and_report(struct hda_codec *codec,
> hda_nid_t nid)
> > {
> > struct hdmi_spec *spec = codec->spec;
> > @@ -1271,6 +1288,11 @@ static void hdmi_intrinsic_event(struct
> hda_codec *codec, unsigned int res)
> > struct hda_jack_tbl *jack;
> > int dev_entry = (res & AC_UNSOL_RES_DE) >>
> AC_UNSOL_RES_DE_SHIFT;
> >
> > + /* assume DP MST uses dyn_pcm_assign and acomp and
> > + * never comes here
> > + * if DP MST supports unsol event, below code need
> > + * consider dev_entry
> > + */
> > jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
> > if (!jack)
> > return;
> > @@ -1499,28 +1521,42 @@ static int intel_cvt_id_to_mux_idx(struct
> hdmi_spec *spec,
> > * by any other pins.
> > */
> > static void intel_not_share_assigned_cvt(struct hda_codec *codec,
> > - hda_nid_t pin_nid, int mux_idx)
> > + hda_nid_t pin_nid,
> > + int dev_id, int mux_idx)
> > {
> > struct hdmi_spec *spec = codec->spec;
> > hda_nid_t nid;
> > int cvt_idx, curr;
> > struct hdmi_spec_per_cvt *per_cvt;
> > + struct hdmi_spec_per_pin *per_pin;
> > + int pin_idx;
> >
> > /* configure all pins, including "no physical connection" ones */
> > - for_each_hda_codec_node(nid, codec) {
> > - unsigned int wid_caps = get_wcaps(codec, nid);
> > - unsigned int wid_type = get_wcaps_type(wid_caps);
> > + for (pin_idx = 0; pin_idx <= spec->num_pins; pin_idx++) {
>
> Is the condition correct? i.e. pin_idx = spec->num_pins is valid?
You are right. My code is wrong.
>
>
> > + int dev_id_saved;
> >
> > - if (wid_type != AC_WID_PIN)
> > + per_pin = get_pin(spec, pin_idx);
> > + /* pin not connected to monitor
> > + * no need to operate on it
> > + */
> > + if (!per_pin->pcm)
> > continue;
> >
> > - if (nid == pin_nid)
> > + if ((per_pin->pin_nid == pin_nid) &&
> > + (per_pin->dev_id == dev_id))
> > continue;
> >
> > + nid = per_pin->pin_nid;
> > +
> > + /* save the dev id for echo pin, and restore it when
> return */
> > + dev_id_saved = snd_hda_get_dev_select(codec, nid);
> > + 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)
> > + if (curr != mux_idx) {
> > + 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.
> > @@ -1537,12 +1573,13 @@ static void
> intel_not_share_assigned_cvt(struct hda_codec *codec,
> > break;
> > }
> > }
> > + snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > }
> > }
> >
> > /* 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;
> > @@ -1557,7 +1594,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);
> > }
> >
> > /* called in hdmi_pcm_open when no pin is assigned to the PCM
> > @@ -1585,7 +1622,7 @@ static int hdmi_pcm_open_no_pin(struct
> hda_pcm_stream *hinfo,
> > per_cvt->assigned = 1;
> > hinfo->nid = per_cvt->cvt_nid;
> >
> > - intel_not_share_assigned_cvt_nid(codec, 0, per_cvt->cvt_nid);
> > + intel_not_share_assigned_cvt_nid(codec, 0, 0, per_cvt->cvt_nid);
> >
> > set_bit(pcm_idx, &spec->pcm_in_use);
> > /* todo: setup spdif ctls assign */
> > @@ -1661,13 +1698,15 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> > per_pin->cvt_nid = per_cvt->cvt_nid;
> > hinfo->nid = per_cvt->cvt_nid;
> >
> > + snd_hda_set_dev_select(codec, per_pin->pin_nid, per_pin-
> >dev_id);
> > snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
> > AC_VERB_SET_CONNECT_SEL,
> > mux_idx);
> >
> > /* configure unused pins to choose other converters */
> > if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> > - intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> mux_idx);
> > + intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> > + per_pin->dev_id, mux_idx);
> >
> > snd_hda_spdif_ctls_assign(codec, pcm_idx, per_cvt->cvt_nid);
> >
> > @@ -1737,13 +1776,13 @@ static int hdmi_find_pcm_slot(struct
> hdmi_spec *spec,
> > return per_pin->pin_nid_idx;
> >
> > /* have a second try; check the "reserved area" over num_pins
> */
> > - for (i = spec->num_pins; i < spec->pcm_used; i++) {
> > + for (i = spec->num_nids; i < spec->pcm_used; i++) {
> > if (!test_bit(i, &spec->pcm_bitmap))
> > return i;
> > }
> >
> > /* the last try; check the empty slots in pins */
> > - for (i = 0; i < spec->num_pins; i++) {
> > + for (i = 0; i < spec->num_nids; i++) {
> > if (!test_bit(i, &spec->pcm_bitmap))
> > return i;
> > }
> > @@ -1818,10 +1857,13 @@ static void hdmi_pcm_setup_pin(struct
> hdmi_spec *spec,
> > 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)
> > + if (mux_idx < per_pin->num_mux_nids) {
> > + snd_hda_set_dev_select(codec, per_pin->pin_nid,
> > + per_pin->dev_id);
> > 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);
> > @@ -1902,7 +1944,7 @@ static void update_eld(struct hda_codec
> *codec,
> > if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> > intel_verify_pin_cvt_connect(codec, per_pin);
> > intel_not_share_assigned_cvt(codec, per_pin-
> >pin_nid,
> > - per_pin->mux_idx);
> > + per_pin->dev_id, per_pin->mux_idx);
> > }
> >
> > hdmi_setup_audio_infoframe(codec, per_pin, per_pin-
> >non_pcm);
> > @@ -1996,6 +2038,7 @@ static struct snd_jack
> *pin_idx_to_jack(struct hda_codec *codec,
> > if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> > jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > else if (!spec->dyn_pcm_assign) {
> > + //jack tbl not support mst
> > jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> >pin_nid);
> > if (jack_tbl)
> > jack = jack_tbl->jack;
> > @@ -2013,6 +2056,9 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
> > int size;
> >
> > mutex_lock(&per_pin->lock);
> > + /* todo:
> > + * to fully support DP MST, need add param dev_id
> > + */
> > size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin-
> >pin_nid,
> > per_pin->dev_id, &eld-
> >monitor_present,
> > eld->eld_buffer, ELD_MAX_SIZE);
> > @@ -2079,7 +2125,7 @@ static void hdmi_repoll_eld(struct
> work_struct *work)
> > }
> >
> > static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
> > - hda_nid_t nid);
> > + hda_nid_t nid, int dev_id);
> >
> > static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> > {
> > @@ -2088,38 +2134,59 @@ static int hdmi_add_pin(struct hda_codec
> *codec, hda_nid_t pin_nid)
> > int pin_idx;
> > struct hdmi_spec_per_pin *per_pin;
> > int err;
> > + int dev_num, i;
> >
> > caps = snd_hda_query_pin_caps(codec, pin_nid);
> > if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))
> > return 0;
> >
> > + /* For DP MST audio, Configuration Default is the same for
> > + * all device entries on the same pin
> > + */
> > config = snd_hda_codec_get_pincfg(codec, pin_nid);
> > if (get_defcfg_connect(config) == AC_JACK_PORT_NONE)
> > return 0;
> >
> > - if (is_haswell_plus(codec))
> > - intel_haswell_fixup_connect_list(codec, pin_nid);
> > -
> > - pin_idx = spec->num_pins;
> > - per_pin = snd_array_new(&spec->pins);
> > - if (!per_pin)
> > - return -ENOMEM;
> > -
> > - 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 = get_hdmi_pcm(spec, pin_idx);
> > - per_pin->pcm_idx = pin_idx;
> > + if (is_haswell_plus(codec)) {
> > + dev_num = 3;
> > + spec->dev_num = 3;
> > + } else {
> > + dev_num = snd_hda_get_num_devices(codec, pin_nid)
> + 1;
>
> Does snd_hda_get_num_devices() return 0? What if it's no mst-capable
> codec, and what if a MST-capable codec having a single device?
If it is no mst-capable, it will return 0. It means only one device is support.
Based on my testing, if I connect MST hub on the port, and connect more
than 1 monitor on the hub, the value will be 2, which means it supports
3 device entries. Otherwise, it will return 0. I'm not sure this is the same
on all other platforms. So I only use the workaround on HSW+.
>
>
> > + /* spec->dev_num is the maxinum number of device
> entries
> > + * among all the pins
> > + */
> > + spec->dev_num = (spec->dev_num > dev_num) ?
> > + spec->dev_num : dev_num;
> > }
>
> The presence of is_haswell_*() or such macro in every place makes
> really hard to maintain the code. In my new patchset (I'll post soon
> later), I tried to reduce these usages, and split the functions to
> Intel-specific ones. Let's try not to contaminate too much again.
I see. I will modify the code based on your change.
Regards,
Libin
>
>
> Takashi
More information about the Alsa-devel
mailing list