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

[Fabrizio Castro]

Hello Peter,

Thank you for your feedback!

snip

> > Yeah, there is some issue with locking here, and that's coming down
> > from the fact that when we call into drm_get_edid, we implicitly call
> > i2c_transfer which ultimately locks the i2c adapter, and then calls
> > into our sii902x_i2c_bypass_select, which in turn calls into the regmap
> > functions and therefore we try and lock the same i2c adapter again,
> > driving the system into a deadlock.
> > Having the option of using "unlocked" flavours of reads and writes
> > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c
> > I couldn't find anything suitable for my case, maybe Mark could advise
> > on this one? I am sure I overlooked something here, is there a better
> > way to address this?
>
> This recursive locking problem surfaces when an i2c mux is operated
> by writing commands on the same i2c bus that is muxed. When this
> happens for a typical i2c mux chip like nxp,pca9548 this can be kept
> local to that driver. However, when there are interactions with other
> parts of the kernel (e.g. if the i2c-mux is operated by gpio pins,
> and those gpio pins in turn are operated by accesses to the i2c bus
> that is muxed) this locked vs. unlocked thing would spread like
> wildfire.
>
> Since I did not like that wildfire, I came up with the "mux-locked"
> scheme. It is not appropriate everywhere, but in this case I think it
> is a perfect fit. By using it, you should be able to dodge all locking
> issues and it should be possible to keep the regmap usage as-is and the
> patch would in all likelihood be much less intrusive.
>
> For some background on "mux-locked" vs. "parent-locked" (which is what
> you have used in this RFC patch), see Documentation/i2c/i2c-topology.

The "mux-locked" implementation was the one I first tried and I discovered
it doesn't work for me, as other traffic on the parent adapter can get in the
way. What we need for this device is no other traffic on the bus during the
"select transaction deselect" procedure, that's why I went with the parent
locking. Also this device needs a delay between stop and start conditions
while addressing the monitor.

>
> There are a couple of more comments below, inline.
>

snip

> >>>
> >>> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> >>> +{
> >>> +       struct sii902x *sii902x = i2c_mux_priv(mux);
> >>> +       struct device *dev = &sii902x->i2c->dev;
> >>> +       unsigned long timeout;
> >>> +       u8 status;
> >>> +       int ret;
> >>> +
> >>> +       ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> >>> +                                   SII902X_SYS_CTRL_DDC_BUS_REQ,
> >>> +                                   SII902X_SYS_CTRL_DDC_BUS_REQ);
> >>> +
> >>> +       timeout = jiffies +
> >>> +                 msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> >>> +       do {
> >>> +               ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> >>> +                                    &status);
> >>> +               if (ret)
> >>> +                       return ret;
> >>> +       } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> >>> +                time_before(jiffies, timeout));
> >>> +
> >>> +       if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> >>> +               dev_err(dev, "Failed to acquire the i2c bus\n");
> >>> +               return -ETIMEDOUT;
> >>> +       }
> >>> +
> >>> +       ret = __sii902x_write(sii902x->i2c, SII902X_SYS_CTRL_DATA, status);
> >>> +       if (ret)
> >>> +               return ret;
>
> Why do I not see a delay here? I thought the whole point of adding the i2c gate
> was that it enabled the introduction of a delay between opening for edid reading
> and the actual edid reading?

The delay is needed between STOP and START condition while in passthrough mode.
__i2c_transfer gets called after the select callback (which enables the passthrough
mode), when __i2c_transfer is done it generates a STOP condition and then we call into
the deselect callback. We need a delay between __i2c_transfer and deselect.

>
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> >>> +{
> >>> +       struct sii902x *sii902x = i2c_mux_priv(mux);
> >>> +       struct device *dev = &sii902x->i2c->dev;
> >>> +       unsigned long timeout;
> >>> +       unsigned int retries;
> >>> +       u8 status;
> >>> +       int ret;
> >>> +
> >>> +       udelay(30);

Here is where we need the delay

snip

> >>> @@ -433,6 +558,22 @@ static int sii902x_probe(struct i2c_client *client,
> >>>
> >>>         i2c_set_clientdata(client, sii902x);
> >>>
> >>> +       sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> >>> +                                       1, 0, I2C_MUX_GATE,
>
> Just use I2C_MUX_GATE | I2C_MUX_LOCKED for the flags argument, and you should be
> able to make normal i2c accesses.

This is precisely what I tried in the first place, but by generating traffic on the parent adapter
(since I don't have any other slave on the same i2c bus I have been using i2cdetect) while talking
to the monitor I have been able to break communications, and sometimes stall the bus, therefore
I have taken out I2C_MUX_LOCKED.

Ah, do you think the following is going to cause any issue in the future?
-edid = drm_get_edid(connector, sii902x->i2c->adapter);
+edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);

Thanks,
Fab

>
> Cheers,
> Peter
>



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