Re: [PATCH] drm/bridge: adv7511: fix support for large EDIDs

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Hans,

Thanks for your patch.

On 2021-03-24 09:53:32 +0100, Hans Verkuil wrote:
> While testing support for large (> 256 bytes) EDIDs on the Renesas
> Koelsch board I noticed that the adv7511 bridge driver only read the
> first two blocks.
> 
> The media V4L2 version for the adv7511 (drivers/media/i2c/adv7511-v4l2.c)
> handled this correctly.
> 
> Besides a simple bug when setting the segment register (it was set to the
> block number instead of block / 2), the logic of the code was also weird.
> In particular reading the DDC_STATUS is odd: this is unrelated to EDID
> reading.
> 
> The reworked code just waits for any EDID segment reads to finish (this
> does nothing if the a segment is already read), checks if the desired
> segment matches the read segment, and if not, then it requests the new
> segment and waits again for the EDID segment to be read.
> 
> Finally it checks if the currently buffered EDID segment contains the
> desired EDID block, and if not it will update the EDID buffer from
> the adv7511.
> 
> Tested with my Koelsch board and with EDIDs of 1, 2, 3 and 4 blocks.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> Testing on the Renesas board also requires these two adv7604 patches
> if you want to test with an HDMI cable between the HDMI input and output:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/00882808-472a-d429-c565-a701da579ead@xs4all.nl/
> https://patchwork.linuxtv.org/project/linux-media/patch/c7093e76-ffb4-b19c-f576-b264f935a3ce@xs4all.nl/
> ---
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 76555ae64e9c..9e8db1c60167 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -328,6 +328,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
>  static void __adv7511_power_on(struct adv7511 *adv7511)
>  {
>  	adv7511->current_edid_segment = -1;
> +	adv7511->edid_read = false;
> 
>  	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>  			   ADV7511_POWER_POWER_DOWN, 0);
> @@ -529,29 +530,35 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	struct adv7511 *adv7511 = data;
>  	struct i2c_msg xfer[2];
>  	uint8_t offset;
> +	unsigned int cur_segment;
>  	unsigned int i;
>  	int ret;
> 
>  	if (len > 128)
>  		return -EINVAL;
> 
> -	if (adv7511->current_edid_segment != block / 2) {
> -		unsigned int status;
> +	/* wait for any EDID segment reads to finish */
> +	adv7511_wait_for_edid(adv7511, 200);
> 
> -		ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
> -				  &status);
> +	ret = regmap_read(adv7511->regmap, ADV7511_REG_EDID_SEGMENT, &cur_segment);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * If the current read segment does not match what we need, then
> +	 * write the new segment and wait for it to be read.
> +	 */
> +	if (cur_segment != block / 2) {
> +		adv7511->edid_read = false;
> +		cur_segment = block / 2;
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
> +			     cur_segment);
> +		ret = adv7511_wait_for_edid(adv7511, 200);
>  		if (ret < 0)
>  			return ret;
> +	}
> 
> -		if (status != 2) {
> -			adv7511->edid_read = false;
> -			regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
> -				     block);
> -			ret = adv7511_wait_for_edid(adv7511, 200);
> -			if (ret < 0)
> -				return ret;
> -		}
> -
> +	if (adv7511->current_edid_segment != cur_segment) {
>  		/* Break this apart, hopefully more I2C controllers will
>  		 * support 64 byte transfers than 256 byte transfers
>  		 */
> @@ -579,7 +586,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  			offset += 64;
>  		}
> 
> -		adv7511->current_edid_segment = block / 2;
> +		adv7511->current_edid_segment = cur_segment;
>  	}
> 
>  	if (block % 2 == 0)

-- 
Regards,
Niklas Söderlund