[Intel-gfx] [PATCH 05/14] drm/i915: Fix HPLL watermark readout for g4x

[Ville Syrjälä]

On Thu, Sep 23, 2021 at 04:24:21PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, Sep 22, 2021 at 05:05:12PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 17, 2021 at 06:34:22PM +0300, Lisovskiy, Stanislav wrote:
> > > On Fri, May 14, 2021 at 03:57:42PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > If HPLL watermarks are already enabled, let's not mark them as
> > > > disabled by forgetting to bump 'level' before we call
> > > > g4x_raw_plane_wm_set().
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 661bc6fdf38c..990ee5a590d3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -6468,7 +6468,8 @@ void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv)
> > > >  		for_each_plane_id_on_crtc(crtc, plane_id)
> > > >  			raw->plane[plane_id] = active->wm.plane[plane_id];
> > > >  
> > > > -		if (++level > max_level)
> > > > +		level = G4X_WM_LEVEL_SR;
> > > > +		if (level > max_level)
> > > 
> > > Do I understand correctly that its basically identical to what
> > > was before, so this is done here just for it to look more explicit?
> > > 
> > > I.e we had for example max_level G4X_WM_LEVEL_SR and level G4X_WM_LEVEL_NORMAL
> > > , after ++level it will anyway become G4X_WM_LEVEL_SR and same for next one.
> > > 
> > > 
> > > >  			goto out;
> > > >  
> > > >  		raw = &crtc_state->wm.g4x.raw[level];
> > > > @@ -6477,7 +6478,8 @@ void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv)
> > > >  		raw->plane[PLANE_SPRITE0] = 0;
> > > >  		raw->fbc = active->sr.fbc;
> > > >  
> > > > -		if (++level > max_level)
> > > > +		level = G4X_WM_LEVEL_HPLL;
> > > > +		if (level > max_level)
> > > >  			goto out;
> > > >  
> > > >  		raw = &crtc_state->wm.g4x.raw[level];
> > > > @@ -6486,6 +6488,7 @@ void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv)
> > > >  		raw->plane[PLANE_SPRITE0] = 0;
> > > >  		raw->fbc = active->hpll.fbc;
> > > >  
> > > > +		level++;
> > > 
> > > Hi Ville,
> > > 
> > > So if we reached here, it means level = G4X_WM_LEVEL_HPLL, which is 
> > > the max wm level defined, why are we then incrementing it even more?
> > > 
> > > the g4x_raw_plane_wm_set will be using that value as a level:
> > > 
> > > for (; level < intel_wm_num_levels(dev_priv); level++) {
> > > 	struct g4x_pipe_wm *raw = &crtc_state->wm.g4x.raw[level];
> > > 
> > > 	dirty |= raw->plane[plane_id] != value;
> > > 	raw->plane[plane_id] = value;
> > > }
> > > 
> > > however level then will be equal to NUM_G4X_WM_LEVELS, which is actually
> > > an illegal value, or is that an expected behaviour?
> > > 
> > > Just trying to understand, whats happening here, before stamping an r-b :)
> > > 
> > > Stan
> > > 
> > > 
> > > >  	out:
> > > >  		for_each_plane_id_on_crtc(crtc, plane_id)
> > > >  			g4x_raw_plane_wm_set(crtc_state, level,
> > 
> > Right, so the code is basically this:
> > 
> > level = G4X_WM_LEVEL_SR;
> > if (level > max_level)
> > 	goto out;
> > level = G4X_WM_LEVEL_HPLL;
> > if (level > max_level)
> > 	goto out;
> > level++ /* ie. level=NUM_G4X_WM_LEVELS */
> > out:
> > invalidate_raw_watermarks_starting_from_level(level);
> > 
> > So if we take the first goto we want to invalidate all
> > watermarks starting from SR, second goto wants to invalidate
> > all watermarks starting from HPLL, and if we didn't take either
> > goto we don't want to invalidate any watermarks because we deemed
> > everything up to and including HPLL is ok. So we can't just
> > leave level==G4X_WM_LEVEL_HPLL or else the code would still invalidate
> > the HPLL watermarks. Instead we level++ so that the invalidate call
> > becomes a nop.
> > 
> > The other option I suppose would be to skip the invalidation stuff
> > if we didn't take either of the gotos, but I'm thinking that would make
> > the code more messy.
> 
> Ah ok, thought its setting wm levels, but if its actually invalidating,
> makes sense. Probably that is why it uses USHRT_MAX as a value.

Yes. I guess it wouldn't hurt to add a tiny raw_wm_invalidate()
wrapper or something to make the code more self explanatory.

> 
> Thanks for the clarification.
> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>

Ta.

-- 
Ville Syrjälä
Intel