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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx