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
