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

[Daniel Vetter <daniel@ffwll.ch>]

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().

> +
> +	/*
>  	 * 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx