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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre