Hi,
On Monday 07 August 2017 04:18 PM, Maarten Lankhorst wrote:
The gen3 watermark calculations are converted to atomic,
but the wm update calls are still done through the legacy
functions.
This will make it easier to bisect things if they go wrong.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/intel_display.c | 3 +-
drivers/gpu/drm/i915/intel_drv.h | 14 +++
drivers/gpu/drm/i915/intel_pm.c | 231 +++++++++++++++++++++--------------
3 files changed, 152 insertions(+), 96 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 974d4b577189..234b94cafc7d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14709,7 +14709,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
skl_wm_get_hw_state(dev);
} else if (HAS_PCH_SPLIT(dev_priv)) {
ilk_wm_get_hw_state(dev);
- }
+ } else if (INTEL_GEN(dev_priv) <= 3 && !IS_PINEVIEW(dev_priv))
+ i9xx_wm_get_hw_state(dev);
for_each_intel_crtc(dev, crtc) {
u64 put_domains;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f91de9cb9697..d53d34756048 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -547,6 +547,15 @@ struct g4x_wm_state {
bool fbc_en;
};
+struct i9xx_wm_state {
+ uint16_t plane_wm;
+ bool cxsr;
+
+ struct {
+ uint16_t plane;
should this also be named as plane_wm, because it's again wm with SR.
just a nitpick but not a stopper.
+ } sr;
+};
+
struct intel_crtc_wm_state {
union {
struct {
@@ -591,6 +600,9 @@ struct intel_crtc_wm_state {
/* optimal watermarks */
struct g4x_wm_state optimal;
} g4x;
new line please to match the coding style of function
+ struct {
+ struct i9xx_wm_state optimal;
+ } i9xx;
};
/*
@@ -823,6 +835,7 @@ struct intel_crtc {
struct intel_pipe_wm ilk;
struct vlv_wm_state vlv;
struct g4x_wm_state g4x;
+ struct i9xx_wm_state i9xx;
} active;
} wm;
@@ -1845,6 +1858,7 @@ void gen6_rps_boost(struct drm_i915_gem_request *rq,
struct intel_rps_client *rps);
void intel_queue_rps_boost_for_request(struct drm_i915_gem_request *req);
void g4x_wm_get_hw_state(struct drm_device *dev);
+void i9xx_wm_get_hw_state(struct drm_device *dev);
void vlv_wm_get_hw_state(struct drm_device *dev);
void ilk_wm_get_hw_state(struct drm_device *dev);
void skl_wm_get_hw_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6e393b217450..807c9a730020 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2224,89 +2224,154 @@ static void i965_update_wm(struct intel_crtc *unused_crtc)
#undef FW_WM
-static void i9xx_update_wm(struct intel_crtc *unused_crtc)
+static const struct intel_watermark_params *i9xx_get_wm_info(struct drm_i915_private *dev_priv,
+ struct intel_crtc *crtc)
{
- struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
- const struct intel_watermark_params *wm_info;
- uint32_t fwater_lo;
- uint32_t fwater_hi;
- int cwm, srwm = 1;
- int fifo_size;
- int planea_wm, planeb_wm;
- struct intel_crtc *crtc, *enabled = NULL;
+ struct intel_plane *plane = to_intel_plane(crtc->base.primary);
if (IS_I945GM(dev_priv))
- wm_info = &i945_wm_info;
+ return &i945_wm_info;
else if (!IS_GEN2(dev_priv))
- wm_info = &i915_wm_info;
+ return &i915_wm_info;
+ else if (plane->plane == PLANE_A)
+ return &i830_a_wm_info;
else
- wm_info = &i830_a_wm_info;
+ return &i830_bc_wm_info;
+}
- fifo_size = dev_priv->display.get_fifo_size(dev_priv, 0);
- crtc = intel_get_crtc_for_plane(dev_priv, 0);
- if (intel_crtc_active(crtc)) {
+static int i9xx_compute_pipe_wm(struct intel_crtc_state *crtc_state)
+{
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ struct intel_atomic_state *state =
+ to_intel_atomic_state(crtc_state->base.state);
+ struct i9xx_wm_state *wm_state = &crtc_state->wm.i9xx.optimal;
+ struct intel_plane *plane = to_intel_plane(crtc->base.primary);
+ const struct drm_plane_state *plane_state = NULL;
+ int fifo_size;
+ const struct intel_watermark_params *wm_info;
+
+ fifo_size = dev_priv->display.get_fifo_size(dev_priv, plane->plane);
+
+ wm_info = i9xx_get_wm_info(dev_priv, crtc);
+
+ wm_state->cxsr = false;
+ memset(&wm_state->sr, 0, sizeof(wm_state->sr));
+
+ if (crtc_state->base.plane_mask & BIT(drm_plane_index(&plane->base)))
+ plane_state = __drm_atomic_get_current_plane_state(&state->base, &plane->base);
+
+ if (!plane_state || !intel_wm_plane_visible(crtc_state, to_intel_plane_state(plane_state))) {
+ wm_state->plane_wm = fifo_size - wm_info->guard_size;
+ if (wm_state->plane_wm > (long)wm_info->max_wm)
+ wm_state->plane_wm = wm_info->max_wm;
+ } else if (HAS_FW_BLC(dev_priv)) {
This part will be called only for (GEN > 2,) So we'll never be
calculating wm for GEN2. but you are changing GEN2 wm also to 2 stages
compute & update in intel_init_pm.
+ struct drm_framebuffer *fb = plane_state->fb;
+ unsigned cpp = IS_GEN2(dev_priv) ? 4 : fb->format->cpp[0];
const struct drm_display_mode *adjusted_mode =
- &crtc->config->base.adjusted_mode;
- const struct drm_framebuffer *fb =
- crtc->base.primary->state->fb;
- int cpp;
+ &crtc_state->base.adjusted_mode;
+ unsigned active_crtcs;
+ bool may_cxsr;
- if (IS_GEN2(dev_priv))
- cpp = 4;
+ if (state->modeset)
+ active_crtcs = state->active_crtcs;
else
- cpp = fb->format->cpp[0];
+ active_crtcs = dev_priv->active_crtcs;
- planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
- wm_info, fifo_size, cpp,
- pessimal_latency_ns);
- enabled = crtc;
- } else {
- planea_wm = fifo_size - wm_info->guard_size;
- if (planea_wm > (long)wm_info->max_wm)
- planea_wm = wm_info->max_wm;
+ may_cxsr = active_crtcs == drm_crtc_mask(&crtc->base);
It would be good to have a comment stating if only one CRTC is enabled
we can go to cxsr.
we can use hweight32() function to check if only one CRTC is enabled
(not mandatory).
+
+ wm_state->plane_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
+ wm_info, fifo_size, cpp,
+ pessimal_latency_ns);
+
+ DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d\n", wm_state->plane_wm);
+
+ if (IS_I915GM(dev_priv) && i915_gem_object_is_tiled(intel_fb_obj(fb)))
+ may_cxsr = false;
+
+ if (may_cxsr) {
+ static const int sr_latency_ns = 6000;
+ unsigned long entries;
+
+ entries = intel_wm_method2(adjusted_mode->crtc_clock,
+ adjusted_mode->crtc_htotal,
+ crtc_state->pipe_src_w, cpp,
+ sr_latency_ns / 100);
+ entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
+
+ DRM_DEBUG_KMS("self-refresh entries: %ld\n", entries);
+
+ if (wm_info->fifo_size >= entries) {
Earlier if (fifo_size < entries) we were assigning srwm = 1 & not
disabling cxsr, Is there any specific Bspec documentation to not enable
cxsr if fifo_size < entries ?
+ wm_state->cxsr = true;
+ wm_state->sr.plane = wm_info->fifo_size - entries;
+ } else
+ may_cxsr = false;
may_cxsr is not used later, so no need to overwrite the value.
+ }
+
+ DRM_DEBUG_KMS("FIFO watermarks - plane watermarks: %d, can cxsr: %s, SR size: %d\n",
+ wm_state->plane_wm, yesno(wm_state->cxsr), wm_state->sr.plane);
}
- if (IS_GEN2(dev_priv))
- wm_info = &i830_bc_wm_info;
+ return 0;
+}
- fifo_size = dev_priv->display.get_fifo_size(dev_priv, 1);
- crtc = intel_get_crtc_for_plane(dev_priv, 1);
- if (intel_crtc_active(crtc)) {
- const struct drm_display_mode *adjusted_mode =
- &crtc->config->base.adjusted_mode;
- const struct drm_framebuffer *fb =
- crtc->base.primary->state->fb;
- int cpp;
+void i9xx_wm_get_hw_state(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_crtc *crtc;
+ uint32_t fwater_lo, planea_wm, planeb_wm;
+
+ fwater_lo = I915_READ(FW_BLC);
+
+ planea_wm = fwater_lo & 0x3f;
+ planeb_wm = (fwater_lo >> 16) & 0x3f;
+
+ for_each_intel_crtc(dev, crtc) {
+ struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
+ struct i9xx_wm_state *wm_state = &cstate->wm.i9xx.optimal;
+ struct intel_plane *plane = to_intel_plane(crtc->base.primary);
+
+ memset(&wm_state->sr, 0, sizeof(wm_state->sr));
+ wm_state->cxsr = false;
- if (IS_GEN2(dev_priv))
- cpp = 4;
+ if (plane == PLANE_A)
here plane is of type intel_plane but you are comparing with enum plane,
I think you meant plane->plane
+ wm_state->plane_wm = planea_wm;
else
- cpp = fb->format->cpp[0];
+ wm_state->plane_wm = planeb_wm;
+
+ crtc->wm.active.i9xx = *wm_state;
+ }
+}
- planeb_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
- wm_info, fifo_size, cpp,
- pessimal_latency_ns);
+static void i9xx_update_wm(struct intel_crtc *crtc)
+{
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ uint32_t fwater_lo;
+ uint32_t fwater_hi;
+ int cwm, srwm = -1;
+ int planea_wm, planeb_wm;
+ struct intel_crtc *enabled = NULL;
enabled is used to check if cxsr can be enabled, IMHO it would be good
to take a bool variable for same & replace use of enabled with
respective intel_crtc variable.
if it seems complex then this approach is also good with me.
+
+ crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
+
+ crtc = intel_get_crtc_for_plane(dev_priv, 0);
+ planea_wm = crtc->wm.active.i9xx.plane_wm;
+ if (intel_crtc_active(crtc))
+ enabled = crtc;
+
+ crtc = intel_get_crtc_for_plane(dev_priv, 1);
+ if (intel_crtc_active(crtc)) {
if (enabled == NULL)
enabled = crtc;
else
enabled = NULL;
- } else {
- planeb_wm = fifo_size - wm_info->guard_size;
- if (planeb_wm > (long)wm_info->max_wm)
- planeb_wm = wm_info->max_wm;
}
+ planeb_wm = crtc->wm.active.i9xx.plane_wm;
DRM_DEBUG_KMS("FIFO watermarks - A: %d, B: %d\n", planea_wm, planeb_wm);
- if (IS_I915GM(dev_priv) && enabled) {
- struct drm_i915_gem_object *obj;
-
- obj = intel_fb_obj(enabled->base.primary->state->fb);
-
- /* self-refresh seems busted with untiled */
- if (!i915_gem_object_is_tiled(obj))
- enabled = NULL;
- }
+ if (!enabled->wm.active.i9xx.cxsr)
+ enabled = NULL;
/*
* Overlay gets an aggressive default since video jitter is bad.
@@ -2317,31 +2382,8 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
intel_set_memory_cxsr(dev_priv, false);
/* Calc sr entries for one plane configs */
- if (HAS_FW_BLC(dev_priv) && enabled) {
- /* self-refresh has much higher latency */
- static const int sr_latency_ns = 6000;
- const struct drm_display_mode *adjusted_mode =
- &enabled->config->base.adjusted_mode;
- const struct drm_framebuffer *fb =
- enabled->base.primary->state->fb;
- int clock = adjusted_mode->crtc_clock;
- int htotal = adjusted_mode->crtc_htotal;
- int hdisplay = enabled->config->pipe_src_w;
- int cpp;
- int entries;
-
- if (IS_I915GM(dev_priv) || IS_I945GM(dev_priv))
- cpp = 4;
- else
- cpp = fb->format->cpp[0];
-
- entries = intel_wm_method2(clock, htotal, hdisplay, cpp,
- sr_latency_ns / 100);
- entries = DIV_ROUND_UP(entries, wm_info->cacheline_size);
- DRM_DEBUG_KMS("self-refresh entries: %d\n", entries);
- srwm = wm_info->fifo_size - entries;
- if (srwm < 0)
- srwm = 1;
+ if (enabled) {
+ srwm = enabled->wm.active.i9xx.sr.plane;
if (IS_I945G(dev_priv) || IS_I945GM(dev_priv))
I915_WRITE(FW_BLC_SELF,
@@ -2367,23 +2409,18 @@ static void i9xx_update_wm(struct intel_crtc *unused_crtc)
intel_set_memory_cxsr(dev_priv, true);
}
-static void i845_update_wm(struct intel_crtc *unused_crtc)
+static void i845_update_wm(struct intel_crtc *crtc)
{
- struct drm_i915_private *dev_priv = to_i915(unused_crtc->base.dev);
- struct intel_crtc *crtc;
- const struct drm_display_mode *adjusted_mode;
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
uint32_t fwater_lo;
int planea_wm;
- crtc = single_enabled_crtc(dev_priv);
- if (crtc == NULL)
earlier we were checking if enabled crtc's != 1 then return, but here
logic got changed.
Oh it seems ok, as platform calling i845 will have only one crtc.
-Mahesh
+ if (!intel_crtc_active(crtc))
return;
- adjusted_mode = &crtc->config->base.adjusted_mode;
- planea_wm = intel_calculate_wm(adjusted_mode->crtc_clock,
- &i845_wm_info,
- dev_priv->display.get_fifo_size(dev_priv, 0),
- 4, pessimal_latency_ns);
+ crtc->wm.active.i9xx = crtc->config->wm.i9xx.optimal;
+ planea_wm = crtc->wm.active.i9xx.plane_wm;
+
fwater_lo = I915_READ(FW_BLC) & ~0xfff;
fwater_lo |= (3<<8) | planea_wm;
@@ -8813,9 +8850,13 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
} else if (IS_GEN4(dev_priv)) {
dev_priv->display.update_wm = i965_update_wm;
} else if (IS_GEN3(dev_priv)) {
+ dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
dev_priv->display.update_wm = i9xx_update_wm;
+
dev_priv->display.get_fifo_size = i9xx_get_fifo_size;
} else if (IS_GEN2(dev_priv)) {
+ dev_priv->display.compute_pipe_wm = i9xx_compute_pipe_wm;
+
if (INTEL_INFO(dev_priv)->num_pipes == 1) {
dev_priv->display.update_wm = i845_update_wm;
dev_priv->display.get_fifo_size = i845_get_fifo_size;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
