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

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

[Andrea Merello]

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

So we perform one single I2C transfer, and if the transfer must be
split, then we introduce a delay.

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;
+}
 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
+	 * 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_mode_connector_update_edid_property(connector, edid);
+
 	if (edid) {
 		num = drm_add_edid_modes(connector, edid);
 		kfree(edid);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

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.

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

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

[Jani Nikula]

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.

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

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

[Andrea Merello]

On Mon, Jan 23, 2017 at 11:20 AM, 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

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

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrea Merello]

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

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

[Boris Brezillon]

On Mon, 23 Jan 2017 13:12:12 +0100
Andrea Merello <andrea.merello@gmail.com> wrote:

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

Yes, I tend to agree with that.

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

Actually, I faced the same problem on an at91-based platform, and came
up with pretty much the same fix (add a delay after each transfer),
except mine was uglier and I never took the time to clean it up and
submit it.

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

Well, let's sum up the situation: the sii902x device is acting both as
a regular i2c device and as an i2c 'pass-through' device. In the
pass-through mode, the si902x has a new constraint: the i2c transfers
have to be separated by an idle period.

Maybe we should expose the pass-through mode as separate i2c adapter,
that would be registered when entering pass-through mode and
de-registered when leaving this mode.
This way, we can implement the ->master_xfer() as we need (add a delay
after each xfer), and still use the generic drm_get_edid().

What do you think?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrea Merello]

On Mon, Jan 23, 2017 at 1:36 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Mon, 23 Jan 2017 13:12:12 +0100
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
>> 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.
>
> Yes, I tend to agree with that.
>
>>
>> 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).
>
> Actually, I faced the same problem on an at91-based platform, and came
> up with pretty much the same fix (add a delay after each transfer),
> except mine was uglier and I never took the time to clean it up and
> submit it.
>
>>
>> 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.
>
> Well, let's sum up the situation: the sii902x device is acting both as
> a regular i2c device and as an i2c 'pass-through' device. In the
> pass-through mode, the si902x has a new constraint: the i2c transfers
> have to be separated by an idle period.
>
> Maybe we should expose the pass-through mode as separate i2c adapter,
> that would be registered when entering pass-through mode and
> de-registered when leaving this mode.
> This way, we can implement the ->master_xfer() as we need (add a delay
> after each xfer), and still use the generic drm_get_edid().
>
> What do you think?

IMHO this could be a good way to go.. Although honestly the current
quirk doesn't seem so much bad to me :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Mon, Jan 23, 2017 at 03:06:36PM +0100, Andrea Merello wrote:
> On Mon, Jan 23, 2017 at 1:36 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Mon, 23 Jan 2017 13:12:12 +0100
> > Andrea Merello <andrea.merello@gmail.com> wrote:
> >
> >> 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.
> >
> > Yes, I tend to agree with that.
> >
> >>
> >> 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).
> >
> > Actually, I faced the same problem on an at91-based platform, and came
> > up with pretty much the same fix (add a delay after each transfer),
> > except mine was uglier and I never took the time to clean it up and
> > submit it.
> >
> >>
> >> 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.
> >
> > Well, let's sum up the situation: the sii902x device is acting both as
> > a regular i2c device and as an i2c 'pass-through' device. In the
> > pass-through mode, the si902x has a new constraint: the i2c transfers
> > have to be separated by an idle period.
> >
> > Maybe we should expose the pass-through mode as separate i2c adapter,
> > that would be registered when entering pass-through mode and
> > de-registered when leaving this mode.
> > This way, we can implement the ->master_xfer() as we need (add a delay
> > after each xfer), and still use the generic drm_get_edid().
> >
> > What do you think?
> 
> IMHO this could be a good way to go.. Although honestly the current
> quirk doesn't seem so much bad to me :)

I agree with Jani that using drm_do_get_edid is the wrong thing to do
here, it's really for cases where you have a hw edid-fetch engine that
entirely hides the underlying i2c bus. If you do have the i2c bus exposed,
then the right place to fix it is at the i2c_adapter level. There's a pile
of userspace tools that also probe the ddc i2c bus (there's not just an
edid there), and if you don't fix this at the i2c_adapter level those
would still be broken.

Since this seems unclear, can you pls also do a patch for the kerneldoc
for the drm_do_get_edid function, so it's clear when and when not this
should be used?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jani Nikula]

On Mon, 23 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jan 23, 2017 at 03:06:36PM +0100, Andrea Merello wrote:
>> On Mon, Jan 23, 2017 at 1:36 PM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > On Mon, 23 Jan 2017 13:12:12 +0100
>> > Andrea Merello <andrea.merello@gmail.com> wrote:
>> >
>> >> 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.
>> >
>> > Yes, I tend to agree with that.
>> >
>> >>
>> >> 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).
>> >
>> > Actually, I faced the same problem on an at91-based platform, and came
>> > up with pretty much the same fix (add a delay after each transfer),
>> > except mine was uglier and I never took the time to clean it up and
>> > submit it.
>> >
>> >>
>> >> 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.
>> >
>> > Well, let's sum up the situation: the sii902x device is acting both as
>> > a regular i2c device and as an i2c 'pass-through' device. In the
>> > pass-through mode, the si902x has a new constraint: the i2c transfers
>> > have to be separated by an idle period.
>> >
>> > Maybe we should expose the pass-through mode as separate i2c adapter,
>> > that would be registered when entering pass-through mode and
>> > de-registered when leaving this mode.
>> > This way, we can implement the ->master_xfer() as we need (add a delay
>> > after each xfer), and still use the generic drm_get_edid().
>> >
>> > What do you think?
>> 
>> IMHO this could be a good way to go.. Although honestly the current
>> quirk doesn't seem so much bad to me :)
>
> I agree with Jani that using drm_do_get_edid is the wrong thing to do
> here, it's really for cases where you have a hw edid-fetch engine that
> entirely hides the underlying i2c bus. If you do have the i2c bus exposed,
> then the right place to fix it is at the i2c_adapter level. There's a pile
> of userspace tools that also probe the ddc i2c bus (there's not just an
> edid there), and if you don't fix this at the i2c_adapter level those
> would still be broken.
>
> Since this seems unclear, can you pls also do a patch for the kerneldoc
> for the drm_do_get_edid function, so it's clear when and when not this
> should be used?

IMO it's perfectly clear in the kernel-doc.

BR,
Jani.


>
> Thanks, Daniel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel