On Fri, Dec 23, 2016 at 11:23:56AM -0200, Paulo Zanoni wrote:
> 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.
Maybe we just want to clear the score to 0 when visible==false?
>
>
> >
> > >
> > > 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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
