[alsa-devel] [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 at linux.intel.com
> Cc: David Henningsson; alsa-devel at alsa-project.org;
> mengdong.lin at linux.intel.com
> Subject: RE: [PATCH] ALSA - hda: hdmi flag to stop playback when
> monitor is disconnected
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Wednesday, November 11, 2015 10:13 PM
> > To: libin.yang at linux.intel.com
> > Cc: David Henningsson; alsa-devel at alsa-project.org;
> > mengdong.lin at 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 at linux.intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang at 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 at 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
> > > >