On Thu, 31 Mar 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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?
Okay.
>
>> +{
>> + 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?
Indeed, thanks!
> 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.
Yes.
BR,
Jani.
>
>> + 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
--
Jani Nikula, Intel Open Source Graphics Center
