Re: [PATCH] drm: Fix EDID firmware load on resume

[Takashi Iwai]

On Wed, 27 Jul 2022 09:41:52 +0200,
Matthieu CHARETTE wrote:
> 
> Loading an EDID using drm.edid_firmware parameter makes resume to fail
> after firmware cache is being cleaned. This is because edid_load() use a
> temporary device to request the firmware. This cause the EDID firmware
> not to be cached from suspend. And, requesting the EDID firmware return
> an error during resume.
> So the request_firmware() call should use a permanent device for each
> connector. Also, we should cache the EDID even if no monitor is
> connected, in case it's plugged while suspended.
> 
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
> Signed-off-by: Matthieu CHARETTE <matthieu.chare...@gmail.com>

Can we simply cache the already loaded EDID bytes instead?
Something like below (totally untested).


thanks,

Takashi

-- 8< --
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..b9d2803b518b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -286,6 +286,7 @@ int drm_connector_init(struct drm_device *dev,
        connector->status = connector_status_unknown;
        connector->display_info.panel_orientation =
                DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+       connector->firmware_edid = NULL;
 
        drm_connector_get_cmdline_mode(connector);
 
@@ -485,6 +486,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
        ida_simple_remove(&dev->mode_config.connector_ida,
                          connector->index);
 
+       kfree(connector->firmware_edid);
        kfree(connector->display_info.bus_formats);
        drm_mode_object_unregister(dev, &connector->base);
        kfree(connector->name);
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..a38fe4e00e4a 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -253,6 +253,13 @@ static void *edid_load(struct drm_connector *connector, 
const char *name,
                        edid = new_edid;
        }
 
+       connector->firmware_edid = drm_edid_duplicate((struct edid *)edid);
+       if (!connector->firmware_edid) {
+               kfree(edid);
+               edid = ERR_PTR(-ENOMEM);
+               goto out;
+       }
+
        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",
@@ -269,6 +276,12 @@ struct edid *drm_load_edid_firmware(struct drm_connector 
*connector)
        char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
        struct edid *edid;
 
+       /* already loaded? */
+       if (connector->firmware_edid) {
+               edid = drm_edid_duplicate(connector->firmware_edid);
+               return edid ? edid : ERR_PTR(-ENOMEM);
+       }
+
        if (edid_firmware[0] == '\0')
                return ERR_PTR(-ENOENT);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..b5d0c87327a3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1528,6 +1528,8 @@ struct drm_connector {
        enum drm_connector_force force;
        /** @override_edid: has the EDID been overwritten through debugfs for 
testing? */
        bool override_edid;
+       /** @firmware_edid: the cached firmware EDID bytes */
+       struct edid *firmware_edid;
        /** @epoch_counter: used to detect any other changes in connector, 
besides status */
        u64 epoch_counter;