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

Commit Message

Yang, Libin May 6, 2019, 6:59 a.m. UTC

From: Libin Yang <[e-mail]>

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.

Signed-off-by: Libin Yang <[e-mail]>
---
 sound/soc/codecs/hdac_hdmi.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

Comments

Jaroslav Kysela May 6, 2019, 8:19 a.m. UTC | #1

Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
> From: Libin Yang <[e-mail]>
> 
> 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.

						Jaroslav

> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  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,
>  };
>  
>

Takashi Iwai May 6, 2019, 8:39 a.m. UTC | #2

On Mon, 06 May 2019 10:19:48 +0200,
Jaroslav Kysela wrote:
> 
> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
> > From: Libin Yang <[e-mail]>
> > 
> > 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.


thanks,

Takashi

> 
> 						Jaroslav
> 
> > 
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  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 <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

Yang, Libin May 6, 2019, 8:46 a.m. UTC | #3

Add Mengdong, Hui, Rander

Hi Jaroslav,

>-----Original Message-----
>From: Jaroslav Kysela [mailto:[e-mail]]
>Sent: Monday, May 6, 2019 4:20 PM
>To: Yang, Libin <[e-mail]>; [e-mail]
>Cc: [e-mail]; [e-mail]; [e-mail];
>[e-mail]; [e-mail]
>Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>monitor in hw_params
>
>Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
>> From: Libin Yang <[e-mail]>
>>
>> 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. 

Regards,
Libin

>
>						Jaroslav
>
>>
>> Signed-off-by: Libin Yang <[e-mail]>
>> ---
>>  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 <[e-mail]>
>Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Yang, Libin May 6, 2019, 8:56 a.m. UTC | #4

Hi Takashi,


>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Monday, May 6, 2019 4:40 PM
>To: Jaroslav Kysela <[e-mail]>
>Cc: Yang, Libin <[e-mail]>; [e-mail]; pierre-
>[e-mail]; [e-mail];
>[e-mail]; [e-mail]
>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 [e-mail] napsal(a):
>> > From: Libin Yang <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > ---
>> >  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 <[e-mail]>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>>

Takashi Iwai May 6, 2019, 9:01 a.m. UTC | #5

On Mon, 06 May 2019 10:56:36 +0200,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:[e-mail]]
> >Sent: Monday, May 6, 2019 4:40 PM
> >To: Jaroslav Kysela <[e-mail]>
> >Cc: Yang, Libin <[e-mail]>; [e-mail]; pierre-
> >[e-mail]; [e-mail];
> >[e-mail]; [e-mail]
> >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 [e-mail] napsal(a):
> >> > From: Libin Yang <[e-mail]>
> >> >
> >> > 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.

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.


thanks,

Takashi

> 
> Regards,
> Libin
> 
> >
> >
> >thanks,
> >
> >Takashi
> >
> >>
> >> 						Jaroslav
> >>
> >> >
> >> > Signed-off-by: Libin Yang <[e-mail]>
> >> > ---
> >> >  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 <[e-mail]>
> >> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> >>
>

Jaroslav Kysela May 6, 2019, 9:03 a.m. UTC | #6

Dne 06. 05. 19 v 10:46 Yang, Libin napsal(a):
> Add Mengdong, Hui, Rander
> 
> Hi Jaroslav,
> 
>> -----Original Message-----
>> From: Jaroslav Kysela [mailto:[e-mail]]
>> Sent: Monday, May 6, 2019 4:20 PM
>> To: Yang, Libin <[e-mail]>; [e-mail]
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>> monitor in hw_params
>>
>> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
>>> From: Libin Yang <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>  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 <[e-mail]>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Yang, Libin May 6, 2019, 9:13 a.m. UTC | #7

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:[e-mail]]
>Sent: Monday, May 6, 2019 5:02 PM
>To: Yang, Libin <[e-mail]>
>Cc: Jaroslav Kysela <[e-mail]>; [e-mail]; pierre-
>[e-mail]; [e-mail]; Lin, Mengdong
><[e-mail]>; Wang, Rander <[e-mail]>;
>[e-mail]
>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:[e-mail]]
>> >Sent: Monday, May 6, 2019 4:40 PM
>> >To: Jaroslav Kysela <[e-mail]>
>> >Cc: Yang, Libin <[e-mail]>; [e-mail];
>> >pierre- [e-mail]; [e-mail];
>> >[e-mail]; [e-mail]
>> >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 [e-mail] napsal(a):
>> >> > From: Libin Yang <[e-mail]>
>> >> >
>> >> > 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 <[e-mail]>
>> >> > ---
>> >> >  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 <[e-mail]>
>> >> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>> >>
>>

