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

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

On Wed, May 18, 2022 at 03:05:44PM +0300, Lisovskiy, Stanislav wrote:
> 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..

There are loads of (old by now) patches on the list for cleaning
up all kinds of stuff in the wm code:
https://patchwork.freedesktop.org/series/50802/
https://patchwork.freedesktop.org/series/83289/
https://patchwork.freedesktop.org/series/90164/

-- 
Ville Syrjälä
Intel