[Intel-gfx] [PATCH] drm/i915: Call intel_fbc_pre_update() after pinning the new pageflip

[chris at chris-wilson.co.uk]

On Tue, Aug 16, 2016 at 04:49:26PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-08-16 às 08:43 +0100, Chris Wilson escreveu:
> > intel_fbc_pre_update() depends upon the new state being already
> > pinned
> > in place in the Global GTT (primarily for both fencing which wants
> > both
> > an offset and a fence register, if assigned). This requires the call
> > to
> > intel_fbc_pre_update() be after intel_pin_and_fence_fb() - but commit
> > 5a21b6650a23 ("drm/i915: Revert async unpin and nonblocking atomic
> > commit") seems to have returned the code to a different state.
> > 
> > Fixes 5a21b6650a23 ("drm/i915: Revert async unpin and
> > nonblocking...")
> 
> I think it actually fixes e8216e502acaad129210c3c8b30cb4ab41e70239
> ("drm/i915/fbc: call intel_fbc_pre_update earlier during page flips").

Hmm, I was pretty sure that the before the revert it was in the right
place, but I could well be mistaken. Certainly that commit sounds more
than a likely suspect.

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > Cc: drm-intel-fixes at lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 37d71d5d2369..916ce7b2d4d7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12061,9 +12061,6 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> >  	crtc->primary->fb = fb;
> >  	update_state_fb(crtc->primary);
> >  
> > -	intel_fbc_pre_update(intel_crtc, intel_crtc->config,
> > -			     to_intel_plane_state(primary->state));
> > -
> >  	work->pending_flip_obj = i915_gem_object_get(obj);
> >  
> >  	ret = i915_mutex_lock_interruptible(dev);
> > @@ -12109,6 +12106,9 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> >  	work->gtt_offset += intel_crtc->dspaddr_offset;
> >  	work->rotation = crtc->primary->state->rotation;
> >  
> > +	intel_fbc_pre_update(intel_crtc, intel_crtc->config,
> > +			     to_intel_plane_state(primary->state));
> > +
> 
> After you reported the problem yesterday I wrote almost the exact same
> patch and just didn't submit it because I didn't have time to test it.
> I also added a little comment on top of the call to try to prevent us
> from further regressions:
> 
> /* There's the potential that the next frame will not be compatible
> with FBC, so we want to call pre_update() before the actual page flip.
> The problem is that pre_update() caches some information about the fb
> object, so we want to do this only after the object is pinned. Let's be
> on the safe side and do this immediately before scheduling the
> flip. */
> 
> With or without this or some other comment:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

Submit yours, and I'll look it over ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre