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=8ee...