Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

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

On Fri, Dec 23, 2016 at 10:23:59AM -0200, Paulo Zanoni wrote:
> Ville pointed out that intel_fbc_choose_crtc() is iterating over all
> planes instead of just the primary planes. There are no real
> consequences of this problem for HSW+, and for the other platforms it
> just means that in some obscure multi-screen cases we'll keep FBC
> disabled when we could have enabled it. Still, iterating over all
> planes doesn't seem to be the best thing to do.
> 
> My initial idea was to just add a check for plane->type and be done,
> but then I realized that in commits not involving the primary plane we
> would reset crtc_state->enable_fbc back to false even when FBC is
> enabled. That also wouldn't result in a bug due to the way the
> enable_fbc variable is checked, but, still, our code can be better
> than this.
> 
> So I went for the solution that involves tracking enable_fbc in the
> primary plane state instead of the CRTC state. This way, if a commit
> doesn't involve the primary plane for the CRTC we won't be resetting
> enable_fbc back to false, so the variable will always reflect the
> reality. And this change also makes more sense since FBC is actually
> tied to the single plane and not the full pipe. As a bonus, we only
> iterate over the CRTCs instead of iterating over all planes.
> 
> v2:
> 
> But Ville pointed that this only works properly if we have a single
> commit with multiple CRTCs. If we have multiple parallel commits, each
> with its own CRTC, we'll end with enable_fbc set to true in more than
> one plane.
> 
> So the solution was to rename enable_fbc to can_enable_fbc. If we just
> did the rename as the patch was, we'd end up with a single plane with
> can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> best one, but we'd still end up with a variable that doesn't 100%
> reflect reality.
> 
> So in the end I adopted Ville's suggestion of the fbc_score variable.
> And then, instead of checking the score at intel_fbc_choose_crtc()
> it should be possible to check for the score at intel_fbc_enable().
> This allows us to simplify intel_fbc_choose_crtc() to the point where
> it only needs to look at the given plane in order to assign its score
> instead of looking at every primary plane on the same commit.
> 
> We still only set scores of 0 and 1 and we don't really do the
> score-checking loop.
> 
> v3: Rebase.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
>  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++------------------------
>  3 files changed, 35 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d8effd4..92527d6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	intel_fbc_choose_crtc(dev_priv, state);
>  	return calc_watermark_data(state);
>  }
>  
> @@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			return ret;
>  	}
>  
> +	intel_fbc_check_plane(state);
> +

We have another 'return 0' a little earlier in the function for the
"plane disable by the user" case. Presumably we'll want to update
the score in that case as well?

>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 025e4c8..7430082 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -406,6 +406,9 @@ struct intel_plane_state {
>  	 */
>  	int scaler_id;
>  
> +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */
> +	unsigned int fbc_score;
> +
>  	struct drm_intel_sprite_colorkey ckey;
>  };
>  
> @@ -652,8 +655,6 @@ struct intel_crtc_state {
>  
>  	bool ips_enabled;
>  
> -	bool enable_fbc;
> -
>  	bool double_wide;
>  
>  	int pbn;
> @@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
>  #endif
>  
>  /* intel_fbc.c */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state);
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
>  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
>  			  struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 26a81a9..ceda6f4 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> - * @dev_priv: i915 device instance
> - * @state: the atomic state structure
> + * intel_fbc_check_plane - check plane for FBC suitability
> + * @plane_state: the plane state
>   *
> - * This function looks at the proposed state for CRTCs and planes, then chooses
> - * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to
> - * true.
> + * This function should set fbc_score based on how suitable the plane is for
> + * FBC. For now the only scores used are 0 and 1.
>   *
> - * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe
> - * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc.
> + * We're not changing dev_priv->fbc, so there's no need for the FBC lock.
>   */
> -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> -			   struct drm_atomic_state *state)
> +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
>  {
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> -	bool crtc_chosen = false;
> -	int i;
> +	struct drm_plane *plane = plane_state->base.plane;
> +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> +	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);

What happens if we don't have a crtc?

>  
> -	mutex_lock(&fbc->lock);
> -
> -	/* Does this atomic commit involve the CRTC currently tied to FBC? */
> -	if (fbc->crtc &&
> -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base))
> -		goto out;
> +	plane_state->fbc_score = 0;
>  
>  	if (!intel_fbc_can_enable(dev_priv))
> -		goto out;
> -
> -	/* Simply choose the first CRTC that is compatible and has a visible
> -	 * plane. We could go for fancier schemes such as checking the plane
> -	 * size, but this would just affect the few platforms that don't tie FBC
> -	 * to pipe or plane A. */
> -	for_each_plane_in_state(state, plane, plane_state, i) {
> -		struct intel_plane_state *intel_plane_state =
> -			to_intel_plane_state(plane_state);
> -		struct intel_crtc_state *intel_crtc_state;
> -		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
> -
> -		if (!intel_plane_state->base.visible)
> -			continue;
> -
> -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> -			continue;
> -
> -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> -			continue;
> -
> -		intel_crtc_state = to_intel_crtc_state(
> -			drm_atomic_get_existing_crtc_state(state, &crtc->base));
> -
> -		intel_crtc_state->enable_fbc = true;
> -		crtc_chosen = true;
> -		break;
> -	}
> -
> -	if (!crtc_chosen)
> -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> +		return;
> +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> +		return;
> +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> +		return;
> +	if (!plane_state->base.visible)
> +		return;
>  
> -out:
> -	mutex_unlock(&fbc->lock);
> +	plane_state->fbc_score = 1;
>  }
>  
>  /**
> @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
>   * possible. Notice that it doesn't activate FBC. It is valid to call
>   * intel_fbc_enable multiple times for the same pipe without an
>   * intel_fbc_disable in the middle, as long as it is deactivated.
> + *
> + * In the future this function could make better use of the fbc_score variable.
> + * We could, for example, loop through all the primary planes involved in the
> + * atomic commit and only enable FBC for the plane with the best fbc_score. We
> + * could also try to do some scheme where a plane with better score takes over
> + * FBC from another plane, but our driver currently can't handle the complexity
> + * of switching planes on the fly. This would only affect from Gen 4 up to IVB.
>   */
>  void intel_fbc_enable(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *crtc_state,
> @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>  	if (fbc->enabled) {
>  		WARN_ON(fbc->crtc == NULL);
>  		if (fbc->crtc == crtc) {
> -			WARN_ON(!crtc_state->enable_fbc);
> +			WARN_ON(plane_state->fbc_score == 0);
>  			WARN_ON(fbc->active);
>  		}
>  		goto out;
>  	}
>  
> -	if (!crtc_state->enable_fbc)
> +	if (plane_state->fbc_score == 0) {
> +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
>  		goto out;
> +	}
>  
>  	WARN_ON(fbc->active);
>  	WARN_ON(fbc->crtc != NULL);
> -- 
> 2.7.4

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