[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 5:02 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: Jaroslav Kysela <perex at perex.cz>; alsa-devel at alsa-project.org; pierre-
>louis.bossart at linux.intel.com; broonie at kernel.org; Lin, Mengdong
><mengdong.lin at intel.com>; Wang, Rander <rander.wang at intel.com>;
>hui.wang at canonical.com
>Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>monitor in hw_params
>
>On Mon, 06 May 2019 10:56:36 +0200,
>Yang, Libin wrote:
>>
>> 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.
>
>This pretty much depends on the driver; the legacy HD-audio has no such
>check.  If it were no monitor connection and a stream is played back, it'll be
>passed to the hardware as-is.

Yes, I refer to legacy HDA driver and wonder if we can remove the check. :)

>
>I don't know why ASoC HD-audio driver has the check, maybe otherwise
>something gets screwed up?  If removing the monitor detection "works"
>(i.e. doesn't give any hiccup in the driver side), it's OK to remove that, of
>course.

I did some test with removing the monitor check, it works well so far.
I will do more tests on removing monitor check tomorrow.

My understanding for the monitor check is that if there is no monitors,
we should not send audio infoframe as there is no sink. But the experiences
show it is OK the audio driver setup the audio infoframe. I believe the
gfx driver or the HW has already handled such situation smoothly.

Regards,
Libin

>
>
>thanks,
>
>Takashi
>
>>
>> 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.
>> >>
>>