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 V3: Remove switch,and avoid sending of segment data for non E-DDC
Based on drm-next branch
Shirish S (1): drm: edid: add support for E-DDC
drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
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 | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; int ret, retries = 5;
/* The core i2c driver will automatically retry the transfer if the @@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = { - { + { /*set segment pointer */ + .addr = DDC_SEGMENT_ADDR, + .flags = segment ? 0 : I2C_M_IGNORE_NAK, + .len = 1, + .buf = &segment, + }, { .addr = DDC_ADDR, .flags = 0, .len = 1, @@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } }; - ret = i2c_transfer(adapter, msgs, 2); + /* Avoid sending the segment addr to not upset non-compliant ddc + * monitors. + */ + if (!segment) + ret = i2c_transfer(adapter, &msgs[1], xfers); + else + ret = i2c_transfer(adapter, msgs, xfers); + if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; } - } while (ret != 2 && --retries); + } while (ret != xfers && --retries);
- return ret == 2 ? 0 : -1; + return ret == xfers ? 0 : -1; }
static bool drm_edid_is_zero(u8 *in_edid, int length)
On Sat, Aug 25, 2012 at 03:13:56PM +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
Looks good.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_edid.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; int ret, retries = 5;
/* The core i2c driver will automatically retry the transfer if the
@@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = {
{
{ /*set segment pointer */.addr = DDC_SEGMENT_ADDR,.flags = segment ? 0 : I2C_M_IGNORE_NAK,.len = 1,.buf = &segment,}, { .addr = DDC_ADDR, .flags = 0, .len = 1,@@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } };
ret = i2c_transfer(adapter, msgs, 2);
- /* Avoid sending the segment addr to not upset non-compliant ddc
* monitors.*/if (!segment)ret = i2c_transfer(adapter, &msgs[1], xfers);elseret = i2c_transfer(adapter, msgs, xfers);- if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; }
- } while (ret != 2 && --retries);
- } while (ret != xfers && --retries);
- return ret == 2 ? 0 : -1;
- return ret == xfers ? 0 : -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
On Sat, Aug 25, 2012 at 03:13:56PM +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 | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; int ret, retries = 5;
/* The core i2c driver will automatically retry the transfer if the
@@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = {
{
{ /*set segment pointer */
Missing whitespace after '/*'. Perhaps just drop the comment. I don't see much value in it.
.addr = DDC_SEGMENT_ADDR,.flags = segment ? 0 : I2C_M_IGNORE_NAK,.len = 1,.buf = &segment,}, { .addr = DDC_ADDR, .flags = 0, .len = 1,@@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } };
ret = i2c_transfer(adapter, msgs, 2);
- /* Avoid sending the segment addr to not upset non-compliant ddc
* monitors.*/
Wrong indentation and comment style is wrong. I'm guessing this didn't go through checkpatch.pl.
Otherwise looks OK to me.
On Wed, Aug 29, 2012 at 4:08 AM, Ville Syrjälä < ville.syrjala@linux.intel.com> wrote:
On Sat, Aug 25, 2012 at 03:13:56PM +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 | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a8743c3..cde7af0 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 char xfers = segment ? 3 : 2; int ret, retries = 5; /* The core i2c driver will automatically retry the transfer if the@@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
unsigned char *buf,
*/ do { struct i2c_msg msgs[] = {
{
{ /*set segment pointer */Missing whitespace after '/*'. Perhaps just drop the comment. I don't see much value in it.
Done.
.addr = DDC_SEGMENT_ADDR,.flags = segment ? 0 : I2C_M_IGNORE_NAK,.len = 1,.buf = &segment,}, { .addr = DDC_ADDR, .flags = 0, .len = 1,@@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter,
unsigned char *buf,
.buf = buf, } };
ret = i2c_transfer(adapter, msgs, 2);
/* Avoid sending the segment addr to not upset non-compliant ddc* monitors.*/Wrong indentation and comment style is wrong. I'm guessing this didn't go through checkpatch.pl.
I never got tha above as either an error or warning in the checkpatch,
anyways have uploaded patch set 5 incorporating your comments.
Otherwise looks OK to me.
-- Ville Syrjälä Intel OTC
- Shirish
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi all,
Sorry for breaking message threading but I was not included in iterations 3 and 4 of this patch.
Random comments about v4:
--- 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 char xfers = segment ? 3 : 2; int ret, retries = 5; /* The core i2c driver will automatically retry the transfer if the@@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = {
{
{ /*set segment pointer */.addr = DDC_SEGMENT_ADDR,.flags = segment ? 0 : I2C_M_IGNORE_NAK,
I don't get the idea. If segment == 0, this message is never sent, so the value of field flags doesn't matter. So flags will always be 0 when this message is sent, so it can be hard-coded.
But from previous discussions my understanding was an agreement on always using I2C_M_IGNORE_NAK for improved compatibility. So I2C_M_IGNORE_NAK should be hard-coded, not 0?
.len = 1,.buf = &segment,}, { .addr = DDC_ADDR, .flags = 0, .len = 1,@@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } };
ret = i2c_transfer(adapter, msgs, 2);
/* Avoid sending the segment addr to not upset non-compliant ddc* monitors.*/
s/segment addr/segment/, plus it's abot E-DCC compliance as I understand it, not DDC.
if (!segment)ret = i2c_transfer(adapter, &msgs[1], xfers);elseret = i2c_transfer(adapter, msgs, xfers);
This can be written:
ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);
Which is more compact and, I suspect, faster.
if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; }
} while (ret != 2 && --retries);
} while (ret != xfers && --retries);
return ret == 2 ? 0 : -1;
return ret == xfers ? 0 : -1;}
static bool drm_edid_is_zero(u8 *in_edid, int length)
Other than this, your code looks reasonable, not so different from what I submitted 8 months ago actually. But ISTU you can test the code with real hardware while I couldn't.
With the changes above applied, you can add:
Reviewed-by: Jean Delvare jdelvare@suse.de
Hi Jean,
On Wed, Aug 29, 2012 at 6:44 AM, Jean Delvare jdelvare@suse.de wrote:
Hi all,
Sorry for breaking message threading but I was not included in iterations 3 and 4 of this patch.
Random comments about v4:
--- 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 char xfers = segment ? 3 : 2; int ret, retries = 5; /* The core i2c driver will automatically retry the transfer ifthe
@@ -264,7 +266,12 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, */ do { struct i2c_msg msgs[] = {
{
{ /*set segment pointer */.addr = DDC_SEGMENT_ADDR,.flags = segment ? 0 : I2C_M_IGNORE_NAK,I don't get the idea. If segment == 0, this message is never sent, so the value of field flags doesn't matter. So flags will always be 0 when this message is sent, so it can be hard-coded.
Agreed.
But from previous discussions my understanding was an agreement on always using I2C_M_IGNORE_NAK for improved compatibility. So I2C_M_IGNORE_NAK should be hard-coded, not 0?
After discussion,daniel had asked for a seprate patch for the flags modification. Will upload that later.
.len = 1,
.buf = &segment,}, { .addr = DDC_ADDR, .flags = 0, .len = 1,@@ -276,15 +283,22 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, .buf = buf, } };
ret = i2c_transfer(adapter, msgs, 2);
/* Avoid sending the segment addr to not upset non-compliant ddc* monitors.*/s/segment addr/segment/, plus it's abot E-DCC compliance as I understand it, not DDC.
if (!segment)ret = i2c_transfer(adapter, &msgs[1], xfers);elseret = i2c_transfer(adapter, msgs, xfers);This can be written:
ret = i2c_transfer(adapter, &msgs[3 - xfers], xfers);Which is more compact and, I suspect, faster.
Agreed.
if (ret == -ENXIO) { DRM_DEBUG_KMS("drm: skipping non-existentadapter %s\n",
adapter->name); break; }
} while (ret != 2 && --retries);
} while (ret != xfers && --retries);
return ret == 2 ? 0 : -1;
return ret == xfers ? 0 : -1;}
static bool drm_edid_is_zero(u8 *in_edid, int length)
Other than this, your code looks reasonable, not so different from what I submitted 8 months ago actually. But ISTU you can test the code with real hardware while I couldn't.
Your patch never checked for the 3 message transfer complete, it checked
only 2.
With the changes above applied, you can add:
Reviewed-by: Jean Delvare jdelvare@suse.de
Will add your review comments in patch set 5 and your reviewed tag.
Thanks & Regards, Shirish S
-- Jean Delvare Suse L3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jean Delvare -
Shirish S -
Shirish S -
Ville Syrjälä