Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 27 Jul 2022 10:18:48 +0200
Message-ID: <875yjjotnb.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Matthieu CHARETTE <matthieu.charette@gmail.com>
Cc:     lkp@intel.com, airlied@linux.ie, andrealmeid@igalia.com,
        daniel@ffwll.ch, dri-devel@lists.freedesktop.org,
        kbuild-all@lists.01.org, linux-kernel@vger.kernel.org,
        maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de
Subject: Re: [PATCH] drm: Fix EDID firmware load on resume
In-Reply-To: <20220727074152.43059-1-matthieu.charette@gmail.com>
References: <202207172035.mtErdlaw-lkp@intel.com>
        <20220727074152.43059-1-matthieu.charette@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@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;