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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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. 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