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

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

On Fri, May 14, 2021 at 03:57:42PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@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,
> -- 
> 2.26.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx