[PATCH v2] ASoC: hdmi-codec: Add option for ELD bypass

[PATCH v2] ASoC: hdmi-codec: Add option for ELD bypass

[Sugar Zhang]

This patch allow users to enable "ELD Bypass" who don't
care anything from EDID Link Data.

Currently, this driver gets ELD(from EDID) to constraint
channels and rates.

Unfortunately, EDID is not always valid, maybe caused by
the fragile HDMI port or cable, in this situation, the max
features are limited to 48kHz stereo.

So, add this option to allow user to select the manual way
to output audio as expected. such as multi-channels LPCM(7.1),
or HBR bitstream for these sink devices.

Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
---

Changes in v2:
- Use MACRO SOC_SINGLE_BOOL_EXT to simplify code.
  Fix event_missing checked by mixer-test.
  Add suffix "Switch" for "ELD Bypass".

 sound/soc/codecs/hdmi-codec.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index b07607a..be46fbd 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -275,6 +275,7 @@ struct hdmi_codec_priv {
 	unsigned int chmap_idx;
 	struct mutex lock;
 	bool busy;
+	bool eld_bypass;
 	struct snd_soc_jack *jack;
 	unsigned int jack_status;
 	u8 iec_status[AES_IEC958_STATUS_SIZE];
@@ -427,6 +428,31 @@ static int hdmi_codec_iec958_mask_get(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+static int hdmi_codec_eld_bypass_get(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.integer.value[0] = hcp->eld_bypass;
+
+	return 0;
+}
+
+static int hdmi_codec_eld_bypass_put(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
+
+	if (hcp->eld_bypass == ucontrol->value.integer.value[0])
+		return 0;
+
+	hcp->eld_bypass = ucontrol->value.integer.value[0];
+
+	return 1;
+}
+
 static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *dai)
 {
@@ -447,7 +473,7 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 			goto err;
 	}
 
-	if (tx && hcp->hcd.ops->get_eld) {
+	if (tx && !hcp->eld_bypass && hcp->hcd.ops->get_eld) {
 		ret = hcp->hcd.ops->get_eld(dai->dev->parent, hcp->hcd.data,
 					    hcp->eld, sizeof(hcp->eld));
 		if (ret)
@@ -770,6 +796,8 @@ static struct snd_kcontrol_new hdmi_codec_controls[] = {
 		.info	= hdmi_eld_ctl_info,
 		.get	= hdmi_eld_ctl_get,
 	},
+	SOC_SINGLE_BOOL_EXT("ELD Bypass Switch", 0,
+			    hdmi_codec_eld_bypass_get, hdmi_codec_eld_bypass_put),
 };
 
 static int hdmi_codec_pcm_new(struct snd_soc_pcm_runtime *rtd,
@@ -854,7 +882,7 @@ static void plugged_cb(struct device *dev, bool plugged)
 	struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
 
 	if (plugged) {
-		if (hcp->hcd.ops->get_eld) {
+		if (!hcp->eld_bypass && hcp->hcd.ops->get_eld) {
 			hcp->hcd.ops->get_eld(dev->parent, hcp->hcd.data,
 					    hcp->eld, sizeof(hcp->eld));
 		}
-- 
2.7.4

Re: [PATCH v2] ASoC: hdmi-codec: Add option for ELD bypass

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2821 bytes --]

Hi,

On Sat, Apr 30, 2022 at 01:41:18PM +0800, Sugar Zhang wrote:
> This patch allow users to enable "ELD Bypass" who don't
> care anything from EDID Link Data.
> 
> Currently, this driver gets ELD(from EDID) to constraint
> channels and rates.
> 
> Unfortunately, EDID is not always valid, maybe caused by
> the fragile HDMI port or cable, in this situation, the max
> features are limited to 48kHz stereo.
> 
> So, add this option to allow user to select the manual way
> to output audio as expected. such as multi-channels LPCM(7.1),
> or HBR bitstream for these sink devices.
> 
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>

I think some more documentation is needed there to describe how it's
going to be used.

Like, you mention that it's relevant when the EDID is not valid. But if
the EDID is valid, is bypass still allowed or not?

> ---
> 
> Changes in v2:
> - Use MACRO SOC_SINGLE_BOOL_EXT to simplify code.
>   Fix event_missing checked by mixer-test.
>   Add suffix "Switch" for "ELD Bypass".
> 
>  sound/soc/codecs/hdmi-codec.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
> index b07607a..be46fbd 100644
> --- a/sound/soc/codecs/hdmi-codec.c
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -275,6 +275,7 @@ struct hdmi_codec_priv {
>  	unsigned int chmap_idx;
>  	struct mutex lock;
>  	bool busy;
> +	bool eld_bypass;
>  	struct snd_soc_jack *jack;
>  	unsigned int jack_status;
>  	u8 iec_status[AES_IEC958_STATUS_SIZE];
> @@ -427,6 +428,31 @@ static int hdmi_codec_iec958_mask_get(struct snd_kcontrol *kcontrol,
>  	return 0;
>  }
>  
> +static int hdmi_codec_eld_bypass_get(struct snd_kcontrol *kcontrol,
> +				     struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> +
> +	ucontrol->value.integer.value[0] = hcp->eld_bypass;
> +
> +	return 0;
> +}
> +
> +static int hdmi_codec_eld_bypass_put(struct snd_kcontrol *kcontrol,
> +				     struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> +
> +	if (hcp->eld_bypass == ucontrol->value.integer.value[0])
> +		return 0;
> +
> +	hcp->eld_bypass = ucontrol->value.integer.value[0];
> +
> +	return 1;
> +}

If the ELD bypass is set, how does it affect the hdmi_codec_params being
passed to the codec?

Also, what is being returned to the userspace by hdmi_eld_ctl_get once
the bypass is enabled?

And shouldn't we call get_eld when we remove the bypass?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] ASoC: hdmi-codec: Add option for ELD bypass

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On Tue, May 03, 2022 at 10:38:52AM +0200, Maxime Ripard wrote:

> I think some more documentation is needed there to describe how it's
> going to be used.
> 
> Like, you mention that it's relevant when the EDID is not valid. But if
> the EDID is valid, is bypass still allowed or not?

I'd expect so given that it's explicitly an override and that it's not
like it's unknown for people to put nonsense in ID information.

> > +static int hdmi_codec_eld_bypass_put(struct snd_kcontrol *kcontrol,
> > +				     struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > +	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> > +
> > +	if (hcp->eld_bypass == ucontrol->value.integer.value[0])
> > +		return 0;
> > +
> > +	hcp->eld_bypass = ucontrol->value.integer.value[0];
> > +
> > +	return 1;
> > +}

> If the ELD bypass is set, how does it affect the hdmi_codec_params being
> passed to the codec?

Presumably we should tell the CODEC what we're trying to play (looks
like that's what the current code does)?

> Also, what is being returned to the userspace by hdmi_eld_ctl_get once
> the bypass is enabled?

My first thought would be that we'd always read whatever is there
rather than trying to make something up, bypass just says we're not
enforcing it.

> And shouldn't we call get_eld when we remove the bypass?

Or given what I just said above should we not change any get_eld() calls
but instead only change things so that we don't look at the ELD data
when setting constraints during startup() and during channel map
operations?  In that case we wouldn't need to read again.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] ASoC: hdmi-codec: Add option for ELD bypass

[sugar zhang]

Hi Mark, Maxime,

在 2022/5/4 4:44, Mark Brown 写道:
> On Tue, May 03, 2022 at 10:38:52AM +0200, Maxime Ripard wrote:
>
>> I think some more documentation is needed there to describe how it's
>> going to be used.
>>
>> Like, you mention that it's relevant when the EDID is not valid. But if
>> the EDID is valid, is bypass still allowed or not?
> I'd expect so given that it's explicitly an override and that it's not
> like it's unknown for people to put nonsense in ID information.
>
>>> +static int hdmi_codec_eld_bypass_put(struct snd_kcontrol *kcontrol,
>>> +				     struct snd_ctl_elem_value *ucontrol)
>>> +{
>>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>>> +	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
>>> +
>>> +	if (hcp->eld_bypass == ucontrol->value.integer.value[0])
>>> +		return 0;
>>> +
>>> +	hcp->eld_bypass = ucontrol->value.integer.value[0];
>>> +
>>> +	return 1;
>>> +}
>> If the ELD bypass is set, how does it affect the hdmi_codec_params being
>> passed to the codec?
> Presumably we should tell the CODEC what we're trying to play (looks
> like that's what the current code does)?
>
>> Also, what is being returned to the userspace by hdmi_eld_ctl_get once
>> the bypass is enabled?
> My first thought would be that we'd always read whatever is there
> rather than trying to make something up, bypass just says we're not
> enforcing it.
>
>> And shouldn't we call get_eld when we remove the bypass?
> Or given what I just said above should we not change any get_eld() calls
> but instead only change things so that we don't look at the ELD data
> when setting constraints during startup() and during channel map
> operations?  In that case we wouldn't need to read again.

I agree this idea that just don't use it for constraints and channel map 
or something else.

but still do get_eld() as it was. will do in v3.


-- 
Best Regards!
张学广/Sugar
瑞芯微电子股份有限公司
Rockchip Electronics Co., Ltd.