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

[Adam Jackson]

On Fri, 2011-12-09 at 11:46 +0000, Kavuri, Sateesh wrote:

> +			if ((multi_val == STRUCTURE_PRESENT) || 
> +				(multi_val == STRUCTURE_MASK_PRESENT) ) {
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x01) == 0x01)
> +					caps->format |= 0x0; /* FRAME_PACKING */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x02) == 0x02)
> +					caps->format |= 0x1; /*FIELD_ALTERNATIVE */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x04) == 0x04)
> +					caps->format |= 0x2; /* LINE_ALTERNATIVE */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x08) == 0x08)
> +					caps->format |= 0x3; /*SIDE_BY_SIDE_FULL */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x10) == 0x10)
> +					caps->format |= 0x4; /* L_DEPTH */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x20) == 0x20)
> +					caps->format |= 0x5; /* L_DEPTH_GFX_GFX_DEPTH */
> +				if ((edid_ext[i+15+hdmi_vic_len] & 0x40) == 0x40)
> +					caps->format |= 0x6; /* TOP_BOTTOM */
> +				if ((edid_ext[i+14+hdmi_vic_len] & 0x01) == 0x01)
> +					caps->format |= 0x7; /* S_BY_S_HALF */
> +				if ((edid_ext[i+14+hdmi_vic_len] & 0x80) == 0x80)
> +					caps->format |= 0x8; /* S_BY_S_HALF_QUINCUNX */
> +			}

This reads poorly, which makes me think it's wrong.  Is format supposed
to be a bitfield or an enum?

> +EXPORT_SYMBOL(drm_detect_3D_monitor);

I suspect this is poorly named.  There exist 3D displays which are not
HDMI.

> +#define VSIF_RESET_INDEX 0xFFFFFFF8
> +#define VSIF_RESET_BIT_22 0xFFBFFFFF
> +#define VSIF_SET_BIT_19 0x80000
> +#define VSIF_RESET_BIT_20 0xFFEFFFFF
> +#define VSIF_RESET_BIT_17 0x10000
> +#define VSIF_SET_BIT_16 0xFFFDFFFF
> +#define VSIF_SET_MASTER_BIT 0x400000

i915 style is usually to write these as (1 << 22) I think.

More importantly, use semantic names for register contents.  "Bit 17"
doesn't mean anything.

> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8020798..5b4d09c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -798,6 +798,8 @@ extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
>  extern u8 *drm_find_cea_extension(struct edid *edid);
>  extern bool drm_detect_hdmi_monitor(struct edid *edid);
>  extern bool drm_detect_monitor_audio(struct edid *edid);
> +extern bool drm_detect_3D_monitor(struct edid *edid);
> +//extern struct panel_3d_caps* drm_detect_3D_monitor(struct edid *edid);

Well, which is it?

> +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 */
> +};

If format is supposed to be an enum, why aren't you using these symbolic
values?

- 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/20111213/353b0808/attachment.pgp>