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:53, Ville Syrjälä wrote:
> On Tue, Oct 20, 2015 at 12:44:05PM +0100, Tvrtko Ursulin wrote:
>>
>> 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?
>
> No, we could misprogram the stride and corrupt the display.

Oh I did not mean that but if it is OK to replace the state under queued 
flips. Sounded like a bug to me but what do I know.

>> Would it not make more sense to wait for pending flips
>> and then swap state?
>
> I think eventually we want to queue up multiple flips and/or allow
> later flips to override previous ones, so consulting the current
> state from the mmio flip is a not a good idea in any case. I would
> just add work->rotation and populate it with a snapshot at the time
> when the flip is queued.

Yes I can certainly do that if deemed acceptable.

Regards,

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