Re: Enhancing EDID quirk functionality

[Ian Pilcher <arequipeno@gmail.com>]

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

I just attached this patch to
https://bugzilla.redhat.com/show_bug.cgi?id=806091, along with the
following comments:

This is the "first draft" of an EDID quirk-based approach to solving
this problem.  I intend to perform a bit of cleanup and break it up
into a series of separate patches, but I'm posting it here for any
comments about the approach.

The patch does the following:

* Changes the vendor field of struct edid_quirk to an array, rather
  than a pointer.  (This has already been sent to the dri-devel list.)
* Adds two new quirks EDID_QUIRK_DISABLE_INFOFRAMES and
  EDID_QUIRK_NO_AUDIO. This first quirk causes drm_detect_hdmi_monitor
  to return false; the second   causes drm_detect_monitor_audio to
  return false.
* Logs the EDID vendor and model of connected monitors.
* Adds an edid_quirks parameter to the drm module, for user-defined
  quirks.

With this patch, my display works when I add
drm.edid_quirks=GSM:0x563f:0x180 to my kernel command line.  (0x80,
EDID_QUIRK_DISABLE_INFOFRAMES, makes it work with the nouveau driver;
0x100, EDID_QUIRK_NO_AUDIO, makes it work with the i915 driver.  I.e.,
the i915 driver is sending audio InfoFrames even when
drm_detect_hdmi_monitor returns false; bug?)

Thoughts?

-- 
========================================================================
Ian Pilcher                                         arequipeno@gmail.com
"If you're going to shift my paradigm ... at least buy me dinner first."
========================================================================

[-- Attachment #2: edid_quirks_all.patch --]
[-- Type: text/x-patch, Size: 9610 bytes --]

diff -ur linux-3.3/drivers/gpu/drm/drm_drv.c linux-3.3-working/drivers/gpu/drm/drm_drv.c
--- linux-3.3/drivers/gpu/drm/drm_drv.c	2012-03-18 18:15:34.000000000 -0500
+++ linux-3.3-working/drivers/gpu/drm/drm_drv.c	2012-04-27 21:30:07.535452184 -0500
@@ -280,6 +280,8 @@
 		ret = -1;
 		goto err_p3;
 	}
+	
+	drm_edid_xtra_quirks_parse();
 
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
@@ -304,6 +306,8 @@
 
 	idr_remove_all(&drm_minors_idr);
 	idr_destroy(&drm_minors_idr);
+	
+	drm_edid_xtra_quirks_free();
 }
 
 module_init(drm_core_init);
diff -ur linux-3.3/drivers/gpu/drm/drm_edid.c linux-3.3-working/drivers/gpu/drm/drm_edid.c
--- linux-3.3/drivers/gpu/drm/drm_edid.c	2012-03-18 18:15:34.000000000 -0500
+++ linux-3.3-working/drivers/gpu/drm/drm_edid.c	2012-04-28 01:11:51.316839743 -0500
@@ -66,6 +66,10 @@
 #define EDID_QUIRK_FIRST_DETAILED_PREFERRED	(1 << 5)
 /* use +hsync +vsync for detailed mode */
 #define EDID_QUIRK_DETAILED_SYNC_PP		(1 << 6)
+/* Display is confused by InfoFrames; don't send any. */
+#define EDID_QUIRK_DISABLE_INFOFRAMES		(1 << 7)
+/* Display doesn't have any audio output */
+#define EDID_QUIRK_NO_AUDIO			(1 << 8)
 
 struct detailed_mode_closure {
 	struct drm_connector *connector;
@@ -81,7 +85,7 @@
 #define LEVEL_CVT	3
 
 static struct edid_quirk {
-	char *vendor;
+	char vendor[4];
 	int product_id;
 	u32 quirks;
 } edid_quirk_list[] = {
@@ -122,13 +126,100 @@
 	{ "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 },
 };
 
+/* Parsed extra quirks from the edid_quirks module parameter */
+static struct edid_quirk *edid_xtra_quirk_list;
+static int edid_num_xtra_quirks;
+
+/*
+ * Free the extra EDID quirks list.
+ */
+void drm_edid_xtra_quirks_free(void)
+{
+	kfree(edid_xtra_quirk_list);
+	edid_xtra_quirk_list = NULL;
+	edid_num_xtra_quirks = 0;
+}
+
+/*
+ * Parse the EDID quirk at s and store it a q. Returns 1 on success, 0 on error.
+ */
+static int drm_edid_parse_quirk(char *s, struct edid_quirk *q)
+{
+	char *c;
+
+	if (sscanf(s, "%3s:%i:%i",
+		   q->vendor, &q->product_id, &q->quirks) == 3) {
+		DRM_DEBUG("Parsed EDID quirk: "
+			  "mfr: %s, product: %d, quirks: %u\n",
+			  q->vendor, q->product_id, q->quirks);
+		return 1;
+	} else {
+		if ((c = strchr(s, ',')) != NULL) {
+			*c = 0;
+		}
+		printk(KERN_WARNING "Invalid EDID quirk: '%s'\n", s);
+		if (c != NULL) {
+			*c = ',';
+		}
+		return 0;
+	}
+}
+
+/*
+ * Parse drm_edid_xtra_quirks and create the extra EDID quirks list.
+ */
+void drm_edid_xtra_quirks_parse(void)
+{
+	int num_quirks;
+	char *c;
+	if (drm_edid_xtra_quirks == NULL) {
+		edid_xtra_quirk_list = NULL;
+		edid_num_xtra_quirks = 0;
+		return;
+	}
+	
+	/* Number of (potential) quirks = number of commas in param + 1 */
+	num_quirks = 1;
+	c = drm_edid_xtra_quirks;
+	while ((c = strchr(c, ',')) != NULL) {
+		++num_quirks;
+		++c;
+	}
+
+	if (num_quirks > DRM_EDID_XTRA_QUIRKS_MAX) {
+		printk(KERN_WARNING "Number of additional EDID quirks limited "
+		       "to " __stringify(DRM_EDID_XTRA_QUIRKS_MAX) "\n");
+		num_quirks = DRM_EDID_XTRA_QUIRKS_MAX;
+	}
+	
+	edid_xtra_quirk_list = kmalloc(sizeof(struct edid_quirk) * num_quirks,
+				       GFP_KERNEL);
+	if (edid_xtra_quirk_list == NULL) {
+		printk(KERN_WARNING "Failed to allocate memory for additional "
+		       "EDID quirks\n");
+		return;
+	}
+	
+	edid_num_xtra_quirks = 0;
+	c = drm_edid_xtra_quirks;
+	while (1) {
+		edid_num_xtra_quirks += drm_edid_parse_quirk(c,
+				&edid_xtra_quirk_list[edid_num_xtra_quirks]);
+		if (edid_num_xtra_quirks == num_quirks ||
+						(c = strchr(c, ',')) == NULL) {
+			break;
+		}
+		++c;
+	}
+}			
+			
 /*** DDC fetch and block validation ***/
 
 static const u8 edid_header[] = {
 	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
 };
 
- /*
+/*
  * Sanity check the header of the base EDID block.  Return 8 if the header
  * is perfect, down to 0 if it's totally wrong.
  */
@@ -366,6 +457,24 @@
 }
 
 /**
+ * edid_vendor_string - decodes EDID vendor field
+ * @edid: EDID from which to extract the vendor field
+ * @edid_vendor: buffer in which to store the vendor string
+ * 
+ * Returns a pointer to @edid_vendor
+ */
+static char *edid_vendor_string(const struct edid *edid, char edid_vendor[4])
+{
+	edid_vendor[0] = ((edid->mfg_id[0] & 0x7c) >> 2) + '@';
+	edid_vendor[1] = (((edid->mfg_id[0] & 0x3) << 3) |
+			  ((edid->mfg_id[1] & 0xe0) >> 5)) + '@';
+	edid_vendor[2] = (edid->mfg_id[1] & 0x1f) + '@';
+	edid_vendor[3] = 0;
+	
+	return edid_vendor;
+}
+
+/**
  * drm_get_edid - get EDID data, if available
  * @connector: connector we're probing
  * @adapter: i2c adapter to use for DDC
@@ -378,15 +487,31 @@
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
+	const struct edid *old_edid;
+	char edid_vendor[4];
 	struct edid *edid = NULL;
-
+	
 	if (drm_probe_ddc(adapter))
 		edid = (struct edid *)drm_do_get_edid(connector, adapter);
+	
+	/* Log if something has changed */
+	if (edid != NULL) {
+		old_edid = (struct edid *)connector->display_info.raw_edid;
+		/* memcmp call compares the mfg_id, prod_code, and serial
+		 * fields of the two (packed) EDID structures for equality */
+		if (!old_edid || memcmp(&edid->mfg_id, &old_edid->mfg_id, 8)) {
+			DRM_INFO("Detected display on connector %s: "
+				 "mfr: %s, product: %04hx, serial: %d\n",
+				 drm_get_connector_name(connector),
+				 edid_vendor_string(edid, edid_vendor),
+				 le16_to_cpu(EDID_PRODUCT_ID(edid)),
+				 le32_to_cpu(edid->serial));
+		}
+	}
 
 	connector->display_info.raw_edid = (char *)edid;
 
 	return edid;
-
 }
 EXPORT_SYMBOL(drm_get_edid);
 
@@ -401,13 +526,9 @@
  */
 static bool edid_vendor(struct edid *edid, char *vendor)
 {
-	char edid_vendor[3];
-
-	edid_vendor[0] = ((edid->mfg_id[0] & 0x7c) >> 2) + '@';
-	edid_vendor[1] = (((edid->mfg_id[0] & 0x3) << 3) |
-			  ((edid->mfg_id[1] & 0xe0) >> 5)) + '@';
-	edid_vendor[2] = (edid->mfg_id[1] & 0x1f) + '@';
+	char edid_vendor[4];
 
+	edid_vendor_string(edid, edid_vendor);
 	return !strncmp(edid_vendor, vendor, 3);
 }
 
@@ -420,17 +541,34 @@
 static u32 edid_get_quirks(struct edid *edid)
 {
 	struct edid_quirk *quirk;
+	u32 quirks;
 	int i;
+	
+	quirks = 0;
 
+	/* Check the built-in quirks first */
 	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
 		quirk = &edid_quirk_list[i];
 
 		if (edid_vendor(edid, quirk->vendor) &&
-		    (EDID_PRODUCT_ID(edid) == quirk->product_id))
-			return quirk->quirks;
+		    (EDID_PRODUCT_ID(edid) == quirk->product_id)) {
+			quirks = quirk->quirks;
+			break;
+		}
+	}
+	
+	/* Extra quirks can override built-in ones */
+	for (i = 0; i < edid_num_xtra_quirks; ++i) {
+		quirk = &edid_xtra_quirk_list[i];
+		
+		if (edid_vendor(edid, quirk->vendor) &&
+		    EDID_PRODUCT_ID(edid) == quirk->product_id) {
+			quirks = quirk->quirks;
+			break;
+		}
 	}
 
-	return 0;
+	return quirks;
 }
 
 #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
@@ -1562,6 +1700,11 @@
 	int i, hdmi_id;
 	int start_offset, end_offset;
 	bool is_hdmi = false;
+	
+	if (edid_get_quirks(edid) & EDID_QUIRK_DISABLE_INFOFRAMES) {
+		DRM_DEBUG_KMS("Disabling HDMI InfoFrames due to EDID quirk\n");
+		goto end;
+	}
 
 	edid_ext = drm_find_cea_extension(edid);
 	if (!edid_ext)
@@ -1610,6 +1753,11 @@
 	int i, j;
 	bool has_audio = false;
 	int start_offset, end_offset;
+	
+	if (edid_get_quirks(edid) & EDID_QUIRK_NO_AUDIO) {
+		DRM_DEBUG_KMS("Disabling HDMI audio due to EDID quirk\n");
+		goto end;
+	}
 
 	edid_ext = drm_find_cea_extension(edid);
 	if (!edid_ext)
diff -ur linux-3.3/drivers/gpu/drm/drm_stub.c linux-3.3-working/drivers/gpu/drm/drm_stub.c
--- linux-3.3/drivers/gpu/drm/drm_stub.c	2012-03-18 18:15:34.000000000 -0500
+++ linux-3.3-working/drivers/gpu/drm/drm_stub.c	2012-04-24 13:32:35.394132568 -0500
@@ -46,16 +46,22 @@
 unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 EXPORT_SYMBOL(drm_timestamp_precision);
 
+char *drm_edid_xtra_quirks = NULL;
+EXPORT_SYMBOL(drm_edid_xtra_quirks);
+
 MODULE_AUTHOR(CORE_AUTHOR);
 MODULE_DESCRIPTION(CORE_DESC);
 MODULE_LICENSE("GPL and additional rights");
 MODULE_PARM_DESC(debug, "Enable debug output");
 MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs]");
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
+MODULE_PARM_DESC(edid_quirks, "Additional EDID quirks [up to "
+		 __stringify(DRM_EDID_XTRA_QUIRKS_MAX) "]");
 
 module_param_named(debug, drm_debug, int, 0600);
 module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
 module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
+module_param_named(edid_quirks, drm_edid_xtra_quirks, charp, 0600);
 
 struct idr drm_minors_idr;
 
diff -ur linux-3.3/include/drm/drmP.h linux-3.3-working/include/drm/drmP.h
--- linux-3.3/include/drm/drmP.h	2012-03-18 18:15:34.000000000 -0500
+++ linux-3.3-working/include/drm/drmP.h	2012-04-27 16:09:12.043494619 -0500
@@ -1468,6 +1468,7 @@
 
 extern unsigned int drm_vblank_offdelay;
 extern unsigned int drm_timestamp_precision;
+extern char *drm_edid_xtra_quirks;
 
 extern struct class *drm_class;
 extern struct proc_dir_entry *drm_proc_root;
@@ -1552,6 +1553,12 @@
 void drm_gem_vm_close(struct vm_area_struct *vma);
 int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
 
+				/* EDID support (drm_edid.c) */
+/* This gets stringified in drm_stub.c and drm_edid.c; don't add parentheses */
+#define DRM_EDID_XTRA_QUIRKS_MAX	10
+void drm_edid_xtra_quirks_parse(void);
+void drm_edid_xtra_quirks_free(void);
+
 #include "drm_global.h"
 
 static inline void

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel