[Intel-gfx] [PATCH v4 1/2] drm/i915: Refactor intel_can_enable_sagv

[James Ausmus]

On Fri, Oct 18, 2019 at 01:34:35AM -0700, Lisovskiy, Stanislav wrote:
> On Thu, 2019-10-17 at 14:53 -0700, James Ausmus wrote:
> > On Tue, Oct 15, 2019 at 04:50:12PM +0300, Stanislav Lisovskiy wrote:
> > > Currently intel_can_enable_sagv function contains
> > > a mix of workarounds for different platforms
> > > some of them are not valid for gens >= 11 already,
> > > so lets split it into separate functions.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala at intel.com>
> > > Cc: James Ausmus <james.ausmus at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 73
> > > +++++++++++++++++++++++++++++++--
> > >  1 file changed, 70 insertions(+), 3 deletions(-)
> > > 
> > > +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_device *dev = state->base.dev;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +	struct intel_crtc *crtc;
> > > +	struct intel_crtc_state *new_crtc_state;
> > > +	int level, latency;
> > > +	int i;
> > > +	int plane_id;
> > > +
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * If there are no active CRTCs, no additional checks need be
> > > performed
> > > +	 */
> > > +	if (hweight8(state->active_pipes) == 0)
> > > +		return true;
> > > +
> > > +	for_each_new_intel_crtc_in_state(state, crtc,
> > > +					     new_crtc_state, i) {
> > > +
> > > +		if (crtc->base.state->adjusted_mode.flags &
> > > DRM_MODE_FLAG_INTERLACE)
> > > +			return false;
> > > +
> > > +		if (!new_crtc_state->base.enable)
> > > +			continue;
> > > +
> > > +		for_each_plane_id_on_crtc(crtc, plane_id) {
> > > +			struct skl_plane_wm *wm =
> > > +				&new_crtc_state-
> > > >wm.skl.optimal.planes[plane_id];
> > > +
> > > +			/* Skip this plane if it's not enabled */
> > > +			if (!wm->wm[0].plane_en)
> > > +				continue;
> > > +
> > > +			/* Find the highest enabled wm level for this
> > > plane */
> > > +			for (level = ilk_wm_max_level(dev_priv);
> > > +			     !wm->wm[level].plane_en; --level)
> > > +			     { }
> > > +
> > > +			latency = dev_priv->wm.skl_latency[level];
> > 
> > This isn't exactly the same for TGL. From BSpec 49325, "Calculate
> > watermark level 0 with level 0 latency + SAGV block time. If the
> > result
> > can be supported (does not exceed maximum), then the plane can
> > tolerate
> > SAGV", so I think it can be simplified for Gen12+ by not having to
> > loop
> > through all the wm levels.
> > 
> > -James
> 
> Yes, we discussed that with Ville as I understood, properly doing
> that for TGL requires changes to watermark/ddb allocation algorithm,
> so that we check if dbuf can fit Level 0 watermark increased by SAGV
> block time. I was not sure whether should I add this patch to this
> series or proceed with that separately, probably this is worth adding
> here - however then it would require changes to watermark algorithm,
> as we need to recalculate min_ddb_alloc for Level 0 latency + SAGV
> and then check if we fit into DBuf again.
> So basically we'll have to calculate watermark level 0 twice - once for
> SAGV enabled and once for SAGV disabled case, I would probably do it 
> already during watermark calculations not when checking bandwidth and
> then set some "SAGV can enable" flag, which will be then used by
> intel_can_enable_sagv, so that we act accordingly when
> bandwidth check and restricting qgv points happens. For pre-gen 12 it
> will otherwise use the old algorithm.

OK, that works then. With the whitespace fix added:

Reviewed-by: James Ausmus <james.ausmus at intel.com>

> 
> > 
> > > +
> > > +			/*
> > > +			 * If any of the planes on this pipe don't
> > > enable wm levels that
> > > +			 * incur memory latencies higher than
> > > sagv_block_time_us we
> > > +			 * can't enable SAGV.
> > > +			 */
> > > +			if (latency < dev_priv->sagv_block_time_us)
> > > +				return false;
> > > +		}
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_device *dev = state->base.dev;
> > > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > +		return icl_can_enable_sagv(state);
> > > +
> > > +	return skl_can_enable_sagv(state);
> > > +}
> > > +
> > >  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> > >  			      const struct intel_crtc_state
> > > *crtc_state,
> > >  			      const u64 total_data_rate,
> > > -- 
> > > 2.17.1
> > >