[Intel-gfx] [PATCH] drm/i915: Consider plane rotation when calculating stride in skl_do_mmio_flip

[Ville Syrjälä]

On Wed, Oct 07, 2015 at 11:01:23AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Previously rotation was ignored and wrong stride programmed
> into the plane registers resulting in a corrupt image on screen.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Sonika Jindal <sonika.jindal at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 539c3737e823..6328788193e4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11126,9 +11126,10 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane *plane = intel_crtc->base.primary;
>  	struct drm_framebuffer *fb = intel_crtc->base.primary->fb;
>  	const enum pipe pipe = intel_crtc->pipe;
> -	u32 ctl, stride;
> +	u32 ctl, stride, tile_height;
>  
>  	ctl = I915_READ(PLANE_CTL(pipe, 0));
>  	ctl &= ~PLANE_CTL_TILED_MASK;
> @@ -11152,9 +11153,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
>  	 * The stride is either expressed as a multiple of 64 bytes chunks for
>  	 * linear buffers or in number of tiles for tiled buffers.
>  	 */
> -	stride = fb->pitches[0] /
> -		 intel_fb_stride_alignment(dev, fb->modifier[0],
> -					   fb->pixel_format);
> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> +		/* stride = Surface height in tiles */
> +		tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 0);
> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> +	} else {
> +		stride = fb->pitches[0] /
> +				intel_fb_stride_alignment(dev, fb->modifier[0],
> +							  fb->pixel_format);
> +	}

I was wondering why we are allowing stride changes during page flip, but
after looking at the history it seems we are not. The reason for
updating the stride register is the fact that the units we specify it
in change between different tiling modes on SKL+. We still reject actual
stride changes during page flip, which is good because allowing it would
cause problems for my fb->offsets[] stuff since the interpretation of the
linear offset would change with the stride.

We do allow changes to the rotated stride though since we don't reject
changes to the fb height. I think I need to draw some pictures before I
can say for sure whether that can cause problems or not. But we ca
leave that for another patch if it turns out to need handling.

One thing that's dodgy here is the plane->state->rotation check. I
think currently we wait for pending flips during the atomic commit
phase after we've swapped the state. So this may end up using the
wrong rotation setting. It would be an even bigger problem if we
already allowed queueing up or replaceing pending plane updates. I
suppose the primary->fb thing doesn't suffer from this problem because
we swap that pointer only after we've waited for pending flips.

>  	/*
>  	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC