[PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[libin.yang]

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

Add a flag that user can decide to stop HDMI/DP playback when
the corresponding monitor is disconnected and refuse to open PCM
if there is no monitor connected.

Background:
When a monitor is disconnected and a new monitor is connected,
the parameters of the 2 monitors may be different. Audio driver
need handle this situation.

Besides, stopping playback when monitor is disconnected will
help to save the power.

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

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index f503a88..4f5023a 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -47,6 +47,11 @@ static bool static_hdmi_pcm;
 module_param(static_hdmi_pcm, bool, 0644);
 MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
 
+static bool hdmi_pcm_stop_on_disconnect;
+module_param(hdmi_pcm_stop_on_disconnect, bool, 0644);
+MODULE_PARM_DESC(hdmi_pcm_stop_on_disconnect,
+				 "Stop PCM when monitor is disconnected");
+
 #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
 #define is_broadwell(codec)    ((codec)->core.vendor_id == 0x80862808)
 #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
@@ -72,6 +77,7 @@ struct hdmi_spec_per_cvt {
 
 struct hdmi_spec_per_pin {
 	hda_nid_t pin_nid;
+	int pin_idx;
 	int num_mux_nids;
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
 	int mux_idx;
@@ -1456,6 +1462,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	per_pin = get_pin(spec, pin_idx);
 	eld = &per_pin->sink_eld;
 
+	if (hdmi_pcm_stop_on_disconnect && !eld->eld_valid)
+		return -ENODEV;
+
 	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
 	if (err < 0)
 		return err;
@@ -1529,6 +1538,28 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 	return 0;
 }
 
+static void hdmi_pcm_stop(struct hdmi_spec *spec,
+					struct hdmi_spec_per_pin *per_pin)
+{
+	struct hda_codec *codec = per_pin->codec;
+	struct snd_pcm_substream *substream;
+	int pin_idx = per_pin->pin_idx;
+
+	if (!hdmi_pcm_stop_on_disconnect)
+		return;
+
+	substream = get_pcm_rec(spec, pin_idx)->pcm->streams[0].substream;
+
+	snd_pcm_stream_lock_irq(substream);
+	if (substream && substream->runtime &&
+		snd_pcm_running(substream)) {
+		codec_info(codec,
+			"HDMI: monitor disconnected, try to stop playback\n");
+		snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
+	}
+	snd_pcm_stream_unlock_irq(substream);
+}
+
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 {
 	struct hda_jack_tbl *jack;
@@ -1586,6 +1617,8 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 		}
 	}
 
+	if (!eld->eld_valid)
+		hdmi_pcm_stop(spec, per_pin);
 	if (pin_eld->eld_valid != eld->eld_valid)
 		eld_changed = true;
 
@@ -1680,6 +1713,7 @@ 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_idx = pin_idx;
 	per_pin->non_pcm = false;
 
 	err = hdmi_read_pin_conn(codec, pin_idx);
-- 
1.9.1

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Takashi Iwai]

On Wed, 11 Nov 2015 09:39:09 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> Add a flag that user can decide to stop HDMI/DP playback when
> the corresponding monitor is disconnected and refuse to open PCM
> if there is no monitor connected.
> 
> Background:
> When a monitor is disconnected and a new monitor is connected,
> the parameters of the 2 monitors may be different. Audio driver
> need handle this situation.
> 
> Besides, stopping playback when monitor is disconnected will
> help to save the power.
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

Thanks.  Below are just nitpicking, so let's test this patch at first,
especially to see whether it has any significant influence on PA, then
respin with the fixes.

David, care to check in your side, too?

> ---
>  sound/pci/hda/patch_hdmi.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index f503a88..4f5023a 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -47,6 +47,11 @@ static bool static_hdmi_pcm;
>  module_param(static_hdmi_pcm, bool, 0644);
>  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
>  
> +static bool hdmi_pcm_stop_on_disconnect;
> +module_param(hdmi_pcm_stop_on_disconnect, bool, 0644);
> +MODULE_PARM_DESC(hdmi_pcm_stop_on_disconnect,
> +				 "Stop PCM when monitor is disconnected");

This codec driver may be used for multiple devices, e.g. an onboard
GPU and a discrete GPU.  Whether a single flag is best is a slight
concern.  Though, as a test patch, it certainly suffices.

>  #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
>  #define is_broadwell(codec)    ((codec)->core.vendor_id == 0x80862808)
>  #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
> @@ -72,6 +77,7 @@ struct hdmi_spec_per_cvt {
>  
>  struct hdmi_spec_per_pin {
>  	hda_nid_t pin_nid;
> +	int pin_idx;
>  	int num_mux_nids;
>  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
>  	int mux_idx;
> @@ -1456,6 +1462,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  	per_pin = get_pin(spec, pin_idx);
>  	eld = &per_pin->sink_eld;
>  
> +	if (hdmi_pcm_stop_on_disconnect && !eld->eld_valid)
> +		return -ENODEV;

An error code might influence on behavior, so let's check it carefully
while testing.


>  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
>  	if (err < 0)
>  		return err;
> @@ -1529,6 +1538,28 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
>  	return 0;
>  }
>  
> +static void hdmi_pcm_stop(struct hdmi_spec *spec,
> +					struct hdmi_spec_per_pin *per_pin)
> +{
> +	struct hda_codec *codec = per_pin->codec;
> +	struct snd_pcm_substream *substream;
> +	int pin_idx = per_pin->pin_idx;
> +
> +	if (!hdmi_pcm_stop_on_disconnect)
> +		return;
> +
> +	substream = get_pcm_rec(spec, pin_idx)->pcm->streams[0].substream;
> +
> +	snd_pcm_stream_lock_irq(substream);
> +	if (substream && substream->runtime &&
> +		snd_pcm_running(substream)) {

substream NULL check has to be before the lock.


> +		codec_info(codec,
> +			"HDMI: monitor disconnected, try to stop playback\n");

The message is good for testing, but no need to spam at each
disconnect in the production state.  Use codec_dbg() in the final
patch.


thanks,

Takashi

> +		snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
> +	}
> +	snd_pcm_stream_unlock_irq(substream);
> +}
> +
>  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  {
>  	struct hda_jack_tbl *jack;
> @@ -1586,6 +1617,8 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  		}
>  	}
>  
> +	if (!eld->eld_valid)
> +		hdmi_pcm_stop(spec, per_pin);
>  	if (pin_eld->eld_valid != eld->eld_valid)
>  		eld_changed = true;
>  
> @@ -1680,6 +1713,7 @@ 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_idx = pin_idx;
>  	per_pin->non_pcm = false;
>  
>  	err = hdmi_read_pin_conn(codec, pin_idx);
> -- 
> 1.9.1
> 

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Takashi Iwai]

On Wed, 11 Nov 2015 10:02:14 +0100,
Takashi Iwai wrote:
> 
> On Wed, 11 Nov 2015 09:39:09 +0100,
> libin.yang@linux.intel.com wrote:
> > 
> > From: Libin Yang <libin.yang@linux.intel.com>
> > 
> > Add a flag that user can decide to stop HDMI/DP playback when
> > the corresponding monitor is disconnected and refuse to open PCM
> > if there is no monitor connected.
> > 
> > Background:
> > When a monitor is disconnected and a new monitor is connected,
> > the parameters of the 2 monitors may be different. Audio driver
> > need handle this situation.
> > 
> > Besides, stopping playback when monitor is disconnected will
> > help to save the power.
> > 
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> 
> Thanks.  Below are just nitpicking, so let's test this patch at first,
> especially to see whether it has any significant influence on PA, then
> respin with the fixes.
> 
> David, care to check in your side, too?

So I tested this with PA 7.1, and it failed, unfortunately.
In short:
- PA needs the PCM access at probe.  If it gets an error, the device
  will be never enumerated again
- PA removes the device when it's disconnected.  The PCM stop with
  DISCONNECT state leads to the device disappearance.


Takashi

> 
> > ---
> >  sound/pci/hda/patch_hdmi.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index f503a88..4f5023a 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -47,6 +47,11 @@ static bool static_hdmi_pcm;
> >  module_param(static_hdmi_pcm, bool, 0644);
> >  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
> >  
> > +static bool hdmi_pcm_stop_on_disconnect;
> > +module_param(hdmi_pcm_stop_on_disconnect, bool, 0644);
> > +MODULE_PARM_DESC(hdmi_pcm_stop_on_disconnect,
> > +				 "Stop PCM when monitor is disconnected");
> 
> This codec driver may be used for multiple devices, e.g. an onboard
> GPU and a discrete GPU.  Whether a single flag is best is a slight
> concern.  Though, as a test patch, it certainly suffices.
> 
> >  #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
> >  #define is_broadwell(codec)    ((codec)->core.vendor_id == 0x80862808)
> >  #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
> > @@ -72,6 +77,7 @@ struct hdmi_spec_per_cvt {
> >  
> >  struct hdmi_spec_per_pin {
> >  	hda_nid_t pin_nid;
> > +	int pin_idx;
> >  	int num_mux_nids;
> >  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> >  	int mux_idx;
> > @@ -1456,6 +1462,9 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
> >  	per_pin = get_pin(spec, pin_idx);
> >  	eld = &per_pin->sink_eld;
> >  
> > +	if (hdmi_pcm_stop_on_disconnect && !eld->eld_valid)
> > +		return -ENODEV;
> 
> An error code might influence on behavior, so let's check it carefully
> while testing.
> 
> 
> >  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
> >  	if (err < 0)
> >  		return err;
> > @@ -1529,6 +1538,28 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
> >  	return 0;
> >  }
> >  
> > +static void hdmi_pcm_stop(struct hdmi_spec *spec,
> > +					struct hdmi_spec_per_pin *per_pin)
> > +{
> > +	struct hda_codec *codec = per_pin->codec;
> > +	struct snd_pcm_substream *substream;
> > +	int pin_idx = per_pin->pin_idx;
> > +
> > +	if (!hdmi_pcm_stop_on_disconnect)
> > +		return;
> > +
> > +	substream = get_pcm_rec(spec, pin_idx)->pcm->streams[0].substream;
> > +
> > +	snd_pcm_stream_lock_irq(substream);
> > +	if (substream && substream->runtime &&
> > +		snd_pcm_running(substream)) {
> 
> substream NULL check has to be before the lock.
> 
> 
> > +		codec_info(codec,
> > +			"HDMI: monitor disconnected, try to stop playback\n");
> 
> The message is good for testing, but no need to spam at each
> disconnect in the production state.  Use codec_dbg() in the final
> patch.
> 
> 
> thanks,
> 
> Takashi
> 
> > +		snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED);
> > +	}
> > +	snd_pcm_stream_unlock_irq(substream);
> > +}
> > +
> >  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >  {
> >  	struct hda_jack_tbl *jack;
> > @@ -1586,6 +1617,8 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >  		}
> >  	}
> >  
> > +	if (!eld->eld_valid)
> > +		hdmi_pcm_stop(spec, per_pin);
> >  	if (pin_eld->eld_valid != eld->eld_valid)
> >  		eld_changed = true;
> >  
> > @@ -1680,6 +1713,7 @@ 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_idx = pin_idx;
> >  	per_pin->non_pcm = false;
> >  
> >  	err = hdmi_read_pin_conn(codec, pin_idx);
> > -- 
> > 1.9.1
> > 

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 11, 2015 10:13 PM
> To: libin.yang@linux.intel.com
> Cc: David Henningsson; alsa-devel@alsa-project.org;
> mengdong.lin@linux.intel.com; Yang, Libin
> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> On Wed, 11 Nov 2015 10:02:14 +0100,
> Takashi Iwai wrote:
> >
> > On Wed, 11 Nov 2015 09:39:09 +0100,
> > libin.yang@linux.intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > Add a flag that user can decide to stop HDMI/DP playback when
> > > the corresponding monitor is disconnected and refuse to open PCM
> > > if there is no monitor connected.
> > >
> > > Background:
> > > When a monitor is disconnected and a new monitor is connected,
> > > the parameters of the 2 monitors may be different. Audio driver
> > > need handle this situation.
> > >
> > > Besides, stopping playback when monitor is disconnected will
> > > help to save the power.
> > >
> > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> >
> > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > especially to see whether it has any significant influence on PA, then
> > respin with the fixes.
> >
> > David, care to check in your side, too?
> 
> So I tested this with PA 7.1, and it failed, unfortunately.
> In short:
> - PA needs the PCM access at probe.  If it gets an error, the device
>   will be never enumerated again

Thanks for test.

If so, should we re-write the hdmi_pcm_open() and 
generic_hdmi_playback_pcm_prepare() function to make
the probe works? Or not support dynamic pcm assignment?

> - PA removes the device when it's disconnected.  The PCM stop with
>   DISCONNECT state leads to the device disappearance.

If we can't use DISCONNECT, what else can we use? Or we can't
stop PCM when monitor is disconnected?

> 
> 
> Takashi
> 
> >
> > > ---
> > >  sound/pci/hda/patch_hdmi.c | 34
> ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > > index f503a88..4f5023a 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -47,6 +47,11 @@ static bool static_hdmi_pcm;
> > >  module_param(static_hdmi_pcm, bool, 0644);
> > >  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM
> parameters per ELD info");
> > >
> > > +static bool hdmi_pcm_stop_on_disconnect;
> > > +module_param(hdmi_pcm_stop_on_disconnect, bool, 0644);
> > > +MODULE_PARM_DESC(hdmi_pcm_stop_on_disconnect,
> > > +				 "Stop PCM when monitor is
> disconnected");
> >
> > This codec driver may be used for multiple devices, e.g. an onboard
> > GPU and a discrete GPU.  Whether a single flag is best is a slight
> > concern.  Though, as a test patch, it certainly suffices.
> >
> > >  #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
> > >  #define is_broadwell(codec)    ((codec)->core.vendor_id ==
> 0x80862808)
> > >  #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
> > > @@ -72,6 +77,7 @@ struct hdmi_spec_per_cvt {
> > >
> > >  struct hdmi_spec_per_pin {
> > >  	hda_nid_t pin_nid;
> > > +	int pin_idx;
> > >  	int num_mux_nids;
> > >  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> > >  	int mux_idx;
> > > @@ -1456,6 +1462,9 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> > >  	per_pin = get_pin(spec, pin_idx);
> > >  	eld = &per_pin->sink_eld;
> > >
> > > +	if (hdmi_pcm_stop_on_disconnect && !eld->eld_valid)
> > > +		return -ENODEV;
> >
> > An error code might influence on behavior, so let's check it carefully
> > while testing.
> >
> >
> > >  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
> > >  	if (err < 0)
> > >  		return err;
> > > @@ -1529,6 +1538,28 @@ static int hdmi_read_pin_conn(struct
> hda_codec *codec, int pin_idx)
> > >  	return 0;
> > >  }
> > >
> > > +static void hdmi_pcm_stop(struct hdmi_spec *spec,
> > > +					struct hdmi_spec_per_pin
> *per_pin)
> > > +{
> > > +	struct hda_codec *codec = per_pin->codec;
> > > +	struct snd_pcm_substream *substream;
> > > +	int pin_idx = per_pin->pin_idx;
> > > +
> > > +	if (!hdmi_pcm_stop_on_disconnect)
> > > +		return;
> > > +
> > > +	substream = get_pcm_rec(spec, pin_idx)->pcm-
> >streams[0].substream;
> > > +
> > > +	snd_pcm_stream_lock_irq(substream);
> > > +	if (substream && substream->runtime &&
> > > +		snd_pcm_running(substream)) {
> >
> > substream NULL check has to be before the lock.
> >
> >
> > > +		codec_info(codec,
> > > +			"HDMI: monitor disconnected, try to stop
> playback\n");
> >
> > The message is good for testing, but no need to spam at each
> > disconnect in the production state.  Use codec_dbg() in the final
> > patch.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > > +		snd_pcm_stop(substream,
> SNDRV_PCM_STATE_DISCONNECTED);
> > > +	}
> > > +	snd_pcm_stream_unlock_irq(substream);
> > > +}
> > > +
> > >  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin,
> int repoll)
> > >  {
> > >  	struct hda_jack_tbl *jack;
> > > @@ -1586,6 +1617,8 @@ static bool hdmi_present_sense(struct
> hdmi_spec_per_pin *per_pin, int repoll)
> > >  		}
> > >  	}
> > >
> > > +	if (!eld->eld_valid)
> > > +		hdmi_pcm_stop(spec, per_pin);
> > >  	if (pin_eld->eld_valid != eld->eld_valid)
> > >  		eld_changed = true;
> > >
> > > @@ -1680,6 +1713,7 @@ 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_idx = pin_idx;
> > >  	per_pin->non_pcm = false;
> > >
> > >  	err = hdmi_read_pin_conn(codec, pin_idx);
> > > --
> > > 1.9.1
> > >

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Takashi Iwai]

On Wed, 11 Nov 2015 15:23:42 +0100,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 11, 2015 10:13 PM
> > To: libin.yang@linux.intel.com
> > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > mengdong.lin@linux.intel.com; Yang, Libin
> > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > monitor is disconnected
> > 
> > On Wed, 11 Nov 2015 10:02:14 +0100,
> > Takashi Iwai wrote:
> > >
> > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > libin.yang@linux.intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > the corresponding monitor is disconnected and refuse to open PCM
> > > > if there is no monitor connected.
> > > >
> > > > Background:
> > > > When a monitor is disconnected and a new monitor is connected,
> > > > the parameters of the 2 monitors may be different. Audio driver
> > > > need handle this situation.
> > > >
> > > > Besides, stopping playback when monitor is disconnected will
> > > > help to save the power.
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > especially to see whether it has any significant influence on PA, then
> > > respin with the fixes.
> > >
> > > David, care to check in your side, too?
> > 
> > So I tested this with PA 7.1, and it failed, unfortunately.
> > In short:
> > - PA needs the PCM access at probe.  If it gets an error, the device
> >   will be never enumerated again
> 
> Thanks for test.
> 
> If so, should we re-write the hdmi_pcm_open() and 
> generic_hdmi_playback_pcm_prepare() function to make
> the probe works? Or not support dynamic pcm assignment?
> 
> > - PA removes the device when it's disconnected.  The PCM stop with
> >   DISCONNECT state leads to the device disappearance.
> 
> If we can't use DISCONNECT, what else can we use? Or we can't
> stop PCM when monitor is disconnected?

One obvious options is: we don't care about it.  Maybe care it only
when really needed, e.g. return an error only in prepare and cleanup,
but not in open.


Takashi

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 11, 2015 10:42 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; David Henningsson; alsa-devel@alsa-
> project.org; mengdong.lin@linux.intel.com
> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> On Wed, 11 Nov 2015 15:23:42 +0100,
> Yang, Libin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 11, 2015 10:13 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > mengdong.lin@linux.intel.com; Yang, Libin
> > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > monitor is disconnected
> > >
> > > On Wed, 11 Nov 2015 10:02:14 +0100,
> > > Takashi Iwai wrote:
> > > >
> > > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > > libin.yang@linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > > the corresponding monitor is disconnected and refuse to open
> PCM
> > > > > if there is no monitor connected.
> > > > >
> > > > > Background:
> > > > > When a monitor is disconnected and a new monitor is connected,
> > > > > the parameters of the 2 monitors may be different. Audio driver
> > > > > need handle this situation.
> > > > >
> > > > > Besides, stopping playback when monitor is disconnected will
> > > > > help to save the power.
> > > > >
> > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > > especially to see whether it has any significant influence on PA, then
> > > > respin with the fixes.
> > > >
> > > > David, care to check in your side, too?
> > >
> > > So I tested this with PA 7.1, and it failed, unfortunately.
> > > In short:
> > > - PA needs the PCM access at probe.  If it gets an error, the device
> > >   will be never enumerated again
> >
> > Thanks for test.
> >
> > If so, should we re-write the hdmi_pcm_open() and
> > generic_hdmi_playback_pcm_prepare() function to make
> > the probe works? Or not support dynamic pcm assignment?
> >
> > > - PA removes the device when it's disconnected.  The PCM stop with
> > >   DISCONNECT state leads to the device disappearance.
> >
> > If we can't use DISCONNECT, what else can we use? Or we can't
> > stop PCM when monitor is disconnected?
> 
> One obvious options is: we don't care about it.  Maybe care it only
> when really needed, e.g. return an error only in prepare and cleanup,
> but not in open.

Get it. I will refine the patch.

So we will not care the disconnect when playback and
not stop the PCM?

If we don't stop pcm, how to handle connect a different monitor?
It seems userspace must handle such scenario.

Regards,
Libin

> 
> 
> Takashi

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Takashi Iwai]

On Wed, 11 Nov 2015 15:53:05 +0100,
Yang, Libin wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 11, 2015 10:42 PM
> > To: Yang, Libin
> > Cc: libin.yang@linux.intel.com; David Henningsson; alsa-devel@alsa-
> > project.org; mengdong.lin@linux.intel.com
> > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > monitor is disconnected
> > 
> > On Wed, 11 Nov 2015 15:23:42 +0100,
> > Yang, Libin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, November 11, 2015 10:13 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > > mengdong.lin@linux.intel.com; Yang, Libin
> > > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > > monitor is disconnected
> > > >
> > > > On Wed, 11 Nov 2015 10:02:14 +0100,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > > > libin.yang@linux.intel.com wrote:
> > > > > >
> > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > >
> > > > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > > > the corresponding monitor is disconnected and refuse to open
> > PCM
> > > > > > if there is no monitor connected.
> > > > > >
> > > > > > Background:
> > > > > > When a monitor is disconnected and a new monitor is connected,
> > > > > > the parameters of the 2 monitors may be different. Audio driver
> > > > > > need handle this situation.
> > > > > >
> > > > > > Besides, stopping playback when monitor is disconnected will
> > > > > > help to save the power.
> > > > > >
> > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > > > especially to see whether it has any significant influence on PA, then
> > > > > respin with the fixes.
> > > > >
> > > > > David, care to check in your side, too?
> > > >
> > > > So I tested this with PA 7.1, and it failed, unfortunately.
> > > > In short:
> > > > - PA needs the PCM access at probe.  If it gets an error, the device
> > > >   will be never enumerated again
> > >
> > > Thanks for test.
> > >
> > > If so, should we re-write the hdmi_pcm_open() and
> > > generic_hdmi_playback_pcm_prepare() function to make
> > > the probe works? Or not support dynamic pcm assignment?
> > >
> > > > - PA removes the device when it's disconnected.  The PCM stop with
> > > >   DISCONNECT state leads to the device disappearance.
> > >
> > > If we can't use DISCONNECT, what else can we use? Or we can't
> > > stop PCM when monitor is disconnected?
> > 
> > One obvious options is: we don't care about it.  Maybe care it only
> > when really needed, e.g. return an error only in prepare and cleanup,
> > but not in open.
> 
> Get it. I will refine the patch.
> 
> So we will not care the disconnect when playback and
> not stop the PCM?
> 
> If we don't stop pcm, how to handle connect a different monitor?
> It seems userspace must handle such scenario.

Yes.  The driver just wishes that user-space reacts.

Of course, you can do more complex.  It's only the question about
user-space interface, so you can actually stop the stream but makes it
appearing as if a normal error to user-space.  But I don't think it's
worth to play with.


Takashi

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 11, 2015 10:58 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; David Henningsson; alsa-devel@alsa-
> project.org; mengdong.lin@linux.intel.com
> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> On Wed, 11 Nov 2015 15:53:05 +0100,
> Yang, Libin wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 11, 2015 10:42 PM
> > > To: Yang, Libin
> > > Cc: libin.yang@linux.intel.com; David Henningsson; alsa-devel@alsa-
> > > project.org; mengdong.lin@linux.intel.com
> > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > monitor is disconnected
> > >
> > > On Wed, 11 Nov 2015 15:23:42 +0100,
> > > Yang, Libin wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Wednesday, November 11, 2015 10:13 PM
> > > > > To: libin.yang@linux.intel.com
> > > > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > > > mengdong.lin@linux.intel.com; Yang, Libin
> > > > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > > > monitor is disconnected
> > > > >
> > > > > On Wed, 11 Nov 2015 10:02:14 +0100,
> > > > > Takashi Iwai wrote:
> > > > > >
> > > > > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > > > > libin.yang@linux.intel.com wrote:
> > > > > > >
> > > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > > >
> > > > > > > Add a flag that user can decide to stop HDMI/DP playback
> when
> > > > > > > the corresponding monitor is disconnected and refuse to open
> > > PCM
> > > > > > > if there is no monitor connected.
> > > > > > >
> > > > > > > Background:
> > > > > > > When a monitor is disconnected and a new monitor is
> connected,
> > > > > > > the parameters of the 2 monitors may be different. Audio
> driver
> > > > > > > need handle this situation.
> > > > > > >
> > > > > > > Besides, stopping playback when monitor is disconnected will
> > > > > > > help to save the power.
> > > > > > >
> > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > >
> > > > > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > > > > especially to see whether it has any significant influence on PA,
> then
> > > > > > respin with the fixes.
> > > > > >
> > > > > > David, care to check in your side, too?
> > > > >
> > > > > So I tested this with PA 7.1, and it failed, unfortunately.
> > > > > In short:
> > > > > - PA needs the PCM access at probe.  If it gets an error, the device
> > > > >   will be never enumerated again
> > > >
> > > > Thanks for test.
> > > >
> > > > If so, should we re-write the hdmi_pcm_open() and
> > > > generic_hdmi_playback_pcm_prepare() function to make
> > > > the probe works? Or not support dynamic pcm assignment?
> > > >
> > > > > - PA removes the device when it's disconnected.  The PCM stop
> with
> > > > >   DISCONNECT state leads to the device disappearance.
> > > >
> > > > If we can't use DISCONNECT, what else can we use? Or we can't
> > > > stop PCM when monitor is disconnected?
> > >
> > > One obvious options is: we don't care about it.  Maybe care it only
> > > when really needed, e.g. return an error only in prepare and cleanup,
> > > but not in open.
> >
> > Get it. I will refine the patch.
> >
> > So we will not care the disconnect when playback and
> > not stop the PCM?
> >
> > If we don't stop pcm, how to handle connect a different monitor?
> > It seems userspace must handle such scenario.
> 
> Yes.  The driver just wishes that user-space reacts.
> 
> Of course, you can do more complex.  It's only the question about
> user-space interface, so you can actually stop the stream but makes it
> appearing as if a normal error to user-space.  But I don't think it's
> worth to play with.

Get it. Hope gfx is OK for it :)

> 
> 
> Takashi

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[David Henningsson]

On 2015-11-11 15:23, Yang, Libin wrote:
>
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Wednesday, November 11, 2015 10:13 PM
>> To: libin.yang@linux.intel.com
>> Cc: David Henningsson; alsa-devel@alsa-project.org;
>> mengdong.lin@linux.intel.com; Yang, Libin
>> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
>> monitor is disconnected
>>
>> On Wed, 11 Nov 2015 10:02:14 +0100,
>> Takashi Iwai wrote:
>>>
>>> On Wed, 11 Nov 2015 09:39:09 +0100,
>>> libin.yang@linux.intel.com wrote:
>>>>
>>>> From: Libin Yang <libin.yang@linux.intel.com>
>>>>
>>>> Add a flag that user can decide to stop HDMI/DP playback when
>>>> the corresponding monitor is disconnected and refuse to open PCM
>>>> if there is no monitor connected.
>>>>
>>>> Background:
>>>> When a monitor is disconnected and a new monitor is connected,
>>>> the parameters of the 2 monitors may be different. Audio driver
>>>> need handle this situation.
>>>>
>>>> Besides, stopping playback when monitor is disconnected will
>>>> help to save the power.
>>>>
>>>> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>>>
>>> Thanks.  Below are just nitpicking, so let's test this patch at first,
>>> especially to see whether it has any significant influence on PA, then
>>> respin with the fixes.
>>>
>>> David, care to check in your side, too?
>>
>> So I tested this with PA 7.1, and it failed, unfortunately.
>> In short:
>> - PA needs the PCM access at probe.  If it gets an error, the device
>>    will be never enumerated again

...and that's the expected behaviour given how PA works today. Which 
also means that this new parameter should not be enabled by default, 
unless we want to break PA 7.1 and earlier versions.

>
> Thanks for test.
>
> If so, should we re-write the hdmi_pcm_open() and
> generic_hdmi_playback_pcm_prepare() function to make
> the probe works?  Or not support dynamic pcm assignment?

I've already suggested how we can make it work, by assigning converter 
nodes dynamically regardless of whether or not the PCM has a monitor 
connected to it.

Keeping the power well on for up to five seconds (and often much less) 
during boot should not be a big deal power saving wise, so the power 
save argument doesn't hold IMO.

>> - PA removes the device when it's disconnected.  The PCM stop with
>>    DISCONNECT state leads to the device disappearance.
>
> If we can't use DISCONNECT, what else can we use? Or we can't
> stop PCM when monitor is disconnected?

The current behavior is to keep the PCM running, which also has the 
benefit to be able to switch monitors on the fly (i e without stopping 
the stream), if both support the stream format. At least I think one can 
do this, I've actually never tried.

While reporting -ENODEV for a disconnected monitor seems useful to some 
scenarios, there are certainly other scenarios where it doesn't work.

PA should use the ELD and/or the Jack kctl to figure out when it needs 
to reprobe the device. This has not been implemented.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Libin Yang]

On 11/12/2015 03:02 PM, David Henningsson wrote:
>
>
> On 2015-11-11 15:23, Yang, Libin wrote:
>>
>>> -----Original Message-----
>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>> Sent: Wednesday, November 11, 2015 10:13 PM
>>> To: libin.yang@linux.intel.com
>>> Cc: David Henningsson; alsa-devel@alsa-project.org;
>>> mengdong.lin@linux.intel.com; Yang, Libin
>>> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
>>> monitor is disconnected
>>>
>>> On Wed, 11 Nov 2015 10:02:14 +0100,
>>> Takashi Iwai wrote:
>>>>
>>>> On Wed, 11 Nov 2015 09:39:09 +0100,
>>>> libin.yang@linux.intel.com wrote:
>>>>>
>>>>> From: Libin Yang <libin.yang@linux.intel.com>
>>>>>
>>>>> Add a flag that user can decide to stop HDMI/DP playback when
>>>>> the corresponding monitor is disconnected and refuse to open PCM
>>>>> if there is no monitor connected.
>>>>>
>>>>> Background:
>>>>> When a monitor is disconnected and a new monitor is connected,
>>>>> the parameters of the 2 monitors may be different. Audio driver
>>>>> need handle this situation.
>>>>>
>>>>> Besides, stopping playback when monitor is disconnected will
>>>>> help to save the power.
>>>>>
>>>>> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>>>>
>>>> Thanks.  Below are just nitpicking, so let's test this patch at
>>>> first,
>>>> especially to see whether it has any significant influence on PA,
>>>> then
>>>> respin with the fixes.
>>>>
>>>> David, care to check in your side, too?
>>>
>>> So I tested this with PA 7.1, and it failed, unfortunately.
>>> In short:
>>> - PA needs the PCM access at probe.  If it gets an error, the device
>>>    will be never enumerated again
>
> ...and that's the expected behaviour given how PA works today. Which
> also means that this new parameter should not be enabled by default,
> unless we want to break PA 7.1 and earlier versions.
>
>>
>> Thanks for test.
>>
>> If so, should we re-write the hdmi_pcm_open() and
>> generic_hdmi_playback_pcm_prepare() function to make
>> the probe works?  Or not support dynamic pcm assignment?
>
> I've already suggested how we can make it work, by assigning converter
> nodes dynamically regardless of whether or not the PCM has a monitor
> connected to it.

It means the PCM will hold the converter even no monitor is connected. 
When will PA release the converter? If PA can't release converter in 
time, other PCM will not find a proper converter. Especially in probe 
time, will it cause probe failure?

>
> Keeping the power well on for up to five seconds (and often much less)
> during boot should not be a big deal power saving wise, so the power
> save argument doesn't hold IMO.

That's fine for power saving.

>
>>> - PA removes the device when it's disconnected.  The PCM stop with
>>>    DISCONNECT state leads to the device disappearance.
>>
>> If we can't use DISCONNECT, what else can we use? Or we can't
>> stop PCM when monitor is disconnected?
>
> The current behavior is to keep the PCM running, which also has the
> benefit to be able to switch monitors on the fly (i e without stopping
> the stream), if both support the stream format. At least I think one
> can do this, I've actually never tried.

Yes, we will keep PCM running even when monitor is disconnected.

Regards,
Libin

>
> While reporting -ENODEV for a disconnected monitor seems useful to
> some scenarios, there are certainly other scenarios where it doesn't
> work.
>
> PA should use the ELD and/or the Jack kctl to figure out when it needs
> to reprobe the device. This has not been implemented.
>

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[David Henningsson]

On 2015-11-12 08:26, Libin Yang wrote:
>
> On 11/12/2015 03:02 PM, David Henningsson wrote:
>>
>>
>> On 2015-11-11 15:23, Yang, Libin wrote:
>>>
>>>> -----Original Message-----
>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>> Sent: Wednesday, November 11, 2015 10:13 PM
>>>> To: libin.yang@linux.intel.com
>>>> Cc: David Henningsson; alsa-devel@alsa-project.org;
>>>> mengdong.lin@linux.intel.com; Yang, Libin
>>>> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
>>>> monitor is disconnected
>>>>
>>>> On Wed, 11 Nov 2015 10:02:14 +0100,
>>>> Takashi Iwai wrote:
>>>>>
>>>>> On Wed, 11 Nov 2015 09:39:09 +0100,
>>>>> libin.yang@linux.intel.com wrote:
>>>>>>
>>>>>> From: Libin Yang <libin.yang@linux.intel.com>
>>>>>>
>>>>>> Add a flag that user can decide to stop HDMI/DP playback when
>>>>>> the corresponding monitor is disconnected and refuse to open PCM
>>>>>> if there is no monitor connected.
>>>>>>
>>>>>> Background:
>>>>>> When a monitor is disconnected and a new monitor is connected,
>>>>>> the parameters of the 2 monitors may be different. Audio driver
>>>>>> need handle this situation.
>>>>>>
>>>>>> Besides, stopping playback when monitor is disconnected will
>>>>>> help to save the power.
>>>>>>
>>>>>> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>>>>>
>>>>> Thanks.  Below are just nitpicking, so let's test this patch at
>>>>> first,
>>>>> especially to see whether it has any significant influence on PA,
>>>>> then
>>>>> respin with the fixes.
>>>>>
>>>>> David, care to check in your side, too?
>>>>
>>>> So I tested this with PA 7.1, and it failed, unfortunately.
>>>> In short:
>>>> - PA needs the PCM access at probe.  If it gets an error, the device
>>>>    will be never enumerated again
>>
>> ...and that's the expected behaviour given how PA works today. Which
>> also means that this new parameter should not be enabled by default,
>> unless we want to break PA 7.1 and earlier versions.
>>
>>>
>>> Thanks for test.
>>>
>>> If so, should we re-write the hdmi_pcm_open() and
>>> generic_hdmi_playback_pcm_prepare() function to make
>>> the probe works?  Or not support dynamic pcm assignment?
>>
>> I've already suggested how we can make it work, by assigning converter
>> nodes dynamically regardless of whether or not the PCM has a monitor
>> connected to it.
>
> It means the PCM will hold the converter even no monitor is connected.

As long as the PCM is open, yes.

> When will PA release the converter? If PA can't release converter in
> time, other PCM will not find a proper converter. Especially in probe
> time, will it cause probe failure?

During probe, PA probes one output + one input at a time (at least by 
default). E g, before PA tries to open "hdmi:0,1", it closes "hdmi:0,0". 
And when PA calls snd_pcm_close, that should also make the driver 
release the converter node.

>> Keeping the power well on for up to five seconds (and often much less)
>> during boot should not be a big deal power saving wise, so the power
>> save argument doesn't hold IMO.
>
> That's fine for power saving.
>
>>
>>>> - PA removes the device when it's disconnected.  The PCM stop with
>>>>    DISCONNECT state leads to the device disappearance.
>>>
>>> If we can't use DISCONNECT, what else can we use? Or we can't
>>> stop PCM when monitor is disconnected?
>>
>> The current behavior is to keep the PCM running, which also has the
>> benefit to be able to switch monitors on the fly (i e without stopping
>> the stream), if both support the stream format. At least I think one
>> can do this, I've actually never tried.
>
> Yes, we will keep PCM running even when monitor is disconnected.
>
> Regards,
> Libin
>
>>
>> While reporting -ENODEV for a disconnected monitor seems useful to
>> some scenarios, there are certainly other scenarios where it doesn't
>> work.
>>
>> PA should use the ELD and/or the Jack kctl to figure out when it needs
>> to reprobe the device. This has not been implemented.
>>
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Libin Yang]

On 11/12/2015 03:32 PM, David Henningsson wrote:
>
>
> On 2015-11-12 08:26, Libin Yang wrote:
>>
>> On 11/12/2015 03:02 PM, David Henningsson wrote:
>>>
>>>
>>> On 2015-11-11 15:23, Yang, Libin wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>> Sent: Wednesday, November 11, 2015 10:13 PM
>>>>> To: libin.yang@linux.intel.com
>>>>> Cc: David Henningsson; alsa-devel@alsa-project.org;
>>>>> mengdong.lin@linux.intel.com; Yang, Libin
>>>>> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
>>>>> monitor is disconnected
>>>>>
>>>>> On Wed, 11 Nov 2015 10:02:14 +0100,
>>>>> Takashi Iwai wrote:
>>>>>>
>>>>>> On Wed, 11 Nov 2015 09:39:09 +0100,
>>>>>> libin.yang@linux.intel.com wrote:
>>>>>>>
>>>>>>> From: Libin Yang <libin.yang@linux.intel.com>
>>>>>>>
>>>>>>> Add a flag that user can decide to stop HDMI/DP playback when
>>>>>>> the corresponding monitor is disconnected and refuse to open PCM
>>>>>>> if there is no monitor connected.
>>>>>>>
>>>>>>> Background:
>>>>>>> When a monitor is disconnected and a new monitor is connected,
>>>>>>> the parameters of the 2 monitors may be different. Audio driver
>>>>>>> need handle this situation.
>>>>>>>
>>>>>>> Besides, stopping playback when monitor is disconnected will
>>>>>>> help to save the power.
>>>>>>>
>>>>>>> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>>>>>>
>>>>>> Thanks.  Below are just nitpicking, so let's test this patch at
>>>>>> first,
>>>>>> especially to see whether it has any significant influence on PA,
>>>>>> then
>>>>>> respin with the fixes.
>>>>>>
>>>>>> David, care to check in your side, too?
>>>>>
>>>>> So I tested this with PA 7.1, and it failed, unfortunately.
>>>>> In short:
>>>>> - PA needs the PCM access at probe.  If it gets an error, the device
>>>>>    will be never enumerated again
>>>
>>> ...and that's the expected behaviour given how PA works today. Which
>>> also means that this new parameter should not be enabled by default,
>>> unless we want to break PA 7.1 and earlier versions.
>>>
>>>>
>>>> Thanks for test.
>>>>
>>>> If so, should we re-write the hdmi_pcm_open() and
>>>> generic_hdmi_playback_pcm_prepare() function to make
>>>> the probe works?  Or not support dynamic pcm assignment?
>>>
>>> I've already suggested how we can make it work, by assigning converter
>>> nodes dynamically regardless of whether or not the PCM has a monitor
>>> connected to it.
>>
>> It means the PCM will hold the converter even no monitor is connected.
>
> As long as the PCM is open, yes.
>
>> When will PA release the converter? If PA can't release converter in
>> time, other PCM will not find a proper converter. Especially in probe
>> time, will it cause probe failure?
>
> During probe, PA probes one output + one input at a time (at least by
> default). E g, before PA tries to open "hdmi:0,1", it closes
> "hdmi:0,0". And when PA calls snd_pcm_close, that should also make the
> driver release the converter node.

That's good. I will try it if Takashi is OK.

Thanks,
Libin

>
>>> Keeping the power well on for up to five seconds (and often much less)
>>> during boot should not be a big deal power saving wise, so the power
>>> save argument doesn't hold IMO.
>>
>> That's fine for power saving.
>>
>>>
>>>>> - PA removes the device when it's disconnected.  The PCM stop with
>>>>>    DISCONNECT state leads to the device disappearance.
>>>>
>>>> If we can't use DISCONNECT, what else can we use? Or we can't
>>>> stop PCM when monitor is disconnected?
>>>
>>> The current behavior is to keep the PCM running, which also has the
>>> benefit to be able to switch monitors on the fly (i e without stopping
>>> the stream), if both support the stream format. At least I think one
>>> can do this, I've actually never tried.
>>
>> Yes, we will keep PCM running even when monitor is disconnected.
>>
>> Regards,
>> Libin
>>
>>>
>>> While reporting -ENODEV for a disconnected monitor seems useful to
>>> some scenarios, there are certainly other scenarios where it doesn't
>>> work.
>>>
>>> PA should use the ELD and/or the Jack kctl to figure out when it needs
>>> to reprobe the device. This has not been implemented.
>>>
>>
>

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Yang, Libin]

> -----Original Message-----
> From: Yang, Libin
> Sent: Wednesday, November 11, 2015 10:24 PM
> To: 'Takashi Iwai'; libin.yang@linux.intel.com
> Cc: David Henningsson; alsa-devel@alsa-project.org;
> mengdong.lin@linux.intel.com
> Subject: RE: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 11, 2015 10:13 PM
> > To: libin.yang@linux.intel.com
> > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > mengdong.lin@linux.intel.com; Yang, Libin
> > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > monitor is disconnected
> >
> > On Wed, 11 Nov 2015 10:02:14 +0100,
> > Takashi Iwai wrote:
> > >
> > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > libin.yang@linux.intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > the corresponding monitor is disconnected and refuse to open PCM
> > > > if there is no monitor connected.
> > > >
> > > > Background:
> > > > When a monitor is disconnected and a new monitor is connected,
> > > > the parameters of the 2 monitors may be different. Audio driver
> > > > need handle this situation.
> > > >
> > > > Besides, stopping playback when monitor is disconnected will
> > > > help to save the power.
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > especially to see whether it has any significant influence on PA, then
> > > respin with the fixes.
> > >
> > > David, care to check in your side, too?
> >
> > So I tested this with PA 7.1, and it failed, unfortunately.
> > In short:
> > - PA needs the PCM access at probe.  If it gets an error, the device
> >   will be never enumerated again
> 
> Thanks for test.
> 
> If so, should we re-write the hdmi_pcm_open() and
> generic_hdmi_playback_pcm_prepare() function to make
> the probe works? Or not support dynamic pcm assignment?
> 
> > - PA removes the device when it's disconnected.  The PCM stop with
> >   DISCONNECT state leads to the device disappearance.
> 
> If we can't use DISCONNECT, what else can we use? Or we can't
> stop PCM when monitor is disconnected?

BTW: I did the test on aplay. But I missed the test on PA.
Where can I get the PA test cases?

Regards,
Libin

> 
> >
> >
> > Takashi
> >
> > >
> > > > ---
> > > >  sound/pci/hda/patch_hdmi.c | 34
> > ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/sound/pci/hda/patch_hdmi.c
> > b/sound/pci/hda/patch_hdmi.c
> > > > index f503a88..4f5023a 100644
> > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > @@ -47,6 +47,11 @@ static bool static_hdmi_pcm;
> > > >  module_param(static_hdmi_pcm, bool, 0644);
> > > >  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM
> > parameters per ELD info");
> > > >
> > > > +static bool hdmi_pcm_stop_on_disconnect;
> > > > +module_param(hdmi_pcm_stop_on_disconnect, bool, 0644);
> > > > +MODULE_PARM_DESC(hdmi_pcm_stop_on_disconnect,
> > > > +				 "Stop PCM when monitor is
> > disconnected");
> > >
> > > This codec driver may be used for multiple devices, e.g. an onboard
> > > GPU and a discrete GPU.  Whether a single flag is best is a slight
> > > concern.  Though, as a test patch, it certainly suffices.
> > >
> > > >  #define is_haswell(codec)  ((codec)->core.vendor_id ==
> 0x80862807)
> > > >  #define is_broadwell(codec)    ((codec)->core.vendor_id ==
> > 0x80862808)
> > > >  #define is_skylake(codec) ((codec)->core.vendor_id ==
> 0x80862809)
> > > > @@ -72,6 +77,7 @@ struct hdmi_spec_per_cvt {
> > > >
> > > >  struct hdmi_spec_per_pin {
> > > >  	hda_nid_t pin_nid;
> > > > +	int pin_idx;
> > > >  	int num_mux_nids;
> > > >  	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
> > > >  	int mux_idx;
> > > > @@ -1456,6 +1462,9 @@ static int hdmi_pcm_open(struct
> > hda_pcm_stream *hinfo,
> > > >  	per_pin = get_pin(spec, pin_idx);
> > > >  	eld = &per_pin->sink_eld;
> > > >
> > > > +	if (hdmi_pcm_stop_on_disconnect && !eld->eld_valid)
> > > > +		return -ENODEV;
> > >
> > > An error code might influence on behavior, so let's check it carefully
> > > while testing.
> > >
> > >
> > > >  	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx, &mux_idx);
> > > >  	if (err < 0)
> > > >  		return err;
> > > > @@ -1529,6 +1538,28 @@ static int hdmi_read_pin_conn(struct
> > hda_codec *codec, int pin_idx)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void hdmi_pcm_stop(struct hdmi_spec *spec,
> > > > +					struct hdmi_spec_per_pin
> > *per_pin)
> > > > +{
> > > > +	struct hda_codec *codec = per_pin->codec;
> > > > +	struct snd_pcm_substream *substream;
> > > > +	int pin_idx = per_pin->pin_idx;
> > > > +
> > > > +	if (!hdmi_pcm_stop_on_disconnect)
> > > > +		return;
> > > > +
> > > > +	substream = get_pcm_rec(spec, pin_idx)->pcm-
> > >streams[0].substream;
> > > > +
> > > > +	snd_pcm_stream_lock_irq(substream);
> > > > +	if (substream && substream->runtime &&
> > > > +		snd_pcm_running(substream)) {
> > >
> > > substream NULL check has to be before the lock.
> > >
> > >
> > > > +		codec_info(codec,
> > > > +			"HDMI: monitor disconnected, try to stop
> > playback\n");
> > >
> > > The message is good for testing, but no need to spam at each
> > > disconnect in the production state.  Use codec_dbg() in the final
> > > patch.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > > +		snd_pcm_stop(substream,
> > SNDRV_PCM_STATE_DISCONNECTED);
> > > > +	}
> > > > +	snd_pcm_stream_unlock_irq(substream);
> > > > +}
> > > > +
> > > >  static bool hdmi_present_sense(struct hdmi_spec_per_pin
> *per_pin,
> > int repoll)
> > > >  {
> > > >  	struct hda_jack_tbl *jack;
> > > > @@ -1586,6 +1617,8 @@ static bool hdmi_present_sense(struct
> > hdmi_spec_per_pin *per_pin, int repoll)
> > > >  		}
> > > >  	}
> > > >
> > > > +	if (!eld->eld_valid)
> > > > +		hdmi_pcm_stop(spec, per_pin);
> > > >  	if (pin_eld->eld_valid != eld->eld_valid)
> > > >  		eld_changed = true;
> > > >
> > > > @@ -1680,6 +1713,7 @@ 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_idx = pin_idx;
> > > >  	per_pin->non_pcm = false;
> > > >
> > > >  	err = hdmi_read_pin_conn(codec, pin_idx);
> > > > --
> > > > 1.9.1
> > > >

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Takashi Iwai]

On Wed, 11 Nov 2015 15:43:00 +0100,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Yang, Libin
> > Sent: Wednesday, November 11, 2015 10:24 PM
> > To: 'Takashi Iwai'; libin.yang@linux.intel.com
> > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > mengdong.lin@linux.intel.com
> > Subject: RE: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > monitor is disconnected
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 11, 2015 10:13 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > mengdong.lin@linux.intel.com; Yang, Libin
> > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > monitor is disconnected
> > >
> > > On Wed, 11 Nov 2015 10:02:14 +0100,
> > > Takashi Iwai wrote:
> > > >
> > > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > > libin.yang@linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > > the corresponding monitor is disconnected and refuse to open PCM
> > > > > if there is no monitor connected.
> > > > >
> > > > > Background:
> > > > > When a monitor is disconnected and a new monitor is connected,
> > > > > the parameters of the 2 monitors may be different. Audio driver
> > > > > need handle this situation.
> > > > >
> > > > > Besides, stopping playback when monitor is disconnected will
> > > > > help to save the power.
> > > > >
> > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > > especially to see whether it has any significant influence on PA, then
> > > > respin with the fixes.
> > > >
> > > > David, care to check in your side, too?
> > >
> > > So I tested this with PA 7.1, and it failed, unfortunately.
> > > In short:
> > > - PA needs the PCM access at probe.  If it gets an error, the device
> > >   will be never enumerated again
> > 
> > Thanks for test.
> > 
> > If so, should we re-write the hdmi_pcm_open() and
> > generic_hdmi_playback_pcm_prepare() function to make
> > the probe works? Or not support dynamic pcm assignment?
> > 
> > > - PA removes the device when it's disconnected.  The PCM stop with
> > >   DISCONNECT state leads to the device disappearance.
> > 
> > If we can't use DISCONNECT, what else can we use? Or we can't
> > stop PCM when monitor is disconnected?
> 
> BTW: I did the test on aplay. But I missed the test on PA.
> Where can I get the PA test cases?

Just use PA on your desktop.  Start PA with and without monitor
plugged.  Play via DP, then unplug, then watch the state change of
PA.


Takashi

Re: [PATCH] ALSA - hda: hdmi flag to stop playback when monitor is disconnected

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 11, 2015 11:00 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; David Henningsson; alsa-devel@alsa-
> project.org; mengdong.lin@linux.intel.com
> Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> On Wed, 11 Nov 2015 15:43:00 +0100,
> Yang, Libin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yang, Libin
> > > Sent: Wednesday, November 11, 2015 10:24 PM
> > > To: 'Takashi Iwai'; libin.yang@linux.intel.com
> > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > mengdong.lin@linux.intel.com
> > > Subject: RE: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > monitor is disconnected
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, November 11, 2015 10:13 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: David Henningsson; alsa-devel@alsa-project.org;
> > > > mengdong.lin@linux.intel.com; Yang, Libin
> > > > Subject: Re: [PATCH] ALSA - hda: hdmi flag to stop playback when
> > > > monitor is disconnected
> > > >
> > > > On Wed, 11 Nov 2015 10:02:14 +0100,
> > > > Takashi Iwai wrote:
> > > > >
> > > > > On Wed, 11 Nov 2015 09:39:09 +0100,
> > > > > libin.yang@linux.intel.com wrote:
> > > > > >
> > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > >
> > > > > > Add a flag that user can decide to stop HDMI/DP playback when
> > > > > > the corresponding monitor is disconnected and refuse to open
> PCM
> > > > > > if there is no monitor connected.
> > > > > >
> > > > > > Background:
> > > > > > When a monitor is disconnected and a new monitor is connected,
> > > > > > the parameters of the 2 monitors may be different. Audio driver
> > > > > > need handle this situation.
> > > > > >
> > > > > > Besides, stopping playback when monitor is disconnected will
> > > > > > help to save the power.
> > > > > >
> > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > Thanks.  Below are just nitpicking, so let's test this patch at first,
> > > > > especially to see whether it has any significant influence on PA,
> then
> > > > > respin with the fixes.
> > > > >
> > > > > David, care to check in your side, too?
> > > >
> > > > So I tested this with PA 7.1, and it failed, unfortunately.
> > > > In short:
> > > > - PA needs the PCM access at probe.  If it gets an error, the device
> > > >   will be never enumerated again
> > >
> > > Thanks for test.
> > >
> > > If so, should we re-write the hdmi_pcm_open() and
> > > generic_hdmi_playback_pcm_prepare() function to make
> > > the probe works? Or not support dynamic pcm assignment?
> > >
> > > > - PA removes the device when it's disconnected.  The PCM stop with
> > > >   DISCONNECT state leads to the device disappearance.
> > >
> > > If we can't use DISCONNECT, what else can we use? Or we can't
> > > stop PCM when monitor is disconnected?
> >
> > BTW: I did the test on aplay. But I missed the test on PA.
> > Where can I get the PA test cases?
> 
> Just use PA on your desktop.  Start PA with and without monitor
> plugged.  Play via DP, then unplug, then watch the state change of
> PA.

Get it. Thanks.

Regards,
Libin

> 
> 
> Takashi