Re: [Intel-gfx] [PATCH 4/4] drm/i915: Don't allocate extra ddb during async flip for DG2

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, Jan 21, 2022 at 02:06:12PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 21, 2022 at 10:06:15AM +0200, Stanislav Lisovskiy wrote:
> > In terms of async flip optimization we don't to allocate
> > extra ddb space, so lets skip it.
> > 
> > v2: - Extracted min ddb async flip check to separate function
> >       (Ville Syrjälä)
> >     - Used this function to prevent false positive WARN
> >       to be triggered(Ville Syrjälä)
> > 
> > v3: - Renamed dg2_need_min_ddb to need_min_ddb thus making
> >       it more universal.
> >     - Also used DISPLAY_VER instead of IS_DG2(Ville Syrjälä)
> >     - Use rate = 0 instead of just setting extra = 0, thus
> >       letting other planes to use extra ddb and avoiding WARN
> >       (Ville Syrjälä)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5fb022a2a4d7..18fb35c480ef 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5118,6 +5118,12 @@ static bool icl_need_wm1_wa(struct drm_i915_private *i915,
> >  	       (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
> >  }
> >  
> > +static bool needs_min_ddb(struct drm_i915_private *i915,
> > +			  struct intel_crtc_state *crtc_state)
> 
> s/needs/use/ to match the wm0 counterpart?
> 
> Could use a comment as well perhaps, or maybe just put this right
> next to the wm0 counterpart so the reader can see both together and
> make the connection.
> 
> Hmm. Actually I think this would also need the plane->async_flip
> check here too or else we'll drop all the planes to min ddb
> instead of just the plane doing async flips.
> 
> Oh, and I think we need this same thing when calculating the
> total_data_rate or else the numbers won't match.

Yes, there seems to be a problem with that approach, we use ratio
from data plane_data_rate/total_data_rate to determine how we split
extra ddb blocks, however if plane data rate can be just set as 0
here localle, total_data_rate is obtained from crtc_state->plane_data_rate,
which is being calculated first.
So if we trick icl_get_total_relative_data_rate function to calculate
total_data_rate corresponding to rate = 0, we will then have 
crtc_state->plane_data_rate[plane_id] set to 0, which is probably
not what we want.

Or should I just edit icl_get_total_relative_data_rate so that it
still calculates crtc_state->plane_data_rate properly however, the
doesn't add those to total_data_rate, if use_min_ddb(plane) is set?

Stan

> 
> > +{
> > +	return DISPLAY_VER(i915) >= 13 && crtc_state->uapi.async_flip;
> > +}
> > +
> >  static int
> >  skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  		       struct intel_crtc *crtc)
> > @@ -5225,9 +5231,14 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->plane_data_rate[plane_id];
> > +
> > +		if (needs_min_ddb(dev_priv, crtc_state))
> > +			rate = 0;
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> > @@ -5236,13 +5247,19 @@ skl_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			break;
> >  
> >  		rate = crtc_state->uv_plane_data_rate[plane_id];
> > +
> > +		if (needs_min_ddb(dev_priv, crtc_state))
> > +			rate = 0;
> > +
> >  		extra = min_t(u16, alloc_size,
> >  			      DIV64_U64_ROUND_UP(alloc_size * rate,
> >  						 total_data_rate));
> > +
> >  		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> >  		alloc_size -= extra;
> >  		total_data_rate -= rate;
> >  	}
> > +
> >  	drm_WARN_ON(&dev_priv->drm, alloc_size != 0 || total_data_rate != 0);
> >  
> >  	/* Set the actual DDB start/end points for each plane */
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
> -- 
> Ville Syrjälä
> Intel