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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
