[RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Jani Nikula]

The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
look up the discussion, but didn't find anyone questioning the EDID
reading part.

Why does it not use drm_get_edid() or drm_do_get_edid()?

I don't know where client->addr comes from, so I guess it could be
different from DDC_ADDR, rendering drm_get_edid() unusable.

There's also the comment:

	/* Yes, read the entire buffer, and do not skip the first
	 * EDID_LENGTH bytes.
	 */

But again, there's not a word on *why*.

Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
away from their own EDID parsing and validity checks, including stop
using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
struct drm_edid, and friends, but this is the first step.)

Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Ian Ray <ian.ray@ge.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Yuan Can <yuancan@huawei.com>
Cc: Zheyu Ma <zheyuma97@gmail.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I haven't even tried to compile this, and I have no way to test
this. Apologies for the long Cc list; I'm hoping someone could explain
the existing code, and perhaps give this approach a spin.
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
 1 file changed, 9 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 460db3c8a08c..0d9eacf3d9b7 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
 
 static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
 
-static u8 *stdp2690_get_edid(struct i2c_client *client)
+static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
 {
+	struct i2c_client *client = context;
 	struct i2c_adapter *adapter = client->adapter;
-	unsigned char start = 0x00;
-	unsigned int total_size;
-	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	unsigned char start = block * EDID_LENGTH;
 
 	struct i2c_msg msgs[] = {
 		{
@@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
 		}, {
 			.addr	= client->addr,
 			.flags	= I2C_M_RD,
-			.len	= EDID_LENGTH,
-			.buf	= block,
+			.len	= len,
+			.buf	= buf,
 		}
 	};
 
-	if (!block)
-		return NULL;
+	if (i2c_transfer(adapter, msgs, 2) != 2)
+		return -1;
 
-	if (i2c_transfer(adapter, msgs, 2) != 2) {
-		DRM_ERROR("Unable to read EDID.\n");
-		goto err;
-	}
-
-	if (!drm_edid_block_valid(block, 0, false, NULL)) {
-		DRM_ERROR("Invalid EDID data\n");
-		goto err;
-	}
-
-	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
-	if (total_size > EDID_LENGTH) {
-		kfree(block);
-		block = kmalloc(total_size, GFP_KERNEL);
-		if (!block)
-			return NULL;
-
-		/* Yes, read the entire buffer, and do not skip the first
-		 * EDID_LENGTH bytes.
-		 */
-		start = 0x00;
-		msgs[1].len = total_size;
-		msgs[1].buf = block;
-
-		if (i2c_transfer(adapter, msgs, 2) != 2) {
-			DRM_ERROR("Unable to read EDID extension blocks.\n");
-			goto err;
-		}
-		if (!drm_edid_block_valid(block, 1, false, NULL)) {
-			DRM_ERROR("Invalid EDID data\n");
-			goto err;
-		}
-	}
-
-	return block;
-
-err:
-	kfree(block);
-	return NULL;
+	return 0;
 }
 
 static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
@@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
 
 	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
 
-	return (struct edid *)stdp2690_get_edid(client);
+	return drm_do_get_edid(connector, stdp2690_read_block, client, NULL);
 }
 
 static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
-- 
2.39.2

Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Jani Nikula]

On Fri, 01 Sep 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> look up the discussion, but didn't find anyone questioning the EDID
> reading part.
>
> Why does it not use drm_get_edid() or drm_do_get_edid()?
>
> I don't know where client->addr comes from, so I guess it could be
> different from DDC_ADDR, rendering drm_get_edid() unusable.
>
> There's also the comment:
>
> 	/* Yes, read the entire buffer, and do not skip the first
> 	 * EDID_LENGTH bytes.
> 	 */
>
> But again, there's not a word on *why*.
>
> Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
> away from their own EDID parsing and validity checks, including stop
> using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
> struct drm_edid, and friends, but this is the first step.)
>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Ian Ray <ian.ray@ge.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Martin Donnelly <martin.donnelly@ge.com>
> Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Yuan Can <yuancan@huawei.com>
> Cc: Zheyu Ma <zheyuma97@gmail.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> ---
>
> I haven't even tried to compile this, and I have no way to test
> this. Apologies for the long Cc list; I'm hoping someone could explain
> the existing code, and perhaps give this approach a spin.
> ---
>  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
>  1 file changed, 9 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> index 460db3c8a08c..0d9eacf3d9b7 100644
> --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> @@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
>  
>  static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
>  
> -static u8 *stdp2690_get_edid(struct i2c_client *client)
> +static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
>  {
> +	struct i2c_client *client = context;
>  	struct i2c_adapter *adapter = client->adapter;
> -	unsigned char start = 0x00;
> -	unsigned int total_size;
> -	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> +	unsigned char start = block * EDID_LENGTH;
>  
>  	struct i2c_msg msgs[] = {
>  		{
> @@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
>  		}, {
>  			.addr	= client->addr,
>  			.flags	= I2C_M_RD,
> -			.len	= EDID_LENGTH,
> -			.buf	= block,
> +			.len	= len,
> +			.buf	= buf,
>  		}
>  	};
>  
> -	if (!block)
> -		return NULL;
> +	if (i2c_transfer(adapter, msgs, 2) != 2)
> +		return -1;
>  
> -	if (i2c_transfer(adapter, msgs, 2) != 2) {
> -		DRM_ERROR("Unable to read EDID.\n");
> -		goto err;
> -	}
> -
> -	if (!drm_edid_block_valid(block, 0, false, NULL)) {
> -		DRM_ERROR("Invalid EDID data\n");
> -		goto err;
> -	}
> -
> -	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> -	if (total_size > EDID_LENGTH) {
> -		kfree(block);
> -		block = kmalloc(total_size, GFP_KERNEL);
> -		if (!block)
> -			return NULL;
> -
> -		/* Yes, read the entire buffer, and do not skip the first
> -		 * EDID_LENGTH bytes.
> -		 */
> -		start = 0x00;
> -		msgs[1].len = total_size;
> -		msgs[1].buf = block;
> -
> -		if (i2c_transfer(adapter, msgs, 2) != 2) {
> -			DRM_ERROR("Unable to read EDID extension blocks.\n");
> -			goto err;
> -		}
> -		if (!drm_edid_block_valid(block, 1, false, NULL)) {
> -			DRM_ERROR("Invalid EDID data\n");
> -			goto err;
> -		}
> -	}
> -
> -	return block;
> -
> -err:
> -	kfree(block);
> -	return NULL;
> +	return 0;
>  }
>  
>  static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
> @@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
>  
>  	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
>  
> -	return (struct edid *)stdp2690_get_edid(client);
> +	return drm_do_get_edid(connector, stdp2690_read_block, client, NULL);

The last NULL param should be dropped, as noted by the build bot.

BR,
Jani.


>  }
>  
>  static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: EXT: Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Ian Ray]

On Fri, Sep 01, 2023 at 05:52:02PM +0300, Jani Nikula wrote:
> 
> On Fri, 01 Sep 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> > The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> > Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> > look up the discussion, but didn't find anyone questioning the EDID
> > reading part.
> >
> > Why does it not use drm_get_edid() or drm_do_get_edid()?
> >
> > I don't know where client->addr comes from, so I guess it could be
> > different from DDC_ADDR, rendering drm_get_edid() unusable.
> >
> > There's also the comment:
> >
> > 	/* Yes, read the entire buffer, and do not skip the first
> > 	 * EDID_LENGTH bytes.
> > 	 */
> >
> > But again, there's not a word on *why*.
> >
> > Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
> > away from their own EDID parsing and validity checks, including stop
> > using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
> > struct drm_edid, and friends, but this is the first step.)
> >
> > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > Cc: Ian Ray <ian.ray@ge.com>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Martin Donnelly <martin.donnelly@ge.com>
> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
> > Cc: Neil Armstrong <neil.armstrong@linaro.org>
> > Cc: Peter Senna Tschudin <peter.senna@gmail.com>
> > Cc: Robert Foss <rfoss@kernel.org>
> > Cc: Yuan Can <yuancan@huawei.com>
> > Cc: Zheyu Ma <zheyuma97@gmail.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >
> > ---
> >
> > I haven't even tried to compile this, and I have no way to test
> > this. Apologies for the long Cc list; I'm hoping someone could explain
> > the existing code, and perhaps give this approach a spin.
> > ---
> >  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
> >  1 file changed, 9 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > index 460db3c8a08c..0d9eacf3d9b7 100644
> > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
> > @@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
> >  
> >  static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
> >  
> > -static u8 *stdp2690_get_edid(struct i2c_client *client)
> > +static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
> >  {
> > +	struct i2c_client *client = context;
> >  	struct i2c_adapter *adapter = client->adapter;
> > -	unsigned char start = 0x00;
> > -	unsigned int total_size;
> > -	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> > +	unsigned char start = block * EDID_LENGTH;
> >  
> >  	struct i2c_msg msgs[] = {
> >  		{
> > @@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
> >  		}, {
> >  			.addr	= client->addr,
> >  			.flags	= I2C_M_RD,
> > -			.len	= EDID_LENGTH,
> > -			.buf	= block,
> > +			.len	= len,
> > +			.buf	= buf,
> >  		}
> >  	};
> >  
> > -	if (!block)
> > -		return NULL;
> > +	if (i2c_transfer(adapter, msgs, 2) != 2)
> > +		return -1;
> >  
> > -	if (i2c_transfer(adapter, msgs, 2) != 2) {
> > -		DRM_ERROR("Unable to read EDID.\n");
> > -		goto err;
> > -	}
> > -
> > -	if (!drm_edid_block_valid(block, 0, false, NULL)) {
> > -		DRM_ERROR("Invalid EDID data\n");
> > -		goto err;
> > -	}
> > -
> > -	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > -	if (total_size > EDID_LENGTH) {
> > -		kfree(block);
> > -		block = kmalloc(total_size, GFP_KERNEL);
> > -		if (!block)
> > -			return NULL;
> > -
> > -		/* Yes, read the entire buffer, and do not skip the first
> > -		 * EDID_LENGTH bytes.
> > -		 */
> > -		start = 0x00;
> > -		msgs[1].len = total_size;
> > -		msgs[1].buf = block;
> > -
> > -		if (i2c_transfer(adapter, msgs, 2) != 2) {
> > -			DRM_ERROR("Unable to read EDID extension blocks.\n");
> > -			goto err;
> > -		}
> > -		if (!drm_edid_block_valid(block, 1, false, NULL)) {
> > -			DRM_ERROR("Invalid EDID data\n");
> > -			goto err;
> > -		}
> > -	}
> > -
> > -	return block;
> > -
> > -err:
> > -	kfree(block);
> > -	return NULL;
> > +	return 0;
> >  }
> >  
> >  static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
> > @@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
> >  
> >  	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
> >  
> > -	return (struct edid *)stdp2690_get_edid(client);
> > +	return drm_do_get_edid(connector, stdp2690_read_block, client, NULL);
> 
> The last NULL param should be dropped, as noted by the build bot.

Tested with various displays, including displays that have one block of
EDID (DELL P2417H) and a second EDID extension block (VES/55UHD_LCD_TV).

Tested-by: Ian Ray <ian.ray@ge.com>

Would you like me to submit the patch Jani?


> 
> BR,
> Jani.
> 
> 
> >  }
> >  
> >  static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> 

Re: EXT: Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Jani Nikula]

On Fri, 08 Sep 2023, Ian Ray <ian.ray@ge.com> wrote:
> On Fri, Sep 01, 2023 at 05:52:02PM +0300, Jani Nikula wrote:
>> 
>> On Fri, 01 Sep 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> > The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
>> > Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
>> > look up the discussion, but didn't find anyone questioning the EDID
>> > reading part.
>> >
>> > Why does it not use drm_get_edid() or drm_do_get_edid()?
>> >
>> > I don't know where client->addr comes from, so I guess it could be
>> > different from DDC_ADDR, rendering drm_get_edid() unusable.
>> >
>> > There's also the comment:
>> >
>> > 	/* Yes, read the entire buffer, and do not skip the first
>> > 	 * EDID_LENGTH bytes.
>> > 	 */
>> >
>> > But again, there's not a word on *why*.
>> >
>> > Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
>> > away from their own EDID parsing and validity checks, including stop
>> > using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
>> > struct drm_edid, and friends, but this is the first step.)
>> >
>> > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> > Cc: Ian Ray <ian.ray@ge.com>
>> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> > Cc: Jonas Karlman <jonas@kwiboo.se>
>> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> > Cc: Martin Donnelly <martin.donnelly@ge.com>
>> > Cc: Martyn Welch <martyn.welch@collabora.co.uk>
>> > Cc: Neil Armstrong <neil.armstrong@linaro.org>
>> > Cc: Peter Senna Tschudin <peter.senna@gmail.com>
>> > Cc: Robert Foss <rfoss@kernel.org>
>> > Cc: Yuan Can <yuancan@huawei.com>
>> > Cc: Zheyu Ma <zheyuma97@gmail.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >
>> > ---
>> >
>> > I haven't even tried to compile this, and I have no way to test
>> > this. Apologies for the long Cc list; I'm hoping someone could explain
>> > the existing code, and perhaps give this approach a spin.
>> > ---
>> >  .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
>> >  1 file changed, 9 insertions(+), 48 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
>> > index 460db3c8a08c..0d9eacf3d9b7 100644
>> > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
>> > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
>> > @@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
>> >  
>> >  static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
>> >  
>> > -static u8 *stdp2690_get_edid(struct i2c_client *client)
>> > +static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
>> >  {
>> > +	struct i2c_client *client = context;
>> >  	struct i2c_adapter *adapter = client->adapter;
>> > -	unsigned char start = 0x00;
>> > -	unsigned int total_size;
>> > -	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
>> > +	unsigned char start = block * EDID_LENGTH;
>> >  
>> >  	struct i2c_msg msgs[] = {
>> >  		{
>> > @@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
>> >  		}, {
>> >  			.addr	= client->addr,
>> >  			.flags	= I2C_M_RD,
>> > -			.len	= EDID_LENGTH,
>> > -			.buf	= block,
>> > +			.len	= len,
>> > +			.buf	= buf,
>> >  		}
>> >  	};
>> >  
>> > -	if (!block)
>> > -		return NULL;
>> > +	if (i2c_transfer(adapter, msgs, 2) != 2)
>> > +		return -1;
>> >  
>> > -	if (i2c_transfer(adapter, msgs, 2) != 2) {
>> > -		DRM_ERROR("Unable to read EDID.\n");
>> > -		goto err;
>> > -	}
>> > -
>> > -	if (!drm_edid_block_valid(block, 0, false, NULL)) {
>> > -		DRM_ERROR("Invalid EDID data\n");
>> > -		goto err;
>> > -	}
>> > -
>> > -	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
>> > -	if (total_size > EDID_LENGTH) {
>> > -		kfree(block);
>> > -		block = kmalloc(total_size, GFP_KERNEL);
>> > -		if (!block)
>> > -			return NULL;
>> > -
>> > -		/* Yes, read the entire buffer, and do not skip the first
>> > -		 * EDID_LENGTH bytes.
>> > -		 */
>> > -		start = 0x00;
>> > -		msgs[1].len = total_size;
>> > -		msgs[1].buf = block;
>> > -
>> > -		if (i2c_transfer(adapter, msgs, 2) != 2) {
>> > -			DRM_ERROR("Unable to read EDID extension blocks.\n");
>> > -			goto err;
>> > -		}
>> > -		if (!drm_edid_block_valid(block, 1, false, NULL)) {
>> > -			DRM_ERROR("Invalid EDID data\n");
>> > -			goto err;
>> > -		}
>> > -	}
>> > -
>> > -	return block;
>> > -
>> > -err:
>> > -	kfree(block);
>> > -	return NULL;
>> > +	return 0;
>> >  }
>> >  
>> >  static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
>> > @@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
>> >  
>> >  	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
>> >  
>> > -	return (struct edid *)stdp2690_get_edid(client);
>> > +	return drm_do_get_edid(connector, stdp2690_read_block, client, NULL);
>> 
>> The last NULL param should be dropped, as noted by the build bot.
>
> Tested with various displays, including displays that have one block of
> EDID (DELL P2417H) and a second EDID extension block (VES/55UHD_LCD_TV).
>
> Tested-by: Ian Ray <ian.ray@ge.com>
>
> Would you like me to submit the patch Jani?

If you have the time to take that on, by all means!

Thanks for testing!

BR,
Jani


>
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >  }
>> >  
>> >  static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center
>> 

-- 
Jani Nikula, Intel

Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Peter Senna Tschudin]

Good morning Jani,

It has been a long time since I wrote the driver, and many many years
since I sent my last kernel patch, so my memory does not serve me very
well, but I will try to shed some light.

On Fri, Sep 1, 2023 at 12:24 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> look up the discussion, but didn't find anyone questioning the EDID
> reading part.
>
> Why does it not use drm_get_edid() or drm_do_get_edid()?
>
> I don't know where client->addr comes from, so I guess it could be
> different from DDC_ADDR, rendering drm_get_edid() unusable.
>
> There's also the comment:
>
>         /* Yes, read the entire buffer, and do not skip the first
>          * EDID_LENGTH bytes.
>          */
>
> But again, there's not a word on *why*.

The video pipeline has two hardware bridges between the LVDS from the
SoC and DP+ output. For reasons, we would get hot plug events from one
of these bridges, and EDID from the other. If I am not mistaken, I
documented this strangeness in the DTS readme file.

Did this shed any light on the *why* or did I tell you something you
already knew?

Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Jani Nikula]

On Sat, 02 Sep 2023, Peter Senna Tschudin <peter.senna@gmail.com> wrote:
> Good morning Jani,
>
> It has been a long time since I wrote the driver, and many many years
> since I sent my last kernel patch, so my memory does not serve me very
> well, but I will try to shed some light.
>
> On Fri, Sep 1, 2023 at 12:24 PM Jani Nikula <jani.nikula@intel.com> wrote:
>>
>> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
>> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
>> look up the discussion, but didn't find anyone questioning the EDID
>> reading part.
>>
>> Why does it not use drm_get_edid() or drm_do_get_edid()?
>>
>> I don't know where client->addr comes from, so I guess it could be
>> different from DDC_ADDR, rendering drm_get_edid() unusable.
>>
>> There's also the comment:
>>
>>         /* Yes, read the entire buffer, and do not skip the first
>>          * EDID_LENGTH bytes.
>>          */
>>
>> But again, there's not a word on *why*.
>
> The video pipeline has two hardware bridges between the LVDS from the
> SoC and DP+ output. For reasons, we would get hot plug events from one
> of these bridges, and EDID from the other. If I am not mistaken, I
> documented this strangeness in the DTS readme file.
>
> Did this shed any light on the *why* or did I tell you something you
> already knew?

I guess that answers the question why it's necessary to specify the ddc
to use, but not why drm_do_get_edid() could not be used. Is it really
necessary to read the EDID in one go?

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: EXT: Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Ian Ray]

On Mon, Sep 04, 2023 at 01:04:40PM +0300, Jani Nikula wrote:
> 
> On Sat, 02 Sep 2023, Peter Senna Tschudin <peter.senna@gmail.com> wrote:
> > Good morning Jani,
> >
> > It has been a long time since I wrote the driver, and many many years
> > since I sent my last kernel patch, so my memory does not serve me very
> > well, but I will try to shed some light.
> >
> > On Fri, Sep 1, 2023 at 12:24 PM Jani Nikula <jani.nikula@intel.com> wrote:
> >>
> >> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> >> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> >> look up the discussion, but didn't find anyone questioning the EDID
> >> reading part.
> >>
> >> Why does it not use drm_get_edid() or drm_do_get_edid()?
> >>
> >> I don't know where client->addr comes from, so I guess it could be
> >> different from DDC_ADDR, rendering drm_get_edid() unusable.
> >>
> >> There's also the comment:
> >>
> >>         /* Yes, read the entire buffer, and do not skip the first
> >>          * EDID_LENGTH bytes.
> >>          */
> >>
> >> But again, there's not a word on *why*.
> >
> > The video pipeline has two hardware bridges between the LVDS from the
> > SoC and DP+ output. For reasons, we would get hot plug events from one
> > of these bridges, and EDID from the other. If I am not mistaken, I
> > documented this strangeness in the DTS readme file.
> >
> > Did this shed any light on the *why* or did I tell you something you
> > already knew?
> 
> I guess that answers the question why it's necessary to specify the ddc
> to use, but not why drm_do_get_edid() could not be used. Is it really
> necessary to read the EDID in one go?

Moi Jani,

Sorry for slow reply -- I will look into this during this week.  I would
hope and expect that we could use common APIs.


> 
> BR,
> Jani.
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
> 

Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Laurent Pinchart]

On Mon, Sep 04, 2023 at 01:04:40PM +0300, Jani Nikula wrote:
> On Sat, 02 Sep 2023, Peter Senna Tschudin wrote:
> > Good morning Jani,
> >
> > It has been a long time since I wrote the driver, and many many years
> > since I sent my last kernel patch, so my memory does not serve me very
> > well, but I will try to shed some light.
> >
> > On Fri, Sep 1, 2023 at 12:24 PM Jani Nikula wrote:
> >>
> >> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> >> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> >> look up the discussion, but didn't find anyone questioning the EDID
> >> reading part.
> >>
> >> Why does it not use drm_get_edid() or drm_do_get_edid()?
> >>
> >> I don't know where client->addr comes from, so I guess it could be
> >> different from DDC_ADDR, rendering drm_get_edid() unusable.
> >>
> >> There's also the comment:
> >>
> >>         /* Yes, read the entire buffer, and do not skip the first
> >>          * EDID_LENGTH bytes.
> >>          */
> >>
> >> But again, there's not a word on *why*.
> >
> > The video pipeline has two hardware bridges between the LVDS from the
> > SoC and DP+ output. For reasons, we would get hot plug events from one
> > of these bridges, and EDID from the other. If I am not mistaken, I
> > documented this strangeness in the DTS readme file.

This should be supported properly by the drm_bridge_connector helper,
which supports delegating HPD and EDID retrieval to different bridges.

> > Did this shed any light on the *why* or did I tell you something you
> > already knew?
> 
> I guess that answers the question why it's necessary to specify the ddc
> to use, but not why drm_do_get_edid() could not be used. Is it really
> necessary to read the EDID in one go?

-- 
Regards,

Laurent Pinchart

Re: [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Peter Senna Tschudin]

On Mon, Sep 4, 2023 at 12:16 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Sat, 02 Sep 2023, Peter Senna Tschudin <peter.senna@gmail.com> wrote:
> > Good morning Jani,
> >
> > It has been a long time since I wrote the driver, and many many years
> > since I sent my last kernel patch, so my memory does not serve me very
> > well, but I will try to shed some light.
> >
> > On Fri, Sep 1, 2023 at 12:24 PM Jani Nikula <jani.nikula@intel.com> wrote:
> >>
> >> The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
> >> Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
> >> look up the discussion, but didn't find anyone questioning the EDID
> >> reading part.
> >>
> >> Why does it not use drm_get_edid() or drm_do_get_edid()?
> >>
> >> I don't know where client->addr comes from, so I guess it could be
> >> different from DDC_ADDR, rendering drm_get_edid() unusable.
> >>
> >> There's also the comment:
> >>
> >>         /* Yes, read the entire buffer, and do not skip the first
> >>          * EDID_LENGTH bytes.
> >>          */
> >>
> >> But again, there's not a word on *why*.
> >
> > The video pipeline has two hardware bridges between the LVDS from the
> > SoC and DP+ output. For reasons, we would get hot plug events from one
> > of these bridges, and EDID from the other. If I am not mistaken, I
> > documented this strangeness in the DTS readme file.
> >
> > Did this shed any light on the *why* or did I tell you something you
> > already knew?
>
> I guess that answers the question why it's necessary to specify the ddc
> to use, but not why drm_do_get_edid() could not be used. Is it really
> necessary to read the EDID in one go?

I have a very weak recollection about hotplug and EDID issues with the
LVDS driver. I am not very confident about this, but maybe I needed to
find ways to read EDID early enough to please the LVDS display driver
during the LVDS driver startup.

[PATCH v2] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()

[Jani Nikula]

The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
look up the discussion, but didn't find anyone questioning the EDID
reading part.

Why does it not use drm_get_edid() or drm_do_get_edid()?

I don't know where client->addr comes from, so I guess it could be
different from DDC_ADDR, rendering drm_get_edid() unusable.

There's also the comment:

	/* Yes, read the entire buffer, and do not skip the first
	 * EDID_LENGTH bytes.
	 */

But again, there's not a word on *why*.

Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
away from their own EDID parsing and validity checks, including stop
using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
struct drm_edid, and friends, but this is the first step.)

v2: Fix build

Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Ian Ray <ian.ray@ge.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Yuan Can <yuancan@huawei.com>
Cc: Zheyu Ma <zheyuma97@gmail.com>
Tested-by: Ian Ray <ian.ray@ge.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I haven't even tried to compile this, and I have no way to test
this. Apologies for the long Cc list; I'm hoping someone could explain
the existing code, and perhaps give this approach a spin.
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
 1 file changed, 9 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 460db3c8a08c..e93083bbec9d 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
 
 static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
 
-static u8 *stdp2690_get_edid(struct i2c_client *client)
+static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
 {
+	struct i2c_client *client = context;
 	struct i2c_adapter *adapter = client->adapter;
-	unsigned char start = 0x00;
-	unsigned int total_size;
-	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	unsigned char start = block * EDID_LENGTH;
 
 	struct i2c_msg msgs[] = {
 		{
@@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
 		}, {
 			.addr	= client->addr,
 			.flags	= I2C_M_RD,
-			.len	= EDID_LENGTH,
-			.buf	= block,
+			.len	= len,
+			.buf	= buf,
 		}
 	};
 
-	if (!block)
-		return NULL;
+	if (i2c_transfer(adapter, msgs, 2) != 2)
+		return -1;
 
-	if (i2c_transfer(adapter, msgs, 2) != 2) {
-		DRM_ERROR("Unable to read EDID.\n");
-		goto err;
-	}
-
-	if (!drm_edid_block_valid(block, 0, false, NULL)) {
-		DRM_ERROR("Invalid EDID data\n");
-		goto err;
-	}
-
-	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
-	if (total_size > EDID_LENGTH) {
-		kfree(block);
-		block = kmalloc(total_size, GFP_KERNEL);
-		if (!block)
-			return NULL;
-
-		/* Yes, read the entire buffer, and do not skip the first
-		 * EDID_LENGTH bytes.
-		 */
-		start = 0x00;
-		msgs[1].len = total_size;
-		msgs[1].buf = block;
-
-		if (i2c_transfer(adapter, msgs, 2) != 2) {
-			DRM_ERROR("Unable to read EDID extension blocks.\n");
-			goto err;
-		}
-		if (!drm_edid_block_valid(block, 1, false, NULL)) {
-			DRM_ERROR("Invalid EDID data\n");
-			goto err;
-		}
-	}
-
-	return block;
-
-err:
-	kfree(block);
-	return NULL;
+	return 0;
 }
 
 static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
@@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
 
 	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
 
-	return (struct edid *)stdp2690_get_edid(client);
+	return drm_do_get_edid(connector, stdp2690_read_block, client);
 }
 
 static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
-- 
2.39.2