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.syrj...@linux.intel.com>
> > Reported-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zan...@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(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. Otherwise, we only pick one CRTC
due to the "break;" statement after setting plane_state->enable_fbc to
true.
>
> >
> > 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
