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

[Takashi Iwai]

On Mon, 06 May 2019 11:25:16 +0200,
Yang, Libin wrote:
> 
> Hi Jaroslav,
> 
> >-----Original Message-----
> >From: Jaroslav Kysela [mailto:perex at perex.cz]
> >Sent: Monday, May 6, 2019 5:04 PM
> >To: Yang, Libin <libin.yang at intel.com>; alsa-devel at alsa-project.org
> >Cc: tiwai at suse.de; pierre-louis.bossart at linux.intel.com; broonie at kernel.org;
> >Hui Wang <hui.wang at canonical.com>; Lin, Mengdong
> ><mengdong.lin at intel.com>; Wang, Rander <rander.wang at intel.com>
> >Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
> >monitor in hw_params
> >
> >Dne 06. 05. 19 v 10:46 Yang, Libin napsal(a):
> >> Add Mengdong, Hui, Rander
> >>
> >> Hi Jaroslav,
> >>
> >>> -----Original Message-----
> >>> From: Jaroslav Kysela [mailto:perex at perex.cz]
> >>> Sent: Monday, May 6, 2019 4:20 PM
> >>> To: Yang, Libin <libin.yang at intel.com>; alsa-devel at alsa-project.org
> >>> Cc: tiwai at suse.de; 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
> >>>
> >>> 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.
> >>
> >> 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.
> 
> We may still need Jack control for this case. This is my second plan to enable
> Jack control. But even we enable Jack control, it seems it is still easy to use UCM.
> This is because ASoC control names don't follow legacy HDA rule. This means
> we may need to change other controls name besides HDMI in the driver or we 
> may need to add more configures in pulseaudio/alsa-mixer/paths.

Ah, I overlooked that hdac_hdmi chose the own jack control names.
That's basically useless, yeah.

> Hi Mengdong & Hui,
> 
> Do you have more comments for Jack control or UCM?

I guess a UCM profile would handle the jack detection as well, by
specifying the right kcontrol names, right?


thanks,

Takashi

> 
> Regards,
> Libin
> 
> >
> >						Jaroslav
> >
> >>
> >> Regards,
> >> Libin
> >>
> >>>
> >>> 						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.
> >
> >
> >--
> >Jaroslav Kysela <perex at perex.cz>
> >Linux Sound Maintainer; ALSA Project; Red Hat, Inc.