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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel