On Tue, Mar 29, 2022 at 09:42:14PM +0300, Jani Nikula wrote:
> 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.)
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_edid.c | 150 ++++++++++++++++++++++---------------
> 1 file changed, 88 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 481643751d10..04eb6949c9c8 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1668,10 +1668,55 @@ 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 base)
s/base/is_base_block/ or something?
> +{
> + const struct edid *block = _block;
> +
> + if (!block)
> + return EDID_BLOCK_NULL;
> +
> + if (base) {
> + int score = drm_edid_header_is_valid(block);
> +
> + if (score < clamp(edid_fixup, 6, 8))
That should clamp to 0-8?
Might be nicer to just define a .set() op for the modparam
and check it there, but that's clearly material for a separate patch.
> + 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 (base) {
> + 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 +1725,69 @@ 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 base = 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, base);
> + 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, base);
> + 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 (base && (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");
This debug message seems to disappear. Intentional?
> - 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);
>
> --
> 2.30.2
--
Ville Syrjälä
Intel
