[PATCH] drm: Fix EDID firmware load on resume

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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.

Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
---
 drivers/gpu/drm/drm_connector.c | 9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h | 12 +++++
 include/drm/drm_edid.h | 3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c 
b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>

+#include <linux/platform_device.h>
 #include <linux/uaccess.h>

 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,

  drm_connector_get_cmdline_mode(connector);

+ connector->edid_load_pdev = NULL;
+ drm_cache_edid_firmware(connector);
+
  /* We should add connectors at the end to avoid upsetting the 
connector
   * index too much.
   */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector 
*connector)
   connector->tile_group = NULL;
  }

+ if (connector->edid_load_pdev) {
+ platform_device_unregister(connector->edid_load_pdev);
+ connector->edid_load_pdev = NULL;
+ }
+
  list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
   drm_mode_remove(connector, mode);

diff --git a/drivers/gpu/drm/drm_edid_load.c 
b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
  return (edid[0x7e] + 1) * EDID_LENGTH;
 }

+static struct platform_device *edid_pdev(const char *connector_name)
+{
+ struct platform_device *pdev = 
platform_device_register_simple(connector_name, -1, NULL, 0);
+
+ if (IS_ERR(pdev)) {
+ DRM_ERROR("Failed to register EDID firmware platform device "
+ "for connector \"%s\"\n", connector_name);
+ return ERR_CAST(pdev);
+ }
+
+ return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char 
*name,
    const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
*connector, const char *name,
   fwdata = generic_edid[builtin];
   fwsize = sizeof(generic_edid[builtin]);
  } else {
- struct platform_device *pdev;
+ struct platform_device *pdev = connector->edid_load_pdev;
   int err;

- pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
- if (IS_ERR(pdev)) {
- DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
- return ERR_CAST(pdev);
+ if (WARN_ON(!pdev)) {
+ pdev = edid_pdev(connector_name);
+ if (IS_ERR(pdev))
+ return ERR_CAST(pdev);
+ connector->edid_load_pdev = pdev;
   }

   err = request_firmware(&fw, name, &pdev->dev);
- platform_device_unregister(pdev);
   if (err) {
    DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
       name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
*connector, const char *name,
  return edid;
 }

-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
- const char *connector_name = connector->name;
  char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
- struct edid *edid;

  if (edid_firmware[0] == '\0')
   return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
drm_connector *connector)
  if (*last == '\n')
   *last = '\0';

- edid = edid_load(connector, edidname, connector_name);
+ edidname = kstrdup(edidname, GFP_KERNEL);
+ if (!edidname) {
+ kfree(fwstr);
+ return ERR_PTR(-ENOMEM);
+ }
+
  kfree(fwstr);
+ return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+ const char *connector_name = connector->name;
+ const char *edidname = edid_name(connector_name);
+ struct platform_device *pdev;
+ int err;
+
+ if (IS_ERR(edidname))
+ return;
+
+ if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+ kfree(edidname);
+ return;
+ }
+
+ pdev = edid_pdev(connector_name);
+ if (IS_ERR(pdev)) {
+ kfree(edidname);
+ return;
+ }
+ connector->edid_load_pdev = pdev;
+
+ err = firmware_request_cache(&pdev->dev, edidname);
+ if (err)
+ DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+ edidname, err);
+
+ kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+ const char *connector_name = connector->name;
+ const char *edidname = edid_name(connector_name);
+ struct edid *edid;
+
+ if (IS_ERR(edidname))
+ return ERR_CAST(edidname);
+
+ edid = edid_load(connector, edidname, connector_name);
+ kfree(edidname);

  return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
   */
  struct i2c_adapter *ddc;

+ /**
+ * @edid_load_pdev: Platform device for loading EDID via firmware.
+ *
+ * The platform device is registered in drm_connector_init() in case a
+ * custom EDID firmware is used with `edid_firmware` parameter. 
Otherwise,
+ * it is set to NULL.
+ *
+ * Platform device is unregistered in drm_connector_cleanup() if it
+ * is not NULL.
+ */
+ struct platform_device *edid_load_pdev;
+
  /**
   * @null_edid_counter: track sinks that give us all zeros for the 
EDID.
   * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..b54cea3f1107 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
*connector,
         const struct drm_display_mode *mode);

 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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 void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.36.1

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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.

Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
---
 drivers/gpu/drm/drm_connector.c | 9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h | 12 +++++
 include/drm/drm_edid.h | 3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c 
b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>

+#include <linux/platform_device.h>
 #include <linux/uaccess.h>

 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,

  drm_connector_get_cmdline_mode(connector);

+ connector->edid_load_pdev = NULL;
+ drm_cache_edid_firmware(connector);
+
  /* We should add connectors at the end to avoid upsetting the 
connector
   * index too much.
   */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector 
*connector)
   connector->tile_group = NULL;
  }

+ if (connector->edid_load_pdev) {
+ platform_device_unregister(connector->edid_load_pdev);
+ connector->edid_load_pdev = NULL;
+ }
+
  list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
   drm_mode_remove(connector, mode);

diff --git a/drivers/gpu/drm/drm_edid_load.c 
b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
  return (edid[0x7e] + 1) * EDID_LENGTH;
 }

+static struct platform_device *edid_pdev(const char *connector_name)
+{
+ struct platform_device *pdev = 
platform_device_register_simple(connector_name, -1, NULL, 0);
+
+ if (IS_ERR(pdev)) {
+ DRM_ERROR("Failed to register EDID firmware platform device "
+ "for connector \"%s\"\n", connector_name);
+ return ERR_CAST(pdev);
+ }
+
+ return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char 
*name,
    const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
*connector, const char *name,
   fwdata = generic_edid[builtin];
   fwsize = sizeof(generic_edid[builtin]);
  } else {
- struct platform_device *pdev;
+ struct platform_device *pdev = connector->edid_load_pdev;
   int err;

- pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
- if (IS_ERR(pdev)) {
- DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
- return ERR_CAST(pdev);
+ if (WARN_ON(!pdev)) {
+ pdev = edid_pdev(connector_name);
+ if (IS_ERR(pdev))
+ return ERR_CAST(pdev);
+ connector->edid_load_pdev = pdev;
   }

   err = request_firmware(&fw, name, &pdev->dev);
- platform_device_unregister(pdev);
   if (err) {
    DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
       name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
*connector, const char *name,
  return edid;
 }

-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
- const char *connector_name = connector->name;
  char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
- struct edid *edid;

  if (edid_firmware[0] == '\0')
   return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
drm_connector *connector)
  if (*last == '\n')
   *last = '\0';

- edid = edid_load(connector, edidname, connector_name);
+ edidname = kstrdup(edidname, GFP_KERNEL);
+ if (!edidname) {
+ kfree(fwstr);
+ return ERR_PTR(-ENOMEM);
+ }
+
  kfree(fwstr);
+ return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+ const char *connector_name = connector->name;
+ const char *edidname = edid_name(connector_name);
+ struct platform_device *pdev;
+ int err;
+
+ if (IS_ERR(edidname))
+ return;
+
+ if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+ kfree(edidname);
+ return;
+ }
+
+ pdev = edid_pdev(connector_name);
+ if (IS_ERR(pdev)) {
+ kfree(edidname);
+ return;
+ }
+ connector->edid_load_pdev = pdev;
+
+ err = firmware_request_cache(&pdev->dev, edidname);
+ if (err)
+ DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+ edidname, err);
+
+ kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+ const char *connector_name = connector->name;
+ const char *edidname = edid_name(connector_name);
+ struct edid *edid;
+
+ if (IS_ERR(edidname))
+ return ERR_CAST(edidname);
+
+ edid = edid_load(connector, edidname, connector_name);
+ kfree(edidname);

  return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
   */
  struct i2c_adapter *ddc;

+ /**
+ * @edid_load_pdev: Platform device for loading EDID via firmware.
+ *
+ * The platform device is registered in drm_connector_init() in case a
+ * custom EDID firmware is used with `edid_firmware` parameter. 
Otherwise,
+ * it is set to NULL.
+ *
+ * Platform device is unregistered in drm_connector_cleanup() if it
+ * is not NULL.
+ */
+ struct platform_device *edid_load_pdev;
+
  /**
   * @null_edid_counter: track sinks that give us all zeros for the 
EDID.
   * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..b54cea3f1107 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
*connector,
         const struct drm_display_mode *mode);

 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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 void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.36.1

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

[Matthieu CHARETTE]

Sorry, I forgot to add a tag.
---
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061

On Wed, Jul 6 2022 at 04:58:08 PM +0200, Matthieu CHARETTE 
<matthieu.charette@gmail.com> 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.
> 
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
> drivers/gpu/drm/drm_connector.c | 9 ++++
> drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
> include/drm/drm_connector.h | 12 +++++
> include/drm/drm_edid.h | 3 ++
> 4 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c 
> b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_privacy_screen_consumer.h>
> #include <drm/drm_sysfs.h>
> 
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
> 
> #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
> 
>  drm_connector_get_cmdline_mode(connector);
> 
> + connector->edid_load_pdev = NULL;
> + drm_cache_edid_firmware(connector);
> +
>  /* We should add connectors at the end to avoid upsetting the 
> connector
>   * index too much.
>   */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector 
> *connector)
>   connector->tile_group = NULL;
>  }
> 
> + if (connector->edid_load_pdev) {
> + platform_device_unregister(connector->edid_load_pdev);
> + connector->edid_load_pdev = NULL;
> + }
> +
>  list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>   drm_mode_remove(connector, mode);
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c 
> b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..5a82be9917ec 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
> data_size)
>  return (edid[0x7e] + 1) * EDID_LENGTH;
> }
> 
> +static struct platform_device *edid_pdev(const char *connector_name)
> +{
> + struct platform_device *pdev = 
> platform_device_register_simple(connector_name, -1, NULL, 0);
> +
> + if (IS_ERR(pdev)) {
> + DRM_ERROR("Failed to register EDID firmware platform device "
> + "for connector \"%s\"\n", connector_name);
> + return ERR_CAST(pdev);
> + }
> +
> + return pdev;
> +}
> +
> static void *edid_load(struct drm_connector *connector, const char 
> *name,
>    const char *connector_name)
> {
> @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
> *connector, const char *name,
>   fwdata = generic_edid[builtin];
>   fwsize = sizeof(generic_edid[builtin]);
>  } else {
> - struct platform_device *pdev;
> + struct platform_device *pdev = connector->edid_load_pdev;
>   int err;
> 
> - pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> - return ERR_CAST(pdev);
> + if (WARN_ON(!pdev)) {
> + pdev = edid_pdev(connector_name);
> + if (IS_ERR(pdev))
> + return ERR_CAST(pdev);
> + connector->edid_load_pdev = pdev;
>   }
> 
>   err = request_firmware(&fw, name, &pdev->dev);
> - platform_device_unregister(pdev);
>   if (err) {
>    DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>       name, err);
> @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
> *connector, const char *name,
>  return edid;
> }
> 
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +static char *edid_name(const char *connector_name)
> {
> - const char *connector_name = connector->name;
>  char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> - struct edid *edid;
> 
>  if (edid_firmware[0] == '\0')
>   return ERR_PTR(-ENOENT);
> @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
> drm_connector *connector)
>  if (*last == '\n')
>   *last = '\0';
> 
> - edid = edid_load(connector, edidname, connector_name);
> + edidname = kstrdup(edidname, GFP_KERNEL);
> + if (!edidname) {
> + kfree(fwstr);
> + return ERR_PTR(-ENOMEM);
> + }
> +
>  kfree(fwstr);
> + return edidname;
> +}
> +
> +void drm_cache_edid_firmware(struct drm_connector *connector)
> +{
> + const char *connector_name = connector->name;
> + const char *edidname = edid_name(connector_name);
> + struct platform_device *pdev;
> + int err;
> +
> + if (IS_ERR(edidname))
> + return;
> +
> + if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
> + kfree(edidname);
> + return;
> + }
> +
> + pdev = edid_pdev(connector_name);
> + if (IS_ERR(pdev)) {
> + kfree(edidname);
> + return;
> + }
> + connector->edid_load_pdev = pdev;
> +
> + err = firmware_request_cache(&pdev->dev, edidname);
> + if (err)
> + DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
> + edidname, err);
> +
> + kfree(edidname);
> +}
> +
> +struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +{
> + const char *connector_name = connector->name;
> + const char *edidname = edid_name(connector_name);
> + struct edid *edid;
> +
> + if (IS_ERR(edidname))
> + return ERR_CAST(edidname);
> +
> + edid = edid_load(connector, edidname, connector_name);
> + kfree(edidname);
> 
>  return edid;
> }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 3ac4bf87f257..47c84741517e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1573,6 +1573,18 @@ struct drm_connector {
>   */
>  struct i2c_adapter *ddc;
> 
> + /**
> + * @edid_load_pdev: Platform device for loading EDID via firmware.
> + *
> + * The platform device is registered in drm_connector_init() in case 
> a
> + * custom EDID firmware is used with `edid_firmware` parameter. 
> Otherwise,
> + * it is set to NULL.
> + *
> + * Platform device is unregistered in drm_connector_cleanup() if it
> + * is not NULL.
> + */
> + struct platform_device *edid_load_pdev;
> +
>  /**
>   * @null_edid_counter: track sinks that give us all zeros for the 
> EDID.
>   * Needed to workaround some HW bugs where we get all 0s
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b2756753370b..b54cea3f1107 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
> *connector,
>         const struct drm_display_mode *mode);
> 
> #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> +void drm_cache_edid_firmware(struct drm_connector *connector);
> struct edid *drm_load_edid_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 void
> +drm_cache_edid_firmware(struct drm_connector *connector);
> static inline struct edid *
> drm_load_edid_firmware(struct drm_connector *connector)
> {
> --
> 2.36.1
> 
> 
> 

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

[Matthieu CHARETTE]

Sorry, I forgot to add a tag.
---
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061

On Wed, Jul 6 2022 at 04:58:08 PM +0200, Matthieu CHARETTE 
<matthieu.charette@gmail.com> 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.
> 
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
> drivers/gpu/drm/drm_connector.c | 9 ++++
> drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
> include/drm/drm_connector.h | 12 +++++
> include/drm/drm_edid.h | 3 ++
> 4 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c 
> b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_privacy_screen_consumer.h>
> #include <drm/drm_sysfs.h>
> 
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
> 
> #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
> 
>  drm_connector_get_cmdline_mode(connector);
> 
> + connector->edid_load_pdev = NULL;
> + drm_cache_edid_firmware(connector);
> +
>  /* We should add connectors at the end to avoid upsetting the 
> connector
>   * index too much.
>   */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector 
> *connector)
>   connector->tile_group = NULL;
>  }
> 
> + if (connector->edid_load_pdev) {
> + platform_device_unregister(connector->edid_load_pdev);
> + connector->edid_load_pdev = NULL;
> + }
> +
>  list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>   drm_mode_remove(connector, mode);
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c 
> b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..5a82be9917ec 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
> data_size)
>  return (edid[0x7e] + 1) * EDID_LENGTH;
> }
> 
> +static struct platform_device *edid_pdev(const char *connector_name)
> +{
> + struct platform_device *pdev = 
> platform_device_register_simple(connector_name, -1, NULL, 0);
> +
> + if (IS_ERR(pdev)) {
> + DRM_ERROR("Failed to register EDID firmware platform device "
> + "for connector \"%s\"\n", connector_name);
> + return ERR_CAST(pdev);
> + }
> +
> + return pdev;
> +}
> +
> static void *edid_load(struct drm_connector *connector, const char 
> *name,
>    const char *connector_name)
> {
> @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
> *connector, const char *name,
>   fwdata = generic_edid[builtin];
>   fwsize = sizeof(generic_edid[builtin]);
>  } else {
> - struct platform_device *pdev;
> + struct platform_device *pdev = connector->edid_load_pdev;
>   int err;
> 
> - pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> - if (IS_ERR(pdev)) {
> - DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> - return ERR_CAST(pdev);
> + if (WARN_ON(!pdev)) {
> + pdev = edid_pdev(connector_name);
> + if (IS_ERR(pdev))
> + return ERR_CAST(pdev);
> + connector->edid_load_pdev = pdev;
>   }
> 
>   err = request_firmware(&fw, name, &pdev->dev);
> - platform_device_unregister(pdev);
>   if (err) {
>    DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>       name, err);
> @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
> *connector, const char *name,
>  return edid;
> }
> 
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +static char *edid_name(const char *connector_name)
> {
> - const char *connector_name = connector->name;
>  char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> - struct edid *edid;
> 
>  if (edid_firmware[0] == '\0')
>   return ERR_PTR(-ENOENT);
> @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
> drm_connector *connector)
>  if (*last == '\n')
>   *last = '\0';
> 
> - edid = edid_load(connector, edidname, connector_name);
> + edidname = kstrdup(edidname, GFP_KERNEL);
> + if (!edidname) {
> + kfree(fwstr);
> + return ERR_PTR(-ENOMEM);
> + }
> +
>  kfree(fwstr);
> + return edidname;
> +}
> +
> +void drm_cache_edid_firmware(struct drm_connector *connector)
> +{
> + const char *connector_name = connector->name;
> + const char *edidname = edid_name(connector_name);
> + struct platform_device *pdev;
> + int err;
> +
> + if (IS_ERR(edidname))
> + return;
> +
> + if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
> + kfree(edidname);
> + return;
> + }
> +
> + pdev = edid_pdev(connector_name);
> + if (IS_ERR(pdev)) {
> + kfree(edidname);
> + return;
> + }
> + connector->edid_load_pdev = pdev;
> +
> + err = firmware_request_cache(&pdev->dev, edidname);
> + if (err)
> + DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
> + edidname, err);
> +
> + kfree(edidname);
> +}
> +
> +struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +{
> + const char *connector_name = connector->name;
> + const char *edidname = edid_name(connector_name);
> + struct edid *edid;
> +
> + if (IS_ERR(edidname))
> + return ERR_CAST(edidname);
> +
> + edid = edid_load(connector, edidname, connector_name);
> + kfree(edidname);
> 
>  return edid;
> }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 3ac4bf87f257..47c84741517e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1573,6 +1573,18 @@ struct drm_connector {
>   */
>  struct i2c_adapter *ddc;
> 
> + /**
> + * @edid_load_pdev: Platform device for loading EDID via firmware.
> + *
> + * The platform device is registered in drm_connector_init() in case 
> a
> + * custom EDID firmware is used with `edid_firmware` parameter. 
> Otherwise,
> + * it is set to NULL.
> + *
> + * Platform device is unregistered in drm_connector_cleanup() if it
> + * is not NULL.
> + */
> + struct platform_device *edid_load_pdev;
> +
>  /**
>   * @null_edid_counter: track sinks that give us all zeros for the 
> EDID.
>   * Needed to workaround some HW bugs where we get all 0s
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b2756753370b..b54cea3f1107 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
> *connector,
>         const struct drm_display_mode *mode);
> 
> #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> +void drm_cache_edid_firmware(struct drm_connector *connector);
> struct edid *drm_load_edid_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 void
> +drm_cache_edid_firmware(struct drm_connector *connector);
> static inline struct edid *
> drm_load_edid_firmware(struct drm_connector *connector)
> {
> --
> 2.36.1
> 
> 
> 

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

[André Almeida]

Hi Matthieu,

Thanks for your patch.

Às 11:58 de 06/07/22, Matthieu CHARETTE escreveu:
> 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.
> 
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
> drivers/gpu/drm/drm_connector.c | 9 ++++
> drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
> include/drm/drm_connector.h | 12 +++++
> include/drm/drm_edid.h | 3 ++
> 4 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c
> b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_privacy_screen_consumer.h>
> #include <drm/drm_sysfs.h>
> 
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
> 
> #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
> 
>  drm_connector_get_cmdline_mode(connector);
> 
> + connector->edid_load_pdev = NULL;
> + drm_cache_edid_firmware(connector);
> +
>  /* We should add connectors at the end to avoid upsetting the connector
>   * index too much.
>   */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector
> *connector)
>   connector->tile_group = NULL;
>  }
> 
> + if (connector->edid_load_pdev) {
> + platform_device_unregister(connector->edid_load_pdev);
> + connector->edid_load_pdev = NULL;
> + }
> +

The indentation of your patch is wrong in different places, like in this
if here. It should be like

+ if (connector->edid_load_pdev) {
+ 	platform_device_unregister(connector->edid_load_pdev);
+ 	connector->edid_load_pdev = NULL;
+ }

./scripts/checkpatch.pl can help you detect those issues for your v2

Thanks,
	André

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

[André Almeida]

Hi Matthieu,

Thanks for your patch.

Às 11:58 de 06/07/22, Matthieu CHARETTE escreveu:
> 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.
> 
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
> drivers/gpu/drm/drm_connector.c | 9 ++++
> drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
> include/drm/drm_connector.h | 12 +++++
> include/drm/drm_edid.h | 3 ++
> 4 files changed, 94 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c
> b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_privacy_screen_consumer.h>
> #include <drm/drm_sysfs.h>
> 
> +#include <linux/platform_device.h>
> #include <linux/uaccess.h>
> 
> #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
> 
>  drm_connector_get_cmdline_mode(connector);
> 
> + connector->edid_load_pdev = NULL;
> + drm_cache_edid_firmware(connector);
> +
>  /* We should add connectors at the end to avoid upsetting the connector
>   * index too much.
>   */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector
> *connector)
>   connector->tile_group = NULL;
>  }
> 
> + if (connector->edid_load_pdev) {
> + platform_device_unregister(connector->edid_load_pdev);
> + connector->edid_load_pdev = NULL;
> + }
> +

The indentation of your patch is wrong in different places, like in this
if here. It should be like

+ if (connector->edid_load_pdev) {
+ 	platform_device_unregister(connector->edid_load_pdev);
+ 	connector->edid_load_pdev = NULL;
+ }

./scripts/checkpatch.pl can help you detect those issues for your v2

Thanks,
	André

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

[Matthieu CHARETTE]

Hi,

Sorry, my email client removed every tab. I will send the v2 in a new 
thread.

Thanks.

Matthieu

On Thu, Jul 14 2022 at 11:23:10 AM -0300, André Almeida 
<andrealmeid@igalia.com> wrote:
> Hi Matthieu,
> 
> Thanks for your patch.
> 
> Às 11:58 de 06/07/22, Matthieu CHARETTE escreveu:
>>  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.
>> 
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>  ---
>>  drivers/gpu/drm/drm_connector.c | 9 ++++
>>  drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>  include/drm/drm_connector.h | 12 +++++
>>  include/drm/drm_edid.h | 3 ++
>>  4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c
>>  b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>  #include <drm/drm_privacy_screen_consumer.h>
>>  #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>  #include <linux/uaccess.h>
>> 
>>  #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   drm_connector_get_cmdline_mode(connector);
>> 
>>  + connector->edid_load_pdev = NULL;
>>  + drm_cache_edid_firmware(connector);
>>  +
>>   /* We should add connectors at the end to avoid upsetting the 
>> connector
>>    * index too much.
>>    */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector
>>  *connector)
>>    connector->tile_group = NULL;
>>   }
>> 
>>  + if (connector->edid_load_pdev) {
>>  + platform_device_unregister(connector->edid_load_pdev);
>>  + connector->edid_load_pdev = NULL;
>>  + }
>>  +
> 
> The indentation of your patch is wrong in different places, like in 
> this
> if here. It should be like
> 
> + if (connector->edid_load_pdev) {
> + 	platform_device_unregister(connector->edid_load_pdev);
> + 	connector->edid_load_pdev = NULL;
> + }
> 
> ./scripts/checkpatch.pl can help you detect those issues for your v2
> 
> Thanks,
> 	André

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

[Matthieu CHARETTE]

Hi,

Sorry, my email client removed every tab. I will send the v2 in a new 
thread.

Thanks.

Matthieu

On Thu, Jul 14 2022 at 11:23:10 AM -0300, André Almeida 
<andrealmeid@igalia.com> wrote:
> Hi Matthieu,
> 
> Thanks for your patch.
> 
> Às 11:58 de 06/07/22, Matthieu CHARETTE escreveu:
>>  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.
>> 
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>  ---
>>  drivers/gpu/drm/drm_connector.c | 9 ++++
>>  drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>  include/drm/drm_connector.h | 12 +++++
>>  include/drm/drm_edid.h | 3 ++
>>  4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c
>>  b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>  #include <drm/drm_privacy_screen_consumer.h>
>>  #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>  #include <linux/uaccess.h>
>> 
>>  #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   drm_connector_get_cmdline_mode(connector);
>> 
>>  + connector->edid_load_pdev = NULL;
>>  + drm_cache_edid_firmware(connector);
>>  +
>>   /* We should add connectors at the end to avoid upsetting the 
>> connector
>>    * index too much.
>>    */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector
>>  *connector)
>>    connector->tile_group = NULL;
>>   }
>> 
>>  + if (connector->edid_load_pdev) {
>>  + platform_device_unregister(connector->edid_load_pdev);
>>  + connector->edid_load_pdev = NULL;
>>  + }
>>  +
> 
> The indentation of your patch is wrong in different places, like in 
> this
> if here. It should be like
> 
> + if (connector->edid_load_pdev) {
> + 	platform_device_unregister(connector->edid_load_pdev);
> + 	connector->edid_load_pdev = NULL;
> + }
> 
> ./scripts/checkpatch.pl can help you detect those issues for your v2
> 
> Thanks,
> 	André

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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>
---
 drivers/gpu/drm/drm_connector.c |  9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h     | 12 +++++
 include/drm/drm_edid.h          |  3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>
 
+#include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	connector->edid_load_pdev = NULL;
+	drm_cache_edid_firmware(connector);
+
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much.
 	 */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->tile_group = NULL;
 	}
 
+	if (connector->edid_load_pdev) {
+		platform_device_unregister(connector->edid_load_pdev);
+		connector->edid_load_pdev = NULL;
+	}
+
 	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
 		drm_mode_remove(connector, mode);
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static struct platform_device *edid_pdev(const char *connector_name)
+{
+	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+
+	if (IS_ERR(pdev)) {
+		DRM_ERROR("Failed to register EDID firmware platform device "
+			"for connector \"%s\"\n", connector_name);
+		return ERR_CAST(pdev);
+	}
+
+	return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		fwdata = generic_edid[builtin];
 		fwsize = sizeof(generic_edid[builtin]);
 	} else {
-		struct platform_device *pdev;
+		struct platform_device *pdev = connector->edid_load_pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
-		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
-			return ERR_CAST(pdev);
+		if (WARN_ON(!pdev)) {
+			pdev = edid_pdev(connector_name);
+			if (IS_ERR(pdev))
+				return ERR_CAST(pdev);
+			connector->edid_load_pdev = pdev;
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
-		platform_device_unregister(pdev);
 		if (err) {
 			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
 				  name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edidname = kstrdup(edidname, GFP_KERNEL);
+	if (!edidname) {
+		kfree(fwstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	kfree(fwstr);
+	return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct platform_device *pdev;
+	int err;
+
+	if (IS_ERR(edidname))
+		return;
+
+	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+		kfree(edidname);
+		return;
+	}
+
+	pdev = edid_pdev(connector_name);
+	if (IS_ERR(pdev)) {
+		kfree(edidname);
+		return;
+	}
+	connector->edid_load_pdev = pdev;
+
+	err = firmware_request_cache(&pdev->dev, edidname);
+	if (err)
+		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+			edidname, err);
+
+	kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct edid *edid;
+
+	if (IS_ERR(edidname))
+		return ERR_CAST(edidname);
+
+	edid = edid_load(connector, edidname, connector_name);
+	kfree(edidname);
 
 	return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
 	 */
 	struct i2c_adapter *ddc;
 
+	/**
+	 * @edid_load_pdev: Platform device for loading EDID via firmware.
+	 *
+	 * The platform device is registered in drm_connector_init() in case a
+	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
+	 * it is set to NULL.
+	 *
+	 * Platform device is unregistered in drm_connector_cleanup() if it
+	 * is not NULL.
+	 */
+	struct platform_device *edid_load_pdev;
+
 	/**
 	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
 	 * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..b54cea3f1107 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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 void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.36.1

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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>
---
 drivers/gpu/drm/drm_connector.c |  9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h     | 12 +++++
 include/drm/drm_edid.h          |  3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>
 
+#include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	connector->edid_load_pdev = NULL;
+	drm_cache_edid_firmware(connector);
+
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much.
 	 */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->tile_group = NULL;
 	}
 
+	if (connector->edid_load_pdev) {
+		platform_device_unregister(connector->edid_load_pdev);
+		connector->edid_load_pdev = NULL;
+	}
+
 	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
 		drm_mode_remove(connector, mode);
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static struct platform_device *edid_pdev(const char *connector_name)
+{
+	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+
+	if (IS_ERR(pdev)) {
+		DRM_ERROR("Failed to register EDID firmware platform device "
+			"for connector \"%s\"\n", connector_name);
+		return ERR_CAST(pdev);
+	}
+
+	return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		fwdata = generic_edid[builtin];
 		fwsize = sizeof(generic_edid[builtin]);
 	} else {
-		struct platform_device *pdev;
+		struct platform_device *pdev = connector->edid_load_pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
-		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
-			return ERR_CAST(pdev);
+		if (WARN_ON(!pdev)) {
+			pdev = edid_pdev(connector_name);
+			if (IS_ERR(pdev))
+				return ERR_CAST(pdev);
+			connector->edid_load_pdev = pdev;
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
-		platform_device_unregister(pdev);
 		if (err) {
 			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
 				  name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edidname = kstrdup(edidname, GFP_KERNEL);
+	if (!edidname) {
+		kfree(fwstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	kfree(fwstr);
+	return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct platform_device *pdev;
+	int err;
+
+	if (IS_ERR(edidname))
+		return;
+
+	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+		kfree(edidname);
+		return;
+	}
+
+	pdev = edid_pdev(connector_name);
+	if (IS_ERR(pdev)) {
+		kfree(edidname);
+		return;
+	}
+	connector->edid_load_pdev = pdev;
+
+	err = firmware_request_cache(&pdev->dev, edidname);
+	if (err)
+		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+			edidname, err);
+
+	kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct edid *edid;
+
+	if (IS_ERR(edidname))
+		return ERR_CAST(edidname);
+
+	edid = edid_load(connector, edidname, connector_name);
+	kfree(edidname);
 
 	return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
 	 */
 	struct i2c_adapter *ddc;
 
+	/**
+	 * @edid_load_pdev: Platform device for loading EDID via firmware.
+	 *
+	 * The platform device is registered in drm_connector_init() in case a
+	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
+	 * it is set to NULL.
+	 *
+	 * Platform device is unregistered in drm_connector_cleanup() if it
+	 * is not NULL.
+	 */
+	struct platform_device *edid_load_pdev;
+
 	/**
 	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
 	 * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..b54cea3f1107 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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 void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.36.1

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc6]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220717/202207170211.AaMpUwPs-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_connector.c:25:
>> include/drm/drm_edid.h:387:1: warning: 'drm_cache_edid_firmware' used but never defined
     387 | drm_cache_edid_firmware(struct drm_connector *connector);
         | ^~~~~~~~~~~~~~~~~~~~~~~


vim +/drm_cache_edid_firmware +387 include/drm/drm_edid.h

   374	
   375	int drm_edid_to_sad(const struct edid *edid, struct cea_sad **sads);
   376	int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb);
   377	int drm_av_sync_delay(struct drm_connector *connector,
   378			      const struct drm_display_mode *mode);
   379	
   380	#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
   381	void drm_cache_edid_firmware(struct drm_connector *connector);
   382	struct edid *drm_load_edid_firmware(struct drm_connector *connector);
   383	int __drm_set_edid_firmware_path(const char *path);
   384	int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
   385	#else
   386	static inline void
 > 387	drm_cache_edid_firmware(struct drm_connector *connector);
   388	static inline struct edid *
   389	drm_load_edid_firmware(struct drm_connector *connector)
   390	{
   391		return ERR_PTR(-ENOENT);
   392	}
   393	#endif
   394	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc6]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: i386-randconfig-a003 (https://download.01.org/0day-ci/archive/20220717/202207170211.AaMpUwPs-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_connector.c:25:
>> include/drm/drm_edid.h:387:1: warning: 'drm_cache_edid_firmware' used but never defined
     387 | drm_cache_edid_firmware(struct drm_connector *connector);
         | ^~~~~~~~~~~~~~~~~~~~~~~


vim +/drm_cache_edid_firmware +387 include/drm/drm_edid.h

   374	
   375	int drm_edid_to_sad(const struct edid *edid, struct cea_sad **sads);
   376	int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb);
   377	int drm_av_sync_delay(struct drm_connector *connector,
   378			      const struct drm_display_mode *mode);
   379	
   380	#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
   381	void drm_cache_edid_firmware(struct drm_connector *connector);
   382	struct edid *drm_load_edid_firmware(struct drm_connector *connector);
   383	int __drm_set_edid_firmware_path(const char *path);
   384	int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
   385	#else
   386	static inline void
 > 387	drm_cache_edid_firmware(struct drm_connector *connector);
   388	static inline struct edid *
   389	drm_load_edid_firmware(struct drm_connector *connector)
   390	{
   391		return ERR_PTR(-ENOENT);
   392	}
   393	#endif
   394	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.19-rc6]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: x86_64-randconfig-a015 (https://download.01.org/0day-ci/archive/20220717/202207170521.UXrz8URh-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: certs/system_keyring.o: in function `load_system_certificate_list':
   system_keyring.c:(.init.text+0x149): undefined reference to `x509_load_certificate_list'
   ld: drivers/gpu/drm/drm_connector.o: in function `drm_connector_init':
>> drm_connector.c:(.text+0x21ce): undefined reference to `drm_cache_edid_firmware'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.19-rc6]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: x86_64-randconfig-a015 (https://download.01.org/0day-ci/archive/20220717/202207170521.UXrz8URh-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: certs/system_keyring.o: in function `load_system_certificate_list':
   system_keyring.c:(.init.text+0x149): undefined reference to `x509_load_certificate_list'
   ld: drivers/gpu/drm/drm_connector.o: in function `drm_connector_init':
>> drm_connector.c:(.text+0x21ce): undefined reference to `drm_cache_edid_firmware'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.19-rc6]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: parisc-randconfig-r023-20220717 (https://download.01.org/0day-ci/archive/20220717/202207170636.Yv3aXya1-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/gpu/drm/drm_connector.o: in function `.LC507':
>> drm_connector.c:(.rodata.cst4+0x1f8): undefined reference to `drm_cache_edid_firmware'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.19-rc6]
[also build test ERROR on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: parisc-randconfig-r023-20220717 (https://download.01.org/0day-ci/archive/20220717/202207170636.Yv3aXya1-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   hppa-linux-ld: drivers/gpu/drm/drm_connector.o: in function `.LC507':
>> drm_connector.c:(.rodata.cst4+0x1f8): undefined reference to `drm_cache_edid_firmware'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc6]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: loongarch-randconfig-s052-20220715 (https://download.01.org/0day-ci/archive/20220717/202207172035.mtErdlaw-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/gpu/drm/drm_connector.c: note: in included file:
>> include/drm/drm_edid.h:387:24: sparse: sparse: marked inline, but without a definition

vim +387 include/drm/drm_edid.h

   374	
   375	int drm_edid_to_sad(const struct edid *edid, struct cea_sad **sads);
   376	int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb);
   377	int drm_av_sync_delay(struct drm_connector *connector,
   378			      const struct drm_display_mode *mode);
   379	
   380	#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
   381	void drm_cache_edid_firmware(struct drm_connector *connector);
   382	struct edid *drm_load_edid_firmware(struct drm_connector *connector);
   383	int __drm_set_edid_firmware_path(const char *path);
   384	int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
   385	#else
   386	static inline void
 > 387	drm_cache_edid_firmware(struct drm_connector *connector);
   388	static inline struct edid *
   389	drm_load_edid_firmware(struct drm_connector *connector)
   390	{
   391		return ERR_PTR(-ENOENT);
   392	}
   393	#endif
   394	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Matthieu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19-rc6]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
base:    32346491ddf24599decca06190ebca03ff9de7f8
config: loongarch-randconfig-s052-20220715 (https://download.01.org/0day-ci/archive/20220717/202207172035.mtErdlaw-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/e583aaf4f6464add35f2350c728d80a3fe790638
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Matthieu-CHARETTE/drm-Fix-EDID-firmware-load-on-resume/20220716-214028
        git checkout e583aaf4f6464add35f2350c728d80a3fe790638
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/gpu/drm/drm_connector.c: note: in included file:
>> include/drm/drm_edid.h:387:24: sparse: sparse: marked inline, but without a definition

vim +387 include/drm/drm_edid.h

   374	
   375	int drm_edid_to_sad(const struct edid *edid, struct cea_sad **sads);
   376	int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb);
   377	int drm_av_sync_delay(struct drm_connector *connector,
   378			      const struct drm_display_mode *mode);
   379	
   380	#ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
   381	void drm_cache_edid_firmware(struct drm_connector *connector);
   382	struct edid *drm_load_edid_firmware(struct drm_connector *connector);
   383	int __drm_set_edid_firmware_path(const char *path);
   384	int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
   385	#else
   386	static inline void
 > 387	drm_cache_edid_firmware(struct drm_connector *connector);
   388	static inline struct edid *
   389	drm_load_edid_firmware(struct drm_connector *connector)
   390	{
   391		return ERR_PTR(-ENOENT);
   392	}
   393	#endif
   394	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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>
---
 drivers/gpu/drm/drm_connector.c |  9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h     | 12 +++++
 include/drm/drm_edid.h          |  3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>
 
+#include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	connector->edid_load_pdev = NULL;
+	drm_cache_edid_firmware(connector);
+
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much.
 	 */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->tile_group = NULL;
 	}
 
+	if (connector->edid_load_pdev) {
+		platform_device_unregister(connector->edid_load_pdev);
+		connector->edid_load_pdev = NULL;
+	}
+
 	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
 		drm_mode_remove(connector, mode);
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static struct platform_device *edid_pdev(const char *connector_name)
+{
+	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+
+	if (IS_ERR(pdev)) {
+		DRM_ERROR("Failed to register EDID firmware platform device "
+			"for connector \"%s\"\n", connector_name);
+		return ERR_CAST(pdev);
+	}
+
+	return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		fwdata = generic_edid[builtin];
 		fwsize = sizeof(generic_edid[builtin]);
 	} else {
-		struct platform_device *pdev;
+		struct platform_device *pdev = connector->edid_load_pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
-		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
-			return ERR_CAST(pdev);
+		if (WARN_ON(!pdev)) {
+			pdev = edid_pdev(connector_name);
+			if (IS_ERR(pdev))
+				return ERR_CAST(pdev);
+			connector->edid_load_pdev = pdev;
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
-		platform_device_unregister(pdev);
 		if (err) {
 			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
 				  name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edidname = kstrdup(edidname, GFP_KERNEL);
+	if (!edidname) {
+		kfree(fwstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	kfree(fwstr);
+	return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct platform_device *pdev;
+	int err;
+
+	if (IS_ERR(edidname))
+		return;
+
+	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+		kfree(edidname);
+		return;
+	}
+
+	pdev = edid_pdev(connector_name);
+	if (IS_ERR(pdev)) {
+		kfree(edidname);
+		return;
+	}
+	connector->edid_load_pdev = pdev;
+
+	err = firmware_request_cache(&pdev->dev, edidname);
+	if (err)
+		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+			edidname, err);
+
+	kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct edid *edid;
+
+	if (IS_ERR(edidname))
+		return ERR_CAST(edidname);
+
+	edid = edid_load(connector, edidname, connector_name);
+	kfree(edidname);
 
 	return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
 	 */
 	struct i2c_adapter *ddc;
 
+	/**
+	 * @edid_load_pdev: Platform device for loading EDID via firmware.
+	 *
+	 * The platform device is registered in drm_connector_init() in case a
+	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
+	 * it is set to NULL.
+	 *
+	 * Platform device is unregistered in drm_connector_cleanup() if it
+	 * is not NULL.
+	 */
+	struct platform_device *edid_load_pdev;
+
 	/**
 	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
 	 * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..e907c928a35d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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
+inline void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.37.1

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 7136 bytes --]

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>
---
 drivers/gpu/drm/drm_connector.c |  9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h     | 12 +++++
 include/drm/drm_edid.h          |  3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>
 
+#include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	connector->edid_load_pdev = NULL;
+	drm_cache_edid_firmware(connector);
+
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much.
 	 */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->tile_group = NULL;
 	}
 
+	if (connector->edid_load_pdev) {
+		platform_device_unregister(connector->edid_load_pdev);
+		connector->edid_load_pdev = NULL;
+	}
+
 	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
 		drm_mode_remove(connector, mode);
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static struct platform_device *edid_pdev(const char *connector_name)
+{
+	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+
+	if (IS_ERR(pdev)) {
+		DRM_ERROR("Failed to register EDID firmware platform device "
+			"for connector \"%s\"\n", connector_name);
+		return ERR_CAST(pdev);
+	}
+
+	return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		fwdata = generic_edid[builtin];
 		fwsize = sizeof(generic_edid[builtin]);
 	} else {
-		struct platform_device *pdev;
+		struct platform_device *pdev = connector->edid_load_pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
-		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
-			return ERR_CAST(pdev);
+		if (WARN_ON(!pdev)) {
+			pdev = edid_pdev(connector_name);
+			if (IS_ERR(pdev))
+				return ERR_CAST(pdev);
+			connector->edid_load_pdev = pdev;
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
-		platform_device_unregister(pdev);
 		if (err) {
 			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
 				  name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edidname = kstrdup(edidname, GFP_KERNEL);
+	if (!edidname) {
+		kfree(fwstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	kfree(fwstr);
+	return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct platform_device *pdev;
+	int err;
+
+	if (IS_ERR(edidname))
+		return;
+
+	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+		kfree(edidname);
+		return;
+	}
+
+	pdev = edid_pdev(connector_name);
+	if (IS_ERR(pdev)) {
+		kfree(edidname);
+		return;
+	}
+	connector->edid_load_pdev = pdev;
+
+	err = firmware_request_cache(&pdev->dev, edidname);
+	if (err)
+		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+			edidname, err);
+
+	kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct edid *edid;
+
+	if (IS_ERR(edidname))
+		return ERR_CAST(edidname);
+
+	edid = edid_load(connector, edidname, connector_name);
+	kfree(edidname);
 
 	return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
 	 */
 	struct i2c_adapter *ddc;
 
+	/**
+	 * @edid_load_pdev: Platform device for loading EDID via firmware.
+	 *
+	 * The platform device is registered in drm_connector_init() in case a
+	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
+	 * it is set to NULL.
+	 *
+	 * Platform device is unregistered in drm_connector_cleanup() if it
+	 * is not NULL.
+	 */
+	struct platform_device *edid_load_pdev;
+
 	/**
 	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
 	 * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..e907c928a35d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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
+inline void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.37.1

[PATCH] drm: Fix EDID firmware load on resume

[Matthieu CHARETTE]

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>
---
 drivers/gpu/drm/drm_connector.c |  9 ++++
 drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
 include/drm/drm_connector.h     | 12 +++++
 include/drm/drm_edid.h          |  3 ++
 4 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..e8819ebf1c4b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -31,6 +31,7 @@
 #include <drm/drm_privacy_screen_consumer.h>
 #include <drm/drm_sysfs.h>
 
+#include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
 #include "drm_crtc_internal.h"
@@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
 
 	drm_connector_get_cmdline_mode(connector);
 
+	connector->edid_load_pdev = NULL;
+	drm_cache_edid_firmware(connector);
+
 	/* We should add connectors at the end to avoid upsetting the connector
 	 * index too much.
 	 */
@@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
 		connector->tile_group = NULL;
 	}
 
+	if (connector->edid_load_pdev) {
+		platform_device_unregister(connector->edid_load_pdev);
+		connector->edid_load_pdev = NULL;
+	}
+
 	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
 		drm_mode_remove(connector, mode);
 
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..5a82be9917ec 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
 	return (edid[0x7e] + 1) * EDID_LENGTH;
 }
 
+static struct platform_device *edid_pdev(const char *connector_name)
+{
+	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
+
+	if (IS_ERR(pdev)) {
+		DRM_ERROR("Failed to register EDID firmware platform device "
+			"for connector \"%s\"\n", connector_name);
+		return ERR_CAST(pdev);
+	}
+
+	return pdev;
+}
+
 static void *edid_load(struct drm_connector *connector, const char *name,
 			const char *connector_name)
 {
@@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 		fwdata = generic_edid[builtin];
 		fwsize = sizeof(generic_edid[builtin]);
 	} else {
-		struct platform_device *pdev;
+		struct platform_device *pdev = connector->edid_load_pdev;
 		int err;
 
-		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
-		if (IS_ERR(pdev)) {
-			DRM_ERROR("Failed to register EDID firmware platform device "
-				  "for connector \"%s\"\n", connector_name);
-			return ERR_CAST(pdev);
+		if (WARN_ON(!pdev)) {
+			pdev = edid_pdev(connector_name);
+			if (IS_ERR(pdev))
+				return ERR_CAST(pdev);
+			connector->edid_load_pdev = pdev;
 		}
 
 		err = request_firmware(&fw, name, &pdev->dev);
-		platform_device_unregister(pdev);
 		if (err) {
 			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
 				  name, err);
@@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	return edid;
 }
 
-struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+static char *edid_name(const char *connector_name)
 {
-	const char *connector_name = connector->name;
 	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
-	struct edid *edid;
 
 	if (edid_firmware[0] == '\0')
 		return ERR_PTR(-ENOENT);
@@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
 	if (*last == '\n')
 		*last = '\0';
 
-	edid = edid_load(connector, edidname, connector_name);
+	edidname = kstrdup(edidname, GFP_KERNEL);
+	if (!edidname) {
+		kfree(fwstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	kfree(fwstr);
+	return edidname;
+}
+
+void drm_cache_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct platform_device *pdev;
+	int err;
+
+	if (IS_ERR(edidname))
+		return;
+
+	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
+		kfree(edidname);
+		return;
+	}
+
+	pdev = edid_pdev(connector_name);
+	if (IS_ERR(pdev)) {
+		kfree(edidname);
+		return;
+	}
+	connector->edid_load_pdev = pdev;
+
+	err = firmware_request_cache(&pdev->dev, edidname);
+	if (err)
+		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
+			edidname, err);
+
+	kfree(edidname);
+}
+
+struct edid *drm_load_edid_firmware(struct drm_connector *connector)
+{
+	const char *connector_name = connector->name;
+	const char *edidname = edid_name(connector_name);
+	struct edid *edid;
+
+	if (IS_ERR(edidname))
+		return ERR_CAST(edidname);
+
+	edid = edid_load(connector, edidname, connector_name);
+	kfree(edidname);
 
 	return edid;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..47c84741517e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1573,6 +1573,18 @@ struct drm_connector {
 	 */
 	struct i2c_adapter *ddc;
 
+	/**
+	 * @edid_load_pdev: Platform device for loading EDID via firmware.
+	 *
+	 * The platform device is registered in drm_connector_init() in case a
+	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
+	 * it is set to NULL.
+	 *
+	 * Platform device is unregistered in drm_connector_cleanup() if it
+	 * is not NULL.
+	 */
+	struct platform_device *edid_load_pdev;
+
 	/**
 	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
 	 * Needed to workaround some HW bugs where we get all 0s
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b2756753370b..e907c928a35d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
 		      const struct drm_display_mode *mode);
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
+void drm_cache_edid_firmware(struct drm_connector *connector);
 struct edid *drm_load_edid_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
+inline void
+drm_cache_edid_firmware(struct drm_connector *connector);
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
-- 
2.37.1

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

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

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]

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;
 

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

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

[Matthieu CHARETTE]

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@suse.de> 
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@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;
> 

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

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 4148 bytes --]

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@suse.de> 
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@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;
> 

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

[Matthieu CHARETTE]

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@suse.de> 
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@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;
> 

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

[Jani Nikula]

On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 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.

AFAICT this breaks changing drm.edid_firmware runtime.

BR,
Jani.

>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  9 ++++
>  drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
>  include/drm/drm_connector.h     | 12 +++++
>  include/drm/drm_edid.h          |  3 ++
>  4 files changed, 94 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/drm_sysfs.h>
>  
> +#include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> +	connector->edid_load_pdev = NULL;
> +	drm_cache_edid_firmware(connector);
> +
>  	/* We should add connectors at the end to avoid upsetting the connector
>  	 * index too much.
>  	 */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		connector->tile_group = NULL;
>  	}
>  
> +	if (connector->edid_load_pdev) {
> +		platform_device_unregister(connector->edid_load_pdev);
> +		connector->edid_load_pdev = NULL;
> +	}
> +
>  	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>  		drm_mode_remove(connector, mode);
>  
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..5a82be9917ec 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
>  	return (edid[0x7e] + 1) * EDID_LENGTH;
>  }
>  
> +static struct platform_device *edid_pdev(const char *connector_name)
> +{
> +	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> +
> +	if (IS_ERR(pdev)) {
> +		DRM_ERROR("Failed to register EDID firmware platform device "
> +			"for connector \"%s\"\n", connector_name);
> +		return ERR_CAST(pdev);
> +	}
> +
> +	return pdev;
> +}
> +
>  static void *edid_load(struct drm_connector *connector, const char *name,
>  			const char *connector_name)
>  {
> @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  		fwdata = generic_edid[builtin];
>  		fwsize = sizeof(generic_edid[builtin]);
>  	} else {
> -		struct platform_device *pdev;
> +		struct platform_device *pdev = connector->edid_load_pdev;
>  		int err;
>  
> -		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> -		if (IS_ERR(pdev)) {
> -			DRM_ERROR("Failed to register EDID firmware platform device "
> -				  "for connector \"%s\"\n", connector_name);
> -			return ERR_CAST(pdev);
> +		if (WARN_ON(!pdev)) {
> +			pdev = edid_pdev(connector_name);
> +			if (IS_ERR(pdev))
> +				return ERR_CAST(pdev);
> +			connector->edid_load_pdev = pdev;
>  		}
>  
>  		err = request_firmware(&fw, name, &pdev->dev);
> -		platform_device_unregister(pdev);
>  		if (err) {
>  			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>  				  name, err);
> @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  	return edid;
>  }
>  
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +static char *edid_name(const char *connector_name)
>  {
> -	const char *connector_name = connector->name;
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> -	struct edid *edid;
>  
>  	if (edid_firmware[0] == '\0')
>  		return ERR_PTR(-ENOENT);
> @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
>  	if (*last == '\n')
>  		*last = '\0';
>  
> -	edid = edid_load(connector, edidname, connector_name);
> +	edidname = kstrdup(edidname, GFP_KERNEL);
> +	if (!edidname) {
> +		kfree(fwstr);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	kfree(fwstr);
> +	return edidname;
> +}
> +
> +void drm_cache_edid_firmware(struct drm_connector *connector)
> +{
> +	const char *connector_name = connector->name;
> +	const char *edidname = edid_name(connector_name);
> +	struct platform_device *pdev;
> +	int err;
> +
> +	if (IS_ERR(edidname))
> +		return;
> +
> +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
> +		kfree(edidname);
> +		return;
> +	}
> +
> +	pdev = edid_pdev(connector_name);
> +	if (IS_ERR(pdev)) {
> +		kfree(edidname);
> +		return;
> +	}
> +	connector->edid_load_pdev = pdev;
> +
> +	err = firmware_request_cache(&pdev->dev, edidname);
> +	if (err)
> +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
> +			edidname, err);
> +
> +	kfree(edidname);
> +}
> +
> +struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +{
> +	const char *connector_name = connector->name;
> +	const char *edidname = edid_name(connector_name);
> +	struct edid *edid;
> +
> +	if (IS_ERR(edidname))
> +		return ERR_CAST(edidname);
> +
> +	edid = edid_load(connector, edidname, connector_name);
> +	kfree(edidname);
>  
>  	return edid;
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 3ac4bf87f257..47c84741517e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1573,6 +1573,18 @@ struct drm_connector {
>  	 */
>  	struct i2c_adapter *ddc;
>  
> +	/**
> +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
> +	 *
> +	 * The platform device is registered in drm_connector_init() in case a
> +	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
> +	 * it is set to NULL.
> +	 *
> +	 * Platform device is unregistered in drm_connector_cleanup() if it
> +	 * is not NULL.
> +	 */
> +	struct platform_device *edid_load_pdev;
> +
>  	/**
>  	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
>  	 * Needed to workaround some HW bugs where we get all 0s
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b2756753370b..e907c928a35d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> +void drm_cache_edid_firmware(struct drm_connector *connector);
>  struct edid *drm_load_edid_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
> +inline void
> +drm_cache_edid_firmware(struct drm_connector *connector);
>  static inline struct edid *
>  drm_load_edid_firmware(struct drm_connector *connector)
>  {

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 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.

AFAICT this breaks changing drm.edid_firmware runtime.

BR,
Jani.

>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
> Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
> ---
>  drivers/gpu/drm/drm_connector.c |  9 ++++
>  drivers/gpu/drm/drm_edid_load.c | 81 ++++++++++++++++++++++++++++-----
>  include/drm/drm_connector.h     | 12 +++++
>  include/drm/drm_edid.h          |  3 ++
>  4 files changed, 94 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 1c48d162c77e..e8819ebf1c4b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/drm_sysfs.h>
>  
> +#include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_connector_get_cmdline_mode(connector);
>  
> +	connector->edid_load_pdev = NULL;
> +	drm_cache_edid_firmware(connector);
> +
>  	/* We should add connectors at the end to avoid upsetting the connector
>  	 * index too much.
>  	 */
> @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  		connector->tile_group = NULL;
>  	}
>  
> +	if (connector->edid_load_pdev) {
> +		platform_device_unregister(connector->edid_load_pdev);
> +		connector->edid_load_pdev = NULL;
> +	}
> +
>  	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>  		drm_mode_remove(connector, mode);
>  
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..5a82be9917ec 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int data_size)
>  	return (edid[0x7e] + 1) * EDID_LENGTH;
>  }
>  
> +static struct platform_device *edid_pdev(const char *connector_name)
> +{
> +	struct platform_device *pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> +
> +	if (IS_ERR(pdev)) {
> +		DRM_ERROR("Failed to register EDID firmware platform device "
> +			"for connector \"%s\"\n", connector_name);
> +		return ERR_CAST(pdev);
> +	}
> +
> +	return pdev;
> +}
> +
>  static void *edid_load(struct drm_connector *connector, const char *name,
>  			const char *connector_name)
>  {
> @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  		fwdata = generic_edid[builtin];
>  		fwsize = sizeof(generic_edid[builtin]);
>  	} else {
> -		struct platform_device *pdev;
> +		struct platform_device *pdev = connector->edid_load_pdev;
>  		int err;
>  
> -		pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> -		if (IS_ERR(pdev)) {
> -			DRM_ERROR("Failed to register EDID firmware platform device "
> -				  "for connector \"%s\"\n", connector_name);
> -			return ERR_CAST(pdev);
> +		if (WARN_ON(!pdev)) {
> +			pdev = edid_pdev(connector_name);
> +			if (IS_ERR(pdev))
> +				return ERR_CAST(pdev);
> +			connector->edid_load_pdev = pdev;
>  		}
>  
>  		err = request_firmware(&fw, name, &pdev->dev);
> -		platform_device_unregister(pdev);
>  		if (err) {
>  			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>  				  name, err);
> @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>  	return edid;
>  }
>  
> -struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +static char *edid_name(const char *connector_name)
>  {
> -	const char *connector_name = connector->name;
>  	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
> -	struct edid *edid;
>  
>  	if (edid_firmware[0] == '\0')
>  		return ERR_PTR(-ENOENT);
> @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct drm_connector *connector)
>  	if (*last == '\n')
>  		*last = '\0';
>  
> -	edid = edid_load(connector, edidname, connector_name);
> +	edidname = kstrdup(edidname, GFP_KERNEL);
> +	if (!edidname) {
> +		kfree(fwstr);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	kfree(fwstr);
> +	return edidname;
> +}
> +
> +void drm_cache_edid_firmware(struct drm_connector *connector)
> +{
> +	const char *connector_name = connector->name;
> +	const char *edidname = edid_name(connector_name);
> +	struct platform_device *pdev;
> +	int err;
> +
> +	if (IS_ERR(edidname))
> +		return;
> +
> +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 0) {
> +		kfree(edidname);
> +		return;
> +	}
> +
> +	pdev = edid_pdev(connector_name);
> +	if (IS_ERR(pdev)) {
> +		kfree(edidname);
> +		return;
> +	}
> +	connector->edid_load_pdev = pdev;
> +
> +	err = firmware_request_cache(&pdev->dev, edidname);
> +	if (err)
> +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed (err=%d)\n",
> +			edidname, err);
> +
> +	kfree(edidname);
> +}
> +
> +struct edid *drm_load_edid_firmware(struct drm_connector *connector)
> +{
> +	const char *connector_name = connector->name;
> +	const char *edidname = edid_name(connector_name);
> +	struct edid *edid;
> +
> +	if (IS_ERR(edidname))
> +		return ERR_CAST(edidname);
> +
> +	edid = edid_load(connector, edidname, connector_name);
> +	kfree(edidname);
>  
>  	return edid;
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 3ac4bf87f257..47c84741517e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1573,6 +1573,18 @@ struct drm_connector {
>  	 */
>  	struct i2c_adapter *ddc;
>  
> +	/**
> +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
> +	 *
> +	 * The platform device is registered in drm_connector_init() in case a
> +	 * custom EDID firmware is used with `edid_firmware` parameter. Otherwise,
> +	 * it is set to NULL.
> +	 *
> +	 * Platform device is unregistered in drm_connector_cleanup() if it
> +	 * is not NULL.
> +	 */
> +	struct platform_device *edid_load_pdev;
> +
>  	/**
>  	 * @null_edid_counter: track sinks that give us all zeros for the EDID.
>  	 * Needed to workaround some HW bugs where we get all 0s
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b2756753370b..e907c928a35d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  		      const struct drm_display_mode *mode);
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
> +void drm_cache_edid_firmware(struct drm_connector *connector);
>  struct edid *drm_load_edid_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
> +inline void
> +drm_cache_edid_firmware(struct drm_connector *connector);
>  static inline struct edid *
>  drm_load_edid_firmware(struct drm_connector *connector)
>  {

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 8275 bytes --]

Sorry, What do you mean?

Matthieu

On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com 
> <mailto:matthieu.charette@gmail.com>> 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.
> 
> AFAICT this breaks changing drm.edid_firmware runtime.
> 
> BR,
> Jani.
> 
>> 
>>  Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com 
>> <mailto:matthieu.charette@gmail.com>>
>>  ---
>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>   drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     | 12 +++++
>>   include/drm/drm_edid.h          |  3 ++
>>   4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>   #include <drm/drm_privacy_screen_consumer.h>
>>   #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>   #include <linux/uaccess.h>
>> 
>>   #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   	drm_connector_get_cmdline_mode(connector);
>> 
>>  +	connector->edid_load_pdev = NULL;
>>  +	drm_cache_edid_firmware(connector);
>>  +
>>   	/* We should add connectors at the end to avoid upsetting the 
>> connector
>>   	 * index too much.
>>   	 */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>> drm_connector *connector)
>>   		connector->tile_group = NULL;
>>   	}
>> 
>>  +	if (connector->edid_load_pdev) {
>>  +		platform_device_unregister(connector->edid_load_pdev);
>>  +		connector->edid_load_pdev = NULL;
>>  +	}
>>  +
>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>   		drm_mode_remove(connector, mode);
>> 
>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>> b/drivers/gpu/drm/drm_edid_load.c
>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>> data_size)
>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>   }
>> 
>>  +static struct platform_device *edid_pdev(const char 
>> *connector_name)
>>  +{
>>  +	struct platform_device *pdev = 
>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>  +
>>  +	if (IS_ERR(pdev)) {
>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>  +			"for connector \"%s\"\n", connector_name);
>>  +		return ERR_CAST(pdev);
>>  +	}
>>  +
>>  +	return pdev;
>>  +}
>>  +
>>   static void *edid_load(struct drm_connector *connector, const char 
>> *name,
>>   			const char *connector_name)
>>   {
>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   		fwdata = generic_edid[builtin];
>>   		fwsize = sizeof(generic_edid[builtin]);
>>   	} else {
>>  -		struct platform_device *pdev;
>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>   		int err;
>> 
>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>> 0);
>>  -		if (IS_ERR(pdev)) {
>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>  -				  "for connector \"%s\"\n", connector_name);
>>  -			return ERR_CAST(pdev);
>>  +		if (WARN_ON(!pdev)) {
>>  +			pdev = edid_pdev(connector_name);
>>  +			if (IS_ERR(pdev))
>>  +				return ERR_CAST(pdev);
>>  +			connector->edid_load_pdev = pdev;
>>   		}
>> 
>>   		err = request_firmware(&fw, name, &pdev->dev);
>>  -		platform_device_unregister(pdev);
>>   		if (err) {
>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>   				  name, err);
>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   	return edid;
>>   }
>> 
>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +static char *edid_name(const char *connector_name)
>>   {
>>  -	const char *connector_name = connector->name;
>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>  -	struct edid *edid;
>> 
>>   	if (edid_firmware[0] == '\0')
>>   		return ERR_PTR(-ENOENT);
>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>> drm_connector *connector)
>>   	if (*last == '\n')
>>   		*last = '\0';
>> 
>>  -	edid = edid_load(connector, edidname, connector_name);
>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>  +	if (!edidname) {
>>  +		kfree(fwstr);
>>  +		return ERR_PTR(-ENOMEM);
>>  +	}
>>  +
>>   	kfree(fwstr);
>>  +	return edidname;
>>  +}
>>  +
>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct platform_device *pdev;
>>  +	int err;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return;
>>  +
>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>> 0) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +
>>  +	pdev = edid_pdev(connector_name);
>>  +	if (IS_ERR(pdev)) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +	connector->edid_load_pdev = pdev;
>>  +
>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>  +	if (err)
>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>> (err=%d)\n",
>>  +			edidname, err);
>>  +
>>  +	kfree(edidname);
>>  +}
>>  +
>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct edid *edid;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return ERR_CAST(edidname);
>>  +
>>  +	edid = edid_load(connector, edidname, connector_name);
>>  +	kfree(edidname);
>> 
>>   	return edid;
>>   }
>>  diff --git a/include/drm/drm_connector.h 
>> b/include/drm/drm_connector.h
>>  index 3ac4bf87f257..47c84741517e 100644
>>  --- a/include/drm/drm_connector.h
>>  +++ b/include/drm/drm_connector.h
>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>   	 */
>>   	struct i2c_adapter *ddc;
>> 
>>  +	/**
>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>  +	 *
>>  +	 * The platform device is registered in drm_connector_init() in 
>> case a
>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>> Otherwise,
>>  +	 * it is set to NULL.
>>  +	 *
>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>> it
>>  +	 * is not NULL.
>>  +	 */
>>  +	struct platform_device *edid_load_pdev;
>>  +
>>   	/**
>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>> EDID.
>>   	 * Needed to workaround some HW bugs where we get all 0s
>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>  index b2756753370b..e907c928a35d 100644
>>  --- a/include/drm/drm_edid.h
>>  +++ b/include/drm/drm_edid.h
>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>> *connector,
>>   		      const struct drm_display_mode *mode);
>> 
>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>   struct edid *drm_load_edid_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
>>  +inline void
>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>   static inline struct edid *
>>   drm_load_edid_firmware(struct drm_connector *connector)
>>   {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center


[-- Attachment #2: Type: text/html, Size: 7990 bytes --]

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

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 8537 bytes --]

Sorry, What do you mean?

Matthieu

On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com 
> <mailto:matthieu.charette@gmail.com>> 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.
> 
> AFAICT this breaks changing drm.edid_firmware runtime.
> 
> BR,
> Jani.
> 
>> 
>>  Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com 
>> <mailto:matthieu.charette@gmail.com>>
>>  ---
>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>   drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     | 12 +++++
>>   include/drm/drm_edid.h          |  3 ++
>>   4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>   #include <drm/drm_privacy_screen_consumer.h>
>>   #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>   #include <linux/uaccess.h>
>> 
>>   #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   	drm_connector_get_cmdline_mode(connector);
>> 
>>  +	connector->edid_load_pdev = NULL;
>>  +	drm_cache_edid_firmware(connector);
>>  +
>>   	/* We should add connectors at the end to avoid upsetting the 
>> connector
>>   	 * index too much.
>>   	 */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>> drm_connector *connector)
>>   		connector->tile_group = NULL;
>>   	}
>> 
>>  +	if (connector->edid_load_pdev) {
>>  +		platform_device_unregister(connector->edid_load_pdev);
>>  +		connector->edid_load_pdev = NULL;
>>  +	}
>>  +
>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>   		drm_mode_remove(connector, mode);
>> 
>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>> b/drivers/gpu/drm/drm_edid_load.c
>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>> data_size)
>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>   }
>> 
>>  +static struct platform_device *edid_pdev(const char 
>> *connector_name)
>>  +{
>>  +	struct platform_device *pdev = 
>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>  +
>>  +	if (IS_ERR(pdev)) {
>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>  +			"for connector \"%s\"\n", connector_name);
>>  +		return ERR_CAST(pdev);
>>  +	}
>>  +
>>  +	return pdev;
>>  +}
>>  +
>>   static void *edid_load(struct drm_connector *connector, const char 
>> *name,
>>   			const char *connector_name)
>>   {
>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   		fwdata = generic_edid[builtin];
>>   		fwsize = sizeof(generic_edid[builtin]);
>>   	} else {
>>  -		struct platform_device *pdev;
>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>   		int err;
>> 
>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>> 0);
>>  -		if (IS_ERR(pdev)) {
>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>  -				  "for connector \"%s\"\n", connector_name);
>>  -			return ERR_CAST(pdev);
>>  +		if (WARN_ON(!pdev)) {
>>  +			pdev = edid_pdev(connector_name);
>>  +			if (IS_ERR(pdev))
>>  +				return ERR_CAST(pdev);
>>  +			connector->edid_load_pdev = pdev;
>>   		}
>> 
>>   		err = request_firmware(&fw, name, &pdev->dev);
>>  -		platform_device_unregister(pdev);
>>   		if (err) {
>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>   				  name, err);
>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   	return edid;
>>   }
>> 
>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +static char *edid_name(const char *connector_name)
>>   {
>>  -	const char *connector_name = connector->name;
>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>  -	struct edid *edid;
>> 
>>   	if (edid_firmware[0] == '\0')
>>   		return ERR_PTR(-ENOENT);
>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>> drm_connector *connector)
>>   	if (*last == '\n')
>>   		*last = '\0';
>> 
>>  -	edid = edid_load(connector, edidname, connector_name);
>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>  +	if (!edidname) {
>>  +		kfree(fwstr);
>>  +		return ERR_PTR(-ENOMEM);
>>  +	}
>>  +
>>   	kfree(fwstr);
>>  +	return edidname;
>>  +}
>>  +
>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct platform_device *pdev;
>>  +	int err;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return;
>>  +
>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>> 0) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +
>>  +	pdev = edid_pdev(connector_name);
>>  +	if (IS_ERR(pdev)) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +	connector->edid_load_pdev = pdev;
>>  +
>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>  +	if (err)
>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>> (err=%d)\n",
>>  +			edidname, err);
>>  +
>>  +	kfree(edidname);
>>  +}
>>  +
>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct edid *edid;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return ERR_CAST(edidname);
>>  +
>>  +	edid = edid_load(connector, edidname, connector_name);
>>  +	kfree(edidname);
>> 
>>   	return edid;
>>   }
>>  diff --git a/include/drm/drm_connector.h 
>> b/include/drm/drm_connector.h
>>  index 3ac4bf87f257..47c84741517e 100644
>>  --- a/include/drm/drm_connector.h
>>  +++ b/include/drm/drm_connector.h
>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>   	 */
>>   	struct i2c_adapter *ddc;
>> 
>>  +	/**
>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>  +	 *
>>  +	 * The platform device is registered in drm_connector_init() in 
>> case a
>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>> Otherwise,
>>  +	 * it is set to NULL.
>>  +	 *
>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>> it
>>  +	 * is not NULL.
>>  +	 */
>>  +	struct platform_device *edid_load_pdev;
>>  +
>>   	/**
>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>> EDID.
>>   	 * Needed to workaround some HW bugs where we get all 0s
>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>  index b2756753370b..e907c928a35d 100644
>>  --- a/include/drm/drm_edid.h
>>  +++ b/include/drm/drm_edid.h
>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>> *connector,
>>   		      const struct drm_display_mode *mode);
>> 
>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>   struct edid *drm_load_edid_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
>>  +inline void
>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>   static inline struct edid *
>>   drm_load_edid_firmware(struct drm_connector *connector)
>>   {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center


[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 7990 bytes --]

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

[Jani Nikula]

On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> wrote:
> Sorry, What do you mean?

You cache with one name at connector init time, but the name specified
using drm.edid_firmware may be changed whenever, to cause the next EDID
read to use a different EDID firmware.

BR,
Jani.


>
> Matthieu
>
> On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
> <jani.nikula@linux.intel.com> wrote:
>> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com 
>> <mailto:matthieu.charette@gmail.com>> 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.
>> 
>> AFAICT this breaks changing drm.edid_firmware runtime.
>> 
>> BR,
>> Jani.
>> 
>>> 
>>>  Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com 
>>> <mailto:matthieu.charette@gmail.com>>
>>>  ---
>>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>>   drivers/gpu/drm/drm_edid_load.c | 81 
>>> ++++++++++++++++++++++++++++-----
>>>   include/drm/drm_connector.h     | 12 +++++
>>>   include/drm/drm_edid.h          |  3 ++
>>>   4 files changed, 94 insertions(+), 11 deletions(-)
>>> 
>>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>>> b/drivers/gpu/drm/drm_connector.c
>>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>>  --- a/drivers/gpu/drm/drm_connector.c
>>>  +++ b/drivers/gpu/drm/drm_connector.c
>>>  @@ -31,6 +31,7 @@
>>>   #include <drm/drm_privacy_screen_consumer.h>
>>>   #include <drm/drm_sysfs.h>
>>> 
>>>  +#include <linux/platform_device.h>
>>>   #include <linux/uaccess.h>
>>> 
>>>   #include "drm_crtc_internal.h"
>>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>>> 
>>>   	drm_connector_get_cmdline_mode(connector);
>>> 
>>>  +	connector->edid_load_pdev = NULL;
>>>  +	drm_cache_edid_firmware(connector);
>>>  +
>>>   	/* We should add connectors at the end to avoid upsetting the 
>>> connector
>>>   	 * index too much.
>>>   	 */
>>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>>> drm_connector *connector)
>>>   		connector->tile_group = NULL;
>>>   	}
>>> 
>>>  +	if (connector->edid_load_pdev) {
>>>  +		platform_device_unregister(connector->edid_load_pdev);
>>>  +		connector->edid_load_pdev = NULL;
>>>  +	}
>>>  +
>>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>>   		drm_mode_remove(connector, mode);
>>> 
>>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>>> b/drivers/gpu/drm/drm_edid_load.c
>>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>>> data_size)
>>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>   }
>>> 
>>>  +static struct platform_device *edid_pdev(const char 
>>> *connector_name)
>>>  +{
>>>  +	struct platform_device *pdev = 
>>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>>  +
>>>  +	if (IS_ERR(pdev)) {
>>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>  +			"for connector \"%s\"\n", connector_name);
>>>  +		return ERR_CAST(pdev);
>>>  +	}
>>>  +
>>>  +	return pdev;
>>>  +}
>>>  +
>>>   static void *edid_load(struct drm_connector *connector, const char 
>>> *name,
>>>   			const char *connector_name)
>>>   {
>>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>>> *connector, const char *name,
>>>   		fwdata = generic_edid[builtin];
>>>   		fwsize = sizeof(generic_edid[builtin]);
>>>   	} else {
>>>  -		struct platform_device *pdev;
>>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>>   		int err;
>>> 
>>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>>> 0);
>>>  -		if (IS_ERR(pdev)) {
>>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>  -				  "for connector \"%s\"\n", connector_name);
>>>  -			return ERR_CAST(pdev);
>>>  +		if (WARN_ON(!pdev)) {
>>>  +			pdev = edid_pdev(connector_name);
>>>  +			if (IS_ERR(pdev))
>>>  +				return ERR_CAST(pdev);
>>>  +			connector->edid_load_pdev = pdev;
>>>   		}
>>> 
>>>   		err = request_firmware(&fw, name, &pdev->dev);
>>>  -		platform_device_unregister(pdev);
>>>   		if (err) {
>>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>>   				  name, err);
>>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>>> *connector, const char *name,
>>>   	return edid;
>>>   }
>>> 
>>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>>> *connector)
>>>  +static char *edid_name(const char *connector_name)
>>>   {
>>>  -	const char *connector_name = connector->name;
>>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>>  -	struct edid *edid;
>>> 
>>>   	if (edid_firmware[0] == '\0')
>>>   		return ERR_PTR(-ENOENT);
>>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>>> drm_connector *connector)
>>>   	if (*last == '\n')
>>>   		*last = '\0';
>>> 
>>>  -	edid = edid_load(connector, edidname, connector_name);
>>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>  +	if (!edidname) {
>>>  +		kfree(fwstr);
>>>  +		return ERR_PTR(-ENOMEM);
>>>  +	}
>>>  +
>>>   	kfree(fwstr);
>>>  +	return edidname;
>>>  +}
>>>  +
>>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>  +{
>>>  +	const char *connector_name = connector->name;
>>>  +	const char *edidname = edid_name(connector_name);
>>>  +	struct platform_device *pdev;
>>>  +	int err;
>>>  +
>>>  +	if (IS_ERR(edidname))
>>>  +		return;
>>>  +
>>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>>> 0) {
>>>  +		kfree(edidname);
>>>  +		return;
>>>  +	}
>>>  +
>>>  +	pdev = edid_pdev(connector_name);
>>>  +	if (IS_ERR(pdev)) {
>>>  +		kfree(edidname);
>>>  +		return;
>>>  +	}
>>>  +	connector->edid_load_pdev = pdev;
>>>  +
>>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>>  +	if (err)
>>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>>> (err=%d)\n",
>>>  +			edidname, err);
>>>  +
>>>  +	kfree(edidname);
>>>  +}
>>>  +
>>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>>> *connector)
>>>  +{
>>>  +	const char *connector_name = connector->name;
>>>  +	const char *edidname = edid_name(connector_name);
>>>  +	struct edid *edid;
>>>  +
>>>  +	if (IS_ERR(edidname))
>>>  +		return ERR_CAST(edidname);
>>>  +
>>>  +	edid = edid_load(connector, edidname, connector_name);
>>>  +	kfree(edidname);
>>> 
>>>   	return edid;
>>>   }
>>>  diff --git a/include/drm/drm_connector.h 
>>> b/include/drm/drm_connector.h
>>>  index 3ac4bf87f257..47c84741517e 100644
>>>  --- a/include/drm/drm_connector.h
>>>  +++ b/include/drm/drm_connector.h
>>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>   	 */
>>>   	struct i2c_adapter *ddc;
>>> 
>>>  +	/**
>>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>>  +	 *
>>>  +	 * The platform device is registered in drm_connector_init() in 
>>> case a
>>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>>> Otherwise,
>>>  +	 * it is set to NULL.
>>>  +	 *
>>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>>> it
>>>  +	 * is not NULL.
>>>  +	 */
>>>  +	struct platform_device *edid_load_pdev;
>>>  +
>>>   	/**
>>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>>> EDID.
>>>   	 * Needed to workaround some HW bugs where we get all 0s
>>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>  index b2756753370b..e907c928a35d 100644
>>>  --- a/include/drm/drm_edid.h
>>>  +++ b/include/drm/drm_edid.h
>>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>>> *connector,
>>>   		      const struct drm_display_mode *mode);
>>> 
>>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>   struct edid *drm_load_edid_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
>>>  +inline void
>>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>>   static inline struct edid *
>>>   drm_load_edid_firmware(struct drm_connector *connector)
>>>   {
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> wrote:
> Sorry, What do you mean?

You cache with one name at connector init time, but the name specified
using drm.edid_firmware may be changed whenever, to cause the next EDID
read to use a different EDID firmware.

BR,
Jani.


>
> Matthieu
>
> On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
> <jani.nikula@linux.intel.com> wrote:
>> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com 
>> <mailto:matthieu.charette@gmail.com>> 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.
>> 
>> AFAICT this breaks changing drm.edid_firmware runtime.
>> 
>> BR,
>> Jani.
>> 
>>> 
>>>  Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com 
>>> <mailto:matthieu.charette@gmail.com>>
>>>  ---
>>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>>   drivers/gpu/drm/drm_edid_load.c | 81 
>>> ++++++++++++++++++++++++++++-----
>>>   include/drm/drm_connector.h     | 12 +++++
>>>   include/drm/drm_edid.h          |  3 ++
>>>   4 files changed, 94 insertions(+), 11 deletions(-)
>>> 
>>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>>> b/drivers/gpu/drm/drm_connector.c
>>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>>  --- a/drivers/gpu/drm/drm_connector.c
>>>  +++ b/drivers/gpu/drm/drm_connector.c
>>>  @@ -31,6 +31,7 @@
>>>   #include <drm/drm_privacy_screen_consumer.h>
>>>   #include <drm/drm_sysfs.h>
>>> 
>>>  +#include <linux/platform_device.h>
>>>   #include <linux/uaccess.h>
>>> 
>>>   #include "drm_crtc_internal.h"
>>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>>> 
>>>   	drm_connector_get_cmdline_mode(connector);
>>> 
>>>  +	connector->edid_load_pdev = NULL;
>>>  +	drm_cache_edid_firmware(connector);
>>>  +
>>>   	/* We should add connectors at the end to avoid upsetting the 
>>> connector
>>>   	 * index too much.
>>>   	 */
>>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>>> drm_connector *connector)
>>>   		connector->tile_group = NULL;
>>>   	}
>>> 
>>>  +	if (connector->edid_load_pdev) {
>>>  +		platform_device_unregister(connector->edid_load_pdev);
>>>  +		connector->edid_load_pdev = NULL;
>>>  +	}
>>>  +
>>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>>   		drm_mode_remove(connector, mode);
>>> 
>>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>>> b/drivers/gpu/drm/drm_edid_load.c
>>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>>> data_size)
>>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>   }
>>> 
>>>  +static struct platform_device *edid_pdev(const char 
>>> *connector_name)
>>>  +{
>>>  +	struct platform_device *pdev = 
>>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>>  +
>>>  +	if (IS_ERR(pdev)) {
>>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>  +			"for connector \"%s\"\n", connector_name);
>>>  +		return ERR_CAST(pdev);
>>>  +	}
>>>  +
>>>  +	return pdev;
>>>  +}
>>>  +
>>>   static void *edid_load(struct drm_connector *connector, const char 
>>> *name,
>>>   			const char *connector_name)
>>>   {
>>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>>> *connector, const char *name,
>>>   		fwdata = generic_edid[builtin];
>>>   		fwsize = sizeof(generic_edid[builtin]);
>>>   	} else {
>>>  -		struct platform_device *pdev;
>>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>>   		int err;
>>> 
>>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>>> 0);
>>>  -		if (IS_ERR(pdev)) {
>>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>  -				  "for connector \"%s\"\n", connector_name);
>>>  -			return ERR_CAST(pdev);
>>>  +		if (WARN_ON(!pdev)) {
>>>  +			pdev = edid_pdev(connector_name);
>>>  +			if (IS_ERR(pdev))
>>>  +				return ERR_CAST(pdev);
>>>  +			connector->edid_load_pdev = pdev;
>>>   		}
>>> 
>>>   		err = request_firmware(&fw, name, &pdev->dev);
>>>  -		platform_device_unregister(pdev);
>>>   		if (err) {
>>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>>   				  name, err);
>>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>>> *connector, const char *name,
>>>   	return edid;
>>>   }
>>> 
>>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>>> *connector)
>>>  +static char *edid_name(const char *connector_name)
>>>   {
>>>  -	const char *connector_name = connector->name;
>>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>>  -	struct edid *edid;
>>> 
>>>   	if (edid_firmware[0] == '\0')
>>>   		return ERR_PTR(-ENOENT);
>>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>>> drm_connector *connector)
>>>   	if (*last == '\n')
>>>   		*last = '\0';
>>> 
>>>  -	edid = edid_load(connector, edidname, connector_name);
>>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>  +	if (!edidname) {
>>>  +		kfree(fwstr);
>>>  +		return ERR_PTR(-ENOMEM);
>>>  +	}
>>>  +
>>>   	kfree(fwstr);
>>>  +	return edidname;
>>>  +}
>>>  +
>>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>  +{
>>>  +	const char *connector_name = connector->name;
>>>  +	const char *edidname = edid_name(connector_name);
>>>  +	struct platform_device *pdev;
>>>  +	int err;
>>>  +
>>>  +	if (IS_ERR(edidname))
>>>  +		return;
>>>  +
>>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>>> 0) {
>>>  +		kfree(edidname);
>>>  +		return;
>>>  +	}
>>>  +
>>>  +	pdev = edid_pdev(connector_name);
>>>  +	if (IS_ERR(pdev)) {
>>>  +		kfree(edidname);
>>>  +		return;
>>>  +	}
>>>  +	connector->edid_load_pdev = pdev;
>>>  +
>>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>>  +	if (err)
>>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>>> (err=%d)\n",
>>>  +			edidname, err);
>>>  +
>>>  +	kfree(edidname);
>>>  +}
>>>  +
>>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>>> *connector)
>>>  +{
>>>  +	const char *connector_name = connector->name;
>>>  +	const char *edidname = edid_name(connector_name);
>>>  +	struct edid *edid;
>>>  +
>>>  +	if (IS_ERR(edidname))
>>>  +		return ERR_CAST(edidname);
>>>  +
>>>  +	edid = edid_load(connector, edidname, connector_name);
>>>  +	kfree(edidname);
>>> 
>>>   	return edid;
>>>   }
>>>  diff --git a/include/drm/drm_connector.h 
>>> b/include/drm/drm_connector.h
>>>  index 3ac4bf87f257..47c84741517e 100644
>>>  --- a/include/drm/drm_connector.h
>>>  +++ b/include/drm/drm_connector.h
>>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>   	 */
>>>   	struct i2c_adapter *ddc;
>>> 
>>>  +	/**
>>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>>  +	 *
>>>  +	 * The platform device is registered in drm_connector_init() in 
>>> case a
>>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>>> Otherwise,
>>>  +	 * it is set to NULL.
>>>  +	 *
>>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>>> it
>>>  +	 * is not NULL.
>>>  +	 */
>>>  +	struct platform_device *edid_load_pdev;
>>>  +
>>>   	/**
>>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>>> EDID.
>>>   	 * Needed to workaround some HW bugs where we get all 0s
>>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>  index b2756753370b..e907c928a35d 100644
>>>  --- a/include/drm/drm_edid.h
>>>  +++ b/include/drm/drm_edid.h
>>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>>> *connector,
>>>   		      const struct drm_display_mode *mode);
>>> 
>>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>   struct edid *drm_load_edid_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
>>>  +inline void
>>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>>   static inline struct edid *
>>>   drm_load_edid_firmware(struct drm_connector *connector)
>>>   {
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

Hi,

Sorry for late reply.

How do you propose to go then?
Can we still use a persistent platform device to load the firmware and 
cache it? But, in this case the system will still crash in case the 
user change drm.edid_firmware then, without replugging the device, he 
suspends and resumes the machine.
Or you may have a better idea?

Thanks,
Matthieu

On Mon, Aug 8 2022 at 08:19:01 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> wrote:
>>  Sorry, What do you mean?
> 
> You cache with one name at connector init time, but the name specified
> using drm.edid_firmware may be changed whenever, to cause the next 
> EDID
> read to use a different EDID firmware.
> 
> BR,
> Jani.
> 
> 
>> 
>>  Matthieu
>> 
>>  On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula
>>  <jani.nikula@linux.intel.com> wrote:
>>>  On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com
>>>  <mailto:matthieu.charette@gmail.com>> 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.
>>> 
>>>  AFAICT this breaks changing drm.edid_firmware runtime.
>>> 
>>>  BR,
>>>  Jani.
>>> 
>>>> 
>>>>   Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>>   Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>  <mailto:matthieu.charette@gmail.com>>
>>>>   ---
>>>>    drivers/gpu/drm/drm_connector.c |  9 ++++
>>>>    drivers/gpu/drm/drm_edid_load.c | 81
>>>>  ++++++++++++++++++++++++++++-----
>>>>    include/drm/drm_connector.h     | 12 +++++
>>>>    include/drm/drm_edid.h          |  3 ++
>>>>    4 files changed, 94 insertions(+), 11 deletions(-)
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_connector.c
>>>>  b/drivers/gpu/drm/drm_connector.c
>>>>   index 1c48d162c77e..e8819ebf1c4b 100644
>>>>   --- a/drivers/gpu/drm/drm_connector.c
>>>>   +++ b/drivers/gpu/drm/drm_connector.c
>>>>   @@ -31,6 +31,7 @@
>>>>    #include <drm/drm_privacy_screen_consumer.h>
>>>>    #include <drm/drm_sysfs.h>
>>>> 
>>>>   +#include <linux/platform_device.h>
>>>>    #include <linux/uaccess.h>
>>>> 
>>>>    #include "drm_crtc_internal.h"
>>>>   @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device 
>>>> *dev,
>>>> 
>>>>    	drm_connector_get_cmdline_mode(connector);
>>>> 
>>>>   +	connector->edid_load_pdev = NULL;
>>>>   +	drm_cache_edid_firmware(connector);
>>>>   +
>>>>    	/* We should add connectors at the end to avoid upsetting the
>>>>  connector
>>>>    	 * index too much.
>>>>    	 */
>>>>   @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct
>>>>  drm_connector *connector)
>>>>    		connector->tile_group = NULL;
>>>>    	}
>>>> 
>>>>   +	if (connector->edid_load_pdev) {
>>>>   +		platform_device_unregister(connector->edid_load_pdev);
>>>>   +		connector->edid_load_pdev = NULL;
>>>>   +	}
>>>>   +
>>>>    	list_for_each_entry_safe(mode, t, &connector->probed_modes, 
>>>> head)
>>>>    		drm_mode_remove(connector, mode);
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_edid_load.c
>>>>  b/drivers/gpu/drm/drm_edid_load.c
>>>>   index 37d8ba3ddb46..5a82be9917ec 100644
>>>>   --- a/drivers/gpu/drm/drm_edid_load.c
>>>>   +++ b/drivers/gpu/drm/drm_edid_load.c
>>>>   @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int
>>>>  data_size)
>>>>    	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>>    }
>>>> 
>>>>   +static struct platform_device *edid_pdev(const char
>>>>  *connector_name)
>>>>   +{
>>>>   +	struct platform_device *pdev =
>>>>  platform_device_register_simple(connector_name, -1, NULL, 0);
>>>>   +
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   +			"for connector \"%s\"\n", connector_name);
>>>>   +		return ERR_CAST(pdev);
>>>>   +	}
>>>>   +
>>>>   +	return pdev;
>>>>   +}
>>>>   +
>>>>    static void *edid_load(struct drm_connector *connector, const 
>>>> char
>>>>  *name,
>>>>    			const char *connector_name)
>>>>    {
>>>>   @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    		fwdata = generic_edid[builtin];
>>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>>    	} else {
>>>>   -		struct platform_device *pdev;
>>>>   +		struct platform_device *pdev = connector->edid_load_pdev;
>>>>    		int err;
>>>> 
>>>>   -		pdev = platform_device_register_simple(connector_name, -1, 
>>>> NULL,
>>>>  0);
>>>>   -		if (IS_ERR(pdev)) {
>>>>   -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   -				  "for connector \"%s\"\n", connector_name);
>>>>   -			return ERR_CAST(pdev);
>>>>   +		if (WARN_ON(!pdev)) {
>>>>   +			pdev = edid_pdev(connector_name);
>>>>   +			if (IS_ERR(pdev))
>>>>   +				return ERR_CAST(pdev);
>>>>   +			connector->edid_load_pdev = pdev;
>>>>    		}
>>>> 
>>>>    		err = request_firmware(&fw, name, &pdev->dev);
>>>>   -		platform_device_unregister(pdev);
>>>>    		if (err) {
>>>>    			DRM_ERROR("Requesting EDID firmware \"%s\" failed 
>>>> (err=%d)\n",
>>>>    				  name, err);
>>>>   @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    	return edid;
>>>>    }
>>>> 
>>>>   -struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +static char *edid_name(const char *connector_name)
>>>>    {
>>>>   -	const char *connector_name = connector->name;
>>>>    	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = 
>>>> NULL;
>>>>   -	struct edid *edid;
>>>> 
>>>>    	if (edid_firmware[0] == '\0')
>>>>    		return ERR_PTR(-ENOENT);
>>>>   @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct
>>>>  drm_connector *connector)
>>>>    	if (*last == '\n')
>>>>    		*last = '\0';
>>>> 
>>>>   -	edid = edid_load(connector, edidname, connector_name);
>>>>   +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>>   +	if (!edidname) {
>>>>   +		kfree(fwstr);
>>>>   +		return ERR_PTR(-ENOMEM);
>>>>   +	}
>>>>   +
>>>>    	kfree(fwstr);
>>>>   +	return edidname;
>>>>   +}
>>>>   +
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct platform_device *pdev;
>>>>   +	int err;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return;
>>>>   +
>>>>   +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >=
>>>>  0) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +
>>>>   +	pdev = edid_pdev(connector_name);
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +	connector->edid_load_pdev = pdev;
>>>>   +
>>>>   +	err = firmware_request_cache(&pdev->dev, edidname);
>>>>   +	if (err)
>>>>   +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed
>>>>  (err=%d)\n",
>>>>   +			edidname, err);
>>>>   +
>>>>   +	kfree(edidname);
>>>>   +}
>>>>   +
>>>>   +struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct edid *edid;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return ERR_CAST(edidname);
>>>>   +
>>>>   +	edid = edid_load(connector, edidname, connector_name);
>>>>   +	kfree(edidname);
>>>> 
>>>>    	return edid;
>>>>    }
>>>>   diff --git a/include/drm/drm_connector.h
>>>>  b/include/drm/drm_connector.h
>>>>   index 3ac4bf87f257..47c84741517e 100644
>>>>   --- a/include/drm/drm_connector.h
>>>>   +++ b/include/drm/drm_connector.h
>>>>   @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>>    	 */
>>>>    	struct i2c_adapter *ddc;
>>>> 
>>>>   +	/**
>>>>   +	 * @edid_load_pdev: Platform device for loading EDID via 
>>>> firmware.
>>>>   +	 *
>>>>   +	 * The platform device is registered in drm_connector_init() in
>>>>  case a
>>>>   +	 * custom EDID firmware is used with `edid_firmware` parameter.
>>>>  Otherwise,
>>>>   +	 * it is set to NULL.
>>>>   +	 *
>>>>   +	 * Platform device is unregistered in drm_connector_cleanup() 
>>>> if
>>>>  it
>>>>   +	 * is not NULL.
>>>>   +	 */
>>>>   +	struct platform_device *edid_load_pdev;
>>>>   +
>>>>    	/**
>>>>    	 * @null_edid_counter: track sinks that give us all zeros for 
>>>> the
>>>>  EDID.
>>>>    	 * Needed to workaround some HW bugs where we get all 0s
>>>>   diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>>   index b2756753370b..e907c928a35d 100644
>>>>   --- a/include/drm/drm_edid.h
>>>>   +++ b/include/drm/drm_edid.h
>>>>   @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector
>>>>  *connector,
>>>>    		      const struct drm_display_mode *mode);
>>>> 
>>>>    #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    struct edid *drm_load_edid_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
>>>>   +inline void
>>>>   +drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    static inline struct edid *
>>>>    drm_load_edid_firmware(struct drm_connector *connector)
>>>>    {
>>> 
>>>  --
>>>  Jani Nikula, Intel Open Source Graphics Center
>> 
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 10178 bytes --]

Hi,

Sorry for late reply.

How do you propose to go then?
Can we still use a persistent platform device to load the firmware and 
cache it? But, in this case the system will still crash in case the 
user change drm.edid_firmware then, without replugging the device, he 
suspends and resumes the machine.
Or you may have a better idea?

Thanks,
Matthieu

On Mon, Aug 8 2022 at 08:19:01 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> wrote:
>>  Sorry, What do you mean?
> 
> You cache with one name at connector init time, but the name specified
> using drm.edid_firmware may be changed whenever, to cause the next 
> EDID
> read to use a different EDID firmware.
> 
> BR,
> Jani.
> 
> 
>> 
>>  Matthieu
>> 
>>  On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula
>>  <jani.nikula@linux.intel.com> wrote:
>>>  On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com
>>>  <mailto:matthieu.charette@gmail.com>> 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.
>>> 
>>>  AFAICT this breaks changing drm.edid_firmware runtime.
>>> 
>>>  BR,
>>>  Jani.
>>> 
>>>> 
>>>>   Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>>   Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>  <mailto:matthieu.charette@gmail.com>>
>>>>   ---
>>>>    drivers/gpu/drm/drm_connector.c |  9 ++++
>>>>    drivers/gpu/drm/drm_edid_load.c | 81
>>>>  ++++++++++++++++++++++++++++-----
>>>>    include/drm/drm_connector.h     | 12 +++++
>>>>    include/drm/drm_edid.h          |  3 ++
>>>>    4 files changed, 94 insertions(+), 11 deletions(-)
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_connector.c
>>>>  b/drivers/gpu/drm/drm_connector.c
>>>>   index 1c48d162c77e..e8819ebf1c4b 100644
>>>>   --- a/drivers/gpu/drm/drm_connector.c
>>>>   +++ b/drivers/gpu/drm/drm_connector.c
>>>>   @@ -31,6 +31,7 @@
>>>>    #include <drm/drm_privacy_screen_consumer.h>
>>>>    #include <drm/drm_sysfs.h>
>>>> 
>>>>   +#include <linux/platform_device.h>
>>>>    #include <linux/uaccess.h>
>>>> 
>>>>    #include "drm_crtc_internal.h"
>>>>   @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device 
>>>> *dev,
>>>> 
>>>>    	drm_connector_get_cmdline_mode(connector);
>>>> 
>>>>   +	connector->edid_load_pdev = NULL;
>>>>   +	drm_cache_edid_firmware(connector);
>>>>   +
>>>>    	/* We should add connectors at the end to avoid upsetting the
>>>>  connector
>>>>    	 * index too much.
>>>>    	 */
>>>>   @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct
>>>>  drm_connector *connector)
>>>>    		connector->tile_group = NULL;
>>>>    	}
>>>> 
>>>>   +	if (connector->edid_load_pdev) {
>>>>   +		platform_device_unregister(connector->edid_load_pdev);
>>>>   +		connector->edid_load_pdev = NULL;
>>>>   +	}
>>>>   +
>>>>    	list_for_each_entry_safe(mode, t, &connector->probed_modes, 
>>>> head)
>>>>    		drm_mode_remove(connector, mode);
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_edid_load.c
>>>>  b/drivers/gpu/drm/drm_edid_load.c
>>>>   index 37d8ba3ddb46..5a82be9917ec 100644
>>>>   --- a/drivers/gpu/drm/drm_edid_load.c
>>>>   +++ b/drivers/gpu/drm/drm_edid_load.c
>>>>   @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int
>>>>  data_size)
>>>>    	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>>    }
>>>> 
>>>>   +static struct platform_device *edid_pdev(const char
>>>>  *connector_name)
>>>>   +{
>>>>   +	struct platform_device *pdev =
>>>>  platform_device_register_simple(connector_name, -1, NULL, 0);
>>>>   +
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   +			"for connector \"%s\"\n", connector_name);
>>>>   +		return ERR_CAST(pdev);
>>>>   +	}
>>>>   +
>>>>   +	return pdev;
>>>>   +}
>>>>   +
>>>>    static void *edid_load(struct drm_connector *connector, const 
>>>> char
>>>>  *name,
>>>>    			const char *connector_name)
>>>>    {
>>>>   @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    		fwdata = generic_edid[builtin];
>>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>>    	} else {
>>>>   -		struct platform_device *pdev;
>>>>   +		struct platform_device *pdev = connector->edid_load_pdev;
>>>>    		int err;
>>>> 
>>>>   -		pdev = platform_device_register_simple(connector_name, -1, 
>>>> NULL,
>>>>  0);
>>>>   -		if (IS_ERR(pdev)) {
>>>>   -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   -				  "for connector \"%s\"\n", connector_name);
>>>>   -			return ERR_CAST(pdev);
>>>>   +		if (WARN_ON(!pdev)) {
>>>>   +			pdev = edid_pdev(connector_name);
>>>>   +			if (IS_ERR(pdev))
>>>>   +				return ERR_CAST(pdev);
>>>>   +			connector->edid_load_pdev = pdev;
>>>>    		}
>>>> 
>>>>    		err = request_firmware(&fw, name, &pdev->dev);
>>>>   -		platform_device_unregister(pdev);
>>>>    		if (err) {
>>>>    			DRM_ERROR("Requesting EDID firmware \"%s\" failed 
>>>> (err=%d)\n",
>>>>    				  name, err);
>>>>   @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    	return edid;
>>>>    }
>>>> 
>>>>   -struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +static char *edid_name(const char *connector_name)
>>>>    {
>>>>   -	const char *connector_name = connector->name;
>>>>    	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = 
>>>> NULL;
>>>>   -	struct edid *edid;
>>>> 
>>>>    	if (edid_firmware[0] == '\0')
>>>>    		return ERR_PTR(-ENOENT);
>>>>   @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct
>>>>  drm_connector *connector)
>>>>    	if (*last == '\n')
>>>>    		*last = '\0';
>>>> 
>>>>   -	edid = edid_load(connector, edidname, connector_name);
>>>>   +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>>   +	if (!edidname) {
>>>>   +		kfree(fwstr);
>>>>   +		return ERR_PTR(-ENOMEM);
>>>>   +	}
>>>>   +
>>>>    	kfree(fwstr);
>>>>   +	return edidname;
>>>>   +}
>>>>   +
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct platform_device *pdev;
>>>>   +	int err;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return;
>>>>   +
>>>>   +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >=
>>>>  0) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +
>>>>   +	pdev = edid_pdev(connector_name);
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +	connector->edid_load_pdev = pdev;
>>>>   +
>>>>   +	err = firmware_request_cache(&pdev->dev, edidname);
>>>>   +	if (err)
>>>>   +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed
>>>>  (err=%d)\n",
>>>>   +			edidname, err);
>>>>   +
>>>>   +	kfree(edidname);
>>>>   +}
>>>>   +
>>>>   +struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct edid *edid;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return ERR_CAST(edidname);
>>>>   +
>>>>   +	edid = edid_load(connector, edidname, connector_name);
>>>>   +	kfree(edidname);
>>>> 
>>>>    	return edid;
>>>>    }
>>>>   diff --git a/include/drm/drm_connector.h
>>>>  b/include/drm/drm_connector.h
>>>>   index 3ac4bf87f257..47c84741517e 100644
>>>>   --- a/include/drm/drm_connector.h
>>>>   +++ b/include/drm/drm_connector.h
>>>>   @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>>    	 */
>>>>    	struct i2c_adapter *ddc;
>>>> 
>>>>   +	/**
>>>>   +	 * @edid_load_pdev: Platform device for loading EDID via 
>>>> firmware.
>>>>   +	 *
>>>>   +	 * The platform device is registered in drm_connector_init() in
>>>>  case a
>>>>   +	 * custom EDID firmware is used with `edid_firmware` parameter.
>>>>  Otherwise,
>>>>   +	 * it is set to NULL.
>>>>   +	 *
>>>>   +	 * Platform device is unregistered in drm_connector_cleanup() 
>>>> if
>>>>  it
>>>>   +	 * is not NULL.
>>>>   +	 */
>>>>   +	struct platform_device *edid_load_pdev;
>>>>   +
>>>>    	/**
>>>>    	 * @null_edid_counter: track sinks that give us all zeros for 
>>>> the
>>>>  EDID.
>>>>    	 * Needed to workaround some HW bugs where we get all 0s
>>>>   diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>>   index b2756753370b..e907c928a35d 100644
>>>>   --- a/include/drm/drm_edid.h
>>>>   +++ b/include/drm/drm_edid.h
>>>>   @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector
>>>>  *connector,
>>>>    		      const struct drm_display_mode *mode);
>>>> 
>>>>    #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    struct edid *drm_load_edid_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
>>>>   +inline void
>>>>   +drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    static inline struct edid *
>>>>    drm_load_edid_firmware(struct drm_connector *connector)
>>>>    {
>>> 
>>>  --
>>>  Jani Nikula, Intel Open Source Graphics Center
>> 
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

Hi,

Sorry for late reply.

How do you propose to go then?
Can we still use a persistent platform device to load the firmware and 
cache it? But, in this case the system will still crash in case the 
user change drm.edid_firmware then, without replugging the device, he 
suspends and resumes the machine.
Or you may have a better idea?

Thanks,
Matthieu

On Mon, Aug 8 2022 at 08:19:01 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> wrote:
>>  Sorry, What do you mean?
> 
> You cache with one name at connector init time, but the name specified
> using drm.edid_firmware may be changed whenever, to cause the next 
> EDID
> read to use a different EDID firmware.
> 
> BR,
> Jani.
> 
> 
>> 
>>  Matthieu
>> 
>>  On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula
>>  <jani.nikula@linux.intel.com> wrote:
>>>  On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com
>>>  <mailto:matthieu.charette@gmail.com>> 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.
>>> 
>>>  AFAICT this breaks changing drm.edid_firmware runtime.
>>> 
>>>  BR,
>>>  Jani.
>>> 
>>>> 
>>>>   Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>>   Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>  <mailto:matthieu.charette@gmail.com>>
>>>>   ---
>>>>    drivers/gpu/drm/drm_connector.c |  9 ++++
>>>>    drivers/gpu/drm/drm_edid_load.c | 81
>>>>  ++++++++++++++++++++++++++++-----
>>>>    include/drm/drm_connector.h     | 12 +++++
>>>>    include/drm/drm_edid.h          |  3 ++
>>>>    4 files changed, 94 insertions(+), 11 deletions(-)
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_connector.c
>>>>  b/drivers/gpu/drm/drm_connector.c
>>>>   index 1c48d162c77e..e8819ebf1c4b 100644
>>>>   --- a/drivers/gpu/drm/drm_connector.c
>>>>   +++ b/drivers/gpu/drm/drm_connector.c
>>>>   @@ -31,6 +31,7 @@
>>>>    #include <drm/drm_privacy_screen_consumer.h>
>>>>    #include <drm/drm_sysfs.h>
>>>> 
>>>>   +#include <linux/platform_device.h>
>>>>    #include <linux/uaccess.h>
>>>> 
>>>>    #include "drm_crtc_internal.h"
>>>>   @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device 
>>>> *dev,
>>>> 
>>>>    	drm_connector_get_cmdline_mode(connector);
>>>> 
>>>>   +	connector->edid_load_pdev = NULL;
>>>>   +	drm_cache_edid_firmware(connector);
>>>>   +
>>>>    	/* We should add connectors at the end to avoid upsetting the
>>>>  connector
>>>>    	 * index too much.
>>>>    	 */
>>>>   @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct
>>>>  drm_connector *connector)
>>>>    		connector->tile_group = NULL;
>>>>    	}
>>>> 
>>>>   +	if (connector->edid_load_pdev) {
>>>>   +		platform_device_unregister(connector->edid_load_pdev);
>>>>   +		connector->edid_load_pdev = NULL;
>>>>   +	}
>>>>   +
>>>>    	list_for_each_entry_safe(mode, t, &connector->probed_modes, 
>>>> head)
>>>>    		drm_mode_remove(connector, mode);
>>>> 
>>>>   diff --git a/drivers/gpu/drm/drm_edid_load.c
>>>>  b/drivers/gpu/drm/drm_edid_load.c
>>>>   index 37d8ba3ddb46..5a82be9917ec 100644
>>>>   --- a/drivers/gpu/drm/drm_edid_load.c
>>>>   +++ b/drivers/gpu/drm/drm_edid_load.c
>>>>   @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int
>>>>  data_size)
>>>>    	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>>    }
>>>> 
>>>>   +static struct platform_device *edid_pdev(const char
>>>>  *connector_name)
>>>>   +{
>>>>   +	struct platform_device *pdev =
>>>>  platform_device_register_simple(connector_name, -1, NULL, 0);
>>>>   +
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   +			"for connector \"%s\"\n", connector_name);
>>>>   +		return ERR_CAST(pdev);
>>>>   +	}
>>>>   +
>>>>   +	return pdev;
>>>>   +}
>>>>   +
>>>>    static void *edid_load(struct drm_connector *connector, const 
>>>> char
>>>>  *name,
>>>>    			const char *connector_name)
>>>>    {
>>>>   @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    		fwdata = generic_edid[builtin];
>>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>>    	} else {
>>>>   -		struct platform_device *pdev;
>>>>   +		struct platform_device *pdev = connector->edid_load_pdev;
>>>>    		int err;
>>>> 
>>>>   -		pdev = platform_device_register_simple(connector_name, -1, 
>>>> NULL,
>>>>  0);
>>>>   -		if (IS_ERR(pdev)) {
>>>>   -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>>   -				  "for connector \"%s\"\n", connector_name);
>>>>   -			return ERR_CAST(pdev);
>>>>   +		if (WARN_ON(!pdev)) {
>>>>   +			pdev = edid_pdev(connector_name);
>>>>   +			if (IS_ERR(pdev))
>>>>   +				return ERR_CAST(pdev);
>>>>   +			connector->edid_load_pdev = pdev;
>>>>    		}
>>>> 
>>>>    		err = request_firmware(&fw, name, &pdev->dev);
>>>>   -		platform_device_unregister(pdev);
>>>>    		if (err) {
>>>>    			DRM_ERROR("Requesting EDID firmware \"%s\" failed 
>>>> (err=%d)\n",
>>>>    				  name, err);
>>>>   @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector
>>>>  *connector, const char *name,
>>>>    	return edid;
>>>>    }
>>>> 
>>>>   -struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +static char *edid_name(const char *connector_name)
>>>>    {
>>>>   -	const char *connector_name = connector->name;
>>>>    	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = 
>>>> NULL;
>>>>   -	struct edid *edid;
>>>> 
>>>>    	if (edid_firmware[0] == '\0')
>>>>    		return ERR_PTR(-ENOENT);
>>>>   @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct
>>>>  drm_connector *connector)
>>>>    	if (*last == '\n')
>>>>    		*last = '\0';
>>>> 
>>>>   -	edid = edid_load(connector, edidname, connector_name);
>>>>   +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>>   +	if (!edidname) {
>>>>   +		kfree(fwstr);
>>>>   +		return ERR_PTR(-ENOMEM);
>>>>   +	}
>>>>   +
>>>>    	kfree(fwstr);
>>>>   +	return edidname;
>>>>   +}
>>>>   +
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct platform_device *pdev;
>>>>   +	int err;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return;
>>>>   +
>>>>   +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >=
>>>>  0) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +
>>>>   +	pdev = edid_pdev(connector_name);
>>>>   +	if (IS_ERR(pdev)) {
>>>>   +		kfree(edidname);
>>>>   +		return;
>>>>   +	}
>>>>   +	connector->edid_load_pdev = pdev;
>>>>   +
>>>>   +	err = firmware_request_cache(&pdev->dev, edidname);
>>>>   +	if (err)
>>>>   +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed
>>>>  (err=%d)\n",
>>>>   +			edidname, err);
>>>>   +
>>>>   +	kfree(edidname);
>>>>   +}
>>>>   +
>>>>   +struct edid *drm_load_edid_firmware(struct drm_connector
>>>>  *connector)
>>>>   +{
>>>>   +	const char *connector_name = connector->name;
>>>>   +	const char *edidname = edid_name(connector_name);
>>>>   +	struct edid *edid;
>>>>   +
>>>>   +	if (IS_ERR(edidname))
>>>>   +		return ERR_CAST(edidname);
>>>>   +
>>>>   +	edid = edid_load(connector, edidname, connector_name);
>>>>   +	kfree(edidname);
>>>> 
>>>>    	return edid;
>>>>    }
>>>>   diff --git a/include/drm/drm_connector.h
>>>>  b/include/drm/drm_connector.h
>>>>   index 3ac4bf87f257..47c84741517e 100644
>>>>   --- a/include/drm/drm_connector.h
>>>>   +++ b/include/drm/drm_connector.h
>>>>   @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>>    	 */
>>>>    	struct i2c_adapter *ddc;
>>>> 
>>>>   +	/**
>>>>   +	 * @edid_load_pdev: Platform device for loading EDID via 
>>>> firmware.
>>>>   +	 *
>>>>   +	 * The platform device is registered in drm_connector_init() in
>>>>  case a
>>>>   +	 * custom EDID firmware is used with `edid_firmware` parameter.
>>>>  Otherwise,
>>>>   +	 * it is set to NULL.
>>>>   +	 *
>>>>   +	 * Platform device is unregistered in drm_connector_cleanup() 
>>>> if
>>>>  it
>>>>   +	 * is not NULL.
>>>>   +	 */
>>>>   +	struct platform_device *edid_load_pdev;
>>>>   +
>>>>    	/**
>>>>    	 * @null_edid_counter: track sinks that give us all zeros for 
>>>> the
>>>>  EDID.
>>>>    	 * Needed to workaround some HW bugs where we get all 0s
>>>>   diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>>   index b2756753370b..e907c928a35d 100644
>>>>   --- a/include/drm/drm_edid.h
>>>>   +++ b/include/drm/drm_edid.h
>>>>   @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector
>>>>  *connector,
>>>>    		      const struct drm_display_mode *mode);
>>>> 
>>>>    #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    struct edid *drm_load_edid_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
>>>>   +inline void
>>>>   +drm_cache_edid_firmware(struct drm_connector *connector);
>>>>    static inline struct edid *
>>>>    drm_load_edid_firmware(struct drm_connector *connector)
>>>>    {
>>> 
>>>  --
>>>  Jani Nikula, Intel Open Source Graphics Center
>> 
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 12 Sep 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> wrote:
> Hi,
>
> Sorry for late reply.
>
> How do you propose to go then?
> Can we still use a persistent platform device to load the firmware and 
> cache it? But, in this case the system will still crash in case the 
> user change drm.edid_firmware then, without replugging the device, he 
> suspends and resumes the machine.
> Or you may have a better idea?

Maybe. Please try [1]. Could it be as simple as that?


BR,
Jani.


[1] https://patchwork.freedesktop.org/patch/msgid/20221006222146.2375217-1-jani.nikula@intel.com

>
> Thanks,
> Matthieu
>
> On Mon, Aug 8 2022 at 08:19:01 PM +0300, Jani Nikula 
> <jani.nikula@linux.intel.com> wrote:
>> On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
>> wrote:
>>>  Sorry, What do you mean?
>> 
>> You cache with one name at connector init time, but the name specified
>> using drm.edid_firmware may be changed whenever, to cause the next 
>> EDID
>> read to use a different EDID firmware.
>> 
>> BR,
>> Jani.
>> 
>> 
>>> 
>>>  Matthieu
>>> 
>>>  On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula
>>>  <jani.nikula@linux.intel.com> wrote:
>>>>  On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>  <mailto:matthieu.charette@gmail.com>> 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.
>>>> 
>>>>  AFAICT this breaks changing drm.edid_firmware runtime.
>>>> 
>>>>  BR,
>>>>  Jani.
>>>> 
>>>>> 
>>>>>   Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>>>   Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>>  <mailto:matthieu.charette@gmail.com>>
>>>>>   ---
>>>>>    drivers/gpu/drm/drm_connector.c |  9 ++++
>>>>>    drivers/gpu/drm/drm_edid_load.c | 81
>>>>>  ++++++++++++++++++++++++++++-----
>>>>>    include/drm/drm_connector.h     | 12 +++++
>>>>>    include/drm/drm_edid.h          |  3 ++
>>>>>    4 files changed, 94 insertions(+), 11 deletions(-)
>>>>> 
>>>>>   diff --git a/drivers/gpu/drm/drm_connector.c
>>>>>  b/drivers/gpu/drm/drm_connector.c
>>>>>   index 1c48d162c77e..e8819ebf1c4b 100644
>>>>>   --- a/drivers/gpu/drm/drm_connector.c
>>>>>   +++ b/drivers/gpu/drm/drm_connector.c
>>>>>   @@ -31,6 +31,7 @@
>>>>>    #include <drm/drm_privacy_screen_consumer.h>
>>>>>    #include <drm/drm_sysfs.h>
>>>>> 
>>>>>   +#include <linux/platform_device.h>
>>>>>    #include <linux/uaccess.h>
>>>>> 
>>>>>    #include "drm_crtc_internal.h"
>>>>>   @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device 
>>>>> *dev,
>>>>> 
>>>>>    	drm_connector_get_cmdline_mode(connector);
>>>>> 
>>>>>   +	connector->edid_load_pdev = NULL;
>>>>>   +	drm_cache_edid_firmware(connector);
>>>>>   +
>>>>>    	/* We should add connectors at the end to avoid upsetting the
>>>>>  connector
>>>>>    	 * index too much.
>>>>>    	 */
>>>>>   @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct
>>>>>  drm_connector *connector)
>>>>>    		connector->tile_group = NULL;
>>>>>    	}
>>>>> 
>>>>>   +	if (connector->edid_load_pdev) {
>>>>>   +		platform_device_unregister(connector->edid_load_pdev);
>>>>>   +		connector->edid_load_pdev = NULL;
>>>>>   +	}
>>>>>   +
>>>>>    	list_for_each_entry_safe(mode, t, &connector->probed_modes, 
>>>>> head)
>>>>>    		drm_mode_remove(connector, mode);
>>>>> 
>>>>>   diff --git a/drivers/gpu/drm/drm_edid_load.c
>>>>>  b/drivers/gpu/drm/drm_edid_load.c
>>>>>   index 37d8ba3ddb46..5a82be9917ec 100644
>>>>>   --- a/drivers/gpu/drm/drm_edid_load.c
>>>>>   +++ b/drivers/gpu/drm/drm_edid_load.c
>>>>>   @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int
>>>>>  data_size)
>>>>>    	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>>>    }
>>>>> 
>>>>>   +static struct platform_device *edid_pdev(const char
>>>>>  *connector_name)
>>>>>   +{
>>>>>   +	struct platform_device *pdev =
>>>>>  platform_device_register_simple(connector_name, -1, NULL, 0);
>>>>>   +
>>>>>   +	if (IS_ERR(pdev)) {
>>>>>   +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>>>   +			"for connector \"%s\"\n", connector_name);
>>>>>   +		return ERR_CAST(pdev);
>>>>>   +	}
>>>>>   +
>>>>>   +	return pdev;
>>>>>   +}
>>>>>   +
>>>>>    static void *edid_load(struct drm_connector *connector, const 
>>>>> char
>>>>>  *name,
>>>>>    			const char *connector_name)
>>>>>    {
>>>>>   @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector
>>>>>  *connector, const char *name,
>>>>>    		fwdata = generic_edid[builtin];
>>>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>>>    	} else {
>>>>>   -		struct platform_device *pdev;
>>>>>   +		struct platform_device *pdev = connector->edid_load_pdev;
>>>>>    		int err;
>>>>> 
>>>>>   -		pdev = platform_device_register_simple(connector_name, -1, 
>>>>> NULL,
>>>>>  0);
>>>>>   -		if (IS_ERR(pdev)) {
>>>>>   -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>>>   -				  "for connector \"%s\"\n", connector_name);
>>>>>   -			return ERR_CAST(pdev);
>>>>>   +		if (WARN_ON(!pdev)) {
>>>>>   +			pdev = edid_pdev(connector_name);
>>>>>   +			if (IS_ERR(pdev))
>>>>>   +				return ERR_CAST(pdev);
>>>>>   +			connector->edid_load_pdev = pdev;
>>>>>    		}
>>>>> 
>>>>>    		err = request_firmware(&fw, name, &pdev->dev);
>>>>>   -		platform_device_unregister(pdev);
>>>>>    		if (err) {
>>>>>    			DRM_ERROR("Requesting EDID firmware \"%s\" failed 
>>>>> (err=%d)\n",
>>>>>    				  name, err);
>>>>>   @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector
>>>>>  *connector, const char *name,
>>>>>    	return edid;
>>>>>    }
>>>>> 
>>>>>   -struct edid *drm_load_edid_firmware(struct drm_connector
>>>>>  *connector)
>>>>>   +static char *edid_name(const char *connector_name)
>>>>>    {
>>>>>   -	const char *connector_name = connector->name;
>>>>>    	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = 
>>>>> NULL;
>>>>>   -	struct edid *edid;
>>>>> 
>>>>>    	if (edid_firmware[0] == '\0')
>>>>>    		return ERR_PTR(-ENOENT);
>>>>>   @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct
>>>>>  drm_connector *connector)
>>>>>    	if (*last == '\n')
>>>>>    		*last = '\0';
>>>>> 
>>>>>   -	edid = edid_load(connector, edidname, connector_name);
>>>>>   +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>>>   +	if (!edidname) {
>>>>>   +		kfree(fwstr);
>>>>>   +		return ERR_PTR(-ENOMEM);
>>>>>   +	}
>>>>>   +
>>>>>    	kfree(fwstr);
>>>>>   +	return edidname;
>>>>>   +}
>>>>>   +
>>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>>>   +{
>>>>>   +	const char *connector_name = connector->name;
>>>>>   +	const char *edidname = edid_name(connector_name);
>>>>>   +	struct platform_device *pdev;
>>>>>   +	int err;
>>>>>   +
>>>>>   +	if (IS_ERR(edidname))
>>>>>   +		return;
>>>>>   +
>>>>>   +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >=
>>>>>  0) {
>>>>>   +		kfree(edidname);
>>>>>   +		return;
>>>>>   +	}
>>>>>   +
>>>>>   +	pdev = edid_pdev(connector_name);
>>>>>   +	if (IS_ERR(pdev)) {
>>>>>   +		kfree(edidname);
>>>>>   +		return;
>>>>>   +	}
>>>>>   +	connector->edid_load_pdev = pdev;
>>>>>   +
>>>>>   +	err = firmware_request_cache(&pdev->dev, edidname);
>>>>>   +	if (err)
>>>>>   +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed
>>>>>  (err=%d)\n",
>>>>>   +			edidname, err);
>>>>>   +
>>>>>   +	kfree(edidname);
>>>>>   +}
>>>>>   +
>>>>>   +struct edid *drm_load_edid_firmware(struct drm_connector
>>>>>  *connector)
>>>>>   +{
>>>>>   +	const char *connector_name = connector->name;
>>>>>   +	const char *edidname = edid_name(connector_name);
>>>>>   +	struct edid *edid;
>>>>>   +
>>>>>   +	if (IS_ERR(edidname))
>>>>>   +		return ERR_CAST(edidname);
>>>>>   +
>>>>>   +	edid = edid_load(connector, edidname, connector_name);
>>>>>   +	kfree(edidname);
>>>>> 
>>>>>    	return edid;
>>>>>    }
>>>>>   diff --git a/include/drm/drm_connector.h
>>>>>  b/include/drm/drm_connector.h
>>>>>   index 3ac4bf87f257..47c84741517e 100644
>>>>>   --- a/include/drm/drm_connector.h
>>>>>   +++ b/include/drm/drm_connector.h
>>>>>   @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>>>    	 */
>>>>>    	struct i2c_adapter *ddc;
>>>>> 
>>>>>   +	/**
>>>>>   +	 * @edid_load_pdev: Platform device for loading EDID via 
>>>>> firmware.
>>>>>   +	 *
>>>>>   +	 * The platform device is registered in drm_connector_init() in
>>>>>  case a
>>>>>   +	 * custom EDID firmware is used with `edid_firmware` parameter.
>>>>>  Otherwise,
>>>>>   +	 * it is set to NULL.
>>>>>   +	 *
>>>>>   +	 * Platform device is unregistered in drm_connector_cleanup() 
>>>>> if
>>>>>  it
>>>>>   +	 * is not NULL.
>>>>>   +	 */
>>>>>   +	struct platform_device *edid_load_pdev;
>>>>>   +
>>>>>    	/**
>>>>>    	 * @null_edid_counter: track sinks that give us all zeros for 
>>>>> the
>>>>>  EDID.
>>>>>    	 * Needed to workaround some HW bugs where we get all 0s
>>>>>   diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>>>   index b2756753370b..e907c928a35d 100644
>>>>>   --- a/include/drm/drm_edid.h
>>>>>   +++ b/include/drm/drm_edid.h
>>>>>   @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector
>>>>>  *connector,
>>>>>    		      const struct drm_display_mode *mode);
>>>>> 
>>>>>    #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>>>    struct edid *drm_load_edid_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
>>>>>   +inline void
>>>>>   +drm_cache_edid_firmware(struct drm_connector *connector);
>>>>>    static inline struct edid *
>>>>>    drm_load_edid_firmware(struct drm_connector *connector)
>>>>>    {
>>>> 
>>>>  --
>>>>  Jani Nikula, Intel Open Source Graphics Center
>>> 
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Jani Nikula]

On Mon, 12 Sep 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> wrote:
> Hi,
>
> Sorry for late reply.
>
> How do you propose to go then?
> Can we still use a persistent platform device to load the firmware and 
> cache it? But, in this case the system will still crash in case the 
> user change drm.edid_firmware then, without replugging the device, he 
> suspends and resumes the machine.
> Or you may have a better idea?

Maybe. Please try [1]. Could it be as simple as that?


BR,
Jani.


[1] https://patchwork.freedesktop.org/patch/msgid/20221006222146.2375217-1-jani.nikula@intel.com

>
> Thanks,
> Matthieu
>
> On Mon, Aug 8 2022 at 08:19:01 PM +0300, Jani Nikula 
> <jani.nikula@linux.intel.com> wrote:
>> On Mon, 08 Aug 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
>> wrote:
>>>  Sorry, What do you mean?
>> 
>> You cache with one name at connector init time, but the name specified
>> using drm.edid_firmware may be changed whenever, to cause the next 
>> EDID
>> read to use a different EDID firmware.
>> 
>> BR,
>> Jani.
>> 
>> 
>>> 
>>>  Matthieu
>>> 
>>>  On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula
>>>  <jani.nikula@linux.intel.com> wrote:
>>>>  On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>  <mailto:matthieu.charette@gmail.com>> 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.
>>>> 
>>>>  AFAICT this breaks changing drm.edid_firmware runtime.
>>>> 
>>>>  BR,
>>>>  Jani.
>>>> 
>>>>> 
>>>>>   Link: <https://gitlab.freedesktop.org/drm/amd/-/issues/2061>
>>>>>   Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com
>>>>>  <mailto:matthieu.charette@gmail.com>>
>>>>>   ---
>>>>>    drivers/gpu/drm/drm_connector.c |  9 ++++
>>>>>    drivers/gpu/drm/drm_edid_load.c | 81
>>>>>  ++++++++++++++++++++++++++++-----
>>>>>    include/drm/drm_connector.h     | 12 +++++
>>>>>    include/drm/drm_edid.h          |  3 ++
>>>>>    4 files changed, 94 insertions(+), 11 deletions(-)
>>>>> 
>>>>>   diff --git a/drivers/gpu/drm/drm_connector.c
>>>>>  b/drivers/gpu/drm/drm_connector.c
>>>>>   index 1c48d162c77e..e8819ebf1c4b 100644
>>>>>   --- a/drivers/gpu/drm/drm_connector.c
>>>>>   +++ b/drivers/gpu/drm/drm_connector.c
>>>>>   @@ -31,6 +31,7 @@
>>>>>    #include <drm/drm_privacy_screen_consumer.h>
>>>>>    #include <drm/drm_sysfs.h>
>>>>> 
>>>>>   +#include <linux/platform_device.h>
>>>>>    #include <linux/uaccess.h>
>>>>> 
>>>>>    #include "drm_crtc_internal.h"
>>>>>   @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device 
>>>>> *dev,
>>>>> 
>>>>>    	drm_connector_get_cmdline_mode(connector);
>>>>> 
>>>>>   +	connector->edid_load_pdev = NULL;
>>>>>   +	drm_cache_edid_firmware(connector);
>>>>>   +
>>>>>    	/* We should add connectors at the end to avoid upsetting the
>>>>>  connector
>>>>>    	 * index too much.
>>>>>    	 */
>>>>>   @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct
>>>>>  drm_connector *connector)
>>>>>    		connector->tile_group = NULL;
>>>>>    	}
>>>>> 
>>>>>   +	if (connector->edid_load_pdev) {
>>>>>   +		platform_device_unregister(connector->edid_load_pdev);
>>>>>   +		connector->edid_load_pdev = NULL;
>>>>>   +	}
>>>>>   +
>>>>>    	list_for_each_entry_safe(mode, t, &connector->probed_modes, 
>>>>> head)
>>>>>    		drm_mode_remove(connector, mode);
>>>>> 
>>>>>   diff --git a/drivers/gpu/drm/drm_edid_load.c
>>>>>  b/drivers/gpu/drm/drm_edid_load.c
>>>>>   index 37d8ba3ddb46..5a82be9917ec 100644
>>>>>   --- a/drivers/gpu/drm/drm_edid_load.c
>>>>>   +++ b/drivers/gpu/drm/drm_edid_load.c
>>>>>   @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int
>>>>>  data_size)
>>>>>    	return (edid[0x7e] + 1) * EDID_LENGTH;
>>>>>    }
>>>>> 
>>>>>   +static struct platform_device *edid_pdev(const char
>>>>>  *connector_name)
>>>>>   +{
>>>>>   +	struct platform_device *pdev =
>>>>>  platform_device_register_simple(connector_name, -1, NULL, 0);
>>>>>   +
>>>>>   +	if (IS_ERR(pdev)) {
>>>>>   +		DRM_ERROR("Failed to register EDID firmware platform device "
>>>>>   +			"for connector \"%s\"\n", connector_name);
>>>>>   +		return ERR_CAST(pdev);
>>>>>   +	}
>>>>>   +
>>>>>   +	return pdev;
>>>>>   +}
>>>>>   +
>>>>>    static void *edid_load(struct drm_connector *connector, const 
>>>>> char
>>>>>  *name,
>>>>>    			const char *connector_name)
>>>>>    {
>>>>>   @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector
>>>>>  *connector, const char *name,
>>>>>    		fwdata = generic_edid[builtin];
>>>>>    		fwsize = sizeof(generic_edid[builtin]);
>>>>>    	} else {
>>>>>   -		struct platform_device *pdev;
>>>>>   +		struct platform_device *pdev = connector->edid_load_pdev;
>>>>>    		int err;
>>>>> 
>>>>>   -		pdev = platform_device_register_simple(connector_name, -1, 
>>>>> NULL,
>>>>>  0);
>>>>>   -		if (IS_ERR(pdev)) {
>>>>>   -			DRM_ERROR("Failed to register EDID firmware platform device "
>>>>>   -				  "for connector \"%s\"\n", connector_name);
>>>>>   -			return ERR_CAST(pdev);
>>>>>   +		if (WARN_ON(!pdev)) {
>>>>>   +			pdev = edid_pdev(connector_name);
>>>>>   +			if (IS_ERR(pdev))
>>>>>   +				return ERR_CAST(pdev);
>>>>>   +			connector->edid_load_pdev = pdev;
>>>>>    		}
>>>>> 
>>>>>    		err = request_firmware(&fw, name, &pdev->dev);
>>>>>   -		platform_device_unregister(pdev);
>>>>>    		if (err) {
>>>>>    			DRM_ERROR("Requesting EDID firmware \"%s\" failed 
>>>>> (err=%d)\n",
>>>>>    				  name, err);
>>>>>   @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector
>>>>>  *connector, const char *name,
>>>>>    	return edid;
>>>>>    }
>>>>> 
>>>>>   -struct edid *drm_load_edid_firmware(struct drm_connector
>>>>>  *connector)
>>>>>   +static char *edid_name(const char *connector_name)
>>>>>    {
>>>>>   -	const char *connector_name = connector->name;
>>>>>    	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = 
>>>>> NULL;
>>>>>   -	struct edid *edid;
>>>>> 
>>>>>    	if (edid_firmware[0] == '\0')
>>>>>    		return ERR_PTR(-ENOENT);
>>>>>   @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct
>>>>>  drm_connector *connector)
>>>>>    	if (*last == '\n')
>>>>>    		*last = '\0';
>>>>> 
>>>>>   -	edid = edid_load(connector, edidname, connector_name);
>>>>>   +	edidname = kstrdup(edidname, GFP_KERNEL);
>>>>>   +	if (!edidname) {
>>>>>   +		kfree(fwstr);
>>>>>   +		return ERR_PTR(-ENOMEM);
>>>>>   +	}
>>>>>   +
>>>>>    	kfree(fwstr);
>>>>>   +	return edidname;
>>>>>   +}
>>>>>   +
>>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector)
>>>>>   +{
>>>>>   +	const char *connector_name = connector->name;
>>>>>   +	const char *edidname = edid_name(connector_name);
>>>>>   +	struct platform_device *pdev;
>>>>>   +	int err;
>>>>>   +
>>>>>   +	if (IS_ERR(edidname))
>>>>>   +		return;
>>>>>   +
>>>>>   +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >=
>>>>>  0) {
>>>>>   +		kfree(edidname);
>>>>>   +		return;
>>>>>   +	}
>>>>>   +
>>>>>   +	pdev = edid_pdev(connector_name);
>>>>>   +	if (IS_ERR(pdev)) {
>>>>>   +		kfree(edidname);
>>>>>   +		return;
>>>>>   +	}
>>>>>   +	connector->edid_load_pdev = pdev;
>>>>>   +
>>>>>   +	err = firmware_request_cache(&pdev->dev, edidname);
>>>>>   +	if (err)
>>>>>   +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed
>>>>>  (err=%d)\n",
>>>>>   +			edidname, err);
>>>>>   +
>>>>>   +	kfree(edidname);
>>>>>   +}
>>>>>   +
>>>>>   +struct edid *drm_load_edid_firmware(struct drm_connector
>>>>>  *connector)
>>>>>   +{
>>>>>   +	const char *connector_name = connector->name;
>>>>>   +	const char *edidname = edid_name(connector_name);
>>>>>   +	struct edid *edid;
>>>>>   +
>>>>>   +	if (IS_ERR(edidname))
>>>>>   +		return ERR_CAST(edidname);
>>>>>   +
>>>>>   +	edid = edid_load(connector, edidname, connector_name);
>>>>>   +	kfree(edidname);
>>>>> 
>>>>>    	return edid;
>>>>>    }
>>>>>   diff --git a/include/drm/drm_connector.h
>>>>>  b/include/drm/drm_connector.h
>>>>>   index 3ac4bf87f257..47c84741517e 100644
>>>>>   --- a/include/drm/drm_connector.h
>>>>>   +++ b/include/drm/drm_connector.h
>>>>>   @@ -1573,6 +1573,18 @@ struct drm_connector {
>>>>>    	 */
>>>>>    	struct i2c_adapter *ddc;
>>>>> 
>>>>>   +	/**
>>>>>   +	 * @edid_load_pdev: Platform device for loading EDID via 
>>>>> firmware.
>>>>>   +	 *
>>>>>   +	 * The platform device is registered in drm_connector_init() in
>>>>>  case a
>>>>>   +	 * custom EDID firmware is used with `edid_firmware` parameter.
>>>>>  Otherwise,
>>>>>   +	 * it is set to NULL.
>>>>>   +	 *
>>>>>   +	 * Platform device is unregistered in drm_connector_cleanup() 
>>>>> if
>>>>>  it
>>>>>   +	 * is not NULL.
>>>>>   +	 */
>>>>>   +	struct platform_device *edid_load_pdev;
>>>>>   +
>>>>>    	/**
>>>>>    	 * @null_edid_counter: track sinks that give us all zeros for 
>>>>> the
>>>>>  EDID.
>>>>>    	 * Needed to workaround some HW bugs where we get all 0s
>>>>>   diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>>>   index b2756753370b..e907c928a35d 100644
>>>>>   --- a/include/drm/drm_edid.h
>>>>>   +++ b/include/drm/drm_edid.h
>>>>>   @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector
>>>>>  *connector,
>>>>>    		      const struct drm_display_mode *mode);
>>>>> 
>>>>>    #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>>>>   +void drm_cache_edid_firmware(struct drm_connector *connector);
>>>>>    struct edid *drm_load_edid_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
>>>>>   +inline void
>>>>>   +drm_cache_edid_firmware(struct drm_connector *connector);
>>>>>    static inline struct edid *
>>>>>    drm_load_edid_firmware(struct drm_connector *connector)
>>>>>    {
>>>> 
>>>>  --
>>>>  Jani Nikula, Intel Open Source Graphics Center
>>> 
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

Sorry, What do you mean?

Matthieu

On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> 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.
> 
> AFAICT this breaks changing drm.edid_firmware runtime.
> 
> BR,
> Jani.
> 
>> 
>>  Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>  ---
>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>   drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     | 12 +++++
>>   include/drm/drm_edid.h          |  3 ++
>>   4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>   #include <drm/drm_privacy_screen_consumer.h>
>>   #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>   #include <linux/uaccess.h>
>> 
>>   #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   	drm_connector_get_cmdline_mode(connector);
>> 
>>  +	connector->edid_load_pdev = NULL;
>>  +	drm_cache_edid_firmware(connector);
>>  +
>>   	/* We should add connectors at the end to avoid upsetting the 
>> connector
>>   	 * index too much.
>>   	 */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>> drm_connector *connector)
>>   		connector->tile_group = NULL;
>>   	}
>> 
>>  +	if (connector->edid_load_pdev) {
>>  +		platform_device_unregister(connector->edid_load_pdev);
>>  +		connector->edid_load_pdev = NULL;
>>  +	}
>>  +
>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>   		drm_mode_remove(connector, mode);
>> 
>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>> b/drivers/gpu/drm/drm_edid_load.c
>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>> data_size)
>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>   }
>> 
>>  +static struct platform_device *edid_pdev(const char 
>> *connector_name)
>>  +{
>>  +	struct platform_device *pdev = 
>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>  +
>>  +	if (IS_ERR(pdev)) {
>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>  +			"for connector \"%s\"\n", connector_name);
>>  +		return ERR_CAST(pdev);
>>  +	}
>>  +
>>  +	return pdev;
>>  +}
>>  +
>>   static void *edid_load(struct drm_connector *connector, const char 
>> *name,
>>   			const char *connector_name)
>>   {
>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   		fwdata = generic_edid[builtin];
>>   		fwsize = sizeof(generic_edid[builtin]);
>>   	} else {
>>  -		struct platform_device *pdev;
>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>   		int err;
>> 
>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>> 0);
>>  -		if (IS_ERR(pdev)) {
>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>  -				  "for connector \"%s\"\n", connector_name);
>>  -			return ERR_CAST(pdev);
>>  +		if (WARN_ON(!pdev)) {
>>  +			pdev = edid_pdev(connector_name);
>>  +			if (IS_ERR(pdev))
>>  +				return ERR_CAST(pdev);
>>  +			connector->edid_load_pdev = pdev;
>>   		}
>> 
>>   		err = request_firmware(&fw, name, &pdev->dev);
>>  -		platform_device_unregister(pdev);
>>   		if (err) {
>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>   				  name, err);
>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   	return edid;
>>   }
>> 
>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +static char *edid_name(const char *connector_name)
>>   {
>>  -	const char *connector_name = connector->name;
>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>  -	struct edid *edid;
>> 
>>   	if (edid_firmware[0] == '\0')
>>   		return ERR_PTR(-ENOENT);
>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>> drm_connector *connector)
>>   	if (*last == '\n')
>>   		*last = '\0';
>> 
>>  -	edid = edid_load(connector, edidname, connector_name);
>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>  +	if (!edidname) {
>>  +		kfree(fwstr);
>>  +		return ERR_PTR(-ENOMEM);
>>  +	}
>>  +
>>   	kfree(fwstr);
>>  +	return edidname;
>>  +}
>>  +
>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct platform_device *pdev;
>>  +	int err;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return;
>>  +
>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>> 0) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +
>>  +	pdev = edid_pdev(connector_name);
>>  +	if (IS_ERR(pdev)) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +	connector->edid_load_pdev = pdev;
>>  +
>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>  +	if (err)
>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>> (err=%d)\n",
>>  +			edidname, err);
>>  +
>>  +	kfree(edidname);
>>  +}
>>  +
>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct edid *edid;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return ERR_CAST(edidname);
>>  +
>>  +	edid = edid_load(connector, edidname, connector_name);
>>  +	kfree(edidname);
>> 
>>   	return edid;
>>   }
>>  diff --git a/include/drm/drm_connector.h 
>> b/include/drm/drm_connector.h
>>  index 3ac4bf87f257..47c84741517e 100644
>>  --- a/include/drm/drm_connector.h
>>  +++ b/include/drm/drm_connector.h
>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>   	 */
>>   	struct i2c_adapter *ddc;
>> 
>>  +	/**
>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>  +	 *
>>  +	 * The platform device is registered in drm_connector_init() in 
>> case a
>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>> Otherwise,
>>  +	 * it is set to NULL.
>>  +	 *
>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>> it
>>  +	 * is not NULL.
>>  +	 */
>>  +	struct platform_device *edid_load_pdev;
>>  +
>>   	/**
>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>> EDID.
>>   	 * Needed to workaround some HW bugs where we get all 0s
>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>  index b2756753370b..e907c928a35d 100644
>>  --- a/include/drm/drm_edid.h
>>  +++ b/include/drm/drm_edid.h
>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>> *connector,
>>   		      const struct drm_display_mode *mode);
>> 
>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>   struct edid *drm_load_edid_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
>>  +inline void
>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>   static inline struct edid *
>>   drm_load_edid_firmware(struct drm_connector *connector)
>>   {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

[-- Attachment #1: Type: text/plain, Size: 8456 bytes --]

Sorry, What do you mean?

Matthieu

On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> 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.
> 
> AFAICT this breaks changing drm.edid_firmware runtime.
> 
> BR,
> Jani.
> 
>> 
>>  Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>  ---
>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>   drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     | 12 +++++
>>   include/drm/drm_edid.h          |  3 ++
>>   4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>   #include <drm/drm_privacy_screen_consumer.h>
>>   #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>   #include <linux/uaccess.h>
>> 
>>   #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   	drm_connector_get_cmdline_mode(connector);
>> 
>>  +	connector->edid_load_pdev = NULL;
>>  +	drm_cache_edid_firmware(connector);
>>  +
>>   	/* We should add connectors at the end to avoid upsetting the 
>> connector
>>   	 * index too much.
>>   	 */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>> drm_connector *connector)
>>   		connector->tile_group = NULL;
>>   	}
>> 
>>  +	if (connector->edid_load_pdev) {
>>  +		platform_device_unregister(connector->edid_load_pdev);
>>  +		connector->edid_load_pdev = NULL;
>>  +	}
>>  +
>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>   		drm_mode_remove(connector, mode);
>> 
>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>> b/drivers/gpu/drm/drm_edid_load.c
>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>> data_size)
>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>   }
>> 
>>  +static struct platform_device *edid_pdev(const char 
>> *connector_name)
>>  +{
>>  +	struct platform_device *pdev = 
>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>  +
>>  +	if (IS_ERR(pdev)) {
>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>  +			"for connector \"%s\"\n", connector_name);
>>  +		return ERR_CAST(pdev);
>>  +	}
>>  +
>>  +	return pdev;
>>  +}
>>  +
>>   static void *edid_load(struct drm_connector *connector, const char 
>> *name,
>>   			const char *connector_name)
>>   {
>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   		fwdata = generic_edid[builtin];
>>   		fwsize = sizeof(generic_edid[builtin]);
>>   	} else {
>>  -		struct platform_device *pdev;
>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>   		int err;
>> 
>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>> 0);
>>  -		if (IS_ERR(pdev)) {
>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>  -				  "for connector \"%s\"\n", connector_name);
>>  -			return ERR_CAST(pdev);
>>  +		if (WARN_ON(!pdev)) {
>>  +			pdev = edid_pdev(connector_name);
>>  +			if (IS_ERR(pdev))
>>  +				return ERR_CAST(pdev);
>>  +			connector->edid_load_pdev = pdev;
>>   		}
>> 
>>   		err = request_firmware(&fw, name, &pdev->dev);
>>  -		platform_device_unregister(pdev);
>>   		if (err) {
>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>   				  name, err);
>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   	return edid;
>>   }
>> 
>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +static char *edid_name(const char *connector_name)
>>   {
>>  -	const char *connector_name = connector->name;
>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>  -	struct edid *edid;
>> 
>>   	if (edid_firmware[0] == '\0')
>>   		return ERR_PTR(-ENOENT);
>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>> drm_connector *connector)
>>   	if (*last == '\n')
>>   		*last = '\0';
>> 
>>  -	edid = edid_load(connector, edidname, connector_name);
>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>  +	if (!edidname) {
>>  +		kfree(fwstr);
>>  +		return ERR_PTR(-ENOMEM);
>>  +	}
>>  +
>>   	kfree(fwstr);
>>  +	return edidname;
>>  +}
>>  +
>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct platform_device *pdev;
>>  +	int err;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return;
>>  +
>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>> 0) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +
>>  +	pdev = edid_pdev(connector_name);
>>  +	if (IS_ERR(pdev)) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +	connector->edid_load_pdev = pdev;
>>  +
>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>  +	if (err)
>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>> (err=%d)\n",
>>  +			edidname, err);
>>  +
>>  +	kfree(edidname);
>>  +}
>>  +
>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct edid *edid;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return ERR_CAST(edidname);
>>  +
>>  +	edid = edid_load(connector, edidname, connector_name);
>>  +	kfree(edidname);
>> 
>>   	return edid;
>>   }
>>  diff --git a/include/drm/drm_connector.h 
>> b/include/drm/drm_connector.h
>>  index 3ac4bf87f257..47c84741517e 100644
>>  --- a/include/drm/drm_connector.h
>>  +++ b/include/drm/drm_connector.h
>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>   	 */
>>   	struct i2c_adapter *ddc;
>> 
>>  +	/**
>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>  +	 *
>>  +	 * The platform device is registered in drm_connector_init() in 
>> case a
>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>> Otherwise,
>>  +	 * it is set to NULL.
>>  +	 *
>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>> it
>>  +	 * is not NULL.
>>  +	 */
>>  +	struct platform_device *edid_load_pdev;
>>  +
>>   	/**
>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>> EDID.
>>   	 * Needed to workaround some HW bugs where we get all 0s
>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>  index b2756753370b..e907c928a35d 100644
>>  --- a/include/drm/drm_edid.h
>>  +++ b/include/drm/drm_edid.h
>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>> *connector,
>>   		      const struct drm_display_mode *mode);
>> 
>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>   struct edid *drm_load_edid_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
>>  +inline void
>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>   static inline struct edid *
>>   drm_load_edid_firmware(struct drm_connector *connector)
>>   {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

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

[Matthieu CHARETTE]

Sorry, What do you mean?

Matthieu

On Tue, Aug 2 2022 at 05:29:12 PM +0300, Jani Nikula 
<jani.nikula@linux.intel.com> wrote:
> On Wed, 27 Jul 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
> 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.
> 
> AFAICT this breaks changing drm.edid_firmware runtime.
> 
> BR,
> Jani.
> 
>> 
>>  Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
>>  Signed-off-by: Matthieu CHARETTE <matthieu.charette@gmail.com>
>>  ---
>>   drivers/gpu/drm/drm_connector.c |  9 ++++
>>   drivers/gpu/drm/drm_edid_load.c | 81 
>> ++++++++++++++++++++++++++++-----
>>   include/drm/drm_connector.h     | 12 +++++
>>   include/drm/drm_edid.h          |  3 ++
>>   4 files changed, 94 insertions(+), 11 deletions(-)
>> 
>>  diff --git a/drivers/gpu/drm/drm_connector.c 
>> b/drivers/gpu/drm/drm_connector.c
>>  index 1c48d162c77e..e8819ebf1c4b 100644
>>  --- a/drivers/gpu/drm/drm_connector.c
>>  +++ b/drivers/gpu/drm/drm_connector.c
>>  @@ -31,6 +31,7 @@
>>   #include <drm/drm_privacy_screen_consumer.h>
>>   #include <drm/drm_sysfs.h>
>> 
>>  +#include <linux/platform_device.h>
>>   #include <linux/uaccess.h>
>> 
>>   #include "drm_crtc_internal.h"
>>  @@ -289,6 +290,9 @@ int drm_connector_init(struct drm_device *dev,
>> 
>>   	drm_connector_get_cmdline_mode(connector);
>> 
>>  +	connector->edid_load_pdev = NULL;
>>  +	drm_cache_edid_firmware(connector);
>>  +
>>   	/* We should add connectors at the end to avoid upsetting the 
>> connector
>>   	 * index too much.
>>   	 */
>>  @@ -473,6 +477,11 @@ void drm_connector_cleanup(struct 
>> drm_connector *connector)
>>   		connector->tile_group = NULL;
>>   	}
>> 
>>  +	if (connector->edid_load_pdev) {
>>  +		platform_device_unregister(connector->edid_load_pdev);
>>  +		connector->edid_load_pdev = NULL;
>>  +	}
>>  +
>>   	list_for_each_entry_safe(mode, t, &connector->probed_modes, head)
>>   		drm_mode_remove(connector, mode);
>> 
>>  diff --git a/drivers/gpu/drm/drm_edid_load.c 
>> b/drivers/gpu/drm/drm_edid_load.c
>>  index 37d8ba3ddb46..5a82be9917ec 100644
>>  --- a/drivers/gpu/drm/drm_edid_load.c
>>  +++ b/drivers/gpu/drm/drm_edid_load.c
>>  @@ -167,6 +167,19 @@ static int edid_size(const u8 *edid, int 
>> data_size)
>>   	return (edid[0x7e] + 1) * EDID_LENGTH;
>>   }
>> 
>>  +static struct platform_device *edid_pdev(const char 
>> *connector_name)
>>  +{
>>  +	struct platform_device *pdev = 
>> platform_device_register_simple(connector_name, -1, NULL, 0);
>>  +
>>  +	if (IS_ERR(pdev)) {
>>  +		DRM_ERROR("Failed to register EDID firmware platform device "
>>  +			"for connector \"%s\"\n", connector_name);
>>  +		return ERR_CAST(pdev);
>>  +	}
>>  +
>>  +	return pdev;
>>  +}
>>  +
>>   static void *edid_load(struct drm_connector *connector, const char 
>> *name,
>>   			const char *connector_name)
>>   {
>>  @@ -182,18 +195,17 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   		fwdata = generic_edid[builtin];
>>   		fwsize = sizeof(generic_edid[builtin]);
>>   	} else {
>>  -		struct platform_device *pdev;
>>  +		struct platform_device *pdev = connector->edid_load_pdev;
>>   		int err;
>> 
>>  -		pdev = platform_device_register_simple(connector_name, -1, NULL, 
>> 0);
>>  -		if (IS_ERR(pdev)) {
>>  -			DRM_ERROR("Failed to register EDID firmware platform device "
>>  -				  "for connector \"%s\"\n", connector_name);
>>  -			return ERR_CAST(pdev);
>>  +		if (WARN_ON(!pdev)) {
>>  +			pdev = edid_pdev(connector_name);
>>  +			if (IS_ERR(pdev))
>>  +				return ERR_CAST(pdev);
>>  +			connector->edid_load_pdev = pdev;
>>   		}
>> 
>>   		err = request_firmware(&fw, name, &pdev->dev);
>>  -		platform_device_unregister(pdev);
>>   		if (err) {
>>   			DRM_ERROR("Requesting EDID firmware \"%s\" failed (err=%d)\n",
>>   				  name, err);
>>  @@ -263,11 +275,9 @@ static void *edid_load(struct drm_connector 
>> *connector, const char *name,
>>   	return edid;
>>   }
>> 
>>  -struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +static char *edid_name(const char *connector_name)
>>   {
>>  -	const char *connector_name = connector->name;
>>   	char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
>>  -	struct edid *edid;
>> 
>>   	if (edid_firmware[0] == '\0')
>>   		return ERR_PTR(-ENOENT);
>>  @@ -310,8 +320,57 @@ struct edid *drm_load_edid_firmware(struct 
>> drm_connector *connector)
>>   	if (*last == '\n')
>>   		*last = '\0';
>> 
>>  -	edid = edid_load(connector, edidname, connector_name);
>>  +	edidname = kstrdup(edidname, GFP_KERNEL);
>>  +	if (!edidname) {
>>  +		kfree(fwstr);
>>  +		return ERR_PTR(-ENOMEM);
>>  +	}
>>  +
>>   	kfree(fwstr);
>>  +	return edidname;
>>  +}
>>  +
>>  +void drm_cache_edid_firmware(struct drm_connector *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct platform_device *pdev;
>>  +	int err;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return;
>>  +
>>  +	if (match_string(generic_edid_name, GENERIC_EDIDS, edidname) >= 
>> 0) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +
>>  +	pdev = edid_pdev(connector_name);
>>  +	if (IS_ERR(pdev)) {
>>  +		kfree(edidname);
>>  +		return;
>>  +	}
>>  +	connector->edid_load_pdev = pdev;
>>  +
>>  +	err = firmware_request_cache(&pdev->dev, edidname);
>>  +	if (err)
>>  +		DRM_ERROR("Requesting EDID firmware cache \"%s\" failed 
>> (err=%d)\n",
>>  +			edidname, err);
>>  +
>>  +	kfree(edidname);
>>  +}
>>  +
>>  +struct edid *drm_load_edid_firmware(struct drm_connector 
>> *connector)
>>  +{
>>  +	const char *connector_name = connector->name;
>>  +	const char *edidname = edid_name(connector_name);
>>  +	struct edid *edid;
>>  +
>>  +	if (IS_ERR(edidname))
>>  +		return ERR_CAST(edidname);
>>  +
>>  +	edid = edid_load(connector, edidname, connector_name);
>>  +	kfree(edidname);
>> 
>>   	return edid;
>>   }
>>  diff --git a/include/drm/drm_connector.h 
>> b/include/drm/drm_connector.h
>>  index 3ac4bf87f257..47c84741517e 100644
>>  --- a/include/drm/drm_connector.h
>>  +++ b/include/drm/drm_connector.h
>>  @@ -1573,6 +1573,18 @@ struct drm_connector {
>>   	 */
>>   	struct i2c_adapter *ddc;
>> 
>>  +	/**
>>  +	 * @edid_load_pdev: Platform device for loading EDID via firmware.
>>  +	 *
>>  +	 * The platform device is registered in drm_connector_init() in 
>> case a
>>  +	 * custom EDID firmware is used with `edid_firmware` parameter. 
>> Otherwise,
>>  +	 * it is set to NULL.
>>  +	 *
>>  +	 * Platform device is unregistered in drm_connector_cleanup() if 
>> it
>>  +	 * is not NULL.
>>  +	 */
>>  +	struct platform_device *edid_load_pdev;
>>  +
>>   	/**
>>   	 * @null_edid_counter: track sinks that give us all zeros for the 
>> EDID.
>>   	 * Needed to workaround some HW bugs where we get all 0s
>>  diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>  index b2756753370b..e907c928a35d 100644
>>  --- a/include/drm/drm_edid.h
>>  +++ b/include/drm/drm_edid.h
>>  @@ -378,10 +378,13 @@ int drm_av_sync_delay(struct drm_connector 
>> *connector,
>>   		      const struct drm_display_mode *mode);
>> 
>>   #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>>  +void drm_cache_edid_firmware(struct drm_connector *connector);
>>   struct edid *drm_load_edid_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
>>  +inline void
>>  +drm_cache_edid_firmware(struct drm_connector *connector);
>>   static inline struct edid *
>>   drm_load_edid_firmware(struct drm_connector *connector)
>>   {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center