[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Ville Syrj?l? writes:
 > On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
 > > 
 > > Something similar should be done for drm_edid_is_valid() - even if the 
 > > driver doesn't bother (for instance because this function is only called
 > > once when the device structures are initialized).
 > > 
 > > The current code ignores the error state for extension blocks i guess it
 > > should not if we want to avoid having repreated logging of errors in the
 > > extension blocks.
 > 
 > I'm not sure. The current code dump all failed extension block, doesn't
 > it? Maybe we actually do want that to happen, at least w/ debugs
 > enabled.

Yes, it does. It drops them silently. I was a bit unclear - what I 
meant was: we ignore the result for the final error state we keep 
to be able to track which errors we still want to log.
 > 
 > Then there are various retry loops in the code, which may or may not be
 > necessary. I have a feeling some of them may have been attempts at
 > papering over a bug that I fixed [1] in the i2c bitbanging code. But if
 > they are necessary, I'm not sure we really appreciate repeated dumps of
 > the same block.
 > 
 > [1] 
 > https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

Right, I know this patch :)
I'm also not sure about the retries. I guess for the base block we want at
least one retry in case the display is in an intermediate state where it
will not send the base block.
Apart from this I've seen many EDID failures however I don't recall seeing
one which got fixed by repeated reads. However to see if those loops
fix any issue we may want to keep all messages enabled for the time being.

Cheers,
Egbert.

[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
> Ville Syrj?l? writes:
>  > > 
>  > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>  > > index dd0df60..aa9b34d 100644
>  > > --- a/drivers/gpu/drm/drm_edid.c
>  > > +++ b/drivers/gpu/drm/drm_edid.c
>  > > @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>  > >  }
>  > >  EXPORT_SYMBOL(drm_edid_header_is_valid);
>  > >  
>  > > +static bool drm_edid_is_zero(u8 *in_edid, int length)
>  > > +{
>  > > +int i;
>  > > +u32 *raw_edid = (u32 *)in_edid;
>  > > +
>  > > +for (i = 0; i < length / 4; i++)
>  > > +if (*(raw_edid + i) != 0)
>  > > +return false;
>  > > +return true;
> 
> [...]
>  > 
>  > You could use memchr_inv() here. But the compiler can't optimize it
>  > since it's not inline, so I suppose it might make it slower.
> 
> 
>  > > +
>  > > +bool
>  > > +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
>  > > +{
>  > > +if (!drm_edid_block_check_error(raw_edid, block, 
> last_error_flags))
>  > > +return true;
>  > > +return false;
>  > 
>  > return !drm_edid_block_check_error();
> 
> Right. 
> It's stupid anyway. See below.
> 
>  > 
>  > >  }
>  > >  EXPORT_SYMBOL(drm_edid_block_valid);
>  > >  
>  > > @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
>  > >  return false;
>  > >  
>  > >  for (i = 0; i <= edid->extensions; i++)
>  > > -if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, 
> true))
>  > > +if (drm_edid_block_check_error(raw + i * EDID_LENGTH, 
> i, true))
>  >  
> 
>  > 
>  > That looks wrong. Also the 
> 's/!drm_edid_block_valid/drm_edid_block_check_error'
> 
> Oops, right. Leftover from old code.
> 
>  > change seems superfluous since you're not using the more detailed return
>  > value from drm_edid_block_check_error().
> 
> This should probably be changed. 
> For the drm_edid_block_valid() I'm already using the previous error state
> as argument - but I don't tell the result of this read. 
> Doesn't make much sense :(
> 
> Something similar should be done for drm_edid_is_valid() - even if the 
> driver doesn't bother (for instance because this function is only called
> once when the device structures are initialized).
> 
> The current code ignores the error state for extension blocks i guess it
> should not if we want to avoid having repreated logging of errors in the
> extension blocks.

I'm not sure. The current code dump all failed extension block, doesn't
it? Maybe we actually do want that to happen, at least w/ debugs
enabled.

Then there are various retry loops in the code, which may or may not be
necessary. I have a feeling some of them may have been attempts at
papering over a bug that I fixed [1] in the i2c bitbanging code. But if
they are necessary, I'm not sure we really appreciate repeated dumps of
the same block.

[1] 
https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

-- 
Ville Syrj??l??
Intel OTC

[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Ville Syrj?l? writes:
 > > 
 > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 > > index dd0df60..aa9b34d 100644
 > > --- a/drivers/gpu/drm/drm_edid.c
 > > +++ b/drivers/gpu/drm/drm_edid.c
 > > @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 > >  }
 > >  EXPORT_SYMBOL(drm_edid_header_is_valid);
 > >  
 > > +static bool drm_edid_is_zero(u8 *in_edid, int length)
 > > +{
 > > +  int i;
 > > +  u32 *raw_edid = (u32 *)in_edid;
 > > +
 > > +  for (i = 0; i < length / 4; i++)
 > > +  if (*(raw_edid + i) != 0)
 > > +  return false;
 > > +  return true;

[...]
 > 
 > You could use memchr_inv() here. But the compiler can't optimize it
 > since it's not inline, so I suppose it might make it slower.


 > > +
 > > +bool
 > > +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
 > > +{
 > > +  if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
 > > +  return true;
 > > +  return false;
 > 
 > return !drm_edid_block_check_error();

Right. 
It's stupid anyway. See below.

 > 
 > >  }
 > >  EXPORT_SYMBOL(drm_edid_block_valid);
 > >  
 > > @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
 > >return false;
 > >  
 > >for (i = 0; i <= edid->extensions; i++)
 > > -  if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
 > > +  if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 >  
 > 
 > That looks wrong. Also the 
 > 's/!drm_edid_block_valid/drm_edid_block_check_error'

Oops, right. Leftover from old code.

 > change seems superfluous since you're not using the more detailed return
 > value from drm_edid_block_check_error().

This should probably be changed. 
For the drm_edid_block_valid() I'm already using the previous error state
as argument - but I don't tell the result of this read. 
Doesn't make much sense :(

Something similar should be done for drm_edid_is_valid() - even if the 
driver doesn't bother (for instance because this function is only called
once when the device structures are initialized).

