On Tue, 09 Oct 2012, Ian Pilcher <arequipeno at 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 at linux.intel.com> wrote:
>
>> On Fri, 28 Sep 2012, Ian Pilcher <arequipeno at 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 at 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 at 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 at 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 at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
