[PATCH v3 00/16] Pre-calculate SKL-style atomic watermarks

[PATCH v3 00/16] Pre-calculate SKL-style atomic watermarks

[Matt Roper]

For a detailed explanation of this series, please see the original cover letter:
  https://lists.freedesktop.org/archives/intel-gfx/2016-April/091293.html

This spin just incorporates the final review feedback from Maarten and
hopefully kicks the CI system (I never got any CI test results back from v2).


Matt Roper (16):
  drm/i915: Reorganize WM structs/unions in CRTC state
  drm/i915: Rename s/skl_compute_pipe_wm/skl_build_pipe_wm/
  drm/i915/gen9: Cache plane data rates in CRTC state
  drm/i915/gen9: Allow calculation of data rate for in-flight state (v2)
  drm/i915/gen9: Store plane minimum blocks in CRTC wm state (v2)
  drm/i915: Track whether an atomic transaction changes the active
    CRTC's
  drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight
    state (v3)
  drm/i915/gen9: Compute DDB allocation at atomic check time (v2)
  drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)
  drm/i915/gen9: Calculate plane WM's from state
  drm/i915/gen9: Allow watermark calculation on in-flight atomic state
    (v3)
  drm/i915/gen9: Use a bitmask to track dirty pipe watermarks
  drm/i915/gen9: Propagate watermark calculation failures up the call
    chain
  drm/i915/gen9: Calculate watermarks during atomic 'check'
  drm/i915/gen9: Reject display updates that exceed wm limitations
  drm/i915: Remove wm_config from dev_priv/intel_atomic_state

 drivers/gpu/drm/i915/i915_drv.h      |  16 +-
 drivers/gpu/drm/i915/intel_display.c |  44 +--
 drivers/gpu/drm/i915/intel_drv.h     |  83 +++--
 drivers/gpu/drm/i915/intel_pm.c      | 586 ++++++++++++++++++++++-------------
 4 files changed, 455 insertions(+), 274 deletions(-)

-- 
2.1.4

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

[PATCH v3 01/16] drm/i915: Reorganize WM structs/unions in CRTC state

[Matt Roper]

Reorganize the nested structures and unions we have for pipe watermark
data in intel_crtc_state so that platform-specific data can be added in
a more sensible manner (and save a bit of memory at the same time).

The change basically changes the organization from:

        union {
                struct intel_pipe_wm ilk;
                struct intel_pipe_wm skl;
        } optimal;

        struct intel_pipe_wm intermediate /* ILK-only */

to

        union {
                struct {
                        struct intel_pipe_wm intermediate;
                        struct intel_pipe_wm optimal;
                } ilk;

                struct {
                        struct intel_pipe_wm optimal;
                } skl;
        }

There should be no functional change here, but it will allow us to add
more platform-specific fields going forward (and more easily extend to
other platform types like VLV).

While we're at it, let's move the entire watermark substructure out to
its own structure definition to make the code slightly more readable.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 61 +++++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_pm.c  | 18 ++++++------
 2 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index beed9e8..a94d651a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -404,6 +404,40 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
+struct intel_crtc_wm_state {
+	union {
+		struct {
+			/*
+			 * Intermediate watermarks; these can be
+			 * programmed immediately since they satisfy
+			 * both the current configuration we're
+			 * switching away from and the new
+			 * configuration we're switching to.
+			 */
+			struct intel_pipe_wm intermediate;
+
+			/*
+			 * Optimal watermarks, programmed post-vblank
+			 * when this state is committed.
+			 */
+			struct intel_pipe_wm optimal;
+		} ilk;
+
+		struct {
+			/* gen9+ only needs 1-step wm programming */
+			struct skl_pipe_wm optimal;
+		} skl;
+	};
+
+	/*
+	 * Platforms with two-step watermark programming will need to
+	 * update watermark programming post-vblank to switch from the
+	 * safe intermediate watermarks to the optimal final
+	 * watermarks.
+	 */
+	bool need_postvbl_update;
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -557,32 +591,7 @@ struct intel_crtc_state {
 	/* IVB sprite scaling w/a (WaCxSRDisabledForSpriteScaling:ivb) */
 	bool disable_lp_wm;
 
-	struct {
-		/*
-		 * Optimal watermarks, programmed post-vblank when this state
-		 * is committed.
-		 */
-		union {
-			struct intel_pipe_wm ilk;
-			struct skl_pipe_wm skl;
-		} optimal;
-
-		/*
-		 * Intermediate watermarks; these can be programmed immediately
-		 * since they satisfy both the current configuration we're
-		 * switching away from and the new configuration we're switching
-		 * to.
-		 */
-		struct intel_pipe_wm intermediate;
-
-		/*
-		 * Platforms with two-step watermark programming will need to
-		 * update watermark programming post-vblank to switch from the
-		 * safe intermediate watermarks to the optimal final
-		 * watermarks.
-		 */
-		bool need_postvbl_update;
-	} wm;
+	struct intel_crtc_wm_state wm;
 
 	/* Gamma mode programmed on the pipe */
 	uint32_t gamma_mode;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c2186..24b4b83 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2309,7 +2309,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 	int level, max_level = ilk_wm_max_level(dev), usable_level;
 	struct ilk_wm_maximums max;
 
-	pipe_wm = &cstate->wm.optimal.ilk;
+	pipe_wm = &cstate->wm.ilk.optimal;
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		struct intel_plane_state *ps;
@@ -2391,7 +2391,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
 				       struct intel_crtc_state *newstate)
 {
-	struct intel_pipe_wm *a = &newstate->wm.intermediate;
+	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
 	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
 	int level, max_level = ilk_wm_max_level(dev);
 
@@ -2400,7 +2400,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 	 * currently active watermarks to get values that are safe both before
 	 * and after the vblank.
 	 */
-	*a = newstate->wm.optimal.ilk;
+	*a = newstate->wm.ilk.optimal;
 	a->pipe_enabled |= b->pipe_enabled;
 	a->sprites_enabled |= b->sprites_enabled;
 	a->sprites_scaled |= b->sprites_scaled;
@@ -2429,7 +2429,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 	 * If our intermediate WM are identical to the final WM, then we can
 	 * omit the post-vblank programming; only update if it's different.
 	 */
-	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
+	if (memcmp(a, &newstate->wm.ilk.optimal, sizeof(*a)) == 0)
 		newstate->wm.need_postvbl_update = false;
 
 	return 0;
@@ -3678,7 +3678,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct skl_pipe_wm *pipe_wm = &cstate->wm.optimal.skl;
+	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
 
 
 	/* Clear all dirty flags */
@@ -3757,7 +3757,7 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
-	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
+	intel_crtc->wm.active.ilk = cstate->wm.ilk.intermediate;
 	ilk_program_watermarks(dev_priv);
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
@@ -3769,7 +3769,7 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 	if (cstate->wm.need_postvbl_update) {
-		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
+		intel_crtc->wm.active.ilk = cstate->wm.ilk.optimal;
 		ilk_program_watermarks(dev_priv);
 	}
 	mutex_unlock(&dev_priv->wm.wm_mutex);
@@ -3826,7 +3826,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct skl_pipe_wm *active = &cstate->wm.optimal.skl;
+	struct skl_pipe_wm *active = &cstate->wm.skl.optimal;
 	enum pipe pipe = intel_crtc->pipe;
 	int level, i, max_level;
 	uint32_t temp;
@@ -3892,7 +3892,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	struct ilk_wm_values *hw = &dev_priv->wm.hw;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-	struct intel_pipe_wm *active = &cstate->wm.optimal.ilk;
+	struct intel_pipe_wm *active = &cstate->wm.ilk.optimal;
 	enum pipe pipe = intel_crtc->pipe;
 	static const i915_reg_t wm0_pipe_reg[] = {
 		[PIPE_A] = WM0_PIPEA_ILK,
-- 
2.1.4

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

[PATCH v3 02/16] drm/i915: Rename s/skl_compute_pipe_wm/skl_build_pipe_wm/

[Matt Roper]

When we added atomic watermarks, we added a new display vfunc
'compute_pipe_wm' that is used to compute any pipe-specific watermark
information that we can at atomic check time.  This was a somewhat poor
naming choice since we already had a 'skl_compute_pipe_wm' function that
doesn't quite fit this model --- the existing SKL function is something
that gets used at atomic commit time, after the DDB allocation has been
determined.  Let's rename the existing SKL function to avoid confusion.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 24b4b83..7a4dc9d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3327,9 +3327,9 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 	}
 }
 
-static void skl_compute_pipe_wm(struct intel_crtc_state *cstate,
-				struct skl_ddb_allocation *ddb,
-				struct skl_pipe_wm *pipe_wm)
+static void skl_build_pipe_wm(struct intel_crtc_state *cstate,
+			      struct skl_ddb_allocation *ddb,
+			      struct skl_pipe_wm *pipe_wm)
 {
 	struct drm_device *dev = cstate->base.crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3596,7 +3596,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
 	skl_allocate_pipe_ddb(cstate, ddb);
-	skl_compute_pipe_wm(cstate, ddb, pipe_wm);
+	skl_build_pipe_wm(cstate, ddb, pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
 		return false;
-- 
2.1.4

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

[PATCH v3 03/16] drm/i915/gen9: Cache plane data rates in CRTC state

[Matt Roper]

This will be important when we start calculating CRTC data rates for
in-flight CRTC states since it will allow us to calculate the total data
rate without needing to grab the plane state for any planes that aren't
updated by the transaction.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ++
 drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++--------------
 2 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a94d651a..e67438f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -426,6 +426,10 @@ struct intel_crtc_wm_state {
 		struct {
 			/* gen9+ only needs 1-step wm programming */
 			struct skl_pipe_wm optimal;
+
+			/* cached plane data rate */
+			unsigned plane_data_rate[I915_MAX_PLANES];
+			unsigned plane_y_data_rate[I915_MAX_PLANES];
 		} skl;
 	};
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7a4dc9d..7e4b149 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2940,6 +2940,14 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
 	struct drm_framebuffer *fb = pstate->fb;
 	uint32_t width = 0, height = 0;
+	unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
+
+	if (!intel_pstate->visible)
+		return 0;
+	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
+		return 0;
+	if (y && format != DRM_FORMAT_NV12)
+		return 0;
 
 	width = drm_rect_width(&intel_pstate->src) >> 16;
 	height = drm_rect_height(&intel_pstate->src) >> 16;
@@ -2948,17 +2956,17 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 		swap(width, height);
 
 	/* for planar format */
-	if (fb->pixel_format == DRM_FORMAT_NV12) {
+	if (format == DRM_FORMAT_NV12) {
 		if (y)  /* y-plane data rate */
 			return width * height *
-				drm_format_plane_cpp(fb->pixel_format, 0);
+				drm_format_plane_cpp(format, 0);
 		else    /* uv-plane data rate */
 			return (width / 2) * (height / 2) *
-				drm_format_plane_cpp(fb->pixel_format, 1);
+				drm_format_plane_cpp(format, 1);
 	}
 
 	/* for packed formats */
-	return width * height * drm_format_plane_cpp(fb->pixel_format, 0);
+	return width * height * drm_format_plane_cpp(format, 0);
 }
 
 /*
@@ -2967,32 +2975,34 @@ 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(const struct intel_crtc_state *cstate)
+skl_get_total_relative_data_rate(struct intel_crtc_state *cstate)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_device *dev = intel_crtc->base.dev;
 	const struct intel_plane *intel_plane;
-	unsigned int total_data_rate = 0;
+	unsigned int rate, total_data_rate = 0;
 
+	/* Calculate and cache data rate for each plane */
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		const struct drm_plane_state *pstate = intel_plane->base.state;
+		int id = skl_wm_plane_id(intel_plane);
 
-		if (pstate->fb == NULL)
-			continue;
+		/* packed/uv */
+		rate = skl_plane_relative_data_rate(cstate, pstate, 0);
+		cstate->wm.skl.plane_data_rate[id] = rate;
 
-		if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
-			continue;
+		/* y-plane */
+		rate = skl_plane_relative_data_rate(cstate, pstate, 1);
+		cstate->wm.skl.plane_y_data_rate[id] = rate;
+	}
 
-		/* packed/uv */
-		total_data_rate += skl_plane_relative_data_rate(cstate,
-								pstate,
-								0);
+	/* 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);
 
-		if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
-			/* y-plane */
-			total_data_rate += skl_plane_relative_data_rate(cstate,
-									pstate,
-									1);
+		/* packed/uv */
+		total_data_rate += cstate->wm.skl.plane_data_rate[id];
+		total_data_rate += cstate->wm.skl.plane_y_data_rate[id];
 	}
 
 	return total_data_rate;
@@ -3056,6 +3066,8 @@ 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);
+	if (total_data_rate == 0)
+		return;
 
 	start = alloc->start;
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
@@ -3070,7 +3082,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		if (plane->type == DRM_PLANE_TYPE_CURSOR)
 			continue;
 
-		data_rate = skl_plane_relative_data_rate(cstate, pstate, 0);
+		data_rate = cstate->wm.skl.plane_data_rate[id];
 
 		/*
 		 * allocation for (packed formats) or (uv-plane part of planar format):
@@ -3089,20 +3101,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		/*
 		 * allocation for y_plane part of planar format:
 		 */
-		if (pstate->fb->pixel_format == DRM_FORMAT_NV12) {
-			y_data_rate = skl_plane_relative_data_rate(cstate,
-								   pstate,
-								   1);
-			y_plane_blocks = y_minimum[id];
-			y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
-						total_data_rate);
-
-			ddb->y_plane[pipe][id].start = start;
-			ddb->y_plane[pipe][id].end = start + y_plane_blocks;
-
-			start += y_plane_blocks;
-		}
+		y_data_rate = cstate->wm.skl.plane_y_data_rate[id];
+
+		y_plane_blocks = y_minimum[id];
+		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
+					total_data_rate);
 
+		ddb->y_plane[pipe][id].start = start;
+		ddb->y_plane[pipe][id].end = start + y_plane_blocks;
+
+		start += y_plane_blocks;
 	}
 
 }
@@ -3879,10 +3887,28 @@ void skl_wm_get_hw_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb;
 	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
 
 	skl_ddb_get_hw_state(dev_priv, ddb);
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
 		skl_pipe_wm_get_hw_state(crtc);
+
+	/* Calculate plane data rates */
+	for_each_intel_crtc(dev, intel_crtc) {
+		struct intel_crtc_state *cstate = intel_crtc->config;
+		struct intel_plane *intel_plane;
+
+		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+			const struct drm_plane_state *pstate =
+				intel_plane->base.state;
+			int id = skl_wm_plane_id(intel_plane);
+
+			cstate->wm.skl.plane_data_rate[id] =
+				skl_plane_relative_data_rate(cstate, pstate, 0);
+			cstate->wm.skl.plane_y_data_rate[id] =
+				skl_plane_relative_data_rate(cstate, pstate, 1);
+		}
+	}
 }
 
 static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
-- 
2.1.4

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

[PATCH v3 04/16] drm/i915/gen9: Allow calculation of data rate for in-flight state (v2)

[Matt Roper]

Our skl_get_total_relative_data_rate() function gets passed a crtc state
object to calculate the data rate for, but it currently always looks
up the committed plane states that correspond to that CRTC.  Let's
check whether the CRTC state is an in-flight state (meaning
cstate->state is non-NULL) and if so, use the corresponding in-flight
plane states.

We'll soon be using this function exclusively for in-flight states; at
that time we'll be able to simplify the function a bit, but for now we
allow it to be used in either mode.

v2:
 - Rebase on top of changes to cache plane data rates.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 74 +++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7e4b149..39ab1cb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2975,25 +2975,69 @@ 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 *cstate)
+skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_device *dev = intel_crtc->base.dev;
+	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);
 	const struct intel_plane *intel_plane;
 	unsigned int rate, total_data_rate = 0;
+	int id;
 
 	/* Calculate and cache data rate for each plane */
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		const struct drm_plane_state *pstate = intel_plane->base.state;
-		int id = skl_wm_plane_id(intel_plane);
+	/*
+	 * FIXME: At the moment this function can be called on either an
+	 * in-flight or a committed state object.  If it's in-flight then we
+	 * only want to re-calculate the plane data rate for planes that are
+	 * part of the transaction (i.e., we don't want to grab any additional
+	 * plane states if we don't have to).  If we're operating on committed
+	 * state, we'll just go ahead and recalculate the plane data rate for
+	 * all planes.
+	 *
+	 * Once we finish moving our DDB allocation to the atomic check phase,
+	 * we'll only be calling this function on in-flight state objects, so
+	 * the 'else' branch here will go away.
+	 */
+	if (state) {
+		struct drm_plane *plane;
+		struct drm_plane_state *pstate;
+		int i;
+
+		for_each_plane_in_state(state, plane, pstate, i) {
+			intel_plane = to_intel_plane(plane);
+			id = skl_wm_plane_id(intel_plane);
+
+			if (intel_plane->pipe != intel_crtc->pipe)
+				continue;
+
+			/* packed/uv */
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    pstate, 0);
+			intel_cstate->wm.skl.plane_data_rate[id] = rate;
+
+			/* y-plane */
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    pstate, 1);
+			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
+		}
+	} else {
+		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+			const struct drm_plane_state *pstate =
+				intel_plane->base.state;
+			int id = skl_wm_plane_id(intel_plane);
 
-		/* packed/uv */
-		rate = skl_plane_relative_data_rate(cstate, pstate, 0);
-		cstate->wm.skl.plane_data_rate[id] = rate;
+			/* packed/uv */
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    pstate, 0);
+			intel_cstate->wm.skl.plane_data_rate[id] = rate;
 
-		/* y-plane */
-		rate = skl_plane_relative_data_rate(cstate, pstate, 1);
-		cstate->wm.skl.plane_y_data_rate[id] = rate;
+			/* y-plane */
+			rate = skl_plane_relative_data_rate(intel_cstate,
+							    pstate, 1);
+			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
+		}
 	}
 
 	/* Calculate CRTC's total data rate from cached values */
@@ -3001,10 +3045,12 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *cstate)
 		int id = skl_wm_plane_id(intel_plane);
 
 		/* packed/uv */
-		total_data_rate += cstate->wm.skl.plane_data_rate[id];
-		total_data_rate += cstate->wm.skl.plane_y_data_rate[id];
+		total_data_rate += intel_cstate->wm.skl.plane_data_rate[id];
+		total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id];
 	}
 
+	WARN_ON(cstate->plane_mask && total_data_rate == 0);
+
 	return total_data_rate;
 }
 
-- 
2.1.4

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

[PATCH v3 05/16] drm/i915/gen9: Store plane minimum blocks in CRTC wm state (v2)

[Matt Roper]

