[PATCH 3/3] Parse the EDID when outputs are added

David Herrmann dh.herrmann at gmail.com
Fri Apr 19 12:41:17 PDT 2013


Hi

On Fri, Apr 19, 2013 at 5:02 PM, Richard Hughes <hughsient at gmail.com> wrote:
> At the moment we're only extracting interesting strings. We have to be quite
> careful parsing the EDID data, as vendors like to do insane things.
>
> The original EDID parsing code was written by me for gnome-color-manager.
> ---
>  src/compositor-drm.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index a454676..4f37447 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -25,6 +25,7 @@
>
>  #include <errno.h>
>  #include <stdlib.h>
> +#include <ctype.h>
>  #include <string.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> @@ -131,6 +132,14 @@ struct drm_fb {
>         void *map;
>  };
>
> +struct drm_edid {
> +       char *monitor_name;
> +       char *serial_number;
> +       char *eisa_id;
> +       char *pnp_id;
> +       /* other things can be added as required */

That comment is pretty useless, isn't it?

> +};
> +
>  struct drm_output {
>         struct weston_output   base;
>
> @@ -140,6 +149,7 @@ struct drm_output {
>         uint32_t connector_id;
>         drmModeCrtcPtr original_crtc;
>         drmModePropertyBlobPtr edid_blob;
> +       struct drm_edid *edid;

Why not "struct drm_edid edid;"? Allocating a separate object saves
4*sizeof(void*) bytes per output without EDID blobs but costs like the
same amount of malloc-overhead for each output with EDID.

Furthermore, you need to check (output->edid && output->edid->XY) in
other code that accesses this, instead of just (output->edid.XY) if
you directly embed it.

I don't think it's worth saving this small amount, but I'm not to decide here.

>
>         int vblank_pending;
>         int page_flip_pending;
> @@ -1005,6 +1015,16 @@ static void
>  drm_output_fini_pixman(struct drm_output *output);
>
>  static void
> +drm_edid_destroy(struct drm_edid *edid)
> +{
> +       free(edid->monitor_name);
> +       free(edid->serial_number);
> +       free(edid->eisa_id);
> +       free(edid->pnp_id);
> +       free(edid);
> +}
> +
> +static void
>  drm_output_destroy(struct weston_output *output_base)
>  {
>         struct drm_output *output = (struct drm_output *) output_base;
> @@ -1014,6 +1034,8 @@ drm_output_destroy(struct weston_output *output_base)
>
>         if (output->edid_blob)
>                 drmModeFreePropertyBlob(output->edid_blob);
> +       if (output->edid)
> +               drm_edid_destroy (output->edid);
>
>         if (output->backlight)
>                 backlight_destroy(output->backlight);
> @@ -1490,6 +1512,131 @@ drm_output_fini_pixman(struct drm_output *output)
>         }
>  }
>
> +static char *
> +edid_parse_string (const uint8_t *data)
> +{
> +       char *text;
> +       int i;
> +       int replaced = 0;
> +
> +       /* this is always 12 bytes, but we can't guarantee it's null
> +        * terminated or not junk. */
> +       text = strndup ((const char *) data, 12);
> +
> +       /* remove insane chars */
> +       for (i = 0; text[i] != '\0'; i++) {
> +               if (text[i] == '\n' ||
> +                   text[i] == '\r')
> +                       text[i] = '\0';

break;
No reason to continue if you write a NUL byte.

> +       }
> +
> +       /* nothing left? */
> +       if (text[0] == '\0') {
> +               free (text);
> +               text = NULL;
> +               goto out;

return NULL; ?

> +       }
> +
> +       /* ensure string is printable */
> +       for (i = 0; text[i] != '\0'; i++) {
> +               if (!isprint (text[i])) {

No space between function-name and '('.

> +                       text[i] = '-';
> +                       replaced++;
> +               }
> +       }
> +
> +       /* if the string is random junk, ignore the string */
> +       if (replaced > 4) {

I guess this "4" is pretty random? Just to be clear.

> +               free (text);
> +               text = NULL;
> +               goto out;

return NULL; ?

> +       }
> +out:

You can remove this label then.

> +       return text;
> +}
> +
> +#define EDID_DESCRIPTOR_ALPHANUMERIC_DATA_STRING       0xfe
> +#define EDID_DESCRIPTOR_DISPLAY_PRODUCT_NAME           0xfc
> +#define EDID_DESCRIPTOR_DISPLAY_PRODUCT_SERIAL_NUMBER  0xff
> +#define EDID_OFFSET_DATA_BLOCKS                                0x36
> +#define EDID_OFFSET_LAST_BLOCK                         0x6c
> +#define EDID_OFFSET_PNPID                              0x08
> +#define EDID_OFFSET_SERIAL                             0x0c
> +
> +static int
> +edid_parse (struct drm_edid *edid, const uint8_t *data, size_t length)
> +{
> +       char *tmp;
> +       int i;
> +       int rc = 0;
> +       uint32_t serial;
> +
> +       /* check header */
> +       if (length < 128) {
> +               rc = -1;
> +               goto out;
> +       }
> +       if (data[0] != 0x00 || data[1] != 0xff) {
> +               rc = -1;
> +               goto out;
> +       }
> +
> +       /* decode the PNP ID from three 5 bit words packed into 2 bytes
> +        * /--08--\/--09--\
> +        * 7654321076543210
> +        * |\---/\---/\---/
> +        * R  C1   C2   C3 */
> +       edid->pnp_id = malloc(4);

A pointer is at least 4 bytes wide on most machines, so I cannot see
why you don't use a static array instead of allocating this on the
heap?

> +       edid->pnp_id[0] = 'A' + ((data[EDID_OFFSET_PNPID+0] & 0x7c) / 4) - 1;
> +       edid->pnp_id[1] = 'A' + ((data[EDID_OFFSET_PNPID+0] & 0x3) * 8) + ((data[EDID_OFFSET_PNPID+1] & 0xe0) / 32) - 1;
> +       edid->pnp_id[2] = 'A' + (data[EDID_OFFSET_PNPID+1] & 0x1f) - 1;
> +       edid->pnp_id[3] = '\0';;

double semicolon

> +
> +       /* maybe there isn't a ASCII serial number descriptor, so use this instead */
> +       serial = (int32_t) data[EDID_OFFSET_SERIAL+0];
> +       serial += (int32_t) data[EDID_OFFSET_SERIAL+1] * 0x100;
> +       serial += (int32_t) data[EDID_OFFSET_SERIAL+2] * 0x10000;
> +       serial += (int32_t) data[EDID_OFFSET_SERIAL+3] * 0x1000000;

You should use "uint32_t" as "serial" is unsigned, too.
(int32_t)(char)255 might produce "-1", which is ~0 casted to uint32_t
instead of 255. (I'm actually not entirely sure with that.. but lets
be safe).

And we use spaces around binary operators.

> +       if (serial > 0) {
> +               edid->serial_number = malloc(9);

serial can be up to 0xffffffff which is "4294967295" to me. So you
need 11 characters. Or change the %li to %lx below so you get the
hexadecimal representation (which is probably want you meant, anyway?)

> +               sprintf (edid->serial_number, "%li", (long int) serial);

Use %lu and (unsigned long) to avoid signed representation. Or just
use %lx as mentioned above.

> +       }
> +
> +       /* parse EDID data */
> +       for (i = EDID_OFFSET_DATA_BLOCKS;
> +            i <= EDID_OFFSET_LAST_BLOCK;
> +            i += 18) {
> +               /* ignore pixel clock data */
> +               if (data[i] != 0)
> +                       continue;
> +               if (data[i+2] != 0)
> +                       continue;
> +
> +               /* any useful blocks? */
> +               if (data[i+3] == EDID_DESCRIPTOR_DISPLAY_PRODUCT_NAME) {
> +                       tmp = edid_parse_string (&data[i+5]);
> +                       if (tmp != NULL) {
> +                               free (edid->monitor_name);
> +                               edid->monitor_name = tmp;
> +                       }
> +               } else if (data[i+3] == EDID_DESCRIPTOR_DISPLAY_PRODUCT_SERIAL_NUMBER) {
> +                       tmp = edid_parse_string (&data[i+5]);
> +                       if (tmp != NULL) {
> +                               free (edid->serial_number);
> +                               edid->serial_number = tmp;
> +                       }
> +               } else if (data[i+3] == EDID_DESCRIPTOR_ALPHANUMERIC_DATA_STRING) {
> +                       tmp = edid_parse_string (&data[i+5]);
> +                       if (tmp != NULL) {
> +                               free (edid->eisa_id);
> +                               edid->eisa_id = tmp;
> +                       }
> +               }
> +       }
> +out:

Again, just use "return -1;" above instead of "goto out;". There is no
cleanup that we have to do so we can just return early. You can also
remove "rc" then.

> +       return rc;
> +}
> +
>  static int
>  create_output_for_connector(struct drm_compositor *ec,
>                             drmModeRes *resources,
> @@ -1505,6 +1652,7 @@ create_output_for_connector(struct drm_compositor *ec,
>         drmModeCrtc *crtc;
>         drmModePropertyPtr property;
>         int i;
> +       int rc;
>         char name[32];
>         const char *type_name;
>
> @@ -1666,7 +1814,23 @@ create_output_for_connector(struct drm_compositor *ec,
>                 weston_log("Got EDID blob %p of size %u.\n",
>                            output->edid_blob->data,
>                            output->edid_blob->length);

If you print the parsed EDID information below, I think this log
message isn't needed?

> -               /* FIXME: actually parse EDID */
> +               output->edid = malloc(sizeof(struct drm_edid));
> +               memset(output->edid, 0, sizeof *output->edid);
> +               rc = edid_parse(output->edid,
> +                               output->edid_blob->data,
> +                               output->edid_blob->length);
> +               if (!rc) {
> +                       weston_log("EDID data '%s', '%s', '%s'\n",
> +                                  output->edid->pnp_id,
> +                                  output->edid->monitor_name,
> +                                  output->edid->serial_number);
> +                       if (output->edid->pnp_id)
> +                               output->base.make = output->edid->pnp_id;
> +                       if (output->edid->monitor_name)
> +                               output->base.model = output->edid->monitor_name;
> +                       if (output->edid->serial_number)
> +                               output->base.serial = output->edid->serial_number;
> +               }
>         }
>
>         output->base.origin = output->base.current;

Other than that, the patches look fine to me.

Regards
David


More information about the wayland-devel mailing list