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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915/skl: Consider plane rotation when calculating stride in skl_do_mmio_flip
Date: Tue, 20 Oct 2015 16:11:34 +0100	[thread overview]
Message-ID: <562659A6.3070105@linux.intel.com> (raw)
In-Reply-To: <20151020150804.GM26517@intel.com>

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