Re: [PATCH 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support

From: Jerome Brunet <jbrunet@baylibre.com>
To: Jonas Karlman <jonas@kwiboo.se>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>
Cc: Kevin Hilman <khilman@baylibre.com>,
	"linux-amlogic\@lists.infradead.org" 
	<linux-amlogic@lists.infradead.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel\@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support
Date: Wed, 07 Aug 2019 18:10:45 +0200	[thread overview]
Message-ID: <1jy305negq.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <HE1PR06MB40117A899E057B36BE461B8EACD40@HE1PR06MB4011.eurprd06.prod.outlook.com>

On Wed 07 Aug 2019 at 14:57, Jonas Karlman <jonas@kwiboo.se> wrote:

> On 2019-08-05 15:41, Jerome Brunet wrote:
>> Provide the eld to the generic hdmi-codec driver.
>> This will let the driver enforce the maximum channel number and set the
>> channel allocation depending on the hdmi sink.
>>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h     |  1 +
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++++++++++
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c           |  1 +
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> index 63b5756f463b..cb07dc0da5a7 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> @@ -14,6 +14,7 @@ struct dw_hdmi_audio_data {
>>  
>>  struct dw_hdmi_i2s_audio_data {
>>  	struct dw_hdmi *hdmi;
>> +	u8 *eld;
>>  
>>  	void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
>>  	u8 (*read)(struct dw_hdmi *hdmi, int offset);
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> index b8ece9c1ba2c..14d499b344c0 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> @@ -121,6 +121,15 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev, void *data)
>>  	dw_hdmi_audio_disable(hdmi);
>>  }
>>  
>> +static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
>> +			       size_t len)
>> +{
>> +	struct dw_hdmi_i2s_audio_data *audio = data;
>> +
>> +	memcpy(buf, audio->eld, min(sizeof(audio->eld), len));
>
> Above sizeof does not work as intended, sizeof(audio->eld) is probably the size of a pointer and not MAX_ELD_BYTES (128),
> resulting in only part of the ELD gets copied to buf.

Silly ... thanks for pointing this out. I'll respin

>
>
> Thanks for reworking dw-hdmi multi channel lpcm support!, this works on Rockchip for most parts.
> Without the [1] patch (reset cts+n after clock is enabled) audio sometime stay silent when switching between e.g. 2ch 44.1khz and 6ch 48khz tracks.
> It is not fully clear to me why reset cts+n helps, if that have
> negative affects on other platforms or if there is another way to
> solve loosing audio.

I did not see that issue my self but I guess could propose this change ?

>
> I also have a small issue with the channel allocation being selected by hdmi-codec, my soundbar reports LPCM 6.1ch instead of LPCM 7.1ch when I play a 7.1ch speaker test clip.
> I have a hdmi-codec patch to fix selection of channel allocation in
> queue.

Yes, I know there is still a few problems around that and stale eld.
But those problem are not really coming from the i2s interface of the
dw-hdmi itself and should be dealt with separatly.

This is just the beginning ;)

>
>
> For patch 1-7:
>
> Reviewed-by: Jonas Karlman <jonas@kwiboo.se>
>
>
> [1] https://github.com/Kwiboo/linux-rockchip/commit/c0839e874f843ad173ddde958303d6878394ef92
>
> Regards,
> Jonas
>
>> +	return 0;
>> +}
>> +
>>  static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
>>  				  struct device_node *endpoint)
>>  {
>> @@ -144,6 +153,7 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
>>  static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
>>  	.hw_params	= dw_hdmi_i2s_hw_params,
>>  	.audio_shutdown	= dw_hdmi_i2s_audio_shutdown,
>> +	.get_eld	= dw_hdmi_i2s_get_eld,
>>  	.get_dai_id	= dw_hdmi_i2s_get_dai_id,
>>  };
>>  
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index bed4bb017afd..8df69c9dbfad 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2797,6 +2797,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
>>  		struct dw_hdmi_i2s_audio_data audio;
>>  
>>  		audio.hdmi	= hdmi;
>> +		audio.eld	= hdmi->connector.eld;
>>  		audio.write	= hdmi_writeb;
>>  		audio.read	= hdmi_readb;
>>  		hdmi->enable_audio = dw_hdmi_i2s_audio_enable;