| Message ID | 1345800410-7191-2-git-send-email-s.shirish@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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)
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(-)