Yang, Libin May 6, 2019, 9:25 a.m. UTC | #8

Hi Jaroslav,

>-----Original Message-----
>From: Jaroslav Kysela [mailto:[e-mail]]
>Sent: Monday, May 6, 2019 5:04 PM
>To: Yang, Libin <[e-mail]>; [e-mail]
>Cc: [e-mail]; [e-mail]; [e-mail];
>Hui Wang <[e-mail]>; Lin, Mengdong
><[e-mail]>; Wang, Rander <[e-mail]>
>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:[e-mail]]
>>> Sent: Monday, May 6, 2019 4:20 PM
>>> To: Yang, Libin <[e-mail]>; [e-mail]
>>> Cc: [e-mail]; [e-mail];
>>> [e-mail]; [e-mail];
>>> [e-mail]
>>> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no
>>> checking monitor in hw_params
>>>
>>> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
>>>> From: Libin Yang <[e-mail]>
>>>>
>>>> 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.

Hi Mengdong & Hui,

Do you have more comments for Jack control or UCM?

Regards,
Libin

>
>						Jaroslav
>
>>
>> Regards,
>> Libin
>>
>>>
>>> 						Jaroslav
>>>
>>>>
>>>> Signed-off-by: Libin Yang <[e-mail]>
>>>> ---
>>>>  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 <[e-mail]>
>>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>
>
>--
>Jaroslav Kysela <[e-mail]>
>Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Takashi Iwai May 6, 2019, 9:25 a.m. UTC | #9

On Mon, 06 May 2019 11:03:56 +0200,
Jaroslav Kysela wrote:
> 
> Dne 06. 05. 19 v 10:46 Yang, Libin napsal(a):
> > Add Mengdong, Hui, Rander
> > 
> > Hi Jaroslav,
> > 
> >> -----Original Message-----
> >> From: Jaroslav Kysela [mailto:[e-mail]]
> >> Sent: Monday, May 6, 2019 4:20 PM
> >> To: Yang, Libin <[e-mail]>; [e-mail]
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
> >> monitor in hw_params
> >>
> >> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
> >>> From: Libin Yang <[e-mail]>
> >>>
> >>> 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.

PA watches the Jack control change and switch the stream
appropriately.  So, the simplest "solution" would be just to remove
the monitor check in the PCM side, as Libin mentioned.


thanks,

Takashi

> 
> 						Jaroslav
> 
> > 
> > Regards,
> > Libin
> > 
> >>
> >> 						Jaroslav
> >>
> >>>
> >>> Signed-off-by: Libin Yang <[e-mail]>
> >>> ---
> >>>  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 <[e-mail]>
> >> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> 
> 
> -- 
> Jaroslav Kysela <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

Takashi Iwai May 6, 2019, 9:31 a.m. UTC | #10

On Mon, 06 May 2019 11:25:16 +0200,
Yang, Libin wrote:
> 
> Hi Jaroslav,
> 
> >-----Original Message-----
> >From: Jaroslav Kysela [mailto:[e-mail]]
> >Sent: Monday, May 6, 2019 5:04 PM
> >To: Yang, Libin <[e-mail]>; [e-mail]
> >Cc: [e-mail]; [e-mail]; [e-mail];
> >Hui Wang <[e-mail]>; Lin, Mengdong
> ><[e-mail]>; Wang, Rander <[e-mail]>
> >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:[e-mail]]
> >>> Sent: Monday, May 6, 2019 4:20 PM
> >>> To: Yang, Libin <[e-mail]>; [e-mail]
> >>> Cc: [e-mail]; [e-mail];
> >>> [e-mail]; [e-mail];
> >>> [e-mail]
> >>> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no
> >>> checking monitor in hw_params
> >>>
> >>> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
> >>>> From: Libin Yang <[e-mail]>
> >>>>
> >>>> 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 <[e-mail]>
> >>>> ---
> >>>>  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 <[e-mail]>
> >>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> >
> >
> >--
> >Jaroslav Kysela <[e-mail]>
> >Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Hui Wang May 6, 2019, 10:58 a.m. UTC | #11

On 2019/5/6 下午5:31, Takashi Iwai wrote:
> On Mon, 06 May 2019 11:25:16 +0200,
> Yang, Libin wrote:
>> Hi Jaroslav,
>>
>>> -----Original Message-----
>>> From: Jaroslav Kysela [mailto:[e-mail]]
>>> Sent: Monday, May 6, 2019 5:04 PM
>>> To: Yang, Libin <[e-mail]>; [e-mail]
>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>> Hui Wang <[e-mail]>; Lin, Mengdong
>>> <[e-mail]>; Wang, Rander <[e-mail]>
>>> 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:[e-mail]]
>>>>> Sent: Monday, May 6, 2019 4:20 PM
>>>>> To: Yang, Libin <[e-mail]>; [e-mail]
>>>>> Cc: [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail];
>>>>> [e-mail]
>>>>> Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no
>>>>> checking monitor in hw_params
>>>>>
>>>>> Dne 06. 05. 19 v 8:59 [e-mail] napsal(a):
>>>>>> From: Libin Yang <[e-mail]>
>>>>>>
>>>>>> 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?

Yes, probably it can work. set a HD-Audio Jack name in the 
SectionDevice, and monitor the ASoC jack name via JackControl. like below:

SectionDevice."Jack HDMI/DP,pcm=3" {
         Comment "HDMI/DP Audio #0"

         Value {
                 CaptureChannels "2"
                 CapturePCM "hw:${card_name},#{subdev}"
                 JackControl "${ASoC_HDMI_Jack_Name} Jack"
         }

...
Thanks,

Hui.

>
> thanks,
>
> Takashi
>
>> Regards,
>> Libin
>>
>>> 						Jaroslav
>>>
>>>> Regards,
>>>> Libin
>>>>
>>>>> 						Jaroslav
>>>>>
>>>>>> Signed-off-by: Libin Yang <[e-mail]>
>>>>>> ---
>>>>>>   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 <[e-mail]>
>>>>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>>>
>>> --
>>> Jaroslav Kysela <[e-mail]>
>>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Pierre-Louis Bossart May 6, 2019, 3:37 p.m. UTC | #12

On 5/6/19 1:59 AM, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
> 
> 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.

Out of curiosity, how is this handled in the legacy driver? I haven't 
done this for a long time but I remember very clearly being able to play 
on the HDMI:3,7, etc devices without any monitors connected. You'd get 
of course no sound but there was no error reported to userspace. The 
hardware is perfectly capable of pushing samples into the display 
controller using the HDAudio/iDisp link.

> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>   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,
>   };
>   
>

Takashi Iwai May 6, 2019, 3:41 p.m. UTC | #13

On Mon, 06 May 2019 17:37:32 +0200,
Pierre-Louis Bossart wrote:
> 
> On 5/6/19 1:59 AM, [e-mail] wrote:
> > From: Libin Yang <[e-mail]>
> >
> > 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.
> 
> Out of curiosity, how is this handled in the legacy driver? I haven't
> done this for a long time but I remember very clearly being able to
> play on the HDMI:3,7, etc devices without any monitors
> connected. You'd get of course no sound but there was no error
> reported to userspace. The hardware is perfectly capable of pushing
> samples into the display controller using the HDAudio/iDisp link.

As mentioned in the thread, PA just picks up the stream that is
connected via a monitor by checking / notified by the corresponding
Jack control.  On hdac_hdmi driver, the jack control has different
base name that is irrelevant with the output pins, so PA doesn't know
how to interpret it, hence it's ignored.


thanks,

Takashi

Pierre-Louis Bossart May 6, 2019, 3:47 p.m. UTC | #14

On 5/6/19 10:41 AM, Takashi Iwai wrote:
> On Mon, 06 May 2019 17:37:32 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 5/6/19 1:59 AM, [e-mail] wrote:
>>> From: Libin Yang <[e-mail]>
>>>
>>> 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.
>>
>> Out of curiosity, how is this handled in the legacy driver? I haven't
>> done this for a long time but I remember very clearly being able to
>> play on the HDMI:3,7, etc devices without any monitors
>> connected. You'd get of course no sound but there was no error
>> reported to userspace. The hardware is perfectly capable of pushing
>> samples into the display controller using the HDAudio/iDisp link.
> 
> As mentioned in the thread, PA just picks up the stream that is
> connected via a monitor by checking / notified by the corresponding
> Jack control.  On hdac_hdmi driver, the jack control has different
> base name that is irrelevant with the output pins, so PA doesn't know
> how to interpret it, hence it's ignored.

Yes, but do we have any error checks in the hw_params or trigger cases 
with the legacy driver?

Takashi Iwai May 6, 2019, 3:50 p.m. UTC | #15

On Mon, 06 May 2019 17:47:25 +0200,
Pierre-Louis Bossart wrote:
> 
> On 5/6/19 10:41 AM, Takashi Iwai wrote:
> > On Mon, 06 May 2019 17:37:32 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >> On 5/6/19 1:59 AM, [e-mail] wrote:
> >>> From: Libin Yang <[e-mail]>
> >>>
> >>> 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.
> >>
> >> Out of curiosity, how is this handled in the legacy driver? I haven't
> >> done this for a long time but I remember very clearly being able to
> >> play on the HDMI:3,7, etc devices without any monitors
> >> connected. You'd get of course no sound but there was no error
> >> reported to userspace. The hardware is perfectly capable of pushing
> >> samples into the display controller using the HDAudio/iDisp link.
> >
> > As mentioned in the thread, PA just picks up the stream that is
> > connected via a monitor by checking / notified by the corresponding
> > Jack control.  On hdac_hdmi driver, the jack control has different
> > base name that is irrelevant with the output pins, so PA doesn't know
> > how to interpret it, hence it's ignored.
> 
> Yes, but do we have any error checks in the hw_params or trigger cases
> with the legacy driver?

No, it just continues playing without the actual output.


Takashi

Pierre-Louis Bossart May 6, 2019, 3:54 p.m. UTC | #16

On 5/6/19 10:50 AM, Takashi Iwai wrote:
> On Mon, 06 May 2019 17:47:25 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 5/6/19 10:41 AM, Takashi Iwai wrote:
>>> On Mon, 06 May 2019 17:37:32 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>> On 5/6/19 1:59 AM, [e-mail] wrote:
>>>>> From: Libin Yang <[e-mail]>
>>>>>
>>>>> 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.
>>>>
>>>> Out of curiosity, how is this handled in the legacy driver? I haven't
>>>> done this for a long time but I remember very clearly being able to
>>>> play on the HDMI:3,7, etc devices without any monitors
>>>> connected. You'd get of course no sound but there was no error
>>>> reported to userspace. The hardware is perfectly capable of pushing
>>>> samples into the display controller using the HDAudio/iDisp link.
>>>
>>> As mentioned in the thread, PA just picks up the stream that is
>>> connected via a monitor by checking / notified by the corresponding
>>> Jack control.  On hdac_hdmi driver, the jack control has different
>>> base name that is irrelevant with the output pins, so PA doesn't know
>>> how to interpret it, hence it's ignored.
>>
>> Yes, but do we have any error checks in the hw_params or trigger cases
>> with the legacy driver?
> 
> No, it just continues playing without the actual output.

ok, so could we remove all these error checks then for hdac_hdmi? The 
problem is really the output selection on jack detect/monitor 
reconfigurations, those checks don't add much value, do they?

Yang, Libin May 7, 2019, 1:29 a.m. UTC | #17

Hi Pierre,

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:[e-mail]]
>Sent: Monday, May 6, 2019 11:54 PM
>To: Takashi Iwai <[e-mail]>
>Cc: Yang, Libin <[e-mail]>; [e-mail];
>[e-mail]; [e-mail];
>[e-mail]
>Subject: Re: [alsa-devel] [RFC PATCH] ASoC: codec: hdac_hdmi: no checking
>monitor in hw_params
>
>On 5/6/19 10:50 AM, Takashi Iwai wrote:
>> On Mon, 06 May 2019 17:47:25 +0200,
>> Pierre-Louis Bossart wrote:
>>>
>>> On 5/6/19 10:41 AM, Takashi Iwai wrote:
>>>> On Mon, 06 May 2019 17:37:32 +0200,
>>>> Pierre-Louis Bossart wrote:
>>>>>
>>>>> On 5/6/19 1:59 AM, [e-mail] wrote:
>>>>>> From: Libin Yang <[e-mail]>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Out of curiosity, how is this handled in the legacy driver? I
>>>>> haven't done this for a long time but I remember very clearly being
>>>>> able to play on the HDMI:3,7, etc devices without any monitors
>>>>> connected. You'd get of course no sound but there was no error
>>>>> reported to userspace. The hardware is perfectly capable of pushing
>>>>> samples into the display controller using the HDAudio/iDisp link.
>>>>
>>>> As mentioned in the thread, PA just picks up the stream that is
>>>> connected via a monitor by checking / notified by the corresponding
>>>> Jack control.  On hdac_hdmi driver, the jack control has different
>>>> base name that is irrelevant with the output pins, so PA doesn't
>>>> know how to interpret it, hence it's ignored.
>>>
>>> Yes, but do we have any error checks in the hw_params or trigger
>>> cases with the legacy driver?
>>
>> No, it just continues playing without the actual output.
>
>ok, so could we remove all these error checks then for hdac_hdmi? The
>problem is really the output selection on jack detect/monitor reconfigurations,
>those checks don't add much value, do they?

Yes, I'm planning to remove the checks in hdac_hdmi. Before that, I will 
do some tests on it.

The monitor check in hw_params only impacts on the playback flow.

Regards,
Libin

Patch

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,
 };