Re: [PATCH 1/3] [VPG HSW-A] drm/i915: Add aspect ratio in drm_display_mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 15, 2013 at 10:06:42AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 15, 2013 at 10:29:01AM +0530, vandana.kannan@xxxxxxxxx wrote:
> > From: vkannan <vandana.kannan@xxxxxxxxx>
> > 
> > Mode is the video format, which is the information the sink needs to
> > properly display an image. a complete definition of video format includes
> > video timing, picture aspect ratio, color space, quantization range,
> > component depth.
> > 
> > video format timing may be associated with more than 1 video format
> > for example, 720x480p formatted in the 4:3 Picture Aspect Ratio or a
> > 720x480p formatted in the 16:9 Picture Aspect Ratio.
> > 
> > For HDMI compliance, a set of CEA modes are tested (CEA 861-D table 3).
> > This list has 64 modes. When one of the modes are set, the corresponding
> > fields should show up correctly (as mentioned in Table 3 of CEA spec).
> > For picture aspect ratio, if the mode is found from the CEA mode list,
> > the corresponding PAR is sent as part of infoframe. If the mode to be set
> > is not part of the CEA mode list, PAR is calculated from resolution.
> > 
> > CEA modes have a specific picture aspect ratio. Adding this field
> > as part of drm_display_mode. This is required to be sent as part of AVI
> > infoframe for HDMI compliance.
> 
> > 
> > Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>

Slight meta comment: When doing patches which also change code outside of
drm/i915 Please try really hard to split it up into a drm core patch to
prepare things. That patch needs a drm/<something>: prefix so that people
on the mailing list notice that it's not just for drm/i915. Then roll out
the change accross all drivers in a set of drm/<driver>: patches.

Also drm core patches always need to be cc'ed to the dri-devel mailing
list, but drm/i915 should also be cc'ed to the intel-gfx mailing list.

And like Ville mentioned we've recently switched to the shared hdmi
infoframe helpers, so this needs to be rebased on top of
drm-intel-nightly.

Thanks, Daniel

> > ---
> >  drivers/gpu/drm/drm_edid.c              |  374 ++++++++++++++++++-------------
> >  drivers/gpu/drm/gma500/oaktrail_lvds.c  |   14 +-
> >  drivers/gpu/drm/gma500/psb_intel_sdvo.c |   38 ++--
> >  drivers/gpu/drm/i915/intel_display.c    |    3 +-
> >  drivers/gpu/drm/i915/intel_sdvo.c       |   38 ++--
> >  include/drm/drm_crtc.h                  |    7 +-
> >  6 files changed, 265 insertions(+), 209 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index e8d17ee..83e2fda 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -135,378 +135,379 @@ static const struct drm_display_mode drm_dmt_modes[] = {
> >  	/* 640x350@85Hz */
> >  	{ DRM_MODE("640x350", DRM_MODE_TYPE_DRIVER, 31500, 640, 672,
> >  		   736, 832, 0, 350, 382, 385, 445, 0,
> > -		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC) },
> > +		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC, 0) },
> <snip a lot of the same>
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 9779ea1..07c0d58 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -30,6 +30,7 @@
> >  #include <linux/types.h>
> >  #include <linux/idr.h>
> >  #include <linux/fb.h>
> > +#include <linux/hdmi.h>
> >  #include <drm/drm_mode.h>
> >  
> >  #include <drm/drm_fourcc.h>
> > @@ -115,12 +116,14 @@ enum drm_mode_status {
> >  #define DRM_MODE_TYPE_CLOCK_CRTC_C (DRM_MODE_TYPE_CLOCK_C | \
> >  				    DRM_MODE_TYPE_CRTC_C)
> >  
> > -#define DRM_MODE(nm, t, c, hd, hss, hse, ht, hsk, vd, vss, vse, vt, vs, f) \
> > +#define DRM_MODE(nm, t, c, hd, hss, hse, ht, hsk, vd, vss, vse, vt, vs, f, \
> > +	ar) \
> >  	.name = nm, .status = 0, .type = (t), .clock = (c), \
> >  	.hdisplay = (hd), .hsync_start = (hss), .hsync_end = (hse), \
> >  	.htotal = (ht), .hskew = (hsk), .vdisplay = (vd), \
> >  	.vsync_start = (vss), .vsync_end = (vse), .vtotal = (vt), \
> >  	.vscan = (vs), .flags = (f), \
> > +	.picture_aspect_ratio = (ar), \
> >  	.base.type = DRM_MODE_OBJECT_MODE
> >  
> >  #define CRTC_INTERLACE_HALVE_V 0x1 /* halve V values for interlacing */
> > @@ -177,6 +180,8 @@ struct drm_display_mode {
> >  
> >  	int vrefresh;		/* in Hz */
> >  	int hsync;		/* in kHz */
> > +
> > +	enum hdmi_picture_aspect picture_aspect_ratio;
> >  };
> 
> I'm not sure we want to bloat drm_display_mode with additional
> junk for the sake of CEA modes. We could perhaps just have a
> separate VIC indexed array for the aspect ratio information.
> 
> At the very least we don't want to modify DRM_MODE() since that makes
> this patch needlessly large, and makes DRM_MODE() even harder to decode
> for humans. Instead you can just use c99 initializers and do it like so:
> 
>   { DRM_MODE(...),
>     .picture_aspect_ratio = x },
> 
> >  
> >  enum drm_connector_status {
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux