On Mon, Nov 14, 2016 at 06:49:22PM -0200, Paulo Zanoni wrote:
> 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.syrj...@linux.intel.com>
> > > > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.c
> > > > > > > > > om>
> > > > > > > > > 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_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.
Well you already have to disable FBC in the hardware when you flip to
a buffer that can't be used with FBC. So that part is there. I admit
there is a little bit of extra complication from making sure you don't
deallocate the cfb before FBC has finished, but that complication is
neatly encapsulated in the FBC code. From the flip/modeset path there
should be just two function calls around the update to signal the FBC
machinery.
> 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.
As I see it the FBC machinery effectively just needs three states:
- cfb deallocated + fbc disabled
- cfb allocated + fbc disabled
- cfb allocated + fbc enabled
and you just move between these states, with some synchronization in
between. I guess you may want to consider enabling/disabling as two
more states which makes the synchronization more clear.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
