Re: [Intel-gfx] [PATCH] drm/i915: Write zero wms if we disable planes for icl+

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

On Wed, May 18, 2022 at 02:44:30PM +0300, Jani Nikula wrote:
> On Wed, 18 May 2022, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> > Otherwise we seem to get FIFO underruns. It is being disabled
> > anyway, so kind of logical to write those as zeroes, even if
> > disabling is temporary.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  .../drm/i915/display/skl_universal_plane.c    |  2 +-
> >  drivers/gpu/drm/i915/intel_pm.c               | 46 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.h               |  2 +
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index caa03324a733..c0251189c042 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -633,7 +633,7 @@ icl_plane_disable_arm(struct intel_plane *plane,
> >  	if (icl_is_hdr_plane(dev_priv, plane_id))
> >  		intel_de_write_fw(dev_priv, PLANE_CUS_CTL(pipe, plane_id), 0);
> >  
> > -	skl_write_plane_wm(plane, crtc_state);
> > +	skl_write_zero_plane_wm(plane, crtc_state);
> >  
> >  	intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
> >  	intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), 0);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ee0047fdc95d..2470c037bfae 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5885,6 +5885,52 @@ void skl_write_plane_wm(struct intel_plane *plane,
> >  				    PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y);
> >  }
> >  
> > +void skl_write_zero_plane_wm(struct intel_plane *plane,
> > +			     const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	int level, max_level = ilk_wm_max_level(dev_priv);
> > +	enum plane_id plane_id = plane->id;
> > +	enum pipe pipe = plane->pipe;
> > +	struct skl_pipe_wm pipe_wm;
> > +	const struct skl_ddb_entry *ddb =
> > +		&crtc_state->wm.skl.plane_ddb[plane_id];
> > +	const struct skl_ddb_entry *ddb_y =
> > +		&crtc_state->wm.skl.plane_ddb_y[plane_id];
> > +
> > +	for (level = 0; level <= max_level; level++) {
> 
> Not your doing here, but why have we adopted this error prone inclusive
> max that always makes you take a double look in the for loops?!
> 
> BR,
> Jani.

No clue, really. It seems to be used same way all over the place in
intel_pm.c. I was suggesting there is some smart reasoning behind this,
so simply follow the common approach.

For me it is more confusing that apparently it is called "ilk_*" which
is IronLake probably, however we use it everywhere.
I would call it simple i915_wm_max_level and make it determine itself based on
dev_priv which platform it is..

Stan



> 
> > +		pipe_wm.planes[plane_id].wm[level].blocks = 0;
> > +		pipe_wm.planes[plane_id].wm[level].lines = 0;
> > +	}
> > +
> > +	pipe_wm.planes[plane_id].trans_wm.blocks = 0;
> > +	pipe_wm.planes[plane_id].trans_wm.lines = 0;
> > +
> > +	for (level = 0; level <= max_level; level++)
> > +		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
> > +				   skl_plane_wm_level(&pipe_wm, plane_id, level));
> > +
> > +	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> > +			   skl_plane_trans_wm(&pipe_wm, plane_id));
> > +
> > +	if (HAS_HW_SAGV_WM(dev_priv)) {
> > +		const struct skl_plane_wm *wm = &pipe_wm.planes[plane_id];
> > +
> > +		skl_write_wm_level(dev_priv, PLANE_WM_SAGV(pipe, plane_id),
> > +				   &wm->sagv.wm0);
> > +		skl_write_wm_level(dev_priv, PLANE_WM_SAGV_TRANS(pipe, plane_id),
> > +				   &wm->sagv.trans_wm);
> > +	}
> > +
> > +	skl_ddb_entry_write(dev_priv,
> > +			    PLANE_BUF_CFG(pipe, plane_id), ddb);
> > +
> > +	if (DISPLAY_VER(dev_priv) < 11)
> > +		skl_ddb_entry_write(dev_priv,
> > +				    PLANE_NV12_BUF_CFG(pipe, plane_id), ddb_y);
> > +}
> > +
> > +
> >  void skl_write_cursor_wm(struct intel_plane *plane,
> >  			 const struct intel_crtc_state *crtc_state)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 50604cf7398c..fb0ac4f143ab 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -61,6 +61,8 @@ bool skl_wm_level_equals(const struct skl_wm_level *l1,
> >  bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry *ddb,
> >  				 const struct skl_ddb_entry *entries,
> >  				 int num_entries, int ignore_idx);
> > +void skl_write_zero_plane_wm(struct intel_plane *plane,
> > +			     const struct intel_crtc_state *crtc_state);
> >  void skl_write_plane_wm(struct intel_plane *plane,
> >  			const struct intel_crtc_state *crtc_state);
> >  void skl_write_cursor_wm(struct intel_plane *plane,
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center