[PATCH V3] drm: edid: add support for E-DDC - dri-devel - freedesktop.org experimental mailing list

List overview All Threads
Download

[PATCH V3] drm: edid: add support for E-DDC

[Bug 25052] kernel modesetting...

[PATCH drm-next v2] drm: udl: usb:...

Shirish S

24 Aug 2012 24 Aug '12

9:26 a.m.

This patch adds support in probing 4 block edid data, for E-DDC. This is the first test case in CTS, for HDMI compliance.

Changes from V2: Add switch for DDC and E-DDC

Based on drm-next branch

Shirish S (1): drm: edid: add support for E-DDC

drivers/gpu/drm/drm_edid.c | 87 ++++++++++++++++++++++++++++++++------------ 1 files changed, 64 insertions(+), 23 deletions(-)

Reply

Show replies by date

Shirish S

24 Aug 24 Aug

9:26 a.m.

The current logic for probing ddc is limited to 2 blocks (256 bytes), this patch adds support for the 4 block (512) data.

To do this, a single 8-bit segment index is passed to the display via the I2C address 30h. Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal.

Signed-off-by: Shirish S s.shirish@samsung.com --- drivers/gpu/drm/drm_edid.c | 83 ++++++++++++++++++++++++++++++++----------- 1 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..a10a130 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) { unsigned char start = block * EDID_LENGTH; + unsigned char segment = block >> 1; + unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK; int ret, retries = 5;

/* The core i2c driver will automatically retry the transfer if the @@ -262,29 +264,68 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, * generate spurious NAKs and timeouts. Retrying the transfer * of the individual block a few times seems to overcome this. */ - do { - struct i2c_msg msgs[] = { - { - .addr = DDC_ADDR, - .flags = 0, - .len = 1, - .buf = &start, - }, { - .addr = DDC_ADDR, - .flags = I2C_M_RD, - .len = len, - .buf = buf, + switch (segment) { + /* To reduce traffic on I2C, we request the i2c xfer + * for segment addr only if 4 block edid data is + * present. + * 0 : DDC + * 1 : E-DDC + */ + case 0: + do { + struct i2c_msg msgs[] = { + { + .addr = DDC_ADDR, + .flags = 0, + .len = 1, + .buf = &start, + }, { + .addr = DDC_ADDR, + .flags = I2C_M_RD, + .len = len, + .buf = buf, + } + }; + ret = i2c_transfer(adapter, msgs, 2); + if (ret == -ENXIO) { + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", + adapter->name); + break; } - }; - ret = i2c_transfer(adapter, msgs, 2); - if (ret == -ENXIO) { - DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", - adapter->name); - break; - } - } while (ret != 2 && --retries); + } while (ret != 2 && --retries); + + return ret == 2 ? 0 : -1; + case 1: + do { + struct i2c_msg msgs[] = { + { /*set segment pointer */ + .addr = DDC_SEGMENT_ADDR, + .flags = segFlags, + .len = 1, + .buf = &segment, + }, { /*set offset */ + .addr = DDC_ADDR, + .flags = 0, + .len = 1, + .buf = &start, + }, { /*set data */ + .addr = DDC_ADDR, + .flags = I2C_M_RD, + .len = len, + .buf = buf, + } + }; + ret = i2c_transfer(adapter, msgs, 3); + if (ret == -ENXIO) { + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", + adapter->name); + break; + } + } while (ret != 3 && --retries);

- return ret == 2 ? 0 : -1; + return ret == 3 ? 0 : -1; + } + return -1; }

static bool drm_edid_is_zero(u8 *in_edid, int length)

-- 1.7.0.4

Reply

Daniel Vetter

6:20 p.m.

On Fri, Aug 24, 2012 at 02:56:50PM +0530, Shirish S wrote:

The current logic for probing ddc is limited to 2 blocks (256 bytes), this patch adds support for the 4 block (512) data.

To do this, a single 8-bit segment index is passed to the display via the I2C address 30h. Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal.

Signed-off-by: Shirish S s.shirish@samsung.com

drivers/gpu/drm/drm_edid.c | 83 ++++++++++++++++++++++++++++++++----------- 1 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..a10a130 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, int block, int len) { unsigned char start = block * EDID_LENGTH;

unsigned char segment = block >> 1;

unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK;

You can inline segFlags to the only place it's used.

int ret, retries = 5;

/* The core i2c driver will automatically retry the transfer if the @@ -262,29 +264,68 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, * generate spurious NAKs and timeouts. Retrying the transfer * of the individual block a few times seems to overcome this. */
do {
struct i2c_msg msgs[] = {
	{
		.addr	= DDC_ADDR,
		.flags	= 0,
		.len	= 1,
		.buf	= &start,
	}, {
		.addr	= DDC_ADDR,
		.flags	= I2C_M_RD,
		.len	= len,
		.buf	= buf,
switch (segment) {
/* To reduce traffic on I2C, we request the i2c xfer
 * for segment addr only if 4 block edid data is
 * present.
 * 0 : DDC
 * 1 : E-DDC
 */

This comment is a bit misleading: We don't avoid the segment xfer to reduce i2c traffic, but to avoid upsetting any non-compliant screens.

case 0:
do {

Keeping the loop as the outermost would imo be clearer and avoid duplicating it (it also would make the patch a bit smaller). You could even do a trick with the msgs array like this:

msgs[] = { full i2c xfer including segment addr} xfers = 3;

/* Avoid sending the segment addr to not upset non-compliant ddc * monitors. */ if (!segments) msgs++, len--;

i2c_transefer(adapter, msgs, len);

Also, I think it'd be good to set the IGNORE_NAK on the start addre transfer, too (but in a separate patch). At least I think we've seen screens out there that need this :(

Yours, Daniel

```
	struct i2c_msg msgs[] = {
```
```
		{
```
```
			.addr	= DDC_ADDR,
```
```
			.flags	= 0,
```
```
			.len	= 1,
```
```
			.buf	= &start,
```
```
		}, {
```
```
			.addr	= DDC_ADDR,
```
```
			.flags	= I2C_M_RD,
```
```
			.len	= len,
```
```
			.buf	= buf,
```
```
		}
```
```
	};
```
```
	ret = i2c_transfer(adapter, msgs, 2);
```
```
	if (ret == -ENXIO) {
```

		DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",

```
					adapter->name);
```
```
		break;
}
```

```
};
```
```
ret = i2c_transfer(adapter, msgs, 2);
```
```
if (ret == -ENXIO) {
```

	DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",

```
			adapter->name);
```
```
	break;
```
```
}
```
} while (ret != 2 && --retries);

```
} while (ret != 2 && --retries);
```
```
return ret == 2 ? 0 : -1;
```
case 1:
```
do {
```
```
	struct i2c_msg msgs[] = {
```
```
		{ /*set segment pointer */
```
```
			.addr	= DDC_SEGMENT_ADDR,
```
```
			.flags	= segFlags,
```
```
			.len	= 1,
```
```
			.buf	= &segment,
```
```
		}, { /*set offset */
```
```
			.addr	= DDC_ADDR,
```
```
			.flags	= 0,
```
```
			.len	= 1,
```
```
			.buf	= &start,
```
```
		}, { /*set data */
```
```
			.addr	= DDC_ADDR,
```
```
			.flags	= I2C_M_RD,
```
```
			.len	= len,
```
```
			.buf	= buf,
```
```
		}
```
```
	};
```
```
	ret = i2c_transfer(adapter, msgs, 3);
```
```
	if (ret == -ENXIO) {
```

		DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",

```
					adapter->name);
```
```
		break;
```
```
	}
```
```
} while (ret != 3 && --retries);
```

return ret == 2 ? 0 : -1;

```
return ret == 3 ? 0 : -1;
```
}
return -1;

}

static bool drm_edid_is_zero(u8 *in_edid, int length)

1.7.0.4

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

-- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48

Reply

Shirish S

25 Aug 25 Aug

12:26 a.m.

Thanks for the comments, will upload patch set with your comments.

On Fri, Aug 24, 2012 at 11:20 AM, Daniel Vetter daniel@ffwll.ch wrote:

On Fri, Aug 24, 2012 at 02:56:50PM +0530, Shirish S wrote:

The current logic for probing ddc is limited to 2 blocks (256 bytes), this patch adds support for the 4 block (512) data.

To do this, a single 8-bit segment index is passed to the display via the I2C address 30h. Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal.

Signed-off-by: Shirish S s.shirish@samsung.com

drivers/gpu/drm/drm_edid.c | 83

++++++++++++++++++++++++++++++++-----------

1 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..a10a130 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -254,6 +254,8 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,

unsigned char *buf,

                int block, int len)

{ unsigned char start = block * EDID_LENGTH;

```
unsigned char segment = block >> 1;
```

unsigned short segFlags = segment ? 0 : I2C_M_IGNORE_NAK;

You can inline segFlags to the only place it's used.

  int ret, retries = 5;

  /* The core i2c driver will automatically retry the transfer if the
@@ -262,29 +264,68 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,

unsigned char *buf,

   * generate spurious NAKs and timeouts. Retrying the transfer
   * of the individual block a few times seems to overcome this.
   */

```
do {
```
```
        struct i2c_msg msgs[] = {
```
```
                {
```

                        .addr   = DDC_ADDR,

```
                        .flags  = 0,
```
```
                        .len    = 1,
```

                        .buf    = &start,

```
                }, {
```

                        .addr   = DDC_ADDR,

                        .flags  = I2C_M_RD,

```
                        .len    = len,
```
```
                        .buf    = buf,
```

```
switch (segment) {
```

        /* To reduce traffic on I2C, we request the i2c xfer

         * for segment addr only if 4 block edid data is

```
         * present.
```
```
         * 0 : DDC
```
```
         * 1 : E-DDC
```
```
         */
```

This comment is a bit misleading: We don't avoid the segment xfer to reduce i2c traffic, but to avoid upsetting any non-compliant screens.

```
case 0:
```
```
        do {
```

Keeping the loop as the outermost would imo be clearer and avoid duplicating it (it also would make the patch a bit smaller). You could even do a trick with the msgs array like this:

    msgs[] = { full i2c xfer including segment addr}
    xfers = 3;

    /* Avoid sending the segment addr to not upset non-compliant ddc
     * monitors. */
    if (!segments)
            msgs++, len--;

    i2c_transefer(adapter, msgs, len);

Also, I think it'd be good to set the IGNORE_NAK on the start addre transfer, too (but in a separate patch). At least I think we've seen screens out there that need this :(

Yours, Daniel

                struct i2c_msg msgs[] = {

```
                        {
```

                                .addr   = DDC_ADDR,

                                .flags  = 0,

                                .len    = 1,

                                .buf    = &start,

```
                        }, {
```

                                .addr   = DDC_ADDR,

                                .flags  = I2C_M_RD,

                                .len    = len,

                                .buf    = buf,

```
                        }
```
```
                };
```

                ret = i2c_transfer(adapter, msgs, 2);

```
                if (ret == -ENXIO) {
```

                        DRM_DEBUG_KMS("drm: skipping non-existent

adapter %s\n",

                                                adapter->name);

                        break;
                }

```
        };
```

        ret = i2c_transfer(adapter, msgs, 2);

```
        if (ret == -ENXIO) {
```

                DRM_DEBUG_KMS("drm: skipping non-existent adapter

%s\n",

                                adapter->name);

```
                break;
```
```
        }
```
```
} while (ret != 2 && --retries);
```

        } while (ret != 2 && --retries);

```
        return ret == 2 ? 0 : -1;
```
```
case 1:
```
```
        do {
```

                struct i2c_msg msgs[] = {

                        { /*set segment pointer */

                                .addr   = DDC_SEGMENT_ADDR,

                                .flags  = segFlags,

                                .len    = 1,

                                .buf    = &segment,

                        }, { /*set offset */

                                .addr   = DDC_ADDR,

                                .flags  = 0,

                                .len    = 1,

                                .buf    = &start,

                        }, { /*set data */

                                .addr   = DDC_ADDR,

                                .flags  = I2C_M_RD,

                                .len    = len,

                                .buf    = buf,

```
                        }
```
```
                };
```

                ret = i2c_transfer(adapter, msgs, 3);

```
                if (ret == -ENXIO) {
```

                        DRM_DEBUG_KMS("drm: skipping non-existent

adapter %s\n",

                                                adapter->name);

```
                        break;
```
```
                }
```

        } while (ret != 3 && --retries);

```
return ret == 2 ? 0 : -1;
```

```
        return ret == 3 ? 0 : -1;
```
```
}
```
```
return -1;
```

}

static bool drm_edid_is_zero(u8 *in_edid, int length)

1.7.0.4

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

-- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply

4639

Age (days ago)

4640

Last active (days ago)

dri-devel@lists.freedesktop.org

3 comments

3 participants

tags (0)

participants (3)

Daniel Vetter
Shirish S
Shirish S