Re: [PATCH v2 11/12] drm/i915: Deal with NV12 CbCr plane AUX surface on SKL+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 10, 2016 at 12:03:08PM +0200, Daniel Vetter wrote:
> On Wed, Aug 10, 2016 at 12:23:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > With NV12 we have two color planes to deal with so we must compute the
> > surface and x/y offsets for the second plane as well.
> > 
> > What makes this a bit nasty is that the hardware expects the surface
> > offset to be specified as a distance from the main surface offset.
> > What's worse, the distance must be non-negative (no neat wraparound or
> > anything). So we must make sure that the main surface offset is always
> > less or equal to the AUX surface offset. We do that by computing the AUX
> > offset first and the main surface offset second. If the main surface
> > offset ends up being above the AUX offset, we just push it down as far
> > as is required while still maintaining the required alignment etc.
> > 
> > Fortunately the AUX offset only reuqires 4K alignment, so we don't need
> > to do any of the backwards searching for an acceptable offset that we
> > must do for the main surface. And X tiled + NV12 isn't a supported
> > combination anyway.
> > 
> > Note that this just computes aux surface offsets, we do not yet program
> > them into the actual hardware registers, and hence we can't yet expose
> > NV12.
> > 
> > v2: Rebase due to drm_plane_state src/dst rects
> >     s/TODO.../something else/ in the commit message/ (Daniel)
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 62 ++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 +++
> >  2 files changed, 64 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b719c07599fd..f8487bcdb271 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2463,8 +2463,14 @@ u32 intel_compute_tile_offset(int *x, int *y,
> >  	const struct drm_i915_private *dev_priv = to_i915(state->base.plane->dev);
> >  	const struct drm_framebuffer *fb = state->base.fb;
> >  	unsigned int rotation = state->base.rotation;
> > -	u32 alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]);
> >  	int pitch = intel_fb_pitch(fb, plane, rotation);
> > +	u32 alignment;
> > +
> > +	/* AUX_DIST needs only 4K alignment */
> > +	if (fb->pixel_format == DRM_FORMAT_NV12 && plane == 1)
> > +		alignment = 4096;
> > +	else
> > +		alignment = intel_surf_alignment(dev_priv, fb->modifier[plane]);
> >  
> >  	return _intel_compute_tile_offset(dev_priv, x, y, fb, plane, pitch,
> >  					  rotation, alignment);
> > @@ -2895,7 +2901,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> >  	int h = drm_rect_height(&plane_state->base.src) >> 16;
> >  	int max_width = skl_max_plane_width(fb, 0, rotation);
> >  	int max_height = 4096;
> > -	u32 alignment, offset;
> > +	u32 alignment, offset, aux_offset = plane_state->aux.offset;
> >  
> >  	if (w > max_width || h > max_height) {
> >  		DRM_DEBUG_KMS("requested Y/RGB source size %dx%d too big (limit %dx%d)\n",
> > @@ -2909,6 +2915,15 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> >  	alignment = intel_surf_alignment(dev_priv, fb->modifier[0]);
> >  
> >  	/*
> > +	 * AUX surface offset is specified as the distance from the
> > +	 * main surface offset, and it must be non-negative. Make
> > +	 * sure that is what we will get.
> > +	 */
> > +	if (offset > aux_offset)
> > +		offset = intel_adjust_tile_offset(&x, &y, plane_state, 0,
> > +						  offset, aux_offset & ~(alignment - 1));
> 
> Note to self: Need to make sure that multiplanar fbs all reference the
> same underlying bo. But that code isn't yet added to
> intel_framebuffer_init().

Yep. It would be a lot more work to support different handles for the
planes, so I figure we shouldn't bother. Frankly I don't understand
why the hw doesn't just take two independent offsets, since it would
require exactly the same number of bits in the registers anyway. But
yeah, we have the hw we have, so we must live with it.

> 
> > +
> > +	/*
> >  	 * When using an X-tiled surface, the plane blows up
> >  	 * if the x offset + width exceed the stride.
> >  	 *
> > @@ -2935,6 +2950,35 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> >  	return 0;
> >  }
> >  
> > +static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
> > +{
> > +	const struct drm_framebuffer *fb = plane_state->base.fb;
> > +	unsigned int rotation = plane_state->base.rotation;
> > +	int max_width = skl_max_plane_width(fb, 1, rotation);
> > +	int max_height = 4096;
> > +	int x = plane_state->base.src.x1 >> 17;
> > +	int y = plane_state->base.src.y1 >> 17;
> > +	int w = drm_rect_width(&plane_state->base.src) >> 17;
> > +	int h = drm_rect_height(&plane_state->base.src) >> 17;
> > +	u32 offset;
> > +
> > +	intel_add_fb_offsets(&x, &y, plane_state, 1);
> > +	offset = intel_compute_tile_offset(&x, &y, plane_state, 1);
> > +
> > +	/* FIXME not quite sure how/if these apply to the chroma plane */
> > +	if (w > max_width || h > max_height) {
> > +		DRM_DEBUG_KMS("CbCr source size %dx%d too big (limit %dx%d)\n",
> > +			      w, h, max_width, max_height);
> > +		return -EINVAL;
> > +	}
> 
> We also need to check pitch of the 2nd plane, but I guess that's done at
> fb_init time, not at atomic_check time. Looks all reasonable as-is.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> > +
> > +	plane_state->aux.offset = offset;
> > +	plane_state->aux.x = x;
> > +	plane_state->aux.y = y;
> > +
> > +	return 0;
> > +}
> > +
> >  int skl_check_plane_surface(struct intel_plane_state *plane_state)
> >  {
> >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > @@ -2946,6 +2990,20 @@ int skl_check_plane_surface(struct intel_plane_state *plane_state)
> >  		drm_rect_rotate(&plane_state->base.src,
> >  				fb->width, fb->height, BIT(DRM_ROTATE_270));
> >  
> > +	/*
> > +	 * Handle the AUX surface first since
> > +	 * the main surface setup depends on it.
> > +	 */
> > +	if (fb->pixel_format == DRM_FORMAT_NV12) {
> > +		ret = skl_check_nv12_aux_surface(plane_state);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		plane_state->aux.offset = ~0xfff;
> > +		plane_state->aux.x = 0;
> > +		plane_state->aux.y = 0;
> > +	}
> > +
> >  	ret = skl_check_main_surface(plane_state);
> >  	if (ret)
> >  		return ret;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 2fde13833fcb..df8e7514c08c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -354,6 +354,10 @@ struct intel_plane_state {
> >  		u32 offset;
> >  		int x, y;
> >  	} main;
> > +	struct {
> > +		u32 offset;
> > +		int x, y;
> > +	} aux;
> >  
> >  	/*
> >  	 * scaler_id
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx