[PATCH v3] DRM/KMS/EDID: Consolidate EDID Error Handling (v3)
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Nov 22 08:09:47 PST 2012
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 <eich at 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 's/!drm_edid_block_valid/drm_edid_block_check_error'
change seems superfluous since you're not using the more detailed return
value from drm_edid_block_check_error().
> 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 (*(raw_edid + i) != 0)
> - return false;
> - return true;
> -}
> -
> static void
> fix_map(u8 *block, int cnt)
> {
> @@ -456,37 +472,40 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> {
> int i, j = 0, valid_extensions = 0;
> u8 *block, *new;
> - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> + int last_error_flags = (drm_debug & DRM_UT_KMS) ? 0 : connector->last_edid_error_flags;
> + unsigned result = EDID_ERR_NO_DATA;
>
> #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> /* check if the user has specified a 'firmware' EDID file */
> block = (u8 *)drm_load_edid_firmware(connector);
> if (block) {
> drm_cache_edid(connector, NULL);
> + connector->last_edid_error_flags = 0;
> return block;
> }
> #endif
> -
> - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> + block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> + if (block == NULL) {
> + result = EDID_ERR_NO_MEM;
> goto error;
> + }
>
> /* base block fetch */
> for (i = 0; i < 4; i++) {
> if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> goto error_free;
> - if (drm_edid_block_valid(block, 0, print_bad_edid))
> + result = drm_edid_block_check_error(block, 0, last_error_flags);
> + if (!result)
> break;
> - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) {
> - connector->null_edid_counter++;
> + if (i == 0 && result & EDID_ERR_NULL)
> goto error_carp;
> - }
> }
> if (i == 4)
> goto error_carp;
>
> /* if there are no extensions, we're done - don't bother caching */
> if (block[EDID_EXTENSION_FLAG_OFFSET] == 0)
> - goto done;
> + goto done_update_cache;
>
> /* don't expect extension blocks in EDID Versions < 1.3: return base block with correct extension flag */
> if (block[EDID_VERSION_MINOR_OFFSET] < 3)
> @@ -494,7 +513,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>
> /* see if EDID is in the cache - no need to read all extension blocks */
> if (compare_get_edid_from_cache(connector, (struct edid **)&block))
> - return block;
> + goto done;
>
> new = krealloc(block, (block[EDID_EXTENSION_FLAG_OFFSET] + 1) * EDID_LENGTH, GFP_KERNEL);
> if (!new) {
> @@ -512,7 +531,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> valid_extensions = 0;
> goto done_fix_extension_count;
> }
> - 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.
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list