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