Em Seg, 2016-11-14 às 22:26 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 11, 2016 at 06:49:59PM -0200, Paulo Zanoni wrote:
> >
> > Em Sex, 2016-11-11 às 22:24 +0200, Ville Syrjälä escreveu:
> > >
> > > On Fri, Nov 11, 2016 at 05:57:28PM -0200, Paulo Zanoni wrote:
> > > >
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> > > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > > > > > om>
> > > > > > > > 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_sta
> > > > > > > > te);
> > > > > > > > - 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_
> > > > > > > > stat
> > > > > > > > e(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_s
> > > > > > > > tate
> > > > > > > > (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
> > >
> > > Maybe something like "fbc_score"? ;)
> >
> > The design of the current function was supposed to allow Ville to
> > implement his fbc_score in case he wanted. But this certainly
> > didn't
> > take into account multiple parallel commits: it would only work if
> > multiple CRTCs were included in the same commit (as you just
> > pointed
> > today).
> >
> > But then: if we're having separate parallel commits, when would we
> > be
> > able to loop through the scores to only actually enable FBC on the
> > best
> > score?
> >
> > For example, if we do two parallel atomic_check()s and end with
> > plane_a_score=1 and plane_b_score=2, then later we do A's commit()
> > and
> > call intel_fbc_enable() for it, how do we conclude that we
> > shouldn't
> > enable FBC for plane A? We're not even sure if plane B is going to
> > actually commit the plane state it calculated (maybe it was
> > check_only).
> >
> > And then, if we decide to only compute everything during commit()
> > instead of check(), we'll just also end up enabling FBC for plane A
> > since A's commit() will run first and we'll have no idea that B's
> > commit is incoming.
> >
> > The only option would be to disable FBC for plane A and enable for
> > plane B during B's commit. But I'm not looking forward to implement
> > this right now.
>
> All the enable/disable should be totally async and so you should be
> able to kick them off from anywhere. All the state, including the
> scores,
> would be protected by the fbc mutex. I had a vblank worker type of
> thing for this purpose in my patches.
As far as I remember, the complicated part here was related to the CFB
allocation/deallocation. If we disable FBC while the pipe is running we
need to wait for a vblank before we deallocate the CFB, otherwise the
HW is going to keep writing to the stolen memory. While we certainly
can adjust to this, in my FBC reworks I simplified this a lot, and now
we only enable/disable the CFB during crtc_{en,dis}able, so we only
ever free the CFB while the pipe is off, and we don't
deallocate+reallocate it at every single page flip.
The other complicated part is that if we try to allocate the new CFB
for the new plane without first freeing the old one (waiting for the
vblank on the other CRTC) we'll probably get an -ENOMEM from the stolen
mm. On the other hand, if we actually do all the vblank the waits,
well, we'll have to implement the code to properly wait for everything.
And while we're doing the waits, the world may change, new modesets may
happen, we may have to abort or change our minds, and handling all
these cases is there things get complicating.
And we have intel_fbc_work_fn() to help.
Anyway, it is possible to do what you're suggesting. It's just that a
lot of simple things will become complicated.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
