All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes.
@ 2016-10-26 13:41 Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2 Maarten Lankhorst
                   ` (11 more replies)
  0 siblings, 12 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

They clean up the remainder of SKL style wm's, and finally makes
SKL watermarks ready for nonblocking modeset by using the crtc_state
for watermarks as much as possible.

The atomic watermark state for skylake is cleaned up to only contain the
minimum required, and unnecessary copies are removed as much as possible.

Maarten Lankhorst (11):
  drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  drm/i915/gen9+: Use cstate plane mask instead of crtc->state.
  drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
  drm/i915/skl+: Remove data_rate from watermark struct, v2.
  drm/i915/skl+: Remove minimum block allocation from crtc state.
  drm/i915/skl+: Clean up minimum allocations, v2.
  drm/i915: Add a atomic evasion step to watermark programming, v2.
  drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2.
  drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
  drm/i915/gen9+: Preserve old allocation from crtc_state.
  drm/i915/gen9+: Kill off hw_ddb from intel_crtc.

 drivers/gpu/drm/i915/i915_drv.h      |  13 +-
 drivers/gpu/drm/i915/intel_display.c |  77 +++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  27 +---
 drivers/gpu/drm/i915/intel_pm.c      | 232 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_sprite.c  |  18 ---
 5 files changed, 157 insertions(+), 210 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-27  7:12   ` Daniel Vetter
  2016-10-26 13:41 ` [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state Maarten Lankhorst
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

Changes since v1:
- Remove plane->pipe checks, they're implied by the macros.
- Split unrelated changes to a separate commit.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9e0e8741253e..044a8c932a1c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,6 +31,7 @@
 #include "intel_drv.h"
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
+#include <drm/drm_atomic_helper.h>
 
 /**
  * DOC: RC6
@@ -3269,24 +3270,20 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 	struct drm_crtc *crtc = cstate->crtc;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_plane *plane;
+	struct drm_plane *plane;
 	const struct intel_plane *intel_plane;
-	struct drm_plane_state *pstate;
+	const struct drm_plane_state *pstate;
 	unsigned int rate, total_data_rate = 0;
 	int id;
-	int i;
 
 	if (WARN_ON(!state))
 		return 0;
 
 	/* Calculate and cache data rate for each plane */
-	for_each_plane_in_state(state, plane, pstate, i) {
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
 		id = skl_wm_plane_id(to_intel_plane(plane));
 		intel_plane = to_intel_plane(plane);
 
-		if (intel_plane->pipe != intel_crtc->pipe)
-			continue;
-
 		/* packed/uv */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 0);
@@ -3383,7 +3380,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane;
 	struct drm_plane *plane;
-	struct drm_plane_state *pstate;
+	const struct drm_plane_state *pstate;
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
 	uint16_t alloc_size, start, cursor_blocks;
@@ -3419,14 +3416,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	alloc_size -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
-	for_each_plane_in_state(state, plane, pstate, i) {
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
 		intel_plane = to_intel_plane(plane);
 		id = skl_wm_plane_id(intel_plane);
 
-		if (intel_plane->pipe != pipe)
-			continue;
-
-		if (!to_intel_plane_state(pstate)->base.visible) {
+		if (!pstate->visible) {
 			minimum[id] = 0;
 			y_minimum[id] = 0;
 			continue;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-27 14:50   ` Matt Roper
  2016-10-26 13:41 ` [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes Maarten Lankhorst
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

I'm planning on getting rid of all obj->state dereferences,
and replace thhem with accessor functions.
Remove this one early, they're equivalent because removed
planes are already part of the state, else they could not
have been removed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 044a8c932a1c..bdb69582e7c5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3975,7 +3975,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
 
 	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
 
-	drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
+	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
 		id = skl_wm_plane_id(to_intel_plane(plane));
 
 		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2 Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-27 13:03   ` Ville Syrjälä
  2016-10-27 17:47   ` Matt Roper
  2016-10-26 13:41 ` [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2 Maarten Lankhorst
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

This will allow us to find all allocations that may have changed,
not just the one added by the atomic state.

This is required to stop adding planes to state when its
allocation changes, and is useful for finding bugs.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bdb69582e7c5..c1520afb2360 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4090,41 +4090,35 @@ skl_print_wm_changes(const struct drm_atomic_state *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;
+	int i;
 
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		pipe = to_intel_crtc(crtc)->pipe;
 
-		for_each_plane_in_state(state, plane, pstate, j) {
+		for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) {
 			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,
+						 intel_plane->base.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,
+						 intel_plane->base.base.id,
 						 pipe_name(pipe),
 						 old->start, old->end,
 						 new->start, new->end);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (2 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-27 18:14   ` Paulo Zanoni
  2016-10-26 13:41 ` [PATCH v2 05/11] drm/i915/skl+: Remove minimum block allocation from crtc state Maarten Lankhorst
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

It's only used in one function, and can be calculated without caching it
in the global struct by using drm_atomic_crtc_state_for_each_plane_state.

There are loops over all planes, including planes that don't exist.
This is harmless, because data_rate will always be 0 for them and we
never program them when updating watermarks.

Changes since v1:
- Rename rate back to data_rate, and change array name to
  plane_data_rate. (Matt)
- Remove whitespace. (Paulo)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ----
 drivers/gpu/drm/i915/intel_pm.c  | 35 ++++++++++++++++-------------------
 2 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7dda79df55d0..d0485457b335 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
 			struct skl_pipe_wm optimal;
 			struct skl_ddb_entry ddb;
 
-			/* cached plane data rate */
-			unsigned plane_data_rate[I915_MAX_PLANES];
-			unsigned plane_y_data_rate[I915_MAX_PLANES];
-
 			/* minimum block allocation */
 			uint16_t minimum_blocks[I915_MAX_PLANES];
 			uint16_t minimum_y_blocks[I915_MAX_PLANES];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c1520afb2360..0c199c51dad9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3263,13 +3263,12 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
  *   3 * 4096 * 8192  * 4 < 2^32
  */
 static unsigned int
-skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
+skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
+				 unsigned *plane_data_rate,
+				 unsigned *plane_y_data_rate)
 {
 	struct drm_crtc_state *cstate = &intel_cstate->base;
 	struct drm_atomic_state *state = cstate->state;
-	struct drm_crtc *crtc = cstate->crtc;
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane;
 	const struct intel_plane *intel_plane;
 	const struct drm_plane_state *pstate;
@@ -3287,21 +3286,16 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 		/* packed/uv */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 0);
-		intel_cstate->wm.skl.plane_data_rate[id] = rate;
+		plane_data_rate[id] = rate;
+
+		total_data_rate += rate;
 
 		/* y-plane */
 		rate = skl_plane_relative_data_rate(intel_cstate,
 						    pstate, 1);
-		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
-	}
+		plane_y_data_rate[id] = rate;
 
-	/* Calculate CRTC's total data rate from cached values */
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		int id = skl_wm_plane_id(intel_plane);
-
-		/* packed/uv */
-		total_data_rate += intel_cstate->wm.skl.plane_data_rate[id];
-		total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id];
+		total_data_rate += rate;
 	}
 
 	return total_data_rate;
@@ -3389,6 +3383,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	unsigned int total_data_rate;
 	int num_active;
 	int id, i;
+	unsigned plane_data_rate[I915_MAX_PLANES] = {};
+	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
 
 	/* Clear the partitioning for disabled planes. */
 	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -3446,17 +3442,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	 *
 	 * FIXME: we may not allocate every single block here.
 	 */
-	total_data_rate = skl_get_total_relative_data_rate(cstate);
+	total_data_rate = skl_get_total_relative_data_rate(cstate,
+							   plane_data_rate,
+							   plane_y_data_rate);
 	if (total_data_rate == 0)
 		return 0;
 
 	start = alloc->start;
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+	for (id = 0; id < I915_MAX_PLANES; id++) {
 		unsigned int data_rate, y_data_rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
-		int id = skl_wm_plane_id(intel_plane);
 
-		data_rate = cstate->wm.skl.plane_data_rate[id];
+		data_rate = plane_data_rate[id];
 
 		/*
 		 * allocation for (packed formats) or (uv-plane part of planar format):
@@ -3478,7 +3475,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		/*
 		 * allocation for y_plane part of planar format:
 		 */
-		y_data_rate = cstate->wm.skl.plane_y_data_rate[id];
+		y_data_rate = plane_y_data_rate[id];
 
 		y_plane_blocks = y_minimum[id];
 		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 05/11] drm/i915/skl+: Remove minimum block allocation from crtc state.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (3 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2 Maarten Lankhorst
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

This is not required any more now that we get fresh state from
drm_atomic_crtc_state_for_each_plane_state. Zero all state
in advance.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ----
 drivers/gpu/drm/i915/intel_pm.c  | 15 +++++----------
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d0485457b335..73be640fad36 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -501,10 +501,6 @@ struct intel_crtc_wm_state {
 			/* gen9+ only needs 1-step wm programming */
 			struct skl_pipe_wm optimal;
 			struct skl_ddb_entry ddb;
-
-			/* minimum block allocation */
-			uint16_t minimum_blocks[I915_MAX_PLANES];
-			uint16_t minimum_y_blocks[I915_MAX_PLANES];
 		} skl;
 	};
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0c199c51dad9..c2b965d36ead 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3378,8 +3378,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	enum pipe pipe = intel_crtc->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;
+	uint16_t minimum[I915_MAX_PLANES] = {};
+	uint16_t y_minimum[I915_MAX_PLANES] = {};
 	unsigned int total_data_rate;
 	int num_active;
 	int id, i;
@@ -3416,16 +3416,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		intel_plane = to_intel_plane(plane);
 		id = skl_wm_plane_id(intel_plane);
 
-		if (!pstate->visible) {
-			minimum[id] = 0;
-			y_minimum[id] = 0;
+		if (!pstate->visible)
 			continue;
-		}
-		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
-			minimum[id] = 0;
-			y_minimum[id] = 0;
+
+		if (plane->type == DRM_PLANE_TYPE_CURSOR)
 			continue;
-		}
 
 		minimum[id] = skl_ddb_min_alloc(pstate, 0);
 		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (4 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 05/11] drm/i915/skl+: Remove minimum block allocation from crtc state Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-27 19:25   ` Paulo Zanoni
  2016-10-26 13:41 ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Maarten Lankhorst
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

Move calculating minimum allocations to a helper, which cleans up the
code some more. The cursor is still allocated in advance because it
doesn't count towards data rate and should always be reserved.

changes since v1:
- Change comment to have a extra opening line. (Matt)
- Rebase to remove unused plane->pipe == pipe, handled by the iterator
  now. (Paulo)

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 62 +++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c2b965d36ead..41953cc41809 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3364,6 +3364,30 @@ skl_ddb_min_alloc(const struct drm_plane_state *pstate,
 	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) * min_scanlines/4 + 3;
 }
 
+static void
+skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
+		 uint16_t *minimum, uint16_t *y_minimum)
+{
+	const struct drm_plane_state *pstate;
+	struct drm_plane *plane;
+
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+		int id = skl_wm_plane_id(intel_plane);
+
+		if (id == PLANE_CURSOR)
+			continue;
+
+		if (!pstate->visible)
+			continue;
+
+		minimum[id] = skl_ddb_min_alloc(pstate, 0);
+		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
+	}
+
+	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
+}
+
 static int
 skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		      struct skl_ddb_allocation *ddb /* out */)
@@ -3372,12 +3396,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	struct drm_plane *plane;
-	const struct drm_plane_state *pstate;
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
-	uint16_t alloc_size, start, cursor_blocks;
+	uint16_t alloc_size, start;
 	uint16_t minimum[I915_MAX_PLANES] = {};
 	uint16_t y_minimum[I915_MAX_PLANES] = {};
 	unsigned int total_data_rate;
@@ -3405,32 +3426,22 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 	}
 
-	cursor_blocks = skl_cursor_allocation(num_active);
-	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - cursor_blocks;
-	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
-
-	alloc_size -= cursor_blocks;
-
-	/* 1. Allocate the mininum required blocks for each active plane */
-	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
-		intel_plane = to_intel_plane(plane);
-		id = skl_wm_plane_id(intel_plane);
-
-		if (!pstate->visible)
-			continue;
-
-		if (plane->type == DRM_PLANE_TYPE_CURSOR)
-			continue;
+	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
 
-		minimum[id] = skl_ddb_min_alloc(pstate, 0);
-		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
-	}
+	/*
+	 * 1. Allocate the mininum required blocks for each active plane
+	 * and allocate the cursor, it doesn't require extra allocation
+	 * proportional to the data rate.
+	 */
 
-	for (i = 0; i < PLANE_CURSOR; i++) {
+	for (i = 0; i < I915_MAX_PLANES; i++) {
 		alloc_size -= minimum[i];
 		alloc_size -= y_minimum[i];
 	}
 
+	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
+	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
+
 	/*
 	 * 2. Distribute the remaining space in proportion to the amount of
 	 * data each plane needs to fetch from memory.
@@ -3448,6 +3459,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		unsigned int data_rate, y_data_rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
 
+		if (id == PLANE_CURSOR)
+			continue;
+
 		data_rate = plane_data_rate[id];
 
 		/*
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (5 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 16:19   ` Ville Syrjälä
  2016-10-26 23:10   ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Matt Roper
  2016-10-26 13:41 ` [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2 Maarten Lankhorst
                   ` (4 subsequent siblings)
  11 siblings, 2 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

Allow the driver to write watermarks during atomic evasion.
This will make it possible to write the watermarks in a cleaner
way on gen9+.

intel_atomic_state is not used here yet, but will be used when
we program all watermarks as a separate step during evasion.

This also writes linetime all the time, while before it was only
done during plane updates. This looks like this could be a bugfix,
but I'm not sure what it affects.

Changes since v1:
- Add comment about atomic evasion to commit message.
- Unwrap I915_WRITE call. (Lyude)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++++-----------
 drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++++++++++--
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a621c74254e..7a477d6a486e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -485,6 +485,7 @@ struct sdvo_device_mapping {
 
 struct intel_connector;
 struct intel_encoder;
+struct intel_atomic_state;
 struct intel_crtc_state;
 struct intel_initial_plane_config;
 struct intel_crtc;
@@ -498,8 +499,9 @@ struct drm_i915_display_funcs {
 	int (*compute_intermediate_wm)(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
 				       struct intel_crtc_state *newstate);
-	void (*initial_watermarks)(struct intel_crtc_state *cstate);
-	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
+	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
+	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
+	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
 	int (*compute_global_watermarks)(struct drm_atomic_state *state);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ea9a3bc0bc0..48eb0635ec0f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5170,7 +5170,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	 * us to.
 	 */
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else if (pipe_config->update_wm_pre)
 		intel_update_watermarks(&crtc->base);
 }
@@ -5384,7 +5384,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_color_load_luts(&pipe_config->base);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(intel_crtc->config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
 	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -5490,7 +5490,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_enable_transcoder_func(crtc);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else
 		intel_update_watermarks(crtc);
 
@@ -14526,7 +14526,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_cstate = to_intel_crtc_state(crtc->state);
 
 		if (dev_priv->display.optimize_watermarks)
-			dev_priv->display.optimize_watermarks(intel_cstate);
+			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
 	}
 
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14934,7 +14934,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
-	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14947,14 +14946,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		intel_color_load_luts(crtc->state);
 	}
 
-	if (intel_cstate->update_pipe) {
+	if (intel_cstate->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
-	} else if (INTEL_GEN(dev_priv) >= 9) {
+	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
-		I915_WRITE(PIPE_WM_LINETIME(pipe),
-			   intel_cstate->wm.skl.optimal.linetime);
-	}
+	if (dev_priv->display.atomic_evade_watermarks)
+		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
@@ -16408,7 +16406,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
 		cs->wm.need_postvbl_update = true;
-		dev_priv->display.optimize_watermarks(cs);
+		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
 	}
 
 put_state:
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 41953cc41809..5ff35833b258 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4199,6 +4199,17 @@ skl_compute_wm(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void skl_evade_crtc_wm(struct intel_atomic_state *state,
+			      struct intel_crtc_state *cstate)
+{
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	enum pipe pipe = crtc->pipe;
+
+	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -4292,7 +4303,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
-static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
+static void ilk_initial_watermarks(struct intel_atomic_state *state,
+				   struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -4303,7 +4315,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
-static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+static void ilk_optimize_watermarks(struct intel_atomic_state *state,
+				    struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -7741,6 +7754,7 @@ void intel_init_pm(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
 		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
 		ilk_setup_wm_latency(dev);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (6 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 23:13   ` Matt Roper
  2016-10-26 13:41 ` [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2 Maarten Lankhorst
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

There's no need to keep a duplicate skl_pipe_wm around any more,
everything can be discovered from crtc_state, which we pass around
correctly now even in case of plane disable.

The copy in intel_crtc->wm.skl.active is equal to
crtc_state->wm.skl.optimal after the atomic commit completes.
It's useful for two-step watermark programming, but not required for
gen9+ which does it in a single step. We can pull the old allocation
from old_crtc_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++----------
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 48eb0635ec0f..592a6ec354a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13478,7 +13478,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
 		return;
 
 	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
-	sw_wm = &intel_crtc->wm.active.skl;
+	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
 
 	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
 	sw_ddb = &dev_priv->wm.skl_hw.ddb;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 73be640fad36..77a0a73e37b0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -723,7 +723,6 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		union {
 			struct intel_pipe_wm ilk;
-			struct skl_pipe_wm skl;
 		} active;
 
 		/* allow CxSR on this pipe */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5ff35833b258..fe4bc97ed56a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3930,11 +3930,11 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 }
 
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
-			      struct skl_ddb_allocation *ddb, /* out */
+			      const struct skl_pipe_wm *old_pipe_wm,
 			      struct skl_pipe_wm *pipe_wm, /* out */
+			      struct skl_ddb_allocation *ddb, /* out */
 			      bool *changed /* out */)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
 	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
 	int ret;
 
@@ -3942,7 +3942,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	if (ret)
 		return ret;
 
-	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
+	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
 		*changed = false;
 	else
 		*changed = true;
@@ -4177,10 +4177,12 @@ skl_compute_wm(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *intel_cstate =
 			to_intel_crtc_state(cstate);
+		const struct skl_pipe_wm *old_pipe_wm =
+			&to_intel_crtc_state(crtc->state)->wm.skl.optimal;
 
 		pipe_wm = &intel_cstate->wm.skl.optimal;
-		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
-					 &changed);
+		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
+					 &results->ddb, &changed);
 		if (ret)
 			return ret;
 
@@ -4224,8 +4226,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
 
-	intel_crtc->wm.active.skl = *pipe_wm;
-
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
 	/*
@@ -4395,10 +4395,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
 
 		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
 
-		if (intel_crtc->active) {
+		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) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (7 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 16:05   ` Lyude Paul
  2016-10-26 23:24   ` Matt Roper
  2016-10-26 13:41 ` [PATCH v2 10/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
                   ` (2 subsequent siblings)
  11 siblings, 2 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

The watermark updates for SKL style watermarks are no longer done
in the plane callbacks, but are now called in a separate watermark
update function that's called during the same vblank evasion,
before the plane updates.

This also gets rid of the global skl_results, which was required for
keeping track of the current atomic commit.

Changes since v1:
- Move line unwrap to correct patch. (Lyude)
- Make sure we don't regress ILK watermarks. (Matt)
- Rephrase commit message. (Matt)

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Lyude <cpaul@redhat.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  7 -------
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  7 -------
 drivers/gpu/drm/i915/intel_pm.c      | 35 ++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_sprite.c  | 18 ----------------
 5 files changed, 31 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a477d6a486e..227993f0e3ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2038,13 +2038,6 @@ struct drm_i915_private {
 		 */
 		uint16_t skl_latency[8];
 
-		/*
-		 * The skl_wm_values structure is a bit too big for stack
-		 * allocation, so we keep the staging struct where we store
-		 * intermediate results here instead.
-		 */
-		struct skl_wm_values skl_results;
-
 		/* current hardware state */
 		union {
 			struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 592a6ec354a7..e80ecf85768a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3384,9 +3384,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3422,9 +3419,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
-	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -3457,18 +3451,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!crtc->primary->state->visible)
-		skl_write_plane_wm(intel_crtc, p_wm,
-				   &dev_priv->wm.skl_results.ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
 	POSTING_READ(PLANE_SURF(pipe, 0));
@@ -10840,16 +10824,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
-	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
-
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 			intel_check_cpu_fifo_underruns(dev_priv);
 			intel_check_pch_fifo_underruns(dev_priv);
 
-			if (!crtc->state->active)
-				intel_update_watermarks(crtc);
+			if (!crtc->state->active) {
+				/*
+				 * Make sure we don't call initial_watermarks
+				 * for ILK-style watermark updates.
+				 */
+				if (HAS_DDI(dev_priv) && dev_priv->display.initial_watermarks)
+					dev_priv->display.initial_watermarks(intel_state,
+									     to_intel_crtc_state(crtc->state));
+				else
+					intel_update_watermarks(crtc);
+			}
 		}
 	}
 
@@ -14631,7 +14617,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(state, true);
 	dev_priv->wm.distrust_bios_wm = false;
-	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 	intel_atomic_track_fbs(state);
 
@@ -14939,7 +14924,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	intel_pipe_update_start(intel_crtc);
 
 	if (modeset)
-		return;
+		goto out;
 
 	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
 		intel_color_set_csc(crtc->state);
@@ -14951,6 +14936,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
+out:
 	if (dev_priv->display.atomic_evade_watermarks)
 		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 77a0a73e37b0..2cd7c5fd9ebd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1747,13 +1747,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       enum pipe pipe);
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 				 struct intel_crtc *intel_crtc);
-void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_plane_wm *wm,
-			 const struct skl_ddb_allocation *ddb);
-void skl_write_plane_wm(struct intel_crtc *intel_crtc,
-			const struct skl_plane_wm *wm,
-			const struct skl_ddb_allocation *ddb,
-			int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe4bc97ed56a..e2541539967b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4201,26 +4201,35 @@ skl_compute_wm(struct drm_atomic_state *state)
 	return 0;
 }
 
-static void skl_evade_crtc_wm(struct intel_atomic_state *state,
-			      struct intel_crtc_state *cstate)
+static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
 {
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
 	enum pipe pipe = crtc->pipe;
+	int plane;
+
+	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
+		return;
 
 	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+
+	for (plane = 0; plane < intel_num_planes(crtc); plane++)
+		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
+
+	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
 }
 
-static void skl_update_wm(struct drm_crtc *crtc)
+static void skl_initial_wm(struct intel_atomic_state *state,
+			   struct intel_crtc_state *cstate)
 {
+	struct drm_crtc *crtc = cstate->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct skl_wm_values *results = &dev_priv->wm.skl_results;
+	struct skl_wm_values *results = &state->wm_results;
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
@@ -4234,16 +4243,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	 * the pipe's shut off, just do so here. Already active pipes will have
 	 * their watermarks updated once we update their planes.
 	 */
-	if (crtc->state->active_changed) {
-		int plane;
-
-		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
-			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
-					   &results->ddb, plane);
-
-		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
-				    &results->ddb);
-	}
+	if (cstate->base.active_changed)
+		skl_evade_crtc_wm(state, cstate);
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
@@ -7751,7 +7752,7 @@ void intel_init_pm(struct drm_device *dev)
 	/* For FIFO watermark updates */
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
-		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.initial_watermarks = skl_initial_wm;
 		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 43d0350856e7..00d17d683f67 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	struct drm_crtc *crtc = crtc_state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[plane];
 	u32 plane_ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 surf_addr = plane_state->main.offset;
@@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
-	if (wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
-
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
@@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!dplane->state->visible)
-		skl_write_plane_wm(to_intel_crtc(crtc),
-				   &cstate->wm.skl.optimal.planes[plane],
-				   &dev_priv->wm.skl_results.ddb, plane);
-
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 10/11] drm/i915/gen9+: Preserve old allocation from crtc_state.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (8 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2 Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 13:41 ` [PATCH v2 11/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
  2016-10-26 14:16 ` ✓ Fi.CI.BAT: success for drm/i915/gen9+: Atomic watermark fixes Patchwork
  11 siblings, 0 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

This is the last bit required for making nonblocking modesets work
correctly. The state in intel_crtc->hw_ddb is not updated until
somewhere in atomic commit, while the previous crtc state should be
accurate if the ddb hasn't changed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e80ecf85768a..f73926d62521 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14349,7 +14349,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			 * new ddb allocation to take effect.
 			 */
 			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
-						 &intel_crtc->hw_ddb) &&
+						 &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb) &&
 			    !crtc->state->active_changed &&
 			    intel_state->wm_results.dirty_pipes != updated)
 				vbl_wait = true;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e2541539967b..6be54cd91f27 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3118,7 +3118,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	 * we currently hold.
 	 */
 	if (!intel_state->active_pipe_changes) {
-		*alloc = to_intel_crtc(for_crtc)->hw_ddb;
+		/*
+		 * alloc may be cleared by clear_intel_crtc_state,
+		 * copy from old state to be sure
+		 */
+		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
 		return;
 	}
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2 11/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (9 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 10/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
@ 2016-10-26 13:41 ` Maarten Lankhorst
  2016-10-26 14:16 ` ✓ Fi.CI.BAT: success for drm/i915/gen9+: Atomic watermark fixes Patchwork
  11 siblings, 0 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-26 13:41 UTC (permalink / raw)
  To: intel-gfx

This member is only used in skl_update_crtcs now. It's easy to remove it
by keeping track of which ddb entries in an array, and update them after
we update the crtc. This removes the last bits of SKL-style watermarks
kept outside of crtc_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     | 11 +++--------
 drivers/gpu/drm/i915/intel_pm.c      | 25 +++++++------------------
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f73926d62521..7107e2c82daf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14316,6 +14316,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 	unsigned int updated = 0;
 	bool progress;
 	enum pipe pipe;
+	int i;
+
+	const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};
+
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i)
+		/* ignore allocations for crtc's that have been turned off. */
+		if (crtc->state->active)
+			entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
 
 	/*
 	 * Whenever the number of active pipes changes, we need to make sure we
@@ -14324,7 +14332,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 	 * cause pipe underruns and other bad stuff.
 	 */
 	do {
-		int i;
 		progress = false;
 
 		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14335,12 +14342,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			cstate = to_intel_crtc_state(crtc->state);
 			pipe = intel_crtc->pipe;
 
-			if (updated & cmask || !crtc->state->active)
+			if (updated & cmask || !cstate->base.active)
 				continue;
-			if (skl_ddb_allocation_overlaps(state, intel_crtc))
+
+			if (skl_ddb_allocation_overlaps(entries, &cstate->wm.skl.ddb, i))
 				continue;
 
 			updated |= cmask;
+			entries[i] = &cstate->wm.skl.ddb;
 
 			/*
 			 * If this is an already active pipe, it's DDB changed,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2cd7c5fd9ebd..4827f1516399 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -729,9 +729,6 @@ struct intel_crtc {
 		bool cxsr_allowed;
 	} wm;
 
-	/* gen9+: ddb allocation currently being used */
-	struct skl_ddb_entry hw_ddb;
-
 	int scanline_offset;
 
 	struct {
@@ -1742,11 +1739,9 @@ 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);
-bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
-				 struct intel_crtc *intel_crtc);
+bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
+				 const struct skl_ddb_entry *ddb,
+				 int ignore);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6be54cd91f27..b923040bd7e2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3910,25 +3910,16 @@ static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
 	return a->start < b->end && b->start < a->end;
 }
 
-bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
-				 struct intel_crtc *intel_crtc)
-{
-	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;
+bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
+				 const struct skl_ddb_entry *ddb,
+				 int ignore)
+{
 	int i;
 
-	for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
-		other_intel_crtc = to_intel_crtc(other_crtc);
-
-		if (other_intel_crtc == intel_crtc)
-			continue;
-
-		if (skl_ddb_entries_overlap(ddb, &other_intel_crtc->hw_ddb))
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		if (i != ignore && entries[i] &&
+		    skl_ddb_entries_overlap(ddb, entries[i]))
 			return true;
-	}
 
 	return false;
 }
@@ -4252,8 +4243,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
 
 	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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/gen9+: Atomic watermark fixes.
  2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
                   ` (10 preceding siblings ...)
  2016-10-26 13:41 ` [PATCH v2 11/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
@ 2016-10-26 14:16 ` Patchwork
  11 siblings, 0 replies; 34+ messages in thread
From: Patchwork @ 2016-10-26 14:16 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/gen9+: Atomic watermark fixes.
URL   : https://patchwork.freedesktop.org/series/14405/
State : success

== Summary ==

Series 14405v1 drm/i915/gen9+: Atomic watermark fixes.
https://patchwork.freedesktop.org/api/1.0/series/14405/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:211  dwarn:0   dfail:0   fail:0   skip:35 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

760634d2b78657f0a2267f73bff0d527f6c69ce5 drm-intel-nightly: 2016y-10m-26d-11h-52m-53s UTC integration manifest
3851736 drm/i915/gen9+: Kill off hw_ddb from intel_crtc.
8088a1f drm/i915/gen9+: Preserve old allocation from crtc_state.
53b40ed drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
280e6c9 drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2.
e1b41c4 drm/i915: Add a atomic evasion step to watermark programming, v2.
e1391f9 drm/i915/skl+: Clean up minimum allocations, v2.
4867fcd drm/i915/skl+: Remove minimum block allocation from crtc state.
e29a244 drm/i915/skl+: Remove data_rate from watermark struct, v2.
e1255cc drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
09955c2 drm/i915/gen9+: Use cstate plane mask instead of crtc->state.
95e2c7a drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2823/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2823/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
  2016-10-26 13:41 ` [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2 Maarten Lankhorst
@ 2016-10-26 16:05   ` Lyude Paul
  2016-10-26 23:24   ` Matt Roper
  1 sibling, 0 replies; 34+ messages in thread
From: Lyude Paul @ 2016-10-26 16:05 UTC (permalink / raw)
  To: Maarten Lankhorst, intel-gfx

This approach is perfect :).

Reviewed-by: Lyude <lyude@redhat.com>

On Wed, 2016-10-26 at 15:41 +0200, Maarten Lankhorst wrote:
> The watermark updates for SKL style watermarks are no longer done
> in the plane callbacks, but are now called in a separate watermark
> update function that's called during the same vblank evasion,
> before the plane updates.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Changes since v1:
> - Move line unwrap to correct patch. (Lyude)
> - Make sure we don't regress ILK watermarks. (Matt)
> - Rephrase commit message. (Matt)
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lyude <cpaul@redhat.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++------------
> ------------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 -------
>  drivers/gpu/drm/i915/intel_pm.c      | 35 ++++++++++++++++--------
> -------
>  drivers/gpu/drm/i915/intel_sprite.c  | 18 ----------------
>  5 files changed, 31 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 7a477d6a486e..227993f0e3ff 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2038,13 +2038,6 @@ struct drm_i915_private {
>  		 */
>  		uint16_t skl_latency[8];
>  
> -		/*
> -		 * The skl_wm_values structure is a bit too big for
> stack
> -		 * allocation, so we keep the staging struct where
> we store
> -		 * intermediate results here instead.
> -		 */
> -		struct skl_wm_values skl_results;
> -
>  		/* current hardware state */
>  		union {
>  			struct ilk_wm_values hw;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 592a6ec354a7..e80ecf85768a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3384,9 +3384,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3422,9 +3419,6 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -3457,18 +3451,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_plane_wm *p_wm = &cstate-
> >wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, p_wm,
> -				   &dev_priv->wm.skl_results.ddb,
> 0);
> -
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -10840,16 +10824,9 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	const struct skl_plane_wm *p_wm =
> -		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> -	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
> -
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				/*
> +				 * Make sure we don't call
> initial_watermarks
> +				 * for ILK-style watermark updates.
> +				 */
> +				if (HAS_DDI(dev_priv) && dev_priv-
> >display.initial_watermarks)
> +					dev_priv-
> >display.initial_watermarks(intel_state,
> +									
>      to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc
> );
> +			}
>  		}
>  	}
>  
> @@ -14631,7 +14617,6 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14939,7 +14924,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;
>  
>  	if (crtc->state->color_mgmt_changed ||
> to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14951,6 +14936,7 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv-
> >display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state
> ->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 77a0a73e37b0..2cd7c5fd9ebd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1747,13 +1747,6 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index fe4bc97ed56a..e2541539967b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4201,26 +4201,35 @@ skl_compute_wm(struct drm_atomic_state
> *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state-
> >base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state-
> >wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc-
> >base)))
> +		return;
>  
>  	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane],
> ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR],
> ddb);
>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4234,16 +4243,8 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  	 * the pipe's shut off, just do so here. Already active
> pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane <
> intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7751,7 +7752,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks =
> skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks =
> skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks =
> skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 43d0350856e7..00d17d683f67 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state-
> >ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
> >min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key-
> >max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if
> the
> -	 * plane's visiblity isn't actually changing neither is its
> watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate-
> >wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb,
> plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2.
  2016-10-26 13:41 ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Maarten Lankhorst
@ 2016-10-26 16:19   ` Ville Syrjälä
  2016-10-27  7:10     ` Daniel Vetter
  2016-10-27 10:37     ` [PATCH v2.1 07/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
  2016-10-26 23:10   ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Matt Roper
  1 sibling, 2 replies; 34+ messages in thread
From: Ville Syrjälä @ 2016-10-26 16:19 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, Paulo Zanoni

On Wed, Oct 26, 2016 at 03:41:35PM +0200, Maarten Lankhorst wrote:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> intel_atomic_state is not used here yet, but will be used when
> we program all watermarks as a separate step during evasion.
> 
> This also writes linetime all the time, while before it was only
> done during plane updates. This looks like this could be a bugfix,
> but I'm not sure what it affects.
> 
> Changes since v1:
> - Add comment about atomic evasion to commit message.
> - Unwrap I915_WRITE call. (Lyude)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>  drivers/gpu/drm/i915/intel_display.c | 20 +++++++++-----------
>  drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++++++++++--
>  3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a621c74254e..7a477d6a486e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -485,6 +485,7 @@ struct sdvo_device_mapping {
>  
>  struct intel_connector;
>  struct intel_encoder;
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_initial_plane_config;
>  struct intel_crtc;
> @@ -498,8 +499,9 @@ struct drm_i915_display_funcs {
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *newstate);
> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);

Same drive by comment I gave before somewhere:
That name is still super confusing. We're not trying to evade watermarks are we?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2.
  2016-10-26 13:41 ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Maarten Lankhorst
  2016-10-26 16:19   ` Ville Syrjälä
@ 2016-10-26 23:10   ` Matt Roper
  1 sibling, 0 replies; 34+ messages in thread
From: Matt Roper @ 2016-10-26 23:10 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx, Paulo Zanoni

Minor grammar fix in the headline...it should be "an atomic" rather than
"a atomic."  Sorry I missed that the first time around.

I know i915 tends to be somewhat lax about adhering to the 80-character
line limit, but the lines that add the extra parameter are starting to
get _really_ long now, so it would be good to wrap them if possible.

Otherwise the change here look good to me, so with the minor cosmetic
stuff fixed,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

On Wed, Oct 26, 2016 at 03:41:35PM +0200, Maarten Lankhorst wrote:
> Allow the driver to write watermarks during atomic evasion.
> This will make it possible to write the watermarks in a cleaner
> way on gen9+.
> 
> intel_atomic_state is not used here yet, but will be used when
> we program all watermarks as a separate step during evasion.
> 
> This also writes linetime all the time, while before it was only
> done during plane updates. This looks like this could be a bugfix,
> but I'm not sure what it affects.
> 
> Changes since v1:
> - Add comment about atomic evasion to commit message.
> - Unwrap I915_WRITE call. (Lyude)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
>  drivers/gpu/drm/i915/intel_display.c | 20 +++++++++-----------
>  drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++++++++++--
>  3 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a621c74254e..7a477d6a486e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -485,6 +485,7 @@ struct sdvo_device_mapping {
>  
>  struct intel_connector;
>  struct intel_encoder;
> +struct intel_atomic_state;
>  struct intel_crtc_state;
>  struct intel_initial_plane_config;
>  struct intel_crtc;
> @@ -498,8 +499,9 @@ struct drm_i915_display_funcs {
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *newstate);
> -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> +	void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
>  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9ea9a3bc0bc0..48eb0635ec0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5170,7 +5170,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	 * us to.
>  	 */
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
>  	else if (pipe_config->update_wm_pre)
>  		intel_update_watermarks(&crtc->base);
>  }
> @@ -5384,7 +5384,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_color_load_luts(&pipe_config->base);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(intel_crtc->config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -5490,7 +5490,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  		intel_ddi_enable_transcoder_func(crtc);
>  
>  	if (dev_priv->display.initial_watermarks != NULL)
> -		dev_priv->display.initial_watermarks(pipe_config);
> +		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
>  	else
>  		intel_update_watermarks(crtc);
>  
> @@ -14526,7 +14526,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_cstate = to_intel_crtc_state(crtc->state);
>  
>  		if (dev_priv->display.optimize_watermarks)
> -			dev_priv->display.optimize_watermarks(intel_cstate);
> +			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14934,7 +14934,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> -	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14947,14 +14946,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  		intel_color_load_luts(crtc->state);
>  	}
>  
> -	if (intel_cstate->update_pipe) {
> +	if (intel_cstate->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_state);
> -	} else if (INTEL_GEN(dev_priv) >= 9) {
> +	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> -		I915_WRITE(PIPE_WM_LINETIME(pipe),
ma> -			   intel_cstate->wm.skl.optimal.linetime);
> -	}
> +	if (dev_priv->display.atomic_evade_watermarks)
> +		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> @@ -16408,7 +16406,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>  
>  		cs->wm.need_postvbl_update = true;
> -		dev_priv->display.optimize_watermarks(cs);
> +		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
>  	}
>  
>  put_state:
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 41953cc41809..5ff35833b258 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4199,6 +4199,17 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> +			      struct intel_crtc_state *cstate)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	enum pipe pipe = crtc->pipe;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -4292,7 +4303,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  	ilk_write_wm_values(dev_priv, &results);
>  }
>  
> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_initial_watermarks(struct intel_atomic_state *state,
> +				   struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -4303,7 +4315,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_optimize_watermarks(struct intel_atomic_state *state,
> +				    struct intel_crtc_state *cstate)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> @@ -7741,6 +7754,7 @@ void intel_init_pm(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
>  		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
>  		ilk_setup_wm_latency(dev);
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2.
  2016-10-26 13:41 ` [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2 Maarten Lankhorst
@ 2016-10-26 23:13   ` Matt Roper
  0 siblings, 0 replies; 34+ messages in thread
From: Matt Roper @ 2016-10-26 23:13 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:36PM +0200, Maarten Lankhorst wrote:
> There's no need to keep a duplicate skl_pipe_wm around any more,
> everything can be discovered from crtc_state, which we pass around
> correctly now even in case of plane disable.
> 
> The copy in intel_crtc->wm.skl.active is equal to
> crtc_state->wm.skl.optimal after the atomic commit completes.
> It's useful for two-step watermark programming, but not required for
> gen9+ which does it in a single step. We can pull the old allocation
> from old_crtc_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++----------
>  3 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 48eb0635ec0f..592a6ec354a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13478,7 +13478,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  		return;
>  
>  	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> -	sw_wm = &intel_crtc->wm.active.skl;
> +	sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal;
>  
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 73be640fad36..77a0a73e37b0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -723,7 +723,6 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		union {
>  			struct intel_pipe_wm ilk;
> -			struct skl_pipe_wm skl;
>  		} active;
>  
>  		/* allow CxSR on this pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5ff35833b258..fe4bc97ed56a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3930,11 +3930,11 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  }
>  
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> -			      struct skl_ddb_allocation *ddb, /* out */
> +			      const struct skl_pipe_wm *old_pipe_wm,
>  			      struct skl_pipe_wm *pipe_wm, /* out */
> +			      struct skl_ddb_allocation *ddb, /* out */
>  			      bool *changed /* out */)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
>  	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
>  	int ret;
>  
> @@ -3942,7 +3942,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  	if (ret)
>  		return ret;
>  
> -	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> +	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
>  		*changed = false;
>  	else
>  		*changed = true;
> @@ -4177,10 +4177,12 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *intel_cstate =
>  			to_intel_crtc_state(cstate);
> +		const struct skl_pipe_wm *old_pipe_wm =
> +			&to_intel_crtc_state(crtc->state)->wm.skl.optimal;
>  
>  		pipe_wm = &intel_cstate->wm.skl.optimal;
> -		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
> -					 &changed);
> +		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> +					 &results->ddb, &changed);
>  		if (ret)
>  			return ret;
>  
> @@ -4224,8 +4226,6 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
>  
> -	intel_crtc->wm.active.skl = *pipe_wm;
> -
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
>  	/*
> @@ -4395,10 +4395,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
>  
>  		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
>  
> -		if (intel_crtc->active) {
> +		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) {
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
  2016-10-26 13:41 ` [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2 Maarten Lankhorst
  2016-10-26 16:05   ` Lyude Paul
@ 2016-10-26 23:24   ` Matt Roper
  2016-10-27  8:29     ` Maarten Lankhorst
  1 sibling, 1 reply; 34+ messages in thread
From: Matt Roper @ 2016-10-26 23:24 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:37PM +0200, Maarten Lankhorst wrote:
> The watermark updates for SKL style watermarks are no longer done
> in the plane callbacks, but are now called in a separate watermark
> update function that's called during the same vblank evasion,
> before the plane updates.
> 
> This also gets rid of the global skl_results, which was required for
> keeping track of the current atomic commit.
> 
> Changes since v1:
> - Move line unwrap to correct patch. (Lyude)
> - Make sure we don't regress ILK watermarks. (Matt)
> - Rephrase commit message. (Matt)
> 
...
> @@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  			intel_check_cpu_fifo_underruns(dev_priv);
>  			intel_check_pch_fifo_underruns(dev_priv);
>  
> -			if (!crtc->state->active)
> -				intel_update_watermarks(crtc);
> +			if (!crtc->state->active) {
> +				/*
> +				 * Make sure we don't call initial_watermarks
> +				 * for ILK-style watermark updates.
> +				 */
> +				if (HAS_DDI(dev_priv) && dev_priv->display.initial_watermarks)

Aren't HSW/BDW DDI platforms?  They still use the ILK-style watermarks,
so I don't think this is protecting all the platforms it needs to.

Even if that weren't the case, I wouldn't be wild about using HAS_DDI
here since whether or not a platform has DDI isn't really the reason
we're programming watermarks differently so the code is a bit confusing
to the casual reader.


Matt

> +					dev_priv->display.initial_watermarks(intel_state,
> +									     to_intel_crtc_state(crtc->state));
> +				else
> +					intel_update_watermarks(crtc);
> +			}
>  		}
>  	}
>  
> @@ -14631,7 +14617,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
> -	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  	intel_atomic_track_fbs(state);
>  
> @@ -14939,7 +14924,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
> -		return;
> +		goto out;
>  
>  	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
>  		intel_color_set_csc(crtc->state);
> @@ -14951,6 +14936,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	else if (INTEL_GEN(dev_priv) >= 9)
>  		skl_detach_scalers(intel_crtc);
>  
> +out:
>  	if (dev_priv->display.atomic_evade_watermarks)
>  		dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 77a0a73e37b0..2cd7c5fd9ebd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1747,13 +1747,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
> -void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_plane_wm *wm,
> -			 const struct skl_ddb_allocation *ddb);
> -void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_plane_wm *wm,
> -			const struct skl_ddb_allocation *ddb,
> -			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fe4bc97ed56a..e2541539967b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4201,26 +4201,35 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void skl_evade_crtc_wm(struct intel_atomic_state *state,
> -			      struct intel_crtc_state *cstate)
> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> +	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
>  	enum pipe pipe = crtc->pipe;
> +	int plane;
> +
> +	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
> +		return;
>  
>  	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
> +
> +	for (plane = 0; plane < intel_num_planes(crtc); plane++)
> +		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
> +
> +	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
>  }
>  
> -static void skl_update_wm(struct drm_crtc *crtc)
> +static void skl_initial_wm(struct intel_atomic_state *state,
> +			   struct intel_crtc_state *cstate)
>  {
> +	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	struct skl_wm_values *results = &state->wm_results;
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> @@ -4234,16 +4243,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	 * the pipe's shut off, just do so here. Already active pipes will have
>  	 * their watermarks updated once we update their planes.
>  	 */
> -	if (crtc->state->active_changed) {
> -		int plane;
> -
> -		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
> -					   &results->ddb, plane);
> -
> -		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
> -				    &results->ddb);
> -	}
> +	if (cstate->base.active_changed)
> +		skl_evade_crtc_wm(state, cstate);
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> @@ -7751,7 +7752,7 @@ void intel_init_pm(struct drm_device *dev)
>  	/* For FIFO watermark updates */
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_setup_wm_latency(dev);
> -		dev_priv->display.update_wm = skl_update_wm;
> +		dev_priv->display.initial_watermarks = skl_initial_wm;
>  		dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm;
>  		dev_priv->display.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 43d0350856e7..00d17d683f67 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> -	struct drm_crtc *crtc = crtc_state->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	const struct skl_plane_wm *p_wm =
> -		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> -	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
> -
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> @@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> -	/*
> -	 * We only populate skl_results on watermark updates, and if the
> -	 * plane's visiblity isn't actually changing neither is its watermarks.
> -	 */
> -	if (!dplane->state->visible)
> -		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &cstate->wm.skl.optimal.planes[plane],
> -				   &dev_priv->wm.skl_results.ddb, plane);
> -
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  
>  	I915_WRITE(PLANE_SURF(pipe, plane), 0);
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2.
  2016-10-26 16:19   ` Ville Syrjälä
@ 2016-10-27  7:10     ` Daniel Vetter
  2016-10-27 10:37     ` [PATCH v2.1 07/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
  1 sibling, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2016-10-27  7:10 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

On Wed, Oct 26, 2016 at 07:19:45PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 26, 2016 at 03:41:35PM +0200, Maarten Lankhorst wrote:
> > Allow the driver to write watermarks during atomic evasion.
> > This will make it possible to write the watermarks in a cleaner
> > way on gen9+.
> > 
> > intel_atomic_state is not used here yet, but will be used when
> > we program all watermarks as a separate step during evasion.
> > 
> > This also writes linetime all the time, while before it was only
> > done during plane updates. This looks like this could be a bugfix,
> > but I'm not sure what it affects.
> > 
> > Changes since v1:
> > - Add comment about atomic evasion to commit message.
> > - Unwrap I915_WRITE call. (Lyude)
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 ++++--
> >  drivers/gpu/drm/i915/intel_display.c | 20 +++++++++-----------
> >  drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++++++++++--
> >  3 files changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7a621c74254e..7a477d6a486e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -485,6 +485,7 @@ struct sdvo_device_mapping {
> >  
> >  struct intel_connector;
> >  struct intel_encoder;
> > +struct intel_atomic_state;
> >  struct intel_crtc_state;
> >  struct intel_initial_plane_config;
> >  struct intel_crtc;
> > @@ -498,8 +499,9 @@ struct drm_i915_display_funcs {
> >  	int (*compute_intermediate_wm)(struct drm_device *dev,
> >  				       struct intel_crtc *intel_crtc,
> >  				       struct intel_crtc_state *newstate);
> > -	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> > -	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> > +	void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> > +	void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate);
> 
> Same drive by comment I gave before somewhere:
> That name is still super confusing. We're not trying to evade watermarks are we?

The atomic_update_watermarks suggestion I've seen fly by on irc sounded
good.

Aside: some proper kerneldocs for display funcs would be also really good
;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  2016-10-26 13:41 ` [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2 Maarten Lankhorst
@ 2016-10-27  7:12   ` Daniel Vetter
  2016-10-27  8:31     ` Maarten Lankhorst
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2016-10-27  7:12 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:29PM +0200, Maarten Lankhorst wrote:
> Changes since v1:
> - Remove plane->pipe checks, they're implied by the macros.
> - Split unrelated changes to a separate commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Drive-by review: The above commit message is way too thin. It prepares for
something, but I can't make any connection at all to that something. 1-2
sentences about why are needed here I think.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9e0e8741253e..044a8c932a1c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  /**
>   * DOC: RC6
> @@ -3269,24 +3270,20 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
>  	struct drm_crtc *crtc = cstate->crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	const struct drm_plane *plane;
> +	struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
> -	struct drm_plane_state *pstate;
> +	const struct drm_plane_state *pstate;
>  	unsigned int rate, total_data_rate = 0;
>  	int id;
> -	int i;
>  
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	/* Calculate and cache data rate for each plane */
> -	for_each_plane_in_state(state, plane, pstate, i) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
>  		id = skl_wm_plane_id(to_intel_plane(plane));
>  		intel_plane = to_intel_plane(plane);
>  
> -		if (intel_plane->pipe != intel_crtc->pipe)
> -			continue;
> -
>  		/* packed/uv */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 0);
> @@ -3383,7 +3380,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *pstate;
> +	const struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
>  	uint16_t alloc_size, start, cursor_blocks;
> @@ -3419,14 +3416,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	alloc_size -= cursor_blocks;
>  
>  	/* 1. Allocate the mininum required blocks for each active plane */
> -	for_each_plane_in_state(state, plane, pstate, i) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
>  		intel_plane = to_intel_plane(plane);
>  		id = skl_wm_plane_id(intel_plane);
>  
> -		if (intel_plane->pipe != pipe)
> -			continue;
> -
> -		if (!to_intel_plane_state(pstate)->base.visible) {
> +		if (!pstate->visible) {
>  			minimum[id] = 0;
>  			y_minimum[id] = 0;
>  			continue;
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2.
  2016-10-26 23:24   ` Matt Roper
@ 2016-10-27  8:29     ` Maarten Lankhorst
  2016-10-27 11:45       ` [PATCH v2.1 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3 Maarten Lankhorst
  0 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-27  8:29 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

Op 27-10-16 om 01:24 schreef Matt Roper:
> On Wed, Oct 26, 2016 at 03:41:37PM +0200, Maarten Lankhorst wrote:
>> The watermark updates for SKL style watermarks are no longer done
>> in the plane callbacks, but are now called in a separate watermark
>> update function that's called during the same vblank evasion,
>> before the plane updates.
>>
>> This also gets rid of the global skl_results, which was required for
>> keeping track of the current atomic commit.
>>
>> Changes since v1:
>> - Move line unwrap to correct patch. (Lyude)
>> - Make sure we don't regress ILK watermarks. (Matt)
>> - Rephrase commit message. (Matt)
>>
> ...
>> @@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  			intel_check_cpu_fifo_underruns(dev_priv);
>>  			intel_check_pch_fifo_underruns(dev_priv);
>>  
>> -			if (!crtc->state->active)
>> -				intel_update_watermarks(crtc);
>> +			if (!crtc->state->active) {
>> +				/*
>> +				 * Make sure we don't call initial_watermarks
>> +				 * for ILK-style watermark updates.
>> +				 */
>> +				if (HAS_DDI(dev_priv) && dev_priv->display.initial_watermarks)
> Aren't HSW/BDW DDI platforms?  They still use the ILK-style watermarks,
> so I don't think this is protecting all the platforms it needs to.
>
> Even if that weren't the case, I wouldn't be wild about using HAS_DDI
> here since whether or not a platform has DDI isn't really the reason
> we're programming watermarks differently so the code is a bit confusing
> to the casual reader.
Oh right, completely forgot about that.

Maybe change the check to if dev_priv->atomic_update_watermarks ?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  2016-10-27  7:12   ` Daniel Vetter
@ 2016-10-27  8:31     ` Maarten Lankhorst
  2016-10-27 17:48       ` Paulo Zanoni
  0 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-27  8:31 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

Op 27-10-16 om 09:12 schreef Daniel Vetter:
> On Wed, Oct 26, 2016 at 03:41:29PM +0200, Maarten Lankhorst wrote:
>> Changes since v1:
>> - Remove plane->pipe checks, they're implied by the macros.
>> - Split unrelated changes to a separate commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Drive-by review: The above commit message is way too thin. It prepares for
> something, but I can't make any connection at all to that something. 1-2
> sentences about why are needed here I think.

Hm, seems to have been lost somewhere. It was on my previous submission:

Caching is not required, drm_atomic_crtc_state_for_each_plane_state
can be used to inspect all plane_states that are assigned to the
current crtc_state, so we can just recalculate every time.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2.1 07/11] drm/i915: Add a atomic evasion step to watermark programming, v3.
  2016-10-26 16:19   ` Ville Syrjälä
  2016-10-27  7:10     ` Daniel Vetter
@ 2016-10-27 10:37     ` Maarten Lankhorst
  1 sibling, 0 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-27 10:37 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

Allow the driver to write watermarks during atomic evasion.
This will make it possible to write the watermarks in a cleaner
way on gen9+.

intel_atomic_state is not used here yet, but will be used when
we program all watermarks as a separate step during evasion.

This also writes linetime all the time, while before it was only
done during plane updates. This looks like this could be a bugfix,
but I'm not sure what it affects.

Changes since v1:
- Add comment about atomic evasion to commit message.
- Unwrap I915_WRITE call. (Lyude)
Changes since v2:
- Rename atomic_evade_watermarks to atomic_update_watermarks. (Ville)
- Add line wraps where appropriate, fix grammar in commit message. (Matt)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++--
 drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_pm.c      | 18 ++++++++++++++++--
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a621c74254e..6639bb812b96 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -485,6 +485,7 @@ struct sdvo_device_mapping {
 
 struct intel_connector;
 struct intel_encoder;
+struct intel_atomic_state;
 struct intel_crtc_state;
 struct intel_initial_plane_config;
 struct intel_crtc;
@@ -498,8 +499,12 @@ struct drm_i915_display_funcs {
 	int (*compute_intermediate_wm)(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
 				       struct intel_crtc_state *newstate);
-	void (*initial_watermarks)(struct intel_crtc_state *cstate);
-	void (*optimize_watermarks)(struct intel_crtc_state *cstate);
+	void (*initial_watermarks)(struct intel_atomic_state *state,
+				   struct intel_crtc_state *cstate);
+	void (*atomic_update_watermarks)(struct intel_atomic_state *state,
+					 struct intel_crtc_state *cstate);
+	void (*optimize_watermarks)(struct intel_atomic_state *state,
+				    struct intel_crtc_state *cstate);
 	int (*compute_global_watermarks)(struct drm_atomic_state *state);
 	void (*update_wm)(struct drm_crtc *crtc);
 	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ea9a3bc0bc0..4b6e2595edec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5170,7 +5170,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	 * us to.
 	 */
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else if (pipe_config->update_wm_pre)
 		intel_update_watermarks(&crtc->base);
 }
@@ -5384,7 +5384,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
 	intel_color_load_luts(&pipe_config->base);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(intel_crtc->config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config);
 	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -5490,7 +5490,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_enable_transcoder_func(crtc);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(pipe_config);
+		dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config);
 	else
 		intel_update_watermarks(crtc);
 
@@ -14526,7 +14526,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_cstate = to_intel_crtc_state(crtc->state);
 
 		if (dev_priv->display.optimize_watermarks)
-			dev_priv->display.optimize_watermarks(intel_cstate);
+			dev_priv->display.optimize_watermarks(intel_state, intel_cstate);
 	}
 
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -14931,10 +14931,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	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 =
+	struct intel_crtc_state *old_intel_cstate =
 		to_intel_crtc_state(old_crtc_state);
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_crtc_state->state);
 	bool modeset = needs_modeset(crtc->state);
-	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14947,14 +14948,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		intel_color_load_luts(crtc->state);
 	}
 
-	if (intel_cstate->update_pipe) {
-		intel_update_pipe_config(intel_crtc, old_intel_state);
-	} else if (INTEL_GEN(dev_priv) >= 9) {
+	if (intel_cstate->update_pipe)
+		intel_update_pipe_config(intel_crtc, old_intel_cstate);
+	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
-		I915_WRITE(PIPE_WM_LINETIME(pipe),
-			   intel_cstate->wm.skl.optimal.linetime);
-	}
+	if (dev_priv->display.atomic_update_watermarks)
+		dev_priv->display.atomic_update_watermarks(old_intel_state, intel_cstate);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
@@ -16408,7 +16408,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
 		cs->wm.need_postvbl_update = true;
-		dev_priv->display.optimize_watermarks(cs);
+		dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs);
 	}
 
 put_state:
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 41953cc41809..a3154617ffb2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4199,6 +4199,17 @@ skl_compute_wm(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
+				      struct intel_crtc_state *cstate)
+{
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	enum pipe pipe = crtc->pipe;
+
+	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -4292,7 +4303,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
-static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
+static void ilk_initial_watermarks(struct intel_atomic_state *state,
+				   struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -4303,7 +4315,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
-static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+static void ilk_optimize_watermarks(struct intel_atomic_state *state,
+				    struct intel_crtc_state *cstate)
 {
 	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
@@ -7741,6 +7754,7 @@ void intel_init_pm(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
 		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.atomic_update_watermarks = skl_atomic_update_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
 		ilk_setup_wm_latency(dev);
-- 
2.7.4


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH v2.1 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3.
  2016-10-27  8:29     ` Maarten Lankhorst
@ 2016-10-27 11:45       ` Maarten Lankhorst
  0 siblings, 0 replies; 34+ messages in thread
From: Maarten Lankhorst @ 2016-10-27 11:45 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

The watermark updates for SKL style watermarks are no longer done
in the plane callbacks, but are now called in a separate watermark
update function that's called during the same vblank evasion,
before the plane updates.

This also gets rid of the global skl_results, which was required for
keeping track of the current atomic commit.

Changes since v1:
- Move line unwrap to correct patch. (Lyude)
- Make sure we don't regress ILK watermarks. (Matt)
- Rephrase commit message. (Matt)
Changes since v2:
- Fix disable watermark check to use the correct way to determine single
  step watermark support.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Lyude <cpaul@redhat.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  7 -------
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  7 -------
 drivers/gpu/drm/i915/intel_pm.c      | 32 +++++++++++++++--------------
 drivers/gpu/drm/i915/intel_sprite.c  | 18 ----------------
 5 files changed, 30 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6639bb812b96..9943cfe01449 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2041,13 +2041,6 @@ struct drm_i915_private {
 		 */
 		uint16_t skl_latency[8];
 
-		/*
-		 * The skl_wm_values structure is a bit too big for stack
-		 * allocation, so we keep the staging struct where we store
-		 * intermediate results here instead.
-		 */
-		struct skl_wm_values skl_results;
-
 		/* current hardware state */
 		union {
 			struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 82bca585c2b3..8ea074ea60da 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3384,9 +3384,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3422,9 +3419,6 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
-	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -3457,18 +3451,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!crtc->primary->state->visible)
-		skl_write_plane_wm(intel_crtc, p_wm,
-				   &dev_priv->wm.skl_results.ddb, 0);
-
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
 	POSTING_READ(PLANE_SURF(pipe, 0));
@@ -10840,16 +10824,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	const struct skl_plane_wm *p_wm =
-		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
-	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
-
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14459,8 +14436,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 			intel_check_cpu_fifo_underruns(dev_priv);
 			intel_check_pch_fifo_underruns(dev_priv);
 
-			if (!crtc->state->active)
-				intel_update_watermarks(crtc);
+			if (!crtc->state->active) {
+				/*
+				 * Make sure we don't call initial_watermarks
+				 * for ILK-style watermark updates.
+				 */
+				if (dev_priv->display.atomic_update_watermarks)
+					dev_priv->display.initial_watermarks(intel_state,
+									     to_intel_crtc_state(crtc->state));
+				else
+					intel_update_watermarks(crtc);
+			}
 		}
 	}
 
@@ -14631,7 +14617,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(state, true);
 	dev_priv->wm.distrust_bios_wm = false;
-	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 	intel_atomic_track_fbs(state);
 
@@ -14941,7 +14926,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	intel_pipe_update_start(intel_crtc);
 
 	if (modeset)
-		return;
+		goto out;
 
 	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
 		intel_color_set_csc(crtc->state);
@@ -14953,6 +14938,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	else if (INTEL_GEN(dev_priv) >= 9)
 		skl_detach_scalers(intel_crtc);
 
+out:
 	if (dev_priv->display.atomic_update_watermarks)
 		dev_priv->display.atomic_update_watermarks(old_intel_state, intel_cstate);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 77a0a73e37b0..2cd7c5fd9ebd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1747,13 +1747,6 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       enum pipe pipe);
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 				 struct intel_crtc *intel_crtc);
-void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_plane_wm *wm,
-			 const struct skl_ddb_allocation *ddb);
-void skl_write_plane_wm(struct intel_crtc *intel_crtc,
-			const struct skl_plane_wm *wm,
-			const struct skl_ddb_allocation *ddb,
-			int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b586bdc1d3a1..d263ca61e96d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4207,20 +4207,30 @@ static void skl_atomic_update_crtc_wm(struct intel_atomic_state *state,
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
+	const struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
 	enum pipe pipe = crtc->pipe;
+	int plane;
+
+	if (!(state->wm_results.dirty_pipes & drm_crtc_mask(&crtc->base)))
+		return;
 
 	I915_WRITE(PIPE_WM_LINETIME(pipe), pipe_wm->linetime);
+
+	for (plane = 0; plane < intel_num_planes(crtc); plane++)
+		skl_write_plane_wm(crtc, &pipe_wm->planes[plane], ddb, plane);
+
+	skl_write_cursor_wm(crtc, &pipe_wm->planes[PLANE_CURSOR], ddb);
 }
 
-static void skl_update_wm(struct drm_crtc *crtc)
+static void skl_initial_wm(struct intel_atomic_state *state,
+			   struct intel_crtc_state *cstate)
 {
+	struct drm_crtc *crtc = cstate->base.crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct skl_wm_values *results = &dev_priv->wm.skl_results;
+	struct skl_wm_values *results = &state->wm_results;
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
@@ -4234,16 +4244,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	 * the pipe's shut off, just do so here. Already active pipes will have
 	 * their watermarks updated once we update their planes.
 	 */
-	if (crtc->state->active_changed) {
-		int plane;
-
-		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
-			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
-					   &results->ddb, plane);
-
-		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
-				    &results->ddb);
-	}
+	if (cstate->base.active_changed)
+		skl_evade_crtc_wm(state, cstate);
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
@@ -7751,7 +7753,7 @@ void intel_init_pm(struct drm_device *dev)
 	/* For FIFO watermark updates */
 	if (INTEL_INFO(dev)->gen >= 9) {
 		skl_setup_wm_latency(dev);
-		dev_priv->display.update_wm = skl_update_wm;
+		dev_priv->display.initial_watermarks = skl_initial_wm;
 		dev_priv->display.atomic_update_watermarks = skl_atomic_update_crtc_wm;
 		dev_priv->display.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 43d0350856e7..00d17d683f67 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,13 +203,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
-	struct drm_crtc *crtc = crtc_state->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
-	const struct skl_plane_wm *p_wm =
-		&crtc_state->wm.skl.optimal.planes[plane];
 	u32 plane_ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 surf_addr = plane_state->main.offset;
@@ -233,9 +228,6 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
-	if (wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
-
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
@@ -291,19 +283,9 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
-	/*
-	 * We only populate skl_results on watermark updates, and if the
-	 * plane's visiblity isn't actually changing neither is its watermarks.
-	 */
-	if (!dplane->state->visible)
-		skl_write_plane_wm(to_intel_crtc(crtc),
-				   &cstate->wm.skl.optimal.planes[plane],
-				   &dev_priv->wm.skl_results.ddb, plane);
-
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
 	I915_WRITE(PLANE_SURF(pipe, plane), 0);
-- 
2.7.4


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
  2016-10-26 13:41 ` [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes Maarten Lankhorst
@ 2016-10-27 13:03   ` Ville Syrjälä
  2016-10-27 17:47   ` Matt Roper
  1 sibling, 0 replies; 34+ messages in thread
From: Ville Syrjälä @ 2016-10-27 13:03 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:31PM +0200, Maarten Lankhorst wrote:
> This will allow us to find all allocations that may have changed,

What is the "this" you are referring to?

Ie. please don't treat the subject line as part of the commit message
itself, because it's confusing.

> not just the one added by the atomic state.
> 
> This is required to stop adding planes to state when its
> allocation changes, and is useful for finding bugs.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bdb69582e7c5..c1520afb2360 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4090,41 +4090,35 @@ skl_print_wm_changes(const struct drm_atomic_state *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;
> +	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {

So what you're saying is that all crtcs whose DDB allocation may have
changed are part of the state, but not all planes on those crtcs may be
part of the state?

>  		pipe = to_intel_crtc(crtc)->pipe;
>  
> -		for_each_plane_in_state(state, plane, pstate, j) {
> +		for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) {
>  			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,
> +						 intel_plane->base.base.id, id + 1,

Slightly unrelated, but
 "[PLANE:%d:%s] ...", plane->base.id, plane->name
and then you can kill off this cursor special case entirely.

We should probably make some macros for those things so pople wouldn't
have to write them by hand.

>  						 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,
> +						 intel_plane->base.base.id,
>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state.
  2016-10-26 13:41 ` [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state Maarten Lankhorst
@ 2016-10-27 14:50   ` Matt Roper
  0 siblings, 0 replies; 34+ messages in thread
From: Matt Roper @ 2016-10-27 14:50 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:30PM +0200, Maarten Lankhorst wrote:
> I'm planning on getting rid of all obj->state dereferences,
> and replace thhem with accessor functions.
> Remove this one early, they're equivalent because removed
> planes are already part of the state, else they could not
> have been removed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 044a8c932a1c..bdb69582e7c5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3975,7 +3975,7 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
>  
>  	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
>  
> -	drm_for_each_plane_mask(plane, dev, crtc->state->plane_mask) {
> +	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
>  		id = skl_wm_plane_id(to_intel_plane(plane));
>  
>  		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][id],
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
  2016-10-26 13:41 ` [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes Maarten Lankhorst
  2016-10-27 13:03   ` Ville Syrjälä
@ 2016-10-27 17:47   ` Matt Roper
  2016-10-27 17:57     ` Paulo Zanoni
  1 sibling, 1 reply; 34+ messages in thread
From: Matt Roper @ 2016-10-27 17:47 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Wed, Oct 26, 2016 at 03:41:31PM +0200, Maarten Lankhorst wrote:
> This will allow us to find all allocations that may have changed,
> not just the one added by the atomic state.
> 
> This is required to stop adding planes to state when its
> allocation changes, and is useful for finding bugs.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Makes sense; with Ville's commit message suggestion acted upon, this is

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bdb69582e7c5..c1520afb2360 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4090,41 +4090,35 @@ skl_print_wm_changes(const struct drm_atomic_state *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;
> +	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		pipe = to_intel_crtc(crtc)->pipe;
>  
> -		for_each_plane_in_state(state, plane, pstate, j) {
> +		for_each_intel_plane_on_crtc(dev, to_intel_crtc(crtc), intel_plane) {
>  			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,
> +						 intel_plane->base.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,
> +						 intel_plane->base.base.id,
>  						 pipe_name(pipe),
>  						 old->start, old->end,
>  						 new->start, new->end);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  2016-10-27  8:31     ` Maarten Lankhorst
@ 2016-10-27 17:48       ` Paulo Zanoni
  2016-10-27 17:55         ` Matt Roper
  0 siblings, 1 reply; 34+ messages in thread
From: Paulo Zanoni @ 2016-10-27 17:48 UTC (permalink / raw)
  To: Maarten Lankhorst, Daniel Vetter; +Cc: intel-gfx

Em Qui, 2016-10-27 às 10:31 +0200, Maarten Lankhorst escreveu:
> Op 27-10-16 om 09:12 schreef Daniel Vetter:
> > 
> > On Wed, Oct 26, 2016 at 03:41:29PM +0200, Maarten Lankhorst wrote:
> > > 
> > > Changes since v1:
> > > - Remove plane->pipe checks, they're implied by the macros.
> > > - Split unrelated changes to a separate commit.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > om>
> > Drive-by review: The above commit message is way too thin. It
> > prepares for
> > something, but I can't make any connection at all to that
> > something. 1-2
> > sentences about why are needed here I think.
> 
> Hm, seems to have been lost somewhere. It was on my previous
> submission:
> 
> Caching is not required, drm_atomic_crtc_state_for_each_plane_state
> can be used to inspect all plane_states that are assigned to the
> current crtc_state, so we can just recalculate every time.

My review of v1 also complained about the commit message, so I got
surprised by seeing it just go away in v2. 

If my understanding of the code is correct, I think the text would make
more sense if it was rewritten to:

Caching is not required, drm_atomic_crtc_state_for_each_plane_state can
be used to inspect the states of all planes assigned to the CRTC even
if they are not part of crtc_state, so we can just recalculate every
time.

But feel free to not use the text above if you think it's not
completely accurate.

With the commit message problem solved somehow:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> ~Maarten
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2.
  2016-10-27 17:48       ` Paulo Zanoni
@ 2016-10-27 17:55         ` Matt Roper
  0 siblings, 0 replies; 34+ messages in thread
From: Matt Roper @ 2016-10-27 17:55 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Oct 27, 2016 at 03:48:19PM -0200, Paulo Zanoni wrote:
> Em Qui, 2016-10-27 às 10:31 +0200, Maarten Lankhorst escreveu:
> > Op 27-10-16 om 09:12 schreef Daniel Vetter:
> > > 
> > > On Wed, Oct 26, 2016 at 03:41:29PM +0200, Maarten Lankhorst wrote:
> > > > 
> > > > Changes since v1:
> > > > - Remove plane->pipe checks, they're implied by the macros.
> > > > - Split unrelated changes to a separate commit.
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > > om>
> > > Drive-by review: The above commit message is way too thin. It
> > > prepares for
> > > something, but I can't make any connection at all to that
> > > something. 1-2
> > > sentences about why are needed here I think.
> > 
> > Hm, seems to have been lost somewhere. It was on my previous
> > submission:
> > 
> > Caching is not required, drm_atomic_crtc_state_for_each_plane_state
> > can be used to inspect all plane_states that are assigned to the
> > current crtc_state, so we can just recalculate every time.
> 
> My review of v1 also complained about the commit message, so I got
> surprised by seeing it just go away in v2. 
> 
> If my understanding of the code is correct, I think the text would make
> more sense if it was rewritten to:
> 
> Caching is not required, drm_atomic_crtc_state_for_each_plane_state can
> be used to inspect the states of all planes assigned to the CRTC even
> if they are not part of crtc_state, so we can just recalculate every
                          ^^^^^^^^^^
This should refer to the top-level state object (i.e.,
drm_atomic_state), not crtc_state.  But otherwise I agree that this text
sounds good.

With the missing commit message fixed,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> time.
> 
> But feel free to not use the text above if you think it's not
> completely accurate.
> 
> With the commit message problem solved somehow:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > 
> > ~Maarten
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes
  2016-10-27 17:47   ` Matt Roper
@ 2016-10-27 17:57     ` Paulo Zanoni
  2016-11-01 11:04       ` [PATCH v2.1 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes, v2 Maarten Lankhorst
  0 siblings, 1 reply; 34+ messages in thread
From: Paulo Zanoni @ 2016-10-27 17:57 UTC (permalink / raw)
  To: Matt Roper, Maarten Lankhorst; +Cc: intel-gfx

Em Qui, 2016-10-27 às 10:47 -0700, Matt Roper escreveu:
> On Wed, Oct 26, 2016 at 03:41:31PM +0200, Maarten Lankhorst wrote:
> > 
> > This will allow us to find all allocations that may have changed,
> > not just the one added by the atomic state.
> > 
> > This is required to stop adding planes to state when its
> > allocation changes, and is useful for finding bugs.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > >
> 
> Makes sense; with Ville's commit message suggestion acted upon, this
> is
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> 
> Matt
> 
> > 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index bdb69582e7c5..c1520afb2360 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4090,41 +4090,35 @@ skl_print_wm_changes(const struct
> > drm_atomic_state *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;
> > +	int i;
> >  
> >  	for_each_crtc_in_state(state, crtc, cstate, i) {
> >  		pipe = to_intel_crtc(crtc)->pipe;
> >  
> > -		for_each_plane_in_state(state, plane, pstate, j) {
> > +		for_each_intel_plane_on_crtc(dev,
> > to_intel_crtc(crtc), intel_plane) {

We could stay under 80 columns if you just declare the intel_crtc above
:)

> >  			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,
> > +						 intel_plane-
> > >base.base.id, id + 1,

Moving the id + 1 to the line below would allow us to stay under 80
columns.

My bikesheds are optional. With Matt's requests acted upon:
Reviewed-by: 
Paulo Zanoni <paulo.r.zanoni@intel.com>

> >  						 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,
> > +						 intel_plane-
> > >base.base.id,
> >  						 pipe_name(pipe),
> >  						 old->start, old-
> > >end,
> >  						 new->start, new-
> > >end);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2.
  2016-10-26 13:41 ` [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2 Maarten Lankhorst
@ 2016-10-27 18:14   ` Paulo Zanoni
  0 siblings, 0 replies; 34+ messages in thread
From: Paulo Zanoni @ 2016-10-27 18:14 UTC (permalink / raw)
  To: Maarten Lankhorst, intel-gfx

Em Qua, 2016-10-26 às 15:41 +0200, Maarten Lankhorst escreveu:
> It's only used in one function, and can be calculated without caching
> it
> in the global struct by using
> drm_atomic_crtc_state_for_each_plane_state.
> 
> There are loops over all planes, including planes that don't exist.
> This is harmless, because data_rate will always be 0 for them and we
> never program them when updating watermarks.
> 
> Changes since v1:
> - Rename rate back to data_rate, and change array name to
>   plane_data_rate. (Matt)
> - Remove whitespace. (Paulo)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'd still prefer if you had used a safer way to loop through the
planes, not something that may just break when someone decides refactor
this code for the Nth time in the future and forget that it only works
because we zero-initialize things and then rely on multiplication by
zero. Since I'm not going to be able to convince you, and the code is
correct today:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>  drivers/gpu/drm/i915/intel_pm.c  | 35 ++++++++++++++++------------
> -------
>  2 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 7dda79df55d0..d0485457b335 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state {
>  			struct skl_pipe_wm optimal;
>  			struct skl_ddb_entry ddb;
>  
> -			/* cached plane data rate */
> -			unsigned plane_data_rate[I915_MAX_PLANES];
> -			unsigned plane_y_data_rate[I915_MAX_PLANES];
> -
>  			/* minimum block allocation */
>  			uint16_t minimum_blocks[I915_MAX_PLANES];
>  			uint16_t minimum_y_blocks[I915_MAX_PLANES];
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index c1520afb2360..0c199c51dad9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3263,13 +3263,12 @@ skl_plane_relative_data_rate(const struct
> intel_crtc_state *cstate,
>   *   3 * 4096 * 8192  * 4 < 2^32
>   */
>  static unsigned int
> -skl_get_total_relative_data_rate(struct intel_crtc_state
> *intel_cstate)
> +skl_get_total_relative_data_rate(struct intel_crtc_state
> *intel_cstate,
> +				 unsigned *plane_data_rate,
> +				 unsigned *plane_y_data_rate)
>  {
>  	struct drm_crtc_state *cstate = &intel_cstate->base;
>  	struct drm_atomic_state *state = cstate->state;
> -	struct drm_crtc *crtc = cstate->crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane;
>  	const struct intel_plane *intel_plane;
>  	const struct drm_plane_state *pstate;
> @@ -3287,21 +3286,16 @@ skl_get_total_relative_data_rate(struct
> intel_crtc_state *intel_cstate)
>  		/* packed/uv */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 0);
> -		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> +		plane_data_rate[id] = rate;
> +
> +		total_data_rate += rate;
>  
>  		/* y-plane */
>  		rate = skl_plane_relative_data_rate(intel_cstate,
>  						    pstate, 1);
> -		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> -	}
> +		plane_y_data_rate[id] = rate;
>  
> -	/* Calculate CRTC's total data rate from cached values */
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		int id = skl_wm_plane_id(intel_plane);
> -
> -		/* packed/uv */
> -		total_data_rate += intel_cstate-
> >wm.skl.plane_data_rate[id];
> -		total_data_rate += intel_cstate-
> >wm.skl.plane_y_data_rate[id];
> +		total_data_rate += rate;
>  	}
>  
>  	return total_data_rate;
> @@ -3389,6 +3383,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	unsigned int total_data_rate;
>  	int num_active;
>  	int id, i;
> +	unsigned plane_data_rate[I915_MAX_PLANES] = {};
> +	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
>  
>  	/* Clear the partitioning for disabled planes. */
>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3446,17 +3442,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	 *
>  	 * FIXME: we may not allocate every single block here.
>  	 */
> -	total_data_rate = skl_get_total_relative_data_rate(cstate);
> +	total_data_rate = skl_get_total_relative_data_rate(cstate,
> +							   plane_dat
> a_rate,
> +							   plane_y_d
> ata_rate);
>  	if (total_data_rate == 0)
>  		return 0;
>  
>  	start = alloc->start;
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> +	for (id = 0; id < I915_MAX_PLANES; id++) {
>  		unsigned int data_rate, y_data_rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
> -		int id = skl_wm_plane_id(intel_plane);
>  
> -		data_rate = cstate->wm.skl.plane_data_rate[id];
> +		data_rate = plane_data_rate[id];
>  
>  		/*
>  		 * allocation for (packed formats) or (uv-plane part
> of planar format):
> @@ -3478,7 +3475,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		/*
>  		 * allocation for y_plane part of planar format:
>  		 */
> -		y_data_rate = cstate->wm.skl.plane_y_data_rate[id];
> +		y_data_rate = plane_y_data_rate[id];
>  
>  		y_plane_blocks = y_minimum[id];
>  		y_plane_blocks += div_u64((uint64_t)alloc_size *
> y_data_rate,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2.
  2016-10-26 13:41 ` [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2 Maarten Lankhorst
@ 2016-10-27 19:25   ` Paulo Zanoni
  0 siblings, 0 replies; 34+ messages in thread
From: Paulo Zanoni @ 2016-10-27 19:25 UTC (permalink / raw)
  To: Maarten Lankhorst, intel-gfx

Em Qua, 2016-10-26 às 15:41 +0200, Maarten Lankhorst escreveu:
> Move calculating minimum allocations to a helper, which cleans up the
> code some more. The cursor is still allocated in advance because it
> doesn't count towards data rate and should always be reserved.
> 
> changes since v1:
> - Change comment to have a extra opening line. (Matt)
> - Rebase to remove unused plane->pipe == pipe, handled by the
> iterator
>   now. (Paulo)
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 62 +++++++++++++++++++++++++----
> ------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index c2b965d36ead..41953cc41809 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3364,6 +3364,30 @@ skl_ddb_min_alloc(const struct drm_plane_state
> *pstate,
>  	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) *
> min_scanlines/4 + 3;
>  }
>  
> +static void
> +skl_ddb_calc_min(const struct intel_crtc_state *cstate, int
> num_active,
> +		 uint16_t *minimum, uint16_t *y_minimum)
> +{
> +	const struct drm_plane_state *pstate;
> +	struct drm_plane *plane;
> +
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> +		struct intel_plane *intel_plane =
> to_intel_plane(plane);
> +		int id = skl_wm_plane_id(intel_plane);
> +
> +		if (id == PLANE_CURSOR)
> +			continue;
> +
> +		if (!pstate->visible)
> +			continue;
> +
> +		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> +		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> +	}
> +
> +	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
> @@ -3372,12 +3396,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> -	uint16_t alloc_size, start, cursor_blocks;
> +	uint16_t alloc_size, start;
>  	uint16_t minimum[I915_MAX_PLANES] = {};
>  	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
> @@ -3405,32 +3426,22 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;
> -
> -	/* 1. Allocate the mininum required blocks for each active
> plane */
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> -		intel_plane = to_intel_plane(plane);
> -		id = skl_wm_plane_id(intel_plane);
> -
> -		if (!pstate->visible)
> -			continue;
> -
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -			continue;
> +	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
>  
> -		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> -		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> -	}
> +	/*
> +	 * 1. Allocate the mininum required blocks for each active
> plane
> +	 * and allocate the cursor, it doesn't require extra
> allocation
> +	 * proportional to the data rate.
> +	 */
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {
>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
>  
> +	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> minimum[PLANE_CURSOR];
> +	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> +
>  	/*
>  	 * 2. Distribute the remaining space in proportion to the
> amount of
>  	 * data each plane needs to fetch from memory.
> @@ -3448,6 +3459,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		unsigned int data_rate, y_data_rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
>  
> +		if (id == PLANE_CURSOR)
> +			continue;
> +
>  		data_rate = plane_data_rate[id];
>  
>  		/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH v2.1 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes, v2.
  2016-10-27 17:57     ` Paulo Zanoni
@ 2016-11-01 11:04       ` Maarten Lankhorst
  2016-11-01 12:33         ` Paulo Zanoni
  0 siblings, 1 reply; 34+ messages in thread
From: Maarten Lankhorst @ 2016-11-01 11:04 UTC (permalink / raw)
  To: Paulo Zanoni, Matt Roper; +Cc: intel-gfx

Using for_each_intel_plane_on_crtc will allow us to find all allocations
that may have changed, not just the one added by the atomic state.

This will print changes to plane allocations for crtc's when some
planes are not added to the atomic state.

Changes since v1:
- Rephrase commit message. (Ville)
- Use plane->base.id and plane->name to kill off cursor special
  case. (Matt)
- Add intel_crtc to prevent a line wrap. (Paulo)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Could I get a quick ack this version is good? Then I'll commit it.

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e6e9cc563484..84db162cd30a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4090,45 +4090,31 @@ skl_print_wm_changes(const struct drm_atomic_state *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;
+	int i;
 
 	for_each_crtc_in_state(state, crtc, cstate, i) {
-		pipe = to_intel_crtc(crtc)->pipe;
+		const struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		pipe = intel_crtc->pipe;
 
-		for_each_plane_in_state(state, plane, pstate, j) {
+		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 			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);
-			}
+			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] ddb (%d - %d) -> (%d - %d)\n",
+					 intel_plane->base.base.id, intel_plane->base.name,
+					 old->start, old->end,
+					 new->start, new->end);
 		}
 	}
 }

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH v2.1 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes, v2.
  2016-11-01 11:04       ` [PATCH v2.1 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes, v2 Maarten Lankhorst
@ 2016-11-01 12:33         ` Paulo Zanoni
  0 siblings, 0 replies; 34+ messages in thread
From: Paulo Zanoni @ 2016-11-01 12:33 UTC (permalink / raw)
  To: Maarten Lankhorst, Matt Roper; +Cc: intel-gfx

Em Ter, 2016-11-01 às 12:04 +0100, Maarten Lankhorst escreveu:
> Using for_each_intel_plane_on_crtc will allow us to find all
> allocations
> that may have changed, not just the one added by the atomic state.
> 
> This will print changes to plane allocations for crtc's when some
> planes are not added to the atomic state.
> 
> Changes since v1:
> - Rephrase commit message. (Ville)
> - Use plane->base.id and plane->name to kill off cursor special
>   case. (Matt)

Wasn't this Ville's suggestion? :)

> - Add intel_crtc to prevent a line wrap. (Paulo)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Could I get a quick ack this version is good? Then I'll commit it.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index e6e9cc563484..84db162cd30a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4090,45 +4090,31 @@ skl_print_wm_changes(const struct
> drm_atomic_state *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;
> +	int i;
>  
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
> -		pipe = to_intel_crtc(crtc)->pipe;
> +		const struct intel_crtc *intel_crtc =
> to_intel_crtc(crtc);
> +		pipe = intel_crtc->pipe;
>  
> -		for_each_plane_in_state(state, plane, pstate, j) {
> +		for_each_intel_plane_on_crtc(dev, intel_crtc,
> intel_plane) {
>  			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);
> -			}
> +			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] ddb (%d -
> %d) -> (%d - %d)\n",
> +					 intel_plane->base.base.id,
> intel_plane->base.name,
> +					 old->start, old->end,
> +					 new->start, new->end);
>  		}
>  	}
>  }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2016-11-01 12:33 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-26 13:41 [PATCH v2 00/11] drm/i915/gen9+: Atomic watermark fixes Maarten Lankhorst
2016-10-26 13:41 ` [PATCH v2 01/11] drm/i915/skl+: Prepare for removing data rate from skl watermark state, v2 Maarten Lankhorst
2016-10-27  7:12   ` Daniel Vetter
2016-10-27  8:31     ` Maarten Lankhorst
2016-10-27 17:48       ` Paulo Zanoni
2016-10-27 17:55         ` Matt Roper
2016-10-26 13:41 ` [PATCH v2 02/11] drm/i915/gen9+: Use cstate plane mask instead of crtc->state Maarten Lankhorst
2016-10-27 14:50   ` Matt Roper
2016-10-26 13:41 ` [PATCH v2 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes Maarten Lankhorst
2016-10-27 13:03   ` Ville Syrjälä
2016-10-27 17:47   ` Matt Roper
2016-10-27 17:57     ` Paulo Zanoni
2016-11-01 11:04       ` [PATCH v2.1 03/11] drm/i915/gen9+: Use for_each_intel_plane_on_crtc in skl_print_wm_changes, v2 Maarten Lankhorst
2016-11-01 12:33         ` Paulo Zanoni
2016-10-26 13:41 ` [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2 Maarten Lankhorst
2016-10-27 18:14   ` Paulo Zanoni
2016-10-26 13:41 ` [PATCH v2 05/11] drm/i915/skl+: Remove minimum block allocation from crtc state Maarten Lankhorst
2016-10-26 13:41 ` [PATCH v2 06/11] drm/i915/skl+: Clean up minimum allocations, v2 Maarten Lankhorst
2016-10-27 19:25   ` Paulo Zanoni
2016-10-26 13:41 ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Maarten Lankhorst
2016-10-26 16:19   ` Ville Syrjälä
2016-10-27  7:10     ` Daniel Vetter
2016-10-27 10:37     ` [PATCH v2.1 07/11] drm/i915: Add a atomic evasion step to watermark programming, v3 Maarten Lankhorst
2016-10-26 23:10   ` [PATCH v2 07/11] drm/i915: Add a atomic evasion step to watermark programming, v2 Matt Roper
2016-10-26 13:41 ` [PATCH v2 08/11] drm/i915/gen9+: Use the watermarks from crtc_state for everything, v2 Maarten Lankhorst
2016-10-26 23:13   ` Matt Roper
2016-10-26 13:41 ` [PATCH v2 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v2 Maarten Lankhorst
2016-10-26 16:05   ` Lyude Paul
2016-10-26 23:24   ` Matt Roper
2016-10-27  8:29     ` Maarten Lankhorst
2016-10-27 11:45       ` [PATCH v2.1 09/11] drm/i915/gen9+: Program watermarks as a separate step during evasion, v3 Maarten Lankhorst
2016-10-26 13:41 ` [PATCH v2 10/11] drm/i915/gen9+: Preserve old allocation from crtc_state Maarten Lankhorst
2016-10-26 13:41 ` [PATCH v2 11/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc Maarten Lankhorst
2016-10-26 14:16 ` ✓ Fi.CI.BAT: success for drm/i915/gen9+: Atomic watermark fixes Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.