[PATCH V3] drm: edid: add support for E-DDC
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 > --- > 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, > +
Re: [PATCH V3] drm: edid: add support for E-DDC
Thanks for the comments, will upload patch set with your comments. On Fri, Aug 24, 2012 at 11:20 AM, Daniel Vetter 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 > > --- > > 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 */ > > +
[PATCH V3] drm: edid: add support for E-DDC
Thanks for the comments, will upload patch set with your comments. On Fri, Aug 24, 2012 at 11:20 AM, Daniel Vetter 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 > > --- > > 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 */ > > +
[PATCH V3] drm: edid: add support for E-DDC
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 --- 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; + } +
[PATCH V3] drm: edid: add support for E-DDC
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(-)
Re: [PATCH V3] drm: edid: add support for E-DDC
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 > --- > 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, > +
[PATCH V3] drm: edid: add support for E-DDC
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 --- 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; + } +
[PATCH V3] drm: edid: add support for E-DDC
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(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel