Re: [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Em Sex, 2016-12-23 às 14:43 +0200, Ville Syrjälä escreveu:
> On Fri, Dec 23, 2016 at 10:23:59AM -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.
> > 
> > v2:
> > 
> > But Ville pointed that this only works properly if we have a single
> > commit with multiple CRTCs. If we have multiple parallel commits,
> > each
> > with its own CRTC, we'll end with enable_fbc set to true in more
> > than
> > one plane.
> > 
> > So the solution was to rename enable_fbc to can_enable_fbc. If we
> > just
> > did the rename as the patch was, we'd end up with a single plane
> > with
> > can_enable_fbc on commits involving multiple CRTCs: we'd choose the
> > best one, but we'd still end up with a variable that doesn't 100%
> > reflect reality.
> > 
> > So in the end I adopted Ville's suggestion of the fbc_score
> > variable.
> > And then, instead of checking the score at intel_fbc_choose_crtc()
> > it should be possible to check for the score at intel_fbc_enable().
> > This allows us to simplify intel_fbc_choose_crtc() to the point
> > where
> > it only needs to look at the given plane in order to assign its
> > score
> > instead of looking at every primary plane on the same commit.
> > 
> > We still only set scores of 0 and 1 and we don't really do the
> > score-checking loop.
> > 
> > v3: Rebase.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  3 +-
> >  drivers/gpu/drm/i915/intel_drv.h     |  8 ++--
> >  drivers/gpu/drm/i915/intel_fbc.c     | 85 ++++++++++++----------
> > --------------
> >  3 files changed, 35 insertions(+), 61 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index d8effd4..92527d6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  	if (ret)
> >  		return ret;
> >  
> > -	intel_fbc_choose_crtc(dev_priv, state);
> >  	return calc_watermark_data(state);
> >  }
> >  
> > @@ -14966,6 +14965,8 @@ intel_check_primary_plane(struct drm_plane
> > *plane,
> >  			return ret;
> >  	}
> >  
> > +	intel_fbc_check_plane(state);
> > +
> 
> We have another 'return 0' a little earlier in the function for the
> "plane disable by the user" case. Presumably we'll want to update
> the score in that case as well?

I always assumed the state was zero-allocated, not copied, and int his
case this wouldn't be a problem... Thanks for pointing this to me.

I'll move it up. Can't move up further since we need plane_state-
>visible to be set.


> 
> > 
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 025e4c8..7430082 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -406,6 +406,9 @@ struct intel_plane_state {
> >  	 */
> >  	int scaler_id;
> >  
> > +	/* 0: not suitable for FBC, 1+: suitable for FBC, more is
> > better. */
> > +	unsigned int fbc_score;
> > +
> >  	struct drm_intel_sprite_colorkey ckey;
> >  };
> >  
> > @@ -652,8 +655,6 @@ struct intel_crtc_state {
> >  
> >  	bool ips_enabled;
> >  
> > -	bool enable_fbc;
> > -
> >  	bool double_wide;
> >  
> >  	int pbn;
> > @@ -1535,8 +1536,7 @@ static inline void
> > intel_fbdev_restore_mode(struct drm_device *dev)
> >  #endif
> >  
> >  /* intel_fbc.c */
> > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -			   struct drm_atomic_state *state);
> > +void intel_fbc_check_plane(struct intel_plane_state *plane_state);
> >  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
> >  void intel_fbc_pre_update(struct intel_crtc *crtc,
> >  			  struct intel_crtc_state *crtc_state,
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 26a81a9..ceda6f4 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct
> > drm_i915_private *dev_priv,
> >  }
> >  
> >  /**
> > - * intel_fbc_choose_crtc - select a CRTC to enable FBC on
> > - * @dev_priv: i915 device instance
> > - * @state: the atomic state structure
> > + * intel_fbc_check_plane - check plane for FBC suitability
> > + * @plane_state: the plane state
> >   *
> > - * This function looks at the proposed state for CRTCs and planes,
> > then chooses
> > - * which pipe is going to have FBC by setting intel_crtc_state-
> > >enable_fbc to
> > - * true.
> > + * This function should set fbc_score based on how suitable the
> > plane is for
> > + * FBC. For now the only scores used are 0 and 1.
> >   *
> > - * Later, intel_fbc_enable is going to look for state->enable_fbc
> > and then maybe
> > - * enable FBC for the chosen CRTC. If it does, it will set
> > dev_priv->fbc.crtc.
> > + * We're not changing dev_priv->fbc, so there's no need for the
> > FBC lock.
> >   */
> > -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> > -			   struct drm_atomic_state *state)
> > +void intel_fbc_check_plane(struct intel_plane_state *plane_state)
> >  {
> > -	struct intel_fbc *fbc = &dev_priv->fbc;
> > -	struct drm_plane *plane;
> > -	struct drm_plane_state *plane_state;
> > -	bool crtc_chosen = false;
> > -	int i;
> > +	struct drm_plane *plane = plane_state->base.plane;
> > +	struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> > >base.crtc);
> 
> What happens if we don't have a crtc?

It's been confirmed to me many times that if a plane is being
committed, its CRTC is included in the atomic commit. Was this what you
were talking aboue? Can you give me an example where we wouldn't have a
CRTC?


> > 
> >  
> > -	mutex_lock(&fbc->lock);
> > -
> > -	/* Does this atomic commit involve the CRTC currently tied
> > to FBC? */
> > -	if (fbc->crtc &&
> > -	    !drm_atomic_get_existing_crtc_state(state, &fbc->crtc-
> > >base))
> > -		goto out;
> > +	plane_state->fbc_score = 0;
> >  
> >  	if (!intel_fbc_can_enable(dev_priv))
> > -		goto out;
> > -
> > -	/* Simply choose the first CRTC that is compatible and has
> > a visible
> > -	 * 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);
> > -
> > -		if (!intel_plane_state->base.visible)
> > -			continue;
> > -
> > -		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe !=
> > PIPE_A)
> > -			continue;
> > -
> > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane
> > != PLANE_A)
> > -			continue;
> > -
> > -		intel_crtc_state = to_intel_crtc_state(
> > -			drm_atomic_get_existing_crtc_state(state,
> > &crtc->base));
> > -
> > -		intel_crtc_state->enable_fbc = true;
> > -		crtc_chosen = true;
> > -		break;
> > -	}
> > -
> > -	if (!crtc_chosen)
> > -		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> > +		return;
> > +	if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> > +		return;
> > +	if (fbc_on_plane_a_only(dev_priv) && crtc->plane !=
> > PLANE_A)
> > +		return;
> > +	if (!plane_state->base.visible)
> > +		return;
> >  
> > -out:
> > -	mutex_unlock(&fbc->lock);
> > +	plane_state->fbc_score = 1;
> >  }
> >  
> >  /**
> > @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct
> > drm_i915_private *dev_priv,
> >   * possible. Notice that it doesn't activate FBC. It is valid to
> > call
> >   * intel_fbc_enable multiple times for the same pipe without an
> >   * intel_fbc_disable in the middle, as long as it is deactivated.
> > + *
> > + * In the future this function could make better use of the
> > fbc_score variable.
> > + * We could, for example, loop through all the primary planes
> > involved in the
> > + * atomic commit and only enable FBC for the plane with the best
> > fbc_score. We
> > + * could also try to do some scheme where a plane with better
> > score takes over
> > + * FBC from another plane, but our driver currently can't handle
> > the complexity
> > + * of switching planes on the fly. This would only affect from Gen
> > 4 up to IVB.
> >   */
> >  void intel_fbc_enable(struct intel_crtc *crtc,
> >  		      struct intel_crtc_state *crtc_state,
> > @@ -1130,14 +1101,16 @@ 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->fbc_score == 0);
> >  			WARN_ON(fbc->active);
> >  		}
> >  		goto out;
> >  	}
> >  
> > -	if (!crtc_state->enable_fbc)
> > +	if (plane_state->fbc_score == 0) {
> > +		fbc->no_fbc_reason = "no suitable CRTC for FBC";
> >  		goto out;
> > +	}
> >  
> >  	WARN_ON(fbc->active);
> >  	WARN_ON(fbc->crtc != NULL);
> > -- 
> > 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux