On Fri, 08 Sep 2023, Ian Ray <ian.ray@xxxxxx> wrote:
> 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?
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