This will eventually allow us to re-use old values without
re-calculating them for unchanged planes (which also helps us avoid
re-grabbing extra plane states).

v2:
 -  Drop unnecessary memset's; they were meant for a later patch (which
    got reworked anyway to not need them, but were mis-rebased into this
    one.  (Maarten)

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e67438f..56f4cf8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -430,6 +430,10 @@ struct intel_crtc_wm_state {
 			/* 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];
 		} skl;
 	};
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 39ab1cb..b0d9f72 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3067,8 +3067,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
 	uint16_t alloc_size, start, cursor_blocks;
-	uint16_t minimum[I915_MAX_PLANES];
-	uint16_t y_minimum[I915_MAX_PLANES];
+	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
+	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
 	unsigned int total_data_rate;
 
 	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc);
-- 
2.1.4

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

[PATCH v3 06/16] drm/i915: Track whether an atomic transaction changes the active CRTC's

[Matt Roper]

For the purposes of DDB re-allocation we need to know whether a
transaction changes the list of CRTC's that are active.  While
state->modeset could be used for this purpose, that would be slightly
too aggressive since it would lead us to re-allocate the DDB when a
CRTC's mode changes, but not its final active state.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ff60241..dee39d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,6 +13273,9 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 			intel_state->active_crtcs |= 1 << i;
 		else
 			intel_state->active_crtcs &= ~(1 << i);
+
+		if (crtc_state->active != crtc->state->active)
+			intel_state->active_pipe_changes |= drm_crtc_mask(crtc);
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 56f4cf8..443dc85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -290,6 +290,16 @@ struct intel_atomic_state {
 
 	bool dpll_set, modeset;
 
+	/*
+	 * Does this transaction change the pipes that are active?  This mask
+	 * tracks which CRTC's have changed their active state at the end of
+	 * the transaction (not counting the temporary disable during modesets).
+	 * This mask should only be non-zero when intel_state->modeset is true,
+	 * but the converse is not necessarily true; simply changing a mode may
+	 * not flip the final active status of any CRTC's
+	 */
+	unsigned int active_pipe_changes;
+
 	unsigned int active_crtcs;
 	unsigned int min_pixclk[I915_MAX_PIPES];
 
-- 
2.1.4

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

[PATCH v3 07/16] drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state (v3)

[Matt Roper]

We eventually want to calculate watermark values at atomic 'check' time
instead of atomic 'commit' time so that any requested configurations
that result in impossible watermark requirements are properly rejected.
The first step along this path is to allocate the DDB at atomic 'check'
time.  As we perform this transition, allow the main allocation function
to operate successfully on either an in-flight state or an
already-commited state.  Once we complete the transition in a future
patch, we'll come back and remove the unnecessary logic for the
already-committed case.

v2: Rebase/refactor; we should no longer need to grab extra plane states
    while allocating the DDB since we can pull cached data rates and
    minimum block counts from the CRTC state for any planes that aren't
    being modified by this transaction.

v3:
 - Simplify memsets to clear DDB plane entries.  (Maarten)
 - Drop a redundant memset of plane[pipe][PLANE_CURSOR] that was added
   by an earlier Coccinelle patch.  (Maarten)
 - Assign *num_active at the top of skl_ddb_get_pipe_allocation_limits()
   so that no code paths return without setting it.  (kbuild robot)

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   6 ++
 drivers/gpu/drm/i915/intel_pm.c | 179 +++++++++++++++++++++++++++++-----------
 2 files changed, 139 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6f1e0f1..275fba4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -324,6 +324,12 @@ struct i915_hotplug {
 			    &dev->mode_config.plane_list,	\
 			    base.head)
 
+#define for_each_intel_plane_mask(dev, intel_plane, plane_mask)		\
+	list_for_each_entry(intel_plane, &dev->mode_config.plane_list,	\
+			    base.head)					\
+		for_each_if ((plane_mask) &				\
+			     (1 << drm_plane_index(&intel_plane->base)))
+
 #define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
 	list_for_each_entry(intel_plane,				\
 			    &(dev)->mode_config.plane_list,		\
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b0d9f72..78380ba 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2849,13 +2849,25 @@ skl_wm_plane_id(const struct intel_plane *plane)
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   const struct intel_wm_config *config,
-				   struct skl_ddb_entry *alloc /* out */)
+				   struct intel_wm_config *config,
+				   struct skl_ddb_entry *alloc, /* out */
+				   int *num_active /* out */)
 {
+	struct drm_atomic_state *state = cstate->base.state;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
 	struct drm_crtc *crtc;
 	unsigned int pipe_size, ddb_size;
 	int nth_active_pipe;
+	int pipe = to_intel_crtc(for_crtc)->pipe;
+
+	if (intel_state && intel_state->active_pipe_changes)
+		*num_active = hweight32(intel_state->active_crtcs);
+	else if (intel_state)
+		*num_active = hweight32(dev_priv->active_crtcs);
+	else
+		*num_active = config->num_pipes_active;
 
 	if (!cstate->base.active) {
 		alloc->start = 0;
@@ -2870,25 +2882,56 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 
 	ddb_size -= 4; /* 4 blocks for bypass path allocation */
 
-	nth_active_pipe = 0;
-	for_each_crtc(dev, crtc) {
-		if (!to_intel_crtc(crtc)->active)
-			continue;
+	/*
+	 * FIXME: At the moment we may be called on either in-flight or fully
+	 * committed cstate's.  Once we fully move DDB allocation in the check
+	 * phase, we'll only be called on in-flight states and the 'else'
+	 * branch here will go away.
+	 *
+	 * The 'else' branch is slightly racy here, but it was racy to begin
+	 * with; since it's going away soon, no effort is made to address that.
+	 */
+	if (state) {
+		/*
+		 * If the state doesn't change the active CRTC's, then there's
+		 * no need to recalculate; the existing pipe allocation limits
+		 * should remain unchanged.  Note that we're safe from racing
+		 * commits since any racing commit that changes the active CRTC
+		 * list would need to grab _all_ crtc locks, including the one
+		 * we currently hold.
+		 */
+		if (!intel_state->active_pipe_changes) {
+			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
+			return;
+		}
 
-		if (crtc == for_crtc)
-			break;
+		nth_active_pipe = hweight32(intel_state->active_crtcs &
+					    (drm_crtc_mask(for_crtc) - 1));
+		pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
+		alloc->start = nth_active_pipe * ddb_size / *num_active;
+		alloc->end = alloc->start + pipe_size;
+	} else {
+		nth_active_pipe = 0;
+		for_each_crtc(dev, crtc) {
+			if (!to_intel_crtc(crtc)->active)
+				continue;
 
-		nth_active_pipe++;
-	}
+			if (crtc == for_crtc)
+				break;
 
-	pipe_size = ddb_size / config->num_pipes_active;
-	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
-	alloc->end = alloc->start + pipe_size;
+			nth_active_pipe++;
+		}
+
+		pipe_size = ddb_size / config->num_pipes_active;
+		alloc->start = nth_active_pipe * ddb_size /
+			config->num_pipes_active;
+		alloc->end = alloc->start + pipe_size;
+	}
 }
 
-static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
+static unsigned int skl_cursor_allocation(int num_active)
 {
-	if (config->num_pipes_active == 1)
+	if (num_active == 1)
 		return 32;
 
 	return 8;
@@ -3054,33 +3097,44 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 	return total_data_rate;
 }
 
-static void
+static int
 skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		      struct skl_ddb_allocation *ddb /* out */)
 {
+	struct drm_atomic_state *state = cstate->base.state;
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_wm_config *config = &dev_priv->wm.config;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane;
+	struct drm_plane *plane;
+	struct drm_plane_state *pstate;
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
 	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;
 	unsigned int total_data_rate;
+	int num_active;
+	int id, i;
 
-	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc);
+	if (!cstate->base.active) {
+		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
+		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
+		memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
+		return 0;
+	}
+
+	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
+					   &num_active);
 	alloc_size = skl_ddb_entry_size(alloc);
 	if (alloc_size == 0) {
 		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
-		memset(&ddb->plane[pipe][PLANE_CURSOR], 0,
-		       sizeof(ddb->plane[pipe][PLANE_CURSOR]));
-		return;
+		return 0;
 	}
 
-	cursor_blocks = skl_cursor_allocation(config);
+	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;
 
@@ -3088,21 +3142,55 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	alloc->end -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		struct drm_plane *plane = &intel_plane->base;
-		struct drm_framebuffer *fb = plane->state->fb;
-		int id = skl_wm_plane_id(intel_plane);
+	/*
+	 * TODO: Remove support for already-committed state once we
+	 * only allocate DDB on in-flight states.
+	 */
+	if (state) {
+		for_each_plane_in_state(state, plane, pstate, i) {
+			intel_plane = to_intel_plane(plane);
+			id = skl_wm_plane_id(intel_plane);
 
-		if (!to_intel_plane_state(plane->state)->visible)
-			continue;
+			if (intel_plane->pipe != pipe)
+				continue;
 
-		if (plane->type == DRM_PLANE_TYPE_CURSOR)
-			continue;
+			if (!to_intel_plane_state(pstate)->visible) {
+				minimum[id] = 0;
+				y_minimum[id] = 0;
+				continue;
+			}
+			if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+				minimum[id] = 0;
+				y_minimum[id] = 0;
+				continue;
+			}
+
+			minimum[id] = 8;
+			if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
+				y_minimum[id] = 8;
+			else
+				y_minimum[id] = 0;
+		}
+	} else {
+		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+			struct drm_plane *plane = &intel_plane->base;
+			struct drm_framebuffer *fb = plane->state->fb;
+			int id = skl_wm_plane_id(intel_plane);
+
+			if (!to_intel_plane_state(plane->state)->visible)
+				continue;
+
+			if (plane->type == DRM_PLANE_TYPE_CURSOR)
+				continue;
+
+			minimum[id] = 8;
+			y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12) ? 8 : 0;
+		}
+	}
 
-		minimum[id] = 8;
-		alloc_size -= minimum[id];
-		y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12) ? 8 : 0;
-		alloc_size -= y_minimum[id];
+	for (i = 0; i < PLANE_CURSOR; i++) {
+		alloc_size -= minimum[i];
+		alloc_size -= y_minimum[i];
 	}
 
 	/*
@@ -3113,21 +3201,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	 */
 	total_data_rate = skl_get_total_relative_data_rate(cstate);
 	if (total_data_rate == 0)
-		return;
+		return 0;
 
 	start = alloc->start;
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		struct drm_plane *plane = &intel_plane->base;
-		struct drm_plane_state *pstate = intel_plane->base.state;
 		unsigned int data_rate, y_data_rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
 		int id = skl_wm_plane_id(intel_plane);
 
-		if (!to_intel_plane_state(pstate)->visible)
-			continue;
-		if (plane->type == DRM_PLANE_TYPE_CURSOR)
-			continue;
-
 		data_rate = cstate->wm.skl.plane_data_rate[id];
 
 		/*
@@ -3139,8 +3220,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
 					total_data_rate);
 
-		ddb->plane[pipe][id].start = start;
-		ddb->plane[pipe][id].end = start + plane_blocks;
+		/* Leave disabled planes at (0,0) */
+		if (data_rate) {
+			ddb->plane[pipe][id].start = start;
+			ddb->plane[pipe][id].end = start + plane_blocks;
+		}
 
 		start += plane_blocks;
 
@@ -3153,12 +3237,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
 					total_data_rate);
 
-		ddb->y_plane[pipe][id].start = start;
-		ddb->y_plane[pipe][id].end = start + y_plane_blocks;
+		if (y_data_rate) {
+			ddb->y_plane[pipe][id].start = start;
+			ddb->y_plane[pipe][id].end = start + y_plane_blocks;
+		}
 
 		start += y_plane_blocks;
 	}
 
+	return 0;
 }
 
 static uint32_t skl_pipe_pixel_rate(const struct intel_crtc_state *config)
@@ -3649,7 +3736,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
-	skl_allocate_pipe_ddb(cstate, ddb);
+	WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
 	skl_build_pipe_wm(cstate, ddb, pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
-- 
2.1.4

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

[PATCH v3 08/16] drm/i915/gen9: Compute DDB allocation at atomic check time (v2)

[Matt Roper]

Calculate the DDB blocks needed to satisfy the current atomic
transaction at atomic check time.  This is a prerequisite to calculating
SKL watermarks during the 'check' phase and rejecting any configurations
that we can't find valid watermarks for.

Due to the nature of DDB allocation, it's possible for the addition of a
new CRTC to make the watermark configuration already in use on another,
unchanged CRTC become invalid.  A change in which CRTC's are active
triggers a recompute of the entire DDB, which unfortunately means we
need to disallow any other atomic commits from racing with such an
update.  If the active CRTC's change, we need to grab the lock on all
CRTC's and run all CRTC's through their 'check' handler to recompute and
re-check their per-CRTC DDB allocations.

Note that with this patch we only compute the DDB allocation but we
don't actually use the computed values during watermark programming yet.
For ease of review/testing/bisecting, we still recompute the DDB at
watermark programming time and just WARN() if it doesn't match the
precomputed values.  A future patch will switch over to using the
precomputed values once we're sure they're being properly computed.

Another clarifying note:  DDB allocation itself shouldn't ever fail with
the algorithm we use today (i.e., we have enough DDB blocks on BXT to
support the minimum needs of the worst-case scenario of every pipe/plane
enabled at full size).  However the watermarks calculations based on the
DDB may fail and we'll be moving those to the atomic check as well in
future patches.

v2:
 - Skip DDB calculations in the rare case where our transaction doesn't
   actually touch any CRTC's at all.  Assuming at least one CRTC state
   is present in our transaction, then it means we can't race with any
   transactions that would update dev_priv->active_crtcs (which requires
   _all_ CRTC locks).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_pm.c      | 70 ++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 275fba4..cf0d37b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -339,6 +339,10 @@ struct i915_hotplug {
 #define for_each_intel_crtc(dev, intel_crtc) \
 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
 
+#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) \
+		for_each_if ((crtc_mask) & (1 << drm_crtc_index(&intel_crtc->base)))
+
 #define for_each_intel_encoder(dev, intel_encoder)		\
 	list_for_each_entry(intel_encoder,			\
 			    &(dev)->mode_config.encoder_list,	\
@@ -594,6 +598,7 @@ struct drm_i915_display_funcs {
 				       struct intel_crtc_state *newstate);
 	void (*initial_watermarks)(struct intel_crtc_state *cstate);
 	void (*optimize_watermarks)(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);
 	void (*modeset_commit_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 dee39d4..c970e3e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13315,6 +13315,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 static void calc_watermark_data(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *cstate;
@@ -13344,6 +13345,10 @@ static void calc_watermark_data(struct drm_atomic_state *state)
 		    pstate->crtc_h != pstate->src_h >> 16)
 			intel_state->wm_config.sprites_scaled = true;
 	}
+
+	/* Is there platform-specific watermark information to calculate? */
+	if (dev_priv->display.compute_global_watermarks)
+		dev_priv->display.compute_global_watermarks(state);
 }
 
 /**
@@ -13711,6 +13716,19 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
+	/*
+	 * Temporary sanity check: make sure our pre-computed DDB matches the
+	 * one we actually wind up programming.
+	 *
+	 * Not a great place to put this, but the easiest place we have access
+	 * to both the pre-computed and final DDB's; we'll be removing this
+	 * check in the next patch anyway.
+	 */
+	WARN(IS_GEN9(dev) &&
+	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
+		    sizeof(intel_state->ddb)),
+	     "Pre-computed DDB does not match final DDB!\n");
+
 	if (intel_state->modeset)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 443dc85..2282494 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -311,6 +311,9 @@ struct intel_atomic_state {
 	 * don't bother calculating intermediate watermarks.
 	 */
 	bool skip_intermediate_wm;
+
+	/* Gen9+ only */
+	struct skl_ddb_allocation ddb;
 };
 
 struct intel_plane_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 78380ba..fca44f8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3812,6 +3812,75 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
 
 }
 
+static int
+skl_compute_ddb(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct intel_crtc *intel_crtc;
+	unsigned realloc_pipes = dev_priv->active_crtcs;
+	int ret;
+
+	/*
+	 * If the modeset changes which CRTC's are active, we need to
+	 * recompute the DDB allocation for *all* active pipes, even
+	 * those that weren't otherwise being modified in any way by this
+	 * atomic commit.  Due to the shrinking of the per-pipe allocations
+	 * when new active CRTC's are added, it's possible for a pipe that
+	 * we were already using and aren't changing at all here to suddenly
+	 * become invalid if its DDB needs exceeds its new allocation.
+	 *
+	 * Note that if we wind up doing a full DDB recompute, we can't let
+	 * any other display updates race with this transaction, so we need
+	 * to grab the lock on *all* CRTC's.
+	 */
+	if (intel_state->active_pipe_changes)
+		realloc_pipes = ~0;
+
+	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
+		struct intel_crtc_state *cstate;
+
+		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(cstate))
+			return PTR_ERR(cstate);
+
+		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+skl_compute_wm(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
+	int ret, i;
+	bool changed = false;
+
+	/*
+	 * If this transaction isn't actually touching any CRTC's, don't
+	 * bother with watermark calculation.  Note that if we pass this
+	 * test, we're guaranteed to hold at least one CRTC state mutex,
+	 * which means we can safely use values like dev_priv->active_crtcs
+	 * since any racing commits that want to update them would need to
+	 * hold _all_ CRTC state mutexes.
+	 */
+	for_each_crtc_in_state(state, crtc, cstate, i)
+		changed = true;
+	if (!changed)
+		return 0;
+
+	ret = skl_compute_ddb(state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -7349,6 +7418,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.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		ilk_setup_wm_latency(dev);
 
-- 
2.1.4

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

[PATCH v3 09/16] drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)

[Matt Roper]

Now that we're properly pre-allocating the DDB during the atomic check
phase and we trust that the allocation is appropriate, let's actually
use the allocation computed and not duplicate that work during the
commit phase.

v2:
 - Significant rebasing now that we can use cached data rates and
   minimum block allocations to avoid grabbing additional plane states.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  14 +--
 drivers/gpu/drm/i915/intel_pm.c      | 224 +++++++++++------------------------
 2 files changed, 67 insertions(+), 171 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c970e3e..80d8f77 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13599,6 +13599,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(dev, state);
 	dev_priv->wm.config = intel_state->wm_config;
+	dev_priv->wm.skl_results.ddb = intel_state->ddb;
 	intel_shared_dpll_commit(state);
 
 	if (intel_state->modeset) {
@@ -13716,19 +13717,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
-	/*
-	 * Temporary sanity check: make sure our pre-computed DDB matches the
-	 * one we actually wind up programming.
-	 *
-	 * Not a great place to put this, but the easiest place we have access
-	 * to both the pre-computed and final DDB's; we'll be removing this
-	 * check in the next patch anyway.
-	 */
-	WARN(IS_GEN9(dev) &&
-	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
-		    sizeof(intel_state->ddb)),
-	     "Pre-computed DDB does not match final DDB!\n");
-
 	if (intel_state->modeset)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fca44f8..f28fd36 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2849,7 +2849,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-				   struct intel_wm_config *config,
 				   struct skl_ddb_entry *alloc, /* out */
 				   int *num_active /* out */)
 {
@@ -2857,24 +2856,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
-	struct drm_crtc *crtc;
 	unsigned int pipe_size, ddb_size;
 	int nth_active_pipe;
 	int pipe = to_intel_crtc(for_crtc)->pipe;
 
-	if (intel_state && intel_state->active_pipe_changes)
-		*num_active = hweight32(intel_state->active_crtcs);
-	else if (intel_state)
-		*num_active = hweight32(dev_priv->active_crtcs);
-	else
-		*num_active = config->num_pipes_active;
-
-	if (!cstate->base.active) {
+	if (WARN_ON(!state) || !cstate->base.active) {
 		alloc->start = 0;
 		alloc->end = 0;
+		*num_active = hweight32(dev_priv->active_crtcs);
 		return;
 	}
 
+	if (intel_state->active_pipe_changes)
+		*num_active = hweight32(intel_state->active_crtcs);
+	else
+		*num_active = hweight32(dev_priv->active_crtcs);
+
 	if (IS_BROXTON(dev))
 		ddb_size = BXT_DDB_SIZE;
 	else
@@ -2883,50 +2880,23 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	ddb_size -= 4; /* 4 blocks for bypass path allocation */
 
 	/*
-	 * FIXME: At the moment we may be called on either in-flight or fully
-	 * committed cstate's.  Once we fully move DDB allocation in the check
-	 * phase, we'll only be called on in-flight states and the 'else'
-	 * branch here will go away.
-	 *
-	 * The 'else' branch is slightly racy here, but it was racy to begin
-	 * with; since it's going away soon, no effort is made to address that.
+	 * If the state doesn't change the active CRTC's, then there's
+	 * no need to recalculate; the existing pipe allocation limits
+	 * should remain unchanged.  Note that we're safe from racing
+	 * commits since any racing commit that changes the active CRTC
+	 * list would need to grab _all_ crtc locks, including the one
+	 * we currently hold.
 	 */
-	if (state) {
-		/*
-		 * If the state doesn't change the active CRTC's, then there's
-		 * no need to recalculate; the existing pipe allocation limits
-		 * should remain unchanged.  Note that we're safe from racing
-		 * commits since any racing commit that changes the active CRTC
-		 * list would need to grab _all_ crtc locks, including the one
-		 * we currently hold.
-		 */
-		if (!intel_state->active_pipe_changes) {
-			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
-			return;
-		}
-
-		nth_active_pipe = hweight32(intel_state->active_crtcs &
-					    (drm_crtc_mask(for_crtc) - 1));
-		pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
-		alloc->start = nth_active_pipe * ddb_size / *num_active;
-		alloc->end = alloc->start + pipe_size;
-	} else {
-		nth_active_pipe = 0;
-		for_each_crtc(dev, crtc) {
-			if (!to_intel_crtc(crtc)->active)
-				continue;
-
-			if (crtc == for_crtc)
-				break;
-
-			nth_active_pipe++;
-		}
-
-		pipe_size = ddb_size / config->num_pipes_active;
-		alloc->start = nth_active_pipe * ddb_size /
-			config->num_pipes_active;
-		alloc->end = alloc->start + pipe_size;
+	if (!intel_state->active_pipe_changes) {
+		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
+		return;
 	}
+
+	nth_active_pipe = hweight32(intel_state->active_crtcs &
+				    (drm_crtc_mask(for_crtc) - 1));
+	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
+	alloc->start = nth_active_pipe * ddb_size / *num_active;
+	alloc->end = alloc->start + pipe_size;
 }
 
 static unsigned int skl_cursor_allocation(int num_active)
@@ -3025,62 +2995,33 @@ 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;
 	const struct intel_plane *intel_plane;
+	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 */
-	/*
-	 * FIXME: At the moment this function can be called on either an
-	 * in-flight or a committed state object.  If it's in-flight then we
-	 * only want to re-calculate the plane data rate for planes that are
-	 * part of the transaction (i.e., we don't want to grab any additional
-	 * plane states if we don't have to).  If we're operating on committed
-	 * state, we'll just go ahead and recalculate the plane data rate for
-	 * all planes.
-	 *
-	 * Once we finish moving our DDB allocation to the atomic check phase,
-	 * we'll only be calling this function on in-flight state objects, so
-	 * the 'else' branch here will go away.
-	 */
-	if (state) {
-		struct drm_plane *plane;
-		struct drm_plane_state *pstate;
-		int i;
-
-		for_each_plane_in_state(state, plane, pstate, i) {
-			intel_plane = to_intel_plane(plane);
-			id = skl_wm_plane_id(intel_plane);
-
-			if (intel_plane->pipe != intel_crtc->pipe)
-				continue;
-
-			/* packed/uv */
-			rate = skl_plane_relative_data_rate(intel_cstate,
-							    pstate, 0);
-			intel_cstate->wm.skl.plane_data_rate[id] = rate;
-
-			/* y-plane */
-			rate = skl_plane_relative_data_rate(intel_cstate,
-							    pstate, 1);
-			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
-		}
-	} else {
-		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-			const struct drm_plane_state *pstate =
-				intel_plane->base.state;
-			int id = skl_wm_plane_id(intel_plane);
+	for_each_plane_in_state(state, plane, pstate, i) {
+		id = skl_wm_plane_id(to_intel_plane(plane));
+		intel_plane = to_intel_plane(plane);
 
-			/* packed/uv */
-			rate = skl_plane_relative_data_rate(intel_cstate,
-							    pstate, 0);
-			intel_cstate->wm.skl.plane_data_rate[id] = rate;
+		if (intel_plane->pipe != intel_crtc->pipe)
+			continue;
 
-			/* y-plane */
-			rate = skl_plane_relative_data_rate(intel_cstate,
-							    pstate, 1);
-			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
-		}
+		/* packed/uv */
+		rate = skl_plane_relative_data_rate(intel_cstate,
+						    pstate, 0);
+		intel_cstate->wm.skl.plane_data_rate[id] = rate;
+
+		/* y-plane */
+		rate = skl_plane_relative_data_rate(intel_cstate,
+						    pstate, 1);
+		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
 	}
 
 	/* Calculate CRTC's total data rate from cached values */
@@ -3104,8 +3045,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct drm_atomic_state *state = cstate->base.state;
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_wm_config *config = &dev_priv->wm.config;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane;
 	struct drm_plane *plane;
@@ -3119,6 +3058,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	int num_active;
 	int id, i;
 
+	if (WARN_ON(!state))
+		return 0;
+
 	if (!cstate->base.active) {
 		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
 		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -3126,8 +3068,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 	}
 
-	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
-					   &num_active);
+	skl_ddb_get_pipe_allocation_limits(dev, cstate, alloc, &num_active);
 	alloc_size = skl_ddb_entry_size(alloc);
 	if (alloc_size == 0) {
 		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -3139,53 +3080,31 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
 
 	alloc_size -= cursor_blocks;
-	alloc->end -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
-	/*
-	 * TODO: Remove support for already-committed state once we
-	 * only allocate DDB on in-flight states.
-	 */
-	if (state) {
-		for_each_plane_in_state(state, plane, pstate, i) {
-			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)->visible) {
-				minimum[id] = 0;
-				y_minimum[id] = 0;
-				continue;
-			}
-			if (plane->type == DRM_PLANE_TYPE_CURSOR) {
-				minimum[id] = 0;
-				y_minimum[id] = 0;
-				continue;
-			}
-
-			minimum[id] = 8;
-			if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
-				y_minimum[id] = 8;
-			else
-				y_minimum[id] = 0;
-		}
-	} else {
-		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-			struct drm_plane *plane = &intel_plane->base;
-			struct drm_framebuffer *fb = plane->state->fb;
-			int id = skl_wm_plane_id(intel_plane);
-
-			if (!to_intel_plane_state(plane->state)->visible)
-				continue;
+	for_each_plane_in_state(state, plane, pstate, i) {
+		intel_plane = to_intel_plane(plane);
+		id = skl_wm_plane_id(intel_plane);
 
-			if (plane->type == DRM_PLANE_TYPE_CURSOR)
-				continue;
+		if (intel_plane->pipe != pipe)
+			continue;
 
-			minimum[id] = 8;
-			y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12) ? 8 : 0;
+		if (!to_intel_plane_state(pstate)->visible) {
+			minimum[id] = 0;
+			y_minimum[id] = 0;
+			continue;
+		}
+		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+			minimum[id] = 0;
+			y_minimum[id] = 0;
+			continue;
 		}
+
+		minimum[id] = 8;
+		if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
+			y_minimum[id] = 8;
+		else
+			y_minimum[id] = 0;
 	}
 
 	for (i = 0; i < PLANE_CURSOR; i++) {
@@ -3736,7 +3655,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 
-	WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
 	skl_build_pipe_wm(cstate, ddb, pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
@@ -3800,16 +3718,6 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
 	memset(watermarks->plane_trans[pipe],
 	       0, sizeof(uint32_t) * I915_MAX_PLANES);
 	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
-
-	/* Clear ddb entries for pipe */
-	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct skl_ddb_entry));
-	memset(&watermarks->ddb.plane[pipe], 0,
-	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
-	memset(&watermarks->ddb.y_plane[pipe], 0,
-	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
-	memset(&watermarks->ddb.plane[pipe][PLANE_CURSOR], 0,
-	       sizeof(struct skl_ddb_entry));
-
 }
 
 static int
-- 
2.1.4

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

[PATCH v3 10/16] drm/i915/gen9: Calculate plane WM's from state

[Matt Roper]

In a future patch we'll want to calculate plane watermarks for in-flight
atomic state rather than the already-committed state.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f28fd36..c665f7e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3240,16 +3240,14 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
 
 static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				 struct intel_crtc_state *cstate,
-				 struct intel_plane *intel_plane,
+				 struct intel_plane_state *intel_pstate,
 				 uint16_t ddb_allocation,
 				 int level,
 				 uint16_t *out_blocks, /* out */
 				 uint8_t *out_lines /* out */)
 {
-	struct drm_plane *plane = &intel_plane->base;
-	struct drm_framebuffer *fb = plane->state->fb;
-	struct intel_plane_state *intel_pstate =
-					to_intel_plane_state(plane->state);
+	struct drm_plane_state *pstate = &intel_pstate->base;
+	struct drm_framebuffer *fb = pstate->fb;
 	uint32_t latency = dev_priv->wm.skl_latency[level];
 	uint32_t method1, method2;
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
@@ -3264,7 +3262,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	width = drm_rect_width(&intel_pstate->src) >> 16;
 	height = drm_rect_height(&intel_pstate->src) >> 16;
 
-	if (intel_rotation_90_or_270(plane->state->rotation))
+	if (intel_rotation_90_or_270(pstate->rotation))
 		swap(width, height);
 
 	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
@@ -3284,7 +3282,7 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
 		uint32_t min_scanlines = 4;
 		uint32_t y_tile_minimum;
-		if (intel_rotation_90_or_270(plane->state->rotation)) {
+		if (intel_rotation_90_or_270(pstate->rotation)) {
 			int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
 				drm_format_plane_cpp(fb->pixel_format, 1) :
 				drm_format_plane_cpp(fb->pixel_format, 0);
@@ -3338,17 +3336,19 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 	struct intel_plane *intel_plane;
+	struct intel_plane_state *intel_pstate;
 	uint16_t ddb_blocks;
 	enum pipe pipe = intel_crtc->pipe;
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		int i = skl_wm_plane_id(intel_plane);
 
+		intel_pstate = to_intel_plane_state(intel_plane->base.state);
 		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
 		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
 						cstate,
-						intel_plane,
+						intel_pstate,
 						ddb_blocks,
 						level,
 						&result->plane_res_b[i],
-- 
2.1.4

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

[PATCH v3 11/16] drm/i915/gen9: Allow watermark calculation on in-flight atomic state (v3)

[Matt Roper]

In an upcoming patch we'll move this calculation to the atomic 'check'
phase so that the display update can be rejected early if no valid
watermark programming is possible.

v2:
 - Drop intel_pstate_for_cstate_plane() helper and add note about how
   the code needs to evolve in the future if we start allowing more than
   one pending commit against a CRTC.  (Maarten)

v3:
 - Only have skl_compute_wm_level calculate watermarks for enabled
   planes; we can just set the other planes on a CRTC to disabled
   without having to look at the plane state.  This is important because
   despite our CRTC lock we can still have racing commits that modify
   a disabled plane's property without turning it on.  (Maarten)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 61 ++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c665f7e..083d1de 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3327,23 +3327,56 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	return true;
 }
 
-static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
-				 struct skl_ddb_allocation *ddb,
-				 struct intel_crtc_state *cstate,
-				 int level,
-				 struct skl_wm_level *result)
+static int
+skl_compute_wm_level(const struct drm_i915_private *dev_priv,
+		     struct skl_ddb_allocation *ddb,
+		     struct intel_crtc_state *cstate,
+		     int level,
+		     struct skl_wm_level *result)
 {
 	struct drm_device *dev = dev_priv->dev;
+	struct drm_atomic_state *state = cstate->base.state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
+	struct drm_plane *plane;
 	struct intel_plane *intel_plane;
 	struct intel_plane_state *intel_pstate;
 	uint16_t ddb_blocks;
 	enum pipe pipe = intel_crtc->pipe;
 
-	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+	/*
+	 * We'll only calculate watermarks for planes that are actually
+	 * enabled, so make sure all other planes are set as disabled.
+	 */
+	memset(result, 0, sizeof(*result));
+
+	for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) {
 		int i = skl_wm_plane_id(intel_plane);
 
-		intel_pstate = to_intel_plane_state(intel_plane->base.state);
+		plane = &intel_plane->base;
+		intel_pstate = NULL;
+		if (state)
+			intel_pstate =
+				intel_atomic_get_existing_plane_state(state,
+								      intel_plane);
+
+		/*
+		 * Note: If we start supporting multiple pending atomic commits
+		 * against the same planes/CRTC's in the future, plane->state
+		 * will no longer be the correct pre-state to use for the
+		 * calculations here and we'll need to change where we get the
+		 * 'unchanged' plane data from.
+		 *
+		 * For now this is fine because we only allow one queued commit
+		 * against a CRTC.  Even if the plane isn't modified by this
+		 * transaction and we don't have a plane lock, we still have
+		 * the CRTC's lock, so we know that no other transactions are
+		 * racing with us to update it.
+		 */
+		if (!intel_pstate)
+			intel_pstate = to_intel_plane_state(plane->state);
+
+		WARN_ON(!intel_pstate->base.fb);
+
 		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
 		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
@@ -3354,6 +3387,8 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 						&result->plane_res_b[i],
 						&result->plane_res_l[i]);
 	}
+
+	return 0;
 }
 
 static uint32_t
@@ -3648,14 +3683,14 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	}
 }
 
-static bool skl_update_pipe_wm(struct drm_crtc *crtc,
+static bool skl_update_pipe_wm(struct drm_crtc_state *cstate,
 			       struct skl_ddb_allocation *ddb, /* out */
 			       struct skl_pipe_wm *pipe_wm /* out */)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc);
+	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
 
-	skl_build_pipe_wm(cstate, ddb, pipe_wm);
+	skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
 
 	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
 		return false;
@@ -3695,7 +3730,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 		if (!intel_crtc->active)
 			continue;
 
-		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
+		wm_changed = skl_update_pipe_wm(intel_crtc->base.state,
 						&r->ddb, &pipe_wm);
 
 		/*
@@ -3804,7 +3839,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	skl_clear_wm(results, intel_crtc->pipe);
 
-	if (!skl_update_pipe_wm(crtc, &results->ddb, pipe_wm))
+	if (!skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm))
 		return;
 
 	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
-- 
2.1.4

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

[PATCH v3 12/16] drm/i915/gen9: Use a bitmask to track dirty pipe watermarks

[Matt Roper]

Slightly easier to work with than an array of bools.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cf0d37b..a34211c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1591,7 +1591,7 @@ struct skl_ddb_allocation {
 };
 
 struct skl_wm_values {
-	bool dirty[I915_MAX_PIPES];
+	unsigned dirty_pipes;
 	struct skl_ddb_allocation ddb;
 	uint32_t wm_linetime[I915_MAX_PIPES];
 	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 083d1de..0dcdf0b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3516,7 +3516,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		int i, level, max_level = ilk_wm_max_level(dev);
 		enum pipe pipe = crtc->pipe;
 
-		if (!new->dirty[pipe])
+		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
 			continue;
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
@@ -3741,7 +3741,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 		WARN_ON(!wm_changed);
 
 		skl_compute_wm_results(dev, &pipe_wm, r, intel_crtc);
-		r->dirty[intel_crtc->pipe] = true;
+		r->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
 	}
 }
 
@@ -3835,7 +3835,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 
 	/* Clear all dirty flags */
-	memset(results->dirty, 0, sizeof(bool) * I915_MAX_PIPES);
+	results->dirty_pipes = 0;
 
 	skl_clear_wm(results, intel_crtc->pipe);
 
@@ -3843,7 +3843,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 		return;
 
 	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
-	results->dirty[intel_crtc->pipe] = true;
+	results->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
 
 	skl_update_other_pipe_wm(dev, crtc, results);
 	skl_write_wm_values(dev_priv, results);
@@ -4002,7 +4002,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	hw->dirty[pipe] = true;
+	hw->dirty_pipes |= drm_crtc_mask(crtc);
 
 	active->linetime = hw->wm_linetime[pipe];
 
-- 
2.1.4

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

[PATCH v3 13/16] drm/i915/gen9: Propagate watermark calculation failures up the call chain

[Matt Roper]

Once we move watermark calculation to the atomic check phase, we'll want
to start rejecting display configurations that exceed out watermark
limits.  At the moment we just assume that there's always a valid set of
watermarks, even though this may not actually be true.  Let's prepare by
passing return codes up through the call stack in preparation.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++--
 drivers/gpu/drm/i915/intel_pm.c      | 90 ++++++++++++++++++++++--------------
 2 files changed, 61 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 80d8f77..7e8fa88 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13312,7 +13312,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
  * phase.  The code here should be run after the per-crtc and per-plane 'check'
  * handlers to ensure that all derived state has been updated.
  */
-static void calc_watermark_data(struct drm_atomic_state *state)
+static int calc_watermark_data(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -13348,7 +13348,9 @@ static void calc_watermark_data(struct drm_atomic_state *state)
 
 	/* Is there platform-specific watermark information to calculate? */
 	if (dev_priv->display.compute_global_watermarks)
-		dev_priv->display.compute_global_watermarks(state);
+		return dev_priv->display.compute_global_watermarks(state);
+
+	return 0;
 }
 
 /**
@@ -13432,9 +13434,7 @@ static int intel_atomic_check(struct drm_device *dev,
 		return ret;
 
 	intel_fbc_choose_crtc(dev_priv, state);
-	calc_watermark_data(state);
-
-	return 0;
+	return calc_watermark_data(state);
 }
 
 static int intel_atomic_prepare_commit(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0dcdf0b..62609ab 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3238,13 +3238,14 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
 	return false;
 }
 
-static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
-				 struct intel_crtc_state *cstate,
-				 struct intel_plane_state *intel_pstate,
-				 uint16_t ddb_allocation,
-				 int level,
-				 uint16_t *out_blocks, /* out */
-				 uint8_t *out_lines /* out */)
+static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
+				struct intel_crtc_state *cstate,
+				struct intel_plane_state *intel_pstate,
+				uint16_t ddb_allocation,
+				int level,
+				uint16_t *out_blocks, /* out */
+				uint8_t *out_lines, /* out */
+				bool *enabled /* out */)
 {
 	struct drm_plane_state *pstate = &intel_pstate->base;
 	struct drm_framebuffer *fb = pstate->fb;
@@ -3256,8 +3257,10 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 
-	if (latency == 0 || !cstate->base.active || !intel_pstate->visible)
-		return false;
+	if (latency == 0 || !cstate->base.active || !intel_pstate->visible) {
+		*enabled = false;
+		return 0;
+	}
 
 	width = drm_rect_width(&intel_pstate->src) >> 16;
 	height = drm_rect_height(&intel_pstate->src) >> 16;
@@ -3318,13 +3321,16 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 			res_blocks++;
 	}
 
-	if (res_blocks >= ddb_allocation || res_lines > 31)
-		return false;
+	if (res_blocks >= ddb_allocation || res_lines > 31) {
+		*enabled = false;
+		return 0;
+	}
 
 	*out_blocks = res_blocks;
 	*out_lines = res_lines;
+	*enabled = true;
 
-	return true;
+	return 0;
 }
 
 static int
@@ -3342,6 +3348,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	struct intel_plane_state *intel_pstate;
 	uint16_t ddb_blocks;
 	enum pipe pipe = intel_crtc->pipe;
+	int ret;
 
 	/*
 	 * We'll only calculate watermarks for planes that are actually
@@ -3379,13 +3386,16 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 
 		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
-		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
-						cstate,
-						intel_pstate,
-						ddb_blocks,
-						level,
-						&result->plane_res_b[i],
-						&result->plane_res_l[i]);
+		ret = skl_compute_plane_wm(dev_priv,
+					   cstate,
+					   intel_pstate,
+					   ddb_blocks,
+					   level,
+					   &result->plane_res_b[i],
+					   &result->plane_res_l[i],
+					   &result->plane_en[i]);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -3422,21 +3432,26 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 	}
 }
 
-static void skl_build_pipe_wm(struct intel_crtc_state *cstate,
-			      struct skl_ddb_allocation *ddb,
-			      struct skl_pipe_wm *pipe_wm)
+static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
+			     struct skl_ddb_allocation *ddb,
+			     struct skl_pipe_wm *pipe_wm)
 {
 	struct drm_device *dev = cstate->base.crtc->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	int level, max_level = ilk_wm_max_level(dev);
+	int ret;
 
 	for (level = 0; level <= max_level; level++) {
-		skl_compute_wm_level(dev_priv, ddb, cstate,
-				     level, &pipe_wm->wm[level]);
+		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
+					   level, &pipe_wm->wm[level]);
+		if (ret)
+			return ret;
 	}
 	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
 
 	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
+
+	return 0;
 }
 
 static void skl_compute_wm_results(struct drm_device *dev,
@@ -3683,21 +3698,27 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	}
 }
 
-static bool skl_update_pipe_wm(struct drm_crtc_state *cstate,
-			       struct skl_ddb_allocation *ddb, /* out */
-			       struct skl_pipe_wm *pipe_wm /* out */)
+static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
+			      struct skl_ddb_allocation *ddb, /* out */
+			      struct skl_pipe_wm *pipe_wm, /* 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;
 
-	skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
+	if (ret)
+		return ret;
 
 	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
-		return false;
+		*changed = false;
+	else
+		*changed = true;
 
 	intel_crtc->wm.active.skl = *pipe_wm;
 
-	return true;
+	return 0;
 }
 
 static void skl_update_other_pipe_wm(struct drm_device *dev,
@@ -3730,8 +3751,8 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 		if (!intel_crtc->active)
 			continue;
 
-		wm_changed = skl_update_pipe_wm(intel_crtc->base.state,
-						&r->ddb, &pipe_wm);
+		skl_update_pipe_wm(intel_crtc->base.state,
+				   &r->ddb, &pipe_wm, &wm_changed);
 
 		/*
 		 * If we end up re-computing the other pipe WM values, it's
@@ -3832,14 +3853,15 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-
+	bool wm_changed;
 
 	/* Clear all dirty flags */
 	results->dirty_pipes = 0;
 
 	skl_clear_wm(results, intel_crtc->pipe);
 
-	if (!skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm))
+	skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed);
+	if (!wm_changed)
 		return;
 
 	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
-- 
2.1.4

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

[PATCH v3 14/16] drm/i915/gen9: Calculate watermarks during atomic 'check'

[Matt Roper]

Moving watermark calculation into the check phase will allow us to to
reject display configurations for which there are no valid watermark
values before we start trying to program the hardware (although those
tests will come in a subsequent patch).

Another advantage of moving this calculation to the check phase is that
we can calculate the watermarks in a single shot as part of the atomic
transaction.  The watermark interfaces we inherited from our legacy
modesetting days are a bit broken in the atomic design because they use
per-crtc entry points but actually re-calculate and re-program something
that is really more of a global state.  That worked okay in the legacy
modesetting world because operations only ever updated a single CRTC at
a time.  However in the atomic world, a transaction can involve multiple
CRTC's, which means we wind up computing and programming the watermarks
NxN times (where N is the number of CRTC's involved).  With this patch
we eliminate the redundant re-calculation of watermark data for atomic
states (which was the cause of the WARN_ON(!wm_changed) problems that
have plagued us for a while).

We still need to work on the 'commit' side of watermark handling so that
we aren't doing redundant NxN programming of watermarks, but that's
content for future patches.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89055
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   2 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 138 +++++++++++++----------------------
 3 files changed, 52 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e8fa88..8bedf12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13599,7 +13599,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_swap_state(dev, state);
 	dev_priv->wm.config = intel_state->wm_config;
-	dev_priv->wm.skl_results.ddb = intel_state->ddb;
+	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 
 	if (intel_state->modeset) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2282494..6316215 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -313,7 +313,7 @@ struct intel_atomic_state {
 	bool skip_intermediate_wm;
 
 	/* Gen9+ only */
-	struct skl_ddb_allocation ddb;
+	struct skl_wm_values wm_results;
 };
 
 struct intel_plane_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 62609ab..fe5adcd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3221,23 +3221,6 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 	return ret;
 }
 
-static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
-				       const struct intel_crtc *intel_crtc)
-{
-	struct drm_device *dev = intel_crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * If ddb allocation of pipes changed, it may require recalculation of
-	 * watermarks
-	 */
-	if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
-		return true;
-
-	return false;
-}
-
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
 				struct intel_plane_state *intel_pstate,
@@ -3716,66 +3699,9 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	else
 		*changed = true;
 
-	intel_crtc->wm.active.skl = *pipe_wm;
-
 	return 0;
 }
 
-static void skl_update_other_pipe_wm(struct drm_device *dev,
-				     struct drm_crtc *crtc,
-				     struct skl_wm_values *r)
-{
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc *this_crtc = to_intel_crtc(crtc);
-
-	/*
-	 * If the WM update hasn't changed the allocation for this_crtc (the
-	 * crtc we are currently computing the new WM values for), other
-	 * enabled crtcs will keep the same allocation and we don't need to
-	 * recompute anything for them.
-	 */
-	if (!skl_ddb_allocation_changed(&r->ddb, this_crtc))
-		return;
-
-	/*
-	 * Otherwise, because of this_crtc being freshly enabled/disabled, the
-	 * other active pipes need new DDB allocation and WM values.
-	 */
-	for_each_intel_crtc(dev, intel_crtc) {
-		struct skl_pipe_wm pipe_wm = {};
-		bool wm_changed;
-
-		if (this_crtc->pipe == intel_crtc->pipe)
-			continue;
-
-		if (!intel_crtc->active)
-			continue;
-
-		skl_update_pipe_wm(intel_crtc->base.state,
-				   &r->ddb, &pipe_wm, &wm_changed);
-
-		/*
-		 * If we end up re-computing the other pipe WM values, it's
-		 * because it was really needed, so we expect the WM values to
-		 * be different.
-		 */
-		WARN_ON(!wm_changed);
-
-		skl_compute_wm_results(dev, &pipe_wm, r, intel_crtc);
-		r->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
-	}
-}
-
-static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
-{
-	watermarks->wm_linetime[pipe] = 0;
-	memset(watermarks->plane[pipe], 0,
-	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
-	memset(watermarks->plane_trans[pipe],
-	       0, sizeof(uint32_t) * I915_MAX_PLANES);
-	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
-}
-
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -3783,6 +3709,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *intel_crtc;
+	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
 	unsigned realloc_pipes = dev_priv->active_crtcs;
 	int ret;
 
@@ -3799,8 +3726,10 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (intel_state->active_pipe_changes)
+	if (intel_state->active_pipe_changes) {
 		realloc_pipes = ~0;
+		intel_state->wm_results.dirty_pipes = ~0;
+	}
 
 	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
 		struct intel_crtc_state *cstate;
@@ -3809,7 +3738,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
 
-		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
+		ret = skl_allocate_pipe_ddb(cstate, ddb);
 		if (ret)
 			return ret;
 	}
@@ -3822,8 +3751,11 @@ skl_compute_wm(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *cstate;
-	int ret, i;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct skl_wm_values *results = &intel_state->wm_results;
+	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
+	int ret, i;
 
 	/*
 	 * If this transaction isn't actually touching any CRTC's, don't
@@ -3838,10 +3770,45 @@ skl_compute_wm(struct drm_atomic_state *state)
 	if (!changed)
 		return 0;
 
+	/* Clear all dirty flags */
+	results->dirty_pipes = 0;
+
 	ret = skl_compute_ddb(state);
 	if (ret)
 		return ret;
 
+	/*
+	 * Calculate WM's for all pipes that are part of this transaction.
+	 * Note that the DDB allocation above may have added more CRTC's that
+	 * weren't otherwise being modified (and set bits in dirty_pipes) if
+	 * pipe allocations had to change.
+	 *
+	 * FIXME:  Now that we're doing this in the atomic check phase, we
+	 * should allow skl_update_pipe_wm() to return failure in cases where
+	 * no suitable watermark values can be found.
+	 */
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_crtc_state *intel_cstate =
+			to_intel_crtc_state(cstate);
+
+		pipe_wm = &intel_cstate->wm.skl.optimal;
+		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
+					 &changed);
+		if (ret)
+			return ret;
+
+		if (changed)
+			results->dirty_pipes |= drm_crtc_mask(crtc);
+
+		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
+			/* This pipe's WM's did not change */
+			continue;
+
+		intel_cstate->update_wm_pre = true;
+		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
+	}
+
 	return 0;
 }
 
@@ -3853,26 +3820,21 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	bool wm_changed;
 
-	/* Clear all dirty flags */
-	results->dirty_pipes = 0;
-
-	skl_clear_wm(results, intel_crtc->pipe);
-
-	skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed);
-	if (!wm_changed)
+	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
 
-	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
-	results->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
+	intel_crtc->wm.active.skl = *pipe_wm;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_update_other_pipe_wm(dev, crtc, results);
 	skl_write_wm_values(dev_priv, results);
 	skl_flush_wm_values(dev_priv, results);
 
 	/* store the new configuration */
 	dev_priv->wm.skl_hw = *results;
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
 static void ilk_compute_wm_config(struct drm_device *dev,
-- 
2.1.4

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

[PATCH v3 15/16] drm/i915/gen9: Reject display updates that exceed wm limitations (v2)

[Matt Roper]

If we can't find any valid level 0 watermark values for the requested
atomic transaction, reject the configuration before we try to start
programming the hardware.

v2:
 - Add extra debugging output when we reject level 0 watermarks so that
   we can more easily debug how/why they were rejected.

Cc: Lyude Paul <cpaul@redhat.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe5adcd..c996b62 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3306,7 +3306,22 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (res_blocks >= ddb_allocation || res_lines > 31) {
 		*enabled = false;
-		return 0;
+
+		/*
+		 * If there are no valid level 0 watermarks, then we can't
+		 * support this display configuration.
+		 */
+		if (level) {
+			return 0;
+		} else {
+			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
+			DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u, lines required = %u/31\n",
+				      to_intel_crtc(cstate->base.crtc)->pipe,
+				      skl_wm_plane_id(to_intel_plane(pstate->plane)),
+				      res_blocks, ddb_allocation, res_lines);
+
+			return -EINVAL;
+		}
 	}
 
 	*out_blocks = res_blocks;
-- 
2.1.4

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

[PATCH v3 16/16] drm/i915: Remove wm_config from dev_priv/intel_atomic_state

[Matt Roper]

We calculate the watermark config into intel_atomic_state and then save
it into dev_priv, but never actually use it from there.  This is
left-over from some early ILK-style watermark programming designs that
got changed over time.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 31 -------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 3 files changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a34211c..ac05007 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1961,9 +1961,6 @@ struct drm_i915_private {
 		 */
 		uint16_t skl_latency[8];
 
-		/* Committed wm config */
-		struct intel_wm_config config;
-
 		/*
 		 * The skl_wm_values structure is a bit too big for stack
 		 * allocation, so we keep the staging struct where we store
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8bedf12..05e87bb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13316,35 +13316,6 @@ static int calc_watermark_data(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *cstate;
-	struct drm_plane *plane;
-	struct drm_plane_state *pstate;
-
-	/*
-	 * Calculate watermark configuration details now that derived
-	 * plane/crtc state is all properly updated.
-	 */
-	drm_for_each_crtc(crtc, dev) {
-		cstate = drm_atomic_get_existing_crtc_state(state, crtc) ?:
-			crtc->state;
-
-		if (cstate->active)
-			intel_state->wm_config.num_pipes_active++;
-	}
-	drm_for_each_legacy_plane(plane, dev) {
-		pstate = drm_atomic_get_existing_plane_state(state, plane) ?:
-			plane->state;
-
-		if (!to_intel_plane_state(pstate)->visible)
-			continue;
-
-		intel_state->wm_config.sprites_enabled = true;
-		if (pstate->crtc_w != pstate->src_w >> 16 ||
-		    pstate->crtc_h != pstate->src_h >> 16)
-			intel_state->wm_config.sprites_scaled = true;
-	}
 
 	/* Is there platform-specific watermark information to calculate? */
 	if (dev_priv->display.compute_global_watermarks)
@@ -13598,7 +13569,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 	}
 
 	drm_atomic_helper_swap_state(dev, state);
-	dev_priv->wm.config = intel_state->wm_config;
 	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 
@@ -15366,7 +15336,6 @@ retry:
 	}
 
 	/* Write calculated watermark values back */
-	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6316215..804e999 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -304,7 +304,6 @@ struct intel_atomic_state {
 	unsigned int min_pixclk[I915_MAX_PIPES];
 
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
-	struct intel_wm_config wm_config;
 
 	/*
 	 * Current watermarks can't be trusted during hardware readout, so
-- 
2.1.4

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

✗ Fi.CI.BAT: warning for Pre-calculate SKL-style atomic watermarks (rev3)

[Patchwork]

== Series Details ==

Series: Pre-calculate SKL-style atomic watermarks (rev3)
URL   : https://patchwork.freedesktop.org/series/5158/
State : warning

== Summary ==

Series 5158v3 Pre-calculate SKL-style atomic watermarks
http://patchwork.freedesktop.org/api/1.0/series/5158/revisions/3/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ilk-hp8440p) UNSTABLE
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                pass       -> SKIP       (hsw-gt2)

bdw-nuci7        total:193  pass:181  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:193  pass:170  dwarn:0   dfail:0   fail:0   skip:23 
byt-nuc          total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:193  pass:169  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:193  pass:173  dwarn:0   dfail:0   fail:0   skip:20 
ilk-hp8440p      total:193  pass:136  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:193  pass:165  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:193  pass:168  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:193  pass:182  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:193  pass:155  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:193  pass:155  dwarn:0   dfail:0   fail:1   skip:37 
bsw-nuc-2 failed to connect after reboot

Results at /archive/results/CI_IGT_test/Patchwork_1984/

d5b5101bd09a7b7e48b1cd78fe8f8a40b21d4deb drm-intel-nightly: 2016y-04m-21d-16h-37m-06s UTC integration manifest
530bba0 drm/i915: Remove wm_config from dev_priv/intel_atomic_state
17b98fd drm/i915/gen9: Reject display updates that exceed wm limitations (v2)
ae900a1 drm/i915/gen9: Calculate watermarks during atomic 'check'
50b2b51 drm/i915/gen9: Propagate watermark calculation failures up the call chain
8a7ed23 drm/i915/gen9: Use a bitmask to track dirty pipe watermarks
09aea01 drm/i915/gen9: Allow watermark calculation on in-flight atomic state (v3)
bbf4ed8 drm/i915/gen9: Calculate plane WM's from state
cf257bc drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)
437d59b drm/i915/gen9: Compute DDB allocation at atomic check time (v2)
c81105f drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state (v3)
c021808 drm/i915: Track whether an atomic transaction changes the active CRTC's
6733614 drm/i915/gen9: Store plane minimum blocks in CRTC wm state (v2)
116b5b4 drm/i915/gen9: Allow calculation of data rate for in-flight state (v2)
69d6bda drm/i915/gen9: Cache plane data rates in CRTC state
275039d drm/i915: Rename s/skl_compute_pipe_wm/skl_build_pipe_wm/
8ef8b01 drm/i915: Reorganize WM structs/unions in CRTC state

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

Re: [PATCH v3 14/16] drm/i915/gen9: Calculate watermarks during atomic 'check'

[Jani Nikula]

On Fri, 22 Apr 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> Moving watermark calculation into the check phase will allow us to to
> reject display configurations for which there are no valid watermark
> values before we start trying to program the hardware (although those
> tests will come in a subsequent patch).
>
> Another advantage of moving this calculation to the check phase is that
> we can calculate the watermarks in a single shot as part of the atomic
> transaction.  The watermark interfaces we inherited from our legacy
> modesetting days are a bit broken in the atomic design because they use
> per-crtc entry points but actually re-calculate and re-program something
> that is really more of a global state.  That worked okay in the legacy
> modesetting world because operations only ever updated a single CRTC at
> a time.  However in the atomic world, a transaction can involve multiple
> CRTC's, which means we wind up computing and programming the watermarks
> NxN times (where N is the number of CRTC's involved).  With this patch
> we eliminate the redundant re-calculation of watermark data for atomic
> states (which was the cause of the WARN_ON(!wm_changed) problems that
> have plagued us for a while).
>
> We still need to work on the 'commit' side of watermark handling so that
> we aren't doing redundant NxN programming of watermarks, but that's
> content for future patches.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89055

From the bug,

Tested-by: Cezar Burlacu <cezar.burlacu@intel.com>

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 138 +++++++++++++----------------------
>  3 files changed, 52 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e8fa88..8bedf12 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13599,7 +13599,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(dev, state);
>  	dev_priv->wm.config = intel_state->wm_config;
> -	dev_priv->wm.skl_results.ddb = intel_state->ddb;
> +	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
>  
>  	if (intel_state->modeset) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2282494..6316215 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -313,7 +313,7 @@ struct intel_atomic_state {
>  	bool skip_intermediate_wm;
>  
>  	/* Gen9+ only */
> -	struct skl_ddb_allocation ddb;
> +	struct skl_wm_values wm_results;
>  };
>  
>  struct intel_plane_state {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 62609ab..fe5adcd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3221,23 +3221,6 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>  	return ret;
>  }
>  
> -static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
> -				       const struct intel_crtc *intel_crtc)
> -{
> -	struct drm_device *dev = intel_crtc->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> -
> -	/*
> -	 * If ddb allocation of pipes changed, it may require recalculation of
> -	 * watermarks
> -	 */
> -	if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
> -		return true;
> -
> -	return false;
> -}
> -
>  static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  				struct intel_crtc_state *cstate,
>  				struct intel_plane_state *intel_pstate,
> @@ -3716,66 +3699,9 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  	else
>  		*changed = true;
>  
> -	intel_crtc->wm.active.skl = *pipe_wm;
> -
>  	return 0;
>  }
>  
> -static void skl_update_other_pipe_wm(struct drm_device *dev,
> -				     struct drm_crtc *crtc,
> -				     struct skl_wm_values *r)
> -{
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc *this_crtc = to_intel_crtc(crtc);
> -
> -	/*
> -	 * If the WM update hasn't changed the allocation for this_crtc (the
> -	 * crtc we are currently computing the new WM values for), other
> -	 * enabled crtcs will keep the same allocation and we don't need to
> -	 * recompute anything for them.
> -	 */
> -	if (!skl_ddb_allocation_changed(&r->ddb, this_crtc))
> -		return;
> -
> -	/*
> -	 * Otherwise, because of this_crtc being freshly enabled/disabled, the
> -	 * other active pipes need new DDB allocation and WM values.
> -	 */
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		struct skl_pipe_wm pipe_wm = {};
> -		bool wm_changed;
> -
> -		if (this_crtc->pipe == intel_crtc->pipe)
> -			continue;
> -
> -		if (!intel_crtc->active)
> -			continue;
> -
> -		skl_update_pipe_wm(intel_crtc->base.state,
> -				   &r->ddb, &pipe_wm, &wm_changed);
> -
> -		/*
> -		 * If we end up re-computing the other pipe WM values, it's
> -		 * because it was really needed, so we expect the WM values to
> -		 * be different.
> -		 */
> -		WARN_ON(!wm_changed);
> -
> -		skl_compute_wm_results(dev, &pipe_wm, r, intel_crtc);
> -		r->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
> -	}
> -}
> -
> -static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
> -{
> -	watermarks->wm_linetime[pipe] = 0;
> -	memset(watermarks->plane[pipe], 0,
> -	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
> -	memset(watermarks->plane_trans[pipe],
> -	       0, sizeof(uint32_t) * I915_MAX_PLANES);
> -	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
> -}
> -
>  static int
>  skl_compute_ddb(struct drm_atomic_state *state)
>  {
> @@ -3783,6 +3709,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct intel_crtc *intel_crtc;
> +	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
>  	unsigned realloc_pipes = dev_priv->active_crtcs;
>  	int ret;
>  
> @@ -3799,8 +3726,10 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	 * any other display updates race with this transaction, so we need
>  	 * to grab the lock on *all* CRTC's.
>  	 */
> -	if (intel_state->active_pipe_changes)
> +	if (intel_state->active_pipe_changes) {
>  		realloc_pipes = ~0;
> +		intel_state->wm_results.dirty_pipes = ~0;
> +	}
>  
>  	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
>  		struct intel_crtc_state *cstate;
> @@ -3809,7 +3738,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  		if (IS_ERR(cstate))
>  			return PTR_ERR(cstate);
>  
> -		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
> +		ret = skl_allocate_pipe_ddb(cstate, ddb);
>  		if (ret)
>  			return ret;
>  	}
> @@ -3822,8 +3751,11 @@ skl_compute_wm(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *cstate;
> -	int ret, i;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct skl_wm_values *results = &intel_state->wm_results;
> +	struct skl_pipe_wm *pipe_wm;
>  	bool changed = false;
> +	int ret, i;
>  
>  	/*
>  	 * If this transaction isn't actually touching any CRTC's, don't
> @@ -3838,10 +3770,45 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	if (!changed)
>  		return 0;
>  
> +	/* Clear all dirty flags */
> +	results->dirty_pipes = 0;
> +
>  	ret = skl_compute_ddb(state);
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Calculate WM's for all pipes that are part of this transaction.
> +	 * Note that the DDB allocation above may have added more CRTC's that
> +	 * weren't otherwise being modified (and set bits in dirty_pipes) if
> +	 * pipe allocations had to change.
> +	 *
> +	 * FIXME:  Now that we're doing this in the atomic check phase, we
> +	 * should allow skl_update_pipe_wm() to return failure in cases where
> +	 * no suitable watermark values can be found.
> +	 */
> +	for_each_crtc_in_state(state, crtc, cstate, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		struct intel_crtc_state *intel_cstate =
> +			to_intel_crtc_state(cstate);
> +
> +		pipe_wm = &intel_cstate->wm.skl.optimal;
> +		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
> +					 &changed);
> +		if (ret)
> +			return ret;
> +
> +		if (changed)
> +			results->dirty_pipes |= drm_crtc_mask(crtc);
> +
> +		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> +			/* This pipe's WM's did not change */
> +			continue;
> +
> +		intel_cstate->update_wm_pre = true;
> +		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -3853,26 +3820,21 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_wm_values *results = &dev_priv->wm.skl_results;
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> -	bool wm_changed;
>  
> -	/* Clear all dirty flags */
> -	results->dirty_pipes = 0;
> -
> -	skl_clear_wm(results, intel_crtc->pipe);
> -
> -	skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed);
> -	if (!wm_changed)
> +	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
>  
> -	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
> -	results->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
> +	intel_crtc->wm.active.skl = *pipe_wm;
> +
> +	mutex_lock(&dev_priv->wm.wm_mutex);
>  
> -	skl_update_other_pipe_wm(dev, crtc, results);
>  	skl_write_wm_values(dev_priv, results);
>  	skl_flush_wm_values(dev_priv, results);
>  
>  	/* store the new configuration */
>  	dev_priv->wm.skl_hw = *results;
> +
> +	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
>  static void ilk_compute_wm_config(struct drm_device *dev,

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 08/16] drm/i915/gen9: Compute DDB allocation at atomic check time (v3)

[Maarten Lankhorst]

Calculate the DDB blocks needed to satisfy the current atomic
transaction at atomic check time.  This is a prerequisite to calculating
SKL watermarks during the 'check' phase and rejecting any configurations
that we can't find valid watermarks for.

Due to the nature of DDB allocation, it's possible for the addition of a
new CRTC to make the watermark configuration already in use on another,
unchanged CRTC become invalid.  A change in which CRTC's are active
triggers a recompute of the entire DDB, which unfortunately means we
need to disallow any other atomic commits from racing with such an
update.  If the active CRTC's change, we need to grab the lock on all
CRTC's and run all CRTC's through their 'check' handler to recompute and
re-check their per-CRTC DDB allocations.

Note that with this patch we only compute the DDB allocation but we
don't actually use the computed values during watermark programming yet.
For ease of review/testing/bisecting, we still recompute the DDB at
watermark programming time and just WARN() if it doesn't match the
precomputed values.  A future patch will switch over to using the
precomputed values once we're sure they're being properly computed.

Another clarifying note:  DDB allocation itself shouldn't ever fail with
the algorithm we use today (i.e., we have enough DDB blocks on BXT to
support the minimum needs of the worst-case scenario of every pipe/plane
enabled at full size).  However the watermarks calculations based on the
DDB may fail and we'll be moving those to the atomic check as well in
future patches.

v2:
 - Skip DDB calculations in the rare case where our transaction doesn't
   actually touch any CRTC's at all.  Assuming at least one CRTC state
   is present in our transaction, then it means we can't race with any
   transactions that would update dev_priv->active_crtcs (which requires
   _all_ CRTC locks).
v3:
 - Also calculate DDB during initial hw readout, to prevent using
   incorrect bios values. (Maarten)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++
 drivers/gpu/drm/i915/intel_display.c | 18 +++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_pm.c      | 71 ++++++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 97fdaaf09e44..d04fdf4cf632 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -339,6 +339,10 @@ struct i915_hotplug {
 #define for_each_intel_crtc(dev, intel_crtc) \
 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
 
+#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) \
+		for_each_if ((crtc_mask) & (1 << drm_crtc_index(&intel_crtc->base)))
+
 #define for_each_intel_encoder(dev, intel_encoder)		\
 	list_for_each_entry(intel_encoder,			\
 			    &(dev)->mode_config.encoder_list,	\
@@ -594,6 +598,7 @@ struct drm_i915_display_funcs {
 				       struct intel_crtc_state *newstate);
 	void (*initial_watermarks)(struct intel_crtc_state *cstate);
 	void (*optimize_watermarks)(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);
 	void (*modeset_commit_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 5c2cf2738798..26ba8c7d1c75 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13310,6 +13310,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 static void calc_watermark_data(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *cstate;
@@ -13339,6 +13340,10 @@ static void calc_watermark_data(struct drm_atomic_state *state)
 		    pstate->crtc_h != pstate->src_h >> 16)
 			intel_state->wm_config.sprites_scaled = true;
 	}
+
+	/* Is there platform-specific watermark information to calculate? */
+	if (dev_priv->display.compute_global_watermarks)
+		dev_priv->display.compute_global_watermarks(state);
 }
 
 /**
@@ -13706,6 +13711,19 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
+	/*
+	 * Temporary sanity check: make sure our pre-computed DDB matches the
+	 * one we actually wind up programming.
+	 *
+	 * Not a great place to put this, but the easiest place we have access
+	 * to both the pre-computed and final DDB's; we'll be removing this
+	 * check in the next patch anyway.
+	 */
+	WARN(IS_GEN9(dev) &&
+	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
+		    sizeof(intel_state->ddb)),
+	     "Pre-computed DDB does not match final DDB!\n");
+
 	if (intel_state->modeset)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c01edf390721..fa59784b3998 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -311,6 +311,9 @@ struct intel_atomic_state {
 	 * don't bother calculating intermediate watermarks.
 	 */
 	bool skip_intermediate_wm;
+
+	/* Gen9+ only */
+	struct skl_ddb_allocation ddb;
 };
 
 struct intel_plane_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 540aa7d3a0e2..ab9c18dee567 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3812,6 +3812,76 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
 
 }
 
+static int
+skl_compute_ddb(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct intel_crtc *intel_crtc;
+	unsigned realloc_pipes = dev_priv->active_crtcs;
+	int ret;
+
+	/*
+	 * If the modeset changes which CRTC's are active, we need to
+	 * recompute the DDB allocation for *all* active pipes, even
+	 * those that weren't otherwise being modified in any way by this
+	 * atomic commit.  Due to the shrinking of the per-pipe allocations
+	 * when new active CRTC's are added, it's possible for a pipe that
+	 * we were already using and aren't changing at all here to suddenly
+	 * become invalid if its DDB needs exceeds its new allocation.
+	 *
+	 * Note that if we wind up doing a full DDB recompute, we can't let
+	 * any other display updates race with this transaction, so we need
+	 * to grab the lock on *all* CRTC's.
+	 */
+	if (intel_state->active_pipe_changes ||
+	    intel_state->skip_intermediate_wm)
+		realloc_pipes = ~0;
+
+	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
+		struct intel_crtc_state *cstate;
+
+		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
+		if (IS_ERR(cstate))
+			return PTR_ERR(cstate);
+
+		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+skl_compute_wm(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
+	int ret, i;
+	bool changed = false;
+
+	/*
+	 * If this transaction isn't actually touching any CRTC's, don't
+	 * bother with watermark calculation.  Note that if we pass this
+	 * test, we're guaranteed to hold at least one CRTC state mutex,
+	 * which means we can safely use values like dev_priv->active_crtcs
+	 * since any racing commits that want to update them would need to
+	 * hold _all_ CRTC state mutexes.
+	 */
+	for_each_crtc_in_state(state, crtc, cstate, i)
+		changed = true;
+	if (!changed)
+		return 0;
+
+	ret = skl_compute_ddb(state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static void skl_update_wm(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -7368,6 +7438,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.compute_global_watermarks = skl_compute_wm;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		ilk_setup_wm_latency(dev);
 
-- 
2.5.5


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

Re: [PATCH v3.1 08/16] drm/i915/gen9: Compute DDB allocation at atomic check time (v3)

[Matt Roper]

On Mon, May 02, 2016 at 02:42:51PM +0200, Maarten Lankhorst wrote:
> Calculate the DDB blocks needed to satisfy the current atomic
> transaction at atomic check time.  This is a prerequisite to calculating
> SKL watermarks during the 'check' phase and rejecting any configurations
> that we can't find valid watermarks for.
> 
> Due to the nature of DDB allocation, it's possible for the addition of a
> new CRTC to make the watermark configuration already in use on another,
> unchanged CRTC become invalid.  A change in which CRTC's are active
> triggers a recompute of the entire DDB, which unfortunately means we
> need to disallow any other atomic commits from racing with such an
> update.  If the active CRTC's change, we need to grab the lock on all
> CRTC's and run all CRTC's through their 'check' handler to recompute and
> re-check their per-CRTC DDB allocations.
> 
> Note that with this patch we only compute the DDB allocation but we
> don't actually use the computed values during watermark programming yet.
> For ease of review/testing/bisecting, we still recompute the DDB at
> watermark programming time and just WARN() if it doesn't match the
> precomputed values.  A future patch will switch over to using the
> precomputed values once we're sure they're being properly computed.
> 
> Another clarifying note:  DDB allocation itself shouldn't ever fail with
> the algorithm we use today (i.e., we have enough DDB blocks on BXT to
> support the minimum needs of the worst-case scenario of every pipe/plane
> enabled at full size).  However the watermarks calculations based on the
> DDB may fail and we'll be moving those to the atomic check as well in
> future patches.
> 
> v2:
>  - Skip DDB calculations in the rare case where our transaction doesn't
>    actually touch any CRTC's at all.  Assuming at least one CRTC state
>    is present in our transaction, then it means we can't race with any
>    transactions that would update dev_priv->active_crtcs (which requires
>    _all_ CRTC locks).
> v3:
>  - Also calculate DDB during initial hw readout, to prevent using
>    incorrect bios values. (Maarten)

Yep, I suspect this should fix the final bug reported by Lyude and
others on this patch series.  I won't have hardware access to actually
test this out myself until next week though.

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

Thanks for taking care of this in my absence!

It might be worth adding a small blurb to the comment right above the
check of skip_intermediate_wm to explain why we're checking that field;
it was initially added and named for the two-stage watermark programming
we do on pre-gen9, so the name doesn't really represent how we're now
also using it on gen9 now (where there are no 'intermediate'
watermarks).  I suspect we'll probably just rename the field completely
in the future.


Matt

> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  5 +++
>  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 71 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 97 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 97fdaaf09e44..d04fdf4cf632 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -339,6 +339,10 @@ struct i915_hotplug {
>  #define for_each_intel_crtc(dev, intel_crtc) \
>  	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
>  
> +#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \
> +	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) \
> +		for_each_if ((crtc_mask) & (1 << drm_crtc_index(&intel_crtc->base)))
> +
>  #define for_each_intel_encoder(dev, intel_encoder)		\
>  	list_for_each_entry(intel_encoder,			\
>  			    &(dev)->mode_config.encoder_list,	\
> @@ -594,6 +598,7 @@ struct drm_i915_display_funcs {
>  				       struct intel_crtc_state *newstate);
>  	void (*initial_watermarks)(struct intel_crtc_state *cstate);
>  	void (*optimize_watermarks)(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);
>  	void (*modeset_commit_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 5c2cf2738798..26ba8c7d1c75 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13310,6 +13310,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  static void calc_watermark_data(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *cstate;
> @@ -13339,6 +13340,10 @@ static void calc_watermark_data(struct drm_atomic_state *state)
>  		    pstate->crtc_h != pstate->src_h >> 16)
>  			intel_state->wm_config.sprites_scaled = true;
>  	}
> +
> +	/* Is there platform-specific watermark information to calculate? */
> +	if (dev_priv->display.compute_global_watermarks)
> +		dev_priv->display.compute_global_watermarks(state);
>  }
>  
>  /**
> @@ -13706,6 +13711,19 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	/*
> +	 * Temporary sanity check: make sure our pre-computed DDB matches the
> +	 * one we actually wind up programming.
> +	 *
> +	 * Not a great place to put this, but the easiest place we have access
> +	 * to both the pre-computed and final DDB's; we'll be removing this
> +	 * check in the next patch anyway.
> +	 */
> +	WARN(IS_GEN9(dev) &&
> +	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> +		    sizeof(intel_state->ddb)),
> +	     "Pre-computed DDB does not match final DDB!\n");
> +
>  	if (intel_state->modeset)
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c01edf390721..fa59784b3998 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -311,6 +311,9 @@ struct intel_atomic_state {
>  	 * don't bother calculating intermediate watermarks.
>  	 */
>  	bool skip_intermediate_wm;
> +
> +	/* Gen9+ only */
> +	struct skl_ddb_allocation ddb;
>  };
>  
>  struct intel_plane_state {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 540aa7d3a0e2..ab9c18dee567 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3812,6 +3812,76 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
>  
>  }
>  
> +static int
> +skl_compute_ddb(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct intel_crtc *intel_crtc;
> +	unsigned realloc_pipes = dev_priv->active_crtcs;
> +	int ret;
> +
> +	/*
> +	 * If the modeset changes which CRTC's are active, we need to
> +	 * recompute the DDB allocation for *all* active pipes, even
> +	 * those that weren't otherwise being modified in any way by this
> +	 * atomic commit.  Due to the shrinking of the per-pipe allocations
> +	 * when new active CRTC's are added, it's possible for a pipe that
> +	 * we were already using and aren't changing at all here to suddenly
> +	 * become invalid if its DDB needs exceeds its new allocation.
> +	 *
> +	 * Note that if we wind up doing a full DDB recompute, we can't let
> +	 * any other display updates race with this transaction, so we need
> +	 * to grab the lock on *all* CRTC's.
> +	 */
> +	if (intel_state->active_pipe_changes ||
> +	    intel_state->skip_intermediate_wm)
> +		realloc_pipes = ~0;
> +
> +	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
> +		struct intel_crtc_state *cstate;
> +
> +		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> +		if (IS_ERR(cstate))
> +			return PTR_ERR(cstate);
> +
> +		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +skl_compute_wm(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	int ret, i;
> +	bool changed = false;
> +
> +	/*
> +	 * If this transaction isn't actually touching any CRTC's, don't
> +	 * bother with watermark calculation.  Note that if we pass this
> +	 * test, we're guaranteed to hold at least one CRTC state mutex,
> +	 * which means we can safely use values like dev_priv->active_crtcs
> +	 * since any racing commits that want to update them would need to
> +	 * hold _all_ CRTC state mutexes.
> +	 */
> +	for_each_crtc_in_state(state, crtc, cstate, i)
> +		changed = true;
> +	if (!changed)
> +		return 0;
> +
> +	ret = skl_compute_ddb(state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static void skl_update_wm(struct drm_crtc *crtc)
>  {
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -7368,6 +7438,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.compute_global_watermarks = skl_compute_wm;
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		ilk_setup_wm_latency(dev);
>  
> -- 
> 2.5.5
> 
> 

-- 
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

Re: [PATCH v3.1 08/16] drm/i915/gen9: Compute DDB allocation at atomic check time (v3)

[Sripada, Radhakrishna]

I was facing the same issue that was reported by Lyude both on Arch Linux and ChromeOs.
 This patch did not help me while testing on a SKL RVP. 

The check for skip_intermediate_wm would not pass on gen9. sanitize_watermarks() 
would return immediately as dev_priv->display.optimize_watermarks is not implemented.
On creating a dummy function and initializing optimize_watermarks the display did not 
come up as skl_ddb_get_pipe_allocation_limits() computes ddb allocation from the values 
cached in dev_priv.

Forcing skl_ddb_get_pipe_allocation_limits() to calculate ddb I got a divide-by-zero oops 
while calculating pipe_size.


The hack that I tried to get display working.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c460e67..490c657 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2889,12 +2889,12 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
         */
        if (!intel_state->active_pipe_changes) {
                *alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
-               return;
+//             return;
        }

        nth_active_pipe = hweight32(intel_state->active_crtcs &
                                    (drm_crtc_mask(for_crtc) - 1));
        pipe_size = ddb_size / hweight32(intel_state->active_crtcs);  <===== Divide-by-zero oops
        alloc->start = nth_active_pipe * ddb_size / *num_active;
        alloc->end = alloc->start + pipe_size;
 }
@@ -3928,6 +3928,10 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
        mutex_unlock(&dev_priv->wm.wm_mutex);
 }

+static void skl_optimize_watermarks(struct intel_crtc_state *cstate)
+{
+}
+
 static void skl_pipe_wm_active_state(uint32_t val,
                                     struct skl_pipe_wm *active,
                                     bool is_transwm,
@@ -7404,6 +7408,7 @@ void intel_init_pm(struct drm_device *dev)
                skl_setup_wm_latency(dev);
                dev_priv->display.update_wm = skl_update_wm;
                dev_priv->display.compute_global_watermarks = skl_compute_wm;
+                       dev_priv->display.optimize_watermarks = skl_optimize_watermarks;
        } else if (HAS_PCH_SPLIT(dev)) {
                ilk_setup_wm_latency(dev);

Thanks,
RK



> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Matt Roper
> Sent: Tuesday, May 03, 2016 1:44 PM
> To: Maarten Lankhorst
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3.1 08/16] drm/i915/gen9: Compute DDB
> allocation at atomic check time (v3)
> 
> On Mon, May 02, 2016 at 02:42:51PM +0200, Maarten Lankhorst wrote:
> > Calculate the DDB blocks needed to satisfy the current atomic
> > transaction at atomic check time.  This is a prerequisite to
> > calculating SKL watermarks during the 'check' phase and rejecting any
> > configurations that we can't find valid watermarks for.
> >
> > Due to the nature of DDB allocation, it's possible for the addition of
> > a new CRTC to make the watermark configuration already in use on
> > another, unchanged CRTC become invalid.  A change in which CRTC's are
> > active triggers a recompute of the entire DDB, which unfortunately
> > means we need to disallow any other atomic commits from racing with
> > such an update.  If the active CRTC's change, we need to grab the lock
> > on all CRTC's and run all CRTC's through their 'check' handler to
> > recompute and re-check their per-CRTC DDB allocations.
> >
> > Note that with this patch we only compute the DDB allocation but we
> > don't actually use the computed values during watermark programming yet.
> > For ease of review/testing/bisecting, we still recompute the DDB at
> > watermark programming time and just WARN() if it doesn't match the
> > precomputed values.  A future patch will switch over to using the
> > precomputed values once we're sure they're being properly computed.
> >
> > Another clarifying note:  DDB allocation itself shouldn't ever fail
> > with the algorithm we use today (i.e., we have enough DDB blocks on
> > BXT to support the minimum needs of the worst-case scenario of every
> > pipe/plane enabled at full size).  However the watermarks calculations
> > based on the DDB may fail and we'll be moving those to the atomic
> > check as well in future patches.
> >
> > v2:
> >  - Skip DDB calculations in the rare case where our transaction doesn't
> >    actually touch any CRTC's at all.  Assuming at least one CRTC state
> >    is present in our transaction, then it means we can't race with any
> >    transactions that would update dev_priv->active_crtcs (which requires
> >    _all_ CRTC locks).
> > v3:
> >  - Also calculate DDB during initial hw readout, to prevent using
> >    incorrect bios values. (Maarten)
> 
> Yep, I suspect this should fix the final bug reported by Lyude and others on this
> patch series.  I won't have hardware access to actually test this out myself until
> next week though.
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Thanks for taking care of this in my absence!
> 
> It might be worth adding a small blurb to the comment right above the check of
> skip_intermediate_wm to explain why we're checking that field; it was initially
> added and named for the two-stage watermark programming we do on pre-
> gen9, so the name doesn't really represent how we're now also using it on gen9
> now (where there are no 'intermediate'
> watermarks).  I suspect we'll probably just rename the field completely in the
> future.
> 
> 
> Matt
> 
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  5 +++
> >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
> >  drivers/gpu/drm/i915/intel_pm.c      | 71
> ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 97fdaaf09e44..d04fdf4cf632
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -339,6 +339,10 @@ struct i915_hotplug {  #define
> > for_each_intel_crtc(dev, intel_crtc) \
> >  	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> > base.head)
> >
> > +#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \
> > +	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list,
> base.head) \
> > +		for_each_if ((crtc_mask) & (1 <<
> > +drm_crtc_index(&intel_crtc->base)))
> > +
> >  #define for_each_intel_encoder(dev, intel_encoder)		\
> >  	list_for_each_entry(intel_encoder,			\
> >  			    &(dev)->mode_config.encoder_list,	\
> > @@ -594,6 +598,7 @@ struct drm_i915_display_funcs {
> >  				       struct intel_crtc_state *newstate);
> >  	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> >  	void (*optimize_watermarks)(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);
> >  	void (*modeset_commit_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 5c2cf2738798..26ba8c7d1c75 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13310,6 +13310,7 @@ static int intel_modeset_checks(struct
> > drm_atomic_state *state)  static void calc_watermark_data(struct
> > drm_atomic_state *state)  {
> >  	struct drm_device *dev = state->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >  	struct drm_crtc *crtc;
> >  	struct drm_crtc_state *cstate;
> > @@ -13339,6 +13340,10 @@ static void calc_watermark_data(struct
> drm_atomic_state *state)
> >  		    pstate->crtc_h != pstate->src_h >> 16)
> >  			intel_state->wm_config.sprites_scaled = true;
> >  	}
> > +
> > +	/* Is there platform-specific watermark information to calculate? */
> > +	if (dev_priv->display.compute_global_watermarks)
> > +		dev_priv->display.compute_global_watermarks(state);
> >  }
> >
> >  /**
> > @@ -13706,6 +13711,19 @@ static int intel_atomic_commit(struct
> drm_device *dev,
> >  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
> >  	}
> >
> > +	/*
> > +	 * Temporary sanity check: make sure our pre-computed DDB matches
> the
> > +	 * one we actually wind up programming.
> > +	 *
> > +	 * Not a great place to put this, but the easiest place we have access
> > +	 * to both the pre-computed and final DDB's; we'll be removing this
> > +	 * check in the next patch anyway.
> > +	 */
> > +	WARN(IS_GEN9(dev) &&
> > +	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> > +		    sizeof(intel_state->ddb)),
> > +	     "Pre-computed DDB does not match final DDB!\n");
> > +
> >  	if (intel_state->modeset)
> >  		intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index c01edf390721..fa59784b3998 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -311,6 +311,9 @@ struct intel_atomic_state {
> >  	 * don't bother calculating intermediate watermarks.
> >  	 */
> >  	bool skip_intermediate_wm;
> > +
> > +	/* Gen9+ only */
> > +	struct skl_ddb_allocation ddb;
> >  };
> >
> >  struct intel_plane_state {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c index 540aa7d3a0e2..ab9c18dee567
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3812,6 +3812,76 @@ static void skl_clear_wm(struct skl_wm_values
> > *watermarks, enum pipe pipe)
> >
> >  }
> >
> > +static int
> > +skl_compute_ddb(struct drm_atomic_state *state) {
> > +	struct drm_device *dev = state->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	struct intel_crtc *intel_crtc;
> > +	unsigned realloc_pipes = dev_priv->active_crtcs;
> > +	int ret;
> > +
> > +	/*
> > +	 * If the modeset changes which CRTC's are active, we need to
> > +	 * recompute the DDB allocation for *all* active pipes, even
> > +	 * those that weren't otherwise being modified in any way by this
> > +	 * atomic commit.  Due to the shrinking of the per-pipe allocations
> > +	 * when new active CRTC's are added, it's possible for a pipe that
> > +	 * we were already using and aren't changing at all here to suddenly
> > +	 * become invalid if its DDB needs exceeds its new allocation.
> > +	 *
> > +	 * Note that if we wind up doing a full DDB recompute, we can't let
> > +	 * any other display updates race with this transaction, so we need
> > +	 * to grab the lock on *all* CRTC's.
> > +	 */
> > +	if (intel_state->active_pipe_changes ||
> > +	    intel_state->skip_intermediate_wm)
> > +		realloc_pipes = ~0;
> > +
> > +	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
> > +		struct intel_crtc_state *cstate;
> > +
> > +		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> > +		if (IS_ERR(cstate))
> > +			return PTR_ERR(cstate);
> > +
> > +		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +skl_compute_wm(struct drm_atomic_state *state) {
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *cstate;
> > +	int ret, i;
> > +	bool changed = false;
> > +
> > +	/*
> > +	 * If this transaction isn't actually touching any CRTC's, don't
> > +	 * bother with watermark calculation.  Note that if we pass this
> > +	 * test, we're guaranteed to hold at least one CRTC state mutex,
> > +	 * which means we can safely use values like dev_priv->active_crtcs
> > +	 * since any racing commits that want to update them would need to
> > +	 * hold _all_ CRTC state mutexes.
> > +	 */
> > +	for_each_crtc_in_state(state, crtc, cstate, i)
> > +		changed = true;
> > +	if (!changed)
> > +		return 0;
> > +
> > +	ret = skl_compute_ddb(state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static void skl_update_wm(struct drm_crtc *crtc)  {
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -7368,6
> > +7438,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.compute_global_watermarks =
> skl_compute_wm;
> >  	} else if (HAS_PCH_SPLIT(dev)) {
> >  		ilk_setup_wm_latency(dev);
> >
> > --
> > 2.5.5
> >
> >
> 
> --
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 09/16] drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)

[Lyude Paul]

On Thu, 2016-04-21 at 16:17 -0700, Matt Roper wrote:
> Now that we're properly pre-allocating the DDB during the atomic check
> phase and we trust that the allocation is appropriate, let's actually
> use the allocation computed and not duplicate that work during the
> commit phase.
> 
> v2:
>  - Significant rebasing now that we can use cached data rates and
>    minimum block allocations to avoid grabbing additional plane states.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  14 +--
>  drivers/gpu/drm/i915/intel_pm.c      | 224 +++++++++++-----------------------
> -
>  2 files changed, 67 insertions(+), 171 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index c970e3e..80d8f77 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13599,6 +13599,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_swap_state(dev, state);
>  	dev_priv->wm.config = intel_state->wm_config;
> +	dev_priv->wm.skl_results.ddb = intel_state->ddb;
>  	intel_shared_dpll_commit(state);
>  
>  	if (intel_state->modeset) {
> @@ -13716,19 +13717,6 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> -	/*
> -	 * Temporary sanity check: make sure our pre-computed DDB matches the
> -	 * one we actually wind up programming.
> -	 *
> -	 * Not a great place to put this, but the easiest place we have
> access
> -	 * to both the pre-computed and final DDB's; we'll be removing this
> -	 * check in the next patch anyway.
> -	 */
> -	WARN(IS_GEN9(dev) &&
> -	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> -		    sizeof(intel_state->ddb)),
> -	     "Pre-computed DDB does not match final DDB!\n");
> -
>  	if (intel_state->modeset)
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fca44f8..f28fd36 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2849,7 +2849,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
> -				   struct intel_wm_config *config,
>  				   struct skl_ddb_entry *alloc, /* out */
>  				   int *num_active /* out */)
>  {
> @@ -2857,24 +2856,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> *dev,
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
> -	struct drm_crtc *crtc;
>  	unsigned int pipe_size, ddb_size;
>  	int nth_active_pipe;
>  	int pipe = to_intel_crtc(for_crtc)->pipe;
>  
> -	if (intel_state && intel_state->active_pipe_changes)
> -		*num_active = hweight32(intel_state->active_crtcs);
> -	else if (intel_state)
> -		*num_active = hweight32(dev_priv->active_crtcs);
> -	else
> -		*num_active = config->num_pipes_active;
> -
> -	if (!cstate->base.active) {
> +	if (WARN_ON(!state) || !cstate->base.active) {
>  		alloc->start = 0;
>  		alloc->end = 0;
> +		*num_active = hweight32(dev_priv->active_crtcs);
>  		return;
>  	}
>  
> +	if (intel_state->active_pipe_changes)
> +		*num_active = hweight32(intel_state->active_crtcs);
> +	else
> +		*num_active = hweight32(dev_priv->active_crtcs);
> +
>  	if (IS_BROXTON(dev))
>  		ddb_size = BXT_DDB_SIZE;
>  	else
> @@ -2883,50 +2880,23 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> *dev,
>  	ddb_size -= 4; /* 4 blocks for bypass path allocation */
>  
>  	/*
> -	 * FIXME: At the moment we may be called on either in-flight or fully
> -	 * committed cstate's.  Once we fully move DDB allocation in the
> check
> -	 * phase, we'll only be called on in-flight states and the 'else'
> -	 * branch here will go away.
> -	 *
> -	 * The 'else' branch is slightly racy here, but it was racy to begin
> -	 * with; since it's going away soon, no effort is made to address
> that.
> +	 * If the state doesn't change the active CRTC's, then there's
> +	 * no need to recalculate; the existing pipe allocation limits
> +	 * should remain unchanged.  Note that we're safe from racing
> +	 * commits since any racing commit that changes the active CRTC
> +	 * list would need to grab _all_ crtc locks, including the one
> +	 * we currently hold.
>  	 */
> -	if (state) {
> -		/*
> -		 * If the state doesn't change the active CRTC's, then
> there's
> -		 * no need to recalculate; the existing pipe allocation
> limits
> -		 * should remain unchanged.  Note that we're safe from racing
> -		 * commits since any racing commit that changes the active
> CRTC
> -		 * list would need to grab _all_ crtc locks, including the
> one
> -		 * we currently hold.
> -		 */
> -		if (!intel_state->active_pipe_changes) {
> -			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> -			return;
> -		}
> -
> -		nth_active_pipe = hweight32(intel_state->active_crtcs &
> -					    (drm_crtc_mask(for_crtc) - 1));
> -		pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> -		alloc->start = nth_active_pipe * ddb_size / *num_active;
> -		alloc->end = alloc->start + pipe_size;
> -	} else {
> -		nth_active_pipe = 0;
> -		for_each_crtc(dev, crtc) {
> -			if (!to_intel_crtc(crtc)->active)
> -				continue;
> -
> -			if (crtc == for_crtc)
> -				break;
> -
> -			nth_active_pipe++;
> -		}
> -
> -		pipe_size = ddb_size / config->num_pipes_active;
> -		alloc->start = nth_active_pipe * ddb_size /
> -			config->num_pipes_active;
> -		alloc->end = alloc->start + pipe_size;
> +	if (!intel_state->active_pipe_changes) {
> +		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];

So I finally figured out what's causing all of the valid wm calculations to get
rejected, leading to blank screens etc. etc. The problem is this line right here
where we assign *alloc, we never actually set dev_priv->wm.skl_hw.ddb.pipe[pipe] 
to a value anywhere so we end up having bogus ddb entry sizes. If you change it
to something like:

		*alloc = dev_priv->wm.skl_hw.ddb.plane[pipe][intel_crtc->plane];

We get the right values and everything starts working as expected. BTW: Things
seem to be pretty stable with this patchset after this fix, haven't tried it on
any other platforms then skl yet though so I'll let you know if I run into any
more problems.

> +		return;
>  	}
> +
> +	nth_active_pipe = hweight32(intel_state->active_crtcs &
> +				    (drm_crtc_mask(for_crtc) - 1));
> +	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +	alloc->start = nth_active_pipe * ddb_size / *num_active;
> +	alloc->end = alloc->start + pipe_size;
>  }
>  
>  static unsigned int skl_cursor_allocation(int num_active)
> @@ -3025,62 +2995,33 @@ 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;
>  	const struct intel_plane *intel_plane;
> +	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 */
> -	/*
> -	 * FIXME: At the moment this function can be called on either an
> -	 * in-flight or a committed state object.  If it's in-flight then we
> -	 * only want to re-calculate the plane data rate for planes that are
> -	 * part of the transaction (i.e., we don't want to grab any
> additional
> -	 * plane states if we don't have to).  If we're operating on
> committed
> -	 * state, we'll just go ahead and recalculate the plane data rate for
> -	 * all planes.
> -	 *
> -	 * Once we finish moving our DDB allocation to the atomic check
> phase,
> -	 * we'll only be calling this function on in-flight state objects, so
> -	 * the 'else' branch here will go away.
> -	 */
> -	if (state) {
> -		struct drm_plane *plane;
> -		struct drm_plane_state *pstate;
> -		int i;
> -
> -		for_each_plane_in_state(state, plane, pstate, i) {
> -			intel_plane = to_intel_plane(plane);
> -			id = skl_wm_plane_id(intel_plane);
> -
> -			if (intel_plane->pipe != intel_crtc->pipe)
> -				continue;
> -
> -			/* packed/uv */
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    pstate, 0);
> -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> -
> -			/* y-plane */
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    pstate, 1);
> -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> -		}
> -	} else {
> -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -			const struct drm_plane_state *pstate =
> -				intel_plane->base.state;
> -			int id = skl_wm_plane_id(intel_plane);
> +	for_each_plane_in_state(state, plane, pstate, i) {
> +		id = skl_wm_plane_id(to_intel_plane(plane));
> +		intel_plane = to_intel_plane(plane);
>  
> -			/* packed/uv */
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    pstate, 0);
> -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> +		if (intel_plane->pipe != intel_crtc->pipe)
> +			continue;
>  
> -			/* y-plane */
> -			rate = skl_plane_relative_data_rate(intel_cstate,
> -							    pstate, 1);
> -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> -		}
> +		/* packed/uv */
> +		rate = skl_plane_relative_data_rate(intel_cstate,
> +						    pstate, 0);
> +		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> +
> +		/* y-plane */
> +		rate = skl_plane_relative_data_rate(intel_cstate,
> +						    pstate, 1);
> +		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
>  	}
>  
>  	/* Calculate CRTC's total data rate from cached values */
> @@ -3104,8 +3045,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	struct drm_atomic_state *state = cstate->base.state;
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane;
>  	struct drm_plane *plane;
> @@ -3119,6 +3058,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	if (WARN_ON(!state))
> +		return 0;
> +
>  	if (!cstate->base.active) {
>  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
>  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3126,8 +3068,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		return 0;
>  	}
>  
> -	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
> -					   &num_active);
> +	skl_ddb_get_pipe_allocation_limits(dev, cstate, alloc, &num_active);
>  	alloc_size = skl_ddb_entry_size(alloc);
>  	if (alloc_size == 0) {
>  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3139,53 +3080,31 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
>  
>  	alloc_size -= cursor_blocks;
> -	alloc->end -= cursor_blocks;
>  
>  	/* 1. Allocate the mininum required blocks for each active plane */
> -	/*
> -	 * TODO: Remove support for already-committed state once we
> -	 * only allocate DDB on in-flight states.
> -	 */
> -	if (state) {
> -		for_each_plane_in_state(state, plane, pstate, i) {
> -			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)->visible) {
> -				minimum[id] = 0;
> -				y_minimum[id] = 0;
> -				continue;
> -			}
> -			if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> -				minimum[id] = 0;
> -				y_minimum[id] = 0;
> -				continue;
> -			}
> -
> -			minimum[id] = 8;
> -			if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> -				y_minimum[id] = 8;
> -			else
> -				y_minimum[id] = 0;
> -		}
> -	} else {
> -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -			struct drm_plane *plane = &intel_plane->base;
> -			struct drm_framebuffer *fb = plane->state->fb;
> -			int id = skl_wm_plane_id(intel_plane);
> -
> -			if (!to_intel_plane_state(plane->state)->visible)
> -				continue;
> +	for_each_plane_in_state(state, plane, pstate, i) {
> +		intel_plane = to_intel_plane(plane);
> +		id = skl_wm_plane_id(intel_plane);
>  
> -			if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -				continue;
> +		if (intel_plane->pipe != pipe)
> +			continue;
>  
> -			minimum[id] = 8;
> -			y_minimum[id] = (fb->pixel_format == DRM_FORMAT_NV12)
> ? 8 : 0;
> +		if (!to_intel_plane_state(pstate)->visible) {
> +			minimum[id] = 0;
> +			y_minimum[id] = 0;
> +			continue;
> +		}
> +		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> +			minimum[id] = 0;
> +			y_minimum[id] = 0;
> +			continue;
>  		}
> +
> +		minimum[id] = 8;
> +		if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> +			y_minimum[id] = 8;
> +		else
> +			y_minimum[id] = 0;
>  	}
>  
>  	for (i = 0; i < PLANE_CURSOR; i++) {
> @@ -3736,7 +3655,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  
> -	WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
>  	skl_build_pipe_wm(cstate, ddb, pipe_wm);
>  
>  	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> @@ -3800,16 +3718,6 @@ static void skl_clear_wm(struct skl_wm_values
> *watermarks, enum pipe pipe)
>  	memset(watermarks->plane_trans[pipe],
>  	       0, sizeof(uint32_t) * I915_MAX_PLANES);
>  	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
> -
> -	/* Clear ddb entries for pipe */
> -	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct skl_ddb_entry));
> -	memset(&watermarks->ddb.plane[pipe], 0,
> -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> -	memset(&watermarks->ddb.y_plane[pipe], 0,
> -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> -	memset(&watermarks->ddb.plane[pipe][PLANE_CURSOR], 0,
> -	       sizeof(struct skl_ddb_entry));
> -
>  }
>  
>  static int
-- 
Cheers,
	Lyude

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

Re: [PATCH v3 09/16] drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)

[Lyude Paul]

On Fri, 2016-05-06 at 14:12 -0400, Lyude Paul wrote:
> On Thu, 2016-04-21 at 16:17 -0700, Matt Roper wrote:
> > 
> > Now that we're properly pre-allocating the DDB during the atomic check
> > phase and we trust that the allocation is appropriate, let's actually
> > use the allocation computed and not duplicate that work during the
> > commit phase.
> > 
> > v2:
> >  - Significant rebasing now that we can use cached data rates and
> >    minimum block allocations to avoid grabbing additional plane states.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  14 +--
> >  drivers/gpu/drm/i915/intel_pm.c      | 224 +++++++++++---------------------
> > --
> > -
> >  2 files changed, 67 insertions(+), 171 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index c970e3e..80d8f77 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13599,6 +13599,7 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> >  
> >  	drm_atomic_helper_swap_state(dev, state);
> >  	dev_priv->wm.config = intel_state->wm_config;
> > +	dev_priv->wm.skl_results.ddb = intel_state->ddb;
> >  	intel_shared_dpll_commit(state);
> >  
> >  	if (intel_state->modeset) {
> > @@ -13716,19 +13717,6 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> >  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc-
> > >state);
> >  	}
> >  
> > -	/*
> > -	 * Temporary sanity check: make sure our pre-computed DDB matches
> > the
> > -	 * one we actually wind up programming.
> > -	 *
> > -	 * Not a great place to put this, but the easiest place we have
> > access
> > -	 * to both the pre-computed and final DDB's; we'll be removing this
> > -	 * check in the next patch anyway.
> > -	 */
> > -	WARN(IS_GEN9(dev) &&
> > -	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> > -		    sizeof(intel_state->ddb)),
> > -	     "Pre-computed DDB does not match final DDB!\n");
> > -
> >  	if (intel_state->modeset)
> >  		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index fca44f8..f28fd36 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2849,7 +2849,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
> >  static void
> >  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> >  				   const struct intel_crtc_state *cstate,
> > -				   struct intel_wm_config *config,
> >  				   struct skl_ddb_entry *alloc, /* out */
> >  				   int *num_active /* out */)
> >  {
> > @@ -2857,24 +2856,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> > *dev,
> >  	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_crtc *for_crtc = cstate->base.crtc;
> > -	struct drm_crtc *crtc;
> >  	unsigned int pipe_size, ddb_size;
> >  	int nth_active_pipe;
> >  	int pipe = to_intel_crtc(for_crtc)->pipe;
> >  
> > -	if (intel_state && intel_state->active_pipe_changes)
> > -		*num_active = hweight32(intel_state->active_crtcs);
> > -	else if (intel_state)
> > -		*num_active = hweight32(dev_priv->active_crtcs);
> > -	else
> > -		*num_active = config->num_pipes_active;
> > -
> > -	if (!cstate->base.active) {
> > +	if (WARN_ON(!state) || !cstate->base.active) {
> >  		alloc->start = 0;
> >  		alloc->end = 0;
> > +		*num_active = hweight32(dev_priv->active_crtcs);
> >  		return;
> >  	}
> >  
> > +	if (intel_state->active_pipe_changes)
> > +		*num_active = hweight32(intel_state->active_crtcs);
> > +	else
> > +		*num_active = hweight32(dev_priv->active_crtcs);
> > +
> >  	if (IS_BROXTON(dev))
> >  		ddb_size = BXT_DDB_SIZE;
> >  	else
> > @@ -2883,50 +2880,23 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> > *dev,
> >  	ddb_size -= 4; /* 4 blocks for bypass path allocation */
> >  
> >  	/*
> > -	 * FIXME: At the moment we may be called on either in-flight or
> > fully
> > -	 * committed cstate's.  Once we fully move DDB allocation in the
> > check
> > -	 * phase, we'll only be called on in-flight states and the 'else'
> > -	 * branch here will go away.
> > -	 *
> > -	 * The 'else' branch is slightly racy here, but it was racy to
> > begin
> > -	 * with; since it's going away soon, no effort is made to address
> > that.
> > +	 * If the state doesn't change the active CRTC's, then there's
> > +	 * no need to recalculate; the existing pipe allocation limits
> > +	 * should remain unchanged.  Note that we're safe from racing
> > +	 * commits since any racing commit that changes the active CRTC
> > +	 * list would need to grab _all_ crtc locks, including the one
> > +	 * we currently hold.
> >  	 */
> > -	if (state) {
> > -		/*
> > -		 * If the state doesn't change the active CRTC's, then
> > there's
> > -		 * no need to recalculate; the existing pipe allocation
> > limits
> > -		 * should remain unchanged.  Note that we're safe from
> > racing
> > -		 * commits since any racing commit that changes the active
> > CRTC
> > -		 * list would need to grab _all_ crtc locks, including the
> > one
> > -		 * we currently hold.
> > -		 */
> > -		if (!intel_state->active_pipe_changes) {
> > -			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> > -			return;
> > -		}
> > -
> > -		nth_active_pipe = hweight32(intel_state->active_crtcs &
> > -					    (drm_crtc_mask(for_crtc) - 1));
> > -		pipe_size = ddb_size / hweight32(intel_state-
> > >active_crtcs);
> > -		alloc->start = nth_active_pipe * ddb_size / *num_active;
> > -		alloc->end = alloc->start + pipe_size;
> > -	} else {
> > -		nth_active_pipe = 0;
> > -		for_each_crtc(dev, crtc) {
> > -			if (!to_intel_crtc(crtc)->active)
> > -				continue;
> > -
> > -			if (crtc == for_crtc)
> > -				break;
> > -
> > -			nth_active_pipe++;
> > -		}
> > -
> > -		pipe_size = ddb_size / config->num_pipes_active;
> > -		alloc->start = nth_active_pipe * ddb_size /
> > -			config->num_pipes_active;
> > -		alloc->end = alloc->start + pipe_size;
> > +	if (!intel_state->active_pipe_changes) {
> > +		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> So I finally figured out what's causing all of the valid wm calculations to
> get
> rejected, leading to blank screens etc. etc. The problem is this line right
> here
> where we assign *alloc, we never actually set dev_priv-
> >wm.skl_hw.ddb.pipe[pipe] 
> to a value anywhere so we end up having bogus ddb entry sizes. If you change
> it
> to something like:
> 
> 		*alloc = dev_priv->wm.skl_hw.ddb.plane[pipe][intel_crtc->plane];
Whoops, I stand corrected on this, since this seems to cause a few issues with
VT switching. We actually want something like this:

		if (!intel_state->active_pipe_changes &&
		    dev_priv->wm.skl_hw.ddb.pipe[pipe].end != 0) {
			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
			return;
		}

> 
> We get the right values and everything starts working as expected. BTW: Things
> seem to be pretty stable with this patchset after this fix, haven't tried it
> on
> any other platforms then skl yet though so I'll let you know if I run into any
> more problems.
> 
> > 
> > +		return;
> >  	}
> > +
> > +	nth_active_pipe = hweight32(intel_state->active_crtcs &
> > +				    (drm_crtc_mask(for_crtc) - 1));
> > +	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> > +	alloc->start = nth_active_pipe * ddb_size / *num_active;
> > +	alloc->end = alloc->start + pipe_size;
> >  }
> >  
> >  static unsigned int skl_cursor_allocation(int num_active)
> > @@ -3025,62 +2995,33 @@ 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;
> >  	const struct intel_plane *intel_plane;
> > +	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 */
> > -	/*
> > -	 * FIXME: At the moment this function can be called on either an
> > -	 * in-flight or a committed state object.  If it's in-flight then
> > we
> > -	 * only want to re-calculate the plane data rate for planes that
> > are
> > -	 * part of the transaction (i.e., we don't want to grab any
> > additional
> > -	 * plane states if we don't have to).  If we're operating on
> > committed
> > -	 * state, we'll just go ahead and recalculate the plane data rate
> > for
> > -	 * all planes.
> > -	 *
> > -	 * Once we finish moving our DDB allocation to the atomic check
> > phase,
> > -	 * we'll only be calling this function on in-flight state objects,
> > so
> > -	 * the 'else' branch here will go away.
> > -	 */
> > -	if (state) {
> > -		struct drm_plane *plane;
> > -		struct drm_plane_state *pstate;
> > -		int i;
> > -
> > -		for_each_plane_in_state(state, plane, pstate, i) {
> > -			intel_plane = to_intel_plane(plane);
> > -			id = skl_wm_plane_id(intel_plane);
> > -
> > -			if (intel_plane->pipe != intel_crtc->pipe)
> > -				continue;
> > -
> > -			/* packed/uv */
> > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > -							    pstate, 0);
> > -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > -
> > -			/* y-plane */
> > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > -							    pstate, 1);
> > -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > -		}
> > -	} else {
> > -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > {
> > -			const struct drm_plane_state *pstate =
> > -				intel_plane->base.state;
> > -			int id = skl_wm_plane_id(intel_plane);
> > +	for_each_plane_in_state(state, plane, pstate, i) {
> > +		id = skl_wm_plane_id(to_intel_plane(plane));
> > +		intel_plane = to_intel_plane(plane);
> >  
> > -			/* packed/uv */
> > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > -							    pstate, 0);
> > -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > +		if (intel_plane->pipe != intel_crtc->pipe)
> > +			continue;
> >  
> > -			/* y-plane */
> > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > -							    pstate, 1);
> > -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > -		}
> > +		/* packed/uv */
> > +		rate = skl_plane_relative_data_rate(intel_cstate,
> > +						    pstate, 0);
> > +		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > +
> > +		/* y-plane */
> > +		rate = skl_plane_relative_data_rate(intel_cstate,
> > +						    pstate, 1);
> > +		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> >  	}
> >  
> >  	/* Calculate CRTC's total data rate from cached values */
> > @@ -3104,8 +3045,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >  	struct drm_atomic_state *state = cstate->base.state;
> >  	struct drm_crtc *crtc = cstate->base.crtc;
> >  	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_wm_config *config = &dev_priv->wm.config;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_plane *intel_plane;
> >  	struct drm_plane *plane;
> > @@ -3119,6 +3058,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >  	int num_active;
> >  	int id, i;
> >  
> > +	if (WARN_ON(!state))
> > +		return 0;
> > +
> >  	if (!cstate->base.active) {
> >  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> >  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > @@ -3126,8 +3068,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> >  		return 0;
> >  	}
> >  
> > -	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
> > -					   &num_active);
> > +	skl_ddb_get_pipe_allocation_limits(dev, cstate, alloc,
> > &num_active);
> >  	alloc_size = skl_ddb_entry_size(alloc);
> >  	if (alloc_size == 0) {
> >  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > @@ -3139,53 +3080,31 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > *cstate,
> >  	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> >  
> >  	alloc_size -= cursor_blocks;
> > -	alloc->end -= cursor_blocks;
> >  
> >  	/* 1. Allocate the mininum required blocks for each active plane */
> > -	/*
> > -	 * TODO: Remove support for already-committed state once we
> > -	 * only allocate DDB on in-flight states.
> > -	 */
> > -	if (state) {
> > -		for_each_plane_in_state(state, plane, pstate, i) {
> > -			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)->visible) {
> > -				minimum[id] = 0;
> > -				y_minimum[id] = 0;
> > -				continue;
> > -			}
> > -			if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > -				minimum[id] = 0;
> > -				y_minimum[id] = 0;
> > -				continue;
> > -			}
> > -
> > -			minimum[id] = 8;
> > -			if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > -				y_minimum[id] = 8;
> > -			else
> > -				y_minimum[id] = 0;
> > -		}
> > -	} else {
> > -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > {
> > -			struct drm_plane *plane = &intel_plane->base;
> > -			struct drm_framebuffer *fb = plane->state->fb;
> > -			int id = skl_wm_plane_id(intel_plane);
> > -
> > -			if (!to_intel_plane_state(plane->state)->visible)
> > -				continue;
> > +	for_each_plane_in_state(state, plane, pstate, i) {
> > +		intel_plane = to_intel_plane(plane);
> > +		id = skl_wm_plane_id(intel_plane);
> >  
> > -			if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > -				continue;
> > +		if (intel_plane->pipe != pipe)
> > +			continue;
> >  
> > -			minimum[id] = 8;
> > -			y_minimum[id] = (fb->pixel_format ==
> > DRM_FORMAT_NV12)
> > ? 8 : 0;
> > +		if (!to_intel_plane_state(pstate)->visible) {
> > +			minimum[id] = 0;
> > +			y_minimum[id] = 0;
> > +			continue;
> > +		}
> > +		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > +			minimum[id] = 0;
> > +			y_minimum[id] = 0;
> > +			continue;
> >  		}
> > +
> > +		minimum[id] = 8;
> > +		if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > +			y_minimum[id] = 8;
> > +		else
> > +			y_minimum[id] = 0;
> >  	}
> >  
> >  	for (i = 0; i < PLANE_CURSOR; i++) {
> > @@ -3736,7 +3655,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> >  
> > -	WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
> >  	skl_build_pipe_wm(cstate, ddb, pipe_wm);
> >  
> >  	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> > @@ -3800,16 +3718,6 @@ static void skl_clear_wm(struct skl_wm_values
> > *watermarks, enum pipe pipe)
> >  	memset(watermarks->plane_trans[pipe],
> >  	       0, sizeof(uint32_t) * I915_MAX_PLANES);
> >  	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
> > -
> > -	/* Clear ddb entries for pipe */
> > -	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct
> > skl_ddb_entry));
> > -	memset(&watermarks->ddb.plane[pipe], 0,
> > -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > -	memset(&watermarks->ddb.y_plane[pipe], 0,
> > -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > -	memset(&watermarks->ddb.plane[pipe][PLANE_CURSOR], 0,
> > -	       sizeof(struct skl_ddb_entry));
> > -
> >  }
> >  
> >  static int
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 00/16] Pre-calculate SKL-style atomic watermarks

[Daniel Stone]

Hi Matt,

On 22 April 2016 at 00:16, Matt Roper <matthew.d.roper@intel.com> wrote:
> For a detailed explanation of this series, please see the original cover letter:
>   https://lists.freedesktop.org/archives/intel-gfx/2016-April/091293.html
>
> This spin just incorporates the final review feedback from Maarten and
> hopefully kicks the CI system (I never got any CI test results back from v2).

For the series, with Maarten and Lyude's amendments:
Tested-by: Daniel Stone <daniels@collabora.com>

I was seeing pretty savage underflows on eDP+DP SKL, but these have
gone away with this patchset on top of -nightly.

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

Re: [PATCH v3 09/16] drm/i915/gen9: Drop re-allocation of DDB at atomic commit (v2)

[Matt Roper]

On Fri, May 06, 2016 at 02:42:46PM -0400, Lyude Paul wrote:
> On Fri, 2016-05-06 at 14:12 -0400, Lyude Paul wrote:
> > On Thu, 2016-04-21 at 16:17 -0700, Matt Roper wrote:
> > > 
> > > Now that we're properly pre-allocating the DDB during the atomic check
> > > phase and we trust that the allocation is appropriate, let's actually
> > > use the allocation computed and not duplicate that work during the
> > > commit phase.
> > > 
> > > v2:
> > >  - Significant rebasing now that we can use cached data rates and
> > >    minimum block allocations to avoid grabbing additional plane states.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  14 +--
> > >  drivers/gpu/drm/i915/intel_pm.c      | 224 +++++++++++---------------------
> > > --
> > > -
> > >  2 files changed, 67 insertions(+), 171 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index c970e3e..80d8f77 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13599,6 +13599,7 @@ static int intel_atomic_commit(struct drm_device
> > > *dev,
> > >  
> > >  	drm_atomic_helper_swap_state(dev, state);
> > >  	dev_priv->wm.config = intel_state->wm_config;
> > > +	dev_priv->wm.skl_results.ddb = intel_state->ddb;
> > >  	intel_shared_dpll_commit(state);
> > >  
> > >  	if (intel_state->modeset) {
> > > @@ -13716,19 +13717,6 @@ static int intel_atomic_commit(struct drm_device
> > > *dev,
> > >  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc-
> > > >state);
> > >  	}
> > >  
> > > -	/*
> > > -	 * Temporary sanity check: make sure our pre-computed DDB matches
> > > the
> > > -	 * one we actually wind up programming.
> > > -	 *
> > > -	 * Not a great place to put this, but the easiest place we have
> > > access
> > > -	 * to both the pre-computed and final DDB's; we'll be removing this
> > > -	 * check in the next patch anyway.
> > > -	 */
> > > -	WARN(IS_GEN9(dev) &&
> > > -	     memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb,
> > > -		    sizeof(intel_state->ddb)),
> > > -	     "Pre-computed DDB does not match final DDB!\n");
> > > -
> > >  	if (intel_state->modeset)
> > >  		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index fca44f8..f28fd36 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2849,7 +2849,6 @@ skl_wm_plane_id(const struct intel_plane *plane)
> > >  static void
> > >  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> > >  				   const struct intel_crtc_state *cstate,
> > > -				   struct intel_wm_config *config,
> > >  				   struct skl_ddb_entry *alloc, /* out */
> > >  				   int *num_active /* out */)
> > >  {
> > > @@ -2857,24 +2856,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> > > *dev,
> > >  	struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct drm_crtc *for_crtc = cstate->base.crtc;
> > > -	struct drm_crtc *crtc;
> > >  	unsigned int pipe_size, ddb_size;
> > >  	int nth_active_pipe;
> > >  	int pipe = to_intel_crtc(for_crtc)->pipe;
> > >  
> > > -	if (intel_state && intel_state->active_pipe_changes)
> > > -		*num_active = hweight32(intel_state->active_crtcs);
> > > -	else if (intel_state)
> > > -		*num_active = hweight32(dev_priv->active_crtcs);
> > > -	else
> > > -		*num_active = config->num_pipes_active;
> > > -
> > > -	if (!cstate->base.active) {
> > > +	if (WARN_ON(!state) || !cstate->base.active) {
> > >  		alloc->start = 0;
> > >  		alloc->end = 0;
> > > +		*num_active = hweight32(dev_priv->active_crtcs);
> > >  		return;
> > >  	}
> > >  
> > > +	if (intel_state->active_pipe_changes)
> > > +		*num_active = hweight32(intel_state->active_crtcs);
> > > +	else
> > > +		*num_active = hweight32(dev_priv->active_crtcs);
> > > +
> > >  	if (IS_BROXTON(dev))
> > >  		ddb_size = BXT_DDB_SIZE;
> > >  	else
> > > @@ -2883,50 +2880,23 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device
> > > *dev,
> > >  	ddb_size -= 4; /* 4 blocks for bypass path allocation */
> > >  
> > >  	/*
> > > -	 * FIXME: At the moment we may be called on either in-flight or
> > > fully
> > > -	 * committed cstate's.  Once we fully move DDB allocation in the
> > > check
> > > -	 * phase, we'll only be called on in-flight states and the 'else'
> > > -	 * branch here will go away.
> > > -	 *
> > > -	 * The 'else' branch is slightly racy here, but it was racy to
> > > begin
> > > -	 * with; since it's going away soon, no effort is made to address
> > > that.
> > > +	 * If the state doesn't change the active CRTC's, then there's
> > > +	 * no need to recalculate; the existing pipe allocation limits
> > > +	 * should remain unchanged.  Note that we're safe from racing
> > > +	 * commits since any racing commit that changes the active CRTC
> > > +	 * list would need to grab _all_ crtc locks, including the one
> > > +	 * we currently hold.
> > >  	 */
> > > -	if (state) {
> > > -		/*
> > > -		 * If the state doesn't change the active CRTC's, then
> > > there's
> > > -		 * no need to recalculate; the existing pipe allocation
> > > limits
> > > -		 * should remain unchanged.  Note that we're safe from
> > > racing
> > > -		 * commits since any racing commit that changes the active
> > > CRTC
> > > -		 * list would need to grab _all_ crtc locks, including the
> > > one
> > > -		 * we currently hold.
> > > -		 */
> > > -		if (!intel_state->active_pipe_changes) {
> > > -			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> > > -			return;
> > > -		}
> > > -
> > > -		nth_active_pipe = hweight32(intel_state->active_crtcs &
> > > -					    (drm_crtc_mask(for_crtc) - 1));
> > > -		pipe_size = ddb_size / hweight32(intel_state-
> > > >active_crtcs);
> > > -		alloc->start = nth_active_pipe * ddb_size / *num_active;
> > > -		alloc->end = alloc->start + pipe_size;
> > > -	} else {
> > > -		nth_active_pipe = 0;
> > > -		for_each_crtc(dev, crtc) {
> > > -			if (!to_intel_crtc(crtc)->active)
> > > -				continue;
> > > -
> > > -			if (crtc == for_crtc)
> > > -				break;
> > > -
> > > -			nth_active_pipe++;
> > > -		}
> > > -
> > > -		pipe_size = ddb_size / config->num_pipes_active;
> > > -		alloc->start = nth_active_pipe * ddb_size /
> > > -			config->num_pipes_active;
> > > -		alloc->end = alloc->start + pipe_size;
> > > +	if (!intel_state->active_pipe_changes) {
> > > +		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> > So I finally figured out what's causing all of the valid wm calculations to
> > get
> > rejected, leading to blank screens etc. etc. The problem is this line right
> > here
> > where we assign *alloc, we never actually set dev_priv-
> > >wm.skl_hw.ddb.pipe[pipe] 
> > to a value anywhere so we end up having bogus ddb entry sizes. If you change
> > it
> > to something like:
> > 
> > 		*alloc = dev_priv->wm.skl_hw.ddb.plane[pipe][intel_crtc->plane];
> Whoops, I stand corrected on this, since this seems to cause a few issues with
> VT switching. We actually want something like this:
> 
> 		if (!intel_state->active_pipe_changes &&
> 		    dev_priv->wm.skl_hw.ddb.pipe[pipe].end != 0) {
> 			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> 			return;
> 		}
> 

dev_priv->wm.skl_hw.ddb.pipe[pipe] does get assigned in skl_update_wm()
via a full structure copy:

        /* store the new configuration */
        dev_priv->wm.skl_hw = *results;

So if you start with a good value in skl_hw and then you only recompute
when the value changes, skl_hw will always have good values when you get
to the point you noted above.  The problem is that if you start with a
bogus/unexpected value (based on the initial BIOS setting), then you've
got a garbage-in, garbage-out situation for every update that tries to
re-use the value.  Only something that triggers a full recompute (i.e.,
a pipe changing active status) will fix the problem at that point.

I've been out of office for the last two weeks, but I'm finally back
today, so I'm incorporating the fixes for this problem into a new
version of the series and rebasing to the latest -nightly.  I'll post my
updated patches shortly.  Sorry for the delay getting these final
details worked out!


Matt

> > 
> > We get the right values and everything starts working as expected. BTW: Things
> > seem to be pretty stable with this patchset after this fix, haven't tried it
> > on
> > any other platforms then skl yet though so I'll let you know if I run into any
> > more problems.
> > 
> > > 
> > > +		return;
> > >  	}
> > > +
> > > +	nth_active_pipe = hweight32(intel_state->active_crtcs &
> > > +				    (drm_crtc_mask(for_crtc) - 1));
> > > +	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> > > +	alloc->start = nth_active_pipe * ddb_size / *num_active;
> > > +	alloc->end = alloc->start + pipe_size;
> > >  }
> > >  
> > >  static unsigned int skl_cursor_allocation(int num_active)
> > > @@ -3025,62 +2995,33 @@ 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;
> > >  	const struct intel_plane *intel_plane;
> > > +	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 */
> > > -	/*
> > > -	 * FIXME: At the moment this function can be called on either an
> > > -	 * in-flight or a committed state object.  If it's in-flight then
> > > we
> > > -	 * only want to re-calculate the plane data rate for planes that
> > > are
> > > -	 * part of the transaction (i.e., we don't want to grab any
> > > additional
> > > -	 * plane states if we don't have to).  If we're operating on
> > > committed
> > > -	 * state, we'll just go ahead and recalculate the plane data rate
> > > for
> > > -	 * all planes.
> > > -	 *
> > > -	 * Once we finish moving our DDB allocation to the atomic check
> > > phase,
> > > -	 * we'll only be calling this function on in-flight state objects,
> > > so
> > > -	 * the 'else' branch here will go away.
> > > -	 */
> > > -	if (state) {
> > > -		struct drm_plane *plane;
> > > -		struct drm_plane_state *pstate;
> > > -		int i;
> > > -
> > > -		for_each_plane_in_state(state, plane, pstate, i) {
> > > -			intel_plane = to_intel_plane(plane);
> > > -			id = skl_wm_plane_id(intel_plane);
> > > -
> > > -			if (intel_plane->pipe != intel_crtc->pipe)
> > > -				continue;
> > > -
> > > -			/* packed/uv */
> > > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > > -							    pstate, 0);
> > > -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > -
> > > -			/* y-plane */
> > > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > > -							    pstate, 1);
> > > -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > > -		}
> > > -	} else {
> > > -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > > {
> > > -			const struct drm_plane_state *pstate =
> > > -				intel_plane->base.state;
> > > -			int id = skl_wm_plane_id(intel_plane);
> > > +	for_each_plane_in_state(state, plane, pstate, i) {
> > > +		id = skl_wm_plane_id(to_intel_plane(plane));
> > > +		intel_plane = to_intel_plane(plane);
> > >  
> > > -			/* packed/uv */
> > > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > > -							    pstate, 0);
> > > -			intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > +		if (intel_plane->pipe != intel_crtc->pipe)
> > > +			continue;
> > >  
> > > -			/* y-plane */
> > > -			rate = skl_plane_relative_data_rate(intel_cstate,
> > > -							    pstate, 1);
> > > -			intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > > -		}
> > > +		/* packed/uv */
> > > +		rate = skl_plane_relative_data_rate(intel_cstate,
> > > +						    pstate, 0);
> > > +		intel_cstate->wm.skl.plane_data_rate[id] = rate;
> > > +
> > > +		/* y-plane */
> > > +		rate = skl_plane_relative_data_rate(intel_cstate,
> > > +						    pstate, 1);
> > > +		intel_cstate->wm.skl.plane_y_data_rate[id] = rate;
> > >  	}
> > >  
> > >  	/* Calculate CRTC's total data rate from cached values */
> > > @@ -3104,8 +3045,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > >  	struct drm_atomic_state *state = cstate->base.state;
> > >  	struct drm_crtc *crtc = cstate->base.crtc;
> > >  	struct drm_device *dev = crtc->dev;
> > > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > > -	struct intel_wm_config *config = &dev_priv->wm.config;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	struct intel_plane *intel_plane;
> > >  	struct drm_plane *plane;
> > > @@ -3119,6 +3058,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > >  	int num_active;
> > >  	int id, i;
> > >  
> > > +	if (WARN_ON(!state))
> > > +		return 0;
> > > +
> > >  	if (!cstate->base.active) {
> > >  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> > >  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > > @@ -3126,8 +3068,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> > >  		return 0;
> > >  	}
> > >  
> > > -	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc,
> > > -					   &num_active);
> > > +	skl_ddb_get_pipe_allocation_limits(dev, cstate, alloc,
> > > &num_active);
> > >  	alloc_size = skl_ddb_entry_size(alloc);
> > >  	if (alloc_size == 0) {
> > >  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> > > @@ -3139,53 +3080,31 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> > > *cstate,
> > >  	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> > >  
> > >  	alloc_size -= cursor_blocks;
> > > -	alloc->end -= cursor_blocks;
> > >  
> > >  	/* 1. Allocate the mininum required blocks for each active plane */
> > > -	/*
> > > -	 * TODO: Remove support for already-committed state once we
> > > -	 * only allocate DDB on in-flight states.
> > > -	 */
> > > -	if (state) {
> > > -		for_each_plane_in_state(state, plane, pstate, i) {
> > > -			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)->visible) {
> > > -				minimum[id] = 0;
> > > -				y_minimum[id] = 0;
> > > -				continue;
> > > -			}
> > > -			if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > -				minimum[id] = 0;
> > > -				y_minimum[id] = 0;
> > > -				continue;
> > > -			}
> > > -
> > > -			minimum[id] = 8;
> > > -			if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > > -				y_minimum[id] = 8;
> > > -			else
> > > -				y_minimum[id] = 0;
> > > -		}
> > > -	} else {
> > > -		for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > > {
> > > -			struct drm_plane *plane = &intel_plane->base;
> > > -			struct drm_framebuffer *fb = plane->state->fb;
> > > -			int id = skl_wm_plane_id(intel_plane);
> > > -
> > > -			if (!to_intel_plane_state(plane->state)->visible)
> > > -				continue;
> > > +	for_each_plane_in_state(state, plane, pstate, i) {
> > > +		intel_plane = to_intel_plane(plane);
> > > +		id = skl_wm_plane_id(intel_plane);
> > >  
> > > -			if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > > -				continue;
> > > +		if (intel_plane->pipe != pipe)
> > > +			continue;
> > >  
> > > -			minimum[id] = 8;
> > > -			y_minimum[id] = (fb->pixel_format ==
> > > DRM_FORMAT_NV12)
> > > ? 8 : 0;
> > > +		if (!to_intel_plane_state(pstate)->visible) {
> > > +			minimum[id] = 0;
> > > +			y_minimum[id] = 0;
> > > +			continue;
> > > +		}
> > > +		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > > +			minimum[id] = 0;
> > > +			y_minimum[id] = 0;
> > > +			continue;
> > >  		}
> > > +
> > > +		minimum[id] = 8;
> > > +		if (pstate->fb->pixel_format == DRM_FORMAT_NV12)
> > > +			y_minimum[id] = 8;
> > > +		else
> > > +			y_minimum[id] = 0;
> > >  	}
> > >  
> > >  	for (i = 0; i < PLANE_CURSOR; i++) {
> > > @@ -3736,7 +3655,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> > >  
> > > -	WARN_ON(skl_allocate_pipe_ddb(cstate, ddb) != 0);
> > >  	skl_build_pipe_wm(cstate, ddb, pipe_wm);
> > >  
> > >  	if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> > > @@ -3800,16 +3718,6 @@ static void skl_clear_wm(struct skl_wm_values
> > > *watermarks, enum pipe pipe)
> > >  	memset(watermarks->plane_trans[pipe],
> > >  	       0, sizeof(uint32_t) * I915_MAX_PLANES);
> > >  	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
> > > -
> > > -	/* Clear ddb entries for pipe */
> > > -	memset(&watermarks->ddb.pipe[pipe], 0, sizeof(struct
> > > skl_ddb_entry));
> > > -	memset(&watermarks->ddb.plane[pipe], 0,
> > > -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > > -	memset(&watermarks->ddb.y_plane[pipe], 0,
> > > -	       sizeof(struct skl_ddb_entry) * I915_MAX_PLANES);
> > > -	memset(&watermarks->ddb.plane[pipe][PLANE_CURSOR], 0,
> > > -	       sizeof(struct skl_ddb_entry));
> > > -
> > >  }
> > >  
> > >  static int

-- 
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