The current code ignores the error state for extension blocks i guess it
should not if we want to avoid having repreated logging of errors in the
extension blocks.
What should be done is:
unsigned err = drm_edid_block_check_error();
result |= err;
if (!err) {
   valid_extensions++;
   ...
}

 > > -  if (drm_edid_block_valid(block + (valid_extensions + 1) 
 > > * EDID_LENGTH, j, print_bad_edid)) {
 > > +  if (!drm_edid_block_check_error(block + 
 > > (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
 > 
 > Again the change of function seems superfluous.
 > 

Exactly. Please see above.

Thanks for looking at the patches!

Cheers,
Egbert.

[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 09:44:42AM -0500, Egbert Eich wrote:
> Consolidate the null_edid_counter and the bad_edid_counter
> into EDID error state flags which for the last EDID read
> are accessible from user.
> Errors are looged it the same error has not been present
> in the previous read of the EDID. This will reset the
> EDID error status for example when the monitor is changed
> but still prevents permanent EDID errors from piling up
> the the kernel logs.
> 
> v2: Fixed conflits due to reordering of commits.
> Set error state where missing.
> v3: Don't update cache when returning block from cache.
> 
> Signed-off-by: Egbert Eich 
> ---
>  drivers/gpu/drm/drm_edid.c |  117 
> +---
>  drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
>  include/drm/drm_crtc.h |4 +-
>  include/drm/drm_edid.h |   10 +++
>  4 files changed, 82 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index dd0df60..aa9b34d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);
>  
> +static bool drm_edid_is_zero(u8 *in_edid, int length)
> +{
> + int i;
> + u32 *raw_edid = (u32 *)in_edid;
> +
> + for (i = 0; i < length / 4; i++)
> + if (*(raw_edid + i) != 0)
> + return false;
> + return true;

You could use memchr_inv() here. But the compiler can't optimize it
since it's not inline, so I suppose it might make it slower.

> +}
> +
>  static int edid_fixup __read_mostly = 6;
>  module_param_named(edid_fixup, edid_fixup, int, 0400);
>  MODULE_PARM_DESC(edid_fixup,
> @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
>   * Sanity check the EDID block (base or extension).  Return 0 if the block
>   * doesn't check out, or 1 if it's valid.
>   */
> -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
> +unsigned
> +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned 
> last_error_flags)
>  {
>   int i;
>   u8 csum = 0;
>   struct edid *edid = (struct edid *)raw_edid;
> + unsigned result = 0;
>  
>   if (edid_fixup > 8 || edid_fixup < 0)
>   edid_fixup = 6;
> @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
> print_bad_edid)
>   DRM_DEBUG("Fixing EDID header, your hardware may be 
> failing\n");
>   memcpy(raw_edid, edid_header, sizeof(edid_header));
>   } else {
> - goto bad;
> + result |= EDID_ERR_NO_BLOCK0;
> + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
> + result |= EDID_ERR_NULL;
> + goto bad;
> + }
>   }
>   }
>  
>   for (i = 0; i < EDID_LENGTH; i++)
>   csum += raw_edid[i];
>   if (csum) {
> - if (print_bad_edid) {
> + if ((last_error_flags & EDID_ERR_CSUM) == 0)
>   DRM_ERROR("EDID checksum is invalid, remainder is 
> %d\n", csum);
> - }
>  
>   /* allow CEA to slide through, switches mangle this */
>   if (raw_edid[0] != 0x02)
> - goto bad;
> + result |= EDID_ERR_CSUM;
>   }
> + if (result)
> + goto bad;
>  
>   /* per-block-type checks */
>   switch (raw_edid[0]) {
>   case 0: /* base */
>   if (edid->version != 1) {
>   DRM_ERROR("EDID has major version %d, instead of 1\n", 
> edid->version);
> + result |= EDID_ERR_UNSUPPORTED_VERSION;
>   goto bad;
>   }
>  
> @@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
> print_bad_edid)
>   break;
>   }
>  
> - return 1;
> + return 0;
>  
>  bad:
> - if (raw_edid && print_bad_edid) {
> + if (raw_edid && last_error_flags != result) {
>   printk(KERN_ERR "Raw EDID:\n");
>   print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
>  raw_edid, EDID_LENGTH, false);
>   }
> - return 0;
> + return result;
> +}
> +
> +bool
> +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
> +{
> + if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
> + return true;
> + return false;

return !drm_edid_block_check_error();

>  }
>  EXPORT_SYMBOL(drm_edid_block_valid);
>  
> @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
>   return false;
>  
>   for (i = 0; i <= edid->extensions; i++)
> - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
> + if (drm_edid_block_check_error(raw + i * 

[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Consolidate the null_edid_counter and the bad_edid_counter
into EDID error state flags which for the last EDID read
are accessible from user.
Errors are looged it the same error has not been present
in the previous read of the EDID. This will reset the
EDID error status for example when the monitor is changed
but still prevents permanent EDID errors from piling up
the the kernel logs.

v2: Fixed conflits due to reordering of commits.
Set error state where missing.
v3: Don't update cache when returning block from cache.

Signed-off-by: Egbert Eich 
---
 drivers/gpu/drm/drm_edid.c |  117 +---
 drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
 include/drm/drm_crtc.h |4 +-
 include/drm/drm_edid.h |   10 +++
 4 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dd0df60..aa9b34d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);

+static bool drm_edid_is_zero(u8 *in_edid, int length)
+{
+   int i;
+   u32 *raw_edid = (u32 *)in_edid;
+
+   for (i = 0; i < length / 4; i++)
+   if (*(raw_edid + i) != 0)
+   return false;
+   return true;
+}
+
 static int edid_fixup __read_mostly = 6;
 module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
@@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
  * Sanity check the EDID block (base or extension).  Return 0 if the block
  * doesn't check out, or 1 if it's valid.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+unsigned
+drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags)
 {
int i;
u8 csum = 0;
struct edid *edid = (struct edid *)raw_edid;
+   unsigned result = 0;

if (edid_fixup > 8 || edid_fixup < 0)
edid_fixup = 6;
@@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
print_bad_edid)
DRM_DEBUG("Fixing EDID header, your hardware may be 
failing\n");
memcpy(raw_edid, edid_header, sizeof(edid_header));
} else {
-   goto bad;
+   result |= EDID_ERR_NO_BLOCK0;
+   if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
+   result |= EDID_ERR_NULL;
+   goto bad;
+   }
}
}

for (i = 0; i < EDID_LENGTH; i++)
csum += raw_edid[i];
if (csum) {
-   if (print_bad_edid) {
+   if ((last_error_flags & EDID_ERR_CSUM) == 0)
DRM_ERROR("EDID checksum is invalid, remainder is 
%d\n", csum);
-   }

/* allow CEA to slide through, switches mangle this */
if (raw_edid[0] != 0x02)
-   goto bad;
+   result |= EDID_ERR_CSUM;
}
+   if (result)
+   goto bad;

/* per-block-type checks */
switch (raw_edid[0]) {
case 0: /* base */
if (edid->version != 1) {
DRM_ERROR("EDID has major version %d, instead of 1\n", 
edid->version);
+   result |= EDID_ERR_UNSUPPORTED_VERSION;
goto bad;
}

@@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
print_bad_edid)
break;
}

-   return 1;
+   return 0;

 bad:
-   if (raw_edid && print_bad_edid) {
+   if (raw_edid && last_error_flags != result) {
printk(KERN_ERR "Raw EDID:\n");
print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1,
   raw_edid, EDID_LENGTH, false);
}
-   return 0;
+   return result;
+}
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+   if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
+   return true;
+   return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);

@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
return false;

for (i = 0; i <= edid->extensions; i++)
-   if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+   if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
return false;

return true;
@@ -310,17 +337,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, 
unsigned char *buf,
return ret == xfers ? 0 : -1;
 }

-static bool drm_edid_is_zero(u8 *in_edid, int length)
-{
-   int i;
-   u32 *raw_edid = (u32 *)in_edid;
-
-   for (i = 0; i < length / 4; i++)
-   if 

[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Consolidate the null_edid_counter and the bad_edid_counter
into EDID error state flags which for the last EDID read
are accessible from user.
Errors are looged it the same error has not been present
in the previous read of the EDID. This will reset the
EDID error status for example when the monitor is changed
but still prevents permanent EDID errors from piling up
the the kernel logs.

v2: Fixed conflits due to reordering of commits.
Set error state where missing.
v3: Don't update cache when returning block from cache.

Signed-off-by: Egbert Eich e...@suse.com
---
 drivers/gpu/drm/drm_edid.c |  117 +---
 drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
 include/drm/drm_crtc.h |4 +-
 include/drm/drm_edid.h |   10 +++
 4 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dd0df60..aa9b34d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
 
+static bool drm_edid_is_zero(u8 *in_edid, int length)
+{
+   int i;
+   u32 *raw_edid = (u32 *)in_edid;
+
+   for (i = 0; i  length / 4; i++)
+   if (*(raw_edid + i) != 0)
+   return false;
+   return true;
+}
+
 static int edid_fixup __read_mostly = 6;
 module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
@@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
  * Sanity check the EDID block (base or extension).  Return 0 if the block
  * doesn't check out, or 1 if it's valid.
  */
-bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
+unsigned
+drm_edid_block_check_error(u8 *raw_edid, int block, unsigned last_error_flags)
 {
int i;
u8 csum = 0;
struct edid *edid = (struct edid *)raw_edid;
+   unsigned result = 0;
 
if (edid_fixup  8 || edid_fixup  0)
edid_fixup = 6;
@@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
print_bad_edid)
DRM_DEBUG(Fixing EDID header, your hardware may be 
failing\n);
memcpy(raw_edid, edid_header, sizeof(edid_header));
} else {
-   goto bad;
+   result |= EDID_ERR_NO_BLOCK0;
+   if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
+   result |= EDID_ERR_NULL;
+   goto bad;
+   }
}
}
 
for (i = 0; i  EDID_LENGTH; i++)
csum += raw_edid[i];
if (csum) {
-   if (print_bad_edid) {
+   if ((last_error_flags  EDID_ERR_CSUM) == 0)
DRM_ERROR(EDID checksum is invalid, remainder is 
%d\n, csum);
-   }
 
/* allow CEA to slide through, switches mangle this */
if (raw_edid[0] != 0x02)
-   goto bad;
+   result |= EDID_ERR_CSUM;
}
+   if (result)
+   goto bad;
 
