Re: [RFC PATCH] ASoC: codec: hdac_hdmi: no checking monitor in hw_params

[Jaroslav Kysela <perex@perex.cz>]

Dne 06. 05. 19 v 10:46 Yang, Libin napsal(a):
> Add Mengdong, Hui, Rander
> 
> Hi Jaroslav,
> 
>> -----Original Message-----
>> From: Jaroslav Kysela [mailto:perex@perex.cz]
>> Sent: Monday, May 6, 2019 4:20 PM
>> To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org
>> Cc: tiwai@suse.de; pierre-louis.bossart@linux.intel.com; broonie@kernel.org;
>> subhransu.s.prusty@intel.com; samreen.nilofer@intel.com
>> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>> monitor in hw_params
>>
>> Dne 06. 05. 19 v 8:59 libin.yang@intel.com napsal(a):
>>> From: Libin Yang <libin.yang@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.
> 
> Before we decided to use UCM, we did some investigation on Jack controls.
> And we found we need do much more changes in driver to support Jack
> Controls. 

How do you handle the dynamic monitor configuration (like when user
disconnects the monitor on-the-fly) then? The control interface can notify the
state change through the Jack controls. The PCM interface does not handle this.

						Jaroslav

> 
> Regards,
> Libin
> 
>>
>> 						Jaroslav
>>
>>>
>>> Signed-off-by: Libin Yang <libin.yang@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@perex.cz>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.