Thanks for the comments. Fixed most of the issues with the earlier patch. Sending out a new one. Comments inline below.
> -----Original Message----- > From: Adam Jackson [mailto:ajax at redhat.com] > Sent: Tuesday, December 13, 2011 9:51 PM > To: Kavuri, Sateesh > Cc: dri-devel at lists.freedesktop.org > Subject: Re: [PATCH] drm: Enable reading 3D capabilities of 3D monitor > > 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. I want to integrate the other interfaces (eDP, DP, MIPI) panel detection in this method itself. Started off with implementing the HDMI part. > > > +#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. These I have fixed. Patch with these fixes below. > > 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? I fixed the part to use a u16 integer value to represent the format. > > - ajax ================
