[PATCH] Extract and parse the EDID when outputs are added

[Ander Conselvan de Oliveira]

Hi,


On 04/22/2013 02:57 PM, Richard Hughes 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.

You should add some information of why this patch is necessary to the 
commit message. It seems you added that to the cover letter, but it 
should be in the commit message so we can find that later without going 
through the list archives.

> The original EDID parsing code was written by me for gnome-color-manager.
> ---
>   src/compositor-drm.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/compositor.h     |   2 +-
>   2 files changed, 140 insertions(+), 1 deletion(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index da1ba79..1909712 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c

[...]

> @@ -1499,7 +1603,10 @@ create_output_for_connector(struct drm_compositor *ec,
>   	drmModeEncoder *encoder;
>   	drmModeModeInfo crtc_mode;
>   	drmModeCrtc *crtc;
> +	drmModePropertyPtr property;
> +	drmModePropertyBlobPtr edid_blob = NULL;
>   	int i;
> +	int rc;
>   	char name[32];
>   	const char *type_name;
>
> @@ -1517,6 +1624,7 @@ create_output_for_connector(struct drm_compositor *ec,
>   	output->base.subpixel = drm_subpixel_to_wayland(connector->subpixel);
>   	output->base.make = "unknown";
>   	output->base.model = "unknown";
> +	output->base.serial = "unknown";
>   	wl_list_init(&output->base.mode_list);
>
>   	if (connector->connector_type < ARRAY_LENGTH(connector_type_names))
> @@ -1642,6 +1750,37 @@ create_output_for_connector(struct drm_compositor *ec,
>
>   	wl_list_insert(ec->base.output_list.prev, &output->base.link);
>
> +	/* find and parse the EDID blob */
> +	for (i = 0; i < connector->count_props && !edid_blob; i++) {
> +		property = drmModeGetProperty(ec->drm.fd, connector->props[i]);
> +		if (!property)
> +			continue;
> +		if ((property->flags & DRM_MODE_PROP_BLOB) &&
> +		    !strcmp(property->name, "EDID")) {
> +			edid_blob = drmModeGetPropertyBlob(ec->drm.fd,
> +							   connector->prop_values[i]);
> +		}
> +		drmModeFreeProperty(property);
> +	}
> +	if (edid_blob) {
> +		rc = edid_parse(&output->edid,
> +				edid_blob->data,
> +				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[0] != '\0')
> +				output->base.make = output->edid.pnp_id;
> +			if (output->edid.monitor_name[0] != '\0')
> +				output->base.model = output->edid.monitor_name;
> +			if (output->edid.serial_number[0] != '\0')
> +				output->base.serial = output->edid.serial_number;
> +		}
> +		drmModeFreePropertyBlob(edid_blob);
> +	}
> +

Could you split this whole hunk into a separate function, 
create_output_for_connector() is already awfully long.

>   	output->base.origin = output->base.current;
>   	output->base.start_repaint_loop = drm_output_start_repaint_loop;
>   	output->base.repaint = drm_output_repaint;
> diff --git a/src/compositor.h b/src/compositor.h
> index 1e999a6..fa6162c 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -178,7 +178,7 @@ struct weston_output {
>   	uint32_t frame_time;
>   	int disable_planes;
>
> -	char *make, *model;
> +	char *make, *model, *serial;

I think it would be better to call this field 'serial_number'. We 
normally use 'serial' for numbers obtained from 
wl_display_next_serial(), and that may lead to unnecessary confusion.


Cheers,
Ander

>   	uint32_t subpixel;
>   	uint32_t transform;
>   	
>