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

[Yang, Libin]

> -----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?

> 
> 
> 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
> > >