| Message ID | 1011a285d30babce3aabd8218abb7ece7dcf58a2.1713259151.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/edid: cleanups, rebase | expand |
Hi Am 16.04.24 um 11:20 schrieb Jani Nikula: > Repurpose drm_edid_are_equal() to be more helpful for its single user, > and rename drm_edid_eq(). Functionally deduce the length from the blob > size, not the blob data, making it more robust against any errors. Could be squashed into patch 6. Best regards Thomas > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------ > 1 file changed, 15 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 463fbad85d90..513590931cc5 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid) > return !memchr_inv(edid, 0, EDID_LENGTH); > } > > -/** > - * drm_edid_are_equal - compare two edid blobs. > - * @edid1: pointer to first blob > - * @edid2: pointer to second blob > - * This helper can be used during probing to determine if > - * edid had changed. > - */ > -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) > +static bool drm_edid_eq(const struct drm_edid *drm_edid, > + const void *raw_edid, size_t raw_edid_size) > { > - int edid1_len, edid2_len; > - bool edid1_present = edid1 != NULL; > - bool edid2_present = edid2 != NULL; > + bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size; > + bool edid2_present = raw_edid && raw_edid_size; > > if (edid1_present != edid2_present) > return false; > > - if (edid1) { > - edid1_len = edid_size(edid1); > - edid2_len = edid_size(edid2); > - > - if (edid1_len != edid2_len) > + if (edid1_present) { > + if (drm_edid->size != raw_edid_size) > return false; > > - if (memcmp(edid1, edid2, edid1_len)) > + if (memcmp(drm_edid->edid, raw_edid, drm_edid->size)) > return false; > } > > @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector, > int ret; > > if (connector->edid_blob_ptr) { > - const struct edid *old_edid = connector->edid_blob_ptr->data; > - > - if (old_edid) { > - if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) { > - connector->epoch_counter++; > - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", > - connector->base.id, connector->name, > - connector->epoch_counter); > - } > + const void *old_edid = connector->edid_blob_ptr->data; > + size_t old_edid_size = connector->edid_blob_ptr->length; > + > + if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) { > + connector->epoch_counter++; > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", > + connector->base.id, connector->name, > + connector->epoch_counter); > } > } >
On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 16.04.24 um 11:20 schrieb Jani Nikula: >> Repurpose drm_edid_are_equal() to be more helpful for its single user, >> and rename drm_edid_eq(). Functionally deduce the length from the blob >> size, not the blob data, making it more robust against any errors. > > Could be squashed into patch 6. Ack. Thanks for the review. I'll hold of on resending these until there are some R-b's... I've send them a few times already with no comments. :( BR, Jani. > > Best regards > Thomas > >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------ >> 1 file changed, 15 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 463fbad85d90..513590931cc5 100644 >> --- a/drivers/gpu/drm/drm_edid.c >> +++ b/drivers/gpu/drm/drm_edid.c >> @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid) >> return !memchr_inv(edid, 0, EDID_LENGTH); >> } >> >> -/** >> - * drm_edid_are_equal - compare two edid blobs. >> - * @edid1: pointer to first blob >> - * @edid2: pointer to second blob >> - * This helper can be used during probing to determine if >> - * edid had changed. >> - */ >> -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) >> +static bool drm_edid_eq(const struct drm_edid *drm_edid, >> + const void *raw_edid, size_t raw_edid_size) >> { >> - int edid1_len, edid2_len; >> - bool edid1_present = edid1 != NULL; >> - bool edid2_present = edid2 != NULL; >> + bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size; >> + bool edid2_present = raw_edid && raw_edid_size; >> >> if (edid1_present != edid2_present) >> return false; >> >> - if (edid1) { >> - edid1_len = edid_size(edid1); >> - edid2_len = edid_size(edid2); >> - >> - if (edid1_len != edid2_len) >> + if (edid1_present) { >> + if (drm_edid->size != raw_edid_size) >> return false; >> >> - if (memcmp(edid1, edid2, edid1_len)) >> + if (memcmp(drm_edid->edid, raw_edid, drm_edid->size)) >> return false; >> } >> >> @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector, >> int ret; >> >> if (connector->edid_blob_ptr) { >> - const struct edid *old_edid = connector->edid_blob_ptr->data; >> - >> - if (old_edid) { >> - if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) { >> - connector->epoch_counter++; >> - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", >> - connector->base.id, connector->name, >> - connector->epoch_counter); >> - } >> + const void *old_edid = connector->edid_blob_ptr->data; >> + size_t old_edid_size = connector->edid_blob_ptr->length; >> + >> + if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) { >> + connector->epoch_counter++; >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", >> + connector->base.id, connector->name, >> + connector->epoch_counter); >> } >> } >>
Hi Am 16.04.24 um 14:27 schrieb Jani Nikula: > On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 16.04.24 um 11:20 schrieb Jani Nikula: >>> Repurpose drm_edid_are_equal() to be more helpful for its single user, >>> and rename drm_edid_eq(). Functionally deduce the length from the blob >>> size, not the blob data, making it more robust against any errors. >> Could be squashed into patch 6. > Ack. > > Thanks for the review. I'll hold of on resending these until there are > some R-b's... I've send them a few times already with no comments. :( Feel free to add Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> to the series. Best regards Thomas > > BR, > Jani. > >> Best regards >> Thomas >> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------ >>> 1 file changed, 15 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>> index 463fbad85d90..513590931cc5 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid) >>> return !memchr_inv(edid, 0, EDID_LENGTH); >>> } >>> >>> -/** >>> - * drm_edid_are_equal - compare two edid blobs. >>> - * @edid1: pointer to first blob >>> - * @edid2: pointer to second blob >>> - * This helper can be used during probing to determine if >>> - * edid had changed. >>> - */ >>> -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) >>> +static bool drm_edid_eq(const struct drm_edid *drm_edid, >>> + const void *raw_edid, size_t raw_edid_size) >>> { >>> - int edid1_len, edid2_len; >>> - bool edid1_present = edid1 != NULL; >>> - bool edid2_present = edid2 != NULL; >>> + bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size; >>> + bool edid2_present = raw_edid && raw_edid_size; >>> >>> if (edid1_present != edid2_present) >>> return false; >>> >>> - if (edid1) { >>> - edid1_len = edid_size(edid1); >>> - edid2_len = edid_size(edid2); >>> - >>> - if (edid1_len != edid2_len) >>> + if (edid1_present) { >>> + if (drm_edid->size != raw_edid_size) >>> return false; >>> >>> - if (memcmp(edid1, edid2, edid1_len)) >>> + if (memcmp(drm_edid->edid, raw_edid, drm_edid->size)) >>> return false; >>> } >>> >>> @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector, >>> int ret; >>> >>> if (connector->edid_blob_ptr) { >>> - const struct edid *old_edid = connector->edid_blob_ptr->data; >>> - >>> - if (old_edid) { >>> - if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) { >>> - connector->epoch_counter++; >>> - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", >>> - connector->base.id, connector->name, >>> - connector->epoch_counter); >>> - } >>> + const void *old_edid = connector->edid_blob_ptr->data; >>> + size_t old_edid_size = connector->edid_blob_ptr->length; >>> + >>> + if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) { >>> + connector->epoch_counter++; >>> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", >>> + connector->base.id, connector->name, >>> + connector->epoch_counter); >>> } >>> } >>>
On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 16.04.24 um 14:27 schrieb Jani Nikula: >> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> Hi >>> >>> Am 16.04.24 um 11:20 schrieb Jani Nikula: >>>> Repurpose drm_edid_are_equal() to be more helpful for its single user, >>>> and rename drm_edid_eq(). Functionally deduce the length from the blob >>>> size, not the blob data, making it more robust against any errors. >>> Could be squashed into patch 6. >> Ack. >> >> Thanks for the review. I'll hold of on resending these until there are >> some R-b's... I've send them a few times already with no comments. :( > > Feel free to add > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > > to the series. Many thanks! Just to double check, do you want me to move patch 5 earlier and squash patches 6&7? BR, Jani.
Hi Am 17.04.24 um 10:21 schrieb Jani Nikula: > On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Hi >> >> Am 16.04.24 um 14:27 schrieb Jani Nikula: >>> On Tue, 16 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> Hi >>>> >>>> Am 16.04.24 um 11:20 schrieb Jani Nikula: >>>>> Repurpose drm_edid_are_equal() to be more helpful for its single user, >>>>> and rename drm_edid_eq(). Functionally deduce the length from the blob >>>>> size, not the blob data, making it more robust against any errors. >>>> Could be squashed into patch 6. >>> Ack. >>> >>> Thanks for the review. I'll hold of on resending these until there are >>> some R-b's... I've send them a few times already with no comments. :( >> Feel free to add >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> >> to the series. > Many thanks! Just to double check, do you want me to move patch 5 > earlier and squash patches 6&7? Your choice. Either is fine by me. Best regards Thomas > > BR, > Jani. > >
On Wed, 17 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Many thanks! Just to double check, do you want me to move patch 5 >> earlier and squash patches 6&7? > > Your choice. Either is fine by me. I jumped at the easy option and merged this as-is. :) Thanks again, Jani.
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 463fbad85d90..513590931cc5 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1820,30 +1820,20 @@ static bool edid_block_is_zero(const void *edid) return !memchr_inv(edid, 0, EDID_LENGTH); } -/** - * drm_edid_are_equal - compare two edid blobs. - * @edid1: pointer to first blob - * @edid2: pointer to second blob - * This helper can be used during probing to determine if - * edid had changed. - */ -static bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2) +static bool drm_edid_eq(const struct drm_edid *drm_edid, + const void *raw_edid, size_t raw_edid_size) { - int edid1_len, edid2_len; - bool edid1_present = edid1 != NULL; - bool edid2_present = edid2 != NULL; + bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size; + bool edid2_present = raw_edid && raw_edid_size; if (edid1_present != edid2_present) return false; - if (edid1) { - edid1_len = edid_size(edid1); - edid2_len = edid_size(edid2); - - if (edid1_len != edid2_len) + if (edid1_present) { + if (drm_edid->size != raw_edid_size) return false; - if (memcmp(edid1, edid2, edid1_len)) + if (memcmp(drm_edid->edid, raw_edid, drm_edid->size)) return false; } @@ -6936,15 +6926,14 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector, int ret; if (connector->edid_blob_ptr) { - const struct edid *old_edid = connector->edid_blob_ptr->data; - - if (old_edid) { - if (!drm_edid_are_equal(drm_edid ? drm_edid->edid : NULL, old_edid)) { - connector->epoch_counter++; - drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", - connector->base.id, connector->name, - connector->epoch_counter); - } + const void *old_edid = connector->edid_blob_ptr->data; + size_t old_edid_size = connector->edid_blob_ptr->length; + + if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) { + connector->epoch_counter++; + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n", + connector->base.id, connector->name, + connector->epoch_counter); } }
Repurpose drm_edid_are_equal() to be more helpful for its single user, and rename drm_edid_eq(). Functionally deduce the length from the blob size, not the blob data, making it more robust against any errors. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_edid.c | 41 ++++++++++++++------------------------ 1 file changed, 15 insertions(+), 26 deletions(-)