Hi Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, March 22, 2016 7:28 PM To: libin.yang@linux.intel.com Cc: alsa-devel@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@linux.intel.com wrote:
From: Libin Yang libin.yang@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*/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*/- /* 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*/@@ -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 whenreturn */
dev_id_saved = snd_hda_get_dev_select(codec, nid);snd_hda_set_dev_select(codec, nid, per_pin->dev_id);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_tcvt_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);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 mstpin_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*/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 deviceentries
* 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