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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
