On Tue, Oct 11, 2022 at 04:49:46PM +0300, Jani Nikula wrote:
> There's a lot going on here, but the main thing is switching the
> firmware EDID loader to use struct drm_edid. Unfortunately, it's
> difficult to reasonably split to smaller pieces.
>
> Convert the EDID loader to struct drm_edid. There's a functional change
> in validation; it no longer tries to fix errors or filter invalid
> blocks. It's stricter in this sense. Hopefully this will not be an
> issue.
>
> As a by-product, this change also allows HF-EEODB extended EDIDs to be
> passed via override/firmware EDID.
Was pondering about that reading the earlier patch. Figured I'd keep
on reading, and voila here it is.
>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_edid.c | 32 ++++++------
> drivers/gpu/drm/drm_edid_load.c | 86 +++++++--------------------------
> include/drm/drm_edid.h | 4 +-
> 3 files changed, 36 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 2c66a901474d..935bdf4e6269 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2202,21 +2202,16 @@ static void connector_bad_edid(struct drm_connector *connector,
> }
>
> /* Get override or firmware EDID */
> -static struct edid *drm_get_override_edid(struct drm_connector *connector,
> - size_t *alloc_size)
> +static const struct drm_edid *drm_edid_override_get(struct drm_connector *connector)
> {
> - struct edid *override = NULL;
> + const struct drm_edid *override = NULL;
>
> if (connector->edid_override)
> - override = drm_edid_duplicate(connector->edid_override->edid);
> + override = drm_edid_dup(connector->edid_override);
>
> if (!override)
> override = drm_edid_load_firmware(connector);
>
> - /* FIXME: Get alloc size from deeper down the stack */
> - if (!IS_ERR_OR_NULL(override) && alloc_size)
> - *alloc_size = edid_size(override);
> -
> return IS_ERR(override) ? NULL : override;
> }
>
> @@ -2277,14 +2272,14 @@ int drm_edid_override_reset(struct drm_connector *connector)
> */
> int drm_edid_override_connector_update(struct drm_connector *connector)
> {
> - struct edid *override;
> + const struct drm_edid *override;
> int num_modes = 0;
>
> - override = drm_get_override_edid(connector, NULL);
> + override = drm_edid_override_get(connector);
> if (override) {
> - drm_connector_update_edid_property(connector, override);
> - num_modes = drm_add_edid_modes(connector, override);
> - kfree(override);
> + num_modes = drm_edid_connector_update(connector, override);
> +
> + drm_edid_free(override);
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> connector->base.id, connector->name, num_modes);
> @@ -2335,12 +2330,19 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
> {
> enum edid_block_status status;
> int i, num_blocks, invalid_blocks = 0;
> + const struct drm_edid *override;
> struct edid *edid, *new;
> size_t alloc_size = EDID_LENGTH;
>
> - edid = drm_get_override_edid(connector, &alloc_size);
> - if (edid)
> + override = drm_edid_override_get(connector);
> + if (override) {
> + alloc_size = override->size;
> + edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
> + drm_edid_free(override);
> + if (!edid)
> + return NULL;
> goto ok;
> + }
>
> edid = kmalloc(alloc_size, GFP_KERNEL);
> if (!edid)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index bc6b96abd1b3..248f0685c33e 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -159,22 +159,12 @@ static const u8 generic_edid[GENERIC_EDIDS][128] = {
> },
> };
>
> -static int edid_size(const u8 *edid, int data_size)
> -{
> - if (data_size < EDID_LENGTH)
> - return 0;
> -
> - return (edid[0x7e] + 1) * EDID_LENGTH;
> -}
> -
> -static void *edid_load(struct drm_connector *connector, const char *name)
> +static const struct drm_edid *edid_load(struct drm_connector *connector, const char *name)
> {
> const struct firmware *fw = NULL;
> const u8 *fwdata;
> - u8 *edid;
> + const struct drm_edid *drm_edid;
> int fwsize, builtin;
> - int i, valid_extensions = 0;
> - bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
>
> builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
> if (builtin >= 0) {
> @@ -203,69 +193,26 @@ static void *edid_load(struct drm_connector *connector, const char *name)
> fwsize = fw->size;
> }
>
> - if (edid_size(fwdata, fwsize) != fwsize) {
> - DRM_ERROR("Size of EDID firmware \"%s\" is invalid "
> - "(expected %d, got %d\n", name,
> - edid_size(fwdata, fwsize), (int)fwsize);
> - edid = ERR_PTR(-EINVAL);
> - goto out;
> - }
> -
> - edid = kmemdup(fwdata, fwsize, GFP_KERNEL);
> - if (edid == NULL) {
> - edid = ERR_PTR(-ENOMEM);
> - goto out;
> - }
> -
> - if (!drm_edid_block_valid(edid, 0, print_bad_edid,
> - &connector->edid_corrupt)) {
> - connector->bad_edid_counter++;
> - DRM_ERROR("Base block of EDID firmware \"%s\" is invalid ",
> - name);
> - kfree(edid);
> - edid = ERR_PTR(-EINVAL);
> - goto out;
> - }
> -
> - for (i = 1; i <= edid[0x7e]; i++) {
> - if (i != valid_extensions + 1)
> - memcpy(edid + (valid_extensions + 1) * EDID_LENGTH,
> - edid + i * EDID_LENGTH, EDID_LENGTH);
> - if (drm_edid_block_valid(edid + i * EDID_LENGTH, i,
> - print_bad_edid,
> - NULL))
> - valid_extensions++;
> - }
> -
> - if (valid_extensions != edid[0x7e]) {
> - u8 *new_edid;
> + drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] Loaded %s firmware EDID \"%s\"\n",
> + connector->base.id, connector->name,
> + builtin >= 0 ? "built-in" : "external", name);
>
> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> - DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> - "\"%s\" for connector \"%s\"\n", valid_extensions,
> - edid[0x7e], name, connector->name);
> - edid[0x7e] = valid_extensions;
> -
> - new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> - GFP_KERNEL);
> - if (new_edid)
> - edid = new_edid;
> + drm_edid = drm_edid_alloc(fwdata, fwsize);
> + if (!drm_edid_valid(drm_edid)) {
> + drm_err(connector->dev, "Invalid firmware EDID \"%s\"\n", name);
> + drm_edid_free(drm_edid);
> + drm_edid = ERR_PTR(-EINVAL);
> }
Lovely diffstat ratio there.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> - DRM_INFO("Got %s EDID base block and %d extension%s from "
> - "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> - "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> - name, connector->name);
> -
> -out:
> release_firmware(fw);
> - return edid;
> +
> + return drm_edid;
> }
>
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector)
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector)
> {
> char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> - struct edid *edid;
> + const struct drm_edid *drm_edid;
>
> if (edid_firmware[0] == '\0')
> return ERR_PTR(-ENOENT);
> @@ -308,8 +255,9 @@ struct edid *drm_edid_load_firmware(struct drm_connector *connector)
> if (*last == '\n')
> *last = '\0';
>
> - edid = edid_load(connector, edidname);
> + drm_edid = edid_load(connector, edidname);
> +
> kfree(fwstr);
>
> - return edid;
> + return drm_edid;
> }
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index dc7467d25cd8..8138613f4e4e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -388,11 +388,11 @@ int drm_av_sync_delay(struct drm_connector *connector,
> const struct drm_display_mode *mode);
>
> #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> -struct edid *drm_edid_load_firmware(struct drm_connector *connector);
> +const struct drm_edid *drm_edid_load_firmware(struct drm_connector *connector);
> int __drm_set_edid_firmware_path(const char *path);
> int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
> #else
> -static inline struct edid *
> +static inline const struct drm_edid *
> drm_edid_load_firmware(struct drm_connector *connector)
> {
> return ERR_PTR(-ENOENT);
> --
> 2.34.1
--
Ville Syrjälä
Intel
