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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 09 Oct 2012, Ian Pilcher <arequipeno@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
>
>> On Fri, 28 Sep 2012, Ian Pilcher <arequipeno@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxx>
>> > +                                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@xxxxxxxxxxxxxxxxxxxxx>.
>> > +
>> > +
>> > +[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@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux