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