drm/edid: transparent low-level override/firmware EDIDs

Handle debugfs override edid and firmware edid at the low level to transparently and completely replace the real edid. Previously, we practically only used the modes from the override EDID, and none of the other data, such as audio parameters.

This change also prevents actual EDID reads when the EDID is to be overridden, but retains the DDC probe. This is useful if the reason for preferring override EDID are problems with reading the data, or corruption of the data.

Move firmware EDID loading from helper to core, as the functionality moves to lower level as well. This will result in a change of module parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which arguably makes more sense anyway.

Some future work remains related to override and firmware EDID validation. Like before, no validation is done for override EDID. The firmware EDID is validated separately in the loader. Some unification and deduplication would be in order, to validate all of them at the drm_do_get_edid() level, like "real" EDIDs.

v2: move firmware loading to core

v3: rebase, commit message refresh

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt |  2 +-
 drivers/gpu/drm/Kconfig                         |  2 +-
 drivers/gpu/drm/Makefile                        |  2 +-
 drivers/gpu/drm/drm_edid.c                      | 15 +++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c              | 19 +------------------
 5 files changed, 19 insertions(+), 21 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +1 -1

index d9c171ce4190..9b393c29953f 100644
@@ -854,7 +854,7 @@
 			The filter can be disabled or changed to another
 			driver later using sysfs.
 
-	drm_kms_helper.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
+	drm.edid_firmware=[<connector>:]<file>[,[<connector>:]<file>]
 			Broken monitors, graphic adapters, KVMs and EDIDless
 			panels may send no or incorrect EDID data sets.
 			This parameter allows to specify an EDID data sets

drivers/gpu/drm/Kconfig +1 -1

index c5e1a8409285..c9f09fc298bb 100644
@@ -110,7 +110,7 @@ config DRM_FBDEV_OVERALLOC
 
 config DRM_LOAD_EDID_FIRMWARE
 	bool "Allow to specify an EDID data set instead of probing for it"
-	depends on DRM_KMS_HELPER
+	depends on DRM
 	help
 	  Say Y here, if you want to use EDID data to be loaded from the
 	  /lib/firmware directory or one of the provided built-in

drivers/gpu/drm/Makefile +1 -1

index df923119ac36..0ee184f56a60 100644
@@ -28,6 +28,7 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
+drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \

@@ -36,7 +37,6 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_scdc_helper.o drm_gem_framebuffer_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
-drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
 drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o

drivers/gpu/drm/drm_edid.c +15 -0

index 6bb6337be920..00ddabfbf980 100644
@@ -1533,6 +1533,10 @@ static void connector_bad_edid(struct drm_connector *connector,
  * level, drivers must make all reasonable efforts to expose it as an I2C
  * adapter and use drm_get_edid() instead of abusing this function.
  *
+ * The EDID may be overridden using debugfs override_edid or firmare EDID
+ * (drm_load_edid_firmware() and drm.edid_firmware parameter), in this priority
+ * order. Having either of them bypasses actual EDID reads.
+ *
  * Return: Pointer to valid EDID or NULL if we couldn't find any.
  */
 struct edid *drm_do_get_edid(struct drm_connector *connector,

@@ -1542,6 +1546,17 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
+	struct edid *override = NULL;
+
+	if (connector->override_edid)
+		override = drm_edid_duplicate((const struct edid *)
+					      connector->edid_blob_ptr->data);
+
+	if (!override)
+		override = drm_load_edid_firmware(connector);
+
+	if (!IS_ERR_OR_NULL(override))
+		return override;
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
 		return NULL;

drivers/gpu/drm/drm_probe_helper.c +1 -18

index 904966cde32b..5840aabbf24e 100644
@@ -353,8 +353,6 @@ EXPORT_SYMBOL(drm_helper_probe_detect);
  *    drm_mode_probed_add(). New modes start their life with status as OK.
  *    Modes are added from a single source using the following priority order.
  *
- *    - debugfs 'override_edid' (used for testing only)
- *    - firmware EDID (drm_load_edid_firmware())
  *    - &drm_connector_helper_funcs.get_modes vfunc
  *    - if the connector status is connector_status_connected, standard
  *      VESA DMT modes up to 1024x768 are automatically added

@@ -483,22 +481,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		goto prune;
 	}
 
-	if (connector->override_edid) {
-		struct edid *edid = (struct edid *) connector->edid_blob_ptr->data;
-
-		count = drm_add_edid_modes(connector, edid);
-		drm_edid_to_eld(connector, edid);
-	} else {
-		struct edid *edid = drm_load_edid_firmware(connector);
-		if (!IS_ERR_OR_NULL(edid)) {
-			drm_mode_connector_update_edid_property(connector, edid);
-			count = drm_add_edid_modes(connector, edid);
-			drm_edid_to_eld(connector, edid);
-			kfree(edid);
-		}
-		if (count == 0)
-			count = (*connector_funcs->get_modes)(connector);
-	}
+	count = (*connector_funcs->get_modes)(connector);
 
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);

Signature · 1 line

2.11.0

If drm_kms_helper.edid_firmware module parameter is set at drm_kms_helper probe time, update the new drm.edid_firmware parameter for backwards compatibility.

The drm_kms_helper.edid_firmware is now read-only in sysfs to prevent runtime updates.

This is a sort of easy middle ground; adding a full runtime support via /sys/module/drm_kms_helper/parameters/edid_firmware would be possible, but considerably more and uglier code.

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---

I'm wondering if the change from drm_kms_helper.edid_firmware to drm.edid_firmware is enough of an ABI breakage to warrant something like this patch.

---
 drivers/gpu/drm/drm_edid_load.c         |  7 +++++++
 drivers/gpu/drm/drm_kms_helper_common.c | 19 +++++++++++++++++++
 include/drm/drm_edid.h                  |  2 ++
 3 files changed, 28 insertions(+)

drivers/gpu/drm/drm_edid_load.c +7 -0

index 1c0495acf341..a94005529cd4 100644
@@ -31,6 +31,13 @@ module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
 	"from built-in data or /lib/firmware instead. ");
 
+/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
+void __drm_set_edid_firmware_path(const char *path)
+{
+	strlcpy(edid_firmware, path, sizeof(edid_firmware));
+}
+EXPORT_SYMBOL(__drm_set_edid_firmware_path);
+
 #define GENERIC_EDIDS 6
 static const char * const generic_edid_name[GENERIC_EDIDS] = {
 	"edid/800x600.bin",

drivers/gpu/drm/drm_kms_helper_common.c +19 -0

index 6e35a56a6102..6130199a83b6 100644
@@ -26,6 +26,7 @@
  */
 
 #include <linux/module.h>
+#include <drm/drmP.h>
 
 #include "drm_crtc_helper_internal.h"
 

@@ -33,10 +34,28 @@ MODULE_AUTHOR("David Airlie, Jesse Barnes");
 MODULE_DESCRIPTION("DRM KMS helper");
 MODULE_LICENSE("GPL and additional rights");
 
+#if IS_ENABLED(CONFIG_DRM_LOAD_EDID_FIRMWARE)
+static char edid_firmware[PATH_MAX];
+module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0444);
+MODULE_PARM_DESC(edid_firmware, "DEPRECATED. Use drm.edid_firmware instead.");
+static inline void legacy_set_edid_firmare_path(void)
+{
+	if (edid_firmware[0]) {
+		DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, "
+			 "please use drm.edid_firmware intead.\n");
+		__drm_set_edid_firmware_path(edid_firmware);
+	}
+}
+#else
+static inline void legacy_set_edid_firmare_path(void) {}
+#endif
+
 static int __init drm_kms_helper_init(void)
 {
 	int ret;
 
+	legacy_set_edid_firmare_path();
+
 	/* Call init functions from specific kms helpers here */
 	ret = drm_fb_helper_modinit();
 	if (ret < 0)

include/drm/drm_edid.h +2 -0

index 1e1908a6b1d6..cdfb793255f0 100644
@@ -341,12 +341,14 @@ int drm_av_sync_delay(struct drm_connector *connector,
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+void __drm_set_edid_firmware_path(const char *path);
 #else
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline void __drm_set_edid_firmware_path(const char *path) {}
 #endif
 
 int

Signature · 1 line

2.11.0

Hmm. Wouldn't you just have to have custom kernel_param_ops and that's about it? Seems like that could be a bit cleaner.

Add drm_kms_helper.edid_firmware module parameter with param ops hooks to set drm.edid_firmware instead, for backwards compatibility.

Suggested-by Ville Syrjälä <[email protected]>

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid_load.c         | 16 ++++++++++++++++
 drivers/gpu/drm/drm_kms_helper_common.c | 28 ++++++++++++++++++++++++++++
 include/drm/drm_edid.h                  |  2 ++
 3 files changed, 46 insertions(+)

drivers/gpu/drm/drm_edid_load.c +3 -0

index 1c0495acf341..a4915099aaa9 100644
@@ -31,6 +31,22 @@ module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
 	"from built-in data or /lib/firmware instead. ");
 
+/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
+int __drm_set_edid_firmware_path(const char *path)
+{

drivers/gpu/drm/drm_edid_load.c +3 -0

+}
+EXPORT_SYMBOL(__drm_get_edid_firmware_path);
+
 #define GENERIC_EDIDS 6
 static const char * const generic_edid_name[GENERIC_EDIDS] = {
 	"edid/800x600.bin",

drivers/gpu/drm/drm_kms_helper_common.c +12 -0

index 6e35a56a6102..5051c3aa4d5d 100644
@@ -26,6 +26,7 @@
  */
 
 #include <linux/module.h>
+#include <drm/drmP.h>
 
 #include "drm_crtc_helper_internal.h"
 

@@ -33,6 +34,33 @@ MODULE_AUTHOR("David Airlie, Jesse Barnes");
 MODULE_DESCRIPTION("DRM KMS helper");
 MODULE_LICENSE("GPL and additional rights");
 
+#if IS_ENABLED(CONFIG_DRM_LOAD_EDID_FIRMWARE)
+
+/* Backward compatibility for drm_kms_helper.edid_firmware */
+static int edid_firmware_set(const char *val, const struct kernel_param *kp)
+{
+	DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, please use drm.edid_firmware intead.\n");
+
+	return __drm_set_edid_firmware_path(val);
+}
+
+static int edid_firmware_get(char *buffer, const struct kernel_param *kp)

drivers/gpu/drm/drm_kms_helper_common.c +6 -0

+__MODULE_PARM_TYPE(edid_firmware, "charp");
+MODULE_PARM_DESC(edid_firmware,
+		 "DEPRECATED. Use drm.edid_firmware module parameter instead.");
+
+#endif
+
 static int __init drm_kms_helper_init(void)
 {
 	int ret;

include/drm/drm_edid.h +2 -0

index 1e1908a6b1d6..6f35909b8add 100644
@@ -341,6 +341,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 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 struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)

Signature · 1 line

2.11.0

Quoted 22 lines · Jani Nikula

Add drm_kms_helper.edid_firmware module parameter with param ops hooks to set drm.edid_firmware instead, for backwards compatibility.

Suggested-by Ville Syrjälä <[email protected]>

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/drm_edid_load.c         | 16 ++++++++++++++++
 drivers/gpu/drm/drm_kms_helper_common.c | 28 ++++++++++++++++++++++++++++
 include/drm/drm_edid.h                  |  2 ++
 3 files changed, 46 insertions(+)

drivers/gpu/drm/drm_edid_load.c +0 -0

index 1c0495acf341..a4915099aaa9 100644
@@ -31,6 +31,22 @@ module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
 	"from built-in data or /lib/firmware instead. ");
 

drivers/gpu/drm/drm_edid_load.c module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644); · line 46

Quoted 13 lines

drivers/gpu/drm/drm_edid_load.c +13 -0

+/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
+int __drm_set_edid_firmware_path(const char *path)
+{
+	scnprintf(edid_firmware, sizeof(edid_firmware), "%s", path);
+
+	return 0;
+}
+EXPORT_SYMBOL(__drm_set_edid_firmware_path);
+
+/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
+int __drm_get_edid_firmware_path(char *buf, size_t bufsize)
+{
+	return scnprintf(buf, bufsize, "%s", edid_firmware);

I guess these could just use strlcpy() or something.

Quoted 39 lines

drivers/gpu/drm/drm_edid_load.c +3 -0

+}
+EXPORT_SYMBOL(__drm_get_edid_firmware_path);
+
 #define GENERIC_EDIDS 6
 static const char * const generic_edid_name[GENERIC_EDIDS] = {
 	"edid/800x600.bin",

drivers/gpu/drm/drm_kms_helper_common.c +17 -0

index 6e35a56a6102..5051c3aa4d5d 100644
@@ -26,6 +26,7 @@
  */
 
 #include <linux/module.h>
+#include <drm/drmP.h>
 
 #include "drm_crtc_helper_internal.h"
 

@@ -33,6 +34,33 @@ MODULE_AUTHOR("David Airlie, Jesse Barnes");
 MODULE_DESCRIPTION("DRM KMS helper");
 MODULE_LICENSE("GPL and additional rights");
 
+#if IS_ENABLED(CONFIG_DRM_LOAD_EDID_FIRMWARE)
+
+/* Backward compatibility for drm_kms_helper.edid_firmware */
+static int edid_firmware_set(const char *val, const struct kernel_param *kp)
+{
+	DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, please use drm.edid_firmware intead.\n");
+
+	return __drm_set_edid_firmware_path(val);
+}
+
+static int edid_firmware_get(char *buffer, const struct kernel_param *kp)
+{
+	return __drm_get_edid_firmware_path(buffer, PAGE_SIZE);
+}
+
+const struct kernel_param_ops edid_firmware_ops = {

drivers/gpu/drm/drm_kms_helper_common.c MODULE_AUTHOR("David Airlie, Jesse Barnes"); · line 57

Quoted 5 lines

drivers/gpu/drm/drm_kms_helper_common.c +5 -0

+	.set = edid_firmware_set,
+	.get = edid_firmware_get,
+};
+
+module_param_cb(edid_firmware, &edid_firmware_ops, NULL, 0644);

Do we want the __parm_check thing here? Looks like it's just some kind of compile time type check though so not critical by any means.

Otherwise looks technically correct so

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 25 lines

drivers/gpu/drm/drm_kms_helper_common.c +6 -0

+__MODULE_PARM_TYPE(edid_firmware, "charp");
+MODULE_PARM_DESC(edid_firmware,
+		 "DEPRECATED. Use drm.edid_firmware module parameter instead.");
+
+#endif
+
 static int __init drm_kms_helper_init(void)
 {
 	int ret;

include/drm/drm_edid.h +2 -0

index 1e1908a6b1d6..6f35909b8add 100644
@@ -341,6 +341,8 @@ int drm_av_sync_delay(struct drm_connector *connector,
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 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 struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)

Signature · 1 line

2.11.0

Patch 1 is v3 of [1]. There are no functional changes to the previous version, just a rebase and a slight refresh of the commit message and comments. I think the conclusion from the discussion was that this should work just fine. At least one user reported this helped with their audio woes with firmware EDID.

Patch 2 is an attempt to mitigate the problem of moving the edid_firmware module parameter from drm_kms_helper to drm. Not sure if it's too much or too little or just right. Need input here. Pedantically it should be part of patch 1, but this division should be easier to grasp in review.

BR,
Jani.

[1] http://patchwork.freedesktop.org/patch/msgid/[email protected]

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Patch 1 is v3 of [1]. There are no functional changes to the previous version, just a rebase and a slight refresh of the commit message and comments. I think the conclusion from the discussion was that this should work just fine. At least one user reported this helped with their audio woes with firmware EDID.

Patch 2 is an attempt to mitigate the problem of moving the edid_firmware module parameter from drm_kms_helper to drm. Not sure if it's too much or too little or just right. Need input here. Pedantically it should be part of patch 1, but this division should be easier to grasp in review.

BR,
Jani.

[1] http://patchwork.freedesktop.org/patch/msgid/[email protected]

Cc Abdiel Janulgue <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjälä <[email protected]>

Jani Nikula (2):
drm: handle override and firmware EDID at drm_do_get_edid() level
drm: add backwards compatibility support for
drm_kms_helper.edid_firmware

 Documentation/admin-guide/kernel-parameters.txt |  2 +-
 drivers/gpu/drm/Kconfig                         |  2 +-
 drivers/gpu/drm/Makefile                        |  2 +-
 drivers/gpu/drm/drm_edid.c                      | 15 +++++++++++++++
 drivers/gpu/drm/drm_edid_load.c                 |  7 +++++++
 drivers/gpu/drm/drm_kms_helper_common.c         | 19 +++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c              | 19 +------------------
 include/drm/drm_edid.h                          |  2 ++
 8 files changed, 47 insertions(+), 21 deletions(-)