Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 6/7] drm/i915/fbc: move from crtc_state->enable_fbc to plane_state->enable_fbc
Date: Mon, 14 Nov 2016 22:26:42 +0200	[thread overview]
Message-ID: <20161114202642.GG31595@intel.com> (raw)
In-Reply-To: <1478897399.19391.61.camel@intel.com>

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@linux.intel.com>
> > > > > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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_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_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
> > 
> > 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx