[Intel-gfx] [PATCH 7/8] drm/i915/gen9+: Program watermarks as a separate step during evasion
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Nov 1 08:38:46 UTC 2016
Op 20-10-16 om 23:57 schreef Matt Roper:
> 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 at linux.intel.com>
>> ---
>> 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.
Should be harmless. Could prevent it if we cared, but I don't see the value. :)
It would be nice if we would start removing planes that were only included because it's wm values change.
Not completely sure how doable it is, depends whether we could trigger updates race free.
~Maarten
More information about the Intel-gfx
mailing list