On Tue, 03 Nov 2015 09:22:52 +0100, libin.yang@linux.intel.com wrote:

From: Libin Yang libin.yang@intel.com

Signed-off-by: Libin Yang libin.yang@intel.com

include/sound/hdaudio.h | 1 + 1 file changed, 1 insertion(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index e2b712c..1f54964 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -15,6 +15,7 @@

/* codec node id */ typedef u16 hda_nid_t; +typedef u8 hda_dev_t;

This isn't used in any ABI, so we don't have to define the type. Just use int instead.

NID was defined there because it's used in ABI and it can represent the purpose more clearly. (Also at the early time of driver writing, the size itself wasn't 100% clear; we started writing it before HD-audio spec got solid.)

thanks,

Takashi

On 11/04/2015 12:44 AM, Takashi Iwai wrote:

On Tue, 03 Nov 2015 09:22:53 +0100, libin.yang@linux.intel.com wrote:

...

From: Libin Yang libin.yang@intel.com

Allocate the PCM based on the number of pin and device entry. The number of PCM is pin num plus device entry number per pin. For example, on Intel platform, it will be 5 PCMs.

Do not attach the PCM to pin in initialization. Dynamically attach PCM to pin when monitor is connected in HDMI audio codec driver.

When monitor is disconnected, detach the PCM from the pin. And if the audio is playing, stop the playback.

The below is the example of device entry and PCM binding: For a monitor is plugged in, we need to dynamically assign this monitor to 5 PCM devices (on Intel platform): For a monitor at pin nid 0x05, dev index 0, it will prefer PCM 3 For a monitor at pin nid 0x06, dev index 0, it will prefer PCM 7 For a monitor at pin nid 0x07, dev index 0, it will prefer PCM 8 For a monitor at dev index 1 (any pin), it will prefer PCM 9 For a monitor at dev index 2 (any pin), it will prefer PCM 10

If the preferred PCM is not available, try PCM in this order: 9, 10, 3, 7 ,8.

Signed-off-by: Libin Yang libin.yang@linux.intel.com

We should split the changes to a few patches here. For example, stopping the stream at discussion is basically an implementation independent from the MST itself. IOW, the dynamic attach/detach can be implemented and tested even without MST support. Let's code them at first, then add MST support.

This patch actually doesn't depend on MST. I use the dev_num to determine how many PCMs to be created.

To Split the patch, how about I create the PCM number based the pin number in the patch?

Also, it's an open question whether we apply the dynamic attach/detach to all devices that use the generic hdmi framework. It means including AMD and Nvidia. You hard-coded it to be applied unconditionally. Would it break anything potentially...?

I have no other verdors platforms to test. But it seems to be OK on other platforms. The impact is, i think, it will stop the stream when monitor is disconnected. Should we apply the code by judging whehter it is Intel platform?

Some more comments below.

...

---

sound/pci/hda/hda_codec.c | 5 +- sound/pci/hda/hda_codec.h | 1 + sound/pci/hda/patch_hdmi.c | 170 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 162 insertions(+), 14 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 8374188..e1d4648 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -313,7 +313,7 @@ EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);

/* return DEVLIST_LEN parameter of the given widget */ -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid) { unsigned int wcaps = get_wcaps(codec, nid); unsigned int parm; @@ -327,6 +327,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) parm = 0; return parm & AC_DEV_LIST_LEN_MASK; } +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices);

Once when exporting, add the proper kernel doc comment, too. I guess this can be split as a preparation patch, too.

OK, I will add the kernel doc comment. And put it in a preparation patch.

...

/**

• snd_hda_get_devices - copy device list without cache

@@ -344,7 +345,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, unsigned int parm; int i, dev_len, devices;

• parm = get_num_devices(codec, nid);

• parm = snd_hda_get_num_devices(codec, nid); if (!parm) /* not multi-stream capable */ return 0;

diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 373fcad..ad4da41 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -347,6 +347,7 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums, const hda_nid_t *list); int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux, hda_nid_t nid, int recursive); +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid); int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, u8 *dev_list, int max_devices);

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index f503a88..8d31366 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -72,6 +72,9 @@ struct hdmi_spec_per_cvt {

struct hdmi_spec_per_pin { hda_nid_t pin_nid;

• hda_dev_t dev_id;
• /* real pin idx. device entries on same pin share same pin_nid_idx */
• int pin_nid_idx; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; int mux_idx;

@@ -82,6 +85,9 @@ struct hdmi_spec_per_pin { struct mutex lock; struct delayed_work work; struct snd_kcontrol *eld_ctl;

• struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] */
• int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */
• bool pcm_detaching; int repoll_count; bool setup; /* the stream has been set up by prepare callback */ int channels; /* current number of channels */

@@ -134,6 +140,10 @@ struct hdmi_spec { int num_pins; struct snd_array pins; /* struct hdmi_spec_per_pin */ struct hda_pcm *pcm_rec[16];

• unsigned long pcm_bitmap;

• struct mutex pcm_lock;

• int pcm_used; /* counter of pcm_rec[] */

• int dev_num; unsigned int channels_max; /* max over all cvts */

  struct hdmi_eld temp_eld;

@@ -378,12 +388,18 @@ static int hinfo_to_pin_index(struct hda_codec *codec, struct hda_pcm_stream *hinfo) { struct hdmi_spec *spec = codec->spec;

• struct hdmi_spec_per_pin *per_pin; int pin_idx;

• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)

• if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
  

• mutex_lock(&spec->pcm_lock);
• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {

• per_pin = get_pin(spec, pin_idx);
  

• if (per_pin->pcm && per_pin->pcm->stream == hinfo) {
  

• 	mutex_unlock(&spec->pcm_lock);
  return pin_idx;
  

• }
  

• }
• mutex_unlock(&spec->pcm_lock); codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo); return -EINVAL; }

@@ -1451,7 +1467,8 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,

/* Validate hinfo */ pin_idx = hinfo_to_pin_index(codec, hinfo);

• if (snd_BUG_ON(pin_idx < 0))

• /* if no pin is attached, return fail */
• if (pin_idx < 0) return -EINVAL; per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;

@@ -1529,6 +1546,102 @@ 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 (per_pin->dev_id == 0) {

• 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 slot_offset;
• int i;
• /* try the prefer PCM */
• slot = get_preferred_pcm_slot(spec, per_pin);
• if (slot != -1)

• return slot;
  

• /* have a second try, try backup PCMs from specified offset */
• if (per_pin->dev_id == 0)

• slot_offset = 0;
  

• else

• slot_offset = per_pin->dev_id - 1;
  

• for (i = spec->num_pins + slot_offset; i < spec->pcm_used; i++) {

• if ((spec->pcm_bitmap & (1 << i)) == 0)
  

• 	return i;
  

• }

IMO, this step can be skipped and just check through the backup PCMs.

This step is used for the situation (for example, on Intel platform): if the device entry id is 1, it will prefer PCM 9; if the device entry id is 2, it will prefer PCM 10;

OK, I will remove the second try.

...

• /* have a third try, try all backup PCMs */
• for (i = spec->num_pins; i < spec->pcm_used; i++) {

• if ((spec->pcm_bitmap & (1 << i)) == 0)
  

• 	return i;
  

• }
• /* the last try, try all PCMs */
• 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;
  

• mutex_lock(&spec->pcm_lock);
• idx = hdmi_find_pcm_slot(spec, per_pin);
• if (idx == -ENODEV) {

• mutex_unlock(&spec->pcm_lock);
  

• return;
  

• }
• per_pin->pcm_idx = idx;
• per_pin->pcm = spec->pcm_rec[idx];
• set_bit(idx, &spec->pcm_bitmap);
• mutex_unlock(&spec->pcm_lock);

+}

+static void hdmi_detach_hda_pcm(struct hdmi_spec *spec,

• 		struct hdmi_spec_per_pin *per_pin)
  

+{

• int idx;
• struct snd_pcm_substream *substream;
• /* pcm already be detached from the pin */
• if (!per_pin->pcm)

• return;
  

• mutex_lock(&spec->pcm_lock);
• idx = per_pin->pcm_idx;
• per_pin->pcm_idx = -1;
• /* only for playback */
• substream = per_pin->pcm->pcm->streams[0].substream;
• clear_bit(idx, &spec->pcm_bitmap);
• if (substream && substream->runtime &&

• snd_pcm_running(substream))	{
  

Maybe better to protect this in snd_pcm_stream_lock_irq() before. The check itself seems racy.

Get it. Thanks.

...

• codec_warn(per_pin->codec,
  

• 	"HDMI: monitor disconnected, try to stop playback\n");
  

This shouldn't be a warning. It's the very normal behavior that user unplug HDMI while playing. At most, it's a debug message.

OK, I see.

...

• per_pin->pcm_detaching = true;
  

• mutex_unlock(&spec->pcm_lock);
  

• snd_pcm_stream_lock_irq(substream);
  

• snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
  

• snd_pcm_stream_unlock_irq(substream);
  

• } else {

• per_pin->pcm = NULL;
  

• mutex_unlock(&spec->pcm_lock);
  

• }

+}

• static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_jack_tbl *jack;

@@ -1586,6 +1699,12 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) } }

• /* need be protected by spec->lock */
• if (eld->eld_valid)

• hdmi_attach_hda_pcm(spec, per_pin);
  

• else

• hdmi_detach_hda_pcm(spec, per_pin);
  

• if (pin_eld->eld_valid != eld->eld_valid) eld_changed = true;

@@ -1662,6 +1781,7 @@ 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;

  caps = snd_hda_query_pin_caps(codec, pin_nid); if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))

@@ -1671,8 +1791,14 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) return 0;

• if (is_haswell_plus(codec))

• if (is_haswell_plus(codec)) { intel_haswell_fixup_connect_list(codec, pin_nid);

• spec->dev_num = 3;
  

• } else {

• dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;
  

• spec->dev_num = (spec->dev_num > dev_num) ?
  

• 	spec->dev_num : dev_num;
  

Is this correct? I thought *_get_num_devices() returns the number of currently plugged devices. Or does it return the max number of devices for the pin?

From the spec, it says: If the Device List Lenght value is 1 - 63, it indicates the Pin Widget is multi stream capable, and 2 - 64 Device Entries are supported in the Pin Widget.

So my understanding is that it should be the pin capability, not the status.

My test on BDW result shows: 1. When there is not DP MST hub connected on the pin, the *_get_num_devices() will return 0, which means Pin Widget is not MST capable. 2. When there is DP MST hub connected to the pin, it will always return 2, no matter how many monitors are connected to the pin. And it will also return 2 even you disconnect the hub.

Takashi

...

• }

  pin_idx = spec->num_pins; per_pin = snd_array_new(&spec->pins);

@@ -1680,6 +1806,9 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) return -ENOMEM;

per_pin->pin_nid = pin_nid;

• per_pin->pin_nid_idx = pin_idx; /* to be fixed for mst audio */

• per_pin->dev_id = 0; /* Not support MST audio so far */

• per_pin->pcm_idx = -1; per_pin->non_pcm = false;

  err = hdmi_read_pin_conn(codec, pin_idx);

@@ -1799,13 +1928,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, hda_nid_t cvt_nid = hinfo->nid; struct hdmi_spec *spec = codec->spec; int pin_idx = hinfo_to_pin_index(codec, hinfo);

• struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
• hda_nid_t pin_nid = per_pin->pin_nid;

• struct hdmi_spec_per_pin *per_pin;

• hda_nid_t pin_nid; struct snd_pcm_runtime *runtime = substream->runtime; struct i915_audio_component *acomp = codec->bus->core.audio_component; bool non_pcm; int pinctl;

• /* no pin is attached */

• if (pin_idx < 0)

• return -EINVAL;
  

• per_pin = get_pin(spec, pin_idx);

• pin_nid = per_pin->pin_nid;

• if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { /* Verify pin:cvt selections to avoid silent audio after S3.

  • After S3, the audio driver restores pin:cvt selections

@@ -1877,6 +2012,12 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, if (snd_BUG_ON(pin_idx < 0)) return -EINVAL; per_pin = get_pin(spec, pin_idx);

• if (per_pin->pcm_detaching) {
  

• 	mutex_lock(&spec->pcm_lock);
  

• 	per_pin->pcm = NULL;
  

• 	per_pin->pcm_detaching = false;
  

• 	mutex_unlock(&spec->pcm_lock);
  

• }
  
  

  if (spec->dyn_pin_out) { pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0,

@@ -1896,7 +2037,6 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, per_pin->channels = 0; mutex_unlock(&per_pin->lock); }

• return 0; }

@@ -2068,22 +2208,26 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol, static int generic_hdmi_build_pcms(struct hda_codec *codec) { struct hdmi_spec *spec = codec->spec;

• int pin_idx;

• int idx;

• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {

• for (idx = 0; idx < spec->num_pins + spec->dev_num - 1; idx++) { struct hda_pcm *info; struct hda_pcm_stream *pstr;

• info = snd_hda_codec_pcm_new(codec, "HDMI %d", pin_idx);
  

• info = snd_hda_codec_pcm_new(codec, "HDMI %d", idx);
  

  if (!info) return -ENOMEM;

• spec->pcm_rec[pin_idx] = info;
  

• spec->pcm_rec[idx] = info;
  

• spec->pcm_used++;
  

  info->pcm_type = HDA_PCM_TYPE_HDMI; info->own_chmap = true;

  pstr = &info->stream[SNDRV_PCM_STREAM_PLAYBACK]; pstr->substreams = 1; pstr->ops = generic_ops;

• /* pcm number is less than 16 */
  

• if (spec->pcm_used >= 16)
  

• 	break;
  

  /* other pstr fields are set in open */ }

@@ -2360,6 +2504,8 @@ static int patch_generic_hdmi(struct hda_codec *codec) return -ENOMEM;

spec->ops = generic_standard_hdmi_ops;

• spec->dev_num = 1; /* initialize to 1 */
• mutex_init(&spec->pcm_lock); codec->spec = spec; hdmi_array_init(spec, 4);

-- 1.9.1

Hi Takashi,

On 11/04/2015 01:38 PM, Libin Yang wrote:

On 11/04/2015 12:44 AM, Takashi Iwai wrote:

...

On Tue, 03 Nov 2015 09:22:53 +0100, libin.yang@linux.intel.com wrote:

...

From: Libin Yang libin.yang@intel.com

Allocate the PCM based on the number of pin and device entry. The number of PCM is pin num plus device entry number per pin. For example, on Intel platform, it will be 5 PCMs.

Do not attach the PCM to pin in initialization. Dynamically attach PCM to pin when monitor is connected in HDMI audio codec driver.

When monitor is disconnected, detach the PCM from the pin. And if the audio is playing, stop the playback.

The below is the example of device entry and PCM binding: For a monitor is plugged in, we need to dynamically assign this monitor to 5 PCM devices (on Intel platform): For a monitor at pin nid 0x05, dev index 0, it will prefer PCM 3 For a monitor at pin nid 0x06, dev index 0, it will prefer PCM 7 For a monitor at pin nid 0x07, dev index 0, it will prefer PCM 8 For a monitor at dev index 1 (any pin), it will prefer PCM 9 For a monitor at dev index 2 (any pin), it will prefer PCM 10

If the preferred PCM is not available, try PCM in this order: 9, 10, 3, 7 ,8.

Signed-off-by: Libin Yang libin.yang@linux.intel.com

We should split the changes to a few patches here. For example, stopping the stream at discussion is basically an implementation independent from the MST itself. IOW, the dynamic attach/detach can be implemented and tested even without MST support. Let's code them at first, then add MST support.

This patch actually doesn't depend on MST. I use the dev_num to determine how many PCMs to be created.

To Split the patch, how about I create the PCM number based the pin number in the patch?

...

Also, it's an open question whether we apply the dynamic attach/detach to all devices that use the generic hdmi framework. It means including AMD and Nvidia. You hard-coded it to be applied unconditionally. Would it break anything potentially...?

I have no other verdors platforms to test. But it seems to be OK on other platforms. The impact is, i think, it will stop the stream when monitor is disconnected. Should we apply the code by judging whehter it is Intel platform?

If you are concerning the impact on other vendors, what about I create a separate mst codec driver? In the driver, I will still use virtual pin, but other vendors platform will not be supported at the moment. Other vendors can add their patches to the new codec driver to support their own mst audio.

...

Some more comments below.

...

---

sound/pci/hda/hda_codec.c | 5 +- sound/pci/hda/hda_codec.h | 1 + sound/pci/hda/patch_hdmi.c | 170 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 162 insertions(+), 14 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 8374188..e1d4648 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -313,7 +313,7 @@ EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);

/* return DEVLIST_LEN parameter of the given widget */ -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid) { unsigned int wcaps = get_wcaps(codec, nid); unsigned int parm; @@ -327,6 +327,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid) parm = 0; return parm & AC_DEV_LIST_LEN_MASK; } +EXPORT_SYMBOL_GPL(snd_hda_get_num_devices);

Once when exporting, add the proper kernel doc comment, too. I guess this can be split as a preparation patch, too.

OK, I will add the kernel doc comment. And put it in a preparation patch.

...

...

/**

• snd_hda_get_devices - copy device list without cache

@@ -344,7 +345,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, unsigned int parm; int i, dev_len, devices;

• parm = get_num_devices(codec, nid);

• parm = snd_hda_get_num_devices(codec, nid); if (!parm) /* not multi-stream capable */ return 0;

diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 373fcad..ad4da41 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -347,6 +347,7 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums, const hda_nid_t *list); int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux, hda_nid_t nid, int recursive); +unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid); int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid, u8 *dev_list, int max_devices);

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index f503a88..8d31366 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -72,6 +72,9 @@ struct hdmi_spec_per_cvt {

struct hdmi_spec_per_pin { hda_nid_t pin_nid;

• hda_dev_t dev_id;
• /* real pin idx. device entries on same pin share same

pin_nid_idx */

• int pin_nid_idx; int num_mux_nids; hda_nid_t mux_nids[HDA_MAX_CONNECTIONS]; int mux_idx;

@@ -82,6 +85,9 @@ struct hdmi_spec_per_pin { struct mutex lock; struct delayed_work work; struct snd_kcontrol *eld_ctl;

• struct hda_pcm *pcm; /* pointer to spec->pcm_rec[n] */
• int pcm_idx; /* which pcm is attached. -1 means no pcm is

attached */

• bool pcm_detaching; int repoll_count; bool setup; /* the stream has been set up by prepare callback */ int channels; /* current number of channels */

@@ -134,6 +140,10 @@ struct hdmi_spec { int num_pins; struct snd_array pins; /* struct hdmi_spec_per_pin */ struct hda_pcm *pcm_rec[16];

• unsigned long pcm_bitmap;

• struct mutex pcm_lock;

• int pcm_used; /* counter of pcm_rec[] */

• int dev_num; unsigned int channels_max; /* max over all cvts */

  struct hdmi_eld temp_eld;

@@ -378,12 +388,18 @@ static int hinfo_to_pin_index(struct hda_codec *codec, struct hda_pcm_stream *hinfo) { struct hdmi_spec *spec = codec->spec;

• struct hdmi_spec_per_pin *per_pin; int pin_idx;

• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)

•    if (get_pcm_rec(spec, pin_idx)->stream == hinfo)
  

• mutex_lock(&spec->pcm_lock);
• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {

•    per_pin = get_pin(spec, pin_idx);
  

•    if (per_pin->pcm && per_pin->pcm->stream == hinfo) {
  

•        mutex_unlock(&spec->pcm_lock);
          return pin_idx;
  

•    }
  

• }
• mutex_unlock(&spec->pcm_lock); codec_warn(codec, "HDMI: hinfo %p not registered\n", hinfo); return -EINVAL; }

@@ -1451,7 +1467,8 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,

  /* Validate hinfo */
  pin_idx = hinfo_to_pin_index(codec, hinfo);

• if (snd_BUG_ON(pin_idx < 0))

• /* if no pin is attached, return fail */
• if (pin_idx < 0) return -EINVAL; per_pin = get_pin(spec, pin_idx); eld = &per_pin->sink_eld;

@@ -1529,6 +1546,102 @@ 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 (per_pin->dev_id == 0) {

•    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 slot_offset;
• int i;
• /* try the prefer PCM */
• slot = get_preferred_pcm_slot(spec, per_pin);
• if (slot != -1)

•    return slot;
  

• /* have a second try, try backup PCMs from specified offset */
• if (per_pin->dev_id == 0)

•    slot_offset = 0;
  

• else

•    slot_offset = per_pin->dev_id - 1;
  

• for (i = spec->num_pins + slot_offset; i < spec->pcm_used; i++) {

•    if ((spec->pcm_bitmap & (1 << i)) == 0)
  

•        return i;
  

• }

IMO, this step can be skipped and just check through the backup PCMs.

This step is used for the situation (for example, on Intel platform): if the device entry id is 1, it will prefer PCM 9; if the device entry id is 2, it will prefer PCM 10;

OK, I will remove the second try.

...

...

• /* have a third try, try all backup PCMs */
• for (i = spec->num_pins; i < spec->pcm_used; i++) {

•    if ((spec->pcm_bitmap & (1 << i)) == 0)
  

•        return i;
  

• }
• /* the last try, try all PCMs */
• 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;
  

• mutex_lock(&spec->pcm_lock);
• idx = hdmi_find_pcm_slot(spec, per_pin);
• if (idx == -ENODEV) {

•    mutex_unlock(&spec->pcm_lock);
  

•    return;
  

• }
• per_pin->pcm_idx = idx;
• per_pin->pcm = spec->pcm_rec[idx];
• set_bit(idx, &spec->pcm_bitmap);
• mutex_unlock(&spec->pcm_lock);

+}

+static void hdmi_detach_hda_pcm(struct hdmi_spec *spec,

•            struct hdmi_spec_per_pin *per_pin)
  

+{

• int idx;
• struct snd_pcm_substream *substream;
• /* pcm already be detached from the pin */
• if (!per_pin->pcm)

•    return;
  

• mutex_lock(&spec->pcm_lock);
• idx = per_pin->pcm_idx;
• per_pin->pcm_idx = -1;
• /* only for playback */
• substream = per_pin->pcm->pcm->streams[0].substream;
• clear_bit(idx, &spec->pcm_bitmap);
• if (substream && substream->runtime &&

•    snd_pcm_running(substream))    {
  

Maybe better to protect this in snd_pcm_stream_lock_irq() before. The check itself seems racy.

Get it. Thanks.

...

...

•    codec_warn(per_pin->codec,
  

•        "HDMI: monitor disconnected, try to stop playback\n");
  

This shouldn't be a warning. It's the very normal behavior that user unplug HDMI while playing. At most, it's a debug message.

OK, I see.

...

...

•    per_pin->pcm_detaching = true;
  

•    mutex_unlock(&spec->pcm_lock);
  

•    snd_pcm_stream_lock_irq(substream);
  

•    snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
  

•    snd_pcm_stream_unlock_irq(substream);
  

• } else {

•    per_pin->pcm = NULL;
  

•    mutex_unlock(&spec->pcm_lock);
  

• }

+}

• static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin,

int repoll) { struct hda_jack_tbl *jack; @@ -1586,6 +1699,12 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) } }

• /* need be protected by spec->lock */
• if (eld->eld_valid)

•    hdmi_attach_hda_pcm(spec, per_pin);
  

• else

•    hdmi_detach_hda_pcm(spec, per_pin);
  

• if (pin_eld->eld_valid != eld->eld_valid)
      eld_changed = true;
  
  

@@ -1662,6 +1781,7 @@ 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;

  caps = snd_hda_query_pin_caps(codec, pin_nid); if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))

@@ -1671,8 +1791,14 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) return 0;

• if (is_haswell_plus(codec))

