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.charette@xxxxxxxxx> 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;
