On Mon, Jan 23, 2017 at 11:20 AM, Boris Brezillon
<boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Andrea,
>
> On Mon, 23 Jan 2017 11:00:02 +0100
> Andrea Merello <andrea.merello@xxxxxxxxx> wrote:
>
>> From: Andrea Merello <andrea.merello@xxxxxxxxx>
>>
>> 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@xxxxxx>
>> Cc: Andrea Merello <andrea.merello@xxxxxxxxx>
>> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> ---
>> 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);
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
