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

[Lisovskiy, Stanislav]

On Fri, 2019-10-25 at 13:24 +0300, Ville Syrjälä wrote:
> On Fri, Oct 25, 2019 at 12:53:51PM +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.
> > 
> > v2:
> >     - Rework watermark calculation algorithm to
> >       attempt to calculate Level 0 watermark
> >       with added sagv block time latency and
> >       check if it fits in DBuf in order to
> >       determine if SAGV can be enabled already
> >       at this stage, just as BSpec 49325 states.
> >       if that fails rollback to usual Level 0
> >       latency and disable SAGV.
> >     - Remove unneeded tabs(James Ausmus)
> > 
> > v3: Rebased the patch
> > 
> > v4: - Added back interlaced check for Gen12 and
> >       added separate function for TGL SAGV check
> >       (thanks to James Ausmus for spotting)
> >     - Removed unneeded gen check
> >     - Extracted Gen12 SAGV decision making code
> >       to a separate function from skl_compute_wm
> > 
> > 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>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |   8 +
> >  drivers/gpu/drm/i915/intel_pm.c               | 254
> > +++++++++++++++++-
> >  2 files changed, 254 insertions(+), 8 deletions(-)
> > 
> > 
> Do we use active or enable elsewhere to decide whether to compute wms
> for a pipe? Should be consistent here so we don't get into some wonky
> state where we didn't compute normal wms but are computing the sagv
> wm.

Good question, I have seen it either this or that everywhere, so we 
probably need to discuss which one I should use.

> 
> > +
> > +		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];
> > +
> > +			/*
> > +			 * 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) >= 12)
> > +		return tgl_can_enable_sagv(state);
> > +	else if (INTEL_GEN(dev_priv) == 11)
> > +		return icl_can_enable_sagv(state);
> > +
> > +	return skl_can_enable_sagv(state);
> 
> Why do we have three separate code paths now? I believe there should
> be
> just two.
> 
> Also if you go to the trouble of adding dev_priv->..can_sagv just
> make
> it work for all platforms.

..can_sagv is not in dev_priv - it is part of intel_atomic_state,
so at least here I avoided using dev_priv.

> 
> > 
> >  	
> > +	/*
> > +	 * Lets assume we can tolerate SAGV for now,
> > +	 * until watermark calculations prove the opposite
> > +	 * if any of the pipe planes in the state will
> > +	 * fail the requirements it will be assigned to false
> > +	 * in skl_compute_ddb.
> > +	 */
> > +	state->gen12_can_sagv = true;
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		ret = tgl_check_pipe_fits_sagv_wm(new_crtc_state, ddb);
> > +		if (ret) {
> > +			state->gen12_can_sagv = false;
> > +			break;
> +		}
> 
> This is not going to work. We need the infromation from _all_ pipes,
> not
> just the ones in the state. We probably want to make that can_sagv
> thing
> a bitmask of pipes so that we don't have to have all pipes in the
> state.

But isn't it so that even if at least one plane/pipe can't tolerate
SAGV, we can't enable it already? So what is the point of checking
other planes/pipes then?
Or may be I'm missing something here.

> 
> > +	}
> > +
> > +	if (state->gen12_can_sagv) {
> > +		/*
> > +		 * If we determined that we can actually enable SAGV,
> > then
> > +		 * actually use those levels
> > tgl_check_pipe_fits_sagv_wm
> > +		 * has already taken care of checking if L0 + sagv
> > block time
> > +		 * fits into ddb.
> > +		 */
> > +		for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +			struct intel_plane *plane;
> > +			for_each_intel_plane_on_crtc(&dev_priv->drm,
> > crtc, plane) {
> > +				enum plane_id plane_id = plane->id;
> > +				struct skl_plane_wm *plane_wm = \
> > +				    &new_crtc_state-
> > >wm.skl.optimal.planes[plane_id];
> > +				struct skl_wm_level *sagv_wm0 =
> > &plane_wm->sagv_wm_l0;
> > +				struct skl_wm_level *l0_wm0 =
> > &plane_wm->wm[0];
> > +
> > +				memcpy(l0_wm0, sagv_wm0, sizeof(struct
> > skl_wm_level));
> > +			}
> > +		}
> > +	}
> > +}
> > +
> >  static int
> >  skl_compute_wm(struct intel_atomic_state *state)
> >  {
> > +	struct drm_device *dev = state->base.dev;
> > +	const struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *crtc;
> >  	struct intel_crtc_state *new_crtc_state;
> >  	struct intel_crtc_state *old_crtc_state;
> > @@ -5553,6 +5785,9 @@ skl_compute_wm(struct intel_atomic_state
> > *state)
> >  	/* Clear all dirty flags */
> >  	results->dirty_pipes = 0;
> >  
> > +	/* If we exit before check is done */
> > +	state->gen12_can_sagv = false;
> > +
> >  	ret = skl_ddb_add_affected_pipes(state);
> >  	if (ret)
> >  		return ret;
> > @@ -5579,6 +5814,9 @@ skl_compute_wm(struct intel_atomic_state
> > *state)
> >  			results->dirty_pipes |= BIT(crtc->pipe);
> >  	}
> >  
> > +	if (INTEL_GEN(dev_priv) >= 12)
> > +		tgl_check_and_set_sagv(state);
> > +
> >  	ret = skl_compute_ddb(state);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.17.1
> 
>