| Message ID | 20191015135013.10630-2-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor Gen11+ SAGV support | expand |
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 <stanislav.lisovskiy@intel.com> > Cc: Ville Syrjälä <ville.syrjala@intel.com> > Cc: James Ausmus <james.ausmus@intel.com> > --- > 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 = > + &new_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 >
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 <stanislav.lisovskiy@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@intel.com> > > Cc: James Ausmus <james.ausmus@intel.com> > > --- > > 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 = > > + &new_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 > >
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 <stanislav.lisovskiy@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@intel.com> > > > Cc: James Ausmus <james.ausmus@intel.com> > > > --- > > > 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 = > > > + &new_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 <james.ausmus@intel.com> > > > > > > + > > > + /* > > > + * 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 > > >
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 = + &new_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,
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 <stanislav.lisovskiy@intel.com> Cc: Ville Syrjälä <ville.syrjala@intel.com> Cc: James Ausmus <james.ausmus@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 73 +++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 3 deletions(-)