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

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

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? 

BR
vinod

>  }
>  
>  /*
> @@ -5651,7 +5650,7 @@ static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state,
>  	result->min_ddb_alloc = max(min_ddb_alloc, blocks) + 1;
>  	result->enable = true;
>  
> -	if (DISPLAY_VER(dev_priv) < 12)
> +	if (DISPLAY_VER(dev_priv) < 12 && dev_priv->sagv_block_time_us)
>  		result->can_sagv = latency >= dev_priv->sagv_block_time_us;
>  }
>  
> @@ -5684,7 +5683,10 @@ static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state,
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
>  	struct skl_wm_level *sagv_wm = &plane_wm->sagv.wm0;
>  	struct skl_wm_level *levels = plane_wm->wm;
> -	unsigned int latency = dev_priv->wm.skl_latency[0] + dev_priv->sagv_block_time_us;
> +	unsigned int latency = 0;
> +
> +	if (dev_priv->sagv_block_time_us)
> +		latency = dev_priv->sagv_block_time_us + dev_priv->wm.skl_latency[0];
>  
>  	skl_compute_plane_wm(crtc_state, plane, 0, latency,
>  			     wm_params, &levels[0],