[v2] drm/i915/skl: Consider plane rotation when calculating stride in skl_do_mmio_flip
diff mbox

Message ID 1445352851-26796-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Oct. 20, 2015, 2:54 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Previously rotation was ignored and wrong stride programmed
into the plane registers resulting in a corrupt image on screen.

v2: Do not access potentialy old plane state at flip time,
    but store the rotation value at the time of queing the flip.
    (Ville)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL)
Cc: Sonika Jindal <sonika.jindal@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 20 insertions(+), 11 deletions(-)

Comments

Ville Syrjala Oct. 20, 2015, 3:08 p.m. UTC | #1
On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Previously rotation was ignored and wrong stride programmed
> into the plane registers resulting in a corrupt image on screen.
> 
> v2: Do not access potentialy old plane state at flip time,
>     but store the rotation value at the time of queing the flip.
>     (Ville)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL)
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1fc1d240d3c1..9cb29086971a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>  }
>  
>  static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> +			     unsigned int rotation,
>  			     struct intel_unpin_work *work)
>  {
>  	struct drm_device *dev = intel_crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	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;
> @@ -11121,9 +11122,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(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);
> +	}
>  
>  	/*
>  	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
>  	intel_pipe_update_start(crtc);
>  
>  	if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
> -		skl_do_mmio_flip(crtc, work);
> +		skl_do_mmio_flip(crtc, mmio_flip->rotation, work);
>  	else
>  		/* use_mmio_flip() retricts MMIO flips to ilk+ */
>  		ilk_do_mmio_flip(crtc, work);
> @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
>  
>  static int intel_queue_mmio_flip(struct drm_device *dev,
>  				 struct drm_crtc *crtc,
> -				 struct drm_framebuffer *fb,
> -				 struct drm_i915_gem_object *obj,
> -				 struct intel_engine_cs *ring,
> -				 uint32_t flags)
> +				 unsigned int rotation,
> +				 struct drm_i915_gem_object *obj)
>  {
>  	struct intel_mmio_flip *mmio_flip;
>  
> @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>  	mmio_flip->i915 = to_i915(dev);
>  	mmio_flip->req = i915_gem_request_reference(obj->last_write_req);
>  	mmio_flip->crtc = to_intel_crtc(crtc);
> +	mmio_flip->rotation = rotation;

Could just do the crtc->primary->state->rotation thing here and avoid
having to pass the rotation from the caller.

But the patch looks OK anyway:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	INIT_WORK(&mmio_flip->work, intel_mmio_flip_work_func);
>  	schedule_work(&mmio_flip->work);
> @@ -11438,8 +11445,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->gtt_offset += intel_crtc->dspaddr_offset;
>  
>  	if (mmio_flip) {
> -		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
> -					    page_flip_flags);
> +		ret = intel_queue_mmio_flip(dev, crtc,
> +					    crtc->primary->state->rotation,
> +					    obj);
>  		if (ret)
>  			goto cleanup_unpin;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 27dccf35f9bc..140f949302c7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -493,6 +493,7 @@ struct intel_mmio_flip {
>  	struct drm_i915_private *i915;
>  	struct drm_i915_gem_request *req;
>  	struct intel_crtc *crtc;
> +	unsigned int rotation;
>  };
>  
>  struct skl_pipe_wm {
> -- 
> 1.9.1
Tvrtko Ursulin Oct. 20, 2015, 3:11 p.m. UTC | #2
On 20/10/15 16:08, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Previously rotation was ignored and wrong stride programmed
>> into the plane registers resulting in a corrupt image on screen.
>>
>> v2: Do not access potentialy old plane state at flip time,
>>      but store the rotation value at the time of queing the flip.
>>      (Ville)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL)
>> Cc: Sonika Jindal <sonika.jindal@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++-----------
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   2 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1fc1d240d3c1..9cb29086971a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>>   }
>>
>>   static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
>> +			     unsigned int rotation,
>>   			     struct intel_unpin_work *work)
>>   {
>>   	struct drm_device *dev = intel_crtc->base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	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;
>> @@ -11121,9 +11122,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(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);
>> +	}
>>
>>   	/*
>>   	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
>> @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
>>   	intel_pipe_update_start(crtc);
>>
>>   	if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
>> -		skl_do_mmio_flip(crtc, work);
>> +		skl_do_mmio_flip(crtc, mmio_flip->rotation, work);
>>   	else
>>   		/* use_mmio_flip() retricts MMIO flips to ilk+ */
>>   		ilk_do_mmio_flip(crtc, work);
>> @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
>>
>>   static int intel_queue_mmio_flip(struct drm_device *dev,
>>   				 struct drm_crtc *crtc,
>> -				 struct drm_framebuffer *fb,
>> -				 struct drm_i915_gem_object *obj,
>> -				 struct intel_engine_cs *ring,
>> -				 uint32_t flags)
>> +				 unsigned int rotation,
>> +				 struct drm_i915_gem_object *obj)
>>   {
>>   	struct intel_mmio_flip *mmio_flip;
>>
>> @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>>   	mmio_flip->i915 = to_i915(dev);
>>   	mmio_flip->req = i915_gem_request_reference(obj->last_write_req);
>>   	mmio_flip->crtc = to_intel_crtc(crtc);
>> +	mmio_flip->rotation = rotation;
>
> Could just do the crtc->primary->state->rotation thing here and avoid
> having to pass the rotation from the caller.

Oh that would indeed be nicer. I'll do a v3.

> But the patch looks OK anyway:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Can I carry over the r-b to v3?

Regards,

Tvrtko
Ville Syrjala Oct. 20, 2015, 3:17 p.m. UTC | #3
On Tue, Oct 20, 2015 at 04:11:34PM +0100, Tvrtko Ursulin wrote:
> 
> On 20/10/15 16:08, Ville Syrjälä wrote:
> > On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Previously rotation was ignored and wrong stride programmed
> >> into the plane registers resulting in a corrupt image on screen.
> >>
> >> v2: Do not access potentialy old plane state at flip time,
> >>      but store the rotation value at the time of queing the flip.
> >>      (Ville)
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL)
> >> Cc: Sonika Jindal <sonika.jindal@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++-----------
> >>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>   2 files changed, 20 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 1fc1d240d3c1..9cb29086971a 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
> >>   }
> >>
> >>   static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> >> +			     unsigned int rotation,
> >>   			     struct intel_unpin_work *work)
> >>   {
> >>   	struct drm_device *dev = intel_crtc->base.dev;
> >>   	struct drm_i915_private *dev_priv = dev->dev_private;
> >>   	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;
> >> @@ -11121,9 +11122,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(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);
> >> +	}
> >>
> >>   	/*
> >>   	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
> >> @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
> >>   	intel_pipe_update_start(crtc);
> >>
> >>   	if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
> >> -		skl_do_mmio_flip(crtc, work);
> >> +		skl_do_mmio_flip(crtc, mmio_flip->rotation, work);
> >>   	else
> >>   		/* use_mmio_flip() retricts MMIO flips to ilk+ */
> >>   		ilk_do_mmio_flip(crtc, work);
> >> @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
> >>
> >>   static int intel_queue_mmio_flip(struct drm_device *dev,
> >>   				 struct drm_crtc *crtc,
> >> -				 struct drm_framebuffer *fb,
> >> -				 struct drm_i915_gem_object *obj,
> >> -				 struct intel_engine_cs *ring,
> >> -				 uint32_t flags)
> >> +				 unsigned int rotation,
> >> +				 struct drm_i915_gem_object *obj)
> >>   {
> >>   	struct intel_mmio_flip *mmio_flip;
> >>
> >> @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
> >>   	mmio_flip->i915 = to_i915(dev);
> >>   	mmio_flip->req = i915_gem_request_reference(obj->last_write_req);
> >>   	mmio_flip->crtc = to_intel_crtc(crtc);
> >> +	mmio_flip->rotation = rotation;
> >
> > Could just do the crtc->primary->state->rotation thing here and avoid
> > having to pass the rotation from the caller.
> 
> Oh that would indeed be nicer. I'll do a v3.
> 
> > But the patch looks OK anyway:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Can I carry over the r-b to v3?

Yes.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fc1d240d3c1..9cb29086971a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11091,13 +11091,14 @@  static bool use_mmio_flip(struct intel_engine_cs *ring,
 }
 
 static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
+			     unsigned int rotation,
 			     struct intel_unpin_work *work)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	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;
@@ -11121,9 +11122,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(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);
+	}
 
 	/*
 	 * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on
@@ -11181,7 +11189,7 @@  static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
 	intel_pipe_update_start(crtc);
 
 	if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
-		skl_do_mmio_flip(crtc, work);
+		skl_do_mmio_flip(crtc, mmio_flip->rotation, work);
 	else
 		/* use_mmio_flip() retricts MMIO flips to ilk+ */
 		ilk_do_mmio_flip(crtc, work);
@@ -11208,10 +11216,8 @@  static void intel_mmio_flip_work_func(struct work_struct *work)
 
 static int intel_queue_mmio_flip(struct drm_device *dev,
 				 struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb,
-				 struct drm_i915_gem_object *obj,
-				 struct intel_engine_cs *ring,
-				 uint32_t flags)
+				 unsigned int rotation,
+				 struct drm_i915_gem_object *obj)
 {
 	struct intel_mmio_flip *mmio_flip;
 
@@ -11222,6 +11228,7 @@  static int intel_queue_mmio_flip(struct drm_device *dev,
 	mmio_flip->i915 = to_i915(dev);
 	mmio_flip->req = i915_gem_request_reference(obj->last_write_req);
 	mmio_flip->crtc = to_intel_crtc(crtc);
+	mmio_flip->rotation = rotation;
 
 	INIT_WORK(&mmio_flip->work, intel_mmio_flip_work_func);
 	schedule_work(&mmio_flip->work);
@@ -11438,8 +11445,9 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->gtt_offset += intel_crtc->dspaddr_offset;
 
 	if (mmio_flip) {
-		ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring,
-					    page_flip_flags);
+		ret = intel_queue_mmio_flip(dev, crtc,
+					    crtc->primary->state->rotation,
+					    obj);
 		if (ret)
 			goto cleanup_unpin;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 27dccf35f9bc..140f949302c7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -493,6 +493,7 @@  struct intel_mmio_flip {
 	struct drm_i915_private *i915;
 	struct drm_i915_gem_request *req;
 	struct intel_crtc *crtc;
+	unsigned int rotation;
 };
 
 struct skl_pipe_wm {