* [PATCH] drm/i915: Disable SAGV on pre plane update.
@ 2018-02-22 19:28 Rodrigo Vivi
2018-02-22 20:00 ` Ville Syrjälä
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2018-02-22 19:28 UTC (permalink / raw)
To: intel-gfx; +Cc: Azhar Shaikh, Rodrigo Vivi
According to Spec "Requirement before plane enabling or
configuration change: Disable SAGV if any enabled plane will not
be able to enable watermarks for memory latency >= SAGV block
time, or any transcoder is interlaced. Else, enable SAGV."
Currently we are only enabling and disabling SAGV on full
modeset. If we end up changing plane configurations and
sagv remains enabled when latency is higher than sagv block
time the machine can hang.
Also we are computing the latency values in different places
and following different conditions/rules.
So let's move the can_enable_sagv check to be inside
skl_compute_wm and disable and enable on {pre,post} plane
updates.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Azhar Shaikh <azhar.shaikh@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 18 +++++-----
drivers/gpu/drm/i915/intel_drv.h | 3 +-
drivers/gpu/drm/i915/intel_pm.c | 64 +++++++++++++-----------------------
3 files changed, 32 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65c8487be7c7..008254579be1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_atomic_state *old_state = old_crtc_state->base.state;
struct intel_crtc_state *pipe_config =
intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
@@ -5120,6 +5122,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
!old_primary_state->base.visible))
intel_post_enable_primary(&crtc->base, pipe_config);
}
+
+ if (pipe_config->sagv)
+ intel_enable_sagv(dev_priv);
}
static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
@@ -5205,6 +5210,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
pipe_config);
else if (pipe_config->update_wm_pre)
intel_update_watermarks(crtc);
+
+ if (!pipe_config->sagv)
+ intel_disable_sagv(dev_priv);
}
static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -12366,13 +12374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
- /*
- * SKL workaround: bspec recommends we disable the SAGV when we
- * have more then one pipe enabled
- */
- if (!intel_can_enable_sagv(state))
- intel_disable_sagv(dev_priv);
-
intel_modeset_verify_disabled(dev, state);
}
@@ -12431,9 +12432,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
if (intel_state->modeset)
intel_verify_planes(intel_state);
- if (intel_state->modeset && intel_can_enable_sagv(state))
- intel_enable_sagv(dev_priv);
-
drm_atomic_helper_commit_hw_done(state);
if (intel_state->modeset) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1535bfb7ea40..4c10a8a94d73 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_crtc_state {
bool update_pipe; /* can a fast modeset be performed? */
bool disable_cxsr;
bool update_wm_pre, update_wm_post; /* watermarks are updated */
+ bool sagv; /* Disable SAGV on any latency higher than its block time */
bool fb_changed; /* fb on any of the planes is changed */
bool fifo_changed; /* FIFO split is changed */
@@ -2001,7 +2002,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
struct skl_pipe_wm *out);
void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
-bool intel_can_enable_sagv(struct drm_atomic_state *state);
+bool intel_can_enable_sagv(struct intel_atomic_state *state, u32 latency);
int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
bool skl_wm_level_equals(const struct skl_wm_level *l1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21dac6ebc202..731b3808a62e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3681,16 +3681,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
return 0;
}
-bool intel_can_enable_sagv(struct drm_atomic_state *state)
+bool intel_can_enable_sagv(struct intel_atomic_state *intel_state, u32 latency)
{
- struct drm_device *dev = state->dev;
+ struct drm_device *dev = intel_state->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct intel_crtc *crtc;
- struct intel_plane *plane;
struct intel_crtc_state *cstate;
enum pipe pipe;
- int level, latency;
int sagv_block_time_us;
if (!intel_has_sagv(dev_priv))
@@ -3703,6 +3700,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
else
sagv_block_time_us = 10;
+ if (latency < sagv_block_time_us)
+ return false;
+
/*
* SKL+ workaround: bspec recommends we disable the SAGV when we have
* more then one pipe enabled
@@ -3722,35 +3722,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
return false;
- for_each_intel_plane_on_crtc(dev, crtc, plane) {
- struct skl_plane_wm *wm =
- &cstate->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 (skl_needs_memory_bw_wa(intel_state) &&
- plane->base.state->fb->modifier ==
- I915_FORMAT_MOD_X_TILED)
- latency += 15;
-
- /*
- * 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 the SAGV.
- */
- if (latency < sagv_block_time_us)
- return false;
- }
-
return true;
}
@@ -4501,7 +4472,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
const struct skl_wm_params *wp,
uint16_t *out_blocks, /* out */
uint8_t *out_lines, /* out */
- bool *enabled /* out */)
+ bool *enabled, /* out */
+ bool *sagv /* out */)
{
const struct drm_plane_state *pstate = &intel_pstate->base;
uint32_t latency = dev_priv->wm.skl_latency[level];
@@ -4528,6 +4500,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
if (apply_memory_bw_wa && wp->x_tiled)
latency += 15;
+ *sagv = intel_can_enable_sagv(state, latency);
+
method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
wp->cpp, latency, wp->dbuf_block_size);
method2 = skl_wm_method2(wp->plane_pixel_rate,
@@ -4629,7 +4603,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
const struct skl_wm_params *wm_params,
- struct skl_plane_wm *wm)
+ struct skl_plane_wm *wm,
+ bool *sagv)
{
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
struct drm_plane *plane = intel_pstate->base.plane;
@@ -4655,7 +4630,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
wm_params,
&result->plane_res_b,
&result->plane_res_l,
- &result->plane_en);
+ &result->plane_en,
+ sagv);
if (ret)
return ret;
}
@@ -4743,7 +4719,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
struct skl_ddb_allocation *ddb,
- struct skl_pipe_wm *pipe_wm)
+ struct skl_pipe_wm *pipe_wm,
+ bool *sagv)
{
struct drm_device *dev = cstate->base.crtc->dev;
struct drm_crtc_state *crtc_state = &cstate->base;
@@ -4777,7 +4754,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
- intel_pstate, &wm_params, wm);
+ intel_pstate, &wm_params, wm, sagv);
if (ret)
return ret;
skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
@@ -4899,12 +4876,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
const struct skl_pipe_wm *old_pipe_wm,
struct skl_pipe_wm *pipe_wm, /* out */
struct skl_ddb_allocation *ddb, /* out */
- bool *changed /* out */)
+ bool *changed /* out */,
+ bool *sagv /* out */)
{
struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
int ret;
- ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+ ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
if (ret)
return ret;
@@ -5099,6 +5077,7 @@ skl_compute_wm(struct drm_atomic_state *state)
struct drm_device *dev = state->dev;
struct skl_pipe_wm *pipe_wm;
bool changed = false;
+ bool sagv = true;
int ret, i;
/*
@@ -5147,7 +5126,7 @@ skl_compute_wm(struct drm_atomic_state *state)
pipe_wm = &intel_cstate->wm.skl.optimal;
ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
- &results->ddb, &changed);
+ &results->ddb, &changed, &sagv);
if (ret)
return ret;
@@ -5159,6 +5138,7 @@ skl_compute_wm(struct drm_atomic_state *state)
continue;
intel_cstate->update_wm_pre = true;
+ intel_cstate->sagv &= sagv;
}
skl_print_wm_changes(state);
--
2.13.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Disable SAGV on pre plane update.
2018-02-22 19:28 [PATCH] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
@ 2018-02-22 20:00 ` Ville Syrjälä
2018-02-22 20:12 ` Rodrigo Vivi
2018-02-23 22:52 ` [PATCH v2] " Rodrigo Vivi
2018-02-22 20:39 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Ville Syrjälä @ 2018-02-22 20:00 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Azhar Shaikh
On Thu, Feb 22, 2018 at 11:28:58AM -0800, Rodrigo Vivi wrote:
> According to Spec "Requirement before plane enabling or
> configuration change: Disable SAGV if any enabled plane will not
> be able to enable watermarks for memory latency >= SAGV block
> time, or any transcoder is interlaced. Else, enable SAGV."
>
> Currently we are only enabling and disabling SAGV on full
> modeset. If we end up changing plane configurations and
> sagv remains enabled when latency is higher than sagv block
> time the machine can hang.
>
> Also we are computing the latency values in different places
> and following different conditions/rules.
>
> So let's move the can_enable_sagv check to be inside
> skl_compute_wm and disable and enable on {pre,post} plane
> updates.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Azhar Shaikh <azhar.shaikh@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 18 +++++-----
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pm.c | 64 +++++++++++++-----------------------
> 3 files changed, 32 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 65c8487be7c7..008254579be1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_atomic_state *old_state = old_crtc_state->base.state;
> struct intel_crtc_state *pipe_config =
> intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
> @@ -5120,6 +5122,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> !old_primary_state->base.visible))
> intel_post_enable_primary(&crtc->base, pipe_config);
> }
> +
> + if (pipe_config->sagv)
> + intel_enable_sagv(dev_priv);
Unfortunately a single pipe can't make a unilateral decision to
enable sagv. The other pipes must be disabled first. And we can't use
state->active_crtcs for this in non-modeset commits since that's only
valid during a modeset.
cxsr has the same problem pretty much. Currently that's handled somewhat
hackily from foo_merge_wm() to disable cxsr when multiple pipes are
active. But I think a better idea might be to maintain a bitmask of
pipes allowing cxsr in dev_priv. And then have enable/disable hooks
that get called for each pipe from the commit path to manipulate the
bitmask and based on what the bitmask ends up looking either enable or
disable cxsr. That idea could be used for sagv as well I suppose.
> }
>
> static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> @@ -5205,6 +5210,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> pipe_config);
> else if (pipe_config->update_wm_pre)
> intel_update_watermarks(crtc);
> +
> + if (!pipe_config->sagv)
> + intel_disable_sagv(dev_priv);
> }
>
> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> @@ -12366,13 +12374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>
> intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
>
> - /*
> - * SKL workaround: bspec recommends we disable the SAGV when we
> - * have more then one pipe enabled
> - */
> - if (!intel_can_enable_sagv(state))
> - intel_disable_sagv(dev_priv);
> -
> intel_modeset_verify_disabled(dev, state);
> }
>
> @@ -12431,9 +12432,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> if (intel_state->modeset)
> intel_verify_planes(intel_state);
>
> - if (intel_state->modeset && intel_can_enable_sagv(state))
> - intel_enable_sagv(dev_priv);
> -
> drm_atomic_helper_commit_hw_done(state);
>
> if (intel_state->modeset) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1535bfb7ea40..4c10a8a94d73 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -712,6 +712,7 @@ struct intel_crtc_state {
> bool update_pipe; /* can a fast modeset be performed? */
> bool disable_cxsr;
> bool update_wm_pre, update_wm_post; /* watermarks are updated */
> + bool sagv; /* Disable SAGV on any latency higher than its block time */
> bool fb_changed; /* fb on any of the planes is changed */
> bool fifo_changed; /* FIFO split is changed */
>
> @@ -2001,7 +2002,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> struct skl_pipe_wm *out);
> void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> -bool intel_can_enable_sagv(struct drm_atomic_state *state);
> +bool intel_can_enable_sagv(struct intel_atomic_state *state, u32 latency);
> int intel_enable_sagv(struct drm_i915_private *dev_priv);
> int intel_disable_sagv(struct drm_i915_private *dev_priv);
> bool skl_wm_level_equals(const struct skl_wm_level *l1,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 21dac6ebc202..731b3808a62e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3681,16 +3681,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> -bool intel_can_enable_sagv(struct drm_atomic_state *state)
> +bool intel_can_enable_sagv(struct intel_atomic_state *intel_state, u32 latency)
> {
> - struct drm_device *dev = state->dev;
> + struct drm_device *dev = intel_state->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct intel_crtc *crtc;
> - struct intel_plane *plane;
> struct intel_crtc_state *cstate;
> enum pipe pipe;
> - int level, latency;
> int sagv_block_time_us;
>
> if (!intel_has_sagv(dev_priv))
> @@ -3703,6 +3700,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> else
> sagv_block_time_us = 10;
>
> + if (latency < sagv_block_time_us)
> + return false;
> +
> /*
> * SKL+ workaround: bspec recommends we disable the SAGV when we have
> * more then one pipe enabled
> @@ -3722,35 +3722,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> return false;
>
> - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - struct skl_plane_wm *wm =
> - &cstate->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 (skl_needs_memory_bw_wa(intel_state) &&
> - plane->base.state->fb->modifier ==
> - I915_FORMAT_MOD_X_TILED)
> - latency += 15;
> -
> - /*
> - * 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 the SAGV.
> - */
> - if (latency < sagv_block_time_us)
> - return false;
> - }
> -
> return true;
> }
>
> @@ -4501,7 +4472,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> const struct skl_wm_params *wp,
> uint16_t *out_blocks, /* out */
> uint8_t *out_lines, /* out */
> - bool *enabled /* out */)
> + bool *enabled, /* out */
> + bool *sagv /* out */)
> {
> const struct drm_plane_state *pstate = &intel_pstate->base;
> uint32_t latency = dev_priv->wm.skl_latency[level];
> @@ -4528,6 +4500,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> if (apply_memory_bw_wa && wp->x_tiled)
> latency += 15;
>
> + *sagv = intel_can_enable_sagv(state, latency);
> +
> method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> wp->cpp, latency, wp->dbuf_block_size);
> method2 = skl_wm_method2(wp->plane_pixel_rate,
> @@ -4629,7 +4603,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> const struct skl_wm_params *wm_params,
> - struct skl_plane_wm *wm)
> + struct skl_plane_wm *wm,
> + bool *sagv)
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> struct drm_plane *plane = intel_pstate->base.plane;
> @@ -4655,7 +4630,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> wm_params,
> &result->plane_res_b,
> &result->plane_res_l,
> - &result->plane_en);
> + &result->plane_en,
> + sagv);
> if (ret)
> return ret;
> }
> @@ -4743,7 +4719,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
>
> static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> struct skl_ddb_allocation *ddb,
> - struct skl_pipe_wm *pipe_wm)
> + struct skl_pipe_wm *pipe_wm,
> + bool *sagv)
> {
> struct drm_device *dev = cstate->base.crtc->dev;
> struct drm_crtc_state *crtc_state = &cstate->base;
> @@ -4777,7 +4754,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> return ret;
>
> ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> - intel_pstate, &wm_params, wm);
> + intel_pstate, &wm_params, wm, sagv);
> if (ret)
> return ret;
> skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> @@ -4899,12 +4876,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> const struct skl_pipe_wm *old_pipe_wm,
> struct skl_pipe_wm *pipe_wm, /* out */
> struct skl_ddb_allocation *ddb, /* out */
> - bool *changed /* out */)
> + bool *changed /* out */,
> + bool *sagv /* out */)
> {
> struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
> int ret;
>
> - ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
> + ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
> if (ret)
> return ret;
>
> @@ -5099,6 +5077,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> struct drm_device *dev = state->dev;
> struct skl_pipe_wm *pipe_wm;
> bool changed = false;
> + bool sagv = true;
> int ret, i;
>
> /*
> @@ -5147,7 +5126,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>
> pipe_wm = &intel_cstate->wm.skl.optimal;
> ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> - &results->ddb, &changed);
> + &results->ddb, &changed, &sagv);
> if (ret)
> return ret;
>
> @@ -5159,6 +5138,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> continue;
>
> intel_cstate->update_wm_pre = true;
> + intel_cstate->sagv &= sagv;
> }
>
> skl_print_wm_changes(state);
> --
> 2.13.6
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Disable SAGV on pre plane update.
2018-02-22 20:00 ` Ville Syrjälä
@ 2018-02-22 20:12 ` Rodrigo Vivi
2018-02-23 22:52 ` [PATCH v2] " Rodrigo Vivi
1 sibling, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2018-02-22 20:12 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Azhar Shaikh
On Thu, Feb 22, 2018 at 10:00:47PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 22, 2018 at 11:28:58AM -0800, Rodrigo Vivi wrote:
> > According to Spec "Requirement before plane enabling or
> > configuration change: Disable SAGV if any enabled plane will not
> > be able to enable watermarks for memory latency >= SAGV block
> > time, or any transcoder is interlaced. Else, enable SAGV."
> >
> > Currently we are only enabling and disabling SAGV on full
> > modeset. If we end up changing plane configurations and
> > sagv remains enabled when latency is higher than sagv block
> > time the machine can hang.
> >
> > Also we are computing the latency values in different places
> > and following different conditions/rules.
> >
> > So let's move the can_enable_sagv check to be inside
> > skl_compute_wm and disable and enable on {pre,post} plane
> > updates.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Azhar Shaikh <azhar.shaikh@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 18 +++++-----
> > drivers/gpu/drm/i915/intel_drv.h | 3 +-
> > drivers/gpu/drm/i915/intel_pm.c | 64 +++++++++++++-----------------------
> > 3 files changed, 32 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 65c8487be7c7..008254579be1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> > static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> > + struct drm_device *dev = crtc->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > struct drm_atomic_state *old_state = old_crtc_state->base.state;
> > struct intel_crtc_state *pipe_config =
> > intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
> > @@ -5120,6 +5122,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> > !old_primary_state->base.visible))
> > intel_post_enable_primary(&crtc->base, pipe_config);
> > }
> > +
> > + if (pipe_config->sagv)
> > + intel_enable_sagv(dev_priv);
>
> Unfortunately a single pipe can't make a unilateral decision to
> enable sagv. The other pipes must be disabled first.
I wondered that...
> And we can't use
> state->active_crtcs for this in non-modeset commits since that's only
> valid during a modeset.
but I thought this was enough :(
>
> cxsr has the same problem pretty much. Currently that's handled somewhat
> hackily from foo_merge_wm() to disable cxsr when multiple pipes are
> active. But I think a better idea might be to maintain a bitmask of
> pipes allowing cxsr in dev_priv. And then have enable/disable hooks
> that get called for each pipe from the commit path to manipulate the
> bitmask and based on what the bitmask ends up looking either enable or
> disable cxsr.
I considered some kind of bitmask for this case this morning, but
I thought that state->active_crtcs would take care of that so I ignored
and move forward.
> That idea could be used for sagv as well I suppose.
could it be the other way around?
>
> > }
> >
> > static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> > @@ -5205,6 +5210,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> > pipe_config);
> > else if (pipe_config->update_wm_pre)
> > intel_update_watermarks(crtc);
> > +
> > + if (!pipe_config->sagv)
> > + intel_disable_sagv(dev_priv);
> > }
> >
> > static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> > @@ -12366,13 +12374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >
> > intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> >
> > - /*
> > - * SKL workaround: bspec recommends we disable the SAGV when we
> > - * have more then one pipe enabled
> > - */
> > - if (!intel_can_enable_sagv(state))
> > - intel_disable_sagv(dev_priv);
> > -
> > intel_modeset_verify_disabled(dev, state);
> > }
> >
> > @@ -12431,9 +12432,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> > if (intel_state->modeset)
> > intel_verify_planes(intel_state);
> >
> > - if (intel_state->modeset && intel_can_enable_sagv(state))
> > - intel_enable_sagv(dev_priv);
> > -
> > drm_atomic_helper_commit_hw_done(state);
> >
> > if (intel_state->modeset) {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1535bfb7ea40..4c10a8a94d73 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -712,6 +712,7 @@ struct intel_crtc_state {
> > bool update_pipe; /* can a fast modeset be performed? */
> > bool disable_cxsr;
> > bool update_wm_pre, update_wm_post; /* watermarks are updated */
> > + bool sagv; /* Disable SAGV on any latency higher than its block time */
> > bool fb_changed; /* fb on any of the planes is changed */
> > bool fifo_changed; /* FIFO split is changed */
> >
> > @@ -2001,7 +2002,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> > struct skl_pipe_wm *out);
> > void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> > void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> > -bool intel_can_enable_sagv(struct drm_atomic_state *state);
> > +bool intel_can_enable_sagv(struct intel_atomic_state *state, u32 latency);
> > int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > bool skl_wm_level_equals(const struct skl_wm_level *l1,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 21dac6ebc202..731b3808a62e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3681,16 +3681,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > return 0;
> > }
> >
> > -bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > +bool intel_can_enable_sagv(struct intel_atomic_state *intel_state, u32 latency)
> > {
> > - struct drm_device *dev = state->dev;
> > + struct drm_device *dev = intel_state->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > struct intel_crtc *crtc;
> > - struct intel_plane *plane;
> > struct intel_crtc_state *cstate;
> > enum pipe pipe;
> > - int level, latency;
> > int sagv_block_time_us;
> >
> > if (!intel_has_sagv(dev_priv))
> > @@ -3703,6 +3700,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > else
> > sagv_block_time_us = 10;
> >
> > + if (latency < sagv_block_time_us)
> > + return false;
> > +
> > /*
> > * SKL+ workaround: bspec recommends we disable the SAGV when we have
> > * more then one pipe enabled
> > @@ -3722,35 +3722,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > return false;
> >
> > - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > - struct skl_plane_wm *wm =
> > - &cstate->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 (skl_needs_memory_bw_wa(intel_state) &&
> > - plane->base.state->fb->modifier ==
> > - I915_FORMAT_MOD_X_TILED)
> > - latency += 15;
> > -
> > - /*
> > - * 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 the SAGV.
> > - */
> > - if (latency < sagv_block_time_us)
> > - return false;
> > - }
> > -
> > return true;
> > }
> >
> > @@ -4501,7 +4472,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > const struct skl_wm_params *wp,
> > uint16_t *out_blocks, /* out */
> > uint8_t *out_lines, /* out */
> > - bool *enabled /* out */)
> > + bool *enabled, /* out */
> > + bool *sagv /* out */)
> > {
> > const struct drm_plane_state *pstate = &intel_pstate->base;
> > uint32_t latency = dev_priv->wm.skl_latency[level];
> > @@ -4528,6 +4500,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> > if (apply_memory_bw_wa && wp->x_tiled)
> > latency += 15;
> >
> > + *sagv = intel_can_enable_sagv(state, latency);
> > +
> > method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> > wp->cpp, latency, wp->dbuf_block_size);
> > method2 = skl_wm_method2(wp->plane_pixel_rate,
> > @@ -4629,7 +4603,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > struct intel_crtc_state *cstate,
> > const struct intel_plane_state *intel_pstate,
> > const struct skl_wm_params *wm_params,
> > - struct skl_plane_wm *wm)
> > + struct skl_plane_wm *wm,
> > + bool *sagv)
> > {
> > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> > struct drm_plane *plane = intel_pstate->base.plane;
> > @@ -4655,7 +4630,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > wm_params,
> > &result->plane_res_b,
> > &result->plane_res_l,
> > - &result->plane_en);
> > + &result->plane_en,
> > + sagv);
> > if (ret)
> > return ret;
> > }
> > @@ -4743,7 +4719,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> >
> > static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > struct skl_ddb_allocation *ddb,
> > - struct skl_pipe_wm *pipe_wm)
> > + struct skl_pipe_wm *pipe_wm,
> > + bool *sagv)
> > {
> > struct drm_device *dev = cstate->base.crtc->dev;
> > struct drm_crtc_state *crtc_state = &cstate->base;
> > @@ -4777,7 +4754,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> > return ret;
> >
> > ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> > - intel_pstate, &wm_params, wm);
> > + intel_pstate, &wm_params, wm, sagv);
> > if (ret)
> > return ret;
> > skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> > @@ -4899,12 +4876,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> > const struct skl_pipe_wm *old_pipe_wm,
> > struct skl_pipe_wm *pipe_wm, /* out */
> > struct skl_ddb_allocation *ddb, /* out */
> > - bool *changed /* out */)
> > + bool *changed /* out */,
> > + bool *sagv /* out */)
> > {
> > struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
> > int ret;
> >
> > - ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
> > + ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
> > if (ret)
> > return ret;
> >
> > @@ -5099,6 +5077,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> > struct drm_device *dev = state->dev;
> > struct skl_pipe_wm *pipe_wm;
> > bool changed = false;
> > + bool sagv = true;
> > int ret, i;
> >
> > /*
> > @@ -5147,7 +5126,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> >
> > pipe_wm = &intel_cstate->wm.skl.optimal;
> > ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> > - &results->ddb, &changed);
> > + &results->ddb, &changed, &sagv);
> > if (ret)
> > return ret;
> >
> > @@ -5159,6 +5138,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> > continue;
> >
> > intel_cstate->update_wm_pre = true;
> > + intel_cstate->sagv &= sagv;
> > }
> >
> > skl_print_wm_changes(state);
> > --
> > 2.13.6
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Disable SAGV on pre plane update.
2018-02-22 19:28 [PATCH] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
2018-02-22 20:00 ` Ville Syrjälä
@ 2018-02-22 20:39 ` Patchwork
2018-02-23 3:43 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-02-23 23:09 ` ✗ Fi.CI.BAT: failure for drm/i915: Disable SAGV on pre plane update. (rev2) Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-02-22 20:39 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Disable SAGV on pre plane update.
URL : https://patchwork.freedesktop.org/series/38806/
State : success
== Summary ==
Series 38806v1 drm/i915: Disable SAGV on pre plane update.
https://patchwork.freedesktop.org/api/1.0/series/38806/revisions/1/mbox/
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-a:
fail -> PASS (fi-skl-6700k2)
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:418s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:422s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:373s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:493s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:288s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:475s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:479s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:471s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:457s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:562s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:414s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:285s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:513s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:387s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:409s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:455s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:412s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:453s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:494s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:491s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:585s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:427s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:499s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:521s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:489s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:484s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:409s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:425s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:518s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:399s
Blacklisted hosts:
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:393s
b9f1df86d4379ec76dddc07cf6e1cc85ffb26ffd drm-tip: 2018y-02m-22d-18h-25m-51s UTC integration manifest
0ded1e800322 drm/i915: Disable SAGV on pre plane update.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8133/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Disable SAGV on pre plane update.
2018-02-22 19:28 [PATCH] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
2018-02-22 20:00 ` Ville Syrjälä
2018-02-22 20:39 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-02-23 3:43 ` Patchwork
2018-02-23 23:09 ` ✗ Fi.CI.BAT: failure for drm/i915: Disable SAGV on pre plane update. (rev2) Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-02-23 3:43 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Disable SAGV on pre plane update.
URL : https://patchwork.freedesktop.org/series/38806/
State : failure
== Summary ==
Test kms_flip_tiling:
Subgroup flip-to-yf-tiled:
fail -> PASS (shard-apl) fdo#103822
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-primscrn-indfb-msflip-blt:
fail -> PASS (shard-apl) fdo#103167
Test gem_eio:
Subgroup in-flight:
pass -> INCOMPLETE (shard-apl) fdo#104945
Test kms_chv_cursor_fail:
Subgroup pipe-b-256x256-top-edge:
pass -> DMESG-WARN (shard-snb)
Test kms_vblank:
Subgroup pipe-b-ts-continuation-suspend:
pass -> SKIP (shard-snb)
Test kms_flip:
Subgroup 2x-wf_vblank-ts-check:
fail -> PASS (shard-hsw) fdo#100368 +1
Test drv_suspend:
Subgroup forcewake:
pass -> SKIP (shard-snb)
Test kms_atomic_transition:
Subgroup 2x-modeset-transitions:
pass -> INCOMPLETE (shard-hsw)
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
shard-apl total:3454 pass:1813 dwarn:1 dfail:0 fail:12 skip:1627 time:12041s
shard-hsw total:3455 pass:1763 dwarn:1 dfail:0 fail:2 skip:1687 time:11331s
shard-snb total:3465 pass:1355 dwarn:2 dfail:0 fail:2 skip:2106 time:6607s
Blacklisted hosts:
shard-kbl total:3465 pass:1957 dwarn:7 dfail:0 fail:14 skip:1487 time:9597s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8133/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drm/i915: Disable SAGV on pre plane update.
2018-02-22 20:00 ` Ville Syrjälä
2018-02-22 20:12 ` Rodrigo Vivi
@ 2018-02-23 22:52 ` Rodrigo Vivi
2018-02-26 4:45 ` kbuild test robot
1 sibling, 1 reply; 9+ messages in thread
From: Rodrigo Vivi @ 2018-02-23 22:52 UTC (permalink / raw)
To: intel-gfx; +Cc: Azhar Shaikh, Rodrigo Vivi
According to Spec "Requirement before plane enabling or
configuration change: Disable SAGV if any enabled plane will not
be able to enable watermarks for memory latency >= SAGV block
time, or any transcoder is interlaced. Else, enable SAGV."
Currently we are only enabling and disabling SAGV on full
modeset. If we end up changing plane configurations and
sagv remains enabled when latency is higher than sagv block
time the machine can hang.
Also we are computing the latency values in different places
and following different conditions/rules. So let's move the
the sagv block time limit check to be inside skl_compute_wm
and, if necessary, disable SAGV on pre plane updates.
SAGV defaults to enabled by the BIOS, so we need to be more
careful and disable everytime we see a mismatch on its
conditions.
v2: - Consider only highest enabled wm level for SAGV block
time limitation.
- Handle sagv bool in a way that we properly consider all
the planes. So we don't end up always disabling SAGV.
- (Ville) Don't enabled on post_plane update, otherwise one
pipe ends up enabling without checking for others. So keep
the old enable/disable solution on atomic commit tail
so we continue following the spec to disable on multiple
pipe or interlaced and re-enable on modeset of a single
pipe non interlaced. Always respecting the latency.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Azhar Shaikh <azhar.shaikh@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 6 +-
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pm.c | 107 +++++++++++++++++------------------
3 files changed, 60 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65c8487be7c7..d98ff5916c85 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_atomic_state *old_state = old_crtc_state->base.state;
struct intel_crtc_state *pipe_config =
intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
@@ -5205,6 +5207,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
pipe_config);
else if (pipe_config->update_wm_pre)
intel_update_watermarks(crtc);
+
+ if (!pipe_config->sagv)
+ intel_disable_sagv(dev_priv);
}
static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -12372,7 +12377,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
*/
if (!intel_can_enable_sagv(state))
intel_disable_sagv(dev_priv);
-
intel_modeset_verify_disabled(dev, state);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1535bfb7ea40..294b6ac6c72c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_crtc_state {
bool update_pipe; /* can a fast modeset be performed? */
bool disable_cxsr;
bool update_wm_pre, update_wm_post; /* watermarks are updated */
+ bool sagv; /* Disable SAGV on any latency higher than its block time */
bool fb_changed; /* fb on any of the planes is changed */
bool fifo_changed; /* FIFO split is changed */
@@ -2002,6 +2003,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
bool intel_can_enable_sagv(struct drm_atomic_state *state);
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv);
int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
bool skl_wm_level_equals(const struct skl_wm_level *l1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21dac6ebc202..0bf3bf386def 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3687,22 +3687,12 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct intel_crtc *crtc;
- struct intel_plane *plane;
struct intel_crtc_state *cstate;
enum pipe pipe;
- int level, latency;
- int sagv_block_time_us;
if (!intel_has_sagv(dev_priv))
return false;
- if (IS_GEN9(dev_priv))
- sagv_block_time_us = 30;
- else if (IS_GEN10(dev_priv))
- sagv_block_time_us = 20;
- else
- sagv_block_time_us = 10;
-
/*
* SKL+ workaround: bspec recommends we disable the SAGV when we have
* more then one pipe enabled
@@ -3719,41 +3709,27 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
cstate = to_intel_crtc_state(crtc->base.state);
- if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+ /* Also check for latency here */
+ cstate = to_intel_crtc_state(crtc->base.state);
+ if (!cstate->sagv)
return false;
- for_each_intel_plane_on_crtc(dev, crtc, plane) {
- struct skl_plane_wm *wm =
- &cstate->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 (skl_needs_memory_bw_wa(intel_state) &&
- plane->base.state->fb->modifier ==
- I915_FORMAT_MOD_X_TILED)
- latency += 15;
-
- /*
- * 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 the SAGV.
- */
- if (latency < sagv_block_time_us)
- return false;
- }
+ if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+ return false;
return true;
}
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv)
+{
+ if (INTEL_GEN(dev_priv) >= 11)
+ return 10;
+ else if (IS_GEN10(dev_priv))
+ return 20;
+ else
+ return 30;
+}
+
static void
skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
const struct intel_crtc_state *cstate,
@@ -4501,10 +4477,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
const struct skl_wm_params *wp,
uint16_t *out_blocks, /* out */
uint8_t *out_lines, /* out */
- bool *enabled /* out */)
+ bool *enabled, /* out */
+ uint32_t *latency /* out */)
{
const struct drm_plane_state *pstate = &intel_pstate->base;
- uint32_t latency = dev_priv->wm.skl_latency[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
uint32_t res_blocks, res_lines;
@@ -4513,7 +4489,9 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
uint32_t min_disp_buf_needed;
- if (latency == 0 ||
+ *latency = dev_priv->wm.skl_latency[level];
+
+ if (*latency == 0 ||
!intel_wm_plane_visible(cstate, intel_pstate)) {
*enabled = false;
return 0;
@@ -4523,16 +4501,16 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
dev_priv->ipc_enabled)
- latency += 4;
+ *latency += 4;
if (apply_memory_bw_wa && wp->x_tiled)
- latency += 15;
+ *latency += 15;
method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
- wp->cpp, latency, wp->dbuf_block_size);
+ wp->cpp, *latency, wp->dbuf_block_size);
method2 = skl_wm_method2(wp->plane_pixel_rate,
cstate->base.adjusted_mode.crtc_htotal,
- latency,
+ *latency,
wp->plane_blocks_per_line);
if (wp->y_tiled) {
@@ -4545,7 +4523,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
else if (ddb_allocation >=
fixed16_to_u32_round_up(wp->plane_blocks_per_line))
selected_result = min_fixed16(method1, method2);
- else if (latency >= wp->linetime_us)
+ else if (*latency >= wp->linetime_us)
selected_result = min_fixed16(method1, method2);
else
selected_result = method1;
@@ -4629,7 +4607,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
const struct skl_wm_params *wm_params,
- struct skl_plane_wm *wm)
+ struct skl_plane_wm *wm,
+ bool *plane_sagv)
{
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
struct drm_plane *plane = intel_pstate->base.plane;
@@ -4638,6 +4617,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
enum pipe pipe = intel_crtc->pipe;
int level, max_level = ilk_wm_max_level(dev_priv);
int ret;
+ u32 latency;
if (WARN_ON(!intel_pstate->base.fb))
return -EINVAL;
@@ -4655,9 +4635,18 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
wm_params,
&result->plane_res_b,
&result->plane_res_l,
- &result->plane_en);
+ &result->plane_en,
+ &latency);
+
if (ret)
return ret;
+
+ /*
+ * Only consider the latency of highest enabled wm level
+ * on the plane for SAGV block time limitation.
+ */
+ if (result->plane_en)
+ *plane_sagv = latency >= intel_sagv_block_time(dev_priv);
}
return 0;
@@ -4743,7 +4732,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
struct skl_ddb_allocation *ddb,
- struct skl_pipe_wm *pipe_wm)
+ struct skl_pipe_wm *pipe_wm,
+ bool *sagv)
{
struct drm_device *dev = cstate->base.crtc->dev;
struct drm_crtc_state *crtc_state = &cstate->base;
@@ -4751,6 +4741,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
struct drm_plane *plane;
const struct drm_plane_state *pstate;
struct skl_plane_wm *wm;
+ bool plane_sagv = false;
int ret;
/*
@@ -4777,9 +4768,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
- intel_pstate, &wm_params, wm);
+ intel_pstate, &wm_params, wm, &plane_sagv);
if (ret)
return ret;
+
+ /* Take all planes in consideration for SAGV block time check */
+ *sagv &= plane_sagv;
+
skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
ddb_blocks, &wm->trans_wm);
}
@@ -4899,12 +4894,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
const struct skl_pipe_wm *old_pipe_wm,
struct skl_pipe_wm *pipe_wm, /* out */
struct skl_ddb_allocation *ddb, /* out */
- bool *changed /* out */)
+ bool *changed /* out */,
+ bool *sagv /* out */)
{
struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
int ret;
- ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+ ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
if (ret)
return ret;
@@ -5099,6 +5095,7 @@ skl_compute_wm(struct drm_atomic_state *state)
struct drm_device *dev = state->dev;
struct skl_pipe_wm *pipe_wm;
bool changed = false;
+ bool sagv = true;
int ret, i;
/*
@@ -5147,10 +5144,12 @@ skl_compute_wm(struct drm_atomic_state *state)
pipe_wm = &intel_cstate->wm.skl.optimal;
ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
- &results->ddb, &changed);
+ &results->ddb, &changed, &sagv);
if (ret)
return ret;
+ intel_cstate->sagv = sagv;
+
if (changed)
results->dirty_pipes |= drm_crtc_mask(crtc);
--
2.13.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Disable SAGV on pre plane update. (rev2)
2018-02-22 19:28 [PATCH] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
` (2 preceding siblings ...)
2018-02-23 3:43 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-02-23 23:09 ` Patchwork
2018-02-23 23:18 ` [PATCH v3] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
3 siblings, 1 reply; 9+ messages in thread
From: Patchwork @ 2018-02-23 23:09 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Disable SAGV on pre plane update. (rev2)
URL : https://patchwork.freedesktop.org/series/38806/
State : failure
== Summary ==
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CHK include/generated/bounds.h
CHK include/generated/timeconst.h
CHK include/generated/asm-offsets.h
CALL scripts/checksyscalls.sh
DESCEND objtool
CHK scripts/mod/devicetable-offsets.h
CHK include/generated/compile.h
CHK kernel/config_data.h
CC [M] drivers/gpu/drm/i915/intel_display.o
drivers/gpu/drm/i915/intel_display.c: In function ‘intel_post_plane_update’:
drivers/gpu/drm/i915/intel_display.c:5094:27: error: unused variable ‘dev_priv’ [-Werror=unused-variable]
struct drm_i915_private *dev_priv = to_i915(dev);
^~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:316: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
scripts/Makefile.build:575: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:575: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:575: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1048: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] drm/i915: Disable SAGV on pre plane update.
2018-02-23 23:09 ` ✗ Fi.CI.BAT: failure for drm/i915: Disable SAGV on pre plane update. (rev2) Patchwork
@ 2018-02-23 23:18 ` Rodrigo Vivi
0 siblings, 0 replies; 9+ messages in thread
From: Rodrigo Vivi @ 2018-02-23 23:18 UTC (permalink / raw)
To: intel-gfx; +Cc: Azhar Shaikh, Rodrigo Vivi
According to Spec "Requirement before plane enabling or
configuration change: Disable SAGV if any enabled plane will not
be able to enable watermarks for memory latency >= SAGV block
time, or any transcoder is interlaced. Else, enable SAGV."
Currently we are only enabling and disabling SAGV on full
modeset. If we end up changing plane configurations and
sagv remains enabled when latency is higher than sagv block
time the machine can hang.
Also we are computing the latency values in different places
and following different conditions/rules. So let's move the
the sagv block time limit check to be inside skl_compute_wm
and, if necessary, disable SAGV on pre plane updates.
SAGV defaults to enabled by the BIOS, so we need to be more
careful and disable everytime we see a mismatch on its
conditions.
v2: - Consider only highest enabled wm level for SAGV block
time limitation.
- Handle sagv bool in a way that we properly consider all
the planes. So we don't end up always disabling SAGV.
- (Ville) Don't enabled on post_plane update, otherwise one
pipe ends up enabling without checking for others. So keep
the old enable/disable solution on atomic commit tail
so we continue following the spec to disable on multiple
pipe or interlaced and re-enable on modeset of a single
pipe non interlaced. Always respecting the latency.
v3: Remove unused dev and dev_priv. Forgot from v1 on v2.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Azhar Shaikh <azhar.shaikh@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 25 +++++++-
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pm.c | 108 +++++++++++++++++------------------
3 files changed, 78 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65c8487be7c7..7b3af459860a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5098,10 +5098,14 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
struct drm_plane_state *old_pri_state =
drm_atomic_get_existing_plane_state(old_state, primary);
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
+
intel_frontbuffer_flip(to_i915(crtc->base.dev), pipe_config->fb_bits);
- if (pipe_config->update_wm_post && pipe_config->base.active)
+ if (pipe_config->update_wm_post && pipe_config->base.active) {
+ DRM_ERROR("I915-DEBUG: %s %d - Update WM post \n", __FUNCTION__, __LINE__);
intel_update_watermarks(crtc);
+ }
if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
hsw_enable_ips(pipe_config);
@@ -5203,8 +5207,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
if (dev_priv->display.initial_watermarks != NULL)
dev_priv->display.initial_watermarks(old_intel_state,
pipe_config);
- else if (pipe_config->update_wm_pre)
+ else if (pipe_config->update_wm_pre) {
+ DRM_ERROR("I915-DEBUG: %s %d - Update WM pre?\n", __FUNCTION__, __LINE__);
intel_update_watermarks(crtc);
+ }
+
+ if (!pipe_config->sagv)
+ intel_disable_sagv(dev_priv);
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
}
static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
@@ -5214,6 +5224,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask
struct drm_plane *p;
int pipe = intel_crtc->pipe;
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_crtc_dpms_overlay_disable(intel_crtc);
drm_for_each_plane_mask(p, dev, plane_mask)
@@ -12152,6 +12163,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
update_scanline_offset(intel_crtc);
dev_priv->display.crtc_enable(pipe_config, state);
} else {
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
pipe_config);
}
@@ -12171,6 +12183,8 @@ static void intel_update_crtcs(struct drm_atomic_state *state)
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
int i;
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
+
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
if (!new_crtc_state->active)
continue;
@@ -12326,10 +12340,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
if (!needs_modeset(new_crtc_state))
continue;
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
to_intel_crtc_state(new_crtc_state));
if (old_crtc_state->active) {
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
intel_crtc->active = false;
@@ -12372,7 +12388,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
*/
if (!intel_can_enable_sagv(state))
intel_disable_sagv(dev_priv);
-
intel_modeset_verify_disabled(dev, state);
}
@@ -12390,6 +12405,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
}
}
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
dev_priv->display.update_crtcs(state);
@@ -12420,6 +12436,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
}
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
if (put_domains[i])
@@ -12461,6 +12478,7 @@ static void intel_atomic_commit_work(struct work_struct *work)
struct drm_atomic_state *state =
container_of(work, struct drm_atomic_state, commit_work);
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
intel_atomic_commit_tail(state);
}
@@ -12586,6 +12604,7 @@ static int intel_atomic_commit(struct drm_device *dev,
}
drm_atomic_state_get(state);
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
INIT_WORK(&state->commit_work, intel_atomic_commit_work);
i915_sw_fence_commit(&intel_state->commit_ready);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1535bfb7ea40..0f733c5f0faf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_crtc_state {
bool update_pipe; /* can a fast modeset be performed? */
bool disable_cxsr;
bool update_wm_pre, update_wm_post; /* watermarks are updated */
+ bool sagv; /* Disable SAGV when latency is higher than its block time */
bool fb_changed; /* fb on any of the planes is changed */
bool fifo_changed; /* FIFO split is changed */
@@ -2002,6 +2003,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
bool intel_can_enable_sagv(struct drm_atomic_state *state);
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv);
int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
bool skl_wm_level_equals(const struct skl_wm_level *l1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21dac6ebc202..92a14cb7e674 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3687,22 +3687,12 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct intel_crtc *crtc;
- struct intel_plane *plane;
struct intel_crtc_state *cstate;
enum pipe pipe;
- int level, latency;
- int sagv_block_time_us;
if (!intel_has_sagv(dev_priv))
return false;
- if (IS_GEN9(dev_priv))
- sagv_block_time_us = 30;
- else if (IS_GEN10(dev_priv))
- sagv_block_time_us = 20;
- else
- sagv_block_time_us = 10;
-
/*
* SKL+ workaround: bspec recommends we disable the SAGV when we have
* more then one pipe enabled
@@ -3719,41 +3709,27 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
cstate = to_intel_crtc_state(crtc->base.state);
- if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+ /* Also check for latency here */
+ cstate = to_intel_crtc_state(crtc->base.state);
+ if (!cstate->sagv)
return false;
- for_each_intel_plane_on_crtc(dev, crtc, plane) {
- struct skl_plane_wm *wm =
- &cstate->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 (skl_needs_memory_bw_wa(intel_state) &&
- plane->base.state->fb->modifier ==
- I915_FORMAT_MOD_X_TILED)
- latency += 15;
-
- /*
- * 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 the SAGV.
- */
- if (latency < sagv_block_time_us)
- return false;
- }
+ if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
+ return false;
return true;
}
+u8 intel_sagv_block_time(const struct drm_i915_private *dev_priv)
+{
+ if (INTEL_GEN(dev_priv) >= 11)
+ return 10;
+ else if (IS_GEN10(dev_priv))
+ return 20;
+ else
+ return 30;
+}
+
static void
skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
const struct intel_crtc_state *cstate,
@@ -4501,10 +4477,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
const struct skl_wm_params *wp,
uint16_t *out_blocks, /* out */
uint8_t *out_lines, /* out */
- bool *enabled /* out */)
+ bool *enabled, /* out */
+ uint32_t *latency /* out */)
{
const struct drm_plane_state *pstate = &intel_pstate->base;
- uint32_t latency = dev_priv->wm.skl_latency[level];
uint_fixed_16_16_t method1, method2;
uint_fixed_16_16_t selected_result;
uint32_t res_blocks, res_lines;
@@ -4513,7 +4489,9 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
uint32_t min_disp_buf_needed;
- if (latency == 0 ||
+ *latency = dev_priv->wm.skl_latency[level];
+
+ if (*latency == 0 ||
!intel_wm_plane_visible(cstate, intel_pstate)) {
*enabled = false;
return 0;
@@ -4523,16 +4501,16 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
if ((IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
IS_CNL_REVID(dev_priv, CNL_REVID_A0, CNL_REVID_B0)) &&
dev_priv->ipc_enabled)
- latency += 4;
+ *latency += 4;
if (apply_memory_bw_wa && wp->x_tiled)
- latency += 15;
+ *latency += 15;
method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
- wp->cpp, latency, wp->dbuf_block_size);
+ wp->cpp, *latency, wp->dbuf_block_size);
method2 = skl_wm_method2(wp->plane_pixel_rate,
cstate->base.adjusted_mode.crtc_htotal,
- latency,
+ *latency,
wp->plane_blocks_per_line);
if (wp->y_tiled) {
@@ -4545,7 +4523,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
else if (ddb_allocation >=
fixed16_to_u32_round_up(wp->plane_blocks_per_line))
selected_result = min_fixed16(method1, method2);
- else if (latency >= wp->linetime_us)
+ else if (*latency >= wp->linetime_us)
selected_result = min_fixed16(method1, method2);
else
selected_result = method1;
@@ -4629,7 +4607,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
struct intel_crtc_state *cstate,
const struct intel_plane_state *intel_pstate,
const struct skl_wm_params *wm_params,
- struct skl_plane_wm *wm)
+ struct skl_plane_wm *wm,
+ bool *plane_sagv)
{
struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
struct drm_plane *plane = intel_pstate->base.plane;
@@ -4638,6 +4617,7 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
enum pipe pipe = intel_crtc->pipe;
int level, max_level = ilk_wm_max_level(dev_priv);
int ret;
+ u32 latency;
if (WARN_ON(!intel_pstate->base.fb))
return -EINVAL;
@@ -4655,9 +4635,18 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
wm_params,
&result->plane_res_b,
&result->plane_res_l,
- &result->plane_en);
+ &result->plane_en,
+ &latency);
+
if (ret)
return ret;
+
+ /*
+ * Only consider the latency of highest enabled wm level
+ * on the plane for SAGV block time limitation.
+ */
+ if (result->plane_en)
+ *plane_sagv = latency >= intel_sagv_block_time(dev_priv);
}
return 0;
@@ -4743,7 +4732,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
struct skl_ddb_allocation *ddb,
- struct skl_pipe_wm *pipe_wm)
+ struct skl_pipe_wm *pipe_wm,
+ bool *sagv)
{
struct drm_device *dev = cstate->base.crtc->dev;
struct drm_crtc_state *crtc_state = &cstate->base;
@@ -4751,6 +4741,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
struct drm_plane *plane;
const struct drm_plane_state *pstate;
struct skl_plane_wm *wm;
+ bool plane_sagv = false;
int ret;
/*
@@ -4777,9 +4768,13 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
return ret;
ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
- intel_pstate, &wm_params, wm);
+ intel_pstate, &wm_params, wm, &plane_sagv);
if (ret)
return ret;
+
+ /* Take all planes in consideration for SAGV block time check */
+ *sagv &= plane_sagv;
+
skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
ddb_blocks, &wm->trans_wm);
}
@@ -4810,6 +4805,7 @@ static void skl_write_wm_level(struct drm_i915_private *dev_priv,
val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
}
+ DRM_ERROR("I915-DEBUG: %s %d\n", __FUNCTION__, __LINE__);
I915_WRITE(reg, val);
}
@@ -4899,12 +4895,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
const struct skl_pipe_wm *old_pipe_wm,
struct skl_pipe_wm *pipe_wm, /* out */
struct skl_ddb_allocation *ddb, /* out */
- bool *changed /* out */)
+ bool *changed /* out */,
+ bool *sagv /* out */)
{
struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
int ret;
- ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+ ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
if (ret)
return ret;
@@ -5099,6 +5096,7 @@ skl_compute_wm(struct drm_atomic_state *state)
struct drm_device *dev = state->dev;
struct skl_pipe_wm *pipe_wm;
bool changed = false;
+ bool sagv = true;
int ret, i;
/*
@@ -5147,10 +5145,12 @@ skl_compute_wm(struct drm_atomic_state *state)
pipe_wm = &intel_cstate->wm.skl.optimal;
ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
- &results->ddb, &changed);
+ &results->ddb, &changed, &sagv);
if (ret)
return ret;
+ intel_cstate->sagv = sagv;
+
if (changed)
results->dirty_pipes |= drm_crtc_mask(crtc);
--
2.13.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/i915: Disable SAGV on pre plane update.
2018-02-23 22:52 ` [PATCH v2] " Rodrigo Vivi
@ 2018-02-26 4:45 ` kbuild test robot
0 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2018-02-26 4:45 UTC (permalink / raw)
Cc: intel-gfx, Rodrigo Vivi, kbuild-all, Azhar Shaikh
[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]
Hi Rodrigo,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20180223]
[cannot apply to v4.16-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i915-Disable-SAGV-on-pre-plane-update/20180226-094030
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x003-02260939 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu//drm/i915/intel_display.c: In function 'intel_post_plane_update':
>> drivers/gpu//drm/i915/intel_display.c:5094:27: error: unused variable 'dev_priv' [-Werror=unused-variable]
struct drm_i915_private *dev_priv = to_i915(dev);
^~~~~~~~
cc1: all warnings being treated as errors
vim +/dev_priv +5094 drivers/gpu//drm/i915/intel_display.c
5089
5090 static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
5091 {
5092 struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
5093 struct drm_device *dev = crtc->base.dev;
> 5094 struct drm_i915_private *dev_priv = to_i915(dev);
5095 struct drm_atomic_state *old_state = old_crtc_state->base.state;
5096 struct intel_crtc_state *pipe_config =
5097 intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
5098 crtc);
5099 struct drm_plane *primary = crtc->base.primary;
5100 struct drm_plane_state *old_pri_state =
5101 drm_atomic_get_existing_plane_state(old_state, primary);
5102
5103 intel_frontbuffer_flip(to_i915(crtc->base.dev), pipe_config->fb_bits);
5104
5105 if (pipe_config->update_wm_post && pipe_config->base.active)
5106 intel_update_watermarks(crtc);
5107
5108 if (hsw_post_update_enable_ips(old_crtc_state, pipe_config))
5109 hsw_enable_ips(pipe_config);
5110
5111 if (old_pri_state) {
5112 struct intel_plane_state *primary_state =
5113 intel_atomic_get_new_plane_state(to_intel_atomic_state(old_state),
5114 to_intel_plane(primary));
5115 struct intel_plane_state *old_primary_state =
5116 to_intel_plane_state(old_pri_state);
5117
5118 intel_fbc_post_update(crtc);
5119
5120 if (primary_state->base.visible &&
5121 (needs_modeset(&pipe_config->base) ||
5122 !old_primary_state->base.visible))
5123 intel_post_enable_primary(&crtc->base, pipe_config);
5124 }
5125 }
5126
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29346 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-02-26 4:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-22 19:28 [PATCH] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
2018-02-22 20:00 ` Ville Syrjälä
2018-02-22 20:12 ` Rodrigo Vivi
2018-02-23 22:52 ` [PATCH v2] " Rodrigo Vivi
2018-02-26 4:45 ` kbuild test robot
2018-02-22 20:39 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-02-23 3:43 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-02-23 23:09 ` ✗ Fi.CI.BAT: failure for drm/i915: Disable SAGV on pre plane update. (rev2) Patchwork
2018-02-23 23:18 ` [PATCH v3] drm/i915: Disable SAGV on pre plane update Rodrigo Vivi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.