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@xxxxxxxxxxxxxxx>
> Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
