Em Sex, 2016-12-23 às 14:43 +0200, Ville Syrjälä escreveu:
> 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?
I always assumed the state was zero-allocated, not copied, and int his
case this wouldn't be a problem... Thanks for pointing this to me.
I'll move it up. Can't move up further since we need plane_state-
>visible to be set.
>
> >
> > 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?
It's been confirmed to me many times that if a plane is being
committed, its CRTC is included in the atomic commit. Was this what you
were talking aboue? Can you give me an example where we wouldn'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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
