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@xxxxxxxxxxxxxxx>
> > > Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > > 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(state,
> > > + crtc-
> > > >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(state,
> > > &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?
>
> >
> > >
> > > 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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
