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.