/* per-block-type checks */
switch (raw_edid[0]) {
case 0: /* base */
if (edid-version != 1) {
DRM_ERROR(EDID has major version %d, instead of 1\n, 
edid-version);
+   result |= EDID_ERR_UNSUPPORTED_VERSION;
goto bad;
}
 
@@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
print_bad_edid)
break;
}
 
-   return 1;
+   return 0;
 
 bad:
-   if (raw_edid  print_bad_edid) {
+   if (raw_edid  last_error_flags != result) {
printk(KERN_ERR Raw EDID:\n);
print_hex_dump(KERN_ERR,  \t, DUMP_PREFIX_NONE, 16, 1,
   raw_edid, EDID_LENGTH, false);
}
-   return 0;
+   return result;
+}
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+   if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
+   return true;
+   return false;
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
return false;
 
for (i = 0; i = edid-extensions; i++)
-   if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
+   if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
return false;
 
return true;
@@ -310,17 +337,6 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, 
unsigned char *buf,
return ret == xfers ? 0 : -1;
 }
 
-static bool drm_edid_is_zero(u8 *in_edid, int length)
-{
-   int i;
-   u32 *raw_edid = (u32 *)in_edid;
-
-   for (i = 0; i  length / 4; i++)
-   if 

Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 09:44:42AM -0500, Egbert Eich wrote:
 Consolidate the null_edid_counter and the bad_edid_counter
 into EDID error state flags which for the last EDID read
 are accessible from user.
 Errors are looged it the same error has not been present
 in the previous read of the EDID. This will reset the
 EDID error status for example when the monitor is changed
 but still prevents permanent EDID errors from piling up
 the the kernel logs.
 
 v2: Fixed conflits due to reordering of commits.
 Set error state where missing.
 v3: Don't update cache when returning block from cache.
 
 Signed-off-by: Egbert Eich e...@suse.com
 ---
  drivers/gpu/drm/drm_edid.c |  117 
 +---
  drivers/gpu/drm/radeon/radeon_connectors.c |2 +-
  include/drm/drm_crtc.h |4 +-
  include/drm/drm_edid.h |   10 +++
  4 files changed, 82 insertions(+), 51 deletions(-)
 
 diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 index dd0df60..aa9b34d 100644
 --- a/drivers/gpu/drm/drm_edid.c
 +++ b/drivers/gpu/drm/drm_edid.c
 @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
  }
  EXPORT_SYMBOL(drm_edid_header_is_valid);
  
 +static bool drm_edid_is_zero(u8 *in_edid, int length)
 +{
 + int i;
 + u32 *raw_edid = (u32 *)in_edid;
 +
 + for (i = 0; i  length / 4; i++)
 + if (*(raw_edid + i) != 0)
 + return false;
 + return true;

You could use memchr_inv() here. But the compiler can't optimize it
since it's not inline, so I suppose it might make it slower.

 +}
 +
  static int edid_fixup __read_mostly = 6;
  module_param_named(edid_fixup, edid_fixup, int, 0400);
  MODULE_PARM_DESC(edid_fixup,
 @@ -166,11 +177,13 @@ MODULE_PARM_DESC(edid_fixup,
   * Sanity check the EDID block (base or extension).  Return 0 if the block
   * doesn't check out, or 1 if it's valid.
   */
 -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid)
 +unsigned
 +drm_edid_block_check_error(u8 *raw_edid, int block, unsigned 
 last_error_flags)
  {
   int i;
   u8 csum = 0;
   struct edid *edid = (struct edid *)raw_edid;
 + unsigned result = 0;
  
   if (edid_fixup  8 || edid_fixup  0)
   edid_fixup = 6;
 @@ -182,27 +195,33 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
 print_bad_edid)
   DRM_DEBUG(Fixing EDID header, your hardware may be 
 failing\n);
   memcpy(raw_edid, edid_header, sizeof(edid_header));
   } else {
 - goto bad;
 + result |= EDID_ERR_NO_BLOCK0;
 + if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) {
 + result |= EDID_ERR_NULL;
 + goto bad;
 + }
   }
   }
  
   for (i = 0; i  EDID_LENGTH; i++)
   csum += raw_edid[i];
   if (csum) {
 - if (print_bad_edid) {
 + if ((last_error_flags  EDID_ERR_CSUM) == 0)
   DRM_ERROR(EDID checksum is invalid, remainder is 
 %d\n, csum);
 - }
  
   /* allow CEA to slide through, switches mangle this */
   if (raw_edid[0] != 0x02)
 - goto bad;
 + result |= EDID_ERR_CSUM;
   }
 + if (result)
 + goto bad;
  
   /* per-block-type checks */
   switch (raw_edid[0]) {
   case 0: /* base */
   if (edid-version != 1) {
   DRM_ERROR(EDID has major version %d, instead of 1\n, 
 edid-version);
 + result |= EDID_ERR_UNSUPPORTED_VERSION;
   goto bad;
   }
  
 @@ -214,15 +233,23 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool 
 print_bad_edid)
   break;
   }
  
 - return 1;
 + return 0;
  
  bad:
 - if (raw_edid  print_bad_edid) {
 + if (raw_edid  last_error_flags != result) {
   printk(KERN_ERR Raw EDID:\n);
   print_hex_dump(KERN_ERR,  \t, DUMP_PREFIX_NONE, 16, 1,
  raw_edid, EDID_LENGTH, false);
   }
 - return 0;
 + return result;
 +}
 +
 +bool
 +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
 +{
 + if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
 + return true;
 + return false;

return !drm_edid_block_check_error();

  }
  EXPORT_SYMBOL(drm_edid_block_valid);
  
 @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
   return false;
  
   for (i = 0; i = edid-extensions; i++)
 - if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
 + if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
 

That looks wrong. Also the 

Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Ville Syrj?l? writes:
   
   diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
   index dd0df60..aa9b34d 100644
   --- a/drivers/gpu/drm/drm_edid.c
   +++ b/drivers/gpu/drm/drm_edid.c
   @@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
}
EXPORT_SYMBOL(drm_edid_header_is_valid);

   +static bool drm_edid_is_zero(u8 *in_edid, int length)
   +{
   +  int i;
   +  u32 *raw_edid = (u32 *)in_edid;
   +
   +  for (i = 0; i  length / 4; i++)
   +  if (*(raw_edid + i) != 0)
   +  return false;
   +  return true;

[...]
  
  You could use memchr_inv() here. But the compiler can't optimize it
  since it's not inline, so I suppose it might make it slower.


   +
   +bool
   +drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
   +{
   +  if (!drm_edid_block_check_error(raw_edid, block, last_error_flags))
   +  return true;
   +  return false;
  
  return !drm_edid_block_check_error();

Right. 
It's stupid anyway. See below.

  
}
EXPORT_SYMBOL(drm_edid_block_valid);

   @@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
  return false;

  for (i = 0; i = edid-extensions; i++)
   -  if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, true))
   +  if (drm_edid_block_check_error(raw + i * EDID_LENGTH, i, true))
   
  
  That looks wrong. Also the 
  's/!drm_edid_block_valid/drm_edid_block_check_error'

Oops, right. Leftover from old code.

  change seems superfluous since you're not using the more detailed return
  value from drm_edid_block_check_error().

This should probably be changed. 
For the drm_edid_block_valid() I'm already using the previous error state
as argument - but I don't tell the result of this read. 
Doesn't make much sense :(

Something similar should be done for drm_edid_is_valid() - even if the 
driver doesn't bother (for instance because this function is only called
once when the device structures are initialized).

The current code ignores the error state for extension blocks i guess it
should not if we want to avoid having repreated logging of errors in the
extension blocks.
What should be done is:
unsigned err = drm_edid_block_check_error();
result |= err;
if (!err) {
   valid_extensions++;
   ...
}

   -  if (drm_edid_block_valid(block + (valid_extensions + 1) 
   * EDID_LENGTH, j, print_bad_edid)) {
   +  if (!drm_edid_block_check_error(block + 
   (valid_extensions + 1) * EDID_LENGTH, j, last_error_flags)) {
  
  Again the change of function seems superfluous.
  

Exactly. Please see above.

Thanks for looking at the patches!

Cheers,
Egbert.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Ville Syrjälä]

On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
 Ville Syrj�l� writes:

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dd0df60..aa9b34d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -157,6 +157,17 @@ int drm_edid_header_is_valid(const u8 *raw_edid)
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
 
+static bool drm_edid_is_zero(u8 *in_edid, int length)
+{
+int i;
+u32 *raw_edid = (u32 *)in_edid;
+
+for (i = 0; i  length / 4; i++)
+if (*(raw_edid + i) != 0)
+return false;
+return true;
 
 [...]
   
   You could use memchr_inv() here. But the compiler can't optimize it
   since it's not inline, so I suppose it might make it slower.
 
 
+
+bool
+drm_edid_block_valid(u8 *raw_edid, int block, unsigned last_error_flags)
+{
+if (!drm_edid_block_check_error(raw_edid, block, 
 last_error_flags))
+return true;
+return false;
   
   return !drm_edid_block_check_error();
 
 Right. 
 It's stupid anyway. See below.
 
   
 }
 EXPORT_SYMBOL(drm_edid_block_valid);
 
@@ -241,7 +268,7 @@ bool drm_edid_is_valid(struct edid *edid)
 return false;
 
 for (i = 0; i = edid-extensions; i++)
-if (!drm_edid_block_valid(raw + i * EDID_LENGTH, i, 
 true))
+if (drm_edid_block_check_error(raw + i * EDID_LENGTH, 
 i, true))

 
   
   That looks wrong. Also the 
 's/!drm_edid_block_valid/drm_edid_block_check_error'
 
 Oops, right. Leftover from old code.
 
   change seems superfluous since you're not using the more detailed return
   value from drm_edid_block_check_error().
 
 This should probably be changed. 
 For the drm_edid_block_valid() I'm already using the previous error state
 as argument - but I don't tell the result of this read. 
 Doesn't make much sense :(
 
 Something similar should be done for drm_edid_is_valid() - even if the 
 driver doesn't bother (for instance because this function is only called
 once when the device structures are initialized).
 
 The current code ignores the error state for extension blocks i guess it
 should not if we want to avoid having repreated logging of errors in the
 extension blocks.

I'm not sure. The current code dump all failed extension block, doesn't
it? Maybe we actually do want that to happen, at least w/ debugs
enabled.

Then there are various retry loops in the code, which may or may not be
necessary. I have a feeling some of them may have been attempts at
papering over a bug that I fixed [1] in the i2c bitbanging code. But if
they are necessary, I'm not sure we really appreciate repeated dumps of
the same block.

[1] 
https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)

[Egbert Eich]

Ville Syrj?l? writes:
  On Thu, Nov 22, 2012 at 07:28:44PM +0100, Egbert Eich wrote:
   
   Something similar should be done for drm_edid_is_valid() - even if the 
   driver doesn't bother (for instance because this function is only called
   once when the device structures are initialized).
   
   The current code ignores the error state for extension blocks i guess it
   should not if we want to avoid having repreated logging of errors in the
   extension blocks.
  
  I'm not sure. The current code dump all failed extension block, doesn't
  it? Maybe we actually do want that to happen, at least w/ debugs
  enabled.

Yes, it does. It drops them silently. I was a bit unclear - what I 
meant was: we ignore the result for the final error state we keep 
to be able to track which errors we still want to log.
  
  Then there are various retry loops in the code, which may or may not be
  necessary. I have a feeling some of them may have been attempts at
  papering over a bug that I fixed [1] in the i2c bitbanging code. But if
  they are necessary, I'm not sure we really appreciate repeated dumps of
  the same block.
  
  [1] 
  https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=8ee161ce5e0cfc689eb677f227a6248191165fac

Right, I know this patch :)
I'm also not sure about the retries. I guess for the base block we want at
least one retry in case the display is in an intermediate state where it
will not send the base block.
Apart from this I've seen many EDID failures however I don't recall seeing
one which got fixed by repeated reads. However to see if those loops
fix any issue we may want to keep all messages enabled for the time being.

Cheers,
Egbert.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel