On Wed, Oct 12, 2016 at 03:28:20PM +0200, Maarten Lankhorst wrote:
> Instead of running the watermark updates from the callbacks run
> them from a separate hook atomic_evade_watermarks.
The commit message here is a bit terse. I'd clarify that the change
we're making is that watermark register programming is no longer
happening in the same display callbacks that write general plane
registers, but rather in a new independent hook. The key thing to
emphasize is that despite the refactoring, the watermark values will
still be written under the same vblank evasion that is covering the rest
of the planes' updates, so they'll still take effect on the same vblank.
>
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 -------
> drivers/gpu/drm/i915/intel_display.c | 36 +++++++++-------------------------
> drivers/gpu/drm/i915/intel_drv.h | 7 -------
> drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_sprite.c | 18 -----------------
> 5 files changed, 28 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 09588c58148f..28e44cb611b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2027,13 +2027,6 @@ struct drm_i915_private {
> */
> uint16_t skl_latency[8];
>
> - /*
> - * The skl_wm_values structure is a bit too big for stack
> - * allocation, so we keep the staging struct where we store
> - * intermediate results here instead.
> - */
> - struct skl_wm_values skl_results;
> -
> /* current hardware state */
> union {
> struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 340861826c46..d3d7d9dc14a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3377,9 +3377,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> struct drm_framebuffer *fb = plane_state->base.fb;
> - const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> - const struct skl_plane_wm *p_wm =
> - &crtc_state->wm.skl.optimal.planes[0];
> int pipe = intel_crtc->pipe;
> u32 plane_ctl;
> unsigned int rotation = plane_state->base.rotation;
> @@ -3415,9 +3412,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> intel_crtc->adjusted_x = src_x;
> intel_crtc->adjusted_y = src_y;
>
> - if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> - skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> -
> I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3450,18 +3444,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
> int pipe = intel_crtc->pipe;
>
> - /*
> - * We only populate skl_results on watermark updates, and if the
> - * plane's visiblity isn't actually changing neither is its watermarks.
> - */
> - if (!crtc->primary->state->visible)
> - skl_write_plane_wm(intel_crtc, p_wm,
> - &dev_priv->wm.skl_results.ddb, 0);
> -
> I915_WRITE(PLANE_CTL(pipe, 0), 0);
> I915_WRITE(PLANE_SURF(pipe, 0), 0);
> POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10824,16 +10808,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> - const struct skl_plane_wm *p_wm =
> - &cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> int pipe = intel_crtc->pipe;
> uint32_t cntl = 0;
>
> - if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
> - skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> -
> if (plane_state && plane_state->base.visible) {
> cntl = MCURSOR_GAMMA_ENABLE;
> switch (plane_state->base.crtc_w) {
> @@ -14436,8 +14413,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> intel_check_cpu_fifo_underruns(dev_priv);
> intel_check_pch_fifo_underruns(dev_priv);
>
> - if (!crtc->state->active)
> - intel_update_watermarks(crtc);
> + if (!crtc->state->active) {
> + if (dev_priv->display.initial_watermarks)
> + dev_priv->display.initial_watermarks(intel_state,
> + to_intel_crtc_state(crtc->state));
> + else
> + intel_update_watermarks(crtc);
> + }
> }
> }
>
> @@ -14599,7 +14581,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>
> drm_atomic_helper_swap_state(state, true);
> dev_priv->wm.distrust_bios_wm = false;
> - dev_priv->wm.skl_results = intel_state->wm_results;
> intel_shared_dpll_commit(state);
> intel_atomic_track_fbs(state);
>
> @@ -14913,7 +14894,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> intel_pipe_update_start(intel_crtc);
>
> if (modeset)
> - return;
> + goto out;
Should this change have been in a previous patch? Were we missing the
programming of linetime watermarks on modeset before?
>
> if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
> intel_color_set_csc(crtc->state);
> @@ -14925,6 +14906,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> else if (INTEL_GEN(dev_priv) >= 9)
> skl_detach_scalers(intel_crtc);
>
> +out:
> if (dev_priv->display.atomic_evade_watermarks)
> dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
> }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9f04e26c4365..17cf1ee83bfb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1761,13 +1761,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> enum pipe pipe);
> bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> - const struct skl_plane_wm *wm,
> - const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> - const struct skl_plane_wm *wm,
> - const struct skl_ddb_allocation *ddb,
> - int plane);
> uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> bool ilk_disable_lp_wm(struct drm_device *dev);
> int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index be3dd8cdc7ae..18c62d1eea19 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4179,27 +4179,35 @@ skl_compute_wm(struct drm_atomic_state *state)
> return 0;
> }
>
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> - struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
> {
> struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> + const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
> enum pipe pipe = crtc->pipe;
> + int plane;
> +
> + if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
> + return;
Should we have had this test even when we were just writing the linetime
watermarks? I.e., should this move to an earlier patch?
>
> - I915_WRITE(PIPE_WM_LINETIME(pipe),
> - pipe_wm->linetime);
> + I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> + for (plane = 0; plane < intel_num_planes(crtc); plane++)
> + skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
> +
> + skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
Technically this will cause us to (re)write unchanged watermark values
more often than we did previously. E.g., we previously skipped writing
values for disabled planes that were staying disabled. Not sure if it
really matters or not, just something I figured I should point out.
Matt
> }
>
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> + struct intel_crtc_state *cstate)
> {
> + struct drm_crtc *crtc = cstate->base.crtc;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct skl_wm_values *results = &dev_priv->wm.skl_results;
> + struct skl_wm_values *results = &state->wm_results;
> struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> enum pipe pipe = intel_crtc->pipe;
>
> if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4213,16 +4221,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
> * the pipe's shut off, just do so here. Already active pipes will have
> * their watermarks updated once we update their planes.
> */
> - if (crtc->state->active_changed) {
> - int plane;
> -
> - for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> - skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
> - &results->ddb, plane);
> -
> - skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
> - &results->ddb);
> - }
> + if (cstate->base.active_changed)
> + skl_evade_crtc_wm(state, cstate);
>
> skl_copy_wm_for_pipe(hw_vals, results, pipe);
>
> @@ -7727,7 +7727,7 @@ void intel_init_pm(struct drm_device *dev)
> /* For FIFO watermark updates */
> if (INTEL_INFO(dev)->gen >= 9) {
> skl_setup_wm_latency(dev);
> - dev_priv->display.update_wm = skl_update_wm;
> + dev_priv->display.initial_watermarks = skl_initial_wm;
> dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
> dev_priv->display.compute_global_watermarks = skl_compute_wm;
> } else if (HAS_PCH_SPLIT(dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0fb775b4c93e..366900dcde34 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> struct drm_framebuffer *fb = plane_state->base.fb;
> - const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> - struct drm_crtc *crtc = crtc_state->base.crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> const int pipe = intel_plane->pipe;
> const int plane = intel_plane->plane + 1;
> - const struct skl_plane_wm *p_wm =
> - &crtc_state->wm.skl.optimal.planes[plane];
> u32 plane_ctl;
> const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
> - if (wm->dirty_pipes & drm_crtc_mask(crtc))
> - skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
> -
> if (key->flags) {
> I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
> I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> struct drm_device *dev = dplane->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *intel_plane = to_intel_plane(dplane);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> const int pipe = intel_plane->pipe;
> const int plane = intel_plane->plane + 1;
>
> - /*
> - * We only populate skl_results on watermark updates, and if the
> - * plane's visiblity isn't actually changing neither is its watermarks.
> - */
> - if (!dplane->state->visible)
> - skl_write_plane_wm(to_intel_crtc(crtc),
> - &cstate->wm.skl.optimal.planes[plane],
> - &dev_priv->wm.skl_results.ddb, plane);
> -
> I915_WRITE(PLANE_CTL(pipe, plane), 0);
>
> I915_WRITE(PLANE_SURF(pipe, plane), 0);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
