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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 20/10/15 12:07, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 10:06:58AM +0100, Tvrtko Ursulin wrote:
>>
>> On 20/10/15 08:42, Daniel Vetter wrote:
>>> On Mon, Oct 19, 2015 at 09:20:36PM +0300, Ville Syrjälä wrote:
>>>> On Wed, Oct 07, 2015 at 11:01:23AM +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.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Sonika Jindal <sonika.jindal@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.
>>>
>>> Current rule is that pageflip doesn't allow any change in any metadata.
>>> There's some minor exception that on some platforms we can change the
>>> tiling because someone asked for that specifically and it's possible.
>>>
>>> atomic flips will be able to cope with this. But for legacy pageflips imo
>>> reject everything aggressively that changes metadata (stride, tiling,
>>> rotation).
>>
>> I am not sure what is the conclusion. To re-iterate, idea is not to
>> allow rotation changes between page flips, just to program PLANE_STRIDE
>> accordingly, when rotation is already enabled. Since otherwise page flip
>> will calculate it incorrectly.
>>
>> I though Ville was raising two concerns:
>>
>> 1. Will the plane state be swapped from under the pending page flips
>> prematurely?
>
> The answer is yes.

And is that OK? Would it not make more sense to wait for pending flips 
and then swap state?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx