Re: [Intel-gfx] [PATCH 4/6] drm/i915: Precompute can_sagv for each wm level

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

On Thu, Nov 12, 2020 at 03:59:40PM +0200, Lisovskiy, Stanislav wrote:
> On Fri, Nov 06, 2020 at 07:30:40PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > In order to remove intel_atomic_crtc_state_for_each_plane_state()
> > from skl_crtc_can_enable_sagv() we can simply precompute whether
> > each wm level can tolerate the SAGV block time latency or not.
> > 
> > This has the nice side benefit that we remove the duplicated
> > wm level latency calculation. In fact the copy of that code
> > we had in skl_crtc_can_enable_sagv() didn't even handle
> > WaIncreaseLatencyIPCEnabled/Display WA #1141 whereas the copy
> > in skl_compute_plane_wm() did. So now we just have the one
> > copy which handles all the w/as.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c               | 21 +++++++------------
> >  2 files changed, 9 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index b977e70e34d7..8a0276044832 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -686,6 +686,7 @@ struct skl_wm_level {
> >  	u8 plane_res_l;
> >  	bool plane_en;
> >  	bool ignore_lines;
> > +	bool can_sagv;
> >  };
> >  
> >  struct skl_plane_wm {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 85b4bfb02e2e..b789ad78319b 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3873,9 +3873,7 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	struct intel_plane *plane;
> > -	const struct intel_plane_state *plane_state;
> > -	int level, latency;
> > +	enum plane_id plane_id;
> >  
> >  	if (!intel_has_sagv(dev_priv))
> >  		return false;
> > @@ -3886,9 +3884,10 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> >  	if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> >  		return false;
> >  
> > -	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > +	for_each_plane_id_on_crtc(crtc, plane_id) {
> >  		const struct skl_plane_wm *wm =
> > -			&crtc_state->wm.skl.optimal.planes[plane->id];
> > +			&crtc_state->wm.skl.optimal.planes[plane_id];
> > +		int level;
> >  
> >  		/* Skip this plane if it's not enabled */
> >  		if (!wm->wm[0].plane_en)
> > @@ -3899,19 +3898,12 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> >  		     !wm->wm[level].plane_en; --level)
> >  		     { }
> >  
> > -		latency = dev_priv->wm.skl_latency[level];
> > -
> > -		if (skl_needs_memory_bw_wa(dev_priv) &&
> > -		    plane_state->uapi.fb->modifier ==
> > -		    I915_FORMAT_MOD_X_TILED)
> > -			latency += 15;
> > -
> >  		/*
> >  		 * 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)
> > +		if (!wm->wm[level].can_sagv)
> >  			return false;
> >  	}
> 
> Ah yet again that "thing". I wonder tbh, do we even need this per level,
> as we anyway do "to SAGV or not to SAGV" decision, based on all wm levels.

We need it because we don't know which levels will actually be enabled
until later when we've done the ddb allocation.

> 
> Also I remember we even discussed that we wanted some clarification here,
> as for Gen12+ we actually checking only if we can fit wm0 + block time to ddb.

Yeah, it's quite unclear still.

Also atm we enable sagv if all planes have enabled at least one
level with latency>=sagv_block_time, but atm there is no guarantee that
said level is the same for all the planes (since the w/as can
change the latencies in ways that make two planes use different
latencies for the same level). That to me feels a bit broken. I suspect
we should be looking to make sure the highest common level for all the
planes can tolerate sagv, because I think that's the highest level that
can actually be used by the hw.

So eg. we could have
plane A wm0/en=yes,sagv=no, wm1/en=yes,sagv=no,  wm2/en=yes,sagv=yes
plane B wm0/en=yes,sagv=no, wm1/en=yes,sagv=yes, wm2/en=no

and the hardware will never actully use wm2 (I think) which
would be required for sagv to be safe with plane A. But I
think I need to double check the hardware behaviour to be sure
I'm thinking this correctly.

> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> 
> >  
> > @@ -5375,6 +5367,9 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
> >  	/* Bspec says: value >= plane ddb allocation -> invalid, hence the +1 here */
> >  	result->min_ddb_alloc = max(min_ddb_alloc, res_blocks) + 1;
> >  	result->plane_en = true;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		result->can_sagv = latency >= dev_priv->sagv_block_time_us;
> >  }
> >  
> >  static void
> > -- 
> > 2.26.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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