Re: [Intel-gfx] [PATCH 3/6] drm/i915: Treat SAGV block time 0 as SAGV disabled

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

On Wed, Mar 09, 2022 at 11:29:37AM +0000, Govindapillai, Vinod wrote:
> On Tue, 2022-03-08 at 19:32 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > For modern platforms the spec explicitly states that a
> > SAGV block time of zero means that SAGV is not supported.
> > Let's extend that to all platforms. Supposedly there should
> > be no systems where this isn't true, and it'll allow us to:
> > - use the same code regardless of older vs. newer platform
> > - wm latencies already treat 0 as disabled, so this fits well
> >   with other related code
> > - make it a bit more clear when SAGV is used vs. not
> > - avoid overflows from adding U32_MAX with a u16 wm0 latency value
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 21c37115c36e..906501d6b298 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3682,7 +3682,7 @@ intel_sagv_block_time(struct drm_i915_private *dev_priv)
> >  				     &val, NULL);
> >  		if (ret) {
> >  			drm_dbg_kms(&dev_priv->drm, "Couldn't read SAGV block time!\n");
> > -			return -1;
> > +			return 0;
> >  		}
> >  
> >  		return val;
> > @@ -3691,8 +3691,7 @@ intel_sagv_block_time(struct drm_i915_private *dev_priv)
> >  	} else if (DISPLAY_VER(dev_priv) == 9 && !IS_LP(dev_priv)) {
> >  		return 30;
> >  	} else {
> > -		/* Default to an unusable block time */
> > -		return -1;
> > +		return 0;
> >  	}
> >  }
> >  
> > @@ -3704,7 +3703,7 @@ static void intel_sagv_init(struct drm_i915_private *i915)
> >  		    str_yes_no(intel_has_sagv(i915)), i915->sagv_block_time_us);
> >  
> >  	if (!intel_has_sagv(i915))
> > -		i915->sagv_block_time_us = -1;
> > +		i915->sagv_block_time_us = 0;
> 
> Hi Ville
> 
> Currently we set the "sagv_status" as "I915_SAGV_NOT_CONTROLLED" based on the number of qgv points.
> So here i915->sagv_block_time_us will be set to 0 even if intel_sagv_block_time(i915) would have
> returned some valid values. Is that the desired behavior for sgav watermarks calcultations?

I don't think there's much point in calculating the sagv wm if we can't
use sagv anyway.

-- 
Ville Syrjälä
Intel