While it (mostly) works, the code for handling watermarks on Skylake has been kind of ugly for a while. As well a lot of it isn't that friendly to atomic transactions, Lots of copy paste, redundant wm values, etc. While this isn't a full cleanup, it's a good start. As well, we add a couple of features for making debugging watermarks a little easier.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
Lyude (6): drm/i915/skl: Move per-pipe ddb allocations into crtc states drm/i915/skl: Remove linetime from skl_wm_values drm/i915: Add enable_sagv option drm/i915/gen9: Make skl_wm_level per-plane drm/i915/gen9: Get rid of redundant watermark values drm/i915/gen9: Add ddb changes to atomic debug output
drivers/gpu/drm/i915/i915_drv.h | 10 +- drivers/gpu/drm/i915/i915_params.c | 5 + drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_display.c | 52 +++-- drivers/gpu/drm/i915/intel_drv.h | 20 +- drivers/gpu/drm/i915/intel_pm.c | 437 ++++++++++++++++------------------- drivers/gpu/drm/i915/intel_sprite.c | 8 +- 7 files changed, 260 insertions(+), 273 deletions(-)
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@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- 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)); 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); }
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@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
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 part of cleaning up the watermark code for skl. This is easy, since it seems that we never actually needed to keep track of the linetime in the skl_wm_values struct anyway.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_display.c | 6 ++++-- drivers/gpu/drm/i915/intel_pm.c | 7 +------ 3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 85e541c..d26e5999 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1643,7 +1643,6 @@ struct skl_ddb_allocation { struct skl_wm_values { unsigned dirty_pipes; struct skl_ddb_allocation ddb; - uint32_t wm_linetime[I915_MAX_PIPES]; uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; uint32_t plane_trans[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 17733af..a71d05a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14832,6 +14832,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, 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 *intel_cstate = + to_intel_crtc_state(crtc->state); struct intel_crtc_state *old_intel_state = to_intel_crtc_state(old_crtc_state); bool modeset = needs_modeset(crtc->state); @@ -14848,13 +14850,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_color_load_luts(crtc->state); }
- if (to_intel_crtc_state(crtc->state)->update_pipe) + if (intel_cstate->update_pipe) intel_update_pipe_config(intel_crtc, old_intel_state); else if (INTEL_GEN(dev_priv) >= 9) { skl_detach_scalers(intel_crtc);
I915_WRITE(PIPE_WM_LINETIME(pipe), - dev_priv->wm.skl_hw.wm_linetime[pipe]); + intel_cstate->wm.skl.optimal.linetime); } }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0383516..af96888 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3839,8 +3839,6 @@ static void skl_compute_wm_results(struct drm_device *dev, temp |= PLANE_WM_EN;
r->plane_trans[pipe][PLANE_CURSOR] = temp; - - r->wm_linetime[pipe] = p_wm->linetime; }
static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, @@ -4069,7 +4067,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, struct skl_wm_values *src, enum pipe pipe) { - dst->wm_linetime[pipe] = src->wm_linetime[pipe]; memcpy(dst->plane[pipe], src->plane[pipe], sizeof(dst->plane[pipe])); memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], @@ -4320,8 +4317,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
max_level = ilk_wm_max_level(dev);
- hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe)); - for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) hw->plane[pipe][i][level] = @@ -4338,7 +4333,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
hw->dirty_pipes |= drm_crtc_mask(crtc);
- active->linetime = hw->wm_linetime[pipe]; + active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) {
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
Next part of cleaning up the watermark code for skl. This is easy, since it seems that we never actually needed to keep track of the linetime in the skl_wm_values struct anyway.
Reviewed-by: Paulo Zanoni paulo.r.zanoni@intel.com
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_display.c | 6 ++++-- drivers/gpu/drm/i915/intel_pm.c | 7 +------ 3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 85e541c..d26e5999 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1643,7 +1643,6 @@ struct skl_ddb_allocation { struct skl_wm_values { unsigned dirty_pipes; struct skl_ddb_allocation ddb;
- uint32_t wm_linetime[I915_MAX_PIPES];
uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; uint32_t plane_trans[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 17733af..a71d05a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14832,6 +14832,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, 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 *intel_cstate =
to_intel_crtc_state(crtc->state);struct intel_crtc_state *old_intel_state = to_intel_crtc_state(old_crtc_state); bool modeset = needs_modeset(crtc->state); @@ -14848,13 +14850,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_color_load_luts(crtc->state); }
- if (to_intel_crtc_state(crtc->state)->update_pipe)
- if (intel_cstate->update_pipe)
intel_update_pipe_config(intel_crtc, old_intel_state); else if (INTEL_GEN(dev_priv) >= 9) { skl_detach_scalers(intel_crtc); I915_WRITE(PIPE_WM_LINETIME(pipe),
dev_priv->wm.skl_hw.wm_linetime[pipe]);
intel_cstate->wm.skl.optimal.linetime);} } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0383516..af96888 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3839,8 +3839,6 @@ static void skl_compute_wm_results(struct drm_device *dev, temp |= PLANE_WM_EN; r->plane_trans[pipe][PLANE_CURSOR] = temp;
- r->wm_linetime[pipe] = p_wm->linetime;
} static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, @@ -4069,7 +4067,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, struct skl_wm_values *src, enum pipe pipe) {
- dst->wm_linetime[pipe] = src->wm_linetime[pipe];
memcpy(dst->plane[pipe], src->plane[pipe], sizeof(dst->plane[pipe])); memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], @@ -4320,8 +4317,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) max_level = ilk_wm_max_level(dev);
- hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) hw->plane[pipe][i][level] = @@ -4338,7 +4333,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) hw->dirty_pipes |= drm_crtc_mask(crtc);
- active->linetime = hw->wm_linetime[pipe];
- active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) {
This option allows us to manually control the SAGV at module load time. This can be useful in situations such as trying to debug watermark changes, since enabled SAGV + incorrect watermarks = total GPU annihilation.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/i915_params.c | 5 +++++ drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++--- 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 768ad89..f462cd4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = false, .enable_gvt = false, + .enable_sagv = -1, };
module_param_named(modeset, i915.modeset, int, 0400); @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); + +module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400); +MODULE_PARM_DESC(enable_sagv, + "Enable the SAGV (gen9+ only)(1=enabled, 0=disabled, -1=driver discretion [default])"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 3a0dd78..a7db125 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -65,6 +65,7 @@ struct i915_params { bool enable_dp_mst; bool enable_dpcd_backlight; bool enable_gvt; + int enable_sagv; };
extern struct i915_params i915 __read_mostly; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a71d05a..dd15ae2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct drm_device *dev) pll->on = false; }
- if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) + if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { vlv_wm_get_hw_state(dev); - else if (IS_GEN9(dev)) + } else if (IS_GEN9(dev)) { skl_wm_get_hw_state(dev); - else if (HAS_PCH_SPLIT(dev)) + + if (i915.enable_sagv != -1) { + if (i915.enable_sagv) + intel_enable_sagv(dev_priv); + else + intel_disable_sagv(dev_priv); + + dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED; + } + } else if (HAS_PCH_SPLIT(dev)) { ilk_wm_get_hw_state(dev); + }
for_each_intel_crtc(dev, crtc) { unsigned long put_domains;
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
This option allows us to manually control the SAGV at module load time. This can be useful in situations such as trying to debug watermark changes, since enabled SAGV + incorrect watermarks = total GPU annihilation.
I'm not a huge fan of adding options that are only for very limited debugging situations, especially simple ones that can always just be re-implemented during debugging sessions such as this one. Anyway, I'm not opposed to adding the option since it's marked as unsafe anyway, I'm just stating my general opinion. See more below.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_params.c | 5 +++++ drivers/gpu/drm/i915/i915_params.h | 1 + drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++--- 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 768ad89..f462cd4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = false, .enable_gvt = false,
- .enable_sagv = -1,
}; module_param_named(modeset, i915.modeset, int, 0400); @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400); +MODULE_PARM_DESC(enable_sagv,
- "Enable the SAGV (gen9+ only)(1=enabled, 0=disabled,
-1=driver discretion [default])"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 3a0dd78..a7db125 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -65,6 +65,7 @@ struct i915_params { bool enable_dp_mst; bool enable_dpcd_backlight; bool enable_gvt;
- int enable_sagv;
}; extern struct i915_params i915 __read_mostly; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a71d05a..dd15ae2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct drm_device *dev) pll->on = false; }
- if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
- if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
vlv_wm_get_hw_state(dev);
- else if (IS_GEN9(dev))
- } else if (IS_GEN9(dev)) {
skl_wm_get_hw_state(dev);
- else if (HAS_PCH_SPLIT(dev))
if (i915.enable_sagv != -1) {if (i915.enable_sagv)intel_enable_sagv(dev_priv);elseintel_disable_sagv(dev_priv);dev_priv->sagv_status =I915_SAGV_NOT_CONTROLLED;
Adding this code to the middle of a get_hw_state() if-ladder doesn't seem to be the best approach. My suggestion would be to create intel_setup_sagv() (or intel_init_sagv) and then call it from somewhere (maybe the end of this function?).
Also, I915_SAGV_NOT_CONTROLLED is only used on Skylake. By setting sagv_status to to you're making i915.enable_sagv behave differently on SKL compared to "all the other" (aka only KBL now) platforms. It would probably be better to have unified behavior, maybe by reworking the I915_SAGV_NOT_CONTROLLED handling or just adding a new flag or something else.
}- } else if (HAS_PCH_SPLIT(dev)) {
ilk_wm_get_hw_state(dev);
- }
for_each_intel_crtc(dev, crtc) { unsigned long put_domains;
Having skl_wm_level contain all of the watermarks for each plane is annoying since it prevents us from having any sort of object to represent a single watermark level, something we take advantage of in the next commit to cut down on all of the copy paste code in here.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 6 +- drivers/gpu/drm/i915/intel_drv.h | 6 +- drivers/gpu/drm/i915/intel_pm.c | 208 +++++++++++++++++---------------------- 3 files changed, 100 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d26e5999..0f97d43 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1648,9 +1648,9 @@ struct skl_wm_values { };
struct skl_wm_level { - bool plane_en[I915_MAX_PLANES]; - uint16_t plane_res_b[I915_MAX_PLANES]; - uint8_t plane_res_l[I915_MAX_PLANES]; + bool plane_en; + uint16_t plane_res_b; + uint8_t plane_res_l; };
/* diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 35ba282..d684f4f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -468,9 +468,13 @@ struct intel_pipe_wm { bool sprites_scaled; };
-struct skl_pipe_wm { +struct skl_plane_wm { struct skl_wm_level wm[8]; struct skl_wm_level trans_wm; +}; + +struct skl_pipe_wm { + struct skl_plane_wm planes[I915_MAX_PLANES]; uint32_t linetime; };
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index af96888..250f12d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3668,67 +3668,52 @@ static int skl_compute_wm_level(const struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb, struct intel_crtc_state *cstate, + struct intel_plane *intel_plane, int level, struct skl_wm_level *result) { struct drm_atomic_state *state = cstate->base.state; struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); - struct drm_plane *plane; - struct intel_plane *intel_plane; - struct intel_plane_state *intel_pstate; + struct drm_plane *plane = &intel_plane->base; + struct intel_plane_state *intel_pstate = NULL; uint16_t ddb_blocks; enum pipe pipe = intel_crtc->pipe; int ret; + int i = skl_wm_plane_id(intel_plane); + + if (state) + intel_pstate = + intel_atomic_get_existing_plane_state(state, + intel_plane);
/* - * We'll only calculate watermarks for planes that are actually - * enabled, so make sure all other planes are set as disabled. + * Note: If we start supporting multiple pending atomic commits against + * the same planes/CRTC's in the future, plane->state will no longer be + * the correct pre-state to use for the calculations here and we'll + * need to change where we get the 'unchanged' plane data from. + * + * For now this is fine because we only allow one queued commit against + * a CRTC. Even if the plane isn't modified by this transaction and we + * don't have a plane lock, we still have the CRTC's lock, so we know + * that no other transactions are racing with us to update it. */ - memset(result, 0, sizeof(*result)); - - for_each_intel_plane_mask(&dev_priv->drm, - intel_plane, - cstate->base.plane_mask) { - int i = skl_wm_plane_id(intel_plane); - - plane = &intel_plane->base; - intel_pstate = NULL; - if (state) - intel_pstate = - intel_atomic_get_existing_plane_state(state, - intel_plane); + if (!intel_pstate) + intel_pstate = to_intel_plane_state(plane->state);
- /* - * Note: If we start supporting multiple pending atomic commits - * against the same planes/CRTC's in the future, plane->state - * will no longer be the correct pre-state to use for the - * calculations here and we'll need to change where we get the - * 'unchanged' plane data from. - * - * For now this is fine because we only allow one queued commit - * against a CRTC. Even if the plane isn't modified by this - * transaction and we don't have a plane lock, we still have - * the CRTC's lock, so we know that no other transactions are - * racing with us to update it. - */ - if (!intel_pstate) - intel_pstate = to_intel_plane_state(plane->state); + WARN_ON(!intel_pstate->base.fb);
- WARN_ON(!intel_pstate->base.fb); + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
- ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]); - - ret = skl_compute_plane_wm(dev_priv, - cstate, - intel_pstate, - ddb_blocks, - level, - &result->plane_res_b[i], - &result->plane_res_l[i], - &result->plane_en[i]); - if (ret) - return ret; - } + ret = skl_compute_plane_wm(dev_priv, + cstate, + intel_pstate, + ddb_blocks, + level, + &result->plane_res_b, + &result->plane_res_l, + &result->plane_en); + if (ret) + return ret;
return 0; } @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate) static void skl_compute_transition_wm(struct intel_crtc_state *cstate, struct skl_wm_level *trans_wm /* out */) { - struct drm_crtc *crtc = cstate->base.crtc; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_plane *intel_plane; - if (!cstate->base.active) return;
/* Until we know more, just disable transition WMs */ - for_each_intel_plane_on_crtc(crtc->dev, intel_crtc, intel_plane) { - int i = skl_wm_plane_id(intel_plane); - - trans_wm->plane_en[i] = false; - } + trans_wm->plane_en = false; }
static int skl_build_pipe_wm(struct intel_crtc_state *cstate, @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, { struct drm_device *dev = cstate->base.crtc->dev; const struct drm_i915_private *dev_priv = to_i915(dev); + struct intel_plane *intel_plane; + struct skl_plane_wm *wm; int level, max_level = ilk_wm_max_level(dev); int ret;
- for (level = 0; level <= max_level; level++) { - ret = skl_compute_wm_level(dev_priv, ddb, cstate, - level, &pipe_wm->wm[level]); - if (ret) - return ret; + /* + * We'll only calculate watermarks for planes that are actually + * enabled, so make sure all other planes are set as disabled. + */ + memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes)); + + for_each_intel_plane_mask(&dev_priv->drm, + intel_plane, + cstate->base.plane_mask) { + wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)]; + + for (level = 0; level <= max_level; level++) { + ret = skl_compute_wm_level(dev_priv, ddb, cstate, + intel_plane, level, + &wm->wm[level]); + if (ret) + return ret; + } + skl_compute_transition_wm(cstate, &wm->trans_wm); } pipe_wm->linetime = skl_compute_linetime_wm(cstate);
- skl_compute_transition_wm(cstate, &pipe_wm->trans_wm); - return 0; }
@@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct drm_device *dev, struct intel_crtc *intel_crtc) { int level, max_level = ilk_wm_max_level(dev); + struct skl_plane_wm *plane_wm; enum pipe pipe = intel_crtc->pipe; uint32_t temp; int i;
- for (level = 0; level <= max_level; level++) { - for (i = 0; i < intel_num_planes(intel_crtc); i++) { + for (i = 0; i < intel_num_planes(intel_crtc); i++) { + plane_wm = &p_wm->planes[i]; + + for (level = 0; level <= max_level; level++) { temp = 0;
- temp |= p_wm->wm[level].plane_res_l[i] << + temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT; - temp |= p_wm->wm[level].plane_res_b[i]; - if (p_wm->wm[level].plane_en[i]) + temp |= plane_wm->wm[level].plane_res_b; + if (plane_wm->wm[level].plane_en) temp |= PLANE_WM_EN;
r->plane[pipe][i][level] = temp; }
- temp = 0; - - temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] << PLANE_WM_LINES_SHIFT; - temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR]; + }
- if (p_wm->wm[level].plane_en[PLANE_CURSOR]) + for (level = 0; level <= max_level; level++) { + plane_wm = &p_wm->planes[PLANE_CURSOR]; + temp = 0; + temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT; + temp |= plane_wm->wm[level].plane_res_b; + if (plane_wm->wm[level].plane_en) temp |= PLANE_WM_EN;
r->plane[pipe][PLANE_CURSOR][level] = temp; - }
/* transition WMs */ for (i = 0; i < intel_num_planes(intel_crtc); i++) { + plane_wm = &p_wm->planes[i]; temp = 0; - temp |= p_wm->trans_wm.plane_res_l[i] << PLANE_WM_LINES_SHIFT; - temp |= p_wm->trans_wm.plane_res_b[i]; - if (p_wm->trans_wm.plane_en[i]) + temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT; + temp |= plane_wm->trans_wm.plane_res_b; + if (plane_wm->trans_wm.plane_en) temp |= PLANE_WM_EN;
r->plane_trans[pipe][i] = temp; }
+ plane_wm = &p_wm->planes[PLANE_CURSOR]; temp = 0; - temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] << PLANE_WM_LINES_SHIFT; - temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR]; - if (p_wm->trans_wm.plane_en[PLANE_CURSOR]) + temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT; + temp |= plane_wm->trans_wm.plane_res_b; + if (plane_wm->trans_wm.plane_en) temp |= PLANE_WM_EN;
r->plane_trans[pipe][PLANE_CURSOR] = temp; @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) static void skl_pipe_wm_active_state(uint32_t val, struct skl_pipe_wm *active, bool is_transwm, - bool is_cursor, int i, int level) { + struct skl_plane_wm *plane_wm = &active->planes[i]; bool is_enabled = (val & PLANE_WM_EN) != 0;
if (!is_transwm) { - if (!is_cursor) { - active->wm[level].plane_en[i] = is_enabled; - active->wm[level].plane_res_b[i] = - val & PLANE_WM_BLOCKS_MASK; - active->wm[level].plane_res_l[i] = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } else { - active->wm[level].plane_en[PLANE_CURSOR] = is_enabled; - active->wm[level].plane_res_b[PLANE_CURSOR] = - val & PLANE_WM_BLOCKS_MASK; - active->wm[level].plane_res_l[PLANE_CURSOR] = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } + plane_wm->wm[level].plane_en = is_enabled; + plane_wm->wm[level].plane_res_b = val & PLANE_WM_BLOCKS_MASK; + plane_wm->wm[level].plane_res_l = + (val >> PLANE_WM_LINES_SHIFT) & + PLANE_WM_LINES_MASK; } else { - if (!is_cursor) { - active->trans_wm.plane_en[i] = is_enabled; - active->trans_wm.plane_res_b[i] = - val & PLANE_WM_BLOCKS_MASK; - active->trans_wm.plane_res_l[i] = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } else { - active->trans_wm.plane_en[PLANE_CURSOR] = is_enabled; - active->trans_wm.plane_res_b[PLANE_CURSOR] = - val & PLANE_WM_BLOCKS_MASK; - active->trans_wm.plane_res_l[PLANE_CURSOR] = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } + plane_wm->trans_wm.plane_en = is_enabled; + plane_wm->trans_wm.plane_res_b = val & PLANE_WM_BLOCKS_MASK; + plane_wm->trans_wm.plane_res_l = + (val >> PLANE_WM_LINES_SHIFT) & + PLANE_WM_LINES_MASK; } }
@@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane[pipe][i][level]; - skl_pipe_wm_active_state(temp, active, false, - false, i, level); + skl_pipe_wm_active_state(temp, active, false, i, level); } temp = hw->plane[pipe][PLANE_CURSOR][level]; - skl_pipe_wm_active_state(temp, active, false, true, i, level); + skl_pipe_wm_active_state(temp, active, false, i, level); }
for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane_trans[pipe][i]; - skl_pipe_wm_active_state(temp, active, true, false, i, 0); + skl_pipe_wm_active_state(temp, active, true, i, 0); }
temp = hw->plane_trans[pipe][PLANE_CURSOR]; - skl_pipe_wm_active_state(temp, active, true, true, i, 0); + skl_pipe_wm_active_state(temp, active, true, i, 0);
intel_crtc->wm.active.skl = *active; }
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
Having skl_wm_level contain all of the watermarks for each plane is annoying since it prevents us from having any sort of object to represent a single watermark level, something we take advantage of in the next commit to cut down on all of the copy paste code in here.
I'd like to start my review pointing that I really like this patch. I agree the current form is annoying.
See below for some details.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_drv.h | 6 +- drivers/gpu/drm/i915/intel_drv.h | 6 +- drivers/gpu/drm/i915/intel_pm.c | 208 +++++++++++++++++----------
3 files changed, 100 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d26e5999..0f97d43 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1648,9 +1648,9 @@ struct skl_wm_values { }; struct skl_wm_level {
- bool plane_en[I915_MAX_PLANES];
- uint16_t plane_res_b[I915_MAX_PLANES];
- uint8_t plane_res_l[I915_MAX_PLANES];
- bool plane_en;
- uint16_t plane_res_b;
- uint8_t plane_res_l;
}; /* diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 35ba282..d684f4f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -468,9 +468,13 @@ struct intel_pipe_wm { bool sprites_scaled; }; -struct skl_pipe_wm { +struct skl_plane_wm { struct skl_wm_level wm[8]; struct skl_wm_level trans_wm; +};
+struct skl_pipe_wm {
- struct skl_plane_wm planes[I915_MAX_PLANES];
uint32_t linetime; }; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index af96888..250f12d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3668,67 +3668,52 @@ static int skl_compute_wm_level(const struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb, struct intel_crtc_state *cstate,
struct intel_plane *intel_plane,int level, struct skl_wm_level *result) { struct drm_atomic_state *state = cstate->base.state; struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
base.crtc);
- struct drm_plane *plane;
- struct intel_plane *intel_plane;
- struct intel_plane_state *intel_pstate;
- struct drm_plane *plane = &intel_plane->base;
- struct intel_plane_state *intel_pstate = NULL;
uint16_t ddb_blocks; enum pipe pipe = intel_crtc->pipe; int ret;
- int i = skl_wm_plane_id(intel_plane);
- if (state)
intel_pstate =intel_atomic_get_existing_plane_state(state,intel_plane); /*
- * We'll only calculate watermarks for planes that are
actually
- * enabled, so make sure all other planes are set as
disabled.
- * Note: If we start supporting multiple pending atomic
commits against
- * the same planes/CRTC's in the future, plane->state will
no longer be
- * the correct pre-state to use for the calculations here
and we'll
- * need to change where we get the 'unchanged' plane data
from.
- *
- * For now this is fine because we only allow one queued
commit against
- * a CRTC. Even if the plane isn't modified by this
transaction and we
- * don't have a plane lock, we still have the CRTC's lock,
so we know
- * that no other transactions are racing with us to update
it. */
- memset(result, 0, sizeof(*result));
- for_each_intel_plane_mask(&dev_priv->drm,
intel_plane,cstate->base.plane_mask) {int i = skl_wm_plane_id(intel_plane);plane = &intel_plane->base;intel_pstate = NULL;if (state)intel_pstate =intel_atomic_get_existing_plane_state(state,
intel_plane);
- if (!intel_pstate)
intel_pstate = to_intel_plane_state(plane->state);
/** Note: If we start supporting multiple pendingatomic commits
* against the same planes/CRTC's in the future,plane->state
* will no longer be the correct pre-state to usefor the
* calculations here and we'll need to change wherewe get the
* 'unchanged' plane data from.** For now this is fine because we only allow onequeued commit
* against a CRTC. Even if the plane isn't modifiedby this
* transaction and we don't have a plane lock, westill have
* the CRTC's lock, so we know that no othertransactions are
* racing with us to update it.*/if (!intel_pstate)intel_pstate = to_intel_plane_state(plane-state);
- WARN_ON(!intel_pstate->base.fb);
WARN_ON(!intel_pstate->base.fb);
- ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
ddb_blocks = skl_ddb_entry_size(&ddb-plane[pipe][i]);
ret = skl_compute_plane_wm(dev_priv,cstate,intel_pstate,ddb_blocks,level,&result->plane_res_b[i],&result->plane_res_l[i],&result->plane_en[i]);if (ret)return ret;- }
- ret = skl_compute_plane_wm(dev_priv,
cstate,intel_pstate,ddb_blocks,level,&result->plane_res_b,&result->plane_res_l,&result->plane_en);- if (ret)
return ret;return 0; } @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate) static void skl_compute_transition_wm(struct intel_crtc_state *cstate, struct skl_wm_level *trans_wm /* out */) {
- struct drm_crtc *crtc = cstate->base.crtc;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct intel_plane *intel_plane;
if (!cstate->base.active) return; /* Until we know more, just disable transition WMs */
- for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
intel_plane) {
int i = skl_wm_plane_id(intel_plane);trans_wm->plane_en[i] = false;- }
- trans_wm->plane_en = false;
} static int skl_build_pipe_wm(struct intel_crtc_state *cstate, @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, { struct drm_device *dev = cstate->base.crtc->dev; const struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_plane *intel_plane;
- struct skl_plane_wm *wm;
int level, max_level = ilk_wm_max_level(dev); int ret;
- for (level = 0; level <= max_level; level++) {
ret = skl_compute_wm_level(dev_priv, ddb, cstate,level, &pipe_wm-wm[level]);
if (ret)return ret;
- /*
- * We'll only calculate watermarks for planes that are
actually
- * enabled, so make sure all other planes are set as
disabled.
- */
- memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
- for_each_intel_plane_mask(&dev_priv->drm,
intel_plane,cstate->base.plane_mask) {wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];for (level = 0; level <= max_level; level++) {ret = skl_compute_wm_level(dev_priv, ddb,cstate,
intel_plane,level,
&wm->wm[level]);if (ret)return ret;}skl_compute_transition_wm(cstate, &wm->trans_wm);} pipe_wm->linetime = skl_compute_linetime_wm(cstate);
- skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
return 0; } @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct drm_device *dev, struct intel_crtc *intel_crtc) { int level, max_level = ilk_wm_max_level(dev);
- struct skl_plane_wm *plane_wm;
enum pipe pipe = intel_crtc->pipe; uint32_t temp; int i;
- for (level = 0; level <= max_level; level++) {
for (i = 0; i < intel_num_planes(intel_crtc); i++) {
- for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];for (level = 0; level <= max_level; level++) {temp = 0;
temp |= p_wm->wm[level].plane_res_l[i] <<
temp |= plane_wm->wm[level].plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= p_wm->wm[level].plane_res_b[i];if (p_wm->wm[level].plane_en[i])
temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en)temp |= PLANE_WM_EN; r->plane[pipe][i][level] = temp; }
temp = 0;temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<PLANE_WM_LINES_SHIFT;
temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
- }
if (p_wm->wm[level].plane_en[PLANE_CURSOR])
- for (level = 0; level <= max_level; level++) {
plane_wm = &p_wm->planes[PLANE_CURSOR];temp = 0;temp |= plane_wm->wm[level].plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en)temp |= PLANE_WM_EN; r->plane[pipe][PLANE_CURSOR][level] = temp;
} /* transition WMs */ for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];temp = 0;
temp |= p_wm->trans_wm.plane_res_l[i] <<PLANE_WM_LINES_SHIFT;
temp |= p_wm->trans_wm.plane_res_b[i];if (p_wm->trans_wm.plane_en[i])
temp |= plane_wm->trans_wm.plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->trans_wm.plane_res_b;if (plane_wm->trans_wm.plane_en)temp |= PLANE_WM_EN; r->plane_trans[pipe][i] = temp; }
- plane_wm = &p_wm->planes[PLANE_CURSOR];
temp = 0;
- temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
PLANE_WM_LINES_SHIFT;
- temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
- if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
- temp |= plane_wm->trans_wm.plane_res_l <<
PLANE_WM_LINES_SHIFT;
- temp |= plane_wm->trans_wm.plane_res_b;
- if (plane_wm->trans_wm.plane_en)
temp |= PLANE_WM_EN; r->plane_trans[pipe][PLANE_CURSOR] = temp; @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) static void skl_pipe_wm_active_state(uint32_t val, struct skl_pipe_wm *active, bool is_transwm,
bool is_cursor,int i, int level) {
- struct skl_plane_wm *plane_wm = &active->planes[i];
bool is_enabled = (val & PLANE_WM_EN) != 0; if (!is_transwm) {
if (!is_cursor) {active->wm[level].plane_en[i] = is_enabled;active->wm[level].plane_res_b[i] =val & PLANE_WM_BLOCKS_MASK;active->wm[level].plane_res_l[i] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;} else {active->wm[level].plane_en[PLANE_CURSOR] =is_enabled;
active->wm[level].plane_res_b[PLANE_CURSOR]=
val & PLANE_WM_BLOCKS_MASK;active->wm[level].plane_res_l[PLANE_CURSOR]=
(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;}
plane_wm->wm[level].plane_en = is_enabled;plane_wm->wm[level].plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->wm[level].plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;
Nitpick: you can join the two lines above and still stay under 80 columns.
} else {
if (!is_cursor) {active->trans_wm.plane_en[i] = is_enabled;active->trans_wm.plane_res_b[i] =val & PLANE_WM_BLOCKS_MASK;active->trans_wm.plane_res_l[i] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;} else {active->trans_wm.plane_en[PLANE_CURSOR] =is_enabled;
active->trans_wm.plane_res_b[PLANE_CURSOR] =val & PLANE_WM_BLOCKS_MASK;active->trans_wm.plane_res_l[PLANE_CURSOR] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;}
plane_wm->trans_wm.plane_en = is_enabled;plane_wm->trans_wm.plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->trans_wm.plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;
Same here.
} } @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane[pipe][i][level];
skl_pipe_wm_active_state(temp, active,false,
false, i, level);
skl_pipe_wm_active_state(temp, active,false, i, level); } temp = hw->plane[pipe][PLANE_CURSOR][level];
skl_pipe_wm_active_state(temp, active, false, true,i, level);
skl_pipe_wm_active_state(temp, active, false, i,level);
While this is not wrong today, history shows that the number of planes increases over time, so we may at some point in the future add PLANE_D and more, so the code will become wrong. Just pass PLANE_CURSOR instead of "i" here and below. Also, this simplification could have been a separate patch.
Everything else looks correct, so if you fix the detail above I'll provide a r-b tag.
} for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane_trans[pipe][i];
skl_pipe_wm_active_state(temp, active, true, false,i, 0);
skl_pipe_wm_active_state(temp, active, true, i, 0);} temp = hw->plane_trans[pipe][PLANE_CURSOR];
- skl_pipe_wm_active_state(temp, active, true, true, i, 0);
- skl_pipe_wm_active_state(temp, active, true, i, 0);
intel_crtc->wm.active.skl = *active; }
Op 05-10-16 om 22:33 schreef Paulo Zanoni:
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
Having skl_wm_level contain all of the watermarks for each plane is annoying since it prevents us from having any sort of object to represent a single watermark level, something we take advantage of in the next commit to cut down on all of the copy paste code in here.
I'd like to start my review pointing that I really like this patch. I agree the current form is annoying.
See below for some details.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_drv.h | 6 +- drivers/gpu/drm/i915/intel_drv.h | 6 +- drivers/gpu/drm/i915/intel_pm.c | 208 +++++++++++++++++----------
3 files changed, 100 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d26e5999..0f97d43 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1648,9 +1648,9 @@ struct skl_wm_values { };
struct skl_wm_level {
- bool plane_en[I915_MAX_PLANES];
- uint16_t plane_res_b[I915_MAX_PLANES];
- uint8_t plane_res_l[I915_MAX_PLANES];
- bool plane_en;
- uint16_t plane_res_b;
- uint8_t plane_res_l;
};
/* diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 35ba282..d684f4f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -468,9 +468,13 @@ struct intel_pipe_wm { bool sprites_scaled; };
-struct skl_pipe_wm { +struct skl_plane_wm { struct skl_wm_level wm[8]; struct skl_wm_level trans_wm; +};
+struct skl_pipe_wm {
- struct skl_plane_wm planes[I915_MAX_PLANES]; uint32_t linetime;
};
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index af96888..250f12d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3668,67 +3668,52 @@ static int skl_compute_wm_level(const struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb, struct intel_crtc_state *cstate,
struct intel_plane *intel_plane, int level, struct skl_wm_level *result){ struct drm_atomic_state *state = cstate->base.state; struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
base.crtc);
- struct drm_plane *plane;
- struct intel_plane *intel_plane;
- struct intel_plane_state *intel_pstate;
- struct drm_plane *plane = &intel_plane->base;
- struct intel_plane_state *intel_pstate = NULL; uint16_t ddb_blocks; enum pipe pipe = intel_crtc->pipe; int ret;
- int i = skl_wm_plane_id(intel_plane);
- if (state)
intel_pstate =intel_atomic_get_existing_plane_state(state,intel_plane);
/*
* We'll only calculate watermarks for planes that areactually
* enabled, so make sure all other planes are set asdisabled.
* Note: If we start supporting multiple pending atomiccommits against
* the same planes/CRTC's in the future, plane->state willno longer be
* the correct pre-state to use for the calculations hereand we'll
* need to change where we get the 'unchanged' plane datafrom.
** For now this is fine because we only allow one queuedcommit against
* a CRTC. Even if the plane isn't modified by thistransaction and we
* don't have a plane lock, we still have the CRTC's lock,so we know
* that no other transactions are racing with us to updateit. */
- memset(result, 0, sizeof(*result));
- for_each_intel_plane_mask(&dev_priv->drm,
intel_plane,cstate->base.plane_mask) {int i = skl_wm_plane_id(intel_plane);plane = &intel_plane->base;intel_pstate = NULL;if (state)intel_pstate =intel_atomic_get_existing_plane_state(state,
- if (!intel_pstate)
intel_pstate = to_intel_plane_state(plane->state);
/** Note: If we start supporting multiple pendingatomic commits
* against the same planes/CRTC's in the future,plane->state
* will no longer be the correct pre-state to usefor the
* calculations here and we'll need to change wherewe get the
* 'unchanged' plane data from.** For now this is fine because we only allow onequeued commit
* against a CRTC. Even if the plane isn't modifiedby this
* transaction and we don't have a plane lock, westill have
* the CRTC's lock, so we know that no othertransactions are
* racing with us to update it.*/if (!intel_pstate)intel_pstate = to_intel_plane_state(plane-state);
- WARN_ON(!intel_pstate->base.fb);
WARN_ON(!intel_pstate->base.fb);
- ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
ddb_blocks = skl_ddb_entry_size(&ddb-plane[pipe][i]);
ret = skl_compute_plane_wm(dev_priv,cstate,intel_pstate,ddb_blocks,level,&result->plane_res_b[i],&result->plane_res_l[i],&result->plane_en[i]);if (ret)return ret;- }
ret = skl_compute_plane_wm(dev_priv,
cstate,intel_pstate,ddb_blocks,level,&result->plane_res_b,&result->plane_res_l,&result->plane_en);if (ret)
return ret;return 0;
} @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate) static void skl_compute_transition_wm(struct intel_crtc_state *cstate, struct skl_wm_level *trans_wm /* out */) {
struct drm_crtc *crtc = cstate->base.crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_plane *intel_plane;
if (!cstate->base.active) return;
/* Until we know more, just disable transition WMs */
for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
intel_plane) {
int i = skl_wm_plane_id(intel_plane);trans_wm->plane_en[i] = false;- }
- trans_wm->plane_en = false;
}
static int skl_build_pipe_wm(struct intel_crtc_state *cstate, @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, { struct drm_device *dev = cstate->base.crtc->dev; const struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_plane *intel_plane;
- struct skl_plane_wm *wm; int level, max_level = ilk_wm_max_level(dev); int ret;
- for (level = 0; level <= max_level; level++) {
ret = skl_compute_wm_level(dev_priv, ddb, cstate,level, &pipe_wm-wm[level]);
if (ret)return ret;
- /*
* We'll only calculate watermarks for planes that areactually
* enabled, so make sure all other planes are set asdisabled.
*/- memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
- for_each_intel_plane_mask(&dev_priv->drm,
intel_plane,cstate->base.plane_mask) {wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];for (level = 0; level <= max_level; level++) {ret = skl_compute_wm_level(dev_priv, ddb,cstate,
intel_plane,level,
&wm->wm[level]);if (ret)return ret;}skl_compute_transition_wm(cstate, &wm->trans_wm);
- skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
- return 0;
}
@@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct drm_device *dev, struct intel_crtc *intel_crtc) { int level, max_level = ilk_wm_max_level(dev);
- struct skl_plane_wm *plane_wm; enum pipe pipe = intel_crtc->pipe; uint32_t temp; int i;
- for (level = 0; level <= max_level; level++) {
for (i = 0; i < intel_num_planes(intel_crtc); i++) {
- for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];for (level = 0; level <= max_level; level++) { temp = 0;
temp |= p_wm->wm[level].plane_res_l[i] <<
temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT;
temp |= p_wm->wm[level].plane_res_b[i];if (p_wm->wm[level].plane_en[i])
temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en) temp |= PLANE_WM_EN; r->plane[pipe][i][level] = temp;}
temp = 0;temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<PLANE_WM_LINES_SHIFT;
temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
- }
if (p_wm->wm[level].plane_en[PLANE_CURSOR])
- for (level = 0; level <= max_level; level++) {
plane_wm = &p_wm->planes[PLANE_CURSOR];temp = 0;temp |= plane_wm->wm[level].plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en) temp |= PLANE_WM_EN;r->plane[pipe][PLANE_CURSOR][level] = temp;
}
/* transition WMs */ for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];
temp |= p_wm->trans_wm.plane_res_l[i] <<PLANE_WM_LINES_SHIFT;
temp |= p_wm->trans_wm.plane_res_b[i];if (p_wm->trans_wm.plane_en[i])
temp |= plane_wm->trans_wm.plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->trans_wm.plane_res_b;if (plane_wm->trans_wm.plane_en) temp |= PLANE_WM_EN;r->plane_trans[pipe][i] = temp; }
plane_wm = &p_wm->planes[PLANE_CURSOR]; temp = 0;
- temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
PLANE_WM_LINES_SHIFT;
- temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
- if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
- temp |= plane_wm->trans_wm.plane_res_l <<
PLANE_WM_LINES_SHIFT;
temp |= plane_wm->trans_wm.plane_res_b;
if (plane_wm->trans_wm.plane_en) temp |= PLANE_WM_EN;
r->plane_trans[pipe][PLANE_CURSOR] = temp;
@@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) static void skl_pipe_wm_active_state(uint32_t val, struct skl_pipe_wm *active, bool is_transwm,
bool is_cursor, int i, int level){
struct skl_plane_wm *plane_wm = &active->planes[i]; bool is_enabled = (val & PLANE_WM_EN) != 0;
if (!is_transwm) {
if (!is_cursor) {active->wm[level].plane_en[i] = is_enabled;active->wm[level].plane_res_b[i] =val & PLANE_WM_BLOCKS_MASK;active->wm[level].plane_res_l[i] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;} else {active->wm[level].plane_en[PLANE_CURSOR] =is_enabled;
active->wm[level].plane_res_b[PLANE_CURSOR]=
val & PLANE_WM_BLOCKS_MASK;active->wm[level].plane_res_l[PLANE_CURSOR]=
(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;}
plane_wm->wm[level].plane_en = is_enabled;plane_wm->wm[level].plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->wm[level].plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;Nitpick: you can join the two lines above and still stay under 80 columns.
} else {
if (!is_cursor) {active->trans_wm.plane_en[i] = is_enabled;active->trans_wm.plane_res_b[i] =val & PLANE_WM_BLOCKS_MASK;active->trans_wm.plane_res_l[i] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;} else {active->trans_wm.plane_en[PLANE_CURSOR] =is_enabled;
active->trans_wm.plane_res_b[PLANE_CURSOR] =val & PLANE_WM_BLOCKS_MASK;active->trans_wm.plane_res_l[PLANE_CURSOR] =(val >>PLANE_WM_LINES_SHIFT) &
PLANE_WM_LINES_MASK;}
plane_wm->trans_wm.plane_en = is_enabled;plane_wm->trans_wm.plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->trans_wm.plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;Same here.
} }
@@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) for (level = 0; level <= max_level; level++) { for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane[pipe][i][level];
skl_pipe_wm_active_state(temp, active,false,
false, i, level);
skl_pipe_wm_active_state(temp, active,false, i, level); } temp = hw->plane[pipe][PLANE_CURSOR][level];
skl_pipe_wm_active_state(temp, active, false, true,i, level);
skl_pipe_wm_active_state(temp, active, false, i,level);
While this is not wrong today, history shows that the number of planes increases over time, so we may at some point in the future add PLANE_D and more, so the code will become wrong. Just pass PLANE_CURSOR instead of "i" here and below. Also, this simplification could have been a separate patch.
Agreed, but I want to note that PLANE_CURSOR is always supposed to be the last member. Unless you have sprite planes covering the cursor, which doesn't ever happen.
~Maarten
Now that we've make skl_wm_levels make a little more sense, we can remove all of the redundant wm information. Up until now we'd been storing two copies of all of the skl watermarks: one being the skl_pipe_wm structs, the other being the global wm struct in drm_i915_private containing the raw register values. This is confusing and problematic, since it means we're prone to accidentally letting the two copies go out of sync. So, get rid of all of the functions responsible for computing the register values and just use a single helper, skl_write_wm_level(), to convert and write the new watermarks on the fly.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 2 - drivers/gpu/drm/i915/intel_display.c | 14 ++- drivers/gpu/drm/i915/intel_drv.h | 6 +- drivers/gpu/drm/i915/intel_pm.c | 203 ++++++++++++----------------------- drivers/gpu/drm/i915/intel_sprite.c | 8 +- 5 files changed, 88 insertions(+), 145 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0f97d43..63519ac 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1643,8 +1643,6 @@ struct skl_ddb_allocation { struct skl_wm_values { unsigned dirty_pipes; struct skl_ddb_allocation ddb; - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; };
struct skl_wm_level { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd15ae2..c580d3d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane, 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; @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane, intel_crtc->adjusted_y = src_y;
if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) - skl_write_plane_wm(intel_crtc, wm, 0); + 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); @@ -3448,6 +3450,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;
/* @@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary, * plane's visiblity isn't actually changing neither is its watermarks. */ if (!crtc->primary->state->visible) - skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0); + 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); @@ -10819,12 +10824,15 @@ 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, wm); + skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
if (plane_state && plane_state->base.visible) { cntl = MCURSOR_GAMMA_ENABLE; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d684f4f..958dc72 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old, 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_wm_values *wm); + 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_wm_values *wm, + 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); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 250f12d..7708646 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) 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_state *cstate; + struct skl_plane_wm *wm; enum pipe pipe; int level, plane;
@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) /* Since we're now guaranteed to only have one active CRTC... */ pipe = ffs(intel_state->active_crtcs) - 1; crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + cstate = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) return false;
for_each_plane(dev_priv, pipe, plane) { + wm = &cstate->wm.skl.optimal.planes[plane]; + /* Skip this plane if it's not enabled */ - if (intel_state->wm_results.plane[pipe][plane][0] == 0) + if (!wm->wm[0].plane_en) continue;
/* Find the highest enabled wm level for this plane */ - for (level = ilk_wm_max_level(dev); - intel_state->wm_results.plane[pipe][plane][level] == 0; --level) + for (level = ilk_wm_max_level(dev); !wm->wm[level].plane_en; + --level) { }
/* @@ -3777,67 +3782,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, return 0; }
-static void skl_compute_wm_results(struct drm_device *dev, - struct skl_pipe_wm *p_wm, - struct skl_wm_values *r, - struct intel_crtc *intel_crtc) -{ - int level, max_level = ilk_wm_max_level(dev); - struct skl_plane_wm *plane_wm; - enum pipe pipe = intel_crtc->pipe; - uint32_t temp; - int i; - - for (i = 0; i < intel_num_planes(intel_crtc); i++) { - plane_wm = &p_wm->planes[i]; - - for (level = 0; level <= max_level; level++) { - temp = 0; - - temp |= plane_wm->wm[level].plane_res_l << - PLANE_WM_LINES_SHIFT; - temp |= plane_wm->wm[level].plane_res_b; - if (plane_wm->wm[level].plane_en) - temp |= PLANE_WM_EN; - - r->plane[pipe][i][level] = temp; - } - - } - - for (level = 0; level <= max_level; level++) { - plane_wm = &p_wm->planes[PLANE_CURSOR]; - temp = 0; - temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT; - temp |= plane_wm->wm[level].plane_res_b; - if (plane_wm->wm[level].plane_en) - temp |= PLANE_WM_EN; - - r->plane[pipe][PLANE_CURSOR][level] = temp; - } - - /* transition WMs */ - for (i = 0; i < intel_num_planes(intel_crtc); i++) { - plane_wm = &p_wm->planes[i]; - temp = 0; - temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT; - temp |= plane_wm->trans_wm.plane_res_b; - if (plane_wm->trans_wm.plane_en) - temp |= PLANE_WM_EN; - - r->plane_trans[pipe][i] = temp; - } - - plane_wm = &p_wm->planes[PLANE_CURSOR]; - temp = 0; - temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT; - temp |= plane_wm->trans_wm.plane_res_b; - if (plane_wm->trans_wm.plane_en) - temp |= PLANE_WM_EN; - - r->plane_trans[pipe][PLANE_CURSOR] = temp; -} - static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, i915_reg_t reg, const struct skl_ddb_entry *entry) @@ -3848,8 +3792,22 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, I915_WRITE(reg, 0); }
+static void skl_write_wm_level(struct drm_i915_private *dev_priv, + i915_reg_t reg, + const struct skl_wm_level *level) +{ + uint32_t val = 0; + + val |= level->plane_res_b; + val |= level->plane_res_l << PLANE_WM_LINES_SHIFT; + val |= level->plane_en; + + I915_WRITE(reg, val); +} + void skl_write_plane_wm(struct intel_crtc *intel_crtc, - const struct skl_wm_values *wm, + const struct skl_plane_wm *wm, + const struct skl_ddb_allocation *ddb, int plane) { struct drm_crtc *crtc = &intel_crtc->base; @@ -3859,19 +3817,21 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc, enum pipe pipe = intel_crtc->pipe;
for (level = 0; level <= max_level; level++) { - I915_WRITE(PLANE_WM(pipe, plane, level), - wm->plane[pipe][plane][level]); + skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane, level), + &wm->wm[level]); } - I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]); + skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane), + &wm->trans_wm);
skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane), - &wm->ddb.plane[pipe][plane]); + &ddb->plane[pipe][plane]); skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane), - &wm->ddb.y_plane[pipe][plane]); + &ddb->y_plane[pipe][plane]); }
void skl_write_cursor_wm(struct intel_crtc *intel_crtc, - const struct skl_wm_values *wm) + const struct skl_plane_wm *wm, + const struct skl_ddb_allocation *ddb) { struct drm_crtc *crtc = &intel_crtc->base; struct drm_device *dev = crtc->dev; @@ -3880,13 +3840,13 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc, enum pipe pipe = intel_crtc->pipe;
for (level = 0; level <= max_level; level++) { - I915_WRITE(CUR_WM(pipe, level), - wm->plane[pipe][PLANE_CURSOR][level]); + skl_write_wm_level(dev_priv, CUR_WM(pipe, level), + &wm->wm[level]); } - I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]); + skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe), - &wm->ddb.plane[pipe][PLANE_CURSOR]); + &ddb->plane[pipe][PLANE_CURSOR]); }
static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a, @@ -4064,11 +4024,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, struct skl_wm_values *src, enum pipe pipe) { - memcpy(dst->plane[pipe], src->plane[pipe], - sizeof(dst->plane[pipe])); - memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], - sizeof(dst->plane_trans[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], @@ -4117,7 +4072,6 @@ skl_compute_wm(struct drm_atomic_state *state) * no suitable watermark values can be found. */ for_each_crtc_in_state(state, crtc, cstate, i) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
@@ -4135,7 +4089,6 @@ skl_compute_wm(struct drm_atomic_state *state) continue;
intel_cstate->update_wm_pre = true; - skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc); }
return 0; @@ -4169,9 +4122,11 @@ static void skl_update_wm(struct drm_crtc *crtc) int plane;
for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) - skl_write_plane_wm(intel_crtc, results, plane); + skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane], + &results->ddb, plane);
- skl_write_cursor_wm(intel_crtc, results); + skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR], + &results->ddb); }
skl_copy_wm_for_pipe(hw_vals, results, pipe); @@ -4256,28 +4211,13 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) mutex_unlock(&dev_priv->wm.wm_mutex); }
-static void skl_pipe_wm_active_state(uint32_t val, - struct skl_pipe_wm *active, - bool is_transwm, - int i, - int level) +static inline void skl_wm_level_from_reg_val(uint32_t val, + struct skl_wm_level *level) { - struct skl_plane_wm *plane_wm = &active->planes[i]; - bool is_enabled = (val & PLANE_WM_EN) != 0; - - if (!is_transwm) { - plane_wm->wm[level].plane_en = is_enabled; - plane_wm->wm[level].plane_res_b = val & PLANE_WM_BLOCKS_MASK; - plane_wm->wm[level].plane_res_l = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } else { - plane_wm->trans_wm.plane_en = is_enabled; - plane_wm->trans_wm.plane_res_b = val & PLANE_WM_BLOCKS_MASK; - plane_wm->trans_wm.plane_res_l = - (val >> PLANE_WM_LINES_SHIFT) & - PLANE_WM_LINES_MASK; - } + level->plane_en = val & PLANE_WM_EN; + level->plane_res_b = val & PLANE_WM_BLOCKS_MASK; + level->plane_res_l = (val & PLANE_WM_LINES_MASK) >> + PLANE_WM_LINES_SHIFT; }
static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) @@ -4287,23 +4227,33 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) struct skl_wm_values *hw = &dev_priv->wm.skl_hw; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); + struct intel_plane *intel_plane; struct skl_pipe_wm *active = &cstate->wm.skl.optimal; + struct skl_plane_wm *wm; enum pipe pipe = intel_crtc->pipe; - int level, i, max_level; - uint32_t temp; + int level, id, max_level = ilk_wm_max_level(dev); + uint32_t val;
- max_level = ilk_wm_max_level(dev); + for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { + id = skl_wm_plane_id(intel_plane); + wm = &cstate->wm.skl.optimal.planes[id];
- for (level = 0; level <= max_level; level++) { - for (i = 0; i < intel_num_planes(intel_crtc); i++) - hw->plane[pipe][i][level] = - I915_READ(PLANE_WM(pipe, i, level)); - hw->plane[pipe][PLANE_CURSOR][level] = I915_READ(CUR_WM(pipe, level)); - } + for (level = 0; level <= max_level; level++) { + if (id != PLANE_CURSOR) + val = I915_READ(PLANE_WM(pipe, id, level)); + else + val = I915_READ(CUR_WM(pipe, level)); + + skl_wm_level_from_reg_val(val, &wm->wm[level]); + }
- for (i = 0; i < intel_num_planes(intel_crtc); i++) - hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i)); - hw->plane_trans[pipe][PLANE_CURSOR] = I915_READ(CUR_WM_TRANS(pipe)); + if (id != PLANE_CURSOR) + val = I915_READ(PLANE_WM_TRANS(pipe, id)); + else + val = I915_READ(CUR_WM_TRANS(pipe)); + + skl_wm_level_from_reg_val(val, &wm->trans_wm); + }
if (!intel_crtc->active) return; @@ -4311,25 +4261,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) hw->dirty_pipes |= drm_crtc_mask(crtc);
active->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); - - for (level = 0; level <= max_level; level++) { - for (i = 0; i < intel_num_planes(intel_crtc); i++) { - temp = hw->plane[pipe][i][level]; - skl_pipe_wm_active_state(temp, active, false, i, level); - } - temp = hw->plane[pipe][PLANE_CURSOR][level]; - skl_pipe_wm_active_state(temp, active, false, i, level); - } - - for (i = 0; i < intel_num_planes(intel_crtc); i++) { - temp = hw->plane_trans[pipe][i]; - skl_pipe_wm_active_state(temp, active, true, i, 0); - } - - temp = hw->plane_trans[pipe][PLANE_CURSOR]; - skl_pipe_wm_active_state(temp, active, true, i, 0); - - intel_crtc->wm.active.skl = *active; }
void skl_wm_get_hw_state(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 73a521f..0fb775b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -208,6 +208,8 @@ skl_update_plane(struct drm_plane *drm_plane, 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; @@ -232,7 +234,7 @@ 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, wm, plane); + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
if (key->flags) { I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value); @@ -289,6 +291,7 @@ 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;
@@ -298,7 +301,8 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) */ if (!dplane->state->visible) skl_write_plane_wm(to_intel_crtc(crtc), - &dev_priv->wm.skl_results, plane); + &cstate->wm.skl.optimal.planes[plane], + &dev_priv->wm.skl_results.ddb, plane);
I915_WRITE(PLANE_CTL(pipe, plane), 0);
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
Now that we've make skl_wm_levels make a little more sense, we can remove all of the redundant wm information. Up until now we'd been storing two copies of all of the skl watermarks: one being the skl_pipe_wm structs, the other being the global wm struct in drm_i915_private containing the raw register values. This is confusing and problematic, since it means we're prone to accidentally letting the two copies go out of sync. So, get rid of all of the functions responsible for computing the register values and just use a single helper, skl_write_wm_level(), to convert and write the new watermarks on the fly.
I like the direction of this patch too, but there are some small possible problems. See below.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_drv.h | 2 - drivers/gpu/drm/i915/intel_display.c | 14 ++- drivers/gpu/drm/i915/intel_drv.h | 6 +- drivers/gpu/drm/i915/intel_pm.c | 203 ++++++++++++-------------
drivers/gpu/drm/i915/intel_sprite.c | 8 +- 5 files changed, 88 insertions(+), 145 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0f97d43..63519ac 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1643,8 +1643,6 @@ struct skl_ddb_allocation { struct skl_wm_values { unsigned dirty_pipes; struct skl_ddb_allocation ddb;
- uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
- uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
}; struct skl_wm_level { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd15ae2..c580d3d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane, 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];
I wish someone would do a patch to convert all these hardcoded values to PLANE_X, and convert "int"s to "enum plane"s everywhere.
int pipe = intel_crtc->pipe; u32 plane_ctl; unsigned int rotation = plane_state->base.rotation; @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane, intel_crtc->adjusted_y = src_y; if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
skl_write_plane_wm(intel_crtc, wm, 0);
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); @@ -3448,6 +3450,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; /* @@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary, * plane's visiblity isn't actually changing neither is its watermarks. */ if (!crtc->primary->state->visible)
skl_write_plane_wm(intel_crtc, &dev_priv-wm.skl_results, 0);
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); @@ -10819,12 +10824,15 @@ 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, wm);
skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);if (plane_state && plane_state->base.visible) { cntl = MCURSOR_GAMMA_ENABLE; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d684f4f..958dc72 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old, 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_wm_values *wm);
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_wm_values *wm,
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); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 250f12d..7708646 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) 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_state *cstate;
- struct skl_plane_wm *wm;
enum pipe pipe; int level, plane; @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state) /* Since we're now guaranteed to only have one active CRTC... */ pipe = ffs(intel_state->active_crtcs) - 1; crtc = dev_priv->pipe_to_crtc_mapping[pipe];
- cstate = intel_atomic_get_crtc_state(state,
to_intel_crtc(crtc)); if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE) return false; for_each_plane(dev_priv, pipe, plane) {
wm = &cstate->wm.skl.optimal.planes[plane];/* Skip this plane if it's not enabled */
if (intel_state->wm_results.plane[pipe][plane][0] ==
if (!wm->wm[0].plane_en)continue; /* Find the highest enabled wm level for this plane */
for (level = ilk_wm_max_level(dev);intel_state-wm_results.plane[pipe][plane][level] == 0; --level)
for (level = ilk_wm_max_level(dev); !wm-wm[level].plane_en;
--level){ } /* @@ -3777,67 +3782,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate, return 0; } -static void skl_compute_wm_results(struct drm_device *dev,
struct skl_pipe_wm *p_wm,struct skl_wm_values *r,struct intel_crtc *intel_crtc)-{
- int level, max_level = ilk_wm_max_level(dev);
- struct skl_plane_wm *plane_wm;
- enum pipe pipe = intel_crtc->pipe;
- uint32_t temp;
- int i;
- for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];for (level = 0; level <= max_level; level++) {temp = 0;temp |= plane_wm->wm[level].plane_res_l <<PLANE_WM_LINES_SHIFT;temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en)temp |= PLANE_WM_EN;r->plane[pipe][i][level] = temp;}- }
- for (level = 0; level <= max_level; level++) {
plane_wm = &p_wm->planes[PLANE_CURSOR];temp = 0;temp |= plane_wm->wm[level].plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->wm[level].plane_res_b;if (plane_wm->wm[level].plane_en)temp |= PLANE_WM_EN;r->plane[pipe][PLANE_CURSOR][level] = temp;- }
- /* transition WMs */
- for (i = 0; i < intel_num_planes(intel_crtc); i++) {
plane_wm = &p_wm->planes[i];temp = 0;temp |= plane_wm->trans_wm.plane_res_l <<PLANE_WM_LINES_SHIFT;
temp |= plane_wm->trans_wm.plane_res_b;if (plane_wm->trans_wm.plane_en)temp |= PLANE_WM_EN;r->plane_trans[pipe][i] = temp;- }
- plane_wm = &p_wm->planes[PLANE_CURSOR];
- temp = 0;
- temp |= plane_wm->trans_wm.plane_res_l <<
PLANE_WM_LINES_SHIFT;
- temp |= plane_wm->trans_wm.plane_res_b;
- if (plane_wm->trans_wm.plane_en)
temp |= PLANE_WM_EN;- r->plane_trans[pipe][PLANE_CURSOR] = temp;
-}
static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, i915_reg_t reg, const struct skl_ddb_entry *entry) @@ -3848,8 +3792,22 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, I915_WRITE(reg, 0); } +static void skl_write_wm_level(struct drm_i915_private *dev_priv,
i915_reg_t reg,const struct skl_wm_level *level)+{
- uint32_t val = 0;
- val |= level->plane_res_b;
- val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
- val |= level->plane_en;
The line above seems wrong, you should check for plane_en and then set PLANE_WM_EN.
IMHO it would even better if we completely zeroed the register in case plane_en is false, so we could have:
uint32_t val = 0;
if (level->plane_en) { val |= PLANE_WM_EN; val |= level->plane_res_b; val |= level->plane_res_l << PLANE_WM_LINES_SHIFT; }
- I915_WRITE(reg, val);
+}
void skl_write_plane_wm(struct intel_crtc *intel_crtc,
const struct skl_wm_values *wm,
const struct skl_plane_wm *wm,const struct skl_ddb_allocation *ddb,int plane) { struct drm_crtc *crtc = &intel_crtc->base; @@ -3859,19 +3817,21 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc, enum pipe pipe = intel_crtc->pipe; for (level = 0; level <= max_level; level++) {
I915_WRITE(PLANE_WM(pipe, plane, level),wm->plane[pipe][plane][level]);
skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane,level),
&wm->wm[level]);}
- I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm-
plane_trans[pipe][plane]);
- skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane),
&wm->trans_wm);skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
&wm->ddb.plane[pipe][plane]);
&ddb->plane[pipe][plane]);skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
&wm->ddb.y_plane[pipe][plane]);
&ddb->y_plane[pipe][plane]);} void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
const struct skl_wm_values *wm)
const struct skl_plane_wm *wm,const struct skl_ddb_allocation *ddb){ struct drm_crtc *crtc = &intel_crtc->base; struct drm_device *dev = crtc->dev; @@ -3880,13 +3840,13 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc, enum pipe pipe = intel_crtc->pipe; for (level = 0; level <= max_level; level++) {
I915_WRITE(CUR_WM(pipe, level),wm->plane[pipe][PLANE_CURSOR][level]);
skl_write_wm_level(dev_priv, CUR_WM(pipe, level),&wm->wm[level]);}
- I915_WRITE(CUR_WM_TRANS(pipe), wm-
plane_trans[pipe][PLANE_CURSOR]);
- skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
trans_wm);
skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
&wm->ddb.plane[pipe][PLANE_CURSOR]);
&ddb->plane[pipe][PLANE_CURSOR]);} static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a, @@ -4064,11 +4024,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, struct skl_wm_values *src, enum pipe pipe) {
- memcpy(dst->plane[pipe], src->plane[pipe],
- sizeof(dst->plane[pipe]));
- memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
- sizeof(dst->plane_trans[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], @@ -4117,7 +4072,6 @@ skl_compute_wm(struct drm_atomic_state *state) * no suitable watermark values can be found. */ for_each_crtc_in_state(state, crtc, cstate, i) {
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate); @@ -4135,7 +4089,6 @@ skl_compute_wm(struct drm_atomic_state *state) continue; intel_cstate->update_wm_pre = true;
skl_compute_wm_results(crtc->dev, pipe_wm, results,intel_crtc); } return 0; @@ -4169,9 +4122,11 @@ static void skl_update_wm(struct drm_crtc *crtc) int plane; for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
skl_write_plane_wm(intel_crtc, results,plane);
skl_write_plane_wm(intel_crtc, &pipe_wm-planes[plane],
&results->ddb, plane);
skl_write_cursor_wm(intel_crtc, results);
skl_write_cursor_wm(intel_crtc, &pipe_wm-planes[PLANE_CURSOR],
&results->ddb);} skl_copy_wm_for_pipe(hw_vals, results, pipe); @@ -4256,28 +4211,13 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) mutex_unlock(&dev_priv->wm.wm_mutex); } -static void skl_pipe_wm_active_state(uint32_t val,
struct skl_pipe_wm *active,bool is_transwm,int i,int level)+static inline void skl_wm_level_from_reg_val(uint32_t val,
struct skl_wm_level*level) {
- struct skl_plane_wm *plane_wm = &active->planes[i];
- bool is_enabled = (val & PLANE_WM_EN) != 0;
- if (!is_transwm) {
plane_wm->wm[level].plane_en = is_enabled;plane_wm->wm[level].plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->wm[level].plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;- } else {
plane_wm->trans_wm.plane_en = is_enabled;plane_wm->trans_wm.plane_res_b = val &PLANE_WM_BLOCKS_MASK;
plane_wm->trans_wm.plane_res_l =(val >> PLANE_WM_LINES_SHIFT) &PLANE_WM_LINES_MASK;- }
- level->plane_en = val & PLANE_WM_EN;
- level->plane_res_b = val & PLANE_WM_BLOCKS_MASK;
- level->plane_res_l = (val & PLANE_WM_LINES_MASK) >>
PLANE_WM_LINES_SHIFT;
This also looks wrong, since PLANE_WM_LINES_MASK is 0x1f. We should do like the original code did: shifting before masking.
} static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) @@ -4287,23 +4227,33 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) struct skl_wm_values *hw = &dev_priv->wm.skl_hw; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
state);
- struct intel_plane *intel_plane;
struct skl_pipe_wm *active = &cstate->wm.skl.optimal;
- struct skl_plane_wm *wm;
enum pipe pipe = intel_crtc->pipe;
- int level, i, max_level;
- uint32_t temp;
- int level, id, max_level = ilk_wm_max_level(dev);
- uint32_t val;
- max_level = ilk_wm_max_level(dev);
- for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
id = skl_wm_plane_id(intel_plane);wm = &cstate->wm.skl.optimal.planes[id];
- for (level = 0; level <= max_level; level++) {
for (i = 0; i < intel_num_planes(intel_crtc); i++)hw->plane[pipe][i][level] =I915_READ(PLANE_WM(pipe, i,level));
hw->plane[pipe][PLANE_CURSOR][level] =I915_READ(CUR_WM(pipe, level));
- }
for (level = 0; level <= max_level; level++) {if (id != PLANE_CURSOR)val = I915_READ(PLANE_WM(pipe, id,level));
elseval = I915_READ(CUR_WM(pipe,level));
skl_wm_level_from_reg_val(val, &wm-wm[level]);
}
- for (i = 0; i < intel_num_planes(intel_crtc); i++)
hw->plane_trans[pipe][i] =I915_READ(PLANE_WM_TRANS(pipe, i));
- hw->plane_trans[pipe][PLANE_CURSOR] =
I915_READ(CUR_WM_TRANS(pipe));
if (id != PLANE_CURSOR)val = I915_READ(PLANE_WM_TRANS(pipe, id));elseval = I915_READ(CUR_WM_TRANS(pipe));skl_wm_level_from_reg_val(val, &wm->trans_wm);- }
if (!intel_crtc->active) return; @@ -4311,25 +4261,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) hw->dirty_pipes |= drm_crtc_mask(crtc); active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
- for (level = 0; level <= max_level; level++) {
for (i = 0; i < intel_num_planes(intel_crtc); i++) {temp = hw->plane[pipe][i][level];skl_pipe_wm_active_state(temp, active,false, i, level);
}temp = hw->plane[pipe][PLANE_CURSOR][level];skl_pipe_wm_active_state(temp, active, false, i,level);
- }
- for (i = 0; i < intel_num_planes(intel_crtc); i++) {
temp = hw->plane_trans[pipe][i];skl_pipe_wm_active_state(temp, active, true, i, 0);- }
- temp = hw->plane_trans[pipe][PLANE_CURSOR];
- skl_pipe_wm_active_state(temp, active, true, i, 0);
- intel_crtc->wm.active.skl = *active;
As far as I understood, we should still be setting intel_crtc-
wm.active.skl. Shouldn't we? If not, why?
Now, due to the problems above, weren't you getting some WARNs about the hw state not matching what it should? In case not, maybe you could investigate why.
} void skl_wm_get_hw_state(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 73a521f..0fb775b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -208,6 +208,8 @@ skl_update_plane(struct drm_plane *drm_plane, 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; @@ -232,7 +234,7 @@ 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, wm, plane);
skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,plane); if (key->flags) { I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
min_value);
@@ -289,6 +291,7 @@ 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; @@ -298,7 +301,8 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) */ if (!dplane->state->visible) skl_write_plane_wm(to_intel_crtc(crtc),
&dev_priv->wm.skl_results,plane);
&cstate-wm.skl.optimal.planes[plane],
&dev_priv->wm.skl_results.ddb,plane); I915_WRITE(PLANE_CTL(pipe, plane), 0);
On Wed, Oct 05, 2016 at 06:44:04PM -0300, Paulo Zanoni wrote:
Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd15ae2..c580d3d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane, 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];I wish someone would do a patch to convert all these hardcoded values to PLANE_X, and convert "int"s to "enum plane"s everywhere.
Note that this is not PLANE_A, but setting up a shorthand local for
const struct skl_plane_wm *p_wm = crtc_state->wm.skl.optimal.planes;
-Chris
Finally, add some debugging output for ddb changes in the atomic debug output. This makes it a lot easier to spot bugs from incorrect ddb allocations.
Signed-off-by: Lyude cpaul@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7708646..2691428 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4030,6 +4030,61 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, sizeof(dst->ddb.plane[pipe])); }
+static void +skl_print_wm_changes(const struct drm_atomic_state *state) +{ + const struct drm_device *dev = state->dev; + const struct drm_i915_private *dev_priv = to_i915(dev); + const struct intel_atomic_state *intel_state = + to_intel_atomic_state(state); + const struct drm_crtc *crtc; + const struct drm_crtc_state *cstate; + const struct drm_plane *plane; + const struct intel_plane *intel_plane; + const struct drm_plane_state *pstate; + const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; + const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; + enum pipe pipe; + int id; + int i, j; + + for_each_crtc_in_state(state, crtc, cstate, i) { + if (!crtc->state) + continue; + + pipe = to_intel_crtc(crtc)->pipe; + + for_each_plane_in_state(state, plane, pstate, j) { + const struct skl_ddb_entry *old, *new; + + intel_plane = to_intel_plane(plane); + id = skl_wm_plane_id(intel_plane); + old = &old_ddb->plane[pipe][id]; + new = &new_ddb->plane[pipe][id]; + + if (intel_plane->pipe != pipe) + continue; + + if (skl_ddb_entry_equal(old, new)) + continue; + + if (id != PLANE_CURSOR) { + DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n", + plane->base.id, id + 1, + pipe_name(pipe), + old->start, old->end, + new->start, new->end); + } else { + DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n", + plane->base.id, + pipe_name(pipe), + old->start, old->end, + new->start, new->end); + } + } + } +} + static int skl_compute_wm(struct drm_atomic_state *state) { @@ -4091,6 +4146,8 @@ skl_compute_wm(struct drm_atomic_state *state) intel_cstate->update_wm_pre = true; }
+ skl_print_wm_changes(state); + return 0; }
Op 05-10-16 om 17:33 schreef Lyude:
While it (mostly) works, the code for handling watermarks on Skylake has been kind of ugly for a while. As well a lot of it isn't that friendly to atomic transactions, Lots of copy paste, redundant wm values, etc. While this isn't a full cleanup, it's a good start. As well, we add a couple of features for making debugging watermarks a little easier.
It's a good start, with the review comments addressed and rebased I think it's good to go. :)
Feel free to add my r-b.
~Maarten
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Lyude -
Maarten Lankhorst -
Paulo Zanoni