[alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking monitor in hw_params

[Yang, Libin]

Hi Takashi,


>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Monday, May 6, 2019 4:40 PM
>To: Jaroslav Kysela <perex at perex.cz>
>Cc: Yang, Libin <libin.yang at intel.com>; alsa-devel at alsa-project.org; pierre-
>louis.bossart at linux.intel.com; broonie at kernel.org;
>subhransu.s.prusty at intel.com; samreen.nilofer at intel.com
>Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>monitor in hw_params
>
>On Mon, 06 May 2019 10:19:48 +0200,
>Jaroslav Kysela wrote:
>>
>> Dne 06. 05. 19 v 8:59 libin.yang at intel.com napsal(a):
>> > From: Libin Yang <libin.yang at intel.com>
>> >
>> > This patch move the check of monitor from hw_params to trigger callback.
>> >
>> > The original code will check the monitor presence in hw_params. If
>> > the monitor doesn't exist, hw_params will return -ENODEV. Mostly this is
>OK.
>> >
>> > However, pulseaudio will check the pcm devices when kernel is booting up.
>> > It will try to open, set hw_params, prepare such pcm devices. We
>> > can't guarantee that the monitor will be connected when kernel is booting
>up.
>> > Especially, hdac_hdmi will export 3 pcms at most. It's hard to say
>> > users will connect 3 monitors to the HDMI/DP ports. This will cause
>> > pulseaudio fail in parsing the pcm devices because the driver will
>> > return -ENODEV in hw_params.
>> >
>> > This patch tries to move the check of monitor presence into trigger
>> > callback. This can "trick" the pulseaudio the pcm is ready.
>> >
>> > This bug is found when we try to enable HDMI detection in
>> > gnome-sound-setting for ASoC hdac_hdmi. After we enable the hdmi in
>> > UCM, pulseaudio will try to parse the hdmi pcm devices. It will
>> > cause failure if there are no monitors connected.
>>
>> I don't like this solution much. PA should use the Jack control to add
>> the devices dynamically and avoid probing when the Jack control is false.
>
>Ideally, yes, but this isn't going to happen soon, I'm afraid.  And we're still
>responsible for fixing for the existing platforms.  So I find the proposed patch
>OK although it's hackish.  The added code in the trigger has almost no
>overhead, and it won't break stuff.

Yes, this is why we want to use UCM. We need more changes in the driver to
use Jack control.

I have another question:
Is it possible that we remove the monitor detection? I mean just returning ok
even there are no monitors?

There is one case: playback -> disconnect monitor -> S3.
As it will trigger start after S3, my patch will cause playback to exit as it will check
the monitors.
The original code will not break playback in this case.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>>
>> 						Jaroslav
>>
>> >
>> > Signed-off-by: Libin Yang <libin.yang at intel.com>
>> > ---
>> >  sound/soc/codecs/hdac_hdmi.c | 44
>> > +++++++++++++++++++++++++++++++-------------
>> >  1 file changed, 31 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/sound/soc/codecs/hdac_hdmi.c
>> > b/sound/soc/codecs/hdac_hdmi.c index 4de1fbf..f482e09 100644
>> > --- a/sound/soc/codecs/hdac_hdmi.c
>> > +++ b/sound/soc/codecs/hdac_hdmi.c
>> > @@ -455,24 +455,11 @@ static int hdac_hdmi_set_hw_params(struct
>snd_pcm_substream *substream,
>> >  	struct snd_pcm_hw_params *hparams, struct snd_soc_dai *dai)  {
>> >  	struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai);
>> > -	struct hdac_device *hdev = hdmi->hdev;
>> >  	struct hdac_hdmi_dai_port_map *dai_map;
>> > -	struct hdac_hdmi_port *port;
>> >  	struct hdac_hdmi_pcm *pcm;
>> >  	int format;
>> >
>> >  	dai_map = &hdmi->dai_map[dai->id];
>> > -	port = dai_map->port;
>> > -
>> > -	if (!port)
>> > -		return -ENODEV;
>> > -
>> > -	if ((!port->eld.monitor_present) || (!port->eld.eld_valid)) {
>> > -		dev_err(&hdev->dev,
>> > -			"device is not configured for this pin:port%d:%d\n",
>> > -					port->pin->nid, port->id);
>> > -		return -ENODEV;
>> > -	}
>> >
>> >  	format = snd_hdac_calc_stream_format(params_rate(hparams),
>> >  			params_channels(hparams),
>params_format(hparams), @@ -630,6
>> > +617,36 @@ static void hdac_hdmi_pcm_close(struct snd_pcm_substream
>*substream,
>> >  		dai_map->port = NULL;
>> >  }
>> >
>> > +static int hdac_hdmi_pcm_trigger(struct snd_pcm_substream *substream,
>int cmd,
>> > +				 struct snd_soc_dai *dai)
>> > +{
>> > +	struct hdac_hdmi_port *port;
>> > +	struct hdac_hdmi_dai_port_map *dai_map;
>> > +	struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai);
>> > +	struct hdac_device *hdev = hdmi->hdev;
>> > +
>> > +	/*
>> > +	 * When start, if there is no monitor,
>> > +	 * It should not start audio.
>> > +	 */
>> > +	if (cmd == SNDRV_PCM_TRIGGER_START) {
>> > +		dai_map = &hdmi->dai_map[dai->id];
>> > +		port = dai_map->port;
>> > +
>> > +		if (!port)
>> > +			return -ENODEV;
>> > +
>> > +		if ((!port->eld.monitor_present) || (!port->eld.eld_valid)) {
>> > +			dev_err(&hdev->dev,
>> > +				"device is not configured for this
>pin:port%d:%d\n",
>> > +				port->pin->nid, port->id);
>> > +			return -ENODEV;
>> > +		}
>> > +	}
>> > +
>> > +	return 0;
>> > +}
>> > +
>> >  static int
>> >  hdac_hdmi_query_cvt_params(struct hdac_device *hdev, struct
>> > hdac_hdmi_cvt *cvt)  { @@ -1389,6 +1406,7 @@ static const struct
>> > snd_soc_dai_ops hdmi_dai_ops = {
>> >  	.startup = hdac_hdmi_pcm_open,
>> >  	.shutdown = hdac_hdmi_pcm_close,
>> >  	.hw_params = hdac_hdmi_set_hw_params,
>> > +	.trigger = hdac_hdmi_pcm_trigger,
>> >  	.set_tdm_slot = hdac_hdmi_set_tdm_slot,  };
>> >
>> >
>>
>>
>> --
>> Jaroslav Kysela <perex at perex.cz>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>>