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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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