On Wed, 30 Mar 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
>> Mixing u8 * and struct edid * is confusing, switch to the latter.
>>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/drm_edid.c | 31 +++++++++++++++----------------
>> 1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index d79b06f7f34c..0650b9217aa2 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,13 +2025,13 @@ 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 *base;
>
> This one points to extension blocks too so using
> struct edid doesn't seem entirely appropriate.
So I've gone back and forth with this. I think I want to get rid of u8*
no matter what, because it always requires casting. I've used void* here
and there to allow mixed use, internally in drm_edid.c while
transitioning, and in public interfaces due to usage all over the place.
OTOH I don't much like arithmetics on void*. It's a gcc extension.
struct edid * is useful for e.g. ->checksum and arithmetics. In many
places I've named it struct edid *block to distinguish. We could have a
struct edid_block too, which could have ->tag and ->checksum members,
for example, but then it would require casting or a function for "safe"
typecasting.
I've also gone back and forth with the helpers for getting a pointer to
a block. For usage like this, kind of need both const and non-const
versions. And, with the plans I have for future, I'm not sure I want to
promote any EDID parsing outside of drm_edid.c, so maybe they should be
static.
Undecided. C is a bit clunky here.
>
>>
>> - connector_bad_edid(connector, edid, edid[0x7e] + 1);
>> + connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
>>
>> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>> - edid[0x7e] = valid_extensions;
>> + edid->checksum += edid->extensions - valid_extensions;
>> + edid->extensions = valid_extensions;
>>
>> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
>> GFP_KERNEL);
>> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>> goto out;
>>
>> base = new;
>> - for (i = 0; i <= edid[0x7e]; i++) {
>> - u8 *block = edid + i * EDID_LENGTH;
>> + for (i = 0; i <= edid->extensions; i++) {
>> + void *block = edid + i;
>
> Hmm. This code seems very broken to me. We read each block
> into its expected offset based on the original base block extension
> count, but here we only iterate up to the new ext block count. So
> if we had eg. a 4 block EDID where block 2 is busted, we set
> the new ext count to 2, copy over blocks 0 and 1, skip block 2,
> and then terminate the loop. So instead of copying block 3 from
> the orignal EDID into block 2 of the new EDID, we leave the
> original garbage block 2 in place.
Ugh. I end up fixing this in the series, in "drm/edid: split out invalid
block filtering to a separate function", but I don't mention it
anywhere.
Looks like it's been broken for 5+ years since commit 14544d0937bf
("drm/edid: Only print the bad edid when aborting").
Which really makes you wonder about the usefulness of trying to "fix"
the EDID by skipping extension blocks. It was added in commit
0ea75e23356f ("DRM: ignore invalid EDID extensions").
> Also this memcpy() business seems entirely poinless in the sense
> that we could just read each ext block into the final offset
> directly AFAICS.
This is how it was before commit 14544d0937bf. I guess the point is if
we decide the EDID is garbage, we want to print the original EDID, once,
not something we've already changed. I also kind of like the idea of
hiding the broken EDID path magic in a separate function.
BR,
Jani.
>
>>
>> if (!drm_edid_block_valid(block, i, false, NULL))
>> continue;
>>
>> memcpy(base, block, EDID_LENGTH);
>> - base += EDID_LENGTH;
>> + base++;
>> }
>>
>> kfree(edid);
>> edid = new;
>> }
>>
>> - return (struct edid *)edid;
>> + return edid;
>>
>> out:
>> kfree(edid);
>> --
>> 2.30.2
--
Jani Nikula, Intel Open Source Graphics Center
