-next trees

-next trees

[Dave Airlie]

Just a reminder to the subtree maintainers that I should be starting
to see signs of -next trees lining up soon,

I've merged a bunch of patches from the list, but I'm sure I've forgotten some,
I've got the 3 quirks from Paul that I'm iffy about due to the lack of
testing against VGA ports that ajax requested, but I could be turned.

Ajax, I think Ian still has an open request on how to proceed with the
infoframes quirk nightmare.

Alex, Christian, Jerome: would be good to resolve that fence issue for
-fixes asap.

Dave.

Re: -next trees

[Alex Deucher]

On Wed, Sep 12, 2012 at 9:44 PM, Dave Airlie <airlied@gmail.com> wrote:
> Just a reminder to the subtree maintainers that I should be starting
> to see signs of -next trees lining up soon,
>
> I've merged a bunch of patches from the list, but I'm sure I've forgotten some,
> I've got the 3 quirks from Paul that I'm iffy about due to the lack of
> testing against VGA ports that ajax requested, but I could be turned.
>
> Ajax, I think Ian still has an open request on how to proceed with the
> infoframes quirk nightmare.
>
> Alex, Christian, Jerome: would be good to resolve that fence issue for
> -fixes asap.

-fixes request sent.  I've also got a wip -next tree here:
http://cgit.freedesktop.org/~agd5f/linux/?h=drm-next-3.7-wip
If anyone has any additional outstanding radeon patches they want in
3.7, let me know.  Christian, you might want to double check the last
set of VM patches, I had to make a minor adjustment to the last one
due to skipping 7/8.

Alex


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

Re: -next trees

[Rob Clark]

On Wed, Sep 12, 2012 at 8:44 PM, Dave Airlie <airlied@gmail.com> wrote:
> Just a reminder to the subtree maintainers that I should be starting
> to see signs of -next trees lining up soon,
>
> I've merged a bunch of patches from the list, but I'm sure I've forgotten some,
> I've got the 3 quirks from Paul that I'm iffy about due to the lack of
> testing against VGA ports that ajax requested, but I could be turned.

What about:

  drm: support for rotated scanout
  drm: refcnt drm_framebuffer
  drm: change ioctl permissions

BR,
-R

> Ajax, I think Ian still has an open request on how to proceed with the
> infoframes quirk nightmare.
>
> Alex, Christian, Jerome: would be good to resolve that fence issue for
> -fixes asap.
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: -next trees

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

Am Donnerstag, den 13.09.2012, 11:44 +1000 schrieb Dave Airlie:
> Just a reminder to the subtree maintainers that I should be starting
> to see signs of -next trees lining up soon,
> 
> I've merged a bunch of patches from the list, but I'm sure I've forgotten some,
> I've got the 3 quirks from Paul that I'm iffy about due to the lack of
> testing against VGA ports that ajax requested, but I could be turned.

I am really sorry, but it looks like more testing is needed, so please
revert them. :/

Testing on a T60 shows that it works out of the box. So it is either
915GM or cable problem. [1]

I will answer to the single patches later on.

[…]


Thanks,

Paul


[1] https://bugs.freedesktop.org/show_bug.cgi?id=53544#c5

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: -next trees

[Ian Pilcher]

On 09/12/2012 08:44 PM, Dave Airlie wrote:
> Ajax, I think Ian still has an open request on how to proceed with the
> infoframes quirk nightmare.

Indeed.

On 08/30/2012 12:23 AM, Ian Pilcher wrote:
> In terms of moving forward with the rest of the EDID quirk stuff, are
> you OK with either of these options:
>
> * Remove EDID_QUIRK_NO_AUDIO from the flags for the LG L246WP.  It won't
>   work "out of the box" with the Intel driver until that driver is
>   fixed to not send audio InfoFrames to non-HDMI displays (but anyone
>   who has the combination will be able to add their own quirk to make
>   it work).
>
> * Leave both flags, because both are accurate.  The display is confused
>   by any InfoFrames (audio or AVI), and it is reporting non-existent
>   audio capabilities.  The fact that the combination of the two flags
>   makes the display work with Intel GPUs is a happy/sad/neutral
>   accident, and the driver's behavior is still considered broken.
>
>   (Essentially, this would mean just editing the comments to reflect the
>   above reasoning.)

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

Re: -next trees

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1271 bytes --]

On Sat, 2012-09-22 at 23:34 -0500, Ian Pilcher wrote:
> On 09/12/2012 08:44 PM, Dave Airlie wrote:
> > Ajax, I think Ian still has an open request on how to proceed with the
> > infoframes quirk nightmare.
> 
> Indeed.

I'm sorry, I thought I was clearer.  Daniel posted a patch to fix the
Intel driver for this:

http://lists.freedesktop.org/archives/intel-gfx/2012-August/020046.html

Which I didn't entirely ack, but which is essentially right.  That's
what we should do; and having done so, if I understand things correctly,
there's no need for any quirks here at all.

Is there something I'm missing?

But also:

> > * Leave both flags, because both are accurate.  The display is confused
> >   by any InfoFrames (audio or AVI), and it is reporting non-existent
> >   audio capabilities.  The fact that the combination of the two flags
> >   makes the display work with Intel GPUs is a happy/sad/neutral
> >   accident, and the driver's behavior is still considered broken.

I haven't actually seen the EDID block for this display, I don't
believe, so I'm not sure whether the "non-existent" part of this is even
accurate, or whether we're just parsing things incorrectly.  There's a
reason I keep a standalone parser around.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: -next trees

[Ian Pilcher]

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

On 09/24/2012 09:34 AM, Adam Jackson wrote:
> I'm sorry, I thought I was clearer.  Daniel posted a patch to fix the
> Intel driver for this:
>
> http://lists.freedesktop.org/archives/intel-gfx/2012-August/020046.html
>
> Which I didn't entirely ack, but which is essentially right.  That's
> what we should do; and having done so, if I understand things correctly,
> there's no need for any quirks here at all.
>
> Is there something I'm missing?


Yes.  The DISABLE_INFOFRAMES quirk will still be required by this
display.

(The reason that the Intel driver got dragged into this is because it is
sending audio InfoFrames to the display, even when
drm_detect_hdmi_monitor returns false.)

> I haven't actually seen the EDID block for this display, I don't
> believe, so I'm not sure whether the "non-existent" part of this is even
> accurate, or whether we're just parsing things incorrectly.  There's a
> reason I keep a standalone parser around.

EDID attached.

In the meantime, I would like to move the ball forward on this issue.
As I see it, there 3 issues that have become conflated:

1.  The display (LG L246WP) is confused by *any* InfoFrames.

2.  The Intel driver is sending audio InfoFrames when
     drm_detect_hdmi_monitor returns false and drm_detect_monitor_audio
     returns true.

3.  drm_detect_monitor_audio is returning true for the LG L246WP, which
     definitely doesn't have any audio capabilities.  This may be a bug
     in the display's EDID, or it may be a parsing error.

Would you be amenable to a patch series that addresses *only* issue
number 1?

Thanks!

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

[-- Attachment #2: edid.bin --]
[-- Type: application/octet-stream, Size: 256 bytes --]

[-- 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

Re: -next trees

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1401 bytes --]

On Tue, 2012-09-25 at 13:47 -0500, Ian Pilcher wrote:

> In the meantime, I would like to move the ball forward on this issue.
> As I see it, there 3 issues that have become conflated:
> 
> 1.  The display (LG L246WP) is confused by *any* InfoFrames.

If this is still the case after applying:

commit adf00b26d18e1b3570451296e03bcb20e4798cdd
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Tue Sep 25 13:23:34 2012 -0300

    drm/i915: make sure we write all the DIP data bytes

Then I suspect I'm compelled to agree that we need a quirk to forcibly
disable InfoFrames entirely.  I don't like to be difficult about this,
but the HDMI spec is quite clear that sinks _must_ accept InfoFrames
defined in either the CEA or HDMI specs, so if we're seeing that class
of failure I tend to strongly suspect our drivers first.

> 3.  drm_detect_monitor_audio is returning true for the LG L246WP, which
>      definitely doesn't have any audio capabilities.  This may be a bug
>      in the display's EDID, or it may be a parsing error.

The display is definitely a filthy liar then:

  Audio data block
    Linear PCM, max channels 1
    Supported sample rates (kHz): 48 44.1 32
    Supported sample sizes (bits): 24 20 16

Hooray for hardware.  Not sure what the logic should be for whether to
send HDMI audio or not, I'll re-read the appropriate scrolls.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

Re: -next trees

[Ian Pilcher]

On 09/26/2012 12:00 PM, Adam Jackson wrote:
> On Tue, 2012-09-25 at 13:47 -0500, Ian Pilcher wrote:
>>
>> 1.  The display (LG L246WP) is confused by *any* InfoFrames.
>
> If this is still the case after applying:
>
> commit adf00b26d18e1b3570451296e03bcb20e4798cdd
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Tue Sep 25 13:23:34 2012 -0300
>
>      drm/i915: make sure we write all the DIP data bytes
>
> Then I suspect I'm compelled to agree that we need a quirk to forcibly
> disable InfoFrames entirely.  I don't like to be difficult about this,
> but the HDMI spec is quite clear that sinks _must_ accept InfoFrames
> defined in either the CEA or HDMI specs, so if we're seeing that class
> of failure I tend to strongly suspect our drivers first.

I just tested with that patch applied, and the problem still exists, so
consider yourself compelled.

Unless you object, I will go ahead and create a patch series that does
only:

- user-defined EDID quirks
- DISABLE_INFOFRAMES quirk
- DISABLE_INFOFRAMES quirk for LG L246WP

(I will specifically *not* address the audio issue in this series.)

>> 3.  drm_detect_monitor_audio is returning true for the LG L246WP, which
>>       definitely doesn't have any audio capabilities.  This may be a bug
>>       in the display's EDID, or it may be a parsing error.
>
> The display is definitely a filthy liar then:
>
>    Audio data block
>      Linear PCM, max channels 1
>      Supported sample rates (kHz): 48 44.1 32
>      Supported sample sizes (bits): 24 20 16
>
> Hooray for hardware.  Not sure what the logic should be for whether to
> send HDMI audio or not, I'll re-read the appropriate scrolls.

Well, if hardware didn't suck we wouldn't need quirks.

If you'd like, I can generate a second patch series that adds some or
all of:

- NO_AUDIO quirk
- drm_monitor_has_hdmi_audio() function
- NO_AUDIO quirk for LG L246WP (kind of redundant if no InfoFrames are
   being sent anyway, but still technically accurate)

Let me know if you'd like any or all of those.

Thanks!

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

Re: -next trees

[Daniel Vetter]

On Wed, Sep 26, 2012 at 04:04:43PM -0500, Ian Pilcher wrote:
> On 09/26/2012 12:00 PM, Adam Jackson wrote:
> >On Tue, 2012-09-25 at 13:47 -0500, Ian Pilcher wrote:
> >>
> >>1.  The display (LG L246WP) is confused by *any* InfoFrames.
> >
> >If this is still the case after applying:
> >
> >commit adf00b26d18e1b3570451296e03bcb20e4798cdd
> >Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >Date:   Tue Sep 25 13:23:34 2012 -0300
> >
> >     drm/i915: make sure we write all the DIP data bytes
> >
> >Then I suspect I'm compelled to agree that we need a quirk to forcibly
> >disable InfoFrames entirely.  I don't like to be difficult about this,
> >but the HDMI spec is quite clear that sinks _must_ accept InfoFrames
> >defined in either the CEA or HDMI specs, so if we're seeing that class
> >of failure I tend to strongly suspect our drivers first.
> 
> I just tested with that patch applied, and the problem still exists, so
> consider yourself compelled.
> 
> Unless you object, I will go ahead and create a patch series that does
> only:
> 
> - user-defined EDID quirks
> - DISABLE_INFOFRAMES quirk
> - DISABLE_INFOFRAMES quirk for LG L246WP
> 
> (I will specifically *not* address the audio issue in this series.)
> 
> >>3.  drm_detect_monitor_audio is returning true for the LG L246WP, which
> >>      definitely doesn't have any audio capabilities.  This may be a bug
> >>      in the display's EDID, or it may be a parsing error.
> >
> >The display is definitely a filthy liar then:
> >
> >   Audio data block
> >     Linear PCM, max channels 1
> >     Supported sample rates (kHz): 48 44.1 32
> >     Supported sample sizes (bits): 24 20 16
> >
> >Hooray for hardware.  Not sure what the logic should be for whether to
> >send HDMI audio or not, I'll re-read the appropriate scrolls.
> 
> Well, if hardware didn't suck we wouldn't need quirks.
> 
> If you'd like, I can generate a second patch series that adds some or
> all of:
> 
> - NO_AUDIO quirk
> - drm_monitor_has_hdmi_audio() function

I've volunteered Antti from our team to implement this (and to patch up
drm/i915), so this should happen already and with the above no infoframes
quirks we shouldn't need any additional quirks.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v5 0/3] Enhanced EDID quirk functionality

[Ian Pilcher]

As promised, this patch series does only the following:

* Add user-defined EDID quirks -- via sysfs or a module parameter
* Add an EDID quirk flag to disable all HDMI functionality
  (InfoFrames)
* Add a quirk to disable HDMI InfoFrames for the LG L246WP

[PATCH v5 1/3] drm: Add user-defined EDID quirks capability

[Ian Pilcher]

Add the ability for users to define their own EDID quirks via a
module parameter or sysfs attribute.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 Documentation/EDID/edid_quirks.txt | 126 ++++++++++
 drivers/gpu/drm/drm_drv.c          |   2 +
 drivers/gpu/drm/drm_edid.c         | 500 ++++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/drm_stub.c         |   6 +
 drivers/gpu/drm/drm_sysfs.c        |  19 ++
 include/drm/drmP.h                 |  10 +
 include/drm/drm_edid.h             |  13 +-
 7 files changed, 615 insertions(+), 61 deletions(-)
 create mode 100644 Documentation/EDID/edid_quirks.txt

diff --git a/Documentation/EDID/edid_quirks.txt b/Documentation/EDID/edid_quirks.txt
new file mode 100644
index 0000000..0c9c746
--- /dev/null
+++ b/Documentation/EDID/edid_quirks.txt
@@ -0,0 +1,126 @@
+                                  EDID Quirks
+                                 =============
+                       Ian Pilcher <arequipeno@gmail.com>
+                                August 11, 2012
+
+
+    "EDID blocks out in the wild have a variety of bugs"
+        -- from drivers/gpu/drm/drm_edid.c
+
+
+Overview
+========
+
+EDID quirks provide a mechanism for working around display hardware with buggy
+EDID data.
+
+An individual EDID quirk maps a display type (identified by its EDID
+manufacturer ID and product code[1]) to a set of "quirk flags."  The kernel
+includes a variety of built-in quirks.  (They are stored in the edid_quirk_list
+array in drivers/gpu/drm/drm_edid.c.)
+
+An example of a built-in EDID quirk is:
+
+    ACR:0xad46:0x00000001
+
+The first field is the manufacturer ID (Acer, Inc.), the second field is the
+manufacturer's product code, and the third field contains the quirk flags for
+that display type.
+
+The quirk flags are defined in drivers/gpu/drm/drm_edid.c.  Each flag has a
+symbolic name beginning with EDID_QUIRK_, along with a numerical value.  Each
+flag should also have an associated comment which provides an idea of its
+effect.  Note that the values in the source file are expressed as bit shifts[2]:
+
+    * 1 << 0: 0x0001
+    * 1 << 1: 0x0002
+    * 1 << 2: 0x0004
+    * etc.
+
+
+sysfs interface
+===============
+
+The current EDID quirk list can be read from /sys/class/drm/edid_quirks:
+
+    # cat /sys/class/drm/edid_quirks
+       ACR:0xad46:0x00000001
+       API:0x7602:0x00000001
+       ACR:0x0977:0x00000020
+    0x9e6a:0x077e:0x00000080
+    ...
+
+("Nonconformant" manufacturer IDs are displayed as hexadecimal values.)
+
+The number of total "slots" in the list can be read from
+/sys/class/drm/edid_quirks_size.  This total includes both occupied slots (i.e.
+the current list) and any slots available for additional quirks.  The number of
+available slots can be calculated by subtracting the number of quirks in the
+current list from the total number of slots.
+
+If a slot is available, an additional quirk can be added to the list by writing
+it to /sys/class/drm/edid_quirks:
+
+    # echo FOO:0xffff:0x100 > /sys/class/drm/edid_quirks
+
+Manufacturer IDs can also be specified numerically.  (This is the only way to
+specify a nonconformant ID.) This command is equivalent to the previous one:
+
+    # echo 0x19ef:0xffff:0x100 > /sys/class/drm/edid_quirks
+
+Numeric values can also be specified in decimal or octal formats; a number that
+begins with a 0 is assumed to be octal:
+
+    # echo FOO:65535:0400 > /sys/class/drm/edid_quirks
+
+An existing quirk can be replaced by writing a new set of flags:
+
+    # echo FOO:0xffff:0x200 > /sys/class/drm/edid_quirks
+
+A quirk can be deleted from the list by writing an empty flag set (0). This
+makes the slot occupied by that quirk available.
+
+    # echo FOO:0xffff:0 > /sys/class/drm/edid_quirks
+
+Writing an "at symbol" (@) clears the entire quirk list:
+
+    # echo @ > /sys/class/drm/edid_quirks
+
+Multiple changes to the list can be specified in a comma (or newline) separated
+list. For example, the following command clears all of the existing quirks in
+the list and adds 3 new quirks:
+
+    # echo @,FOO:0xffff:0x100,BAR:0x1111:0x001,BAZ:0x2222:0x002 > \
+            /sys/class/drm/edid_quirks
+
+Note however, that any error (an incorrectly formatted quirk or an attempt to
+add a quirk when no slot is available) will abort processing of any further
+changes, potentially making it difficult to determine exactly which change
+caused the error and what changes were made.  For this reason, making changes
+one at a time is recommended, particularly if the changes are being made by a
+script or program.
+
+
+Module parameter
+================
+
+The EDID quirk list can also be modified via the edid_quirks module parameter
+(drm.edid_quirks on the kernel command line).  The effect of setting this
+parameter is identical to the effect of writing its value to
+/sys/class/drm/edid_quirks, with one important difference.  When an error is
+encountered during module parameter parsing or processing, any remaining quirks
+in the parameter string will still be processed.  (It is hoped that this approach
+maximizes the probability of producing a working display.)
+
+
+Follow-up
+=========
+
+If you encounter a display that requires an additional EDID quirk in order to
+function properly, please report it to the direct rendering development mailing
+list <dri-devel@lists.freedesktop.org>.
+
+
+[1] See http://en.wikipedia.org/wiki/Extended_display_identification_data for a
+    description of the manufacturer ID and product code fields.
+[2] https://en.wikipedia.org/wiki/Bitwise_operation#Bit_shifts
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9238de4..7fe39e0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -276,6 +276,8 @@ static int __init drm_core_init(void)
 		goto err_p3;
 	}
 
+	drm_edid_quirks_param_process();
+
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
 	return 0;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a8743c3..ea535f6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -31,6 +31,8 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/ctype.h>
+
 #include "drmP.h"
 #include "drm_edid.h"
 #include "drm_edid_modes.h"
@@ -82,51 +84,457 @@ struct detailed_mode_closure {
 #define LEVEL_GTF2	2
 #define LEVEL_CVT	3
 
-static struct edid_quirk {
-	char vendor[4];
-	int product_id;
-	u32 quirks;
-} edid_quirk_list[] = {
+union edid_quirk {
+	struct {
+		union edid_display_id display_id;
+		u32 quirks;
+	} __attribute__((packed)) s;
+	u64 u;
+};
+
+#define EDID_MFG_ID(c1, c2, c3)		cpu_to_be16(			\
+						(c1 & 0x1f) << 10 |	\
+						(c2 & 0x1f) << 5 |	\
+						(c3 & 0x1f)		\
+					)
+
+#define EDID_QUIRK_LIST_SIZE	24
+
+union edid_quirk edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
+
 	/* Acer AL1706 */
-	{ "ACR", 44358, EDID_QUIRK_PREFER_LARGE_60 },
+	{ { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(44358) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
 	/* Acer F51 */
-	{ "API", 0x7602, EDID_QUIRK_PREFER_LARGE_60 },
+	{ { { { EDID_MFG_ID('A', 'P', 'I'), cpu_to_le16(0x7602) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
 	/* Unknown Acer */
-	{ "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
+	{ { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(2423) } },
+		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
 
 	/* Belinea 10 15 55 */
-	{ "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
-	{ "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
+	{ { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(1516) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
+	{ { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(0x77e) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
 
 	/* Envision Peripherals, Inc. EN-7100e */
-	{ "EPI", 59264, EDID_QUIRK_135_CLOCK_TOO_HIGH },
+	{ { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(59264) } },
+		EDID_QUIRK_135_CLOCK_TOO_HIGH } },
 	/* Envision EN2028 */
-	{ "EPI", 8232, EDID_QUIRK_PREFER_LARGE_60 },
+	{ { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(8232) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
 
 	/* Funai Electronics PM36B */
-	{ "FCM", 13600, EDID_QUIRK_PREFER_LARGE_75 |
-	  EDID_QUIRK_DETAILED_IN_CM },
+	{ { { { EDID_MFG_ID('F', 'C', 'M'), cpu_to_le16(13600) } },
+		EDID_QUIRK_PREFER_LARGE_75 | EDID_QUIRK_DETAILED_IN_CM } },
 
 	/* LG Philips LCD LP154W01-A5 */
-	{ "LPL", 0, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
-	{ "LPL", 0x2a00, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
+	{ { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0) } },
+		EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
+	{ { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0x2a00) } },
+		EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
 
 	/* Philips 107p5 CRT */
-	{ "PHL", 57364, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
+	{ { { { EDID_MFG_ID('P', 'H', 'L'), cpu_to_le16(57364) } },
+		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
 
 	/* Proview AY765C */
-	{ "PTS", 765, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
+	{ { { { EDID_MFG_ID('P', 'T', 'S'), cpu_to_le16(765) } },
+		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
 
 	/* Samsung SyncMaster 205BW.  Note: irony */
-	{ "SAM", 541, EDID_QUIRK_DETAILED_SYNC_PP },
+	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(541) } },
+		EDID_QUIRK_DETAILED_SYNC_PP } },
 	/* Samsung SyncMaster 22[5-6]BW */
-	{ "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 },
-	{ "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 },
+	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(596) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
+	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(638) } },
+		EDID_QUIRK_PREFER_LARGE_60 } },
 
 	/* ViewSonic VA2026w */
-	{ "VSC", 5020, EDID_QUIRK_FORCE_REDUCED_BLANKING },
+	{ { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
+		EDID_QUIRK_FORCE_REDUCED_BLANKING } },
+
+	/*
+	 * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE to
+	 * provide some room for user-supplied quirks.
+	 */
 };
 
+DEFINE_MUTEX(edid_quirk_list_mutex);
+
+/**
+ * drm_edid_mfg_format - format an "encoded" EDID manufacturer ID for printing
+ * @mfg_id: the encoded manufacturer ID
+ * @buf: destination buffer for the formatted manufacturer ID (minimum 7 bytes)
+ * @strip: if non-zero, the returned pointer will skip any leading spaces
+ *
+ * An EDID manufacturer ID is supposed to consist of 3 capital letters (A-Z).
+ * Each letter is stored as a 5-bit value between 1 and 26, taking up 15 bits of
+ * the 16-bit ID. The remaining bit should always be 0. If display manufacturers
+ * always did things correctly, however, EDID quirks wouldn't be required in
+ * the first place. This function does the following:
+ *
+ * - Broken IDs are printed in hexadecimal (0xffff).
+ * - "Correct" IDs are formatted as a 3-letter ID string, preceded by 3 spaces;
+ *   the spaces ensure that both output formats are the same length.
+ *
+ * Thus, a formatted manufacturer ID is always 6 characters long (not including
+ * the terminating 0).
+ *
+ * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
+ * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
+ * been formatted as a 3-letter string, a pointer to the first non-space
+ * character (@buf + 3) is returned.
+ */
+static const char *drm_edid_mfg_format(__be16 mfg_id, char *buf, int strip)
+{
+	u16 id = be16_to_cpu(mfg_id);
+
+	if (id & 0x8000)
+		goto bad_id;
+
+	buf[3] = ((id & 0x7c00) >> 10) + '@';
+	if (!isupper(buf[3]))
+		goto bad_id;
+
+	buf[4] = ((id & 0x03e0) >> 5) + '@';
+	if (!isupper(buf[4]))
+		goto bad_id;
+
+	buf[5] = (id & 0x001f) + '@';
+	if (!isupper(buf[5]))
+		goto bad_id;
+
+	memset(buf, ' ', 3);
+	buf[6] = 0;
+
+	return strip ? (buf + 3) : buf;
+
+bad_id:
+	sprintf(buf, "0x%04hx", id);
+	return buf;
+}
+
+#define EDID_MFG_BUF_SIZE		7
+
+/**
+ * drm_edid_display_id_format - format an EDID "display ID" (manufacturer ID
+ * 				and product code) for printing
+ * @display_id: the display ID
+ * @buf: destination buffer for the formatted display ID (minimum 14 bytes)
+ * @strip: if non-zero, the returned pointer will skip any leading spaces
+ *
+ * A formatted display ID is always 13 characters long (not including the
+ * terminating 0).
+ *
+ * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
+ * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
+ * been formatted as a 3-letter string, a pointer to the first non-space
+ * character (@buf + 3) is returned.
+ */
+static const char *drm_edid_display_id_format(union edid_display_id display_id,
+					      char *buf, int strip)
+{
+	const char *s;
+
+	s = drm_edid_mfg_format(display_id.s.mfg_id, buf, strip);
+	sprintf(buf + EDID_MFG_BUF_SIZE - 1, ":0x%04hx",
+		le16_to_cpu(display_id.s.prod_code));
+
+	return s;
+}
+
+#define EDID_DISPLAY_ID_BUF_SIZE	(EDID_MFG_BUF_SIZE + 7)
+
+/**
+ * drm_edid_quirk_format - format an EDID quirk for printing
+ * @quirk: the quirk
+ * @buf: destination buffer for the formatted quirk (minimum 25 bytes)
+ * @strip: if non-zero, the returned pointer will skip any leading spaces
+ *
+ * A formatted EDID quirk is always 24 characters long (not including the
+ * terminating 0).
+ *
+ * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
+ * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
+ * been formatted as a 3-letter string, a pointer to the first non-space
+ * character (@buf + 3) is returned.
+ */
+static const char *drm_edid_quirk_format(const union edid_quirk *quirk,
+					 char *buf, int strip)
+{
+	const char *s;
+
+	s = drm_edid_display_id_format(quirk->s.display_id, buf, strip);
+	sprintf(buf + EDID_DISPLAY_ID_BUF_SIZE - 1, ":0x%08x", quirk->s.quirks);
+
+	return s;
+}
+
+#define EDID_QUIRK_BUF_SIZE		(EDID_DISPLAY_ID_BUF_SIZE + 11)
+
+/**
+ * drm_edid_quirk_parse - parse an EDID quirk
+ * @s: string containing the quirk to be parsed
+ * @quirk: destination for parsed quirk
+ *
+ * Returns 0 on success, < 0 (currently -EINVAL) on error.
+ */
+static int drm_edid_quirk_parse(const char *s, union edid_quirk *quirk)
+{
+	char buf[EDID_QUIRK_BUF_SIZE];
+	s32 mfg;
+	s32 product;
+	s64 quirks;
+	char *c;
+
+	if (sscanf(s, "%i:%i:%lli", &mfg, &product, &quirks) == 3) {
+		if (mfg < 0 || mfg > 0xffff)
+			goto error;
+		quirk->s.display_id.s.mfg_id = cpu_to_be16((u16)mfg);
+	} else {
+		if (sscanf(s, "%3s:%i:%lli", buf, &product, &quirks) != 3 ||
+				!isupper(buf[0]) ||
+				!isupper(buf[1]) ||
+				!isupper(buf[2]))
+			goto error;
+		quirk->s.display_id.s.mfg_id =
+				EDID_MFG_ID(buf[0], buf[1], buf[2]);
+	}
+
+	if (product < 0 || product > 0xffff ||
+			quirks < 0 || quirks > 0xffffffffLL)
+		goto error;
+
+	quirk->s.display_id.s.prod_code = cpu_to_le16((u16)product);
+	quirk->s.quirks = (u32)quirks;
+
+	DRM_DEBUG("Successfully parsed EDID quirk: %s\n",
+		  drm_edid_quirk_format(quirk, buf, 1));
+
+	return 0;
+
+error:
+	c = strpbrk(s, ",\n");
+	if (c == NULL) {
+		printk(KERN_WARNING "Invalid EDID quirk: '%s'\n", s);
+	} else {
+		printk(KERN_WARNING "Invalid EDID quirk: '%.*s'\n",
+		      (int)(c - s), s);
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * drm_edid_quirk_find_by_id - find the EDID quirk matching a display ID
+ * @display_id: the display ID to match
+ *
+ * Caller MUST hold edid_quirk_list_mutex.
+ *
+ * Returns a pointer to the matching quirk list entry, NULL if no such entry
+ * exists.
+ */
+static union edid_quirk *drm_edid_quirk_find_by_id(union edid_display_id id)
+{
+	union edid_quirk *q = edid_quirk_list;
+
+	do {
+		if (q->s.display_id.u == id.u && q->s.quirks != 0)
+			return q;
+	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
+
+	return NULL;
+}
+
+/**
+ * drm_edid_quirk_find_slot - find an empty slot in the EDID quirk list
+ *
+ * Caller MUST hold edid_quirk_list_mutex.
+ *
+ * Returns a pointer to the first empty slot, NULL if no empty slots exist.
+ */
+static union edid_quirk *drm_edid_quirk_find_empty(void)
+{
+	union edid_quirk *q = edid_quirk_list;
+
+	do {
+		if (q->s.quirks == 0)
+			return q;
+	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
+
+	return NULL;
+}
+
+/**
+ * drm_edid_quirk_process - process a newly parsed EDID quirk
+ * @quirk: the quirk to be processed
+ *
+ * Depending on the newly parsed quirk and the contents of the quirks list, this
+ * function will add, remove, or replace a quirk.
+ *
+ * Returns 0 on success, < 0 on error (-ENOSPC if there is no free slot for a
+ * new quirk). Note that trying to remove a quirk that isn't present is not
+ * considered an error.
+ */
+static int drm_edid_quirk_process(const union edid_quirk *quirk)
+{
+	char buf[EDID_QUIRK_BUF_SIZE];
+	union edid_quirk *q;
+	int res = 0;
+
+	mutex_lock(&edid_quirk_list_mutex);
+
+	if (quirk->s.quirks == 0) {
+		DRM_INFO("Removing EDID quirk for display %s\n",
+			 drm_edid_display_id_format(quirk->s.display_id,
+						    buf, 1));
+		q = drm_edid_quirk_find_by_id(quirk->s.display_id);
+		if (q == NULL) {
+			printk(KERN_WARNING "No quirk found for display %s\n",
+			       drm_edid_display_id_format(quirk->s.display_id,
+							  buf, 1));
+		} else {
+			q->u = 0;
+		}
+	} else {
+		DRM_INFO("Adding EDID quirk: %s\n",
+			 drm_edid_quirk_format(quirk, buf, 1));
+		q = drm_edid_quirk_find_by_id(quirk->s.display_id);
+		if (q == NULL) {
+			q = drm_edid_quirk_find_empty();
+			if (q == NULL) {
+				printk(KERN_WARNING
+				       "No free slot in EDID quirk list\n");
+				res = -ENOSPC;
+			} else {
+				q->u = quirk->u;
+			}
+		} else {
+			DRM_INFO("Replacing existing quirk: %s\n",
+				 drm_edid_quirk_format(q, buf, 1));
+			q->s.quirks = quirk->s.quirks;
+		}
+	}
+
+	mutex_unlock(&edid_quirk_list_mutex);
+
+	return res;
+}
+
+/**
+ * drm_edid_quirks_process - parse and process a comma separated list of EDID
+ * 			     quirks
+ * @s: string containing the quirks to be processed
+ * @strict: if non-zero, any parsing or processing error aborts further
+ * 	    processing
+ *
+ * Returns 0 on success, < 0 if any error is encountered.  (If multiple errors
+ * occur when strict is set to 0, the last error encountered is returned.)
+ */
+static int drm_edid_quirks_process(const char *s, int strict)
+{
+	union edid_quirk quirk;
+	int res = 0;
+
+	do {
+
+		if (*s == '@') {
+			DRM_INFO("Clearing EDID quirk list\n");
+			mutex_lock(&edid_quirk_list_mutex);
+			memset(edid_quirk_list, 0, sizeof edid_quirk_list);
+			mutex_unlock(&edid_quirk_list_mutex);
+		} else {
+			res = drm_edid_quirk_parse(s, &quirk);
+			if (res != 0) {
+				if (strict)
+					goto error;
+				continue;
+			}
+
+			res = drm_edid_quirk_process(&quirk);
+			if (res != 0) {
+				if (strict)
+					goto error;
+			}
+		}
+
+		s = strpbrk(s, ",\n");
+
+	} while (s != NULL && *(++s) != 0);
+
+	return res;
+
+error:
+	printk(KERN_WARNING "Aborting EDID quirk parsing\n");
+	return res;
+}
+
+/**
+ * drm_edid_quirks_param_process - process the edid_quirks module parameter
+ */
+void drm_edid_quirks_param_process(void)
+{
+	if (drm_edid_quirks != NULL)
+		drm_edid_quirks_process(drm_edid_quirks, 0);
+}
+
+/**
+ * drm_edid_quirks_size_show - show the size of the EDID quirk list in sysfs
+ * @buf: destination buffer (PAGE_SIZE bytes)
+ */
+ssize_t drm_edid_quirks_size_show(struct class *class,
+				  struct class_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%zu\n", ARRAY_SIZE(edid_quirk_list));
+}
+
+/**
+ * drm_edid_quirks_show - show the contents of the EDID quirk list in sysfs
+ * @buf: destination buffer (PAGE_SIZE bytes)
+ */
+ssize_t drm_edid_quirks_show(struct class *class, struct class_attribute *attr,
+			     char *buf)
+{
+	const union edid_quirk *q = edid_quirk_list;
+	ssize_t count = 0;
+
+	BUILD_BUG_ON(ARRAY_SIZE(edid_quirk_list) >
+				PAGE_SIZE / EDID_QUIRK_BUF_SIZE);
+
+	mutex_lock(&edid_quirk_list_mutex);
+
+	do {
+		if (q->s.quirks != 0) {
+			drm_edid_quirk_format(q, buf + count, 0);
+			(buf + count)[EDID_QUIRK_BUF_SIZE - 1] = '\n';
+			count += EDID_QUIRK_BUF_SIZE;
+		}
+	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
+
+	mutex_unlock(&edid_quirk_list_mutex);
+
+	return count;
+}
+
+/**
+ * drm_edid_quirks_store - parse and process EDID qurik list changes written
+ *			   to sysfs attribute
+ */
+ssize_t drm_edid_quirks_store(struct class *class, struct class_attribute *attr,
+			      const char *buf, size_t count)
+{
+	int res;
+
+	res = drm_edid_quirks_process(buf, 1);
+	if (res != 0)
+		return res;
+
+	return count;
+}
+
 /*** DDC fetch and block validation ***/
 
 static const u8 edid_header[] = {
@@ -409,25 +817,6 @@ EXPORT_SYMBOL(drm_get_edid);
 /*** EDID parsing ***/
 
 /**
- * edid_vendor - match a string against EDID's obfuscated vendor field
- * @edid: EDID to match
- * @vendor: vendor string
- *
- * Returns true if @vendor is in @edid, false otherwise
- */
-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) + '@';
-
-	return !strncmp(edid_vendor, vendor, 3);
-}
-
-/**
  * edid_get_quirks - return quirk flags for a given EDID
  * @edid: EDID to process
  *
@@ -435,18 +824,18 @@ static bool edid_vendor(struct edid *edid, char *vendor)
  */
 static u32 edid_get_quirks(struct edid *edid)
 {
-	struct edid_quirk *quirk;
-	int i;
+	union edid_quirk *q;
+	u32 quirks = 0;
 
-	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
-		quirk = &edid_quirk_list[i];
+	mutex_lock(&edid_quirk_list_mutex);
 
-		if (edid_vendor(edid, quirk->vendor) &&
-		    (EDID_PRODUCT_ID(edid) == quirk->product_id))
-			return quirk->quirks;
-	}
+	q = drm_edid_quirk_find_by_id(edid->display_id);
+	if (q != NULL)
+		quirks = q->s.quirks;
 
-	return 0;
+	mutex_unlock(&edid_quirk_list_mutex);
+
+	return quirks;
 }
 
 #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
@@ -1162,7 +1551,7 @@ do_inferred_modes(struct detailed_timing *timing, void *c)
 	closure->modes += drm_dmt_modes_for_range(closure->connector,
 						  closure->edid,
 						  timing);
-	
+
 	if (!version_greater(closure->edid, 1, 1))
 		return; /* GTF not defined yet */
 
@@ -1399,7 +1788,7 @@ do_cvt_mode(struct detailed_timing *timing, void *c)
 
 static int
 add_cvt_modes(struct drm_connector *connector, struct edid *edid)
-{	
+{
 	struct detailed_mode_closure closure = {
 		connector, edid, 0, 0, 0
 	};
@@ -1615,15 +2004,12 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
 
 	eld[0] = 2 << 3;		/* ELD version: 2 */
 
-	eld[16] = edid->mfg_id[0];
-	eld[17] = edid->mfg_id[1];
-	eld[18] = edid->prod_code[0];
-	eld[19] = edid->prod_code[1];
+	*(u32 *)(&eld[16]) = edid->display_id.u;
 
 	if (cea[1] >= 3)
 		for (db = cea + 4; db < cea + cea[2]; db += dbl + 1) {
 			dbl = db[0] & 0x1f;
-			
+
 			switch ((db[0] & 0xe0) >> 5) {
 			case AUDIO_BLOCK:
 				/* Audio Data Block, contains SADs */
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 21bcd4a..b939d51 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -46,16 +46,22 @@ EXPORT_SYMBOL(drm_vblank_offdelay);
 unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
 EXPORT_SYMBOL(drm_timestamp_precision);
 
+char *drm_edid_quirks = NULL;
+EXPORT_SYMBOL(drm_edid_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, "MFG:prod:flags[,MFG:prod:flags[...]]\n"
+			      "(See Documentation/EDID/edid_quirks.txt)");
 
 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_quirks, charp, 0400);
 
 struct idr drm_minors_idr;
 
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 45ac8d6..84dc365 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -84,6 +84,11 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
 		__stringify(CORE_PATCHLEVEL) " "
 		CORE_DATE);
 
+static CLASS_ATTR(edid_quirks_size, 0400, drm_edid_quirks_size_show, 0);
+
+static CLASS_ATTR(edid_quirks, 0600, drm_edid_quirks_show,
+		  drm_edid_quirks_store);
+
 /**
  * drm_sysfs_create - create a struct drm_sysfs_class structure
  * @owner: pointer to the module that is to "own" this struct drm_sysfs_class
@@ -113,10 +118,22 @@ struct class *drm_sysfs_create(struct module *owner, char *name)
 	if (err)
 		goto err_out_class;
 
+	err = class_create_file(class, &class_attr_edid_quirks_size);
+	if (err)
+		goto err_out_version;
+
+	err = class_create_file(class, &class_attr_edid_quirks);
+	if (err)
+		goto err_out_quirks_size;
+
 	class->devnode = drm_devnode;
 
 	return class;
 
+err_out_quirks_size:
+	class_remove_file(class, &class_attr_edid_quirks_size);
+err_out_version:
+	class_remove_file(class, &class_attr_version.attr);
 err_out_class:
 	class_destroy(class);
 err_out:
@@ -132,6 +149,8 @@ void drm_sysfs_destroy(void)
 {
 	if ((drm_class == NULL) || (IS_ERR(drm_class)))
 		return;
+	class_remove_file(drm_class, &class_attr_edid_quirks);
+	class_remove_file(drm_class, &class_attr_edid_quirks_size);
 	class_remove_file(drm_class, &class_attr_version.attr);
 	class_destroy(drm_class);
 	drm_class = NULL;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d6b67bb..c947f3e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1501,6 +1501,7 @@ extern unsigned int drm_debug;
 
 extern unsigned int drm_vblank_offdelay;
 extern unsigned int drm_timestamp_precision;
+extern char *drm_edid_quirks;
 
 extern struct class *drm_class;
 extern struct proc_dir_entry *drm_proc_root;
@@ -1612,6 +1613,15 @@ void drm_gem_vm_open(struct vm_area_struct *vma);
 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) */
+void drm_edid_quirks_param_process(void);
+ssize_t drm_edid_quirks_size_show(struct class *class,
+				  struct class_attribute *attr, char *buf);
+ssize_t drm_edid_quirks_show(struct class *class, struct class_attribute *attr,
+			     char *buf);
+ssize_t drm_edid_quirks_store(struct class *class, struct class_attribute *attr,
+			      const char *buf, size_t count);
+
 #include "drm_global.h"
 
 static inline void
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 0cac551..713229b 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -202,11 +202,18 @@ struct detailed_timing {
 #define DRM_EDID_FEATURE_PM_SUSPEND       (1 << 6)
 #define DRM_EDID_FEATURE_PM_STANDBY       (1 << 7)
 
+union edid_display_id {
+	struct {
+		__be16 mfg_id;
+		__le16 prod_code;
+	} __attribute__((packed)) s;
+	u32 u;
+};
+
 struct edid {
 	u8 header[8];
 	/* Vendor & product info */
-	u8 mfg_id[2];
-	u8 prod_code[2];
+	union edid_display_id display_id;
 	u32 serial; /* FIXME: byte order */
 	u8 mfg_week;
 	u8 mfg_year;
@@ -242,8 +249,6 @@ struct edid {
 	u8 checksum;
 } __attribute__((packed));
 
-#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
-
 struct drm_encoder;
 struct drm_connector;
 struct drm_display_mode;
-- 
1.7.11.4

Re: [PATCH v5 1/3] drm: Add user-defined EDID quirks capability

[Jani Nikula]

On Fri, 28 Sep 2012, Ian Pilcher <arequipeno@gmail.com> wrote:
> Add the ability for users to define their own EDID quirks via a
> module parameter or sysfs attribute.

Hi Ian -

IMHO this patch should be chopped up to smaller pieces. For example,
change the edid_quirk_list format first (if you must), then add module
parameter support, then add sysfs support, in separate patches. It'll be
easier to review.

Please see some other comments inline.

BR,
Jani.

>
> Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> ---
>  Documentation/EDID/edid_quirks.txt | 126 ++++++++++
>  drivers/gpu/drm/drm_drv.c          |   2 +
>  drivers/gpu/drm/drm_edid.c         | 500 ++++++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/drm_stub.c         |   6 +
>  drivers/gpu/drm/drm_sysfs.c        |  19 ++
>  include/drm/drmP.h                 |  10 +
>  include/drm/drm_edid.h             |  13 +-
>  7 files changed, 615 insertions(+), 61 deletions(-)
>  create mode 100644 Documentation/EDID/edid_quirks.txt
>
> diff --git a/Documentation/EDID/edid_quirks.txt b/Documentation/EDID/edid_quirks.txt
> new file mode 100644
> index 0000000..0c9c746
> --- /dev/null
> +++ b/Documentation/EDID/edid_quirks.txt
> @@ -0,0 +1,126 @@
> +                                  EDID Quirks
> +                                 =============
> +                       Ian Pilcher <arequipeno@gmail.com>
> +                                August 11, 2012
> +
> +
> +    "EDID blocks out in the wild have a variety of bugs"
> +        -- from drivers/gpu/drm/drm_edid.c
> +
> +
> +Overview
> +========
> +
> +EDID quirks provide a mechanism for working around display hardware with buggy
> +EDID data.
> +
> +An individual EDID quirk maps a display type (identified by its EDID
> +manufacturer ID and product code[1]) to a set of "quirk flags."  The kernel
> +includes a variety of built-in quirks.  (They are stored in the edid_quirk_list
> +array in drivers/gpu/drm/drm_edid.c.)
> +
> +An example of a built-in EDID quirk is:
> +
> +    ACR:0xad46:0x00000001
> +
> +The first field is the manufacturer ID (Acer, Inc.), the second field is the
> +manufacturer's product code, and the third field contains the quirk flags for
> +that display type.
> +
> +The quirk flags are defined in drivers/gpu/drm/drm_edid.c.  Each flag has a
> +symbolic name beginning with EDID_QUIRK_, along with a numerical value.  Each
> +flag should also have an associated comment which provides an idea of its
> +effect.  Note that the values in the source file are expressed as bit shifts[2]:
> +
> +    * 1 << 0: 0x0001
> +    * 1 << 1: 0x0002
> +    * 1 << 2: 0x0004
> +    * etc.
> +
> +
> +sysfs interface
> +===============
> +
> +The current EDID quirk list can be read from /sys/class/drm/edid_quirks:
> +
> +    # cat /sys/class/drm/edid_quirks
> +       ACR:0xad46:0x00000001
> +       API:0x7602:0x00000001
> +       ACR:0x0977:0x00000020
> +    0x9e6a:0x077e:0x00000080
> +    ...
> +
> +("Nonconformant" manufacturer IDs are displayed as hexadecimal values.)
> +
> +The number of total "slots" in the list can be read from
> +/sys/class/drm/edid_quirks_size.  This total includes both occupied slots (i.e.
> +the current list) and any slots available for additional quirks.  The number of
> +available slots can be calculated by subtracting the number of quirks in the
> +current list from the total number of slots.
> +
> +If a slot is available, an additional quirk can be added to the list by writing
> +it to /sys/class/drm/edid_quirks:
> +
> +    # echo FOO:0xffff:0x100 > /sys/class/drm/edid_quirks
> +
> +Manufacturer IDs can also be specified numerically.  (This is the only way to
> +specify a nonconformant ID.) This command is equivalent to the previous one:
> +
> +    # echo 0x19ef:0xffff:0x100 > /sys/class/drm/edid_quirks
> +
> +Numeric values can also be specified in decimal or octal formats; a number that
> +begins with a 0 is assumed to be octal:
> +
> +    # echo FOO:65535:0400 > /sys/class/drm/edid_quirks
> +
> +An existing quirk can be replaced by writing a new set of flags:
> +
> +    # echo FOO:0xffff:0x200 > /sys/class/drm/edid_quirks
> +
> +A quirk can be deleted from the list by writing an empty flag set (0). This
> +makes the slot occupied by that quirk available.
> +
> +    # echo FOO:0xffff:0 > /sys/class/drm/edid_quirks
> +
> +Writing an "at symbol" (@) clears the entire quirk list:
> +
> +    # echo @ > /sys/class/drm/edid_quirks
> +
> +Multiple changes to the list can be specified in a comma (or newline) separated
> +list. For example, the following command clears all of the existing quirks in
> +the list and adds 3 new quirks:
> +
> +    # echo @,FOO:0xffff:0x100,BAR:0x1111:0x001,BAZ:0x2222:0x002 > \
> +            /sys/class/drm/edid_quirks
> +
> +Note however, that any error (an incorrectly formatted quirk or an attempt to
> +add a quirk when no slot is available) will abort processing of any further
> +changes, potentially making it difficult to determine exactly which change
> +caused the error and what changes were made.  For this reason, making changes
> +one at a time is recommended, particularly if the changes are being made by a
> +script or program.

Generally it seems like a bad idea to add support for something you
specifically recommend against using. It should be a hint not to add
it. It looks like you support multiple changes in sysfs only because it
comes free with the module parameter support.

> +
> +
> +Module parameter
> +================
> +
> +The EDID quirk list can also be modified via the edid_quirks module parameter
> +(drm.edid_quirks on the kernel command line).  The effect of setting this
> +parameter is identical to the effect of writing its value to
> +/sys/class/drm/edid_quirks, with one important difference.  When an error is
> +encountered during module parameter parsing or processing, any remaining quirks
> +in the parameter string will still be processed.  (It is hoped that this approach
> +maximizes the probability of producing a working display.)
> +
> +
> +Follow-up
> +=========
> +
> +If you encounter a display that requires an additional EDID quirk in order to
> +function properly, please report it to the direct rendering development mailing
> +list <dri-devel@lists.freedesktop.org>.
> +
> +
> +[1] See http://en.wikipedia.org/wiki/Extended_display_identification_data for a
> +    description of the manufacturer ID and product code fields.
> +[2] https://en.wikipedia.org/wiki/Bitwise_operation#Bit_shifts
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9238de4..7fe39e0 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -276,6 +276,8 @@ static int __init drm_core_init(void)
>  		goto err_p3;
>  	}
>  
> +	drm_edid_quirks_param_process();
> +
>  	DRM_INFO("Initialized %s %d.%d.%d %s\n",
>  		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a8743c3..ea535f6 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -31,6 +31,8 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/ctype.h>
> +
>  #include "drmP.h"
>  #include "drm_edid.h"
>  #include "drm_edid_modes.h"
> @@ -82,51 +84,457 @@ struct detailed_mode_closure {
>  #define LEVEL_GTF2	2
>  #define LEVEL_CVT	3
>  
> -static struct edid_quirk {
> -	char vendor[4];
> -	int product_id;
> -	u32 quirks;
> -} edid_quirk_list[] = {
> +union edid_quirk {
> +	struct {
> +		union edid_display_id display_id;
> +		u32 quirks;
> +	} __attribute__((packed)) s;
> +	u64 u;
> +};

This does not need to be an union. Just make it a struct, and in the
couple of places you need .u, you can do a memset and a struct
assignment or memcpy.

> +
> +#define EDID_MFG_ID(c1, c2, c3)		cpu_to_be16(			\
> +						(c1 & 0x1f) << 10 |	\
> +						(c2 & 0x1f) << 5 |	\
> +						(c3 & 0x1f)		\
> +					)
> +
> +#define EDID_QUIRK_LIST_SIZE	24
> +
> +union edid_quirk edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
> +
>  	/* Acer AL1706 */
> -	{ "ACR", 44358, EDID_QUIRK_PREFER_LARGE_60 },
> +	{ { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(44358) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },

I wonder whether would be better to have this all in cpu byte order and
code to handle it, or this confusing mixture of explicit big-endian,
explicit little-endian, and cpu order. Someone, somewhere is bound to
miss a byte order change later on...

>  	/* Acer F51 */
> -	{ "API", 0x7602, EDID_QUIRK_PREFER_LARGE_60 },
> +	{ { { { EDID_MFG_ID('A', 'P', 'I'), cpu_to_le16(0x7602) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
>  	/* Unknown Acer */
> -	{ "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> +	{ { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(2423) } },
> +		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>  
>  	/* Belinea 10 15 55 */
> -	{ "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
> -	{ "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
> +	{ { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(1516) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
> +	{ { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(0x77e) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
>  
>  	/* Envision Peripherals, Inc. EN-7100e */
> -	{ "EPI", 59264, EDID_QUIRK_135_CLOCK_TOO_HIGH },
> +	{ { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(59264) } },
> +		EDID_QUIRK_135_CLOCK_TOO_HIGH } },
>  	/* Envision EN2028 */
> -	{ "EPI", 8232, EDID_QUIRK_PREFER_LARGE_60 },
> +	{ { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(8232) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
>  
>  	/* Funai Electronics PM36B */
> -	{ "FCM", 13600, EDID_QUIRK_PREFER_LARGE_75 |
> -	  EDID_QUIRK_DETAILED_IN_CM },
> +	{ { { { EDID_MFG_ID('F', 'C', 'M'), cpu_to_le16(13600) } },
> +		EDID_QUIRK_PREFER_LARGE_75 | EDID_QUIRK_DETAILED_IN_CM } },
>  
>  	/* LG Philips LCD LP154W01-A5 */
> -	{ "LPL", 0, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
> -	{ "LPL", 0x2a00, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
> +	{ { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0) } },
> +		EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
> +	{ { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0x2a00) } },
> +		EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
>  
>  	/* Philips 107p5 CRT */
> -	{ "PHL", 57364, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> +	{ { { { EDID_MFG_ID('P', 'H', 'L'), cpu_to_le16(57364) } },
> +		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>  
>  	/* Proview AY765C */
> -	{ "PTS", 765, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> +	{ { { { EDID_MFG_ID('P', 'T', 'S'), cpu_to_le16(765) } },
> +		EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>  
>  	/* Samsung SyncMaster 205BW.  Note: irony */
> -	{ "SAM", 541, EDID_QUIRK_DETAILED_SYNC_PP },
> +	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(541) } },
> +		EDID_QUIRK_DETAILED_SYNC_PP } },
>  	/* Samsung SyncMaster 22[5-6]BW */
> -	{ "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 },
> -	{ "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 },
> +	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(596) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
> +	{ { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(638) } },
> +		EDID_QUIRK_PREFER_LARGE_60 } },
>  
>  	/* ViewSonic VA2026w */
> -	{ "VSC", 5020, EDID_QUIRK_FORCE_REDUCED_BLANKING },
> +	{ { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
> +		EDID_QUIRK_FORCE_REDUCED_BLANKING } },
> +
> +	/*
> +	 * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE to
> +	 * provide some room for user-supplied quirks.
> +	 */
>  };
>  
> +DEFINE_MUTEX(edid_quirk_list_mutex);
> +
> +/**
> + * drm_edid_mfg_format - format an "encoded" EDID manufacturer ID for printing
> + * @mfg_id: the encoded manufacturer ID
> + * @buf: destination buffer for the formatted manufacturer ID (minimum 7 bytes)
> + * @strip: if non-zero, the returned pointer will skip any leading spaces
> + *
> + * An EDID manufacturer ID is supposed to consist of 3 capital letters (A-Z).
> + * Each letter is stored as a 5-bit value between 1 and 26, taking up 15 bits of
> + * the 16-bit ID. The remaining bit should always be 0. If display manufacturers
> + * always did things correctly, however, EDID quirks wouldn't be required in
> + * the first place. This function does the following:
> + *
> + * - Broken IDs are printed in hexadecimal (0xffff).
> + * - "Correct" IDs are formatted as a 3-letter ID string, preceded by 3 spaces;
> + *   the spaces ensure that both output formats are the same length.
> + *
> + * Thus, a formatted manufacturer ID is always 6 characters long (not including
> + * the terminating 0).
> + *
> + * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
> + * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
> + * been formatted as a 3-letter string, a pointer to the first non-space
> + * character (@buf + 3) is returned.
> + */
> +static const char *drm_edid_mfg_format(__be16 mfg_id, char *buf, int strip)
> +{
> +	u16 id = be16_to_cpu(mfg_id);
> +
> +	if (id & 0x8000)
> +		goto bad_id;
> +
> +	buf[3] = ((id & 0x7c00) >> 10) + '@';

Shift first and you can use the same mask for all three.

> +	if (!isupper(buf[3]))
> +		goto bad_id;
> +
> +	buf[4] = ((id & 0x03e0) >> 5) + '@';
> +	if (!isupper(buf[4]))
> +		goto bad_id;
> +
> +	buf[5] = (id & 0x001f) + '@';
> +	if (!isupper(buf[5]))
> +		goto bad_id;
> +
> +	memset(buf, ' ', 3);
> +	buf[6] = 0;
> +
> +	return strip ? (buf + 3) : buf;
> +
> +bad_id:
> +	sprintf(buf, "0x%04hx", id);
> +	return buf;
> +}
> +
> +#define EDID_MFG_BUF_SIZE		7
> +
> +/**
> + * drm_edid_display_id_format - format an EDID "display ID" (manufacturer ID
> + * 				and product code) for printing
> + * @display_id: the display ID
> + * @buf: destination buffer for the formatted display ID (minimum 14 bytes)
> + * @strip: if non-zero, the returned pointer will skip any leading spaces
> + *
> + * A formatted display ID is always 13 characters long (not including the
> + * terminating 0).
> + *
> + * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
> + * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
> + * been formatted as a 3-letter string, a pointer to the first non-space
> + * character (@buf + 3) is returned.
> + */
> +static const char *drm_edid_display_id_format(union edid_display_id display_id,
> +					      char *buf, int strip)
> +{
> +	const char *s;
> +
> +	s = drm_edid_mfg_format(display_id.s.mfg_id, buf, strip);
> +	sprintf(buf + EDID_MFG_BUF_SIZE - 1, ":0x%04hx",
> +		le16_to_cpu(display_id.s.prod_code));
> +
> +	return s;
> +}
> +
> +#define EDID_DISPLAY_ID_BUF_SIZE	(EDID_MFG_BUF_SIZE + 7)
> +
> +/**
> + * drm_edid_quirk_format - format an EDID quirk for printing
> + * @quirk: the quirk
> + * @buf: destination buffer for the formatted quirk (minimum 25 bytes)
> + * @strip: if non-zero, the returned pointer will skip any leading spaces
> + *
> + * A formatted EDID quirk is always 24 characters long (not including the
> + * terminating 0).
> + *
> + * If @strip is 0, or the manufacturer ID has been formatted as a hexadecimal
> + * number, @buf is returned.  If @strip is non-zero, and the manufacturer ID has
> + * been formatted as a 3-letter string, a pointer to the first non-space
> + * character (@buf + 3) is returned.
> + */
> +static const char *drm_edid_quirk_format(const union edid_quirk *quirk,
> +					 char *buf, int strip)
> +{
> +	const char *s;
> +
> +	s = drm_edid_display_id_format(quirk->s.display_id, buf, strip);
> +	sprintf(buf + EDID_DISPLAY_ID_BUF_SIZE - 1, ":0x%08x", quirk->s.quirks);
> +
> +	return s;
> +}
> +
> +#define EDID_QUIRK_BUF_SIZE		(EDID_DISPLAY_ID_BUF_SIZE + 11)
> +
> +/**
> + * drm_edid_quirk_parse - parse an EDID quirk
> + * @s: string containing the quirk to be parsed
> + * @quirk: destination for parsed quirk
> + *
> + * Returns 0 on success, < 0 (currently -EINVAL) on error.
> + */
> +static int drm_edid_quirk_parse(const char *s, union edid_quirk *quirk)
> +{
> +	char buf[EDID_QUIRK_BUF_SIZE];
> +	s32 mfg;
> +	s32 product;
> +	s64 quirks;
> +	char *c;
> +
> +	if (sscanf(s, "%i:%i:%lli", &mfg, &product, &quirks) == 3) {
> +		if (mfg < 0 || mfg > 0xffff)
> +			goto error;
> +		quirk->s.display_id.s.mfg_id = cpu_to_be16((u16)mfg);
> +	} else {
> +		if (sscanf(s, "%3s:%i:%lli", buf, &product, &quirks) != 3 ||
> +				!isupper(buf[0]) ||
> +				!isupper(buf[1]) ||
> +				!isupper(buf[2]))
> +			goto error;
> +		quirk->s.display_id.s.mfg_id =
> +				EDID_MFG_ID(buf[0], buf[1], buf[2]);
> +	}
> +
> +	if (product < 0 || product > 0xffff ||
> +			quirks < 0 || quirks > 0xffffffffLL)
> +		goto error;
> +
> +	quirk->s.display_id.s.prod_code = cpu_to_le16((u16)product);
> +	quirk->s.quirks = (u32)quirks;
> +
> +	DRM_DEBUG("Successfully parsed EDID quirk: %s\n",
> +		  drm_edid_quirk_format(quirk, buf, 1));
> +
> +	return 0;
> +
> +error:
> +	c = strpbrk(s, ",\n");
> +	if (c == NULL) {
> +		printk(KERN_WARNING "Invalid EDID quirk: '%s'\n", s);
> +	} else {
> +		printk(KERN_WARNING "Invalid EDID quirk: '%.*s'\n",
> +		      (int)(c - s), s);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/**
> + * drm_edid_quirk_find_by_id - find the EDID quirk matching a display ID
> + * @display_id: the display ID to match
> + *
> + * Caller MUST hold edid_quirk_list_mutex.
> + *
> + * Returns a pointer to the matching quirk list entry, NULL if no such entry
> + * exists.
> + */
> +static union edid_quirk *drm_edid_quirk_find_by_id(union edid_display_id id)
> +{
> +	union edid_quirk *q = edid_quirk_list;
> +
> +	do {
> +		if (q->s.display_id.u == id.u && q->s.quirks != 0)
> +			return q;
> +	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));

The same with less cognitive burden on the reader:

	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
		...
	}

Ditto elsewhere.

> +
> +	return NULL;
> +}
> +
> +/**
> + * drm_edid_quirk_find_slot - find an empty slot in the EDID quirk list
> + *
> + * Caller MUST hold edid_quirk_list_mutex.
> + *
> + * Returns a pointer to the first empty slot, NULL if no empty slots exist.
> + */
> +static union edid_quirk *drm_edid_quirk_find_empty(void)
> +{
> +	union edid_quirk *q = edid_quirk_list;
> +
> +	do {
> +		if (q->s.quirks == 0)
> +			return q;
> +	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
> +
> +	return NULL;
> +}
> +
> +/**
> + * drm_edid_quirk_process - process a newly parsed EDID quirk
> + * @quirk: the quirk to be processed
> + *
> + * Depending on the newly parsed quirk and the contents of the quirks list, this
> + * function will add, remove, or replace a quirk.
> + *
> + * Returns 0 on success, < 0 on error (-ENOSPC if there is no free slot for a
> + * new quirk). Note that trying to remove a quirk that isn't present is not
> + * considered an error.
> + */
> +static int drm_edid_quirk_process(const union edid_quirk *quirk)
> +{
> +	char buf[EDID_QUIRK_BUF_SIZE];
> +	union edid_quirk *q;
> +	int res = 0;
> +
> +	mutex_lock(&edid_quirk_list_mutex);
> +
> +	if (quirk->s.quirks == 0) {
> +		DRM_INFO("Removing EDID quirk for display %s\n",
> +			 drm_edid_display_id_format(quirk->s.display_id,
> +						    buf, 1));
> +		q = drm_edid_quirk_find_by_id(quirk->s.display_id);
> +		if (q == NULL) {
> +			printk(KERN_WARNING "No quirk found for display %s\n",
> +			       drm_edid_display_id_format(quirk->s.display_id,
> +							  buf, 1));
> +		} else {
> +			q->u = 0;

Ditch the union and use memset.

> +		}
> +	} else {
> +		DRM_INFO("Adding EDID quirk: %s\n",
> +			 drm_edid_quirk_format(quirk, buf, 1));
> +		q = drm_edid_quirk_find_by_id(quirk->s.display_id);
> +		if (q == NULL) {
> +			q = drm_edid_quirk_find_empty();
> +			if (q == NULL) {
> +				printk(KERN_WARNING
> +				       "No free slot in EDID quirk list\n");
> +				res = -ENOSPC;
> +			} else {
> +				q->u = quirk->u;

Ditch the union and use memcpy or struct assignment.

> +			}
> +		} else {
> +			DRM_INFO("Replacing existing quirk: %s\n",
> +				 drm_edid_quirk_format(q, buf, 1));
> +			q->s.quirks = quirk->s.quirks;
> +		}
> +	}
> +
> +	mutex_unlock(&edid_quirk_list_mutex);
> +
> +	return res;
> +}
> +
> +/**
> + * drm_edid_quirks_process - parse and process a comma separated list of EDID
> + * 			     quirks
> + * @s: string containing the quirks to be processed
> + * @strict: if non-zero, any parsing or processing error aborts further
> + * 	    processing
> + *
> + * Returns 0 on success, < 0 if any error is encountered.  (If multiple errors
> + * occur when strict is set to 0, the last error encountered is returned.)
> + */
> +static int drm_edid_quirks_process(const char *s, int strict)
> +{
> +	union edid_quirk quirk;
> +	int res = 0;
> +
> +	do {
> +
> +		if (*s == '@') {
> +			DRM_INFO("Clearing EDID quirk list\n");
> +			mutex_lock(&edid_quirk_list_mutex);
> +			memset(edid_quirk_list, 0, sizeof edid_quirk_list);
> +			mutex_unlock(&edid_quirk_list_mutex);
> +		} else {
> +			res = drm_edid_quirk_parse(s, &quirk);
> +			if (res != 0) {
> +				if (strict)
> +					goto error;
> +				continue;
> +			}
> +
> +			res = drm_edid_quirk_process(&quirk);
> +			if (res != 0) {
> +				if (strict)
> +					goto error;
> +			}
> +		}
> +
> +		s = strpbrk(s, ",\n");
> +
> +	} while (s != NULL && *(++s) != 0);
> +
> +	return res;
> +
> +error:
> +	printk(KERN_WARNING "Aborting EDID quirk parsing\n");
> +	return res;
> +}
> +
> +/**
> + * drm_edid_quirks_param_process - process the edid_quirks module parameter
> + */
> +void drm_edid_quirks_param_process(void)
> +{
> +	if (drm_edid_quirks != NULL)
> +		drm_edid_quirks_process(drm_edid_quirks, 0);
> +}
> +
> +/**
> + * drm_edid_quirks_size_show - show the size of the EDID quirk list in sysfs
> + * @buf: destination buffer (PAGE_SIZE bytes)
> + */
> +ssize_t drm_edid_quirks_size_show(struct class *class,
> +				  struct class_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%zu\n", ARRAY_SIZE(edid_quirk_list));
> +}
> +
> +/**
> + * drm_edid_quirks_show - show the contents of the EDID quirk list in sysfs
> + * @buf: destination buffer (PAGE_SIZE bytes)
> + */
> +ssize_t drm_edid_quirks_show(struct class *class, struct class_attribute *attr,
> +			     char *buf)
> +{
> +	const union edid_quirk *q = edid_quirk_list;
> +	ssize_t count = 0;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(edid_quirk_list) >
> +				PAGE_SIZE / EDID_QUIRK_BUF_SIZE);
> +
> +	mutex_lock(&edid_quirk_list_mutex);
> +
> +	do {
> +		if (q->s.quirks != 0) {
> +			drm_edid_quirk_format(q, buf + count, 0);
> +			(buf + count)[EDID_QUIRK_BUF_SIZE - 1] = '\n';
> +			count += EDID_QUIRK_BUF_SIZE;
> +		}
> +	} while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
> +
> +	mutex_unlock(&edid_quirk_list_mutex);
> +
> +	return count;
> +}
> +
> +/**
> + * drm_edid_quirks_store - parse and process EDID qurik list changes written
> + *			   to sysfs attribute
> + */
> +ssize_t drm_edid_quirks_store(struct class *class, struct class_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	int res;
> +
> +	res = drm_edid_quirks_process(buf, 1);
> +	if (res != 0)
> +		return res;
> +
> +	return count;
> +}
> +
>  /*** DDC fetch and block validation ***/
>  
>  static const u8 edid_header[] = {
> @@ -409,25 +817,6 @@ EXPORT_SYMBOL(drm_get_edid);
>  /*** EDID parsing ***/
>  
>  /**
> - * edid_vendor - match a string against EDID's obfuscated vendor field
> - * @edid: EDID to match
> - * @vendor: vendor string
> - *
> - * Returns true if @vendor is in @edid, false otherwise
> - */
> -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) + '@';
> -
> -	return !strncmp(edid_vendor, vendor, 3);
> -}
> -
> -/**
>   * edid_get_quirks - return quirk flags for a given EDID
>   * @edid: EDID to process
>   *
> @@ -435,18 +824,18 @@ static bool edid_vendor(struct edid *edid, char *vendor)
>   */
>  static u32 edid_get_quirks(struct edid *edid)
>  {
> -	struct edid_quirk *quirk;
> -	int i;
> +	union edid_quirk *q;
> +	u32 quirks = 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
> -		quirk = &edid_quirk_list[i];
> +	mutex_lock(&edid_quirk_list_mutex);
>  
> -		if (edid_vendor(edid, quirk->vendor) &&
> -		    (EDID_PRODUCT_ID(edid) == quirk->product_id))
> -			return quirk->quirks;
> -	}
> +	q = drm_edid_quirk_find_by_id(edid->display_id);
> +	if (q != NULL)
> +		quirks = q->s.quirks;
>  
> -	return 0;
> +	mutex_unlock(&edid_quirk_list_mutex);
> +
> +	return quirks;
>  }
>  
>  #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
> @@ -1162,7 +1551,7 @@ do_inferred_modes(struct detailed_timing *timing, void *c)
>  	closure->modes += drm_dmt_modes_for_range(closure->connector,
>  						  closure->edid,
>  						  timing);
> -	
> +
>  	if (!version_greater(closure->edid, 1, 1))
>  		return; /* GTF not defined yet */
>  
> @@ -1399,7 +1788,7 @@ do_cvt_mode(struct detailed_timing *timing, void *c)
>  
>  static int
>  add_cvt_modes(struct drm_connector *connector, struct edid *edid)
> -{	
> +{
>  	struct detailed_mode_closure closure = {
>  		connector, edid, 0, 0, 0
>  	};
> @@ -1615,15 +2004,12 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  
>  	eld[0] = 2 << 3;		/* ELD version: 2 */
>  
> -	eld[16] = edid->mfg_id[0];
> -	eld[17] = edid->mfg_id[1];
> -	eld[18] = edid->prod_code[0];
> -	eld[19] = edid->prod_code[1];
> +	*(u32 *)(&eld[16]) = edid->display_id.u;
>  
>  	if (cea[1] >= 3)
>  		for (db = cea + 4; db < cea + cea[2]; db += dbl + 1) {
>  			dbl = db[0] & 0x1f;
> -			
> +
>  			switch ((db[0] & 0xe0) >> 5) {
>  			case AUDIO_BLOCK:
>  				/* Audio Data Block, contains SADs */
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 21bcd4a..b939d51 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -46,16 +46,22 @@ EXPORT_SYMBOL(drm_vblank_offdelay);
>  unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
>  EXPORT_SYMBOL(drm_timestamp_precision);
>  
> +char *drm_edid_quirks = NULL;
> +EXPORT_SYMBOL(drm_edid_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, "MFG:prod:flags[,MFG:prod:flags[...]]\n"
> +			      "(See Documentation/EDID/edid_quirks.txt)");
>  
>  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_quirks, charp, 0400);
>  
>  struct idr drm_minors_idr;
>  
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 45ac8d6..84dc365 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -84,6 +84,11 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
>  		__stringify(CORE_PATCHLEVEL) " "
>  		CORE_DATE);
>  
> +static CLASS_ATTR(edid_quirks_size, 0400, drm_edid_quirks_size_show, 0);
> +
> +static CLASS_ATTR(edid_quirks, 0600, drm_edid_quirks_show,
> +		  drm_edid_quirks_store);
> +
>  /**
>   * drm_sysfs_create - create a struct drm_sysfs_class structure
>   * @owner: pointer to the module that is to "own" this struct drm_sysfs_class
> @@ -113,10 +118,22 @@ struct class *drm_sysfs_create(struct module *owner, char *name)
>  	if (err)
>  		goto err_out_class;
>  
> +	err = class_create_file(class, &class_attr_edid_quirks_size);
> +	if (err)
> +		goto err_out_version;
> +
> +	err = class_create_file(class, &class_attr_edid_quirks);
> +	if (err)
> +		goto err_out_quirks_size;
> +
>  	class->devnode = drm_devnode;
>  
>  	return class;
>  
> +err_out_quirks_size:
> +	class_remove_file(class, &class_attr_edid_quirks_size);
> +err_out_version:
> +	class_remove_file(class, &class_attr_version.attr);
>  err_out_class:
>  	class_destroy(class);
>  err_out:
> @@ -132,6 +149,8 @@ void drm_sysfs_destroy(void)
>  {
>  	if ((drm_class == NULL) || (IS_ERR(drm_class)))
>  		return;
> +	class_remove_file(drm_class, &class_attr_edid_quirks);
> +	class_remove_file(drm_class, &class_attr_edid_quirks_size);
>  	class_remove_file(drm_class, &class_attr_version.attr);
>  	class_destroy(drm_class);
>  	drm_class = NULL;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d6b67bb..c947f3e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1501,6 +1501,7 @@ extern unsigned int drm_debug;
>  
>  extern unsigned int drm_vblank_offdelay;
>  extern unsigned int drm_timestamp_precision;
> +extern char *drm_edid_quirks;
>  
>  extern struct class *drm_class;
>  extern struct proc_dir_entry *drm_proc_root;
> @@ -1612,6 +1613,15 @@ void drm_gem_vm_open(struct vm_area_struct *vma);
>  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) */
> +void drm_edid_quirks_param_process(void);
> +ssize_t drm_edid_quirks_size_show(struct class *class,
> +				  struct class_attribute *attr, char *buf);
> +ssize_t drm_edid_quirks_show(struct class *class, struct class_attribute *attr,
> +			     char *buf);
> +ssize_t drm_edid_quirks_store(struct class *class, struct class_attribute *attr,
> +			      const char *buf, size_t count);
> +
>  #include "drm_global.h"
>  
>  static inline void
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 0cac551..713229b 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -202,11 +202,18 @@ struct detailed_timing {
>  #define DRM_EDID_FEATURE_PM_SUSPEND       (1 << 6)
>  #define DRM_EDID_FEATURE_PM_STANDBY       (1 << 7)
>  
> +union edid_display_id {
> +	struct {
> +		__be16 mfg_id;
> +		__le16 prod_code;
> +	} __attribute__((packed)) s;
> +	u32 u;
> +};

I think adding this union is counterproductive. The u32 version is
helpful in one comparison and one assignment, while making all the rest
just slightly more confusing.

> +
>  struct edid {
>  	u8 header[8];
>  	/* Vendor & product info */
> -	u8 mfg_id[2];
> -	u8 prod_code[2];
> +	union edid_display_id display_id;
>  	u32 serial; /* FIXME: byte order */
>  	u8 mfg_week;
>  	u8 mfg_year;
> @@ -242,8 +249,6 @@ struct edid {
>  	u8 checksum;
>  } __attribute__((packed));
>  
> -#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
> -
>  struct drm_encoder;
>  struct drm_connector;
>  struct drm_display_mode;
> -- 
> 1.7.11.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/3] drm: Add user-defined EDID quirks capability

[Ian Pilcher]

[-- Attachment #1.1: Type: text/plain, Size: 36440 bytes --]

OK. I'm done.

I've literally been discussing these patches on this list for months, and
you bring this up now?

It's far easier to simply recompile every kernel that comes out than to
continue this dance.
On Oct 9, 2012 7:10 AM, "Jani Nikula" <jani.nikula@linux.intel.com> wrote:

> On Fri, 28 Sep 2012, Ian Pilcher <arequipeno@gmail.com> wrote:
> > Add the ability for users to define their own EDID quirks via a
> > module parameter or sysfs attribute.
>
> Hi Ian -
>
> IMHO this patch should be chopped up to smaller pieces. For example,
> change the edid_quirk_list format first (if you must), then add module
> parameter support, then add sysfs support, in separate patches. It'll be
> easier to review.
>
> Please see some other comments inline.
>
> BR,
> Jani.
>
> >
> > Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
> > ---
> >  Documentation/EDID/edid_quirks.txt | 126 ++++++++++
> >  drivers/gpu/drm/drm_drv.c          |   2 +
> >  drivers/gpu/drm/drm_edid.c         | 500
> ++++++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/drm_stub.c         |   6 +
> >  drivers/gpu/drm/drm_sysfs.c        |  19 ++
> >  include/drm/drmP.h                 |  10 +
> >  include/drm/drm_edid.h             |  13 +-
> >  7 files changed, 615 insertions(+), 61 deletions(-)
> >  create mode 100644 Documentation/EDID/edid_quirks.txt
> >
> > diff --git a/Documentation/EDID/edid_quirks.txt
> b/Documentation/EDID/edid_quirks.txt
> > new file mode 100644
> > index 0000000..0c9c746
> > --- /dev/null
> > +++ b/Documentation/EDID/edid_quirks.txt
> > @@ -0,0 +1,126 @@
> > +                                  EDID Quirks
> > +                                 =============
> > +                       Ian Pilcher <arequipeno@gmail.com>
> > +                                August 11, 2012
> > +
> > +
> > +    "EDID blocks out in the wild have a variety of bugs"
> > +        -- from drivers/gpu/drm/drm_edid.c
> > +
> > +
> > +Overview
> > +========
> > +
> > +EDID quirks provide a mechanism for working around display hardware
> with buggy
> > +EDID data.
> > +
> > +An individual EDID quirk maps a display type (identified by its EDID
> > +manufacturer ID and product code[1]) to a set of "quirk flags."  The
> kernel
> > +includes a variety of built-in quirks.  (They are stored in the
> edid_quirk_list
> > +array in drivers/gpu/drm/drm_edid.c.)
> > +
> > +An example of a built-in EDID quirk is:
> > +
> > +    ACR:0xad46:0x00000001
> > +
> > +The first field is the manufacturer ID (Acer, Inc.), the second field
> is the
> > +manufacturer's product code, and the third field contains the quirk
> flags for
> > +that display type.
> > +
> > +The quirk flags are defined in drivers/gpu/drm/drm_edid.c.  Each flag
> has a
> > +symbolic name beginning with EDID_QUIRK_, along with a numerical value.
>  Each
> > +flag should also have an associated comment which provides an idea of
> its
> > +effect.  Note that the values in the source file are expressed as bit
> shifts[2]:
> > +
> > +    * 1 << 0: 0x0001
> > +    * 1 << 1: 0x0002
> > +    * 1 << 2: 0x0004
> > +    * etc.
> > +
> > +
> > +sysfs interface
> > +===============
> > +
> > +The current EDID quirk list can be read from /sys/class/drm/edid_quirks:
> > +
> > +    # cat /sys/class/drm/edid_quirks
> > +       ACR:0xad46:0x00000001
> > +       API:0x7602:0x00000001
> > +       ACR:0x0977:0x00000020
> > +    0x9e6a:0x077e:0x00000080
> > +    ...
> > +
> > +("Nonconformant" manufacturer IDs are displayed as hexadecimal values.)
> > +
> > +The number of total "slots" in the list can be read from
> > +/sys/class/drm/edid_quirks_size.  This total includes both occupied
> slots (i.e.
> > +the current list) and any slots available for additional quirks.  The
> number of
> > +available slots can be calculated by subtracting the number of quirks
> in the
> > +current list from the total number of slots.
> > +
> > +If a slot is available, an additional quirk can be added to the list by
> writing
> > +it to /sys/class/drm/edid_quirks:
> > +
> > +    # echo FOO:0xffff:0x100 > /sys/class/drm/edid_quirks
> > +
> > +Manufacturer IDs can also be specified numerically.  (This is the only
> way to
> > +specify a nonconformant ID.) This command is equivalent to the previous
> one:
> > +
> > +    # echo 0x19ef:0xffff:0x100 > /sys/class/drm/edid_quirks
> > +
> > +Numeric values can also be specified in decimal or octal formats; a
> number that
> > +begins with a 0 is assumed to be octal:
> > +
> > +    # echo FOO:65535:0400 > /sys/class/drm/edid_quirks
> > +
> > +An existing quirk can be replaced by writing a new set of flags:
> > +
> > +    # echo FOO:0xffff:0x200 > /sys/class/drm/edid_quirks
> > +
> > +A quirk can be deleted from the list by writing an empty flag set (0).
> This
> > +makes the slot occupied by that quirk available.
> > +
> > +    # echo FOO:0xffff:0 > /sys/class/drm/edid_quirks
> > +
> > +Writing an "at symbol" (@) clears the entire quirk list:
> > +
> > +    # echo @ > /sys/class/drm/edid_quirks
> > +
> > +Multiple changes to the list can be specified in a comma (or newline)
> separated
> > +list. For example, the following command clears all of the existing
> quirks in
> > +the list and adds 3 new quirks:
> > +
> > +    # echo @,FOO:0xffff:0x100,BAR:0x1111:0x001,BAZ:0x2222:0x002 > \
> > +            /sys/class/drm/edid_quirks
> > +
> > +Note however, that any error (an incorrectly formatted quirk or an
> attempt to
> > +add a quirk when no slot is available) will abort processing of any
> further
> > +changes, potentially making it difficult to determine exactly which
> change
> > +caused the error and what changes were made.  For this reason, making
> changes
> > +one at a time is recommended, particularly if the changes are being
> made by a
> > +script or program.
>
> Generally it seems like a bad idea to add support for something you
> specifically recommend against using. It should be a hint not to add
> it. It looks like you support multiple changes in sysfs only because it
> comes free with the module parameter support.
>
> > +
> > +
> > +Module parameter
> > +================
> > +
> > +The EDID quirk list can also be modified via the edid_quirks module
> parameter
> > +(drm.edid_quirks on the kernel command line).  The effect of setting
> this
> > +parameter is identical to the effect of writing its value to
> > +/sys/class/drm/edid_quirks, with one important difference.  When an
> error is
> > +encountered during module parameter parsing or processing, any
> remaining quirks
> > +in the parameter string will still be processed.  (It is hoped that
> this approach
> > +maximizes the probability of producing a working display.)
> > +
> > +
> > +Follow-up
> > +=========
> > +
> > +If you encounter a display that requires an additional EDID quirk in
> order to
> > +function properly, please report it to the direct rendering development
> mailing
> > +list <dri-devel@lists.freedesktop.org>.
> > +
> > +
> > +[1] See
> http://en.wikipedia.org/wiki/Extended_display_identification_data for a
> > +    description of the manufacturer ID and product code fields.
> > +[2] https://en.wikipedia.org/wiki/Bitwise_operation#Bit_shifts
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 9238de4..7fe39e0 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -276,6 +276,8 @@ static int __init drm_core_init(void)
> >               goto err_p3;
> >       }
> >
> > +     drm_edid_quirks_param_process();
> > +
> >       DRM_INFO("Initialized %s %d.%d.%d %s\n",
> >                CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL,
> CORE_DATE);
> >       return 0;
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index a8743c3..ea535f6 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -31,6 +31,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> > +#include <linux/ctype.h>
> > +
> >  #include "drmP.h"
> >  #include "drm_edid.h"
> >  #include "drm_edid_modes.h"
> > @@ -82,51 +84,457 @@ struct detailed_mode_closure {
> >  #define LEVEL_GTF2   2
> >  #define LEVEL_CVT    3
> >
> > -static struct edid_quirk {
> > -     char vendor[4];
> > -     int product_id;
> > -     u32 quirks;
> > -} edid_quirk_list[] = {
> > +union edid_quirk {
> > +     struct {
> > +             union edid_display_id display_id;
> > +             u32 quirks;
> > +     } __attribute__((packed)) s;
> > +     u64 u;
> > +};
>
> This does not need to be an union. Just make it a struct, and in the
> couple of places you need .u, you can do a memset and a struct
> assignment or memcpy.
>
> > +
> > +#define EDID_MFG_ID(c1, c2, c3)              cpu_to_be16(
>      \
> > +                                             (c1 & 0x1f) << 10 |     \
> > +                                             (c2 & 0x1f) << 5 |      \
> > +                                             (c3 & 0x1f)             \
> > +                                     )
> > +
> > +#define EDID_QUIRK_LIST_SIZE 24
> > +
> > +union edid_quirk edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
> > +
> >       /* Acer AL1706 */
> > -     { "ACR", 44358, EDID_QUIRK_PREFER_LARGE_60 },
> > +     { { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(44358) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>
> I wonder whether would be better to have this all in cpu byte order and
> code to handle it, or this confusing mixture of explicit big-endian,
> explicit little-endian, and cpu order. Someone, somewhere is bound to
> miss a byte order change later on...
>
> >       /* Acer F51 */
> > -     { "API", 0x7602, EDID_QUIRK_PREFER_LARGE_60 },
> > +     { { { { EDID_MFG_ID('A', 'P', 'I'), cpu_to_le16(0x7602) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> >       /* Unknown Acer */
> > -     { "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> > +     { { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(2423) } },
> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
> >
> >       /* Belinea 10 15 55 */
> > -     { "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
> > -     { "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
> > +     { { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(1516) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> > +     { { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(0x77e) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> >
> >       /* Envision Peripherals, Inc. EN-7100e */
> > -     { "EPI", 59264, EDID_QUIRK_135_CLOCK_TOO_HIGH },
> > +     { { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(59264) } },
> > +             EDID_QUIRK_135_CLOCK_TOO_HIGH } },
> >       /* Envision EN2028 */
> > -     { "EPI", 8232, EDID_QUIRK_PREFER_LARGE_60 },
> > +     { { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(8232) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> >
> >       /* Funai Electronics PM36B */
> > -     { "FCM", 13600, EDID_QUIRK_PREFER_LARGE_75 |
> > -       EDID_QUIRK_DETAILED_IN_CM },
> > +     { { { { EDID_MFG_ID('F', 'C', 'M'), cpu_to_le16(13600) } },
> > +             EDID_QUIRK_PREFER_LARGE_75 | EDID_QUIRK_DETAILED_IN_CM } },
> >
> >       /* LG Philips LCD LP154W01-A5 */
> > -     { "LPL", 0, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
> > -     { "LPL", 0x2a00, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
> > +     { { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0) } },
> > +             EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
> > +     { { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0x2a00) } },
> > +             EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
> >
> >       /* Philips 107p5 CRT */
> > -     { "PHL", 57364, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> > +     { { { { EDID_MFG_ID('P', 'H', 'L'), cpu_to_le16(57364) } },
> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
> >
> >       /* Proview AY765C */
> > -     { "PTS", 765, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
> > +     { { { { EDID_MFG_ID('P', 'T', 'S'), cpu_to_le16(765) } },
> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
> >
> >       /* Samsung SyncMaster 205BW.  Note: irony */
> > -     { "SAM", 541, EDID_QUIRK_DETAILED_SYNC_PP },
> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(541) } },
> > +             EDID_QUIRK_DETAILED_SYNC_PP } },
> >       /* Samsung SyncMaster 22[5-6]BW */
> > -     { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 },
> > -     { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 },
> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(596) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(638) } },
> > +             EDID_QUIRK_PREFER_LARGE_60 } },
> >
> >       /* ViewSonic VA2026w */
> > -     { "VSC", 5020, EDID_QUIRK_FORCE_REDUCED_BLANKING },
> > +     { { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
> > +             EDID_QUIRK_FORCE_REDUCED_BLANKING } },
> > +
> > +     /*
> > +      * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE
> to
> > +      * provide some room for user-supplied quirks.
> > +      */
> >  };
> >
> > +DEFINE_MUTEX(edid_quirk_list_mutex);
> > +
> > +/**
> > + * drm_edid_mfg_format - format an "encoded" EDID manufacturer ID for
> printing
> > + * @mfg_id: the encoded manufacturer ID
> > + * @buf: destination buffer for the formatted manufacturer ID (minimum
> 7 bytes)
> > + * @strip: if non-zero, the returned pointer will skip any leading
> spaces
> > + *
> > + * An EDID manufacturer ID is supposed to consist of 3 capital letters
> (A-Z).
> > + * Each letter is stored as a 5-bit value between 1 and 26, taking up
> 15 bits of
> > + * the 16-bit ID. The remaining bit should always be 0. If display
> manufacturers
> > + * always did things correctly, however, EDID quirks wouldn't be
> required in
> > + * the first place. This function does the following:
> > + *
> > + * - Broken IDs are printed in hexadecimal (0xffff).
> > + * - "Correct" IDs are formatted as a 3-letter ID string, preceded by 3
> spaces;
> > + *   the spaces ensure that both output formats are the same length.
> > + *
> > + * Thus, a formatted manufacturer ID is always 6 characters long (not
> including
> > + * the terminating 0).
> > + *
> > + * If @strip is 0, or the manufacturer ID has been formatted as a
> hexadecimal
> > + * number, @buf is returned.  If @strip is non-zero, and the
> manufacturer ID has
> > + * been formatted as a 3-letter string, a pointer to the first non-space
> > + * character (@buf + 3) is returned.
> > + */
> > +static const char *drm_edid_mfg_format(__be16 mfg_id, char *buf, int
> strip)
> > +{
> > +     u16 id = be16_to_cpu(mfg_id);
> > +
> > +     if (id & 0x8000)
> > +             goto bad_id;
> > +
> > +     buf[3] = ((id & 0x7c00) >> 10) + '@';
>
> Shift first and you can use the same mask for all three.
>
> > +     if (!isupper(buf[3]))
> > +             goto bad_id;
> > +
> > +     buf[4] = ((id & 0x03e0) >> 5) + '@';
> > +     if (!isupper(buf[4]))
> > +             goto bad_id;
> > +
> > +     buf[5] = (id & 0x001f) + '@';
> > +     if (!isupper(buf[5]))
> > +             goto bad_id;
> > +
> > +     memset(buf, ' ', 3);
> > +     buf[6] = 0;
> > +
> > +     return strip ? (buf + 3) : buf;
> > +
> > +bad_id:
> > +     sprintf(buf, "0x%04hx", id);
> > +     return buf;
> > +}
> > +
> > +#define EDID_MFG_BUF_SIZE            7
> > +
> > +/**
> > + * drm_edid_display_id_format - format an EDID "display ID"
> (manufacturer ID
> > + *                           and product code) for printing
> > + * @display_id: the display ID
> > + * @buf: destination buffer for the formatted display ID (minimum 14
> bytes)
> > + * @strip: if non-zero, the returned pointer will skip any leading
> spaces
> > + *
> > + * A formatted display ID is always 13 characters long (not including
> the
> > + * terminating 0).
> > + *
> > + * If @strip is 0, or the manufacturer ID has been formatted as a
> hexadecimal
> > + * number, @buf is returned.  If @strip is non-zero, and the
> manufacturer ID has
> > + * been formatted as a 3-letter string, a pointer to the first non-space
> > + * character (@buf + 3) is returned.
> > + */
> > +static const char *drm_edid_display_id_format(union edid_display_id
> display_id,
> > +                                           char *buf, int strip)
> > +{
> > +     const char *s;
> > +
> > +     s = drm_edid_mfg_format(display_id.s.mfg_id, buf, strip);
> > +     sprintf(buf + EDID_MFG_BUF_SIZE - 1, ":0x%04hx",
> > +             le16_to_cpu(display_id.s.prod_code));
> > +
> > +     return s;
> > +}
> > +
> > +#define EDID_DISPLAY_ID_BUF_SIZE     (EDID_MFG_BUF_SIZE + 7)
> > +
> > +/**
> > + * drm_edid_quirk_format - format an EDID quirk for printing
> > + * @quirk: the quirk
> > + * @buf: destination buffer for the formatted quirk (minimum 25 bytes)
> > + * @strip: if non-zero, the returned pointer will skip any leading
> spaces
> > + *
> > + * A formatted EDID quirk is always 24 characters long (not including
> the
> > + * terminating 0).
> > + *
> > + * If @strip is 0, or the manufacturer ID has been formatted as a
> hexadecimal
> > + * number, @buf is returned.  If @strip is non-zero, and the
> manufacturer ID has
> > + * been formatted as a 3-letter string, a pointer to the first non-space
> > + * character (@buf + 3) is returned.
> > + */
> > +static const char *drm_edid_quirk_format(const union edid_quirk *quirk,
> > +                                      char *buf, int strip)
> > +{
> > +     const char *s;
> > +
> > +     s = drm_edid_display_id_format(quirk->s.display_id, buf, strip);
> > +     sprintf(buf + EDID_DISPLAY_ID_BUF_SIZE - 1, ":0x%08x",
> quirk->s.quirks);
> > +
> > +     return s;
> > +}
> > +
> > +#define EDID_QUIRK_BUF_SIZE          (EDID_DISPLAY_ID_BUF_SIZE + 11)
> > +
> > +/**
> > + * drm_edid_quirk_parse - parse an EDID quirk
> > + * @s: string containing the quirk to be parsed
> > + * @quirk: destination for parsed quirk
> > + *
> > + * Returns 0 on success, < 0 (currently -EINVAL) on error.
> > + */
> > +static int drm_edid_quirk_parse(const char *s, union edid_quirk *quirk)
> > +{
> > +     char buf[EDID_QUIRK_BUF_SIZE];
> > +     s32 mfg;
> > +     s32 product;
> > +     s64 quirks;
> > +     char *c;
> > +
> > +     if (sscanf(s, "%i:%i:%lli", &mfg, &product, &quirks) == 3) {
> > +             if (mfg < 0 || mfg > 0xffff)
> > +                     goto error;
> > +             quirk->s.display_id.s.mfg_id = cpu_to_be16((u16)mfg);
> > +     } else {
> > +             if (sscanf(s, "%3s:%i:%lli", buf, &product, &quirks) != 3
> ||
> > +                             !isupper(buf[0]) ||
> > +                             !isupper(buf[1]) ||
> > +                             !isupper(buf[2]))
> > +                     goto error;
> > +             quirk->s.display_id.s.mfg_id =
> > +                             EDID_MFG_ID(buf[0], buf[1], buf[2]);
> > +     }
> > +
> > +     if (product < 0 || product > 0xffff ||
> > +                     quirks < 0 || quirks > 0xffffffffLL)
> > +             goto error;
> > +
> > +     quirk->s.display_id.s.prod_code = cpu_to_le16((u16)product);
> > +     quirk->s.quirks = (u32)quirks;
> > +
> > +     DRM_DEBUG("Successfully parsed EDID quirk: %s\n",
> > +               drm_edid_quirk_format(quirk, buf, 1));
> > +
> > +     return 0;
> > +
> > +error:
> > +     c = strpbrk(s, ",\n");
> > +     if (c == NULL) {
> > +             printk(KERN_WARNING "Invalid EDID quirk: '%s'\n", s);
> > +     } else {
> > +             printk(KERN_WARNING "Invalid EDID quirk: '%.*s'\n",
> > +                   (int)(c - s), s);
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +/**
> > + * drm_edid_quirk_find_by_id - find the EDID quirk matching a display ID
> > + * @display_id: the display ID to match
> > + *
> > + * Caller MUST hold edid_quirk_list_mutex.
> > + *
> > + * Returns a pointer to the matching quirk list entry, NULL if no such
> entry
> > + * exists.
> > + */
> > +static union edid_quirk *drm_edid_quirk_find_by_id(union
> edid_display_id id)
> > +{
> > +     union edid_quirk *q = edid_quirk_list;
> > +
> > +     do {
> > +             if (q->s.display_id.u == id.u && q->s.quirks != 0)
> > +                     return q;
> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
>
> The same with less cognitive burden on the reader:
>
>         for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
>                 ...
>         }
>
> Ditto elsewhere.
>
> > +
> > +     return NULL;
> > +}
> > +
> > +/**
> > + * drm_edid_quirk_find_slot - find an empty slot in the EDID quirk list
> > + *
> > + * Caller MUST hold edid_quirk_list_mutex.
> > + *
> > + * Returns a pointer to the first empty slot, NULL if no empty slots
> exist.
> > + */
> > +static union edid_quirk *drm_edid_quirk_find_empty(void)
> > +{
> > +     union edid_quirk *q = edid_quirk_list;
> > +
> > +     do {
> > +             if (q->s.quirks == 0)
> > +                     return q;
> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
> > +
> > +     return NULL;
> > +}
> > +
> > +/**
> > + * drm_edid_quirk_process - process a newly parsed EDID quirk
> > + * @quirk: the quirk to be processed
> > + *
> > + * Depending on the newly parsed quirk and the contents of the quirks
> list, this
> > + * function will add, remove, or replace a quirk.
> > + *
> > + * Returns 0 on success, < 0 on error (-ENOSPC if there is no free slot
> for a
> > + * new quirk). Note that trying to remove a quirk that isn't present is
> not
> > + * considered an error.
> > + */
> > +static int drm_edid_quirk_process(const union edid_quirk *quirk)
> > +{
> > +     char buf[EDID_QUIRK_BUF_SIZE];
> > +     union edid_quirk *q;
> > +     int res = 0;
> > +
> > +     mutex_lock(&edid_quirk_list_mutex);
> > +
> > +     if (quirk->s.quirks == 0) {
> > +             DRM_INFO("Removing EDID quirk for display %s\n",
> > +                      drm_edid_display_id_format(quirk->s.display_id,
> > +                                                 buf, 1));
> > +             q = drm_edid_quirk_find_by_id(quirk->s.display_id);
> > +             if (q == NULL) {
> > +                     printk(KERN_WARNING "No quirk found for display
> %s\n",
> > +
>  drm_edid_display_id_format(quirk->s.display_id,
> > +                                                       buf, 1));
> > +             } else {
> > +                     q->u = 0;
>
> Ditch the union and use memset.
>
> > +             }
> > +     } else {
> > +             DRM_INFO("Adding EDID quirk: %s\n",
> > +                      drm_edid_quirk_format(quirk, buf, 1));
> > +             q = drm_edid_quirk_find_by_id(quirk->s.display_id);
> > +             if (q == NULL) {
> > +                     q = drm_edid_quirk_find_empty();
> > +                     if (q == NULL) {
> > +                             printk(KERN_WARNING
> > +                                    "No free slot in EDID quirk
> list\n");
> > +                             res = -ENOSPC;
> > +                     } else {
> > +                             q->u = quirk->u;
>
> Ditch the union and use memcpy or struct assignment.
>
> > +                     }
> > +             } else {
> > +                     DRM_INFO("Replacing existing quirk: %s\n",
> > +                              drm_edid_quirk_format(q, buf, 1));
> > +                     q->s.quirks = quirk->s.quirks;
> > +             }
> > +     }
> > +
> > +     mutex_unlock(&edid_quirk_list_mutex);
> > +
> > +     return res;
> > +}
> > +
> > +/**
> > + * drm_edid_quirks_process - parse and process a comma separated list
> of EDID
> > + *                        quirks
> > + * @s: string containing the quirks to be processed
> > + * @strict: if non-zero, any parsing or processing error aborts further
> > + *       processing
> > + *
> > + * Returns 0 on success, < 0 if any error is encountered.  (If multiple
> errors
> > + * occur when strict is set to 0, the last error encountered is
> returned.)
> > + */
> > +static int drm_edid_quirks_process(const char *s, int strict)
> > +{
> > +     union edid_quirk quirk;
> > +     int res = 0;
> > +
> > +     do {
> > +
> > +             if (*s == '@') {
> > +                     DRM_INFO("Clearing EDID quirk list\n");
> > +                     mutex_lock(&edid_quirk_list_mutex);
> > +                     memset(edid_quirk_list, 0, sizeof edid_quirk_list);
> > +                     mutex_unlock(&edid_quirk_list_mutex);
> > +             } else {
> > +                     res = drm_edid_quirk_parse(s, &quirk);
> > +                     if (res != 0) {
> > +                             if (strict)
> > +                                     goto error;
> > +                             continue;
> > +                     }
> > +
> > +                     res = drm_edid_quirk_process(&quirk);
> > +                     if (res != 0) {
> > +                             if (strict)
> > +                                     goto error;
> > +                     }
> > +             }
> > +
> > +             s = strpbrk(s, ",\n");
> > +
> > +     } while (s != NULL && *(++s) != 0);
> > +
> > +     return res;
> > +
> > +error:
> > +     printk(KERN_WARNING "Aborting EDID quirk parsing\n");
> > +     return res;
> > +}
> > +
> > +/**
> > + * drm_edid_quirks_param_process - process the edid_quirks module
> parameter
> > + */
> > +void drm_edid_quirks_param_process(void)
> > +{
> > +     if (drm_edid_quirks != NULL)
> > +             drm_edid_quirks_process(drm_edid_quirks, 0);
> > +}
> > +
> > +/**
> > + * drm_edid_quirks_size_show - show the size of the EDID quirk list in
> sysfs
> > + * @buf: destination buffer (PAGE_SIZE bytes)
> > + */
> > +ssize_t drm_edid_quirks_size_show(struct class *class,
> > +                               struct class_attribute *attr, char *buf)
> > +{
> > +     return sprintf(buf, "%zu\n", ARRAY_SIZE(edid_quirk_list));
> > +}
> > +
> > +/**
> > + * drm_edid_quirks_show - show the contents of the EDID quirk list in
> sysfs
> > + * @buf: destination buffer (PAGE_SIZE bytes)
> > + */
> > +ssize_t drm_edid_quirks_show(struct class *class, struct
> class_attribute *attr,
> > +                          char *buf)
> > +{
> > +     const union edid_quirk *q = edid_quirk_list;
> > +     ssize_t count = 0;
> > +
> > +     BUILD_BUG_ON(ARRAY_SIZE(edid_quirk_list) >
> > +                             PAGE_SIZE / EDID_QUIRK_BUF_SIZE);
> > +
> > +     mutex_lock(&edid_quirk_list_mutex);
> > +
> > +     do {
> > +             if (q->s.quirks != 0) {
> > +                     drm_edid_quirk_format(q, buf + count, 0);
> > +                     (buf + count)[EDID_QUIRK_BUF_SIZE - 1] = '\n';
> > +                     count += EDID_QUIRK_BUF_SIZE;
> > +             }
> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
> > +
> > +     mutex_unlock(&edid_quirk_list_mutex);
> > +
> > +     return count;
> > +}
> > +
> > +/**
> > + * drm_edid_quirks_store - parse and process EDID qurik list changes
> written
> > + *                      to sysfs attribute
> > + */
> > +ssize_t drm_edid_quirks_store(struct class *class, struct
> class_attribute *attr,
> > +                           const char *buf, size_t count)
> > +{
> > +     int res;
> > +
> > +     res = drm_edid_quirks_process(buf, 1);
> > +     if (res != 0)
> > +             return res;
> > +
> > +     return count;
> > +}
> > +
> >  /*** DDC fetch and block validation ***/
> >
> >  static const u8 edid_header[] = {
> > @@ -409,25 +817,6 @@ EXPORT_SYMBOL(drm_get_edid);
> >  /*** EDID parsing ***/
> >
> >  /**
> > - * edid_vendor - match a string against EDID's obfuscated vendor field
> > - * @edid: EDID to match
> > - * @vendor: vendor string
> > - *
> > - * Returns true if @vendor is in @edid, false otherwise
> > - */
> > -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) + '@';
> > -
> > -     return !strncmp(edid_vendor, vendor, 3);
> > -}
> > -
> > -/**
> >   * edid_get_quirks - return quirk flags for a given EDID
> >   * @edid: EDID to process
> >   *
> > @@ -435,18 +824,18 @@ static bool edid_vendor(struct edid *edid, char
> *vendor)
> >   */
> >  static u32 edid_get_quirks(struct edid *edid)
> >  {
> > -     struct edid_quirk *quirk;
> > -     int i;
> > +     union edid_quirk *q;
> > +     u32 quirks = 0;
> >
> > -     for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
> > -             quirk = &edid_quirk_list[i];
> > +     mutex_lock(&edid_quirk_list_mutex);
> >
> > -             if (edid_vendor(edid, quirk->vendor) &&
> > -                 (EDID_PRODUCT_ID(edid) == quirk->product_id))
> > -                     return quirk->quirks;
> > -     }
> > +     q = drm_edid_quirk_find_by_id(edid->display_id);
> > +     if (q != NULL)
> > +             quirks = q->s.quirks;
> >
> > -     return 0;
> > +     mutex_unlock(&edid_quirk_list_mutex);
> > +
> > +     return quirks;
> >  }
> >
> >  #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
> > @@ -1162,7 +1551,7 @@ do_inferred_modes(struct detailed_timing *timing,
> void *c)
> >       closure->modes += drm_dmt_modes_for_range(closure->connector,
> >                                                 closure->edid,
> >                                                 timing);
> > -
> > +
> >       if (!version_greater(closure->edid, 1, 1))
> >               return; /* GTF not defined yet */
> >
> > @@ -1399,7 +1788,7 @@ do_cvt_mode(struct detailed_timing *timing, void
> *c)
> >
> >  static int
> >  add_cvt_modes(struct drm_connector *connector, struct edid *edid)
> > -{
> > +{
> >       struct detailed_mode_closure closure = {
> >               connector, edid, 0, 0, 0
> >       };
> > @@ -1615,15 +2004,12 @@ void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> >
> >       eld[0] = 2 << 3;                /* ELD version: 2 */
> >
> > -     eld[16] = edid->mfg_id[0];
> > -     eld[17] = edid->mfg_id[1];
> > -     eld[18] = edid->prod_code[0];
> > -     eld[19] = edid->prod_code[1];
> > +     *(u32 *)(&eld[16]) = edid->display_id.u;
> >
> >       if (cea[1] >= 3)
> >               for (db = cea + 4; db < cea + cea[2]; db += dbl + 1) {
> >                       dbl = db[0] & 0x1f;
> > -
> > +
> >                       switch ((db[0] & 0xe0) >> 5) {
> >                       case AUDIO_BLOCK:
> >                               /* Audio Data Block, contains SADs */
> > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> > index 21bcd4a..b939d51 100644
> > --- a/drivers/gpu/drm/drm_stub.c
> > +++ b/drivers/gpu/drm/drm_stub.c
> > @@ -46,16 +46,22 @@ EXPORT_SYMBOL(drm_vblank_offdelay);
> >  unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
> >  EXPORT_SYMBOL(drm_timestamp_precision);
> >
> > +char *drm_edid_quirks = NULL;
> > +EXPORT_SYMBOL(drm_edid_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, "MFG:prod:flags[,MFG:prod:flags[...]]\n"
> > +                           "(See Documentation/EDID/edid_quirks.txt)");
> >
> >  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_quirks, charp, 0400);
> >
> >  struct idr drm_minors_idr;
> >
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 45ac8d6..84dc365 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -84,6 +84,11 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
> >               __stringify(CORE_PATCHLEVEL) " "
> >               CORE_DATE);
> >
> > +static CLASS_ATTR(edid_quirks_size, 0400, drm_edid_quirks_size_show, 0);
> > +
> > +static CLASS_ATTR(edid_quirks, 0600, drm_edid_quirks_show,
> > +               drm_edid_quirks_store);
> > +
> >  /**
> >   * drm_sysfs_create - create a struct drm_sysfs_class structure
> >   * @owner: pointer to the module that is to "own" this struct
> drm_sysfs_class
> > @@ -113,10 +118,22 @@ struct class *drm_sysfs_create(struct module
> *owner, char *name)
> >       if (err)
> >               goto err_out_class;
> >
> > +     err = class_create_file(class, &class_attr_edid_quirks_size);
> > +     if (err)
> > +             goto err_out_version;
> > +
> > +     err = class_create_file(class, &class_attr_edid_quirks);
> > +     if (err)
> > +             goto err_out_quirks_size;
> > +
> >       class->devnode = drm_devnode;
> >
> >       return class;
> >
> > +err_out_quirks_size:
> > +     class_remove_file(class, &class_attr_edid_quirks_size);
> > +err_out_version:
> > +     class_remove_file(class, &class_attr_version.attr);
> >  err_out_class:
> >       class_destroy(class);
> >  err_out:
> > @@ -132,6 +149,8 @@ void drm_sysfs_destroy(void)
> >  {
> >       if ((drm_class == NULL) || (IS_ERR(drm_class)))
> >               return;
> > +     class_remove_file(drm_class, &class_attr_edid_quirks);
> > +     class_remove_file(drm_class, &class_attr_edid_quirks_size);
> >       class_remove_file(drm_class, &class_attr_version.attr);
> >       class_destroy(drm_class);
> >       drm_class = NULL;
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index d6b67bb..c947f3e 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1501,6 +1501,7 @@ extern unsigned int drm_debug;
> >
> >  extern unsigned int drm_vblank_offdelay;
> >  extern unsigned int drm_timestamp_precision;
> > +extern char *drm_edid_quirks;
> >
> >  extern struct class *drm_class;
> >  extern struct proc_dir_entry *drm_proc_root;
> > @@ -1612,6 +1613,15 @@ void drm_gem_vm_open(struct vm_area_struct *vma);
> >  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) */
> > +void drm_edid_quirks_param_process(void);
> > +ssize_t drm_edid_quirks_size_show(struct class *class,
> > +                               struct class_attribute *attr, char *buf);
> > +ssize_t drm_edid_quirks_show(struct class *class, struct
> class_attribute *attr,
> > +                          char *buf);
> > +ssize_t drm_edid_quirks_store(struct class *class, struct
> class_attribute *attr,
> > +                           const char *buf, size_t count);
> > +
> >  #include "drm_global.h"
> >
> >  static inline void
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 0cac551..713229b 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -202,11 +202,18 @@ struct detailed_timing {
> >  #define DRM_EDID_FEATURE_PM_SUSPEND       (1 << 6)
> >  #define DRM_EDID_FEATURE_PM_STANDBY       (1 << 7)
> >
> > +union edid_display_id {
> > +     struct {
> > +             __be16 mfg_id;
> > +             __le16 prod_code;
> > +     } __attribute__((packed)) s;
> > +     u32 u;
> > +};
>
> I think adding this union is counterproductive. The u32 version is
> helpful in one comparison and one assignment, while making all the rest
> just slightly more confusing.
>
> > +
> >  struct edid {
> >       u8 header[8];
> >       /* Vendor & product info */
> > -     u8 mfg_id[2];
> > -     u8 prod_code[2];
> > +     union edid_display_id display_id;
> >       u32 serial; /* FIXME: byte order */
> >       u8 mfg_week;
> >       u8 mfg_year;
> > @@ -242,8 +249,6 @@ struct edid {
> >       u8 checksum;
> >  } __attribute__((packed));
> >
> > -#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] <<
> 8))
> > -
> >  struct drm_encoder;
> >  struct drm_connector;
> >  struct drm_display_mode;
> > --
> > 1.7.11.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 44161 bytes --]

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

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

Re: [PATCH v5 1/3] drm: Add user-defined EDID quirks capability

[Jani Nikula]

On Tue, 09 Oct 2012, Ian Pilcher <arequipeno@gmail.com> wrote:
> OK. I'm done.

Sincerely, if you want to get your stuff in the kernel, you need to not
give up so easily.

> I've literally been discussing these patches on this list for months, and
> you bring this up now?

Apologies, I have only been reading the list for a few months, and this
is the first time I look at the patches. I don't think a patch should
get any special treatment for reaching v5.

> It's far easier to simply recompile every kernel that comes out than to
> continue this dance.

Please note that I don't make the calls about pushing the patches. I can
merely offer my review comments and opinions to hopefully make those
decisions easier for others. Patches that attract review probably have a
better chance of getting pushed than patches that nobody cares about.


BR,
Jani.


> On Oct 9, 2012 7:10 AM, "Jani Nikula" <jani.nikula@linux.intel.com> wrote:
>
>> On Fri, 28 Sep 2012, Ian Pilcher <arequipeno@gmail.com> wrote:
>> > Add the ability for users to define their own EDID quirks via a
>> > module parameter or sysfs attribute.
>>
>> Hi Ian -
>>
>> IMHO this patch should be chopped up to smaller pieces. For example,
>> change the edid_quirk_list format first (if you must), then add module
>> parameter support, then add sysfs support, in separate patches. It'll be
>> easier to review.
>>
>> Please see some other comments inline.
>>
>> BR,
>> Jani.
>>
>> >
>> > Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
>> > ---
>> >  Documentation/EDID/edid_quirks.txt | 126 ++++++++++
>> >  drivers/gpu/drm/drm_drv.c          |   2 +
>> >  drivers/gpu/drm/drm_edid.c         | 500
>> ++++++++++++++++++++++++++++++++-----
>> >  drivers/gpu/drm/drm_stub.c         |   6 +
>> >  drivers/gpu/drm/drm_sysfs.c        |  19 ++
>> >  include/drm/drmP.h                 |  10 +
>> >  include/drm/drm_edid.h             |  13 +-
>> >  7 files changed, 615 insertions(+), 61 deletions(-)
>> >  create mode 100644 Documentation/EDID/edid_quirks.txt
>> >
>> > diff --git a/Documentation/EDID/edid_quirks.txt
>> b/Documentation/EDID/edid_quirks.txt
>> > new file mode 100644
>> > index 0000000..0c9c746
>> > --- /dev/null
>> > +++ b/Documentation/EDID/edid_quirks.txt
>> > @@ -0,0 +1,126 @@
>> > +                                  EDID Quirks
>> > +                                 =============
>> > +                       Ian Pilcher <arequipeno@gmail.com>
>> > +                                August 11, 2012
>> > +
>> > +
>> > +    "EDID blocks out in the wild have a variety of bugs"
>> > +        -- from drivers/gpu/drm/drm_edid.c
>> > +
>> > +
>> > +Overview
>> > +========
>> > +
>> > +EDID quirks provide a mechanism for working around display hardware
>> with buggy
>> > +EDID data.
>> > +
>> > +An individual EDID quirk maps a display type (identified by its EDID
>> > +manufacturer ID and product code[1]) to a set of "quirk flags."  The
>> kernel
>> > +includes a variety of built-in quirks.  (They are stored in the
>> edid_quirk_list
>> > +array in drivers/gpu/drm/drm_edid.c.)
>> > +
>> > +An example of a built-in EDID quirk is:
>> > +
>> > +    ACR:0xad46:0x00000001
>> > +
>> > +The first field is the manufacturer ID (Acer, Inc.), the second field
>> is the
>> > +manufacturer's product code, and the third field contains the quirk
>> flags for
>> > +that display type.
>> > +
>> > +The quirk flags are defined in drivers/gpu/drm/drm_edid.c.  Each flag
>> has a
>> > +symbolic name beginning with EDID_QUIRK_, along with a numerical value.
>>  Each
>> > +flag should also have an associated comment which provides an idea of
>> its
>> > +effect.  Note that the values in the source file are expressed as bit
>> shifts[2]:
>> > +
>> > +    * 1 << 0: 0x0001
>> > +    * 1 << 1: 0x0002
>> > +    * 1 << 2: 0x0004
>> > +    * etc.
>> > +
>> > +
>> > +sysfs interface
>> > +===============
>> > +
>> > +The current EDID quirk list can be read from /sys/class/drm/edid_quirks:
>> > +
>> > +    # cat /sys/class/drm/edid_quirks
>> > +       ACR:0xad46:0x00000001
>> > +       API:0x7602:0x00000001
>> > +       ACR:0x0977:0x00000020
>> > +    0x9e6a:0x077e:0x00000080
>> > +    ...
>> > +
>> > +("Nonconformant" manufacturer IDs are displayed as hexadecimal values.)
>> > +
>> > +The number of total "slots" in the list can be read from
>> > +/sys/class/drm/edid_quirks_size.  This total includes both occupied
>> slots (i.e.
>> > +the current list) and any slots available for additional quirks.  The
>> number of
>> > +available slots can be calculated by subtracting the number of quirks
>> in the
>> > +current list from the total number of slots.
>> > +
>> > +If a slot is available, an additional quirk can be added to the list by
>> writing
>> > +it to /sys/class/drm/edid_quirks:
>> > +
>> > +    # echo FOO:0xffff:0x100 > /sys/class/drm/edid_quirks
>> > +
>> > +Manufacturer IDs can also be specified numerically.  (This is the only
>> way to
>> > +specify a nonconformant ID.) This command is equivalent to the previous
>> one:
>> > +
>> > +    # echo 0x19ef:0xffff:0x100 > /sys/class/drm/edid_quirks
>> > +
>> > +Numeric values can also be specified in decimal or octal formats; a
>> number that
>> > +begins with a 0 is assumed to be octal:
>> > +
>> > +    # echo FOO:65535:0400 > /sys/class/drm/edid_quirks
>> > +
>> > +An existing quirk can be replaced by writing a new set of flags:
>> > +
>> > +    # echo FOO:0xffff:0x200 > /sys/class/drm/edid_quirks
>> > +
>> > +A quirk can be deleted from the list by writing an empty flag set (0).
>> This
>> > +makes the slot occupied by that quirk available.
>> > +
>> > +    # echo FOO:0xffff:0 > /sys/class/drm/edid_quirks
>> > +
>> > +Writing an "at symbol" (@) clears the entire quirk list:
>> > +
>> > +    # echo @ > /sys/class/drm/edid_quirks
>> > +
>> > +Multiple changes to the list can be specified in a comma (or newline)
>> separated
>> > +list. For example, the following command clears all of the existing
>> quirks in
>> > +the list and adds 3 new quirks:
>> > +
>> > +    # echo @,FOO:0xffff:0x100,BAR:0x1111:0x001,BAZ:0x2222:0x002 > \
>> > +            /sys/class/drm/edid_quirks
>> > +
>> > +Note however, that any error (an incorrectly formatted quirk or an
>> attempt to
>> > +add a quirk when no slot is available) will abort processing of any
>> further
>> > +changes, potentially making it difficult to determine exactly which
>> change
>> > +caused the error and what changes were made.  For this reason, making
>> changes
>> > +one at a time is recommended, particularly if the changes are being
>> made by a
>> > +script or program.
>>
>> Generally it seems like a bad idea to add support for something you
>> specifically recommend against using. It should be a hint not to add
>> it. It looks like you support multiple changes in sysfs only because it
>> comes free with the module parameter support.
>>
>> > +
>> > +
>> > +Module parameter
>> > +================
>> > +
>> > +The EDID quirk list can also be modified via the edid_quirks module
>> parameter
>> > +(drm.edid_quirks on the kernel command line).  The effect of setting
>> this
>> > +parameter is identical to the effect of writing its value to
>> > +/sys/class/drm/edid_quirks, with one important difference.  When an
>> error is
>> > +encountered during module parameter parsing or processing, any
>> remaining quirks
>> > +in the parameter string will still be processed.  (It is hoped that
>> this approach
>> > +maximizes the probability of producing a working display.)
>> > +
>> > +
>> > +Follow-up
>> > +=========
>> > +
>> > +If you encounter a display that requires an additional EDID quirk in
>> order to
>> > +function properly, please report it to the direct rendering development
>> mailing
>> > +list <dri-devel@lists.freedesktop.org>.
>> > +
>> > +
>> > +[1] See
>> http://en.wikipedia.org/wiki/Extended_display_identification_data for a
>> > +    description of the manufacturer ID and product code fields.
>> > +[2] https://en.wikipedia.org/wiki/Bitwise_operation#Bit_shifts
>> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> > index 9238de4..7fe39e0 100644
>> > --- a/drivers/gpu/drm/drm_drv.c
>> > +++ b/drivers/gpu/drm/drm_drv.c
>> > @@ -276,6 +276,8 @@ static int __init drm_core_init(void)
>> >               goto err_p3;
>> >       }
>> >
>> > +     drm_edid_quirks_param_process();
>> > +
>> >       DRM_INFO("Initialized %s %d.%d.%d %s\n",
>> >                CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL,
>> CORE_DATE);
>> >       return 0;
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index a8743c3..ea535f6 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -31,6 +31,8 @@
>> >  #include <linux/slab.h>
>> >  #include <linux/i2c.h>
>> >  #include <linux/module.h>
>> > +#include <linux/ctype.h>
>> > +
>> >  #include "drmP.h"
>> >  #include "drm_edid.h"
>> >  #include "drm_edid_modes.h"
>> > @@ -82,51 +84,457 @@ struct detailed_mode_closure {
>> >  #define LEVEL_GTF2   2
>> >  #define LEVEL_CVT    3
>> >
>> > -static struct edid_quirk {
>> > -     char vendor[4];
>> > -     int product_id;
>> > -     u32 quirks;
>> > -} edid_quirk_list[] = {
>> > +union edid_quirk {
>> > +     struct {
>> > +             union edid_display_id display_id;
>> > +             u32 quirks;
>> > +     } __attribute__((packed)) s;
>> > +     u64 u;
>> > +};
>>
>> This does not need to be an union. Just make it a struct, and in the
>> couple of places you need .u, you can do a memset and a struct
>> assignment or memcpy.
>>
>> > +
>> > +#define EDID_MFG_ID(c1, c2, c3)              cpu_to_be16(
>>      \
>> > +                                             (c1 & 0x1f) << 10 |     \
>> > +                                             (c2 & 0x1f) << 5 |      \
>> > +                                             (c3 & 0x1f)             \
>> > +                                     )
>> > +
>> > +#define EDID_QUIRK_LIST_SIZE 24
>> > +
>> > +union edid_quirk edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
>> > +
>> >       /* Acer AL1706 */
>> > -     { "ACR", 44358, EDID_QUIRK_PREFER_LARGE_60 },
>> > +     { { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(44358) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>>
>> I wonder whether would be better to have this all in cpu byte order and
>> code to handle it, or this confusing mixture of explicit big-endian,
>> explicit little-endian, and cpu order. Someone, somewhere is bound to
>> miss a byte order change later on...
>>
>> >       /* Acer F51 */
>> > -     { "API", 0x7602, EDID_QUIRK_PREFER_LARGE_60 },
>> > +     { { { { EDID_MFG_ID('A', 'P', 'I'), cpu_to_le16(0x7602) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> >       /* Unknown Acer */
>> > -     { "ACR", 2423, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
>> > +     { { { { EDID_MFG_ID('A', 'C', 'R'), cpu_to_le16(2423) } },
>> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>> >
>> >       /* Belinea 10 15 55 */
>> > -     { "MAX", 1516, EDID_QUIRK_PREFER_LARGE_60 },
>> > -     { "MAX", 0x77e, EDID_QUIRK_PREFER_LARGE_60 },
>> > +     { { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(1516) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> > +     { { { { EDID_MFG_ID('M', 'A', 'X'), cpu_to_le16(0x77e) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> >
>> >       /* Envision Peripherals, Inc. EN-7100e */
>> > -     { "EPI", 59264, EDID_QUIRK_135_CLOCK_TOO_HIGH },
>> > +     { { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(59264) } },
>> > +             EDID_QUIRK_135_CLOCK_TOO_HIGH } },
>> >       /* Envision EN2028 */
>> > -     { "EPI", 8232, EDID_QUIRK_PREFER_LARGE_60 },
>> > +     { { { { EDID_MFG_ID('E', 'P', 'I'), cpu_to_le16(8232) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> >
>> >       /* Funai Electronics PM36B */
>> > -     { "FCM", 13600, EDID_QUIRK_PREFER_LARGE_75 |
>> > -       EDID_QUIRK_DETAILED_IN_CM },
>> > +     { { { { EDID_MFG_ID('F', 'C', 'M'), cpu_to_le16(13600) } },
>> > +             EDID_QUIRK_PREFER_LARGE_75 | EDID_QUIRK_DETAILED_IN_CM } },
>> >
>> >       /* LG Philips LCD LP154W01-A5 */
>> > -     { "LPL", 0, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
>> > -     { "LPL", 0x2a00, EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE },
>> > +     { { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0) } },
>> > +             EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
>> > +     { { { { EDID_MFG_ID('L', 'P', 'L'), cpu_to_le16(0x2a00) } },
>> > +             EDID_QUIRK_DETAILED_USE_MAXIMUM_SIZE } },
>> >
>> >       /* Philips 107p5 CRT */
>> > -     { "PHL", 57364, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
>> > +     { { { { EDID_MFG_ID('P', 'H', 'L'), cpu_to_le16(57364) } },
>> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>> >
>> >       /* Proview AY765C */
>> > -     { "PTS", 765, EDID_QUIRK_FIRST_DETAILED_PREFERRED },
>> > +     { { { { EDID_MFG_ID('P', 'T', 'S'), cpu_to_le16(765) } },
>> > +             EDID_QUIRK_FIRST_DETAILED_PREFERRED } },
>> >
>> >       /* Samsung SyncMaster 205BW.  Note: irony */
>> > -     { "SAM", 541, EDID_QUIRK_DETAILED_SYNC_PP },
>> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(541) } },
>> > +             EDID_QUIRK_DETAILED_SYNC_PP } },
>> >       /* Samsung SyncMaster 22[5-6]BW */
>> > -     { "SAM", 596, EDID_QUIRK_PREFER_LARGE_60 },
>> > -     { "SAM", 638, EDID_QUIRK_PREFER_LARGE_60 },
>> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(596) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> > +     { { { { EDID_MFG_ID('S', 'A', 'M'), cpu_to_le16(638) } },
>> > +             EDID_QUIRK_PREFER_LARGE_60 } },
>> >
>> >       /* ViewSonic VA2026w */
>> > -     { "VSC", 5020, EDID_QUIRK_FORCE_REDUCED_BLANKING },
>> > +     { { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
>> > +             EDID_QUIRK_FORCE_REDUCED_BLANKING } },
>> > +
>> > +     /*
>> > +      * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE
>> to
>> > +      * provide some room for user-supplied quirks.
>> > +      */
>> >  };
>> >
>> > +DEFINE_MUTEX(edid_quirk_list_mutex);
>> > +
>> > +/**
>> > + * drm_edid_mfg_format - format an "encoded" EDID manufacturer ID for
>> printing
>> > + * @mfg_id: the encoded manufacturer ID
>> > + * @buf: destination buffer for the formatted manufacturer ID (minimum
>> 7 bytes)
>> > + * @strip: if non-zero, the returned pointer will skip any leading
>> spaces
>> > + *
>> > + * An EDID manufacturer ID is supposed to consist of 3 capital letters
>> (A-Z).
>> > + * Each letter is stored as a 5-bit value between 1 and 26, taking up
>> 15 bits of
>> > + * the 16-bit ID. The remaining bit should always be 0. If display
>> manufacturers
>> > + * always did things correctly, however, EDID quirks wouldn't be
>> required in
>> > + * the first place. This function does the following:
>> > + *
>> > + * - Broken IDs are printed in hexadecimal (0xffff).
>> > + * - "Correct" IDs are formatted as a 3-letter ID string, preceded by 3
>> spaces;
>> > + *   the spaces ensure that both output formats are the same length.
>> > + *
>> > + * Thus, a formatted manufacturer ID is always 6 characters long (not
>> including
>> > + * the terminating 0).
>> > + *
>> > + * If @strip is 0, or the manufacturer ID has been formatted as a
>> hexadecimal
>> > + * number, @buf is returned.  If @strip is non-zero, and the
>> manufacturer ID has
>> > + * been formatted as a 3-letter string, a pointer to the first non-space
>> > + * character (@buf + 3) is returned.
>> > + */
>> > +static const char *drm_edid_mfg_format(__be16 mfg_id, char *buf, int
>> strip)
>> > +{
>> > +     u16 id = be16_to_cpu(mfg_id);
>> > +
>> > +     if (id & 0x8000)
>> > +             goto bad_id;
>> > +
>> > +     buf[3] = ((id & 0x7c00) >> 10) + '@';
>>
>> Shift first and you can use the same mask for all three.
>>
>> > +     if (!isupper(buf[3]))
>> > +             goto bad_id;
>> > +
>> > +     buf[4] = ((id & 0x03e0) >> 5) + '@';
>> > +     if (!isupper(buf[4]))
>> > +             goto bad_id;
>> > +
>> > +     buf[5] = (id & 0x001f) + '@';
>> > +     if (!isupper(buf[5]))
>> > +             goto bad_id;
>> > +
>> > +     memset(buf, ' ', 3);
>> > +     buf[6] = 0;
>> > +
>> > +     return strip ? (buf + 3) : buf;
>> > +
>> > +bad_id:
>> > +     sprintf(buf, "0x%04hx", id);
>> > +     return buf;
>> > +}
>> > +
>> > +#define EDID_MFG_BUF_SIZE            7
>> > +
>> > +/**
>> > + * drm_edid_display_id_format - format an EDID "display ID"
>> (manufacturer ID
>> > + *                           and product code) for printing
>> > + * @display_id: the display ID
>> > + * @buf: destination buffer for the formatted display ID (minimum 14
>> bytes)
>> > + * @strip: if non-zero, the returned pointer will skip any leading
>> spaces
>> > + *
>> > + * A formatted display ID is always 13 characters long (not including
>> the
>> > + * terminating 0).
>> > + *
>> > + * If @strip is 0, or the manufacturer ID has been formatted as a
>> hexadecimal
>> > + * number, @buf is returned.  If @strip is non-zero, and the
>> manufacturer ID has
>> > + * been formatted as a 3-letter string, a pointer to the first non-space
>> > + * character (@buf + 3) is returned.
>> > + */
>> > +static const char *drm_edid_display_id_format(union edid_display_id
>> display_id,
>> > +                                           char *buf, int strip)
>> > +{
>> > +     const char *s;
>> > +
>> > +     s = drm_edid_mfg_format(display_id.s.mfg_id, buf, strip);
>> > +     sprintf(buf + EDID_MFG_BUF_SIZE - 1, ":0x%04hx",
>> > +             le16_to_cpu(display_id.s.prod_code));
>> > +
>> > +     return s;
>> > +}
>> > +
>> > +#define EDID_DISPLAY_ID_BUF_SIZE     (EDID_MFG_BUF_SIZE + 7)
>> > +
>> > +/**
>> > + * drm_edid_quirk_format - format an EDID quirk for printing
>> > + * @quirk: the quirk
>> > + * @buf: destination buffer for the formatted quirk (minimum 25 bytes)
>> > + * @strip: if non-zero, the returned pointer will skip any leading
>> spaces
>> > + *
>> > + * A formatted EDID quirk is always 24 characters long (not including
>> the
>> > + * terminating 0).
>> > + *
>> > + * If @strip is 0, or the manufacturer ID has been formatted as a
>> hexadecimal
>> > + * number, @buf is returned.  If @strip is non-zero, and the
>> manufacturer ID has
>> > + * been formatted as a 3-letter string, a pointer to the first non-space
>> > + * character (@buf + 3) is returned.
>> > + */
>> > +static const char *drm_edid_quirk_format(const union edid_quirk *quirk,
>> > +                                      char *buf, int strip)
>> > +{
>> > +     const char *s;
>> > +
>> > +     s = drm_edid_display_id_format(quirk->s.display_id, buf, strip);
>> > +     sprintf(buf + EDID_DISPLAY_ID_BUF_SIZE - 1, ":0x%08x",
>> quirk->s.quirks);
>> > +
>> > +     return s;
>> > +}
>> > +
>> > +#define EDID_QUIRK_BUF_SIZE          (EDID_DISPLAY_ID_BUF_SIZE + 11)
>> > +
>> > +/**
>> > + * drm_edid_quirk_parse - parse an EDID quirk
>> > + * @s: string containing the quirk to be parsed
>> > + * @quirk: destination for parsed quirk
>> > + *
>> > + * Returns 0 on success, < 0 (currently -EINVAL) on error.
>> > + */
>> > +static int drm_edid_quirk_parse(const char *s, union edid_quirk *quirk)
>> > +{
>> > +     char buf[EDID_QUIRK_BUF_SIZE];
>> > +     s32 mfg;
>> > +     s32 product;
>> > +     s64 quirks;
>> > +     char *c;
>> > +
>> > +     if (sscanf(s, "%i:%i:%lli", &mfg, &product, &quirks) == 3) {
>> > +             if (mfg < 0 || mfg > 0xffff)
>> > +                     goto error;
>> > +             quirk->s.display_id.s.mfg_id = cpu_to_be16((u16)mfg);
>> > +     } else {
>> > +             if (sscanf(s, "%3s:%i:%lli", buf, &product, &quirks) != 3
>> ||
>> > +                             !isupper(buf[0]) ||
>> > +                             !isupper(buf[1]) ||
>> > +                             !isupper(buf[2]))
>> > +                     goto error;
>> > +             quirk->s.display_id.s.mfg_id =
>> > +                             EDID_MFG_ID(buf[0], buf[1], buf[2]);
>> > +     }
>> > +
>> > +     if (product < 0 || product > 0xffff ||
>> > +                     quirks < 0 || quirks > 0xffffffffLL)
>> > +             goto error;
>> > +
>> > +     quirk->s.display_id.s.prod_code = cpu_to_le16((u16)product);
>> > +     quirk->s.quirks = (u32)quirks;
>> > +
>> > +     DRM_DEBUG("Successfully parsed EDID quirk: %s\n",
>> > +               drm_edid_quirk_format(quirk, buf, 1));
>> > +
>> > +     return 0;
>> > +
>> > +error:
>> > +     c = strpbrk(s, ",\n");
>> > +     if (c == NULL) {
>> > +             printk(KERN_WARNING "Invalid EDID quirk: '%s'\n", s);
>> > +     } else {
>> > +             printk(KERN_WARNING "Invalid EDID quirk: '%.*s'\n",
>> > +                   (int)(c - s), s);
>> > +     }
>> > +
>> > +     return -EINVAL;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirk_find_by_id - find the EDID quirk matching a display ID
>> > + * @display_id: the display ID to match
>> > + *
>> > + * Caller MUST hold edid_quirk_list_mutex.
>> > + *
>> > + * Returns a pointer to the matching quirk list entry, NULL if no such
>> entry
>> > + * exists.
>> > + */
>> > +static union edid_quirk *drm_edid_quirk_find_by_id(union
>> edid_display_id id)
>> > +{
>> > +     union edid_quirk *q = edid_quirk_list;
>> > +
>> > +     do {
>> > +             if (q->s.display_id.u == id.u && q->s.quirks != 0)
>> > +                     return q;
>> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
>>
>> The same with less cognitive burden on the reader:
>>
>>         for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
>>                 ...
>>         }
>>
>> Ditto elsewhere.
>>
>> > +
>> > +     return NULL;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirk_find_slot - find an empty slot in the EDID quirk list
>> > + *
>> > + * Caller MUST hold edid_quirk_list_mutex.
>> > + *
>> > + * Returns a pointer to the first empty slot, NULL if no empty slots
>> exist.
>> > + */
>> > +static union edid_quirk *drm_edid_quirk_find_empty(void)
>> > +{
>> > +     union edid_quirk *q = edid_quirk_list;
>> > +
>> > +     do {
>> > +             if (q->s.quirks == 0)
>> > +                     return q;
>> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
>> > +
>> > +     return NULL;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirk_process - process a newly parsed EDID quirk
>> > + * @quirk: the quirk to be processed
>> > + *
>> > + * Depending on the newly parsed quirk and the contents of the quirks
>> list, this
>> > + * function will add, remove, or replace a quirk.
>> > + *
>> > + * Returns 0 on success, < 0 on error (-ENOSPC if there is no free slot
>> for a
>> > + * new quirk). Note that trying to remove a quirk that isn't present is
>> not
>> > + * considered an error.
>> > + */
>> > +static int drm_edid_quirk_process(const union edid_quirk *quirk)
>> > +{
>> > +     char buf[EDID_QUIRK_BUF_SIZE];
>> > +     union edid_quirk *q;
>> > +     int res = 0;
>> > +
>> > +     mutex_lock(&edid_quirk_list_mutex);
>> > +
>> > +     if (quirk->s.quirks == 0) {
>> > +             DRM_INFO("Removing EDID quirk for display %s\n",
>> > +                      drm_edid_display_id_format(quirk->s.display_id,
>> > +                                                 buf, 1));
>> > +             q = drm_edid_quirk_find_by_id(quirk->s.display_id);
>> > +             if (q == NULL) {
>> > +                     printk(KERN_WARNING "No quirk found for display
>> %s\n",
>> > +
>>  drm_edid_display_id_format(quirk->s.display_id,
>> > +                                                       buf, 1));
>> > +             } else {
>> > +                     q->u = 0;
>>
>> Ditch the union and use memset.
>>
>> > +             }
>> > +     } else {
>> > +             DRM_INFO("Adding EDID quirk: %s\n",
>> > +                      drm_edid_quirk_format(quirk, buf, 1));
>> > +             q = drm_edid_quirk_find_by_id(quirk->s.display_id);
>> > +             if (q == NULL) {
>> > +                     q = drm_edid_quirk_find_empty();
>> > +                     if (q == NULL) {
>> > +                             printk(KERN_WARNING
>> > +                                    "No free slot in EDID quirk
>> list\n");
>> > +                             res = -ENOSPC;
>> > +                     } else {
>> > +                             q->u = quirk->u;
>>
>> Ditch the union and use memcpy or struct assignment.
>>
>> > +                     }
>> > +             } else {
>> > +                     DRM_INFO("Replacing existing quirk: %s\n",
>> > +                              drm_edid_quirk_format(q, buf, 1));
>> > +                     q->s.quirks = quirk->s.quirks;
>> > +             }
>> > +     }
>> > +
>> > +     mutex_unlock(&edid_quirk_list_mutex);
>> > +
>> > +     return res;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirks_process - parse and process a comma separated list
>> of EDID
>> > + *                        quirks
>> > + * @s: string containing the quirks to be processed
>> > + * @strict: if non-zero, any parsing or processing error aborts further
>> > + *       processing
>> > + *
>> > + * Returns 0 on success, < 0 if any error is encountered.  (If multiple
>> errors
>> > + * occur when strict is set to 0, the last error encountered is
>> returned.)
>> > + */
>> > +static int drm_edid_quirks_process(const char *s, int strict)
>> > +{
>> > +     union edid_quirk quirk;
>> > +     int res = 0;
>> > +
>> > +     do {
>> > +
>> > +             if (*s == '@') {
>> > +                     DRM_INFO("Clearing EDID quirk list\n");
>> > +                     mutex_lock(&edid_quirk_list_mutex);
>> > +                     memset(edid_quirk_list, 0, sizeof edid_quirk_list);
>> > +                     mutex_unlock(&edid_quirk_list_mutex);
>> > +             } else {
>> > +                     res = drm_edid_quirk_parse(s, &quirk);
>> > +                     if (res != 0) {
>> > +                             if (strict)
>> > +                                     goto error;
>> > +                             continue;
>> > +                     }
>> > +
>> > +                     res = drm_edid_quirk_process(&quirk);
>> > +                     if (res != 0) {
>> > +                             if (strict)
>> > +                                     goto error;
>> > +                     }
>> > +             }
>> > +
>> > +             s = strpbrk(s, ",\n");
>> > +
>> > +     } while (s != NULL && *(++s) != 0);
>> > +
>> > +     return res;
>> > +
>> > +error:
>> > +     printk(KERN_WARNING "Aborting EDID quirk parsing\n");
>> > +     return res;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirks_param_process - process the edid_quirks module
>> parameter
>> > + */
>> > +void drm_edid_quirks_param_process(void)
>> > +{
>> > +     if (drm_edid_quirks != NULL)
>> > +             drm_edid_quirks_process(drm_edid_quirks, 0);
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirks_size_show - show the size of the EDID quirk list in
>> sysfs
>> > + * @buf: destination buffer (PAGE_SIZE bytes)
>> > + */
>> > +ssize_t drm_edid_quirks_size_show(struct class *class,
>> > +                               struct class_attribute *attr, char *buf)
>> > +{
>> > +     return sprintf(buf, "%zu\n", ARRAY_SIZE(edid_quirk_list));
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirks_show - show the contents of the EDID quirk list in
>> sysfs
>> > + * @buf: destination buffer (PAGE_SIZE bytes)
>> > + */
>> > +ssize_t drm_edid_quirks_show(struct class *class, struct
>> class_attribute *attr,
>> > +                          char *buf)
>> > +{
>> > +     const union edid_quirk *q = edid_quirk_list;
>> > +     ssize_t count = 0;
>> > +
>> > +     BUILD_BUG_ON(ARRAY_SIZE(edid_quirk_list) >
>> > +                             PAGE_SIZE / EDID_QUIRK_BUF_SIZE);
>> > +
>> > +     mutex_lock(&edid_quirk_list_mutex);
>> > +
>> > +     do {
>> > +             if (q->s.quirks != 0) {
>> > +                     drm_edid_quirk_format(q, buf + count, 0);
>> > +                     (buf + count)[EDID_QUIRK_BUF_SIZE - 1] = '\n';
>> > +                     count += EDID_QUIRK_BUF_SIZE;
>> > +             }
>> > +     } while (++q < edid_quirk_list + ARRAY_SIZE(edid_quirk_list));
>> > +
>> > +     mutex_unlock(&edid_quirk_list_mutex);
>> > +
>> > +     return count;
>> > +}
>> > +
>> > +/**
>> > + * drm_edid_quirks_store - parse and process EDID qurik list changes
>> written
>> > + *                      to sysfs attribute
>> > + */
>> > +ssize_t drm_edid_quirks_store(struct class *class, struct
>> class_attribute *attr,
>> > +                           const char *buf, size_t count)
>> > +{
>> > +     int res;
>> > +
>> > +     res = drm_edid_quirks_process(buf, 1);
>> > +     if (res != 0)
>> > +             return res;
>> > +
>> > +     return count;
>> > +}
>> > +
>> >  /*** DDC fetch and block validation ***/
>> >
>> >  static const u8 edid_header[] = {
>> > @@ -409,25 +817,6 @@ EXPORT_SYMBOL(drm_get_edid);
>> >  /*** EDID parsing ***/
>> >
>> >  /**
>> > - * edid_vendor - match a string against EDID's obfuscated vendor field
>> > - * @edid: EDID to match
>> > - * @vendor: vendor string
>> > - *
>> > - * Returns true if @vendor is in @edid, false otherwise
>> > - */
>> > -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) + '@';
>> > -
>> > -     return !strncmp(edid_vendor, vendor, 3);
>> > -}
>> > -
>> > -/**
>> >   * edid_get_quirks - return quirk flags for a given EDID
>> >   * @edid: EDID to process
>> >   *
>> > @@ -435,18 +824,18 @@ static bool edid_vendor(struct edid *edid, char
>> *vendor)
>> >   */
>> >  static u32 edid_get_quirks(struct edid *edid)
>> >  {
>> > -     struct edid_quirk *quirk;
>> > -     int i;
>> > +     union edid_quirk *q;
>> > +     u32 quirks = 0;
>> >
>> > -     for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
>> > -             quirk = &edid_quirk_list[i];
>> > +     mutex_lock(&edid_quirk_list_mutex);
>> >
>> > -             if (edid_vendor(edid, quirk->vendor) &&
>> > -                 (EDID_PRODUCT_ID(edid) == quirk->product_id))
>> > -                     return quirk->quirks;
>> > -     }
>> > +     q = drm_edid_quirk_find_by_id(edid->display_id);
>> > +     if (q != NULL)
>> > +             quirks = q->s.quirks;
>> >
>> > -     return 0;
>> > +     mutex_unlock(&edid_quirk_list_mutex);
>> > +
>> > +     return quirks;
>> >  }
>> >
>> >  #define MODE_SIZE(m) ((m)->hdisplay * (m)->vdisplay)
>> > @@ -1162,7 +1551,7 @@ do_inferred_modes(struct detailed_timing *timing,
>> void *c)
>> >       closure->modes += drm_dmt_modes_for_range(closure->connector,
>> >                                                 closure->edid,
>> >                                                 timing);
>> > -
>> > +
>> >       if (!version_greater(closure->edid, 1, 1))
>> >               return; /* GTF not defined yet */
>> >
>> > @@ -1399,7 +1788,7 @@ do_cvt_mode(struct detailed_timing *timing, void
>> *c)
>> >
>> >  static int
>> >  add_cvt_modes(struct drm_connector *connector, struct edid *edid)
>> > -{
>> > +{
>> >       struct detailed_mode_closure closure = {
>> >               connector, edid, 0, 0, 0
>> >       };
>> > @@ -1615,15 +2004,12 @@ void drm_edid_to_eld(struct drm_connector
>> *connector, struct edid *edid)
>> >
>> >       eld[0] = 2 << 3;                /* ELD version: 2 */
>> >
>> > -     eld[16] = edid->mfg_id[0];
>> > -     eld[17] = edid->mfg_id[1];
>> > -     eld[18] = edid->prod_code[0];
>> > -     eld[19] = edid->prod_code[1];
>> > +     *(u32 *)(&eld[16]) = edid->display_id.u;
>> >
>> >       if (cea[1] >= 3)
>> >               for (db = cea + 4; db < cea + cea[2]; db += dbl + 1) {
>> >                       dbl = db[0] & 0x1f;
>> > -
>> > +
>> >                       switch ((db[0] & 0xe0) >> 5) {
>> >                       case AUDIO_BLOCK:
>> >                               /* Audio Data Block, contains SADs */
>> > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> > index 21bcd4a..b939d51 100644
>> > --- a/drivers/gpu/drm/drm_stub.c
>> > +++ b/drivers/gpu/drm/drm_stub.c
>> > @@ -46,16 +46,22 @@ EXPORT_SYMBOL(drm_vblank_offdelay);
>> >  unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
>> >  EXPORT_SYMBOL(drm_timestamp_precision);
>> >
>> > +char *drm_edid_quirks = NULL;
>> > +EXPORT_SYMBOL(drm_edid_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, "MFG:prod:flags[,MFG:prod:flags[...]]\n"
>> > +                           "(See Documentation/EDID/edid_quirks.txt)");
>> >
>> >  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_quirks, charp, 0400);
>> >
>> >  struct idr drm_minors_idr;
>> >
>> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> > index 45ac8d6..84dc365 100644
>> > --- a/drivers/gpu/drm/drm_sysfs.c
>> > +++ b/drivers/gpu/drm/drm_sysfs.c
>> > @@ -84,6 +84,11 @@ static CLASS_ATTR_STRING(version, S_IRUGO,
>> >               __stringify(CORE_PATCHLEVEL) " "
>> >               CORE_DATE);
>> >
>> > +static CLASS_ATTR(edid_quirks_size, 0400, drm_edid_quirks_size_show, 0);
>> > +
>> > +static CLASS_ATTR(edid_quirks, 0600, drm_edid_quirks_show,
>> > +               drm_edid_quirks_store);
>> > +
>> >  /**
>> >   * drm_sysfs_create - create a struct drm_sysfs_class structure
>> >   * @owner: pointer to the module that is to "own" this struct
>> drm_sysfs_class
>> > @@ -113,10 +118,22 @@ struct class *drm_sysfs_create(struct module
>> *owner, char *name)
>> >       if (err)
>> >               goto err_out_class;
>> >
>> > +     err = class_create_file(class, &class_attr_edid_quirks_size);
>> > +     if (err)
>> > +             goto err_out_version;
>> > +
>> > +     err = class_create_file(class, &class_attr_edid_quirks);
>> > +     if (err)
>> > +             goto err_out_quirks_size;
>> > +
>> >       class->devnode = drm_devnode;
>> >
>> >       return class;
>> >
>> > +err_out_quirks_size:
>> > +     class_remove_file(class, &class_attr_edid_quirks_size);
>> > +err_out_version:
>> > +     class_remove_file(class, &class_attr_version.attr);
>> >  err_out_class:
>> >       class_destroy(class);
>> >  err_out:
>> > @@ -132,6 +149,8 @@ void drm_sysfs_destroy(void)
>> >  {
>> >       if ((drm_class == NULL) || (IS_ERR(drm_class)))
>> >               return;
>> > +     class_remove_file(drm_class, &class_attr_edid_quirks);
>> > +     class_remove_file(drm_class, &class_attr_edid_quirks_size);
>> >       class_remove_file(drm_class, &class_attr_version.attr);
>> >       class_destroy(drm_class);
>> >       drm_class = NULL;
>> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> > index d6b67bb..c947f3e 100644
>> > --- a/include/drm/drmP.h
>> > +++ b/include/drm/drmP.h
>> > @@ -1501,6 +1501,7 @@ extern unsigned int drm_debug;
>> >
>> >  extern unsigned int drm_vblank_offdelay;
>> >  extern unsigned int drm_timestamp_precision;
>> > +extern char *drm_edid_quirks;
>> >
>> >  extern struct class *drm_class;
>> >  extern struct proc_dir_entry *drm_proc_root;
>> > @@ -1612,6 +1613,15 @@ void drm_gem_vm_open(struct vm_area_struct *vma);
>> >  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) */
>> > +void drm_edid_quirks_param_process(void);
>> > +ssize_t drm_edid_quirks_size_show(struct class *class,
>> > +                               struct class_attribute *attr, char *buf);
>> > +ssize_t drm_edid_quirks_show(struct class *class, struct
>> class_attribute *attr,
>> > +                          char *buf);
>> > +ssize_t drm_edid_quirks_store(struct class *class, struct
>> class_attribute *attr,
>> > +                           const char *buf, size_t count);
>> > +
>> >  #include "drm_global.h"
>> >
>> >  static inline void
>> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > index 0cac551..713229b 100644
>> > --- a/include/drm/drm_edid.h
>> > +++ b/include/drm/drm_edid.h
>> > @@ -202,11 +202,18 @@ struct detailed_timing {
>> >  #define DRM_EDID_FEATURE_PM_SUSPEND       (1 << 6)
>> >  #define DRM_EDID_FEATURE_PM_STANDBY       (1 << 7)
>> >
>> > +union edid_display_id {
>> > +     struct {
>> > +             __be16 mfg_id;
>> > +             __le16 prod_code;
>> > +     } __attribute__((packed)) s;
>> > +     u32 u;
>> > +};
>>
>> I think adding this union is counterproductive. The u32 version is
>> helpful in one comparison and one assignment, while making all the rest
>> just slightly more confusing.
>>
>> > +
>> >  struct edid {
>> >       u8 header[8];
>> >       /* Vendor & product info */
>> > -     u8 mfg_id[2];
>> > -     u8 prod_code[2];
>> > +     union edid_display_id display_id;
>> >       u32 serial; /* FIXME: byte order */
>> >       u8 mfg_week;
>> >       u8 mfg_year;
>> > @@ -242,8 +249,6 @@ struct edid {
>> >       u8 checksum;
>> >  } __attribute__((packed));
>> >
>> > -#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] <<
>> 8))
>> > -
>> >  struct drm_encoder;
>> >  struct drm_connector;
>> >  struct drm_display_mode;
>> > --
>> > 1.7.11.4
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>

[PATCH v5 2/3] drm: Add EDID quirk to disable HDMI InfoFrames

[Ian Pilcher]

EDID_QUIRK_DISABLE_INFOFRAMES turns off all HDMI-specific
functionality (audio, HDCP, 3D, etc.).  Intended for displays that
are confused by *any* InfoFrames.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ea535f6..0d788b5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -70,6 +70,8 @@
 #define EDID_QUIRK_DETAILED_SYNC_PP		(1 << 6)
 /* Force reduced-blanking timings for detailed modes */
 #define EDID_QUIRK_FORCE_REDUCED_BLANKING	(1 << 7)
+/* Display is confused by InfoFrames; don't send any */
+#define EDID_QUIRK_DISABLE_INFOFRAMES		(1 << 8)
 
 struct detailed_mode_closure {
 	struct drm_connector *connector;
@@ -2109,6 +2111,14 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
 	int i, hdmi_id;
 	int start_offset, end_offset;
 	bool is_hdmi = false;
+	char buf[EDID_DISPLAY_ID_BUF_SIZE];
+
+	if (edid_get_quirks(edid) & EDID_QUIRK_DISABLE_INFOFRAMES) {
+		DRM_INFO("Disabling HDMI InfoFrames on display %s "
+			 "due to EDID quirk\n",
+			 drm_edid_display_id_format(edid->display_id, buf, 1));
+		goto end;
+	}
 
 	edid_ext = drm_find_cea_extension(edid);
 	if (!edid_ext)
-- 
1.7.11.4

[PATCH v5 3/3] drm: Add EDID quirk for LG L246WP

[Ian Pilcher]

This display is apparently confused by any InfoFrames (see
https://bugzilla.redhat.com/show_bug.cgi?id=806091).

Tested on a ThinkPad T510 (nVidia GT218 [NVS 3100M]).

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/gpu/drm/drm_edid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0d788b5..cbd651e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -158,6 +158,10 @@ union edid_quirk edid_quirk_list[EDID_QUIRK_LIST_SIZE] = {
 	{ { { { EDID_MFG_ID('V', 'S', 'C'), cpu_to_le16(5020) } },
 		EDID_QUIRK_FORCE_REDUCED_BLANKING } },
 
+	/* LG L246WP */
+	{ { { { EDID_MFG_ID('G', 'S', 'M'), cpu_to_le16(0x563f) } },
+		EDID_QUIRK_DISABLE_INFOFRAMES } },
+
 	/*
 	 * When adding built-in quirks, please adjust EDID_QUIRK_LIST_SIZE to
 	 * provide some room for user-supplied quirks.
-- 
1.7.11.4

Re: [PATCH v5 0/3] Enhanced EDID quirk functionality

[Ian Pilcher]

On 09/28/2012 02:03 AM, Ian Pilcher wrote:
> As promised, this patch series does only the following:
>
> * Add user-defined EDID quirks -- via sysfs or a module parameter
> * Add an EDID quirk flag to disable all HDMI functionality
>    (InfoFrames)
> * Add a quirk to disable HDMI InfoFrames for the LG L246WP
>

ACK?  NACK?  I'm on crack?

(I'm starting to think that this is some sort of hazing ritual.)

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

Re: [PATCH v5 0/3] Enhanced EDID quirk functionality

[Adam Jackson]

On 10/8/12 7:22 PM, Ian Pilcher wrote:
> On 09/28/2012 02:03 AM, Ian Pilcher wrote:
>> As promised, this patch series does only the following:
>>
>> * Add user-defined EDID quirks -- via sysfs or a module parameter
>> * Add an EDID quirk flag to disable all HDMI functionality
>>    (InfoFrames)
>> * Add a quirk to disable HDMI InfoFrames for the LG L246WP
>>
>
> ACK?  NACK?  I'm on crack?
>
> (I'm starting to think that this is some sort of hazing ritual.)

Sorry, I'm just not paying close attention to EDID stuff at the moment.

The series as is is clearly better than what we've got.  I still think 
there's something fundamentally amiss with a display that doesn't want 
to accept InfoFrames, but without having it in hand to mess with - and 
without wanting to play telephone to figure it out - I'm fine with this.

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax