Re: [Intel-gfx] [PATCH 1/6] drm/i915/skl: Move per-pipe ddb allocations into crtc states

From: Paulo Zanoni
Date: Wed Oct 05 2016 - 16:23:46 EST

Next message: Paulo Zanoni: "Re: [Intel-gfx] [PATCH 2/6] drm/i915/skl: Remove linetime from skl_wm_values"
Previous message: Dave Chinner: "Re: [PATCH] fs/block_dev.c: return the right error in thaw_bdev()"
In reply to: Lyude: "[PATCH 1/6] drm/i915/skl: Move per-pipe ddb allocations into crtc states"
Next in thread: Maarten Lankhorst: "Re: [PATCH 0/6] Start of skl watermark cleanup"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Qua, 2016-10-05 Ãs 11:33 -0400, Lyude escreveu:
> First part of cleaning up all of the skl watermark code. This moves
> the
> structures for storing the ddb allocations of each pipe into
> intel_crtc_state, along with moving the structures for storing the
> current ddb allocations active on hardware into intel_crtc.
>
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville SyrjÃlÃ <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> Âdrivers/gpu/drm/i915/i915_drv.hÂÂÂÂÂÂ|ÂÂ1 -
> Âdrivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------
> Âdrivers/gpu/drm/i915/intel_drv.hÂÂÂÂÂ|ÂÂ8 +++++---
> Âdrivers/gpu/drm/i915/intel_pm.cÂÂÂÂÂÂ| 40 +++++++++++++++-----------
> ----------
> Â4 files changed, 30 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f8c66ee..85e541c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1636,7 +1636,6 @@ static inline bool skl_ddb_entry_equal(const
> struct skl_ddb_entry *e1,
> Â}
> Â
> Âstruct skl_ddb_allocation {
> - struct skl_ddb_entry pipe[I915_MAX_PIPES];
> Â struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
> /* packed/uv */
> Â struct skl_ddb_entry
> y_plane[I915_MAX_PIPES][I915_MAX_PLANES];
> Â};
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a366656..17733af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14235,12 +14235,11 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
> Â ÂÂÂÂÂunsigned int *crtc_vblank_mask)
> Â{
> Â struct drm_device *dev = state->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> Â struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> Â struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> Â struct drm_crtc_state *old_crtc_state;
> - struct skl_ddb_allocation *new_ddb = &intel_state-
> >wm_results.ddb;
> - struct skl_ddb_allocation *cur_ddb = &dev_priv-
> >wm.skl_hw.ddb;
> + struct intel_crtc_state *cstate;
> Â unsigned int updated = 0;
> Â bool progress;
> Â enum pipe pipe;
> @@ -14258,12 +14257,14 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
> Â for_each_crtc_in_state(state, crtc, old_crtc_state,
> i) {
> Â bool vbl_wait = false;
> Â unsigned int cmask = drm_crtc_mask(crtc);
> - pipe = to_intel_crtc(crtc)->pipe;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cstate = to_intel_crtc_state(crtc->state);
> + pipe = intel_crtc->pipe;
> Â
> Â if (updated & cmask || !crtc->state->active)
> Â continue;
> - if (skl_ddb_allocation_overlaps(state,
> cur_ddb, new_ddb,
> - pipe))
> + if (skl_ddb_allocation_overlaps(state,
> intel_crtc))
> Â continue;
> Â
> Â updated |= cmask;
> @@ -14274,7 +14275,8 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
> Â Â* then we need to wait for a vblank to pass
> for the
> Â Â* new ddb allocation to take effect.
> Â Â*/
> - if (!skl_ddb_allocation_equals(cur_ddb,
> new_ddb, pipe) &&
> + if (!skl_ddb_entry_equal(&cstate-
> >wm.skl.ddb,
> + Â&intel_crtc-
> >hw_ddb) &&
> Â ÂÂÂÂ!crtc->state->active_changed &&
> Â ÂÂÂÂintel_state->wm_results.dirty_pipes !=
> updated)
> Â vbl_wait = true;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index f48e79a..35ba282 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -496,6 +496,7 @@ struct intel_crtc_wm_state {
> Â struct {
> Â /* gen9+ only needs 1-step wm programming */
> Â struct skl_pipe_wm optimal;
> + struct skl_ddb_entry ddb;
> Â
> Â /* cached plane data rate */
> Â unsigned plane_data_rate[I915_MAX_PLANES];
> @@ -733,6 +734,9 @@ struct intel_crtc {
> Â bool cxsr_allowed;
> Â } wm;
> Â
> + /* gen9+: ddb allocation currently being used */
> + struct skl_ddb_entry hw_ddb;
> +
> Â int scanline_offset;
> Â
> Â struct {
> @@ -1755,9 +1759,7 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
> Â ÂÂÂÂÂÂÂconst struct skl_ddb_allocation *new,
> Â ÂÂÂÂÂÂÂenum pipe pipe);
> Âbool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> - Âconst struct skl_ddb_allocation
> *old,
> - Âconst struct skl_ddb_allocation
> *new,
> - Âenum pipe pipe);
> + Âstruct intel_crtc *intel_crtc);
> Âvoid skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> Â Âconst struct skl_wm_values *wm);
> Âvoid skl_write_plane_wm(struct intel_crtc *intel_crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 7f1748a..0383516 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3058,7 +3058,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
> Â struct drm_crtc *for_crtc = cstate->base.crtc;
> Â unsigned int pipe_size, ddb_size;
> Â int nth_active_pipe;
> - int pipe = to_intel_crtc(for_crtc)->pipe;
> Â
> Â if (WARN_ON(!state) || !cstate->base.active) {
> Â alloc->start = 0;
> @@ -3086,7 +3085,7 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
> Â Â* we currently hold.
> Â Â*/
> Â if (!intel_state->active_pipe_changes) {
> - *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> + *alloc = to_intel_crtc(for_crtc)->hw_ddb;
> Â return;
> Â }
> Â
> @@ -3354,7 +3353,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
> Â struct drm_plane *plane;
> Â struct drm_plane_state *pstate;
> Â enum pipe pipe = intel_crtc->pipe;
> - struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
> + struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> Â uint16_t alloc_size, start, cursor_blocks;
> Â uint16_t *minimum = cstate->wm.skl.minimum_blocks;
> Â uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> @@ -3366,7 +3365,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
> Â return 0;
> Â
> Â if (!cstate->base.active) {
> - ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> + memset(alloc, 0, sizeof(*alloc));

Nitpick: I would have kept the double assignment here instead of
calling memset:
alloc->start = alloc->end = 0;

Anyway, this chunk will have a small conflict with my current multi-
pipe fix, and I'd like to see the multi-pipe fix get merged before this
since it will make it much easier to backport it to stable if we don't
have yet another round of code churn separating stable from dinq.

Anyway, this patch looks correct and and it seems the multi-pipe fix
won't require any change besides the small rebase above. I'm still
going to test the series later, but I suppose a rebased version of this
patch will be ready to get a reviewed-by tag from me.

> Â memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
> Â memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
> Â return 0;
> @@ -3895,14 +3894,6 @@ void skl_write_cursor_wm(struct intel_crtc
> *intel_crtc,
> Â ÂÂÂÂ&wm->ddb.plane[pipe][PLANE_CURSOR]);
> Â}
> Â
> -bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> - ÂÂÂÂÂÂÂconst struct skl_ddb_allocation *new,
> - ÂÂÂÂÂÂÂenum pipe pipe)
> -{
> - return new->pipe[pipe].start == old->pipe[pipe].start &&
> - ÂÂÂÂÂÂÂnew->pipe[pipe].end == old->pipe[pipe].end;
> -}
> -
> Âstatic inline bool skl_ddb_entries_overlap(const struct
> skl_ddb_entry *a,
> Â ÂÂÂconst struct
> skl_ddb_entry *b)
> Â{
> @@ -3910,22 +3901,22 @@ static inline bool
> skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
> Â}
> Â
> Âbool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> - Âconst struct skl_ddb_allocation
> *old,
> - Âconst struct skl_ddb_allocation
> *new,
> - Âenum pipe pipe)
> + Âstruct intel_crtc *intel_crtc)
> Â{
> - struct drm_device *dev = state->dev;
> - struct intel_crtc *intel_crtc;
> - enum pipe otherp;
> + struct drm_crtc *other_crtc;
> + struct drm_crtc_state *other_cstate;
> + struct intel_crtc *other_intel_crtc;
> + const struct skl_ddb_entry *ddb =
> + &to_intel_crtc_state(intel_crtc->base.state)-
> >wm.skl.ddb;
> + int i;
> Â
> - for_each_intel_crtc(dev, intel_crtc) {
> - otherp = intel_crtc->pipe;
> + for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
> + other_intel_crtc = to_intel_crtc(other_crtc);
> Â
> - if (otherp == pipe)
> + if (other_intel_crtc == intel_crtc)
> Â continue;
> Â
> - if (skl_ddb_entries_overlap(&new->pipe[pipe],
> - ÂÂÂÂ&old->pipe[otherp]))
> + if (skl_ddb_entries_overlap(ddb, &other_intel_crtc-
> >hw_ddb))
> Â return true;
> Â }
> Â
> @@ -4084,7 +4075,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
> Â memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
> Â ÂÂÂÂÂÂÂsizeof(dst->plane_trans[pipe]));
> Â
> - dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
> Â memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
> Â ÂÂÂÂÂÂÂsizeof(dst->ddb.y_plane[pipe]));
> Â memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
> @@ -4192,6 +4182,8 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
> Â
> Â skl_copy_wm_for_pipe(hw_vals, results, pipe);
> Â
> + intel_crtc->hw_ddb = cstate->wm.skl.ddb;
> +
> Â mutex_unlock(&dev_priv->wm.wm_mutex);
> Â}
> Â

Next message: Paulo Zanoni: "Re: [Intel-gfx] [PATCH 2/6] drm/i915/skl: Remove linetime from skl_wm_values"
Previous message: Dave Chinner: "Re: [PATCH] fs/block_dev.c: return the right error in thaw_bdev()"
In reply to: Lyude: "[PATCH 1/6] drm/i915/skl: Move per-pipe ddb allocations into crtc states"
Next in thread: Maarten Lankhorst: "Re: [PATCH 0/6] Start of skl watermark cleanup"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]