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

[Matthieu CHARETTE <matthieu.charette@xxxxxxxxx>]

Hi,

Caching the EDID via the firmware API makes the kernel able to reclaim 
the memory in case it's needed. And eventually, the kernel will load it 
again before suspending.
But for 128 bytes, even if we have many monitors it will not make any 
difference.
I don't know if storing a platform device can take more memory than 128 
bytes of data.
I let you decide which option is better. Just tell me if I should cache 
the bytes instead.

Thanks.

Matthieu

On Wed, Jul 27 2022 at 10:18:48 AM +0200, Takashi Iwai <tiwai@xxxxxxx> 
wrote:
> 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;