Re: [PATCH 11/25] drm/i915/fbc: fix the FBC state checking code

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Seg, 2016-08-15 às 21:55 +0100, Chris Wilson escreveu:
> On Tue, Jan 19, 2016 at 11:35:44AM -0200, Paulo Zanoni wrote:
> > 
> > We'll now call intel_fbc_pre_update instead of intel_fbc_deactivate
> > during atomic commits. This will continue to guarantee that we
> > deactivate FBC and it will also update the state checking
> > structures
> > at the correct time. Then, later, at the point where we were
> > calling
> > intel_fbc_update, we'll only need to call intel_fbc_post_update.
> > 
> > Also add the proper warnings in case we don't have the appropriate
> > locks. Daniel mentioned the warnings will have to be removed for
> > async
> > commits, but let's keep them here while we can.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++++------
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 +++++---
> >  drivers/gpu/drm/i915/intel_fbc.c     | 33 ++++++++++++++++++----
> > -----------
> >  3 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index baab41046..baa4cc9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> 
> > 
> > @@ -11733,7 +11733,7 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> >  			  to_intel_plane(primary)-
> > >frontbuffer_bit);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> > -	intel_fbc_deactivate(intel_crtc);
> > +	intel_fbc_pre_update(intel_crtc);
> >  	intel_frontbuffer_flip_prepare(dev,
> >  				       to_intel_plane(primary)-
> > >frontbuffer_bit);
> >  
> > +void intel_fbc_pre_update(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev-
> > >dev_private;
> >  	struct intel_fbc *fbc = &dev_priv->fbc;
> >  
> > -	WARN_ON(!mutex_is_locked(&fbc->lock));
> > +	if (!fbc_supported(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&fbc->lock);
> >  
> >  	if (!multiple_pipes_ok(dev_priv)) {
> >  		set_no_fbc_reason(dev_priv, "more than one pipe
> > active");
> > @@ -907,15 +915,17 @@ static void intel_fbc_pre_update(struct
> > intel_crtc *crtc)
> >  	}
> >  
> >  	if (!fbc->enabled || fbc->crtc != crtc)
> > -		return;
> > +		goto unlock;
> >  
> >  	intel_fbc_update_state_cache(crtc);
> >  
> >  deactivate:
> >  	__intel_fbc_deactivate(dev_priv);
> > +unlock:
> > +	mutex_unlock(&fbc->lock);
> >  }
> 
> 
> So this change is broken as intel_fbc_pre_update() depends upon state
> derived from the pinned framebuffer object but is being called by
> intel_crtc_page_flip() before that object is pinned with
> intel_pin_and_fence_fb - i.e. state such as the gtt_offset or the
> fence
> for the object are incorrect, and even trying to look at them can
> cause
> an oops.

Nope:
$ git show
1eb52238a5f5b6a3f497b47e6da39ccfebe6b878:drivers/gpu/drm/i915/intel_dis
play.c

Back when the commit was merged, the pre_update() call was done after
pin_and_fence_fb(). It looks like some later commit introduced the
problem you point.

Still, looks like we have code to fix.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx