diff mbox

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

Message ID 1453210558-7875-12-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R Jan. 19, 2016, 1:35 p.m. UTC
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(-)

Comments

Chris Wilson Aug. 15, 2016, 8:55 p.m. UTC | #1
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.
-Chris
Zanoni, Paulo R Aug. 15, 2016, 10:47 p.m. UTC | #2
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

>
Chris Wilson Aug. 16, 2016, 7:36 a.m. UTC | #3
On Mon, Aug 15, 2016 at 10:47:18PM +0000, Zanoni, Paulo R wrote:
> Em Seg, 2016-08-15 às 21:55 +0100, Chris Wilson escreveu:
> > 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.

Hmm, 5a21b6650a239ebc020912968a44047701104159 ?

> Still, looks like we have code to fix.

Well the good news is that answers the question of whether that code is
intended to be safe under the struct_mutex as it once previously was.

So we can just rearrange the code to do the pre_update after the pin?
-Chris
diff mbox

Patch

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
@@ -4803,7 +4803,7 @@  static void intel_post_plane_update(struct intel_crtc *crtc)
 		intel_update_watermarks(&crtc->base);
 
 	if (atomic->update_fbc)
-		intel_fbc_update(crtc);
+		intel_fbc_post_update(crtc);
 
 	if (atomic->post_enable_primary)
 		intel_post_enable_primary(&crtc->base);
@@ -4819,8 +4819,8 @@  static void intel_pre_plane_update(struct intel_crtc *crtc)
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 
-	if (atomic->disable_fbc)
-		intel_fbc_deactivate(crtc);
+	if (atomic->update_fbc)
+		intel_fbc_pre_update(crtc);
 
 	if (crtc->atomic.disable_ips)
 		hsw_disable_ips(crtc);
@@ -10935,7 +10935,7 @@  static void intel_unpin_work_fn(struct work_struct *__work)
 	mutex_unlock(&dev->struct_mutex);
 
 	intel_frontbuffer_flip_complete(dev, to_intel_plane(primary)->frontbuffer_bit);
-	intel_fbc_update(crtc);
+	intel_fbc_post_update(crtc);
 	drm_framebuffer_unreference(work->old_fb);
 
 	BUG_ON(atomic_read(&crtc->unpin_work_count) == 0);
@@ -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);
 
@@ -11928,7 +11928,6 @@  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	case DRM_PLANE_TYPE_PRIMARY:
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
-		intel_crtc->atomic.disable_fbc = true;
 		intel_crtc->atomic.update_fbc = true;
 
 		if (turn_off) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 059b46e..986644d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -564,16 +564,17 @@  struct intel_mmio_flip {
  */
 struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
-	bool disable_fbc;
 	bool disable_ips;
 	bool pre_disable_primary;
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool wait_vblank;
-	bool update_fbc;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
+
+	/* Sleepable operations to perform before and after commit */
+	bool update_fbc;
 };
 
 struct intel_crtc {
@@ -1353,7 +1354,8 @@  static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 /* intel_fbc.c */
 bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_deactivate(struct intel_crtc *crtc);
-void intel_fbc_update(struct intel_crtc *crtc);
+void intel_fbc_pre_update(struct intel_crtc *crtc);
+void intel_fbc_post_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
 void intel_fbc_enable(struct intel_crtc *crtc);
 void intel_fbc_disable(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 2983bcd..4f2133e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -508,6 +508,7 @@  static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->gen > 4)
 		return true;
 
+	/* FIXME: we don't have the appropriate state locks to do this here. */
 	for_each_pipe(dev_priv, pipe) {
 		crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
@@ -730,12 +731,16 @@  static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
 	struct intel_fbc_state_cache *cache = &fbc->state_cache;
-	struct intel_crtc_state *crtc_state = crtc->config;
+	struct intel_crtc_state *crtc_state =
+		to_intel_crtc_state(crtc->base.state);
 	struct intel_plane_state *plane_state =
 		to_intel_plane_state(crtc->base.primary->state);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	struct drm_i915_gem_object *obj;
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->base.mutex));
+	WARN_ON(!drm_modeset_is_locked(&crtc->base.primary->mutex));
+
 	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		cache->crtc.hsw_bdw_pixel_rate =
@@ -894,12 +899,15 @@  static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
 	return memcmp(params1, params2, sizeof(*params1)) == 0;
 }
 
-static void intel_fbc_pre_update(struct intel_crtc *crtc)
+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);
 }
 
-static void intel_fbc_post_update(struct intel_crtc *crtc)
+static void __intel_fbc_post_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
@@ -948,13 +958,7 @@  static void intel_fbc_post_update(struct intel_crtc *crtc)
 	fbc->no_fbc_reason = "FBC enabled (active or scheduled)";
 }
 
-/*
- * intel_fbc_update - activate/deactivate FBC as needed
- * @crtc: the CRTC that triggered the update
- *
- * This function reevaluates the overall state and activates or deactivates FBC.
- */
-void intel_fbc_update(struct intel_crtc *crtc)
+void intel_fbc_post_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
@@ -963,8 +967,7 @@  void intel_fbc_update(struct intel_crtc *crtc)
 		return;
 
 	mutex_lock(&fbc->lock);
-	intel_fbc_pre_update(crtc);
-	intel_fbc_post_update(crtc);
+	__intel_fbc_post_update(crtc);
 	mutex_unlock(&fbc->lock);
 }
 
@@ -1018,7 +1021,7 @@  void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
 		else
-			intel_fbc_post_update(fbc->crtc);
+			__intel_fbc_post_update(fbc->crtc);
 	}
 
 	mutex_unlock(&fbc->lock);