[Intel-gfx] [RFC 6/8] drm/i915: Calculate ILK-style watermarks during atomic check (v2)
Daniel Vetter
daniel at ffwll.ch
Mon Jul 6 02:13:09 PDT 2015
On Wed, Jul 01, 2015 at 07:25:59PM -0700, Matt Roper wrote:
> Calculate pipe watermarks during atomic calculation phase, based on the
> contents of the atomic transaction's state structure. We still program
> the watermarks at the same time we did before, but the computation now
> happens much earlier.
>
> While this patch isn't too exciting by itself, it paves the way for
> future patches. The eventual goal (which will be realized in future
> patches in this series) is to calculate multiple sets up watermark
> values up front, and then program them at different times (pre- vs
> post-vblank) on the platforms that need a two-step watermark update.
>
> While we're at it, s/intel_compute_pipe_wm/ilk_compute_pipe_wm/ since
> this function only applies to ILK-style watermarks and we have a
> completely different function for SKL-style watermarks.
>
> Note that the original code had a memcmp() in ilk_update_wm() to avoid
> calling ilk_program_watermarks() if the watermarks hadn't changed. This
> memcmp vanishes here, which means we may do some unnecessary result
> generation and merging in cases where watermarks didn't change, but the
> lower-level function ilk_write_wm_values already makes sure that we
> don't actually try to program the watermark registers again.
>
> v2: Squash a few commits from the original series together; no longer
> leave pre-calculated wm's in a separate temporary structure since
> it's easier to follow the logic if we just cut over to using the
> pre-calculated values directly.
>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/intel_display.c | 6 +++
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_pm.c | 87 ++++++++++++++++++------------------
> 4 files changed, 53 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6aa8083..2774976 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -621,6 +621,8 @@ struct drm_i915_display_funcs {
> int target, int refclk,
> struct dpll *match_clock,
> struct dpll *best_clock);
> + int (*compute_pipe_wm)(struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
All the new callbacks you add here are per-crtc, but we might want to
rebalance the fifo space on some platforms where that's not perfectly
per-crtc. Otoh that can wait until someone actually writes that code,
maybe we'll decide to only rebalance wm when we need to do a modeset
anyway. I think we can stick with this for now.
-Daniel
> void (*update_wm)(struct drm_crtc *crtc);
> void (*update_sprite_wm)(struct drm_plane *plane,
> struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 36ae3f7..46b62cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11857,6 +11857,12 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> return ret;
> }
>
> + if (dev_priv->display.compute_pipe_wm) {
> + ret = dev_priv->display.compute_pipe_wm(crtc, state);
> + if (ret)
> + return ret;
> + }
> +
> return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c23cf7d..335b24b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1445,6 +1445,8 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
> int intel_atomic_setup_scalers(struct drm_device *dev,
> struct intel_crtc *intel_crtc,
> struct intel_crtc_state *crtc_state);
> +int intel_check_crtc(struct drm_crtc *crtc,
> + struct drm_crtc_state *state);
>
> /* intel_atomic_plane.c */
> struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0e28806..c6e735f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2039,9 +2039,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> const struct intel_crtc *intel_crtc,
> int level,
> struct intel_crtc_state *cstate,
> + struct intel_plane_state *pristate,
> + struct intel_plane_state *sprstate,
> + struct intel_plane_state *curstate,
> struct intel_wm_level *result)
> {
> - struct intel_plane *intel_plane;
> uint16_t pri_latency = dev_priv->wm.pri_latency[level];
> uint16_t spr_latency = dev_priv->wm.spr_latency[level];
> uint16_t cur_latency = dev_priv->wm.cur_latency[level];
> @@ -2053,29 +2055,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
> cur_latency *= 5;
> }
>
> - for_each_intel_plane_on_crtc(dev_priv->dev, intel_crtc, intel_plane) {
> - struct intel_plane_state *pstate =
> - to_intel_plane_state(intel_plane->base.state);
> -
> - switch (intel_plane->base.type) {
> - case DRM_PLANE_TYPE_PRIMARY:
> - result->pri_val = ilk_compute_pri_wm(cstate, pstate,
> - pri_latency,
> - level);
> - result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
> - result->pri_val);
> - break;
> - case DRM_PLANE_TYPE_OVERLAY:
> - result->spr_val = ilk_compute_spr_wm(cstate, pstate,
> - spr_latency);
> - break;
> - case DRM_PLANE_TYPE_CURSOR:
> - result->cur_val = ilk_compute_cur_wm(cstate, pstate,
> - cur_latency);
> - break;
> - }
> - }
> -
> + result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> + pri_latency, level);
> + result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> + result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> + result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
> result->enable = true;
> }
>
> @@ -2355,15 +2339,20 @@ static void ilk_compute_wm_config(struct drm_device *dev,
> }
>
> /* Compute new watermarks for the pipe */
> -static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> - struct intel_pipe_wm *pipe_wm)
> +static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> {
> - struct drm_crtc *crtc = cstate->base.crtc;
> + struct intel_pipe_wm *pipe_wm;
> struct drm_device *dev = crtc->dev;
> const struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct drm_crtc_state *cs;
> + struct intel_crtc_state *cstate = NULL;
> struct intel_plane *intel_plane;
> + struct drm_plane_state *ps;
> + struct intel_plane_state *pristate = NULL;
> struct intel_plane_state *sprstate = NULL;
> + struct intel_plane_state *curstate = NULL;
> int level, max_level = ilk_wm_max_level(dev);
> /* LP0 watermark maximums depend on this pipe alone */
> struct intel_wm_config config = {
> @@ -2371,11 +2360,26 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> };
> struct ilk_wm_maximums max;
>
> + cs = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(cs))
> + return PTR_ERR(cs);
> + else
> + cstate = to_intel_crtc_state(cs);
> +
> + pipe_wm = &cstate->wm.active.ilk;
> +
> for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> - if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
> - sprstate = to_intel_plane_state(intel_plane->base.state);
> - break;
> - }
> + ps = drm_atomic_get_plane_state(state,
> + &intel_plane->base);
> + if (IS_ERR(ps))
> + return PTR_ERR(ps);
> +
> + if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> + pristate = to_intel_plane_state(ps);
> + else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> + sprstate = to_intel_plane_state(ps);
> + else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + curstate = to_intel_plane_state(ps);
> }
>
> config.sprites_enabled = sprstate->visible;
> @@ -2384,7 +2388,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
>
> pipe_wm->pipe_enabled = cstate->base.active;
> - pipe_wm->sprites_enabled = sprstate->visible;
> + pipe_wm->sprites_enabled = config.sprites_enabled;
> pipe_wm->sprites_scaled = config.sprites_scaled;
>
> /* ILK/SNB: LP2+ watermarks only w/o sprites */
> @@ -2395,7 +2399,8 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> if (config.sprites_scaled)
> max_level = 0;
>
> - ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
> + ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> + pristate, sprstate, curstate, &pipe_wm->wm[0]);
>
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> @@ -2405,14 +2410,15 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>
> /* At least LP0 must be valid */
> if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> - return false;
> + return -EINVAL;
>
> ilk_compute_wm_reg_maximums(dev, 1, &max);
>
> for (level = 1; level <= max_level; level++) {
> struct intel_wm_level wm = {};
>
> - ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
> + ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> + pristate, sprstate, curstate, &wm);
>
> /*
> * Disable any watermark level that exceeds the
> @@ -2425,7 +2431,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> pipe_wm->wm[level] = wm;
> }
>
> - return true;
> + return 0;
> }
>
> /*
> @@ -3752,7 +3758,6 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - struct intel_pipe_wm pipe_wm = {};
>
> /*
> * IVB workaround: must disable low power watermarks for at least
> @@ -3766,13 +3771,6 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> }
>
> - intel_compute_pipe_wm(cstate, &pipe_wm);
> -
> - if (!memcmp(&cstate->wm.active.ilk, &pipe_wm, sizeof(pipe_wm)))
> - return;
> -
> - cstate->wm.active.ilk = pipe_wm;
> -
> ilk_program_watermarks(dev_priv);
> }
>
> @@ -7049,6 +7047,7 @@ void intel_init_pm(struct drm_device *dev)
> (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
> dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> dev_priv->display.update_wm = ilk_update_wm;
> + dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> } else {
> DRM_DEBUG_KMS("Failed to read display plane latency. "
> "Disable CxSR\n");
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list