Re: [PATCH] drm/bridge: adv7511: fix support for large EDIDs

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Fri, Mar 26, 2021 at 07:31:09AM +0100, Hans Verkuil wrote:
> On 26/03/2021 02:00, Laurent Pinchart wrote:
> > On Wed, Mar 24, 2021 at 09:53:32AM +0100, Hans Verkuil wrote:
> >> While testing support for large (> 256 bytes) EDIDs on the Renesas
> >> Koelsch board I noticed that the adv7511 bridge driver only read the
> >> first two blocks.
> >>
> >> The media V4L2 version for the adv7511 (drivers/media/i2c/adv7511-v4l2.c)
> >> handled this correctly.
> >>
> >> Besides a simple bug when setting the segment register (it was set to the
> >> block number instead of block / 2), the logic of the code was also weird.
> >> In particular reading the DDC_STATUS is odd: this is unrelated to EDID
> >> reading.
> > 
> > Bits 3:0 of DDC_STATUS report the DDC controller state, which can be
> > used to wait until the DDC controller is idle (it reports, among other
> > possible states, if an EDID read is in progress). Other options are
> > possible of course, including waiting for ADV7511_INT0_EDID_READY as
> > done in adv7511_wait_for_edid(), but I wonder if the !irq case in
> > adv7511_wait_for_edid() wouldn't be better of busy-looping on the DDC
> > status instead of running the interrupt handler manually. That's
> > unrelated to this patch though.
> 
> The DDC status tests for other things as well, including HDCP.

I haven't read the chip's documentation in details, but if HDCP
negotiation is in progress, doesn't that keep the DDC bus busy,
preventing an EDID read ?

> I think it is pure luck that this code even worked:
> 
>         if (adv7511->current_edid_segment != block / 2) {
>                 unsigned int status;
> 
>                 ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
>                                   &status);
>                 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;
>                 }
> 
> What happens on power on is that the adv7511 starts reading the EDID.
> So the DDC_STATUS is 1 (Reading EDID). This code is called, it falls
> in the status != 2 block, it writes the EDID_SEGMENT with 0 (it already
> is 0 after a power on), then waits for the EDID read to finish.
> 
> The only reason this works is that this code is called fast enough
> after the device is powered on that it is still reading the EDID.

Yes, I agree with you. Luck is nice, except when it makes us merge
incorrect code :-)

> It fails if you want to read the next segment, since in that case the
> status is 2 (IDLE) and it will never write the new segment to the
> EDID_SEGMENT register.
> 
> And besides, status wasn't ANDed with 0xf either, and HDCP might
> also be ongoing (should that be enabled in the future).

-- 
Regards,

Laurent Pinchart