Re: [Intel-gfx] [PATCH v2 1/8] drm/i915: Treat SAGV block time 0 as SAGV disabled
On Wed, Mar 09, 2022 at 06:49:41PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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 > which could cause us to miscalculate the SAGV watermarks on tgl+ > > Cc: sta...@vger.kernel.org > Signed-off-by: Ville Syrjälä Reviewed-by: Stanislav Lisovskiy > --- > drivers/gpu/drm/i915/intel_pm.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8ee31c9590a7..40a3094e55ca 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3696,8 +3696,7 @@ skl_setup_sagv_block_time(struct drm_i915_private > *dev_priv) > MISSING_CASE(DISPLAY_VER(dev_priv)); > } > > - /* Default to an unusable block time */ > - dev_priv->sagv_block_time_us = -1; > + dev_priv->sagv_block_time_us = 0; > } > > /* > @@ -5644,7 +5643,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; > } > > @@ -5677,7 +5676,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], > -- > 2.34.1 >
Re: [Intel-gfx] [PATCH v2 1/8] drm/i915: Treat SAGV block time 0 as SAGV disabled
On Wed, Mar 09, 2022 at 09:29:58PM +0200, Lisovskiy, Stanislav wrote: > On Wed, Mar 09, 2022 at 06:49:41PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > 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 > > which could cause us to miscalculate the SAGV watermarks on tgl+ > > > > Cc: sta...@vger.kernel.org > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_pm.c | 10 ++ > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 8ee31c9590a7..40a3094e55ca 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3696,8 +3696,7 @@ skl_setup_sagv_block_time(struct drm_i915_private > > *dev_priv) > > MISSING_CASE(DISPLAY_VER(dev_priv)); > > } > > > > - /* Default to an unusable block time */ > > - dev_priv->sagv_block_time_us = -1; > > + dev_priv->sagv_block_time_us = 0; > > } > > > > /* > > @@ -5644,7 +5643,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; > > } > > > > @@ -5677,7 +5676,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]; > > Should we may be add this to intel_has_sagv? I thought this was supposed to > tell, > if SAGV is supported or not. Should we just call it hear as well, may be.. > Now we kinda making it less obvious. We already use intel_has_sagv() to see if we should zero out the block time. I don't think I want to make it a full circle. > > Stan > > > > > skl_compute_plane_wm(crtc_state, plane, 0, latency, > > wm_params, &levels[0], > > -- > > 2.34.1 > > -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH v2 1/8] drm/i915: Treat SAGV block time 0 as SAGV disabled
On Wed, Mar 09, 2022 at 06:49:41PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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 > which could cause us to miscalculate the SAGV watermarks on tgl+ > > Cc: sta...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_pm.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8ee31c9590a7..40a3094e55ca 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3696,8 +3696,7 @@ skl_setup_sagv_block_time(struct drm_i915_private > *dev_priv) > MISSING_CASE(DISPLAY_VER(dev_priv)); > } > > - /* Default to an unusable block time */ > - dev_priv->sagv_block_time_us = -1; > + dev_priv->sagv_block_time_us = 0; > } > > /* > @@ -5644,7 +5643,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; > } > > @@ -5677,7 +5676,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]; Should we may be add this to intel_has_sagv? I thought this was supposed to tell, if SAGV is supported or not. Should we just call it hear as well, may be.. Now we kinda making it less obvious. Stan > > skl_compute_plane_wm(crtc_state, plane, 0, latency, >wm_params, &levels[0], > -- > 2.34.1 >
[Intel-gfx] [PATCH v2 1/8] drm/i915: Treat SAGV block time 0 as SAGV disabled
From: Ville Syrjälä 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 which could cause us to miscalculate the SAGV watermarks on tgl+ Cc: sta...@vger.kernel.org Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_pm.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8ee31c9590a7..40a3094e55ca 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3696,8 +3696,7 @@ skl_setup_sagv_block_time(struct drm_i915_private *dev_priv) MISSING_CASE(DISPLAY_VER(dev_priv)); } - /* Default to an unusable block time */ - dev_priv->sagv_block_time_us = -1; + dev_priv->sagv_block_time_us = 0; } /* @@ -5644,7 +5643,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; } @@ -5677,7 +5676,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], -- 2.34.1
