[Intel-gfx] [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc
Paulo Zanoni
paulo.r.zanoni at intel.com
Fri Nov 11 19:57:28 UTC 2016
Em Sex, 2016-11-11 às 21:13 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 05:01:54PM -0200, Paulo Zanoni wrote:
> >
> > Em Sex, 2016-11-11 às 20:51 +0200, Ville Syrjälä escreveu:
> > >
> > > On Fri, Nov 11, 2016 at 02:57:40PM -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.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_drv.h | 4 ++--
> > > > drivers/gpu/drm/i915/intel_fbc.c | 36 +++++++++++++++++++-----
> > > > ----
> > > > --------
> > > > 2 files changed, 21 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 003afb8..025cb74 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -403,6 +403,8 @@ struct intel_plane_state {
> > > > int scaler_id;
> > > >
> > > > struct drm_intel_sprite_colorkey ckey;
> > > > +
> > > > + bool enable_fbc;
> > > > };
> > > >
> > > > struct intel_initial_plane_config {
> > > > @@ -648,8 +650,6 @@ struct intel_crtc_state {
> > > >
> > > > bool ips_enabled;
> > > >
> > > > - bool enable_fbc;
> > > > -
> > > > bool double_wide;
> > > >
> > > > bool dp_encoder_is_mst;
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > index b095175..fc4ac57 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > @@ -1055,16 +1055,17 @@ void intel_fbc_choose_crtc(struct
> > > > drm_i915_private *dev_priv,
> > > > struct drm_atomic_state *state)
> > > > {
> > > > struct intel_fbc *fbc = &dev_priv->fbc;
> > > > - struct drm_plane *plane;
> > > > - struct drm_plane_state *plane_state;
> > > > + struct drm_crtc *crtc;
> > > > + struct drm_crtc_state *crtc_state;
> > > > bool crtc_chosen = false;
> > > > int i;
> > > >
> > > > mutex_lock(&fbc->lock);
> > > >
> > > > - /* Does this atomic commit involve the CRTC currently
> > > > tied
> > > > to FBC? */
> > > > + /* Does this atomic commit involve the plane currently
> > > > tied to FBC? */
> > > > if (fbc->crtc &&
> > > > - !drm_atomic_get_existing_crtc_state(state, &fbc-
> > > > >crtc-
> > > > >
> > > > > base))
> > > > + !drm_atomic_get_existing_plane_state(state,
> > > > + fbc->crtc-
> > > > >
> > > > > base.primary))
> > > > goto out;
> > > >
> > > > if (!intel_fbc_can_enable(dev_priv))
> > > > @@ -1074,25 +1075,26 @@ void intel_fbc_choose_crtc(struct
> > > > drm_i915_private *dev_priv,
> > > > * 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);
> > > > + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > > + struct intel_plane_state *plane_state =
> > > > to_intel_plane_state(
> > > > + drm_atomic_get_existing_plane_state(st
> > > > ate,
> > > > + cr
> > > > tc-
> > > > >
> > > > > primary));
> > > > + struct intel_crtc *intel_crtc =
> > > > to_intel_crtc(crtc);
> > > >
> > > > - if (!intel_plane_state->base.visible)
> > > > + if (!plane_state)
> > > > continue;
> > > >
> > > > - if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe
> > > > !=
> > > > PIPE_A)
> > > > + if (!plane_state->base.visible)
> > > > continue;
> > > >
> > > > - if (fbc_on_plane_a_only(dev_priv) && crtc-
> > > > >plane
> > > > != PLANE_A)
> > > > + if (fbc_on_pipe_a_only(dev_priv) &&
> > > > intel_crtc-
> > > > >
> > > > > pipe != PIPE_A)
> > > > continue;
> > > >
> > > > - intel_crtc_state = to_intel_crtc_state(
> > > > - drm_atomic_get_existing_crtc_state(sta
> > > > te,
> > > > &crtc->base));
> > > > + if (fbc_on_plane_a_only(dev_priv) &&
> > > > + intel_crtc->plane != PLANE_A)
> > > > + continue;
> > > >
> > > > - intel_crtc_state->enable_fbc = true;
> > > > + plane_state->enable_fbc = true;
> > >
> > > So looking at this whole thing, I can't see anything that would
> > > prevent
> > > enable_fbc being true for multiple primary planes at the same
> > > time
> > > Well, apart from the whole "we enable it only for platforms that
> > > can
> > > only do
> > > pipe A" thing.
> > >
> > > So what happens in that case? FBC just ends up getting enabling
> > > on
> > > one of the pipes based on the order intel_fbc_enable() gets
> > > called,
> > > or something?
> >
> > The first check of intel_fbc_choose_crtc() is supposed to prevent
> > this
> > case: if fbc->crtc->primary is not included in the commit we just
> > return without selecting any plane.
>
> The fbc->crtc thing only works if intel_fbc_enable() was already
> called
> for some crtc. But what it it wasn't?
>
> >
> > Otherwise, we only pick one CRTC
> > due to the "break;" statement after setting plane_state->enable_fbc
> > to
> > true.
>
> Only one per atomic operation. But what if there are several
> happening
> in parallel on different crtcs?
I see your point now. Yeah, we'll end up with
plane_state.enable_fbc=true for two different planes. Later, the first
one to call intel_fbc_enable() will win, and the others will be
ignored, so we'll indeed end up with plane states having
enable_fbc=true but FBC not enabled by them. Not a real bug, I would
still like to avoid this confusion.
The simplest solution I can think would be to just
s/plane_state.enable_fbc/plane_state.can_enable_fbc/ and just let the
first one to call intel_fbc_enable() win... And then, if we ever decide
to enable FBC on the older platforms, we can choose to maybe implement
a better method (and fix all the possible FBC problems for those
platforms). Is this solution worth a R-B from you?
If you have any better idea here, feel free to suggest it. I'm not sure
it's possible to come up with something much better due to all the
parallelism involved. Making one CRTC "take over" FBC from the other is
something I'd really love to avoid.
Thanks,
Paulo
>
> >
> >
> > >
> > >
> > > >
> > > >
> > > > crtc_chosen = true;
> > > > break;
> > > > }
> > > > @@ -1130,13 +1132,13 @@ 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->enable_fbc);
> > > > WARN_ON(fbc->active);
> > > > }
> > > > goto out;
> > > > }
> > > >
> > > > - if (!crtc_state->enable_fbc)
> > > > + if (!plane_state->enable_fbc)
> > > > goto out;
> > > >
> > > > WARN_ON(fbc->active);
> > > > --
> > > > 2.7.4
> > >
>
More information about the Intel-gfx
mailing list