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

[Fabrizio Castro]

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

>
> > 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?

>
> > 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?

Thank you!

Fab

> The registers you touch from select/deselect should probably be volatile or
> something like that?
>
> Cheers,
> Peter



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