On Tue, 2022-03-08 at 19:32 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > 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ä <[email protected]> > --- > 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],
