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

[Jani Nikula <jani.nikula@linux.intel.com>]

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