Re: [PATCH v2 4/4] drm/i915/fbc: Use PLANE_HAS_FENCE to determine if the plane is fenced

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 20, 2018 at 01:42:08PM +0000, Chris Wilson wrote:
> Rather than trusting the cached value of plane_state->vma->fence to
> imply whether the plane_state itself holds a reference on the
> framebuffer's fence, use the information provided in the
> plane_state->flags (PLANE_HAS_FENCE). Note that we still assume that FBC
> is entirely bounded by the plane_state active life span; it's not clear
> if that is a safe assumption.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20575b3ee406..848dc8f68b47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -667,6 +667,7 @@ struct intel_fbc {
>  	 */
>  	struct intel_fbc_state_cache {
>  		struct i915_vma *vma;
> +		unsigned long flags;
>  
>  		struct {
>  			unsigned int mode_flags;
> @@ -705,6 +706,7 @@ struct intel_fbc {
>  	 */
>  	struct intel_fbc_reg_params {
>  		struct i915_vma *vma;
> +		unsigned long flags;
>  
>  		struct {
>  			enum pipe pipe;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d7d1ac79c38a..f66f6fb5743d 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -183,7 +183,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
>  	else
>  		dpfc_ctl |= DPFC_CTL_LIMIT_1X;
>  
> -	if (params->vma->fence) {
> +	if (params->flags & PLANE_HAS_FENCE) {

These checks seem redundant now that we shouldn't even try to enable fbc
without the fence. But they're not harming anyone (well, apart from making
the code a bit inconsistent by not having the check in the fbc1 codepath).

But the patch is 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
regardless

>  		dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id;
>  		I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset);
>  	} else {
> @@ -241,7 +241,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	if (params->vma->fence) {
> +	if (params->flags & PLANE_HAS_FENCE) {
>  		dpfc_ctl |= DPFC_CTL_FENCE_EN;
>  		if (IS_GEN5(dev_priv))
>  			dpfc_ctl |= params->vma->fence->id;
> @@ -324,7 +324,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	if (params->vma->fence) {
> +	if (params->flags & PLANE_HAS_FENCE) {
>  		dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN;
>  		I915_WRITE(SNB_DPFC_CTL_SA,
>  			   SNB_CPU_FENCE_ENABLE |
> @@ -753,6 +753,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  
>  	cache->vma = NULL;
> +	cache->flags = 0;
>  
>  	cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags;
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> @@ -778,6 +779,9 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc,
>  	cache->fb.stride = fb->pitches[0];
>  
>  	cache->vma = plane_state->vma;
> +	cache->flags = plane_state->flags;
> +	if (WARN_ON(cache->flags & PLANE_HAS_FENCE && !cache->vma->fence))
> +		cache->flags &= ~PLANE_HAS_FENCE;
>  }
>  
>  static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> @@ -816,7 +820,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  	 * so have no fence associated with it) due to aperture constaints
>  	 * at the time of pinning.
>  	 */
> -	if (!cache->vma->fence) {
> +	if (!(cache->flags & PLANE_HAS_FENCE)) {
>  		fbc->no_fbc_reason = "framebuffer not tiled or fenced";
>  		return false;
>  	}
> @@ -897,6 +901,7 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
>  	memset(params, 0, sizeof(*params));
>  
>  	params->vma = cache->vma;
> +	params->flags = cache->flags;
>  
>  	params->crtc.pipe = crtc->pipe;
>  	params->crtc.i9xx_plane = to_intel_plane(crtc->base.primary)->i9xx_plane;
> -- 
> 2.16.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx