diff mbox series

[05/14] drm/i915: Fix HPLL watermark readout for g4x

Message ID 20210514125751.17075-6-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: g4x/vlv/chv CxSR/wm fixes/cleanups | expand

Commit Message

Ville Syrjälä May 14, 2021, 12:57 p.m. UTC 
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(-)

Comments

Lisovskiy, Stanislav Sept. 17, 2021, 3:34 p.m. UTC | #1 
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
Ville Syrjälä Sept. 22, 2021, 2:05 p.m. UTC | #2 
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@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,

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.
Lisovskiy, Stanislav Sept. 23, 2021, 1:24 p.m. UTC | #3 
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@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,
> 
> 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.

Thanks for the clarification.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Sept. 23, 2021, 3:51 p.m. UTC | #4 
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@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,
> > 
> > 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@intel.com>

Ta.
diff mbox series

Patch

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)
 			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++;
 	out:
 		for_each_plane_id_on_crtc(crtc, plane_id)
 			g4x_raw_plane_wm_set(crtc_state, level,