[PATCH] drm: Enable reading 3D capabilities of 3D monitor

[Adam Jackson]

On Wed, 2011-12-14 at 13:04 +0000, Kavuri, Sateesh wrote:

> +	/* Data block offset in CEA extension block */
> +	start_offset = 4;
> +	end_offset = edid_ext[2];
> +
> +	/* 3D vars */
> +	int multi_present = 0;

Pretty sure kernel style frowns upon mixed decls and code.

> +	/*
> +	 * Because HDMI identifier is in Vendor Specific Block,
> +	 * search it from all data blocks of CEA extension.
> +	 */
> +	for (i = start_offset; i < end_offset;
> +		/* Increased by data block len */
> +		i += ((edid_ext[i] & 0x1f) + 1)) {
> +		/* Find vendor specific block */
> +		/* 6'th bit is the VIDEO_PRESENT bit */
> +		if ( ((edid_ext[i] >> 5) == VENDOR_BLOCK) &&
> +	             ((edid_ext[i+8] & 0x20) == MONITOR_VIDEO_PRESENT) ) {

This conditional will always be false:

    if ((x == VENDOR_BLOCK) &&
        ((y & 0x20) == 0x01))

I suspect you want:

#define MONITOR_VIDEO_PRESENT (1 << 6)
...
    if ((x == VENDOR_BLOCK) && (y & MONITOR_VIDEO_PRESENT)) {
    ...

> +			hdmi_id = edid_ext[i + 1] | (edid_ext[i + 2] << 8) |
> +				  edid_ext[i + 3] << 16;
> +			/* Find HDMI identifier */
> +			if (hdmi_id == HDMI_IDENTIFIER)
> +				is_hdmi = true;
> +
> +			/* Check for the 3D_Present flag */
> +			if ((edid_ext[i+13] >> 6) == PANEL_SUPPORTS_3D) {
> +				caps = kmalloc(sizeof(struct drm_panel_3D_capabilities), GFP_KERNEL);
> +				caps->panel_supports_3D = 1;
> +			}

This will probably also not do what you want.  Consider what happens if
edid_ext[i+13] has (1 << 7) set.

> +			/* Check if 3D_Multi_present is set */
> +			if ((edid_ext[i+13] & 0x60) == 0x0) {
> +				multi_present = true;
> +			}

Code and comment disagree.

> +
> +			/* Collect 3D capabilities of the monitor */
> +			int hdmi_3d_len = 0;
> +			int hdmi_vic_len = 0;
> +			hdmi_vic_len = (edid_ext[i+14] >> 0x05);
> +			hdmi_3d_len = ((edid_ext[i+14] << 0x03) >>0x03);
> +			int multi_val = edid_ext[i+13] & 0x6;

Note: multi_val can only have the values {0, 2, 4, 6} now.

> +			if (multi_val == 0x0)
> +				multi_present = NO_SPL_CAPS;
> +			else if (multi_val == 0x1)
> +				multi_present = STRUCTURE_PRESENT;

These two assignments (and the one above) are the only use of the
multi_present variable, it's never used in a subsequent conditional or
passed back to the caller.

The 'else' here can never be true, as noted above.

> +			if ((multi_val == STRUCTURE_PRESENT) || 
> +				(multi_val == STRUCTURE_MASK_PRESENT) ) {
> +				if (edid_ext[i+15+hdmi_vic_len] & (1 << 0))
> +					caps->format |= (1 << 0); /* FRAME_PACKING */
> +				if (edid_ext[i+15+hdmi_vic_len] & (1 << 1))
> +					caps->format |= (1 << 1); /*FIELD_ALTERNATIVE */
> +				if (edid_ext[i+15+hdmi_vic_len] & (1 << 2))
> +					caps->format |= (1 << 2); /* LINE_ALTERNATIVE */
> +				if (edid_ext[i+15+hdmi_vic_len] & 0x3)
> +					caps->format |= (1 << 3); /*SIDE_BY_SIDE_FULL */
> +				if (edid_ext[i+15+hdmi_vic_len] & (1 << 4))
> +					caps->format |= (1 << 4); /* L_DEPTH */
> +				if (edid_ext[i+15+hdmi_vic_len] & 0x5)
> +					caps->format |= (1 << 5); /* L_DEPTH_GFX_GFX_DEPTH */
> +				if (edid_ext[i+15+hdmi_vic_len] & 0x6)
> +					caps->format |= (1 << 6); /* TOP_BOTTOM */
> +				if (edid_ext[i+14+hdmi_vic_len] & 0x7)
> +					caps->format |= (1 << 7); /* S_BY_S_HALF */
> +				if (edid_ext[i+14+hdmi_vic_len] & (1 << 8))
> +					caps->format |= (1 << 8); /* S_BY_S_HALF_QUINCUNX */
> +			}

Here you've made it clear that ->format is a bitfield, but the values
should be in a header (so drivers can use them) and this code should
only be using the symbolic names.

> @@ -1675,6 +1783,7 @@ static void drm_add_display_info(struct edid *edid,
>  		return;
>  
>  	info->cea_rev = edid_ext[1];
> +	info->caps_3D= drm_detect_3D_monitor(edid);
>  }
>  
>  /**

Whitespace.

> +/* Pre-defined 3D formats as per HDMI spec */
> +enum s3d_formats {
> +        FRAME_PACKING           = 0x0,
> +        FIELD_ALTERNATIVE       = 0x1,
> +        LINE_ALTERNATIVE        = 0x2,
> +        SIDE_BY_SIDE_FULL       = 0x3,
> +        L_DEPTH                 = 0x4,
> +        L_DEPTH_GFX_GFX_DEPTH   = 0x5,
> +        TOP_BOTTOM              = 0x6, /* 0x7 is reserved for future */
> +        SIDE_BY_SIDE_HALF       = 0x8  /* 0x9 onwards is reserved for future */
> +};

These don't match the bit shifts you used earlier.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20111214/cb789388/attachment.pgp>