| Message ID | 1476278901-15750-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Loving this patch so far! Would it be possible to get this split into two separate patches though? One for removing skl_results and one for programming watermarks as a separate step. On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote: > Instead of running the watermark updates from the callbacks run > them from a separate hook atomic_evade_watermarks. > > 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@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; > > 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; > > - 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); > } > > -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);
Loving this patch so far! Would it be possible to get this split into two separate patches though? One for removing skl_results and one for programming watermarks as a separate step. On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote: > Instead of running the watermark updates from the callbacks run > them from a separate hook atomic_evade_watermarks. > > 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@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; > > 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; > > - 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); > } > > -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);
Accidentally sent original view twice and found one more issue after looking at the rest of them, sorry about that! On Wed, 2016-10-12 at 13:04 -0400, Lyude wrote: > Loving this patch so far! Would it be possible to get this split into > two separate patches though? One for removing skl_results and one for > programming watermarks as a separate step. > > On Wed, 2016-10-12 at 15:28 +0200, Maarten Lankhorst wrote: > > > > Instead of running the watermark updates from the callbacks run > > them from a separate hook atomic_evade_watermarks. > > > > 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@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(cr > > tc > > ); > > + } > > } > > } > > > > @@ -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; > > > > 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_st > > > ate > > ->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; > > > > - I915_WRITE(PIPE_WM_LINETIME(pipe), > > - pipe_wm->linetime); > > + I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime); Little bit of rebase debris? Might as well just fix this in patch 5. With the changes I mentioned: Reviewed-by: Lyude <cpaul@redhat.com> > > + > > + 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); > > } > > > > -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);
Op 12-10-16 om 19:15 schreef Lyude: > Accidentally sent original view twice and found one more issue after > looking at the rest of them, sorry about that! > > On Wed, 2016-10-12 at 13:04 -0400, Lyude wrote: >> Loving this patch so far! Would it be possible to get this split into >> two separate patches though? One for removing skl_results and one for >> programming watermarks as a separate step. Yeah the small hunk has to be moved to patch #5. I can't split this patch up. skl_results becomes useless after moving the programming to a separate step. The old way of doing it was storing intel_state->wm_results in dev_priv->wm.skl_results and using it in the plane callbacks. With the watermark update in its own function, this becomes useless because the pointer to watermarks can be retrieved from state directly. ~Maarten
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. > > 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@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(-) > ... > @@ -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); > + } > } This will change the behavior on ILK-style platforms won't it? Previously the intel_update_watermarks here was a noop on those platforms, but now we're calling initial_watermarks after the CRTC is disabled there (note that there's also a call to it in pre_plane_update that we purposely skip when doing any kind of modeset). Matt > } > > @@ -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; > > 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; > > - 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); > } > > -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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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. 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 20-10-16 om 20:35 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. >> >> 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@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(-) >> > ... >> @@ -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); >> + } >> } > This will change the behavior on ILK-style platforms won't it? > Previously the intel_update_watermarks here was a noop on those > platforms, but now we're calling initial_watermarks after the CRTC is > disabled there (note that there's also a call to it in pre_plane_update > that we purposely skip when doing any kind of modeset). Yeah, it could be better to change it to if (HAS_DDI(dev_priv)), that way it's not going to matter..
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@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
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; 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; - 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); } -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);
Instead of running the watermark updates from the callbacks run them from a separate hook atomic_evade_watermarks. 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@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(-)