[RFC] drm/bridge/sii902x: Fix EDID readback

[Fabrizio Castro]

Hello Peter,

Again, thank you very much for your precious comments.
I'll send a patch out shortly addressing all of the comments I have received so far,
including yours.

Cheers,
Fab

> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback
>
> On 2018-11-01 18:32, Fabrizio Castro wrote:
> > Hello Peter,
> >
> > Thank you for your feedback!
> >
> >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback
> >
> > snip
> >
> >>>
> >>> To further detail the problem, the system is vulnerable from before the last write
> >>> performed by sii902x_i2c_bypass_select to after we confirm we need the switch
> >>> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time
> >>> we could keep the parent adapter locked for, I guess I am stuck with a parent-locking
> >>> architecture, aren't I?
> >>
> >> If that problem description is correct, then yes, I think the *only* solution
> >> is to combine the three parts "open bypass mode", "edid xfer" and "close bypass
> >> mode", and to keep the i2c adapter locked during the procedure so that other
> >> xfers do not creep in and crap thing up from the side. And one way to combine
> >> the three parts is to use a parent-locked i2c gate. And since you need to keep
> >> the i2c adapter locked over the whole procedure, you need to use unlocked xfers
> >> (as you have already discovered). But how do you know that this problem
> >> description is accurate?
> >
> > I basically observed what was going on on the bus (with a logic analyser) while generating
> > traffic on the parent adapter
> >
> >> Why is it not ok for unrelated xfers to creep in
> >> between opening the bypass mode and the edid xfer, and how do you know that
> >> this is not ok?
> >
> > Because those transfers would come with no extra delay between STOP and START
> > conditions while the HDMI transmitter is in passthrough mode
>
> Yeah yeah, now I get it. It's not the edid eeprom that's bad, it's the
> passthrough mode in the hdmi transmitter that's broken. Your problem
> description follows naturally.
>
> >>
> >>> But I guess I could release it when it's not actually needed,
> >>
> >> How would you figure out when it's not needed?
> >
> > The moment you tell the HDMI transmitter you are going to talk to the monitor nothing else can
> > flow on the bus, up until you gracefully close the pass through session, which means I wouldn't
> > really need to hold the parent lock during the entire duration of the select callback, I would need
> > to hold the parent lock only from before the last write as that's when we tell the HDMI transmitter
> > to activate the passthrough mode, but I guess it would make the driver hard to maintain (as in
> > others would need to understand this properly before making any changes), wouldn't it?
>
> Yes, that would just complicate things and would probably not have all that
> much benefit. I don't think I'd go there...
>
> >>
> >>> or is this going to be a pain to maintain? Shall I just keep going with the parent-locking
> >>> but using bare i2c transactions only within select and deselect and leave regmap
> >>> to deal with everything else?
> >>
> >> That's a possibility. Take care to not mess up any cached state in regmap though.
> >
> > The original version of the driver wasn't using any caching, so I guess I would need to fallback
> > exactly to the same implementation.
> >
> > So, what should I do? Should I keep the parent-locking, the unlocked flavours of rd/wr/rmw from
> > within select/deselect, and put back regmap based rd/wr/rmw with no caching for everything else?
>
> I think that sounds like a reasonable compromise, but be careful since you still
> might need the two implementations to interact, e.g. the two rmw variants might
> still need a common lock so that they don't trample on each others toes. At
> least if there are accesses to the same register (SII902X_SYS_CTRL_DATA in this
> case if I read it right).
>
> Cheers,
> Peter



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.