[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