[RFC PATCH 0/2] hdmi audio dynamically bind PCM to pin

[RFC PATCH 0/2] hdmi audio dynamically bind PCM to pin

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

The patches are trying to support dynamically binding PCM to pin in HDMI audio.

The first patch:
When PCM is open when there is no pin (port) bound to the PCM,
the driver try to open the PCM successfully. It will try to find
an available converter to assign to the PCM.
In PCM prepare, if there is no pin bound to the PCM, the driver
will do the simple configuration and return true.
In PCM close, if there is no pin bound to the PCM, the driver will
simply close it.
All the pin configuration will be setup in the hotplug which is done
in patch 2.
As the PCM is dynamically bound to the pin, the pcm_lock is necessary
for the whole pcm open/prepare/close process.

The second patch:
The patch mainly handle the hotplug event.
Each time monitor is connected, a proper PCM will be bound to the pin.
If the PCM is in use, the pin must be configured.
Each time monitor is disconnect, the PCM will be unbound from the pin.
The pin will be reset.

Libin Yang (2):
  ALSA: hda - hdmi playback without monitor in dynamic pcm bind mode
  ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug

 sound/pci/hda/hda_codec.h  |   1 +
 sound/pci/hda/patch_hdmi.c | 354 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 337 insertions(+), 18 deletions(-)

-- 
1.9.1

[RFC PATCH 1/2] ALSA: hda - hdmi playback without monitor in dynamic pcm bind mode

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

Pulseaudio requires open pcm successfully when probing.

This patch handles playback without monitor in dynamic pcm assignment
mode. It tries to open/prepare/close pcm successfully even there is
no pin bound to the PCM. On the meantime, it will try to find a proper
converter for the PCM.

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 sound/pci/hda/patch_hdmi.c | 163 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 152 insertions(+), 11 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 92e05fb..12fc506 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -135,6 +135,7 @@ struct hdmi_spec {
 	int num_pins;
 	struct snd_array pins; /* struct hdmi_spec_per_pin */
 	struct hda_pcm *pcm_rec[16];
+	struct mutex pcm_lock;
 	unsigned int channels_max; /* max over all cvts */
 
 	struct hdmi_eld temp_eld;
@@ -1388,6 +1389,17 @@ static void intel_verify_pin_cvt_connect(struct hda_codec *codec,
 					    mux_idx);
 }
 
+static int intel_cvt_id_to_mux_idx(struct hdmi_spec *spec,
+			hda_nid_t cvt_nid)
+{
+	int i;
+
+	for (i = 0; i < spec->num_cvts; i++)
+		if (spec->cvt_nids[i] == cvt_nid)
+			return i;
+	return -EINVAL;
+}
+
 /* Intel HDMI workaround to fix audio routing issue:
  * For some Intel display codecs, pins share the same connection list.
  * So a conveter can be selected by multiple pins and playback on any of these
@@ -1439,6 +1451,77 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
 	}
 }
 
+static int hdmi_find_available_cvt(struct hda_codec *codec,
+				int *cvt_id)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_cvt *per_cvt = NULL;
+	int cvt_idx;
+
+	/* Dynamically assign converter to stream */
+	for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
+		per_cvt = get_cvt(spec, cvt_idx);
+
+		/* Must not already be assigned */
+		if (!per_cvt->assigned)
+			break;
+	}
+
+	/* No free converters */
+	if (cvt_idx == spec->num_cvts)
+		return -EBUSY;
+
+	if (cvt_id)
+		*cvt_id = cvt_idx;
+
+	return 0;
+}
+
+/* called in hdmi_pcm_open when no pin is assigned to the PCM
+ * in dyn_pcm_assign mode.
+ */
+static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo,
+			 struct hda_codec *codec,
+			 struct snd_pcm_substream *substream)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	int cvt_idx, mux_idx;
+	struct hdmi_spec_per_cvt *per_cvt = NULL;
+	int err;
+
+	err = hdmi_find_available_cvt(codec, &cvt_idx);
+	if (err)
+		return err;
+
+	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);
+
+	/* todo: setup spdif ctls assign */
+
+	/* Initially set the converter's capabilities */
+	hinfo->channels_min = per_cvt->channels_min;
+	hinfo->channels_max = per_cvt->channels_max;
+	hinfo->rates = per_cvt->rates;
+	hinfo->formats = per_cvt->formats;
+	hinfo->maxbps = per_cvt->maxbps;
+
+	/* Store the updated parameters */
+	runtime->hw.channels_min = hinfo->channels_min;
+	runtime->hw.channels_max = hinfo->channels_max;
+	runtime->hw.formats = hinfo->formats;
+	runtime->hw.rates = hinfo->rates;
+
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+				   SNDRV_PCM_HW_PARAM_CHANNELS, 2);
+	return 0;
+}
+
 /*
  * HDA PCM callbacks
  */
@@ -1455,19 +1538,36 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	int err;
 
 	/* Validate hinfo */
+	mutex_lock(&spec->pcm_lock);
 	pin_idx = hinfo_to_pin_index(codec, hinfo);
-	if (snd_BUG_ON(pin_idx < 0))
-		return -EINVAL;
-	per_pin = get_pin(spec, pin_idx);
-	eld = &per_pin->sink_eld;
+	if (!spec->dyn_pcm_assign) {
+		if (snd_BUG_ON(pin_idx < 0)) {
+			mutex_unlock(&spec->pcm_lock);
+			return -EINVAL;
+		}
+	} else {
+		/* no pin is assigned to the PCM
+		 * PA need pcm open successfully when probe
+		 */
+		if (pin_idx < 0) {
+			err = hdmi_pcm_open_no_pin(hinfo, codec, substream);
+			mutex_unlock(&spec->pcm_lock);
+			return err;
+		}
+	}
 
 	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
-	if (err < 0)
+	if (err < 0) {
+		mutex_unlock(&spec->pcm_lock);
 		return err;
+	}
 
 	per_cvt = get_cvt(spec, cvt_idx);
 	/* Claim converter */
 	per_cvt->assigned = 1;
+
+
+	per_pin = get_pin(spec, pin_idx);
 	per_pin->cvt_nid = per_cvt->cvt_nid;
 	hinfo->nid = per_cvt->cvt_nid;
 
@@ -1488,6 +1588,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	hinfo->formats = per_cvt->formats;
 	hinfo->maxbps = per_cvt->maxbps;
 
+	eld = &per_pin->sink_eld;
 	/* Restrict capabilities by ELD if this isn't disabled */
 	if (!static_hdmi_pcm && eld->eld_valid) {
 		snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);
@@ -1496,10 +1597,12 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 			per_cvt->assigned = 0;
 			hinfo->nid = 0;
 			snd_hda_spdif_ctls_unassign(codec, pin_idx);
+			mutex_unlock(&spec->pcm_lock);
 			return -ENODEV;
 		}
 	}
 
+	mutex_unlock(&spec->pcm_lock);
 	/* Store the updated parameters */
 	runtime->hw.channels_min = hinfo->channels_min;
 	runtime->hw.channels_max = hinfo->channels_max;
@@ -1803,13 +1906,38 @@ 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;
+	int pin_idx;
+	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 mux_idx;
 	int pinctl;
+	int err;
+
+	mutex_lock(&spec->pcm_lock);
+	pin_idx = hinfo_to_pin_index(codec, hinfo);
+	if (spec->dyn_pcm_assign && pin_idx < 0) {
+		/* when dyn_pcm_assign and pcm is not bound to a pin
+		 * skip pin setup and return 0 to make audio playback
+		 * be ongoing
+		 */
+		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
+			mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
+			if (mux_idx > 0)
+				intel_not_share_assigned_cvt(codec, 0, mux_idx);
+		}
+		snd_hda_codec_setup_stream(codec, cvt_nid,
+					stream_tag, 0, format);
+		mutex_unlock(&spec->pcm_lock);
+		return 0;
+	}
+
+	if (snd_BUG_ON(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.
@@ -1838,7 +1966,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 
 	hdmi_setup_audio_infoframe(codec, per_pin, non_pcm);
 	mutex_unlock(&per_pin->lock);
-
+	mutex_unlock(&spec->pcm_lock);
 	if (spec->dyn_pin_out) {
 		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
 					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
@@ -1847,7 +1975,10 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 				    pinctl | PIN_OUT);
 	}
 
-	return spec->ops.setup_stream(codec, cvt_nid, pin_nid, stream_tag, format);
+	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
+				 stream_tag, format);
+	mutex_unlock(&spec->pcm_lock);
+	return err;
 }
 
 static int generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo,
@@ -1878,9 +2009,17 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
 		per_cvt->assigned = 0;
 		hinfo->nid = 0;
 
+		mutex_lock(&spec->pcm_lock);
 		pin_idx = hinfo_to_pin_index(codec, hinfo);
-		if (snd_BUG_ON(pin_idx < 0))
+		if (spec->dyn_pcm_assign && pin_idx < 0) {
+			mutex_unlock(&spec->pcm_lock);
+			return 0;
+		}
+
+		if (snd_BUG_ON(pin_idx < 0)) {
+			mutex_unlock(&spec->pcm_lock);
 			return -EINVAL;
+		}
 		per_pin = get_pin(spec, pin_idx);
 
 		if (spec->dyn_pin_out) {
@@ -1900,6 +2039,7 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
 		per_pin->setup = false;
 		per_pin->channels = 0;
 		mutex_unlock(&per_pin->lock);
+		mutex_unlock(&spec->pcm_lock);
 	}
 
 	return 0;
@@ -2370,6 +2510,7 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		return -ENOMEM;
 
 	spec->ops = generic_standard_hdmi_ops;
+	mutex_init(&spec->pcm_lock);
 	codec->spec = spec;
 	hdmi_array_init(spec, 4);
 
-- 
1.9.1

Re: [RFC PATCH 1/2] ALSA: hda - hdmi playback without monitor in dynamic pcm bind mode

[Takashi Iwai]

On Wed, 18 Nov 2015 09:46:58 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> Pulseaudio requires open pcm successfully when probing.
> 
> This patch handles playback without monitor in dynamic pcm assignment
> mode. It tries to open/prepare/close pcm successfully even there is
> no pin bound to the PCM. On the meantime, it will try to find a proper
> converter for the PCM.
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 163 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 152 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 92e05fb..12fc506 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -135,6 +135,7 @@ struct hdmi_spec {
>  	int num_pins;
>  	struct snd_array pins; /* struct hdmi_spec_per_pin */
>  	struct hda_pcm *pcm_rec[16];
> +	struct mutex pcm_lock;

Ideally this should be a rw semaphore so that it won't block too
much.  But we can optimize it later.

>  	unsigned int channels_max; /* max over all cvts */
>  
>  	struct hdmi_eld temp_eld;
> @@ -1388,6 +1389,17 @@ static void intel_verify_pin_cvt_connect(struct hda_codec *codec,
>  					    mux_idx);
>  }
>  
> +static int intel_cvt_id_to_mux_idx(struct hdmi_spec *spec,
> +			hda_nid_t cvt_nid)
> +{
> +	int i;
> +
> +	for (i = 0; i < spec->num_cvts; i++)
> +		if (spec->cvt_nids[i] == cvt_nid)
> +			return i;
> +	return -EINVAL;
> +}

This is always a couple with intel_not_share_assigned_cvt().
So better to have a helper doing both in a shot.
(And give a comment what's doing.)


> +
>  /* Intel HDMI workaround to fix audio routing issue:
>   * For some Intel display codecs, pins share the same connection list.
>   * So a conveter can be selected by multiple pins and playback on any of these
> @@ -1439,6 +1451,77 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
>  	}
>  }
>  
> +static int hdmi_find_available_cvt(struct hda_codec *codec,
> +				int *cvt_id)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +	struct hdmi_spec_per_cvt *per_cvt = NULL;
> +	int cvt_idx;
> +
> +	/* Dynamically assign converter to stream */
> +	for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> +		per_cvt = get_cvt(spec, cvt_idx);
> +
> +		/* Must not already be assigned */
> +		if (!per_cvt->assigned)
> +			break;
> +	}
> +
> +	/* No free converters */
> +	if (cvt_idx == spec->num_cvts)
> +		return -EBUSY;
> +
> +	if (cvt_id)
> +		*cvt_id = cvt_idx;
> +
> +	return 0;
> +}

This could be merged into hdmi_choose_cvt().  Let per_pin = NULL if
pin_idx < 0, and skip the mux check, for example.

> +
> +/* called in hdmi_pcm_open when no pin is assigned to the PCM
> + * in dyn_pcm_assign mode.
> + */
> +static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo,
> +			 struct hda_codec *codec,
> +			 struct snd_pcm_substream *substream)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int cvt_idx, mux_idx;
> +	struct hdmi_spec_per_cvt *per_cvt = NULL;
> +	int err;
> +
> +	err = hdmi_find_available_cvt(codec, &cvt_idx);
> +	if (err)
> +		return err;
> +
> +	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);

Don't forget the hsw+/vlv+ condition check like in other places.
Or maybe better put the check into the callee, the new helper
function.


Takashi


> +
> +	/* todo: setup spdif ctls assign */
> +
> +	/* Initially set the converter's capabilities */
> +	hinfo->channels_min = per_cvt->channels_min;
> +	hinfo->channels_max = per_cvt->channels_max;
> +	hinfo->rates = per_cvt->rates;
> +	hinfo->formats = per_cvt->formats;
> +	hinfo->maxbps = per_cvt->maxbps;
> +
> +	/* Store the updated parameters */
> +	runtime->hw.channels_min = hinfo->channels_min;
> +	runtime->hw.channels_max = hinfo->channels_max;
> +	runtime->hw.formats = hinfo->formats;
> +	runtime->hw.rates = hinfo->rates;
> +
> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
> +				   SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> +	return 0;
> +}
> +
>  /*
>   * HDA PCM callbacks
>   */
> @@ -1455,19 +1538,36 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  	int err;
>  
>  	/* Validate hinfo */
> +	mutex_lock(&spec->pcm_lock);
>  	pin_idx = hinfo_to_pin_index(codec, hinfo);
> -	if (snd_BUG_ON(pin_idx < 0))
> -		return -EINVAL;
> -	per_pin = get_pin(spec, pin_idx);
> -	eld = &per_pin->sink_eld;
> +	if (!spec->dyn_pcm_assign) {
> +		if (snd_BUG_ON(pin_idx < 0)) {
> +			mutex_unlock(&spec->pcm_lock);
> +			return -EINVAL;
> +		}
> +	} else {
> +		/* no pin is assigned to the PCM
> +		 * PA need pcm open successfully when probe
> +		 */
> +		if (pin_idx < 0) {
> +			err = hdmi_pcm_open_no_pin(hinfo, codec, substream);
> +			mutex_unlock(&spec->pcm_lock);
> +			return err;
> +		}
> +	}
>  
>  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
> -	if (err < 0)
> +	if (err < 0) {
> +		mutex_unlock(&spec->pcm_lock);
>  		return err;
> +	}
>  
>  	per_cvt = get_cvt(spec, cvt_idx);
>  	/* Claim converter */
>  	per_cvt->assigned = 1;
> +
> +
> +	per_pin = get_pin(spec, pin_idx);
>  	per_pin->cvt_nid = per_cvt->cvt_nid;
>  	hinfo->nid = per_cvt->cvt_nid;
>  
> @@ -1488,6 +1588,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  	hinfo->formats = per_cvt->formats;
>  	hinfo->maxbps = per_cvt->maxbps;
>  
> +	eld = &per_pin->sink_eld;
>  	/* Restrict capabilities by ELD if this isn't disabled */
>  	if (!static_hdmi_pcm && eld->eld_valid) {
>  		snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);
> @@ -1496,10 +1597,12 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  			per_cvt->assigned = 0;
>  			hinfo->nid = 0;
>  			snd_hda_spdif_ctls_unassign(codec, pin_idx);
> +			mutex_unlock(&spec->pcm_lock);
>  			return -ENODEV;
>  		}
>  	}
>  
> +	mutex_unlock(&spec->pcm_lock);
>  	/* Store the updated parameters */
>  	runtime->hw.channels_min = hinfo->channels_min;
>  	runtime->hw.channels_max = hinfo->channels_max;
> @@ -1803,13 +1906,38 @@ 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;
> +	int pin_idx;
> +	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 mux_idx;
>  	int pinctl;
> +	int err;
> +
> +	mutex_lock(&spec->pcm_lock);
> +	pin_idx = hinfo_to_pin_index(codec, hinfo);
> +	if (spec->dyn_pcm_assign && pin_idx < 0) {
> +		/* when dyn_pcm_assign and pcm is not bound to a pin
> +		 * skip pin setup and return 0 to make audio playback
> +		 * be ongoing
> +		 */
> +		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> +			mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
> +			if (mux_idx > 0)
> +				intel_not_share_assigned_cvt(codec, 0, mux_idx);
> +		}
> +		snd_hda_codec_setup_stream(codec, cvt_nid,
> +					stream_tag, 0, format);
> +		mutex_unlock(&spec->pcm_lock);
> +		return 0;
> +	}
> +
> +	if (snd_BUG_ON(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.
> @@ -1838,7 +1966,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  
>  	hdmi_setup_audio_infoframe(codec, per_pin, non_pcm);
>  	mutex_unlock(&per_pin->lock);
> -
> +	mutex_unlock(&spec->pcm_lock);
>  	if (spec->dyn_pin_out) {
>  		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
>  					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> @@ -1847,7 +1975,10 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  				    pinctl | PIN_OUT);
>  	}
>  
> -	return spec->ops.setup_stream(codec, cvt_nid, pin_nid, stream_tag, format);
> +	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
> +				 stream_tag, format);
> +	mutex_unlock(&spec->pcm_lock);
> +	return err;
>  }
>  
>  static int generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo,
> @@ -1878,9 +2009,17 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
>  		per_cvt->assigned = 0;
>  		hinfo->nid = 0;
>  
> +		mutex_lock(&spec->pcm_lock);
>  		pin_idx = hinfo_to_pin_index(codec, hinfo);
> -		if (snd_BUG_ON(pin_idx < 0))
> +		if (spec->dyn_pcm_assign && pin_idx < 0) {
> +			mutex_unlock(&spec->pcm_lock);
> +			return 0;
> +		}
> +
> +		if (snd_BUG_ON(pin_idx < 0)) {
> +			mutex_unlock(&spec->pcm_lock);
>  			return -EINVAL;
> +		}
>  		per_pin = get_pin(spec, pin_idx);
>  
>  		if (spec->dyn_pin_out) {
> @@ -1900,6 +2039,7 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
>  		per_pin->setup = false;
>  		per_pin->channels = 0;
>  		mutex_unlock(&per_pin->lock);
> +		mutex_unlock(&spec->pcm_lock);
>  	}
>  
>  	return 0;
> @@ -2370,6 +2510,7 @@ static int patch_generic_hdmi(struct hda_codec *codec)
>  		return -ENOMEM;
>  
>  	spec->ops = generic_standard_hdmi_ops;
> +	mutex_init(&spec->pcm_lock);
>  	codec->spec = spec;
>  	hdmi_array_init(spec, 4);
>  
> -- 
> 1.9.1
> 

Re: [RFC PATCH 1/2] ALSA: hda - hdmi playback without monitor in dynamic pcm bind mode

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 18, 2015 7:13 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; mengdong.lin@linux.intel.com; Yang,
> Libin
> Subject: Re: [alsa-devel] [RFC PATCH 1/2] ALSA: hda - hdmi playback
> without monitor in dynamic pcm bind mode
> 
> On Wed, 18 Nov 2015 09:46:58 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > Pulseaudio requires open pcm successfully when probing.
> >
> > This patch handles playback without monitor in dynamic pcm
> assignment
> > mode. It tries to open/prepare/close pcm successfully even there is
> > no pin bound to the PCM. On the meantime, it will try to find a proper
> > converter for the PCM.
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > ---
> >  sound/pci/hda/patch_hdmi.c | 163
> ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 152 insertions(+), 11 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > index 92e05fb..12fc506 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -135,6 +135,7 @@ struct hdmi_spec {
> >  	int num_pins;
> >  	struct snd_array pins; /* struct hdmi_spec_per_pin */
> >  	struct hda_pcm *pcm_rec[16];
> > +	struct mutex pcm_lock;
> 
> Ideally this should be a rw semaphore so that it won't block too
> much.  But we can optimize it later.
> 
> >  	unsigned int channels_max; /* max over all cvts */
> >
> >  	struct hdmi_eld temp_eld;
> > @@ -1388,6 +1389,17 @@ static void
> intel_verify_pin_cvt_connect(struct hda_codec *codec,
> >  					    mux_idx);
> >  }
> >
> > +static int intel_cvt_id_to_mux_idx(struct hdmi_spec *spec,
> > +			hda_nid_t cvt_nid)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < spec->num_cvts; i++)
> > +		if (spec->cvt_nids[i] == cvt_nid)
> > +			return i;
> > +	return -EINVAL;
> > +}
> 
> This is always a couple with intel_not_share_assigned_cvt().
> So better to have a helper doing both in a shot.
> (And give a comment what's doing.)

OK. I will add a helper for it.

> 
> 
> > +
> >  /* Intel HDMI workaround to fix audio routing issue:
> >   * For some Intel display codecs, pins share the same connection list.
> >   * So a conveter can be selected by multiple pins and playback on any
> of these
> > @@ -1439,6 +1451,77 @@ static void
> intel_not_share_assigned_cvt(struct hda_codec *codec,
> >  	}
> >  }
> >
> > +static int hdmi_find_available_cvt(struct hda_codec *codec,
> > +				int *cvt_id)
> > +{
> > +	struct hdmi_spec *spec = codec->spec;
> > +	struct hdmi_spec_per_cvt *per_cvt = NULL;
> > +	int cvt_idx;
> > +
> > +	/* Dynamically assign converter to stream */
> > +	for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> > +		per_cvt = get_cvt(spec, cvt_idx);
> > +
> > +		/* Must not already be assigned */
> > +		if (!per_cvt->assigned)
> > +			break;
> > +	}
> > +
> > +	/* No free converters */
> > +	if (cvt_idx == spec->num_cvts)
> > +		return -EBUSY;
> > +
> > +	if (cvt_id)
> > +		*cvt_id = cvt_idx;
> > +
> > +	return 0;
> > +}
> 
> This could be merged into hdmi_choose_cvt().  Let per_pin = NULL if
> pin_idx < 0, and skip the mux check, for example.

OK, I will merge these functions.

> 
> > +
> > +/* called in hdmi_pcm_open when no pin is assigned to the PCM
> > + * in dyn_pcm_assign mode.
> > + */
> > +static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo,
> > +			 struct hda_codec *codec,
> > +			 struct snd_pcm_substream *substream)
> > +{
> > +	struct hdmi_spec *spec = codec->spec;
> > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > +	int cvt_idx, mux_idx;
> > +	struct hdmi_spec_per_cvt *per_cvt = NULL;
> > +	int err;
> > +
> > +	err = hdmi_find_available_cvt(codec, &cvt_idx);
> > +	if (err)
> > +		return err;
> > +
> > +	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);
> 
> Don't forget the hsw+/vlv+ condition check like in other places.
> Or maybe better put the check into the callee, the new helper
> function.

I forget it. It seems a new helper is better. I will do it.

Regards,
Libin

> 
> 
> Takashi
> 
> 
> > +
> > +	/* todo: setup spdif ctls assign */
> > +
> > +	/* Initially set the converter's capabilities */
> > +	hinfo->channels_min = per_cvt->channels_min;
> > +	hinfo->channels_max = per_cvt->channels_max;
> > +	hinfo->rates = per_cvt->rates;
> > +	hinfo->formats = per_cvt->formats;
> > +	hinfo->maxbps = per_cvt->maxbps;
> > +
> > +	/* Store the updated parameters */
> > +	runtime->hw.channels_min = hinfo->channels_min;
> > +	runtime->hw.channels_max = hinfo->channels_max;
> > +	runtime->hw.formats = hinfo->formats;
> > +	runtime->hw.rates = hinfo->rates;
> > +
> > +	snd_pcm_hw_constraint_step(substream->runtime, 0,
> > +
> SNDRV_PCM_HW_PARAM_CHANNELS, 2);
> > +	return 0;
> > +}
> > +
> >  /*
> >   * HDA PCM callbacks
> >   */
> > @@ -1455,19 +1538,36 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> >  	int err;
> >
> >  	/* Validate hinfo */
> > +	mutex_lock(&spec->pcm_lock);
> >  	pin_idx = hinfo_to_pin_index(codec, hinfo);
> > -	if (snd_BUG_ON(pin_idx < 0))
> > -		return -EINVAL;
> > -	per_pin = get_pin(spec, pin_idx);
> > -	eld = &per_pin->sink_eld;
> > +	if (!spec->dyn_pcm_assign) {
> > +		if (snd_BUG_ON(pin_idx < 0)) {
> > +			mutex_unlock(&spec->pcm_lock);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		/* no pin is assigned to the PCM
> > +		 * PA need pcm open successfully when probe
> > +		 */
> > +		if (pin_idx < 0) {
> > +			err = hdmi_pcm_open_no_pin(hinfo, codec,
> substream);
> > +			mutex_unlock(&spec->pcm_lock);
> > +			return err;
> > +		}
> > +	}
> >
> >  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
> > -	if (err < 0)
> > +	if (err < 0) {
> > +		mutex_unlock(&spec->pcm_lock);
> >  		return err;
> > +	}
> >
> >  	per_cvt = get_cvt(spec, cvt_idx);
> >  	/* Claim converter */
> >  	per_cvt->assigned = 1;
> > +
> > +
> > +	per_pin = get_pin(spec, pin_idx);
> >  	per_pin->cvt_nid = per_cvt->cvt_nid;
> >  	hinfo->nid = per_cvt->cvt_nid;
> >
> > @@ -1488,6 +1588,7 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> >  	hinfo->formats = per_cvt->formats;
> >  	hinfo->maxbps = per_cvt->maxbps;
> >
> > +	eld = &per_pin->sink_eld;
> >  	/* Restrict capabilities by ELD if this isn't disabled */
> >  	if (!static_hdmi_pcm && eld->eld_valid) {
> >  		snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);
> > @@ -1496,10 +1597,12 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> >  			per_cvt->assigned = 0;
> >  			hinfo->nid = 0;
> >  			snd_hda_spdif_ctls_unassign(codec, pin_idx);
> > +			mutex_unlock(&spec->pcm_lock);
> >  			return -ENODEV;
> >  		}
> >  	}
> >
> > +	mutex_unlock(&spec->pcm_lock);
> >  	/* Store the updated parameters */
> >  	runtime->hw.channels_min = hinfo->channels_min;
> >  	runtime->hw.channels_max = hinfo->channels_max;
> > @@ -1803,13 +1906,38 @@ 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;
> > +	int pin_idx;
> > +	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 mux_idx;
> >  	int pinctl;
> > +	int err;
> > +
> > +	mutex_lock(&spec->pcm_lock);
> > +	pin_idx = hinfo_to_pin_index(codec, hinfo);
> > +	if (spec->dyn_pcm_assign && pin_idx < 0) {
> > +		/* when dyn_pcm_assign and pcm is not bound to a pin
> > +		 * skip pin setup and return 0 to make audio playback
> > +		 * be ongoing
> > +		 */
> > +		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> > +			mux_idx = intel_cvt_id_to_mux_idx(spec,
> cvt_nid);
> > +			if (mux_idx > 0)
> > +				intel_not_share_assigned_cvt(codec, 0,
> mux_idx);
> > +		}
> > +		snd_hda_codec_setup_stream(codec, cvt_nid,
> > +					stream_tag, 0, format);
> > +		mutex_unlock(&spec->pcm_lock);
> > +		return 0;
> > +	}
> > +
> > +	if (snd_BUG_ON(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.
> > @@ -1838,7 +1966,7 @@ static int
> generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >
> >  	hdmi_setup_audio_infoframe(codec, per_pin, non_pcm);
> >  	mutex_unlock(&per_pin->lock);
> > -
> > +	mutex_unlock(&spec->pcm_lock);
> >  	if (spec->dyn_pin_out) {
> >  		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> >
> AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> > @@ -1847,7 +1975,10 @@ static int
> generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >  				    pinctl | PIN_OUT);
> >  	}
> >
> > -	return spec->ops.setup_stream(codec, cvt_nid, pin_nid,
> stream_tag, format);
> > +	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
> > +				 stream_tag, format);
> > +	mutex_unlock(&spec->pcm_lock);
> > +	return err;
> >  }
> >
> >  static int generic_hdmi_playback_pcm_cleanup(struct
> hda_pcm_stream *hinfo,
> > @@ -1878,9 +2009,17 @@ static int hdmi_pcm_close(struct
> hda_pcm_stream *hinfo,
> >  		per_cvt->assigned = 0;
> >  		hinfo->nid = 0;
> >
> > +		mutex_lock(&spec->pcm_lock);
> >  		pin_idx = hinfo_to_pin_index(codec, hinfo);
> > -		if (snd_BUG_ON(pin_idx < 0))
> > +		if (spec->dyn_pcm_assign && pin_idx < 0) {
> > +			mutex_unlock(&spec->pcm_lock);
> > +			return 0;
> > +		}
> > +
> > +		if (snd_BUG_ON(pin_idx < 0)) {
> > +			mutex_unlock(&spec->pcm_lock);
> >  			return -EINVAL;
> > +		}
> >  		per_pin = get_pin(spec, pin_idx);
> >
> >  		if (spec->dyn_pin_out) {
> > @@ -1900,6 +2039,7 @@ static int hdmi_pcm_close(struct
> hda_pcm_stream *hinfo,
> >  		per_pin->setup = false;
> >  		per_pin->channels = 0;
> >  		mutex_unlock(&per_pin->lock);
> > +		mutex_unlock(&spec->pcm_lock);
> >  	}
> >
> >  	return 0;
> > @@ -2370,6 +2510,7 @@ static int patch_generic_hdmi(struct
> hda_codec *codec)
> >  		return -ENOMEM;
> >
> >  	spec->ops = generic_standard_hdmi_ops;
> > +	mutex_init(&spec->pcm_lock);
> >  	codec->spec = spec;
> >  	hdmi_array_init(spec, 4);
> >
> > --
> > 1.9.1
> >

[RFC PATCH 2/2] ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug

[libin.yang]

From: Libin Yang <libin.yang@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.

Signed-off-by: Libin Yang <libin.yang@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;
 	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;
 	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 */
 	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[] */
 	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

Re: [RFC PATCH 2/2] ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug

[Takashi Iwai]

On Wed, 18 Nov 2015 09:46:59 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@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@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,

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
> 

Re: [RFC PATCH 2/2] ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 18, 2015 7:31 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; mengdong.lin@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@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@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@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
> >

Re: [RFC PATCH 0/2] hdmi audio dynamically bind PCM to pin

[Takashi Iwai]

On Wed, 18 Nov 2015 09:46:57 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> The patches are trying to support dynamically binding PCM to pin in HDMI audio.
> 
> The first patch:
> When PCM is open when there is no pin (port) bound to the PCM,
> the driver try to open the PCM successfully. It will try to find
> an available converter to assign to the PCM.
> In PCM prepare, if there is no pin bound to the PCM, the driver
> will do the simple configuration and return true.
> In PCM close, if there is no pin bound to the PCM, the driver will
> simply close it.
> All the pin configuration will be setup in the hotplug which is done
> in patch 2.
> As the PCM is dynamically bound to the pin, the pcm_lock is necessary
> for the whole pcm open/prepare/close process.

Such a detailed explanation should be put into the patch itself, not
in the cover letter.  The current description is way too sparse.


Takashi

> The second patch:
> The patch mainly handle the hotplug event.
> Each time monitor is connected, a proper PCM will be bound to the pin.
> If the PCM is in use, the pin must be configured.
> Each time monitor is disconnect, the PCM will be unbound from the pin.
> The pin will be reset.
> Libin Yang (2):
>   ALSA: hda - hdmi playback without monitor in dynamic pcm bind mode
>   ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug
> 
>  sound/pci/hda/hda_codec.h  |   1 +
>  sound/pci/hda/patch_hdmi.c | 354 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 337 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [RFC PATCH 0/2] hdmi audio dynamically bind PCM to pin

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 18, 2015 7:24 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; mengdong.lin@linux.intel.com; Yang,
> Libin
> Subject: Re: [alsa-devel] [RFC PATCH 0/2] hdmi audio dynamically bind
> PCM to pin
> 
> On Wed, 18 Nov 2015 09:46:57 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > The patches are trying to support dynamically binding PCM to pin in
> HDMI audio.
> >
> > The first patch:
> > When PCM is open when there is no pin (port) bound to the PCM,
> > the driver try to open the PCM successfully. It will try to find
> > an available converter to assign to the PCM.
> > In PCM prepare, if there is no pin bound to the PCM, the driver
> > will do the simple configuration and return true.
> > In PCM close, if there is no pin bound to the PCM, the driver will
> > simply close it.
> > All the pin configuration will be setup in the hotplug which is done
> > in patch 2.
> > As the PCM is dynamically bound to the pin, the pcm_lock is necessary
> > for the whole pcm open/prepare/close process.
> 
> Such a detailed explanation should be put into the patch itself, not
> in the cover letter.  The current description is way too sparse.

OK. I see.

Regards,
Libin
> 
> 
> Takashi
> 
> > The second patch:
> > The patch mainly handle the hotplug event.
> > Each time monitor is connected, a proper PCM will be bound to the pin.
> > If the PCM is in use, the pin must be configured.
> > Each time monitor is disconnect, the PCM will be unbound from the pin.
> > The pin will be reset.
> > Libin Yang (2):
> >   ALSA: hda - hdmi playback without monitor in dynamic pcm bind
> mode
> >   ALSA: hda - hdmi dynamically bind PCM to pin when monitor hotplug
> >
> >  sound/pci/hda/hda_codec.h  |   1 +
> >  sound/pci/hda/patch_hdmi.c | 354
> ++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 337 insertions(+), 18 deletions(-)
> >
> > --
> > 1.9.1
> >