• if (is_haswell_plus(codec)) { intel_haswell_fixup_connect_list(codec, pin_nid);

•    spec->dev_num = 3;
  

• } else {

•    dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;
  

•    spec->dev_num = (spec->dev_num > dev_num) ?
  

•        spec->dev_num : dev_num;
  

Is this correct? I thought *_get_num_devices() returns the number of currently plugged devices. Or does it return the max number of devices for the pin?

From the spec, it says: If the Device List Lenght value is 1 - 63, it indicates the Pin Widget is multi stream capable, and 2 - 64 Device Entries are supported in the Pin Widget.

So my understanding is that it should be the pin capability, not the status.

My test on BDW result shows:

1. When there is not DP MST hub connected on the pin, the

*_get_num_devices() will return 0, which means Pin Widget is not MST capable. 2. When there is DP MST hub connected to the pin, it will always return 2, no matter how many monitors are connected to the pin. And it will also return 2 even you disconnect the hub.

...

Takashi

...

• }

  pin_idx = spec->num_pins; per_pin = snd_array_new(&spec->pins);

@@ -1680,6 +1806,9 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) return -ENOMEM;

  per_pin->pin_nid = pin_nid;

• per_pin->pin_nid_idx = pin_idx; /* to be fixed for mst audio */

• per_pin->dev_id = 0; /* Not support MST audio so far */

• per_pin->pcm_idx = -1; per_pin->non_pcm = false;

  err = hdmi_read_pin_conn(codec, pin_idx);

@@ -1799,13 +1928,19 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, hda_nid_t cvt_nid = hinfo->nid; struct hdmi_spec *spec = codec->spec; int pin_idx = hinfo_to_pin_index(codec, hinfo);

• struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
• hda_nid_t pin_nid = per_pin->pin_nid;

• struct hdmi_spec_per_pin *per_pin;
• hda_nid_t pin_nid; struct snd_pcm_runtime *runtime = substream->runtime; struct i915_audio_component *acomp =

codec->bus->core.audio_component; bool non_pcm; int pinctl;

• /* no pin is attached */
• if (pin_idx < 0)

•    return -EINVAL;
  

• per_pin = get_pin(spec, pin_idx);
• pin_nid = per_pin->pin_nid;

• if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
      /* Verify pin:cvt selections to avoid silent audio after S3.
       * After S3, the audio driver restores pin:cvt selections
  

@@ -1877,6 +2012,12 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, if (snd_BUG_ON(pin_idx < 0)) return -EINVAL; per_pin = get_pin(spec, pin_idx);

•    if (per_pin->pcm_detaching) {
  

•        mutex_lock(&spec->pcm_lock);
  

•        per_pin->pcm = NULL;
  

•        per_pin->pcm_detaching = false;
  

•        mutex_unlock(&spec->pcm_lock);
  

•    }
  
      if (spec->dyn_pin_out) {
          pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
  

@@ -1896,7 +2037,6 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo, per_pin->channels = 0; mutex_unlock(&per_pin->lock); }

• return 0;
  

  }

@@ -2068,22 +2208,26 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol, static int generic_hdmi_build_pcms(struct hda_codec *codec) { struct hdmi_spec *spec = codec->spec;

• int pin_idx;

• int idx;

• for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {

• for (idx = 0; idx < spec->num_pins + spec->dev_num - 1; idx++) { struct hda_pcm *info; struct hda_pcm_stream *pstr;

•    info = snd_hda_codec_pcm_new(codec, "HDMI %d", pin_idx);
  

•    info = snd_hda_codec_pcm_new(codec, "HDMI %d", idx);
      if (!info)
          return -ENOMEM;
  

•    spec->pcm_rec[pin_idx] = info;
  

•    spec->pcm_rec[idx] = info;
  

•    spec->pcm_used++;
      info->pcm_type = HDA_PCM_TYPE_HDMI;
      info->own_chmap = true;
  
      pstr = &info->stream[SNDRV_PCM_STREAM_PLAYBACK];
      pstr->substreams = 1;
      pstr->ops = generic_ops;
  

•    /* pcm number is less than 16 */
  

•    if (spec->pcm_used >= 16)
  

•        break;
      /* other pstr fields are set in open */
  }
  
  

@@ -2360,6 +2504,8 @@ static int patch_generic_hdmi(struct hda_codec *codec) return -ENOMEM;

  spec->ops = generic_standard_hdmi_ops;

• spec->dev_num = 1; /* initialize to 1 */
• mutex_init(&spec->pcm_lock); codec->spec = spec; hdmi_array_init(spec, 4);

-- 1.9.1