Re: [PATCH] drm/bridge: sii902x: fix get edid may fail

[Andrea Merello <andrea.merello@gmail.com>]

On Mon, Jan 23, 2017 at 12:32 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 23 Jan 2017, Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> Hi Andrea,
>>
>> On Mon, 23 Jan 2017 11:00:02 +0100
>> Andrea Merello <andrea.merello@gmail.com> wrote:
>>
>>> From: Andrea Merello <andrea.merello@gmail.com>
>>>
>>> The standard DRM function to get the edid from the i2c bus performs
>>> (at least) two transfers.
>>>
>>> By experiments it seems that the sii9022a have problems with the
>>> 2nd I2C start, at least unless a wait is introduced detween the
>>
>>                                                     ^ between
>>
>>> two transfers.
>>>
>>> So we perform one single I2C transfer, and if the transfer must be
>>> split, then we introduce a delay.
>>
>> That's not exactly what this patch does: you're introducing a delay
>> between each retry. So, if the transceiver really requires a delay
>> between each transfer, you'll have to retry at least once on the 2nd
>> transfer.
>>
>> I guess a better solution would be to add a delay even in case of
>> success, or maybe modify drm_do_get_edid() to optionally wait for a
>> specified time between each transfer.
>
> Is the problem related specifically to EDID reads, or generally to I2C
> transfers? Perhaps this should be fixed at the adapter master_xfer layer
> instead? Does the master_xfer perhaps return -EAGAIN, have you looked
> into i2c_adapter timeout member? (See __i2c_transfer in i2c-core.c.)
>
> The intention of drm_do_get_edid() is to facilitate *alternative*
> methods to doing I2C, not to workaround quirks like this.

This problem seems not related to the I2C master itself. I saw the I2C
master to TX correctly by looking at the I2C bus before the sii9022a
with the scope, and I saw the same I2C transfer to be corrupted after
the sii9022a; actually the sii9022a seems to gate the bus damaging the
I2C start of the 2nd transfer, unless we place this delay.

This happened with two different I2C master (the one on the SoC as
well as another different one implemented on FPGA).

So IMHO this quirk should be done in the sii902x dirver. Said that, I
admit that I've not looked at i2c_adapter timeout member or other
details in upper layers.

> BR,
> Jani.
>
>
>>
>> BTW, sii902x_do_probe_ddc_edid() and drm_do_probe_ddc_edid() are almost
>> identical (except for the extra delay()), so maybe we should export
>> drm_do_probe_ddc_edid() and add an extra delay_us to it.
>>
>>>
>>> Signed-off-by: Andrea Merello <andrea.merello@iit.it>
>>> Cc: Andrea Merello <andrea.merello@gmail.com>
>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>>> Cc: David Airlie <airlied@linux.ie>
>>> ---
>>>  drivers/gpu/drm/bridge/sii902x.c | 70 +++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 69 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>>> index 9126d03..042d7e2 100644
>>> --- a/drivers/gpu/drm/bridge/sii902x.c
>>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>>> @@ -133,6 +133,62 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
>>>      .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>  };
>>>
>>> +#define DDC_SEGMENT_ADDR 0x30
>>> +static int sii902x_do_probe_ddc_edid(void *data, u8 *buf,
>>> +                                 unsigned int block, size_t len)
>>> +{
>>> +    struct i2c_adapter *adapter = data;
>>> +    unsigned char start = block * EDID_LENGTH;
>>> +    unsigned char segment = block >> 1;
>>> +    unsigned char xfers = segment ? 3 : 2;
>>> +    int ret, retries = 5;
>>> +
>>> +    /*
>>> +     * The core I2C driver will automatically retry the transfer if the
>>> +     * adapter reports EAGAIN. However, we find that bit-banging transfers
>>> +     * are susceptible to errors under a heavily loaded machine and
>>> +     * generate spurious NAKs and timeouts. Retrying the transfer
>>> +     * of the individual block a few times seems to overcome this.
>>> +     */
>>> +    while (1) {
>>> +            struct i2c_msg msgs[] = {
>>> +                    {
>>> +                            .addr   = DDC_SEGMENT_ADDR,
>>> +                            .flags  = 0,
>>> +                            .len    = 1,
>>> +                            .buf    = &segment,
>>> +                    }, {
>>> +                            .addr   = DDC_ADDR,
>>> +                            .flags  = 0,
>>> +                            .len    = 1,
>>> +                            .buf    = &start,
>>> +                    }, {
>>> +                            .addr   = DDC_ADDR,
>>> +                            .flags  = I2C_M_RD,
>>> +                            .len    = len,
>>> +                            .buf    = buf,
>>> +                    }
>>> +            };
>>> +
>>> +            /*
>>> +             * Avoid sending the segment addr to not upset non-compliant
>>> +             * DDC monitors.
>>> +             */
>>> +            ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
>>> +
>>> +            if (ret == -ENXIO) {
>>> +                    DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",
>>> +                                  adapter->name);
>>> +                    break;
>>> +            }
>>> +            if (ret == xfers || --retries == 0)
>>> +                    break;
>>> +
>>> +            udelay(100);
>>> +    }
>>> +
>>> +    return ret == xfers ? 0 : -1;
>>> +}
>>
>> Missing empty line here.
>>
>>>  static int sii902x_get_modes(struct drm_connector *connector)
>>>  {
>>>      struct sii902x *sii902x = connector_to_sii902x(connector);
>>> @@ -168,8 +224,20 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>>      if (ret)
>>>              return ret;
>>>
>>> -    edid = drm_get_edid(connector, sii902x->i2c->adapter);
>>> +    /* drm_get_edid() runs two I2C transfers. The sii902x seems
>>
>> Please use kernel comment style:
>>
>>       /*
>>        * ...
>>
>>> +     * to have problem with the 2nd I2C start. A wait seems needed.
>>> +     * So, we don't perform use drm_get_edid(). We don't perform
>>> +     * the first "probe" transfer, and we use a custom block read
>>> +     * function that, in case the trasfer is split, does introduce
>>> +     * a delay.
>>> +     */
>>> +    edid = drm_do_get_edid(connector, sii902x_do_probe_ddc_edid,
>>> +                           sii902x->i2c->adapter);
>>> +    if (!edid)
>>> +            return num;
>>> +
>>
>> drm_get_edid() calls drm_get_displayid() just after drm_do_get_edid().
>> Are you sure this is not needed here?
>>
>>>      drm_mode_connector_update_edid_property(connector, edid);
>>> +
>>>      if (edid) {
>>>              num = drm_add_edid_modes(connector, edid);
>>>              kfree(edid);
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel