[alsa-devel] [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 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
- 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
> >