2016-10-14 21:32:24

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 00/10] Start of skl watermark cleanup

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.

Rebased for latest nightly, new r-bs added + some changes

Lyude (10):
drm/i915/skl: Move per-pipe ddb allocations into crtc states
drm/i915/skl: Remove linetime from skl_wm_values
drm/i915/gen9: Make skl_wm_level per-plane
drm/i915/gen9: Cleanup skl_pipe_wm_active_state
drm/i915/gen9: Get rid of redundant watermark values
drm/i915/gen9: Add ddb changes to atomic debug output
drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable
drm/i915/gen9: Add skl_wm_level_equals()
drm/i915/gen9: Actually verify WM levels in verify_wm_state()
drm/i915/gen9: Don't wrap strings in verify_wm_state()

drivers/gpu/drm/i915/i915_drv.h | 10 +-
drivers/gpu/drm/i915/intel_display.c | 138 ++++++++---
drivers/gpu/drm/i915/intel_drv.h | 24 +-
drivers/gpu/drm/i915/intel_pm.c | 466 +++++++++++++++++------------------
drivers/gpu/drm/i915/intel_sprite.c | 8 +-
5 files changed, 355 insertions(+), 291 deletions(-)

--
2.7.4


2016-10-14 21:32:30

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v2 07/10] drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable

There's not much of a reason this should have the locations to read out
the hardware state hardcoded, so allow the caller to specify the
location and add this function to intel_drv.h. As well, we're going to
need this function to be reusable for the next patch.

Changes since v1:
- Fix accidental behavior change in the code that Paulo pointed out

Signed-off-by: Lyude <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++----------
2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a85ce2c..7036310 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1756,6 +1756,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
void skl_wm_get_hw_state(struct drm_device *dev);
void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
struct skl_ddb_allocation *ddb /* out */);
+void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
+ struct skl_pipe_wm *out);
bool intel_can_enable_sagv(struct drm_atomic_state *state);
int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2fe851e..6eaeb87 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4288,15 +4288,13 @@ static inline void skl_wm_level_from_reg_val(uint32_t val,
PLANE_WM_LINES_MASK;
}

-static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
+void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
+ struct skl_pipe_wm *out)
{
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
- 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, id, max_level;
@@ -4306,7 +4304,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)

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];
+ wm = &out->planes[id];

for (level = 0; level <= max_level; level++) {
if (id != PLANE_CURSOR)
@@ -4328,20 +4326,30 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
if (!intel_crtc->active)
return;

- hw->dirty_pipes |= drm_crtc_mask(crtc);
- active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
- intel_crtc->wm.active.skl = *active;
+ out->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
}

void skl_wm_get_hw_state(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
+ struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb;
struct drm_crtc *crtc;
+ struct intel_crtc *intel_crtc;
+ struct intel_crtc_state *cstate;

skl_ddb_get_hw_state(dev_priv, ddb);
- list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
- skl_pipe_wm_get_hw_state(crtc);
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ intel_crtc = to_intel_crtc(crtc);
+ cstate = to_intel_crtc_state(crtc->state);
+
+ skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
+
+ if (intel_crtc->active) {
+ hw->dirty_pipes |= drm_crtc_mask(crtc);
+ intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
+ }
+ }

if (dev_priv->active_crtcs) {
/* Fully recompute DDB on first atomic commit */
--
2.7.4

2016-10-14 21:34:04

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v2 10/10] drm/i915/gen9: Don't wrap strings in verify_wm_state()

Wrapping strings is against the guidelines in Documentation/CodingStyle,
chapter 2.

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca12f0e..eaec9f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13509,8 +13509,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
sw_ddb_entry = &sw_ddb->plane[pipe][plane];

if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
- DRM_ERROR("mismatch in DDB state pipe %c plane %d "
- "(expected (%u,%u), found (%u,%u))\n",
+ DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
pipe_name(pipe), plane + 1,
sw_ddb_entry->start, sw_ddb_entry->end,
hw_ddb_entry->start, hw_ddb_entry->end);
@@ -13560,8 +13559,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];

if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
- DRM_ERROR("mismatch in DDB state pipe %c cursor "
- "(expected (%u,%u), found (%u,%u))\n",
+ DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
pipe_name(pipe),
sw_ddb_entry->start, sw_ddb_entry->end,
hw_ddb_entry->start, hw_ddb_entry->end);
--
2.7.4

2016-10-14 21:34:23

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v2 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()

Thanks to Paulo Zanoni for indirectly pointing this out.

Looks like we never actually added any code for checking whether or not
we actually wrote watermark levels properly. Let's fix that.

Changes since v1:
- Use %u instead of %d when printing WM state mismatches

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 100 +++++++++++++++++++++++++++++------
1 file changed, 84 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 18e82b4..ca12f0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13455,30 +13455,66 @@ static void verify_wm_state(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct skl_ddb_allocation hw_ddb, *sw_ddb;
- struct skl_ddb_entry *hw_entry, *sw_entry;
+ struct skl_pipe_wm hw_wm, *sw_wm;
+ struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
+ struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
const enum pipe pipe = intel_crtc->pipe;
- int plane;
+ int plane, level, max_level = ilk_wm_max_level(dev_priv);

if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
return;

+ skl_pipe_wm_get_hw_state(crtc, &hw_wm);
+ sw_wm = &intel_crtc->wm.active.skl;
+
skl_ddb_get_hw_state(dev_priv, &hw_ddb);
sw_ddb = &dev_priv->wm.skl_hw.ddb;

/* planes */
for_each_plane(dev_priv, pipe, plane) {
- hw_entry = &hw_ddb.plane[pipe][plane];
- sw_entry = &sw_ddb->plane[pipe][plane];
+ hw_plane_wm = &hw_wm.planes[plane];
+ sw_plane_wm = &sw_wm->planes[plane];

- if (skl_ddb_entry_equal(hw_entry, sw_entry))
- continue;
+ /* Watermarks */
+ for (level = 0; level <= max_level; level++) {
+ if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+ &sw_plane_wm->wm[level]))
+ continue;
+
+ DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+ pipe_name(pipe), plane + 1, level,
+ sw_plane_wm->wm[level].plane_en,
+ sw_plane_wm->wm[level].plane_res_b,
+ sw_plane_wm->wm[level].plane_res_l,
+ hw_plane_wm->wm[level].plane_en,
+ hw_plane_wm->wm[level].plane_res_b,
+ hw_plane_wm->wm[level].plane_res_l);
+ }

- DRM_ERROR("mismatch in DDB state pipe %c plane %d "
- "(expected (%u,%u), found (%u,%u))\n",
- pipe_name(pipe), plane + 1,
- sw_entry->start, sw_entry->end,
- hw_entry->start, hw_entry->end);
+ if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+ &sw_plane_wm->trans_wm)) {
+ DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+ pipe_name(pipe), plane + 1,
+ sw_plane_wm->trans_wm.plane_en,
+ sw_plane_wm->trans_wm.plane_res_b,
+ sw_plane_wm->trans_wm.plane_res_l,
+ hw_plane_wm->trans_wm.plane_en,
+ hw_plane_wm->trans_wm.plane_res_b,
+ hw_plane_wm->trans_wm.plane_res_l);
+ }
+
+ /* DDB */
+ hw_ddb_entry = &hw_ddb.plane[pipe][plane];
+ sw_ddb_entry = &sw_ddb->plane[pipe][plane];
+
+ if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
+ DRM_ERROR("mismatch in DDB state pipe %c plane %d "
+ "(expected (%u,%u), found (%u,%u))\n",
+ pipe_name(pipe), plane + 1,
+ sw_ddb_entry->start, sw_ddb_entry->end,
+ hw_ddb_entry->start, hw_ddb_entry->end);
+ }
}

/*
@@ -13488,15 +13524,47 @@ static void verify_wm_state(struct drm_crtc *crtc,
* once the plane becomes visible, we can skip this check
*/
if (intel_crtc->cursor_addr) {
- hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
- sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+ hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
+ sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
+
+ /* Watermarks */
+ for (level = 0; level <= max_level; level++) {
+ if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+ &sw_plane_wm->wm[level]))
+ continue;
+
+ DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+ pipe_name(pipe), level,
+ sw_plane_wm->wm[level].plane_en,
+ sw_plane_wm->wm[level].plane_res_b,
+ sw_plane_wm->wm[level].plane_res_l,
+ hw_plane_wm->wm[level].plane_en,
+ hw_plane_wm->wm[level].plane_res_b,
+ hw_plane_wm->wm[level].plane_res_l);
+ }
+
+ if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+ &sw_plane_wm->trans_wm)) {
+ DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%u l=%u, got e=%d b=%u l=%u)\n",
+ pipe_name(pipe),
+ sw_plane_wm->trans_wm.plane_en,
+ sw_plane_wm->trans_wm.plane_res_b,
+ sw_plane_wm->trans_wm.plane_res_l,
+ hw_plane_wm->trans_wm.plane_en,
+ hw_plane_wm->trans_wm.plane_res_b,
+ hw_plane_wm->trans_wm.plane_res_l);
+ }
+
+ /* DDB */
+ hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+ sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];

- if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+ if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
DRM_ERROR("mismatch in DDB state pipe %c cursor "
"(expected (%u,%u), found (%u,%u))\n",
pipe_name(pipe),
- sw_entry->start, sw_entry->end,
- hw_entry->start, hw_entry->end);
+ sw_ddb_entry->start, sw_ddb_entry->end,
+ hw_ddb_entry->start, hw_ddb_entry->end);
}
}
}
--
2.7.4

2016-10-14 21:32:29

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 06/10] drm/i915/gen9: Add ddb changes to atomic debug output

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 <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_pm.c | 54 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5df5cea..2fe851e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4044,6 +4044,58 @@ 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) {
+ 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)
{
@@ -4105,6 +4157,8 @@ skl_compute_wm(struct drm_atomic_state *state)
intel_cstate->update_wm_pre = true;
}

+ skl_print_wm_changes(state);
+
return 0;
}

--
2.7.4

2016-10-14 21:36:17

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v2 08/10] drm/i915/gen9: Add skl_wm_level_equals()

Helper we're going to be using for implementing verification of the wm
levels in skl_verify_wm_level().

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 14 ++++++++++++++
2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7036310..96963ea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1761,6 +1761,8 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
bool intel_can_enable_sagv(struct drm_atomic_state *state);
int intel_enable_sagv(struct drm_i915_private *dev_priv);
int intel_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_wm_level_equals(const struct skl_wm_level *l1,
+ const struct skl_wm_level *l2);
bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
const struct skl_ddb_allocation *new,
enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6eaeb87..fae3ce4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3857,6 +3857,20 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
&ddb->plane[pipe][PLANE_CURSOR]);
}

+bool skl_wm_level_equals(const struct skl_wm_level *l1,
+ const struct skl_wm_level *l2)
+{
+ if (l1->plane_en != l2->plane_en)
+ return false;
+
+ /* If both planes aren't enabled, the rest shouldn't matter */
+ if (!l1->plane_en)
+ return true;
+
+ return (l1->plane_res_l == l2->plane_res_l &&
+ l1->plane_res_b == l2->plane_res_b);
+}
+
static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
const struct skl_ddb_entry *b)
{
--
2.7.4

2016-10-14 21:32:27

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 01/10] drm/i915/skl: Move per-pipe ddb allocations into crtc states

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.

Changes since v1:
- Don't replace alloc->start = alloc->end = 0;

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
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 fe875b2..ac4287f99 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1641,7 +1641,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 60c699e..7a68cc3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14246,12 +14246,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;
@@ -14269,12 +14268,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;
@@ -14285,7 +14286,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 5bc1154..10a0cf2 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 48b3904..66586af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3062,7 +3062,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;
@@ -3090,7 +3089,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;
}

@@ -3358,7 +3357,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;
@@ -3374,7 +3373,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;
+ alloc->start = alloc->end = 0;
return 0;
}

@@ -3901,14 +3900,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)
{
@@ -3916,22 +3907,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;
}

@@ -4096,7 +4087,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],
@@ -4204,6 +4194,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);
}

--
2.7.4

2016-10-14 21:38:10

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v2 04/10] drm/i915/gen9: Cleanup skl_pipe_wm_active_state

This function is a wreck, let's help it get its life back together and
cleanup all of the copy pasta here.

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++------------------------------
1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1bdccc9..2d63a37 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4271,46 +4271,22 @@ 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->planes[i].wm[level].plane_en = is_enabled;
- active->planes[i].wm[level].plane_res_b =
- val & PLANE_WM_BLOCKS_MASK;
- active->planes[i].wm[level].plane_res_l =
- (val >> PLANE_WM_LINES_SHIFT) &
- PLANE_WM_LINES_MASK;
- } else {
- active->planes[PLANE_CURSOR].wm[level].plane_en =
- is_enabled;
- active->planes[PLANE_CURSOR].wm[level].plane_res_b =
- val & PLANE_WM_BLOCKS_MASK;
- active->planes[PLANE_CURSOR].wm[level].plane_res_l =
- (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->planes[i].trans_wm.plane_en = is_enabled;
- active->planes[i].trans_wm.plane_res_b =
- val & PLANE_WM_BLOCKS_MASK;
- active->planes[i].trans_wm.plane_res_l =
- (val >> PLANE_WM_LINES_SHIFT) &
- PLANE_WM_LINES_MASK;
- } else {
- active->planes[PLANE_CURSOR].trans_wm.plane_en =
- is_enabled;
- active->planes[PLANE_CURSOR].trans_wm.plane_res_b =
- val & PLANE_WM_BLOCKS_MASK;
- active->planes[PLANE_CURSOR].trans_wm.plane_res_l =
- (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;
}
}

@@ -4349,20 +4325,20 @@ 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, PLANE_CURSOR,
+ 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, PLANE_CURSOR, 0);

intel_crtc->wm.active.skl = *active;
}
--
2.7.4

2016-10-14 21:38:08

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 05/10] drm/i915/gen9: Get rid of redundant watermark values

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl
Changes since v2:
- Fix usage of wrong cstate

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
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 | 204 ++++++++++++-----------------------
drivers/gpu/drm/i915/intel_sprite.c | 8 +-
5 files changed, 91 insertions(+), 143 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4d1133f..929b643 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1648,8 +1648,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 ced70ad..18e82b4 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);
@@ -10826,12 +10831,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 84301d3..a85ce2c 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 2d63a37..5df5cea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3004,6 +3004,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;

@@ -3024,19 +3026,22 @@ 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 = to_intel_crtc_state(crtc->state);

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_priv);
- intel_state->wm_results.plane[pipe][plane][level] == 0; --level)
- { }
+ !wm->wm[level].plane_en;
+ --level) { }

/*
* If any of the planes on this pipe don't enable wm levels
@@ -3783,67 +3788,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(to_i915(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)
@@ -3854,8 +3798,24 @@ 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;
+
+ 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;
@@ -3865,19 +3825,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;
@@ -3886,13 +3848,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,
@@ -4076,11 +4038,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],
@@ -4129,7 +4086,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);

@@ -4147,7 +4103,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;
@@ -4181,9 +4136,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);
@@ -4268,26 +4225,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_SHIFT) &
+ PLANE_WM_LINES_MASK;
}

static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
@@ -4297,49 +4241,41 @@ 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;
+ uint32_t val;

max_level = ilk_wm_max_level(dev_priv);

- 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 (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 (!intel_crtc->active)
- return;
-
- hw->dirty_pipes |= drm_crtc_mask(crtc);
+ 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];

- active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
+ 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));

- 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);
+ skl_wm_level_from_reg_val(val, &wm->wm[level]);
}
- temp = hw->plane[pipe][PLANE_CURSOR][level];
- skl_pipe_wm_active_state(temp, active, false, PLANE_CURSOR,
- 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);
+ 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);
}

- temp = hw->plane_trans[pipe][PLANE_CURSOR];
- skl_pipe_wm_active_state(temp, active, true, PLANE_CURSOR, 0);
+ if (!intel_crtc->active)
+ return;

+ hw->dirty_pipes |= drm_crtc_mask(crtc);
+ active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
intel_crtc->wm.active.skl = *active;
}

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c7d9a20..e7d2cff 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);

--
2.7.4

2016-10-14 21:40:08

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 02/10] drm/i915/skl: Remove linetime from skl_wm_values

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 <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
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 ac4287f99..e56d4a4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1648,7 +1648,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 7a68cc3..ced70ad 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14844,6 +14844,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);
@@ -14860,13 +14862,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 66586af..a7d5721 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3845,8 +3845,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,
@@ -4081,7 +4079,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],
@@ -4332,8 +4329,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)

max_level = ilk_wm_max_level(dev_priv);

- 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] =
@@ -4350,7 +4345,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++) {
--
2.7.4

2016-10-14 21:40:07

by Lyude Paul

[permalink] [raw]
Subject: [PATCH v3 03/10] drm/i915/gen9: Make skl_wm_level per-plane

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.

Changes since v1:
- Style nitpicks
- Fix accidental usage of i vs. PLANE_CURSOR
- Split out skl_pipe_wm_active_state simplification into seperate patch

Signed-off-by: Lyude <[email protected]>
Reviewed-by: Paulo Zanoni <[email protected]>
Reviewed-by: Maarten Lankhorst <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Matt Roper <[email protected]
Cc: Paulo Zanoni <[email protected]>
---
drivers/gpu/drm/i915/i915_drv.h | 6 +-
drivers/gpu/drm/i915/intel_drv.h | 6 +-
drivers/gpu/drm/i915/intel_pm.c | 207 +++++++++++++++++++--------------------
3 files changed, 111 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e56d4a4..4d1133f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1653,9 +1653,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 10a0cf2..84301d3 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 a7d5721..1bdccc9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3674,67 +3674,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));
+ if (!intel_pstate)
+ intel_pstate = to_intel_plane_state(plane->state);

- 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);
+ WARN_ON(!intel_pstate->base.fb);

- /*
- * 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);
+ ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);

- WARN_ON(!intel_pstate->base.fb);
-
- 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;
}
@@ -3755,19 +3740,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,
@@ -3776,19 +3753,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_priv);
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;
}

@@ -3798,50 +3789,56 @@ static void skl_compute_wm_results(struct drm_device *dev,
struct intel_crtc *intel_crtc)
{
int level, max_level = ilk_wm_max_level(to_i915(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;
@@ -4282,35 +4279,37 @@ static void skl_pipe_wm_active_state(uint32_t val,

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;
+ active->planes[i].wm[level].plane_en = is_enabled;
+ active->planes[i].wm[level].plane_res_b =
+ val & PLANE_WM_BLOCKS_MASK;
+ active->planes[i].wm[level].plane_res_l =
+ (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;
+ active->planes[PLANE_CURSOR].wm[level].plane_en =
+ is_enabled;
+ active->planes[PLANE_CURSOR].wm[level].plane_res_b =
+ val & PLANE_WM_BLOCKS_MASK;
+ active->planes[PLANE_CURSOR].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;
+ active->planes[i].trans_wm.plane_en = is_enabled;
+ active->planes[i].trans_wm.plane_res_b =
+ val & PLANE_WM_BLOCKS_MASK;
+ active->planes[i].trans_wm.plane_res_l =
+ (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;
+ active->planes[PLANE_CURSOR].trans_wm.plane_en =
+ is_enabled;
+ active->planes[PLANE_CURSOR].trans_wm.plane_res_b =
+ val & PLANE_WM_BLOCKS_MASK;
+ active->planes[PLANE_CURSOR].trans_wm.plane_res_l =
+ (val >> PLANE_WM_LINES_SHIFT) &
+ PLANE_WM_LINES_MASK;
}
}
}
--
2.7.4

2016-10-16 23:21:15

by Lyude Paul

[permalink] [raw]
Subject: Re: [PATCH v3 00/10] Start of skl watermark cleanup

Pushed patches 1-4 in this series (with some very small style changes
to make checkpatch happy), drm-intel-nightly also rebuilt.

On Fri, 2016-10-14 at 17:31 -0400, Lyude wrote:
> 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.
>
> Rebased for latest nightly, new r-bs added + some changes
>
> Lyude (10):
>   drm/i915/skl: Move per-pipe ddb allocations into crtc states
>   drm/i915/skl: Remove linetime from skl_wm_values
>   drm/i915/gen9: Make skl_wm_level per-plane
>   drm/i915/gen9: Cleanup skl_pipe_wm_active_state
>   drm/i915/gen9: Get rid of redundant watermark values
>   drm/i915/gen9: Add ddb changes to atomic debug output
>   drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable
>   drm/i915/gen9: Add skl_wm_level_equals()
>   drm/i915/gen9: Actually verify WM levels in verify_wm_state()
>   drm/i915/gen9: Don't wrap strings in verify_wm_state()
>
>  drivers/gpu/drm/i915/i915_drv.h      |  10 +-
>  drivers/gpu/drm/i915/intel_display.c | 138 ++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  24 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 466 +++++++++++++++++------
> ------------
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>  5 files changed, 355 insertions(+), 291 deletions(-)
>
--
Cheers,
Lyude

2016-10-18 18:12:41

by Zanoni, Paulo R

[permalink] [raw]
Subject: Re: [PATCH v2 07/10] drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable

Em Sex, 2016-10-14 às 17:31 -0400, Lyude escreveu:
> There's not much of a reason this should have the locations to read
> out
> the hardware state hardcoded, so allow the caller to specify the
> location and add this function to intel_drv.h. As well, we're going
> to
> need this function to be reusable for the next patch.
>
> Changes since v1:
> - Fix accidental behavior change in the code that Paulo pointed out

Reviewed-by: Paulo Zanoni <[email protected]>

I just submitted v4 of patch 5 solving the conflicts I created. With
that + this review, we can merge this series. If you give me an ack on
patch 5 I can just go and merge these, so we can move to Maarten's
series and then later to Mahesh's series.

>
> Signed-off-by: Lyude <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>
> Cc: Ville Syrjälä <[email protected]>
> Cc: Matt Roper <[email protected]
> Cc: Paulo Zanoni <[email protected]>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 28 ++++++++++++++++++----------
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a85ce2c..7036310 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1756,6 +1756,8 @@ void ilk_wm_get_hw_state(struct drm_device
> *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>     struct skl_ddb_allocation *ddb /* out */);
> +void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> +       struct skl_pipe_wm *out);
>  bool intel_can_enable_sagv(struct drm_atomic_state *state);
>  int intel_enable_sagv(struct drm_i915_private *dev_priv);
>  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 2fe851e..6eaeb87 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4288,15 +4288,13 @@ static inline void
> skl_wm_level_from_reg_val(uint32_t val,
>   PLANE_WM_LINES_MASK;
>  }
>  
> -static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> +void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> +       struct skl_pipe_wm *out)
>  {
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
> - 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, id, max_level;
> @@ -4306,7 +4304,7 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  
>   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];
> + wm = &out->planes[id];
>  
>   for (level = 0; level <= max_level; level++) {
>   if (id != PLANE_CURSOR)
> @@ -4328,20 +4326,30 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>   if (!intel_crtc->active)
>   return;
>  
> - hw->dirty_pipes |= drm_crtc_mask(crtc);
> - active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
> - intel_crtc->wm.active.skl = *active;
> + out->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
>  }
>  
>  void skl_wm_get_hw_state(struct drm_device *dev)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
> + struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
>   struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb;
>   struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct intel_crtc_state *cstate;
>  
>   skl_ddb_get_hw_state(dev_priv, ddb);
> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> - skl_pipe_wm_get_hw_state(crtc);
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> {
> + intel_crtc = to_intel_crtc(crtc);
> + cstate = to_intel_crtc_state(crtc->state);
> +
> + skl_pipe_wm_get_hw_state(crtc, &cstate-
> >wm.skl.optimal);
> +
> + if (intel_crtc->active) {
> + hw->dirty_pipes |= drm_crtc_mask(crtc);
> + intel_crtc->wm.active.skl = cstate-
> >wm.skl.optimal;
> + }
> + }
>  
>   if (dev_priv->active_crtcs) {
>   /* Fully recompute DDB on first atomic commit */