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

[Andrea Merello]

On Mon, Jan 23, 2017 at 11:20 AM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> Hi Andrea,
>
> On Mon, 23 Jan 2017 11:00:02 +0100
> Andrea Merello <andrea.merello at gmail.com> wrote:
>
>> From: Andrea Merello <andrea.merello at 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

OK

>> 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.

You are right. I've missed that. The delay should be placed just after
(or maybe before) i2c_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.

I thought the requirement for delay to be specific to this HDMI
encoder; if you think that it can be useful even for other chips then
IMHO it might worth to add the optional delay in the core code, such
as drm_do_get_edid() as you suggested.

> 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.

I like the idea, but  this would not introduce any delay between
retries, that I would assume to be required.

>>
>> Signed-off-by: Andrea Merello <andrea.merello at iit.it>
>> Cc: Andrea Merello <andrea.merello at gmail.com>
>> Cc: Boris Brezillon <boris.brezillon at free-electrons.com>
>> Cc: Archit Taneja <architt at codeaurora.org>
>> Cc: David Airlie <airlied at 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.

OK

>
>>  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:

OK

>         /*
>          * ...
>
>> +      * 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?

No :/  I don't know why I've missed it; that was not intentional.

>>       drm_mode_connector_update_edid_property(connector, edid);
>> +
>>       if (edid) {
>>               num = drm_add_edid_modes(connector, edid);
>>               kfree(edid);
>