[PATCH] drm: bridge: adv7511: fix reading edid segments

[PATCH] drm: bridge: adv7511: fix reading edid segments

[Emil Abildgaard Svendsen]

Currently reading EDID only works because usually only two EDID blocks
of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
blocks. And the first EDID segment read works fine but E-EDID specifies
up to 128 segments.

The logic is broken so change EDID segment index to multiple of 256
bytes and not 128 (block size).

Also change check of DDC status. Instead of silently not reading EDID
when in "IDLE" state [1]. Check if the DDC controller is in reset
instead (no HPD).

[1]
ADV7511 Programming Guide: Table 11: DDCController Status:

  0xC8 [3:0]  DDC Controller State

  0000        In Reset (No Hot Plug Detected)
  0001        Reading EDID
  0010        IDLE (Waiting for HDCP Requested)
  0011        Initializing HDCP
  0100        HDCP Enabled
  0101        Initializing HDCP Repeater

Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 24 ++++++++++++--------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8be235144f6d..c641c2ccd412 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -537,6 +537,8 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 				  size_t len)
 {
 	struct adv7511 *adv7511 = data;
+	struct device* dev = &adv7511->i2c_edid->dev;
+	int edid_segment = block / 2;
 	struct i2c_msg xfer[2];
 	uint8_t offset;
 	unsigned int i;
@@ -545,7 +547,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 	if (len > 128)
 		return -EINVAL;
 
-	if (adv7511->current_edid_segment != block / 2) {
+	if (adv7511->current_edid_segment != edid_segment) {
 		unsigned int status;
 
 		ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
@@ -553,15 +555,19 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
 		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 (!(status & 0x0F)) {
+			dev_dbg(dev, "DDC in reset no hot plug detected %x\n",
+				 status);
+			return -EIO;
 		}
 
+		adv7511->edid_read = false;
+		regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
+			     edid_segment);
+		ret = adv7511_wait_for_edid(adv7511, 200);
+		if (ret < 0)
+			return ret;
+
 		/* Break this apart, hopefully more I2C controllers will
 		 * support 64 byte transfers than 256 byte transfers
 		 */
@@ -589,7 +595,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 = edid_segment;
 	}
 
 	if (block % 2 == 0)
-- 
2.34.1

Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Fabio Estevam]

Hi Emil,

On Thu, Oct 26, 2023 at 11:47 AM Emil Abildgaard Svendsen
<EMAS@bang-olufsen.dk> wrote:
>
> Currently reading EDID only works because usually only two EDID blocks
> of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> blocks. And the first EDID segment read works fine but E-EDID specifies
> up to 128 segments.
>
> The logic is broken so change EDID segment index to multiple of 256
> bytes and not 128 (block size).
>
> Also change check of DDC status. Instead of silently not reading EDID
> when in "IDLE" state [1]. Check if the DDC controller is in reset
> instead (no HPD).
>
> [1]
> ADV7511 Programming Guide: Table 11: DDCController Status:
>
>   0xC8 [3:0]  DDC Controller State
>
>   0000        In Reset (No Hot Plug Detected)
>   0001        Reading EDID
>   0010        IDLE (Waiting for HDCP Requested)
>   0011        Initializing HDCP
>   0100        HDCP Enabled
>   0101        Initializing HDCP Repeater
>
> Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>

What about passing a Fixes tag?

Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Frieder Schrempf]

On 26.10.23 13:30, Emil Abildgaard Svendsen wrote:
> [Sie erhalten nicht häufig E-Mails von emas@bang-olufsen.dk. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> 
> Currently reading EDID only works because usually only two EDID blocks
> of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> blocks. And the first EDID segment read works fine but E-EDID specifies
> up to 128 segments.
> 
> The logic is broken so change EDID segment index to multiple of 256
> bytes and not 128 (block size).
> 
> Also change check of DDC status. Instead of silently not reading EDID
> when in "IDLE" state [1]. Check if the DDC controller is in reset
> instead (no HPD).
> 
> [1]
> ADV7511 Programming Guide: Table 11: DDCController Status:
> 
>   0xC8 [3:0]  DDC Controller State
> 
>   0000        In Reset (No Hot Plug Detected)
>   0001        Reading EDID
>   0010        IDLE (Waiting for HDCP Requested)
>   0011        Initializing HDCP
>   0100        HDCP Enabled
>   0101        Initializing HDCP Repeater
> 
> Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>

This patch somehow seems to break the reported display modes with my
setup (i.MX8MM DSI -> ADV7535 -> FullHD screen) when I test on current
linux-next.

Without this patch I get 30 valid modes reported by modetest.

With this patch applied I only get 5 valid modes. The screen still comes
up with the 1080p default mode though, that is now not even in the list
anymore.

Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Emil Abildgaard Svendsen]

On Thu, Oct 26, 2023 at 09:11:53PM +0200, Frieder Schrempf wrote:
> [You don't often get email from frieder.schrempf@kontron.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 26.10.23 13:30, Emil Abildgaard Svendsen wrote:
> > [Sie erhalten nicht häufig E-Mails von emas@bang-olufsen.dk. Weitere Informationen, warum dies wichtig ist, finden Sie unter https://aka.ms/LearnAboutSenderIdentification ]
> >
> > Currently reading EDID only works because usually only two EDID blocks
> > of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> > blocks. And the first EDID segment read works fine but E-EDID specifies
> > up to 128 segments.
> >
> > The logic is broken so change EDID segment index to multiple of 256
> > bytes and not 128 (block size).
> >
> > Also change check of DDC status. Instead of silently not reading EDID
> > when in "IDLE" state [1]. Check if the DDC controller is in reset
> > instead (no HPD).
> >
> > [1]
> > ADV7511 Programming Guide: Table 11: DDCController Status:
> >
> >   0xC8 [3:0]  DDC Controller State
> >
> >   0000        In Reset (No Hot Plug Detected)
> >   0001        Reading EDID
> >   0010        IDLE (Waiting for HDCP Requested)
> >   0011        Initializing HDCP
> >   0100        HDCP Enabled
> >   0101        Initializing HDCP Repeater
> >
> > Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>
> 
> This patch somehow seems to break the reported display modes with my
> setup (i.MX8MM DSI -> ADV7535 -> FullHD screen) when I test on current
> linux-next.
> 
> Without this patch I get 30 valid modes reported by modetest.
> 
> With this patch applied I only get 5 valid modes. The screen still comes
> up with the 1080p default mode though, that is now not even in the list
> anymore.

Okay, I believe it's because I return when DDC controller is in reset. I
believe you hit this because HPD override is enabled for ADV7535 which
in my experiments isn't needed. But I will see if I can address that in
another commit.

For this commit I will simply remove the return. Reading EDID clearly
still works when DDC is in reset. Maybe because of the 200 ms wait?

Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Emil Abildgaard Svendsen]

On Thu, Oct 26, 2023 at 11:49:00AM -0300, Fabio Estevam wrote:
> Hi Emil,
> 
> On Thu, Oct 26, 2023 at 11:47 AM Emil Abildgaard Svendsen
> <EMAS@bang-olufsen.dk> wrote:
> >
> > Currently reading EDID only works because usually only two EDID blocks
> > of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> > blocks. And the first EDID segment read works fine but E-EDID specifies
> > up to 128 segments.
> >
> > The logic is broken so change EDID segment index to multiple of 256
> > bytes and not 128 (block size).
> >
> > Also change check of DDC status. Instead of silently not reading EDID
> > when in "IDLE" state [1]. Check if the DDC controller is in reset
> > instead (no HPD).
> >
> > [1]
> > ADV7511 Programming Guide: Table 11: DDCController Status:
> >
> >   0xC8 [3:0]  DDC Controller State
> >
> >   0000        In Reset (No Hot Plug Detected)
> >   0001        Reading EDID
> >   0010        IDLE (Waiting for HDCP Requested)
> >   0011        Initializing HDCP
> >   0100        HDCP Enabled
> >   0101        Initializing HDCP Repeater
> >
> > Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>
> 
> What about passing a Fixes tag?

Thank you! I simply forgot. I will add it.

Re: [PATCH] drm: bridge: adv7511: fix reading edid segments

[Jani Nikula]

On Thu, 26 Oct 2023, Emil Abildgaard Svendsen <EMAS@bang-olufsen.dk> wrote:
> Currently reading EDID only works because usually only two EDID blocks
> of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> blocks. And the first EDID segment read works fine but E-EDID specifies
> up to 128 segments.
>
> The logic is broken so change EDID segment index to multiple of 256
> bytes and not 128 (block size).
>
> Also change check of DDC status. Instead of silently not reading EDID
> when in "IDLE" state [1]. Check if the DDC controller is in reset
> instead (no HPD).

"Also" in a commit message is often a good indication that the patch
should be split to do the "also" in a separate patch. One "thing" per
patch. Here, it'll be useful for debugging [1], too, to figure out which
part broken things. (I suspect it's the status handling.)


BR,
Jani.


[1] https://lore.kernel.org/r/5aa375f1-07cd-4e28-8cd5-7e10b4b05c6a@kontron.de


>
> [1]
> ADV7511 Programming Guide: Table 11: DDCController Status:
>
>   0xC8 [3:0]  DDC Controller State
>
>   0000        In Reset (No Hot Plug Detected)
>   0001        Reading EDID
>   0010        IDLE (Waiting for HDCP Requested)
>   0011        Initializing HDCP
>   0100        HDCP Enabled
>   0101        Initializing HDCP Repeater
>
> Signed-off-by: Emil Svendsen <emas@bang-olufsen.dk>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 24 ++++++++++++--------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8be235144f6d..c641c2ccd412 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -537,6 +537,8 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  				  size_t len)
>  {
>  	struct adv7511 *adv7511 = data;
> +	struct device* dev = &adv7511->i2c_edid->dev;
> +	int edid_segment = block / 2;
>  	struct i2c_msg xfer[2];
>  	uint8_t offset;
>  	unsigned int i;
> @@ -545,7 +547,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	if (len > 128)
>  		return -EINVAL;
>  
> -	if (adv7511->current_edid_segment != block / 2) {
> +	if (adv7511->current_edid_segment != edid_segment) {
>  		unsigned int status;
>  
>  		ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
> @@ -553,15 +555,19 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
>  		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 (!(status & 0x0F)) {
> +			dev_dbg(dev, "DDC in reset no hot plug detected %x\n",
> +				 status);
> +			return -EIO;
>  		}
>  
> +		adv7511->edid_read = false;
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
> +			     edid_segment);
> +		ret = adv7511_wait_for_edid(adv7511, 200);
> +		if (ret < 0)
> +			return ret;
> +
>  		/* Break this apart, hopefully more I2C controllers will
>  		 * support 64 byte transfers than 256 byte transfers
>  		 */
> @@ -589,7 +595,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 = edid_segment;
>  	}
>  
>  	if (block % 2 == 0)

-- 
Jani Nikula, Intel