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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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_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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx