[Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.

[Runyan, Arthur J]

I'll take a look.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com] 
Sent: Tuesday, May 12, 2015 3:44 AM
To: Konduru, Chandra
Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville; Jindal, Sonika; Runyan, Arthur J
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.

On Mon, May 11, 2015 at 11:32:07PM +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > Sent: Monday, May 11, 2015 5:03 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12
> > format.
> > 
> > On Fri, May 08, 2015 at 08:22:47PM -0700, Chandra Konduru wrote:
> > > Adding NV12 90/270 rotation support for primary and sprite planes.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   23 ++++++++++++++++-------
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   32 +++++++++++++++++++++++++------
> > -
> > >  2 files changed, 41 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > > index c385a3b..77d7f69 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3105,7 +3105,8 @@ static void skylake_update_primary_plane(struct
> > drm_crtc *crtc,
> > >  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
> > >  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
> > >  	int scaler_id = -1;
> > > -	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> > > +	unsigned long aux_dist = 0;
> > > +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> > >  	u32 tile_row_adjustment = 0;
> > >
> > >  	plane_state = to_intel_plane_state(plane->state);
> > > @@ -3163,12 +3164,16 @@ static void skylake_update_primary_plane(struct
> > drm_crtc *crtc,
> > >  		x_offset = stride * tile_height - y - src_h;
> > >  		y_offset = x;
> > >  		plane_size = (src_w - 1) << 16 | (src_h - 1);
> > > -		/*
> > > -		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> > > -		 * be treated as separate surfaces and GTT remapping for
> > > -		 * rotation should be done separately for each subplane.
> > > -		 * Enable support once seperate remappings are available.
> > > -		 */
> > > +
> > > +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> > > +			u32 uv_tile_height = intel_tile_height(dev, fb-
> > >pixel_format,
> > > +						fb->modifier[0], 1);
> > > +			aux_stride = DIV_ROUND_UP(fb->height / 2,
> > uv_tile_height);
> > > +			aux_dist =
> > intel_plane_obj_offset(to_intel_plane(plane), obj, 1) -
> > > +				surf_addr;
> > > +			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb-
> > >height / 2;
> > > +			aux_y_offset = x / 2;
> > > +		}
> > >  	} else {
> > >  		stride = fb->pitches[0] / stride_div;
> > >  		x_offset = x;
> > > @@ -13144,6 +13149,10 @@ intel_check_primary_plane(struct drm_plane
> > *plane,
> > >  	if (fb && format_is_yuv(fb->pixel_format)) {
> > >  		src->x1 &= ~0x10000;
> > >  		src->x2 &= ~0x10000;
> > > +		if (intel_rotation_90_or_270(state->base.rotation)) {
> > > +			src->y1 &= ~0x10000;
> > > +			src->y2 &= ~0x10000;
> > > +		}
> > 
> > This feels fishy. Why do we need to make the Y coordinates even? The
> > reson for making the X coordinates even is to make them macropixel
> > aligned, but there are no macropixels in the Y direction so this
> > doesn't make much sense to me.
> 
> Hi Ville,
> Per skl spec, it is expecting even lines aligned with 90/270 rotation not only
> for NV12 but also for 422 formats. Perhaps we might have missed when 90/270
> enabled for packed YUV formats.

The src coordinates are always in the fb orientation, so macropixels
appear in the src.x direction only. And when we do 90/270 rotation the
hardware Y offset comes from src.x coordinates.

The spec does seem a bit confused though; It claims the X offset must
always be even for YUV422+NV12, and the Y offset must be even when
rotated 90/270 degrees. I suspect the X offset text just didn't get
updated when 90/270 rotation was added.

Art, can you confirm?

-- 
Ville Syrjälä
Intel OTC