On Fri, Mar 05, 2021 at 05:36:06PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Say we have two planes enabled with watermarks configured
> as follows:
> plane A: wm0=enabled/can_sagv=false, wm1=enabled/can_sagv=true
> plane B: wm0=enabled/can_sagv=true, wm1=disabled
Was thinking about this, always thought its not possible, i.e
wm1 kinda requires more resources, so if we can do wm1, should
always be able to do wm0..
>
> This is possible since the latency we use to calculate
> can_sagv may not be the same for both planes due to
> skl_needs_memory_bw_wa().
The current code, which I see in internal at least looks like this:
/*
* FIXME: We still don't have the proper code detect if we need to apply the WA,
* so assume we'll always need it in order to avoid underruns.
*/
static bool skl_needs_memory_bw_wa(struct drm_i915_private *dev_priv)
{
return IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv);
}
i.e I think it will return same latency for all planes.
Or am I missing something?..
Stan
>
> In this case skl_crtc_can_enable_sagv() will see that
> both planes have enabled at least one watermark level
> with can_sagv==true, and thus proceeds to allow SAGV.
> However, since plane B does not have wm1 enabled
> plane A can't actually use it either. Thus we are
> now running with SAGV enabled, but plane A can't
> actually tolerate the extra latency it imposes.
>
> To remedy this only allow SAGV on if the highest common
> enabled watermark level for all active planes can tolerate
> the extra SAGV latency.
>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 854ffecd98d9..b6e34d1701a0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3876,6 +3876,7 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> enum plane_id plane_id;
> + int max_level = INT_MAX;
>
> if (!intel_has_sagv(dev_priv))
> return false;
> @@ -3900,12 +3901,23 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> !wm->wm[level].plane_en; --level)
> { }
>
> + /* Highest common enabled wm level for all planes */
> + max_level = min(level, max_level);
> + }
> +
> + /* No enabled planes? */
> + if (max_level == INT_MAX)
> + return true;
> +
> + for_each_plane_id_on_crtc(crtc, plane_id) {
> + const struct skl_plane_wm *wm =
> + &crtc_state->wm.skl.optimal.planes[plane_id];
> +
> /*
> - * 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.
> + * All enabled planes must have enabled a common wm level that
> + * can tolerate memory latencies higher than sagv_block_time_us
> */
> - if (!wm->wm[level].can_sagv)
> + if (wm->wm[0].plane_en && !wm->wm[max_level].can_sagv)
> return false;
> }
>
> --
> 2.26.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
