v2 of https://patchwork.freedesktop.org/series/101931/
Rebased, review comments addressed.
BR, Jani.
Jani Nikula (12): drm/edid: use struct edid * in drm_do_get_edid() drm/edid: clean up EDID block checksum functions drm/edid: add edid_block_tag() helper to get the EDID extension tag drm/edid: make drm_edid_header_is_valid() accept void pointer drm/edid: clean up edid_is_zero() drm/edid: split out edid_header_fix() drm/edid: split drm_edid_block_valid() to check and act parts drm/edid: use a better variable name for EDID block read retries drm/edid: simplify block check when filtering invalid blocks drm/edid: split out invalid block filtering to a separate function drm/edid: track invalid blocks in drm_do_get_edid() drm/edid: reduce magic when updating the EDID block checksum
drivers/gpu/drm/drm_edid.c | 295 +++++++++++++++++++++---------------- include/drm/drm_edid.h | 2 +- 2 files changed, 173 insertions(+), 124 deletions(-)
Mixing u8 * and struct edid * is confusing, switch to the latter.
v2: - Rebase on the invalid block filtering fix - Rename struct edid *base to *dest_block for clarity (Ville)
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 8829120470ab..5c7065561d4c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, void *data) { int i, j = 0, valid_extensions = 0; - u8 *edid, *new; - struct edid *override; + struct edid *edid, *new, *override;
override = drm_get_override_edid(connector); if (override) return override;
- edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block, data); + edid = drm_do_get_edid_base_block(connector, get_edid_block, data); if (!edid) return NULL;
/* if there's no extensions or no connector, we're done */ - valid_extensions = edid[0x7e]; + valid_extensions = edid->extensions; if (valid_extensions == 0) - return (struct edid *)edid; + return edid;
new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; edid = new;
- for (j = 1; j <= edid[0x7e]; j++) { - u8 *block = edid + j * EDID_LENGTH; + for (j = 1; j <= edid->extensions; j++) { + void *block = edid + j;
for (i = 0; i < 4; i++) { if (get_edid_block(data, block, j, EDID_LENGTH)) @@ -2026,35 +2025,35 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, valid_extensions--; }
- if (valid_extensions != edid[0x7e]) { - u8 *base; + if (valid_extensions != edid->extensions) { + struct edid *dest_block;
- connector_bad_edid(connector, edid, edid[0x7e] + 1); + connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL); if (!new) goto out;
- base = new; - for (i = 0; i <= edid[0x7e]; i++) { - u8 *block = edid + i * EDID_LENGTH; + dest_block = new; + for (i = 0; i <= edid->extensions; i++) { + void *block = edid + i;
if (!drm_edid_block_valid(block, i, false, NULL)) continue;
- memcpy(base, block, EDID_LENGTH); - base += EDID_LENGTH; + memcpy(dest_block, block, EDID_LENGTH); + dest_block++; }
- new[EDID_LENGTH - 1] += new[0x7e] - valid_extensions; - new[0x7e] = valid_extensions; + new->checksum += new->extensions - valid_extensions; + new->extensions = valid_extensions;
kfree(edid); edid = new; }
- return (struct edid *)edid; + return edid;
out: kfree(edid);
Have two clear functions, one to compute the checksum over the EDID, and another to get the checksum from the EDID. Throw away the diff function.
Ditch the drm_ prefix for static functions, and accept const void * to help transition to struct edid * usage.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 5c7065561d4c..82e00650af14 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1597,25 +1597,25 @@ module_param_named(edid_fixup, edid_fixup, int, 0400); MODULE_PARM_DESC(edid_fixup, "Minimum number of valid EDID header bytes (0-8, default 6)");
-static int drm_edid_block_checksum(const u8 *raw_edid) +static int edid_block_compute_checksum(const void *_block) { + const u8 *block = _block; int i; u8 csum = 0, crc = 0;
for (i = 0; i < EDID_LENGTH - 1; i++) - csum += raw_edid[i]; + csum += block[i];
crc = 0x100 - csum;
return crc; }
-static bool drm_edid_block_checksum_diff(const u8 *raw_edid, u8 real_checksum) +static int edid_block_get_checksum(const void *_block) { - if (raw_edid[EDID_LENGTH - 1] != real_checksum) - return true; - else - return false; + const struct edid *block = _block; + + return block->checksum; }
static bool drm_edid_is_zero(const u8 *in_edid, int length) @@ -1704,8 +1704,8 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, } }
- csum = drm_edid_block_checksum(raw_edid); - if (drm_edid_block_checksum_diff(raw_edid, csum)) { + csum = edid_block_compute_checksum(raw_edid); + if (csum != edid_block_get_checksum(raw_edid)) { if (edid_corrupt) *edid_corrupt = true;
@@ -1859,7 +1859,7 @@ static void connector_bad_edid(struct drm_connector *connector, /* Calculate real checksum for the last edid extension block data */ if (last_block < num_blocks) connector->real_edid_checksum = - drm_edid_block_checksum(edid + last_block * EDID_LENGTH); + edid_block_compute_checksum(edid + last_block * EDID_LENGTH);
if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS)) return;
The extension tag at offset 0 is not present in struct edid, add a helper for it to reduce the need to use u8 *.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 82e00650af14..7c9ce5b0bd5b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1618,6 +1618,13 @@ static int edid_block_get_checksum(const void *_block) return block->checksum; }
+static int edid_block_tag(const void *_block) +{ + const u8 *block = _block; + + return block[0]; +} + static bool drm_edid_is_zero(const u8 *in_edid, int length) { if (memchr_inv(in_edid, 0, length)) @@ -1710,7 +1717,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, *edid_corrupt = true;
/* allow CEA to slide through, switches mangle this */ - if (raw_edid[0] == CEA_EXT) { + if (edid_block_tag(raw_edid) == CEA_EXT) { DRM_DEBUG("EDID checksum is invalid, remainder is %d\n", csum); DRM_DEBUG("Assuming a KVM switch modified the CEA block but left the original checksum\n"); } else { @@ -1722,7 +1729,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, }
/* per-block-type checks */ - switch (raw_edid[0]) { + switch (edid_block_tag(raw_edid)) { case 0: /* base */ if (edid->version != 1) { DRM_NOTE("EDID has major version %d, instead of 1\n", edid->version); @@ -3366,7 +3373,7 @@ const u8 *drm_find_edid_extension(const struct edid *edid, /* Find CEA extension */ for (i = *ext_index; i < edid->extensions; i++) { edid_ext = (const u8 *)edid + EDID_LENGTH * (i + 1); - if (edid_ext[0] == ext_id) + if (edid_block_tag(edid_ext) == ext_id) break; }
It will be useful to accept a struct edid *, but for compatibility with existing usage accept void *.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 8 +++++--- include/drm/drm_edid.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7c9ce5b0bd5b..92e51f502fde 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1580,13 +1580,15 @@ static const u8 edid_header[] = { * * Return: 8 if the header is perfect, down to 0 if it's totally wrong. */ -int drm_edid_header_is_valid(const u8 *raw_edid) +int drm_edid_header_is_valid(const void *_edid) { + const struct edid *edid = _edid; int i, score = 0;
- for (i = 0; i < sizeof(edid_header); i++) - if (raw_edid[i] == edid_header[i]) + for (i = 0; i < sizeof(edid_header); i++) { + if (edid->header[i] == edid_header[i]) score++; + }
return score; } diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 48b1bf9c315a..b7e170584000 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -578,7 +578,7 @@ int drm_add_modes_noedid(struct drm_connector *connector, void drm_set_preferred_mode(struct drm_connector *connector, int hpref, int vpref);
-int drm_edid_header_is_valid(const u8 *raw_edid); +int drm_edid_header_is_valid(const void *edid); bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, bool *edid_corrupt); bool drm_edid_is_valid(struct edid *edid);
Simplify, rename, take void pointer. No need for the drm_ prefix for internal helpers.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 92e51f502fde..081aff2d1068 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1627,12 +1627,9 @@ static int edid_block_tag(const void *_block) return block[0]; }
-static bool drm_edid_is_zero(const u8 *in_edid, int length) +static bool edid_is_zero(const void *edid, int length) { - if (memchr_inv(in_edid, 0, length)) - return false; - - return true; + return !memchr_inv(edid, 0, length); }
/** @@ -1750,7 +1747,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
bad: if (print_bad_edid) { - if (drm_edid_is_zero(raw_edid, EDID_LENGTH)) { + if (edid_is_zero(raw_edid, EDID_LENGTH)) { pr_notice("EDID block is all zeroes\n"); } else { pr_notice("Raw EDID:\n"); @@ -1878,7 +1875,7 @@ static void connector_bad_edid(struct drm_connector *connector, u8 *block = edid + i * EDID_LENGTH; char prefix[20];
- if (drm_edid_is_zero(block, EDID_LENGTH)) + if (edid_is_zero(block, EDID_LENGTH)) sprintf(prefix, "\t[%02x] ZERO ", i); else if (!drm_edid_block_valid(block, i, false, NULL)) sprintf(prefix, "\t[%02x] BAD ", i); @@ -1955,7 +1952,7 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector, goto out; if (drm_edid_block_valid(edid, 0, false, edid_corrupt)) break; - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { + if (i == 0 && edid_is_zero(edid, EDID_LENGTH)) { if (null_edid_counter) (*null_edid_counter)++; goto carp;
Give a name to the EDID header fixup instead of having an inline memcpy.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 081aff2d1068..f8169ffd062d 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1572,6 +1572,11 @@ static const u8 edid_header[] = { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00 };
+static void edid_header_fix(void *edid) +{ + memcpy(edid, edid_header, sizeof(edid_header)); +} + /** * drm_edid_header_is_valid - sanity check the header of the base EDID block * @raw_edid: pointer to raw base EDID block @@ -1702,7 +1707,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, if (edid_corrupt) *edid_corrupt = true; DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); - memcpy(raw_edid, edid_header, sizeof(edid_header)); + edid_header_fix(raw_edid); } else { if (edid_corrupt) *edid_corrupt = true;
Add edid_block_check() that only checks the EDID block validity, without any actions. Turns out it's simple and crystal clear.
Rewrite drm_edid_block_valid() around it, keeping all the functionality fairly closely the same, warts and all. Turns out it's incredibly complicated for a function you'd expect to be simple, with all the fixing and printing and special casing. (Maybe we'll want to simplify it in the future.)
To slightly simplify, drop the warning for EDID minor revisions > 4.
v2: - Fix edid_fixup clamp (Ville) - s/base/is_base_block/ (Ville)
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_edid.c | 152 ++++++++++++++++++++++--------------- 1 file changed, 90 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index f8169ffd062d..9e747145938b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1668,10 +1668,56 @@ bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) } EXPORT_SYMBOL(drm_edid_are_equal);
+enum edid_block_status { + EDID_BLOCK_OK = 0, + EDID_BLOCK_NULL, + EDID_BLOCK_HEADER_CORRUPT, + EDID_BLOCK_HEADER_REPAIR, + EDID_BLOCK_HEADER_FIXED, + EDID_BLOCK_CHECKSUM, + EDID_BLOCK_VERSION, +}; + +static enum edid_block_status edid_block_check(const void *_block, + bool is_base_block) +{ + const struct edid *block = _block; + + if (!block) + return EDID_BLOCK_NULL; + + if (is_base_block) { + int score = drm_edid_header_is_valid(block); + + if (score < clamp(edid_fixup, 0, 8)) + return EDID_BLOCK_HEADER_CORRUPT; + + if (score < 8) + return EDID_BLOCK_HEADER_REPAIR; + } + + if (edid_block_compute_checksum(block) != edid_block_get_checksum(block)) + return EDID_BLOCK_CHECKSUM; + + if (is_base_block) { + if (block->version != 1) + return EDID_BLOCK_VERSION; + } + + return EDID_BLOCK_OK; +} + +static bool edid_block_status_valid(enum edid_block_status status, int tag) +{ + return status == EDID_BLOCK_OK || + status == EDID_BLOCK_HEADER_FIXED || + (status == EDID_BLOCK_CHECKSUM && tag == CEA_EXT); +} + /** * drm_edid_block_valid - Sanity check the EDID block (base or extension) * @raw_edid: pointer to raw EDID block - * @block: type of block to validate (0 for base, extension otherwise) + * @block_num: type of block to validate (0 for base, extension otherwise) * @print_bad_edid: if true, dump bad EDID blocks to the console * @edid_corrupt: if true, the header or checksum is invalid * @@ -1680,88 +1726,70 @@ EXPORT_SYMBOL(drm_edid_are_equal); * * Return: True if the block is valid, false otherwise. */ -bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid, +bool drm_edid_block_valid(u8 *_block, int block_num, bool print_bad_edid, bool *edid_corrupt) { - u8 csum; - struct edid *edid = (struct edid *)raw_edid; + struct edid *block = (struct edid *)_block; + enum edid_block_status status; + bool is_base_block = block_num == 0; + bool valid;
- if (WARN_ON(!raw_edid)) + if (WARN_ON(!block)) return false;
- if (edid_fixup > 8 || edid_fixup < 0) - edid_fixup = 6; - - if (block == 0) { - int score = drm_edid_header_is_valid(raw_edid); + status = edid_block_check(block, is_base_block); + if (status == EDID_BLOCK_HEADER_REPAIR) { + DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); + edid_header_fix(block);
- if (score == 8) { - if (edid_corrupt) - *edid_corrupt = false; - } else if (score >= edid_fixup) { - /* Displayport Link CTS Core 1.2 rev1.1 test 4.2.2.6 - * The corrupt flag needs to be set here otherwise, the - * fix-up code here will correct the problem, the - * checksum is correct and the test fails - */ - if (edid_corrupt) - *edid_corrupt = true; - DRM_DEBUG("Fixing EDID header, your hardware may be failing\n"); - edid_header_fix(raw_edid); - } else { - if (edid_corrupt) - *edid_corrupt = true; - goto bad; - } + /* Retry with fixed header, update status if that worked. */ + status = edid_block_check(block, is_base_block); + if (status == EDID_BLOCK_OK) + status = EDID_BLOCK_HEADER_FIXED; }
- csum = edid_block_compute_checksum(raw_edid); - if (csum != edid_block_get_checksum(raw_edid)) { - if (edid_corrupt) + if (edid_corrupt) { + /* + * Unknown major version isn't corrupt but we can't use it. Only + * the base block can reset edid_corrupt to false. + */ + if (is_base_block && + (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)) + *edid_corrupt = false; + else if (status != EDID_BLOCK_OK) *edid_corrupt = true; - - /* allow CEA to slide through, switches mangle this */ - if (edid_block_tag(raw_edid) == CEA_EXT) { - DRM_DEBUG("EDID checksum is invalid, remainder is %d\n", csum); - DRM_DEBUG("Assuming a KVM switch modified the CEA block but left the original checksum\n"); - } else { - if (print_bad_edid) - DRM_NOTE("EDID checksum is invalid, remainder is %d\n", csum); - - goto bad; - } }
- /* per-block-type checks */ - switch (edid_block_tag(raw_edid)) { - case 0: /* base */ - if (edid->version != 1) { - DRM_NOTE("EDID has major version %d, instead of 1\n", edid->version); - goto bad; + /* Determine whether we can use this block with this status. */ + valid = edid_block_status_valid(status, edid_block_tag(block)); + + /* Some fairly random status printouts. */ + if (status == EDID_BLOCK_CHECKSUM) { + if (valid) { + DRM_DEBUG("EDID block checksum is invalid, remainder is %d\n", + edid_block_compute_checksum(block)); + DRM_DEBUG("Assuming a KVM switch modified the block but left the original checksum\n"); + } else if (print_bad_edid) { + DRM_NOTE("EDID block checksum is invalid, remainder is %d\n", + edid_block_compute_checksum(block)); } - - if (edid->revision > 4) - DRM_DEBUG("EDID minor > 4, assuming backward compatibility\n"); - break; - - default: - break; + } else if (status == EDID_BLOCK_VERSION) { + DRM_NOTE("EDID has major version %d, instead of 1\n", + block->version); }
- return true; - -bad: - if (print_bad_edid) { - if (edid_is_zero(raw_edid, EDID_LENGTH)) { + if (!valid && print_bad_edid) { + if (edid_is_zero(block, EDID_LENGTH)) { pr_notice("EDID block is all zeroes\n"); } else { pr_notice("Raw EDID:\n"); print_hex_dump(KERN_NOTICE, " \t", DUMP_PREFIX_NONE, 16, 1, - raw_edid, EDID_LENGTH, false); + block, EDID_LENGTH, false); } } - return false; + + return valid; } EXPORT_SYMBOL(drm_edid_block_valid);
Just i is a bit terse, clarify what it's about.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9e747145938b..a967e6e65ab5 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1973,25 +1973,25 @@ static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector, int *null_edid_counter = connector ? &connector->null_edid_counter : NULL; bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL; void *edid; - int i; + int try;
edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (edid == NULL) return NULL;
/* base block fetch */ - for (i = 0; i < 4; i++) { + for (try = 0; try < 4; try++) { if (get_edid_block(data, edid, 0, EDID_LENGTH)) goto out; if (drm_edid_block_valid(edid, 0, false, edid_corrupt)) break; - if (i == 0 && edid_is_zero(edid, EDID_LENGTH)) { + if (try == 0 && edid_is_zero(edid, EDID_LENGTH)) { if (null_edid_counter) (*null_edid_counter)++; goto carp; } } - if (i == 4) + if (try == 4) goto carp;
return edid; @@ -2029,7 +2029,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, size_t len), void *data) { - int i, j = 0, valid_extensions = 0; + int j, valid_extensions = 0; struct edid *edid, *new, *override;
override = drm_get_override_edid(connector); @@ -2052,20 +2052,22 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
for (j = 1; j <= edid->extensions; j++) { void *block = edid + j; + int try;
- for (i = 0; i < 4; i++) { + for (try = 0; try < 4; try++) { if (get_edid_block(data, block, j, EDID_LENGTH)) goto out; if (drm_edid_block_valid(block, j, false, NULL)) break; }
- if (i == 4) + if (try == 4) valid_extensions--; }
if (valid_extensions != edid->extensions) { struct edid *dest_block; + int i;
connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
There's no need to handle complicated scenarios or debug log when filtering blocks that have already been identified as invalid. Simplify by adding an edid_block_valid() helper that operates on const data and prints nothing.
(Finally, here's the justification for the previously added separate edid_block_status_valid() function!)
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index a967e6e65ab5..4803033d44fd 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1714,6 +1714,12 @@ static bool edid_block_status_valid(enum edid_block_status status, int tag) (status == EDID_BLOCK_CHECKSUM && tag == CEA_EXT); }
+static bool edid_block_valid(const void *block, bool base) +{ + return edid_block_status_valid(edid_block_check(block, base), + edid_block_tag(block)); +} + /** * drm_edid_block_valid - Sanity check the EDID block (base or extension) * @raw_edid: pointer to raw EDID block @@ -2080,7 +2086,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, for (i = 0; i <= edid->extensions; i++) { void *block = edid + i;
- if (!drm_edid_block_valid(block, i, false, NULL)) + if (!edid_block_valid(block, i == 0)) continue;
memcpy(dest_block, block, EDID_LENGTH);
It's such a special case there's no point in keeping it inline in the happy day scenario, confusing matters.
v2: Rebase on the invalid block filtering fix
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 52 ++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4803033d44fd..73bb2c4a7014 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1823,6 +1823,33 @@ bool drm_edid_is_valid(struct edid *edid) } EXPORT_SYMBOL(drm_edid_is_valid);
+static struct edid *edid_filter_invalid_blocks(const struct edid *edid, + int valid_extensions) +{ + struct edid *new, *dest_block; + int i; + + new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL); + if (!new) + goto out; + + dest_block = new; + for (i = 0; i <= edid->extensions; i++) { + const void *block = edid + i; + + if (edid_block_valid(block, i == 0)) + memcpy(dest_block++, block, EDID_LENGTH); + } + + new->checksum += new->extensions - valid_extensions; + new->extensions = valid_extensions; + +out: + kfree(edid); + + return new; +} + #define DDC_SEGMENT_ADDR 0x30 /** * drm_do_probe_ddc_edid() - get EDID information via I2C @@ -2072,32 +2099,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, }
if (valid_extensions != edid->extensions) { - struct edid *dest_block; - int i; - connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
- new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, - GFP_KERNEL); - if (!new) - goto out; - - dest_block = new; - for (i = 0; i <= edid->extensions; i++) { - void *block = edid + i; - - if (!edid_block_valid(block, i == 0)) - continue; - - memcpy(dest_block, block, EDID_LENGTH); - dest_block++; - } - - new->checksum += new->extensions - valid_extensions; - new->extensions = valid_extensions; - - kfree(edid); - edid = new; + edid = edid_filter_invalid_blocks(edid, valid_extensions); }
return edid;
Track invalid blocks instead of valid extensions to minimize impact on the happy day scenario, and hide the details in the separate function.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 73bb2c4a7014..8bf0ae72fd2c 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1824,9 +1824,10 @@ bool drm_edid_is_valid(struct edid *edid) EXPORT_SYMBOL(drm_edid_is_valid);
static struct edid *edid_filter_invalid_blocks(const struct edid *edid, - int valid_extensions) + int invalid_blocks) { struct edid *new, *dest_block; + int valid_extensions = edid->extensions - invalid_blocks; int i;
new = kmalloc_array(valid_extensions + 1, EDID_LENGTH, GFP_KERNEL); @@ -2062,7 +2063,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, size_t len), void *data) { - int j, valid_extensions = 0; + int j, invalid_blocks = 0; struct edid *edid, *new, *override;
override = drm_get_override_edid(connector); @@ -2073,12 +2074,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, if (!edid) return NULL;
- /* if there's no extensions or no connector, we're done */ - valid_extensions = edid->extensions; - if (valid_extensions == 0) + if (edid->extensions == 0) return edid;
- new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL); + new = krealloc(edid, (edid->extensions + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; edid = new; @@ -2095,13 +2094,13 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, }
if (try == 4) - valid_extensions--; + invalid_blocks++; }
- if (valid_extensions != edid->extensions) { + if (invalid_blocks) { connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
- edid = edid_filter_invalid_blocks(edid, valid_extensions); + edid = edid_filter_invalid_blocks(edid, invalid_blocks); }
return edid;
The code modifying the EDID block should not need to do tricks to fix the checksum. We have a function for computing the checksum, use it.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 8bf0ae72fd2c..04e818ecd662 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1842,8 +1842,8 @@ static struct edid *edid_filter_invalid_blocks(const struct edid *edid, memcpy(dest_block++, block, EDID_LENGTH); }
- new->checksum += new->extensions - valid_extensions; new->extensions = valid_extensions; + new->checksum = edid_block_compute_checksum(new);
out: kfree(edid);
On Thu, 31 Mar 2022, Jani Nikula jani.nikula@intel.com wrote:
v2 of https://patchwork.freedesktop.org/series/101931/
Rebased, review comments addressed.
Ville, care to double check patches 1 & 7 please?
Thanks, Jani.
BR, Jani.
Jani Nikula (12): drm/edid: use struct edid * in drm_do_get_edid() drm/edid: clean up EDID block checksum functions drm/edid: add edid_block_tag() helper to get the EDID extension tag drm/edid: make drm_edid_header_is_valid() accept void pointer drm/edid: clean up edid_is_zero() drm/edid: split out edid_header_fix() drm/edid: split drm_edid_block_valid() to check and act parts drm/edid: use a better variable name for EDID block read retries drm/edid: simplify block check when filtering invalid blocks drm/edid: split out invalid block filtering to a separate function drm/edid: track invalid blocks in drm_do_get_edid() drm/edid: reduce magic when updating the EDID block checksum
drivers/gpu/drm/drm_edid.c | 295 +++++++++++++++++++++---------------- include/drm/drm_edid.h | 2 +- 2 files changed, 173 insertions(+), 124 deletions(-)
On Fri, Apr 01, 2022 at 11:55:21AM +0300, Jani Nikula wrote:
On Thu, 31 Mar 2022, Jani Nikula jani.nikula@intel.com wrote:
v2 of https://patchwork.freedesktop.org/series/101931/
Rebased, review comments addressed.
Ville, care to double check patches 1 & 7 please?
Looks fine by me Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Thanks, Jani.
BR, Jani.
Jani Nikula (12): drm/edid: use struct edid * in drm_do_get_edid() drm/edid: clean up EDID block checksum functions drm/edid: add edid_block_tag() helper to get the EDID extension tag drm/edid: make drm_edid_header_is_valid() accept void pointer drm/edid: clean up edid_is_zero() drm/edid: split out edid_header_fix() drm/edid: split drm_edid_block_valid() to check and act parts drm/edid: use a better variable name for EDID block read retries drm/edid: simplify block check when filtering invalid blocks drm/edid: split out invalid block filtering to a separate function drm/edid: track invalid blocks in drm_do_get_edid() drm/edid: reduce magic when updating the EDID block checksum
drivers/gpu/drm/drm_edid.c | 295 +++++++++++++++++++++---------------- include/drm/drm_edid.h | 2 +- 2 files changed, 173 insertions(+), 124 deletions(-)
-- Jani Nikula, Intel Open Source Graphics Center
On Fri, 01 Apr 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Apr 01, 2022 at 11:55:21AM +0300, Jani Nikula wrote:
On Thu, 31 Mar 2022, Jani Nikula jani.nikula@intel.com wrote:
v2 of https://patchwork.freedesktop.org/series/101931/
Rebased, review comments addressed.
Ville, care to double check patches 1 & 7 please?
Looks fine by me Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Thanks, pushed the lot to drm-misc-next.
BR, Jani.
Thanks, Jani.
BR, Jani.
Jani Nikula (12): drm/edid: use struct edid * in drm_do_get_edid() drm/edid: clean up EDID block checksum functions drm/edid: add edid_block_tag() helper to get the EDID extension tag drm/edid: make drm_edid_header_is_valid() accept void pointer drm/edid: clean up edid_is_zero() drm/edid: split out edid_header_fix() drm/edid: split drm_edid_block_valid() to check and act parts drm/edid: use a better variable name for EDID block read retries drm/edid: simplify block check when filtering invalid blocks drm/edid: split out invalid block filtering to a separate function drm/edid: track invalid blocks in drm_do_get_edid() drm/edid: reduce magic when updating the EDID block checksum
drivers/gpu/drm/drm_edid.c | 295 +++++++++++++++++++++---------------- include/drm/drm_edid.h | 2 +- 2 files changed, 173 insertions(+), 124 deletions(-)
-- Jani Nikula, Intel Open Source Graphics Center
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Ville Syrjälä