Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Refactor intel_can_enable_sagv
On Fri, Oct 18, 2019 at 01:34:35AM -0700, Lisovskiy, Stanislav wrote:
> On Thu, 2019-10-17 at 14:53 -0700, James Ausmus wrote:
> > On Tue, Oct 15, 2019 at 04:50:12PM +0300, Stanislav Lisovskiy wrote:
> > > Currently intel_can_enable_sagv function contains
> > > a mix of workarounds for different platforms
> > > some of them are not valid for gens >= 11 already,
> > > so lets split it into separate functions.
> > >
> > > Signed-off-by: Stanislav Lisovskiy
> > > Cc: Ville Syrjälä
> > > Cc: James Ausmus
> > > ---
> > > drivers/gpu/drm/i915/intel_pm.c | 73
> > > +++--
> > > 1 file changed, 70 insertions(+), 3 deletions(-)
> > >
> > > +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> > > +{
> > > + struct drm_device *dev = state->base.dev;
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > + struct intel_crtc *crtc;
> > > + struct intel_crtc_state *new_crtc_state;
> > > + int level, latency;
> > > + int i;
> > > + int plane_id;
> > > +
> > > + if (!intel_has_sagv(dev_priv))
> > > + return false;
> > > +
> > > + /*
> > > + * If there are no active CRTCs, no additional checks need be
> > > performed
> > > + */
> > > + if (hweight8(state->active_pipes) == 0)
> > > + return true;
> > > +
> > > + for_each_new_intel_crtc_in_state(state, crtc,
> > > + new_crtc_state, i) {
> > > +
> > > + if (crtc->base.state->adjusted_mode.flags &
> > > DRM_MODE_FLAG_INTERLACE)
> > > + return false;
> > > +
> > > + if (!new_crtc_state->base.enable)
> > > + continue;
> > > +
> > > + for_each_plane_id_on_crtc(crtc, plane_id) {
> > > + struct skl_plane_wm *wm =
> > > + _crtc_state-
> > > >wm.skl.optimal.planes[plane_id];
> > > +
> > > + /* Skip this plane if it's not enabled */
> > > + if (!wm->wm[0].plane_en)
> > > + continue;
> > > +
> > > + /* Find the highest enabled wm level for this
> > > plane */
> > > + for (level = ilk_wm_max_level(dev_priv);
> > > + !wm->wm[level].plane_en; --level)
> > > + { }
> > > +
> > > + latency = dev_priv->wm.skl_latency[level];
> >
> > This isn't exactly the same for TGL. From BSpec 49325, "Calculate
> > watermark level 0 with level 0 latency + SAGV block time. If the
> > result
> > can be supported (does not exceed maximum), then the plane can
> > tolerate
> > SAGV", so I think it can be simplified for Gen12+ by not having to
> > loop
> > through all the wm levels.
> >
> > -James
>
> Yes, we discussed that with Ville as I understood, properly doing
> that for TGL requires changes to watermark/ddb allocation algorithm,
> so that we check if dbuf can fit Level 0 watermark increased by SAGV
> block time. I was not sure whether should I add this patch to this
> series or proceed with that separately, probably this is worth adding
> here - however then it would require changes to watermark algorithm,
> as we need to recalculate min_ddb_alloc for Level 0 latency + SAGV
> and then check if we fit into DBuf again.
> So basically we'll have to calculate watermark level 0 twice - once for
> SAGV enabled and once for SAGV disabled case, I would probably do it
> already during watermark calculations not when checking bandwidth and
> then set some "SAGV can enable" flag, which will be then used by
> intel_can_enable_sagv, so that we act accordingly when
> bandwidth check and restricting qgv points happens. For pre-gen 12 it
> will otherwise use the old algorithm.
OK, that works then. With the whitespace fix added:
Reviewed-by: James Ausmus
>
> >
> > > +
> > > + /*
> > > + * If any of the planes on this pipe don't
> > > enable wm levels that
> > > + * incur memory latencies higher than
> > > sagv_block_time_us we
> > > + * can't enable SAGV.
> > > + */
> > > + if (latency < dev_priv->sagv_block_time_us)
> > > + return false;
> > > + }
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > +{
> > > + struct drm_device *dev = state->base.dev;
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > +
> > > + if (INTEL_GEN(dev_priv) >= 11)
> > > + return icl_can_enable_sagv(state);
> > > +
> > > + return skl_can_enable_sagv(state);
> > > +}
> > > +
> > > static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> > > const struct intel_crtc_state
> > > *crtc_state,
> > > const u64 total_data_rate,
> > > --
> > > 2.17.1
> > >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Refactor intel_can_enable_sagv
On Thu, 2019-10-17 at 14:53 -0700, James Ausmus wrote:
> On Tue, Oct 15, 2019 at 04:50:12PM +0300, Stanislav Lisovskiy wrote:
> > Currently intel_can_enable_sagv function contains
> > a mix of workarounds for different platforms
> > some of them are not valid for gens >= 11 already,
> > so lets split it into separate functions.
> >
> > Signed-off-by: Stanislav Lisovskiy
> > Cc: Ville Syrjälä
> > Cc: James Ausmus
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 73
> > +++--
> > 1 file changed, 70 insertions(+), 3 deletions(-)
> >
> > +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct intel_crtc *crtc;
> > + struct intel_crtc_state *new_crtc_state;
> > + int level, latency;
> > + int i;
> > + int plane_id;
> > +
> > + if (!intel_has_sagv(dev_priv))
> > + return false;
> > +
> > + /*
> > +* If there are no active CRTCs, no additional checks need be
> > performed
> > +*/
> > + if (hweight8(state->active_pipes) == 0)
> > + return true;
> > +
> > + for_each_new_intel_crtc_in_state(state, crtc,
> > +new_crtc_state, i) {
> > +
> > + if (crtc->base.state->adjusted_mode.flags &
> > DRM_MODE_FLAG_INTERLACE)
> > + return false;
> > +
> > + if (!new_crtc_state->base.enable)
> > + continue;
> > +
> > + for_each_plane_id_on_crtc(crtc, plane_id) {
> > + struct skl_plane_wm *wm =
> > + _crtc_state-
> > >wm.skl.optimal.planes[plane_id];
> > +
> > + /* Skip this plane if it's not enabled */
> > + if (!wm->wm[0].plane_en)
> > + continue;
> > +
> > + /* Find the highest enabled wm level for this
> > plane */
> > + for (level = ilk_wm_max_level(dev_priv);
> > +!wm->wm[level].plane_en; --level)
> > +{ }
> > +
> > + latency = dev_priv->wm.skl_latency[level];
>
> This isn't exactly the same for TGL. From BSpec 49325, "Calculate
> watermark level 0 with level 0 latency + SAGV block time. If the
> result
> can be supported (does not exceed maximum), then the plane can
> tolerate
> SAGV", so I think it can be simplified for Gen12+ by not having to
> loop
> through all the wm levels.
>
> -James
Yes, we discussed that with Ville as I understood, properly doing
that for TGL requires changes to watermark/ddb allocation algorithm,
so that we check if dbuf can fit Level 0 watermark increased by SAGV
block time. I was not sure whether should I add this patch to this
series or proceed with that separately, probably this is worth adding
here - however then it would require changes to watermark algorithm,
as we need to recalculate min_ddb_alloc for Level 0 latency + SAGV
and then check if we fit into DBuf again.
So basically we'll have to calculate watermark level 0 twice - once for
SAGV enabled and once for SAGV disabled case, I would probably do it
already during watermark calculations not when checking bandwidth and
then set some "SAGV can enable" flag, which will be then used by
intel_can_enable_sagv, so that we act accordingly when
bandwidth check and restricting qgv points happens. For pre-gen 12 it
will otherwise use the old algorithm.
>
> > +
> > + /*
> > +* If any of the planes on this pipe don't
> > enable wm levels that
> > +* incur memory latencies higher than
> > sagv_block_time_us we
> > +* can't enable SAGV.
> > +*/
> > + if (latency < dev_priv->sagv_block_time_us)
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > + if (INTEL_GEN(dev_priv) >= 11)
> > + return icl_can_enable_sagv(state);
> > +
> > + return skl_can_enable_sagv(state);
> > +}
> > +
> > static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> > const struct intel_crtc_state
> > *crtc_state,
> > const u64 total_data_rate,
> > --
> > 2.17.1
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Refactor intel_can_enable_sagv
On Tue, Oct 15, 2019 at 04:50:12PM +0300, Stanislav Lisovskiy wrote:
> Currently intel_can_enable_sagv function contains
> a mix of workarounds for different platforms
> some of them are not valid for gens >= 11 already,
> so lets split it into separate functions.
>
> Signed-off-by: Stanislav Lisovskiy
> Cc: Ville Syrjälä
> Cc: James Ausmus
> ---
> drivers/gpu/drm/i915/intel_pm.c | 73 +++--
> 1 file changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 67d171456f59..662a36ff2f43 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3750,7 +3750,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +bool skl_can_enable_sagv(struct intel_atomic_state *state)
> {
> struct drm_device *dev = state->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3801,8 +3801,8 @@ bool intel_can_enable_sagv(struct intel_atomic_state
> *state)
>
> if (skl_needs_memory_bw_wa(dev_priv) &&
> plane->base.state->fb->modifier ==
> - I915_FORMAT_MOD_X_TILED)
> - latency += 15;
> + I915_FORMAT_MOD_X_TILED)
> + latency += 15;
This whitespace change doesn't look right
>
> /*
>* If any of the planes on this pipe don't enable wm levels that
> @@ -3816,6 +3816,73 @@ bool intel_can_enable_sagv(struct intel_atomic_state
> *state)
> return true;
> }
>
> +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> +{
> + struct drm_device *dev = state->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *new_crtc_state;
> + int level, latency;
> + int i;
> + int plane_id;
> +
> + if (!intel_has_sagv(dev_priv))
> + return false;
> +
> + /*
> + * If there are no active CRTCs, no additional checks need be performed
> + */
> + if (hweight8(state->active_pipes) == 0)
> + return true;
> +
> + for_each_new_intel_crtc_in_state(state, crtc,
> + new_crtc_state, i) {
> +
> + if (crtc->base.state->adjusted_mode.flags &
> DRM_MODE_FLAG_INTERLACE)
> + return false;
> +
> + if (!new_crtc_state->base.enable)
> + continue;
> +
> + for_each_plane_id_on_crtc(crtc, plane_id) {
> + struct skl_plane_wm *wm =
> +
> _crtc_state->wm.skl.optimal.planes[plane_id];
> +
> + /* Skip this plane if it's not enabled */
> + if (!wm->wm[0].plane_en)
> + continue;
> +
> + /* Find the highest enabled wm level for this plane */
> + for (level = ilk_wm_max_level(dev_priv);
> + !wm->wm[level].plane_en; --level)
> + { }
> +
> + latency = dev_priv->wm.skl_latency[level];
This isn't exactly the same for TGL. From BSpec 49325, "Calculate
watermark level 0 with level 0 latency + SAGV block time. If the result
can be supported (does not exceed maximum), then the plane can tolerate
SAGV", so I think it can be simplified for Gen12+ by not having to loop
through all the wm levels.
-James
> +
> + /*
> + * If any of the planes on this pipe don't enable wm
> levels that
> + * incur memory latencies higher than
> sagv_block_time_us we
> + * can't enable SAGV.
> + */
> + if (latency < dev_priv->sagv_block_time_us)
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +{
> + struct drm_device *dev = state->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (INTEL_GEN(dev_priv) >= 11)
> + return icl_can_enable_sagv(state);
> +
> + return skl_can_enable_sagv(state);
> +}
> +
> static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> const struct intel_crtc_state *crtc_state,
> const u64 total_data_rate,
> --
> 2.17.1
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4 1/2] drm/i915: Refactor intel_can_enable_sagv
Currently intel_can_enable_sagv function contains
a mix of workarounds for different platforms
some of them are not valid for gens >= 11 already,
so lets split it into separate functions.
Signed-off-by: Stanislav Lisovskiy
Cc: Ville Syrjälä
Cc: James Ausmus
---
drivers/gpu/drm/i915/intel_pm.c | 73 +++--
1 file changed, 70 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 67d171456f59..662a36ff2f43 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3750,7 +3750,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
return 0;
}
-bool intel_can_enable_sagv(struct intel_atomic_state *state)
+bool skl_can_enable_sagv(struct intel_atomic_state *state)
{
struct drm_device *dev = state->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -3801,8 +3801,8 @@ bool intel_can_enable_sagv(struct intel_atomic_state
*state)
if (skl_needs_memory_bw_wa(dev_priv) &&
plane->base.state->fb->modifier ==
- I915_FORMAT_MOD_X_TILED)
- latency += 15;
+ I915_FORMAT_MOD_X_TILED)
+ latency += 15;
/*
* If any of the planes on this pipe don't enable wm levels that
@@ -3816,6 +3816,73 @@ bool intel_can_enable_sagv(struct intel_atomic_state
*state)
return true;
}
+bool icl_can_enable_sagv(struct intel_atomic_state *state)
+{
+ struct drm_device *dev = state->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_crtc *crtc;
+ struct intel_crtc_state *new_crtc_state;
+ int level, latency;
+ int i;
+ int plane_id;
+
+ if (!intel_has_sagv(dev_priv))
+ return false;
+
+ /*
+* If there are no active CRTCs, no additional checks need be performed
+*/
+ if (hweight8(state->active_pipes) == 0)
+ return true;
+
+ for_each_new_intel_crtc_in_state(state, crtc,
+new_crtc_state, i) {
+
+ if (crtc->base.state->adjusted_mode.flags &
DRM_MODE_FLAG_INTERLACE)
+ return false;
+
+ if (!new_crtc_state->base.enable)
+ continue;
+
+ for_each_plane_id_on_crtc(crtc, plane_id) {
+ struct skl_plane_wm *wm =
+
_crtc_state->wm.skl.optimal.planes[plane_id];
+
+ /* Skip this plane if it's not enabled */
+ if (!wm->wm[0].plane_en)
+ continue;
+
+ /* Find the highest enabled wm level for this plane */
+ for (level = ilk_wm_max_level(dev_priv);
+!wm->wm[level].plane_en; --level)
+{ }
+
+ latency = dev_priv->wm.skl_latency[level];
+
+ /*
+* If any of the planes on this pipe don't enable wm
levels that
+* incur memory latencies higher than
sagv_block_time_us we
+* can't enable SAGV.
+*/
+ if (latency < dev_priv->sagv_block_time_us)
+ return false;
+ }
+ }
+
+ return true;
+}
+
+bool intel_can_enable_sagv(struct intel_atomic_state *state)
+{
+ struct drm_device *dev = state->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+
+ if (INTEL_GEN(dev_priv) >= 11)
+ return icl_can_enable_sagv(state);
+
+ return skl_can_enable_sagv(state);
+}
+
static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
const struct intel_crtc_state *crtc_state,
const u64 total_data_rate,
--
2.17.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
