On Fri, Sep 01, 2023 at 05:52:02PM +0300, Jani Nikula wrote: > > On Fri, 01 Sep 2023, Jani Nikula <jani.nikula@xxxxxxxxx> 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@xxxxxxxxx> > > Cc: Ian Ray <ian.ray@xxxxxx> > > Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx> > > Cc: Jonas Karlman <jonas@xxxxxxxxx> > > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx> > > Cc: Martin Donnelly <martin.donnelly@xxxxxx> > > Cc: Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx> > > Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx> > > Cc: Peter Senna Tschudin <peter.senna@xxxxxxxxx> > > Cc: Robert Foss <rfoss@xxxxxxxxxx> > > Cc: Yuan Can <yuancan@xxxxxxxxxx> > > Cc: Zheyu Ma <zheyuma97@xxxxxxxxx> > > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> > > > > --- > > > > 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@xxxxxx> 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 >