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

[Peter Rosin]

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