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

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

[Matt Roper]

See previous cover letter for detailed explanation of series:
  https://lists.freedesktop.org/archives/intel-gfx/2016-April/091293.html

Key changes since the last spin (mostly based on Maarten's feedback):
 - During DDB/WM calculation, avoid grabbing extra plane_state's for the
   planes that aren't being modified by the transaction.  We do this by caching
   data rates and minimum block sizes in the crtc_state so that they can be
   re-used for subsequent transactions if the pipe's DDB size isn't changing.
 - Only use ->active_crtcs when we're sure we're not racing with
   anything that could update it.
 - Drop the pstate_for_cstate_plane() helper and describe more clearly in
   comments how/why the code may need to change in the future if we start
   supporting multiple pending commits.


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
  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 (v2)
  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
    (v2)
  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      | 573 ++++++++++++++++++++++-------------
 4 files changed, 446 insertions(+), 270 deletions(-)

-- 
2.1.4

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

[PATCH v2 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 v2 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 v2 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>
---
 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 v2 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>
---
 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 v2 05/16] drm/i915/gen9: Store plane minimum blocks in CRTC wm state

[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).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 4 ++++
 drivers/gpu/drm/i915/intel_pm.c  | 6 ++++--
 2 files changed, 8 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..479a890 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);
@@ -3088,6 +3088,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	alloc->end -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
+	memset(minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
+	memset(y_minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
 	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;
-- 
2.1.4

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

[PATCH v2 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>
---
 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 v2 07/16] drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state (v2)

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

Signed-off-by: Matt Roper <matthew.d.roper@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 85102ad..e91d39d 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 479a890..640305a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2849,13 +2849,18 @@ 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 (!cstate->base.active) {
 		alloc->start = 0;
@@ -2870,25 +2875,59 @@ 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];
+			*num_active = hweight32(dev_priv->active_crtcs);
+			return;
+		}
 
-		if (crtc == for_crtc)
-			break;
+		*num_active = hweight32(intel_state->active_crtcs);
+		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;
+		*num_active = config->num_pipes_active;
+	}
 }
 
-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 +3093,48 @@ 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;
+
+	if (!cstate->base.active) {
+		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
+		memset(ddb->plane[pipe], 0,
+		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
+		memset(ddb->y_plane[pipe], 0,
+		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
+		return 0;
+	}
 
-	skl_ddb_get_pipe_allocation_limits(dev, cstate, config, alloc);
+	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,23 +3142,57 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	alloc->end -= cursor_blocks;
 
 	/* 1. Allocate the mininum required blocks for each active plane */
-	memset(minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
-	memset(y_minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
-	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 {
+		memset(minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
+		memset(y_minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
+		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];
 	}
 
 	/*
@@ -3115,21 +3203,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];
 
 		/*
@@ -3141,8 +3222,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;
 
@@ -3155,12 +3239,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)
@@ -3651,7 +3738,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 v2 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>
---
 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 e91d39d..55522b9 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 640305a..0cc3443 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3814,6 +3814,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);
@@ -7351,6 +7420,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 v2 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>
---
 drivers/gpu/drm/i915/intel_display.c |  14 +--
 drivers/gpu/drm/i915/intel_pm.c      | 219 ++++++++++-------------------------
 2 files changed, 65 insertions(+), 168 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 0cc3443..a54d3c1 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,11 +2856,13 @@ 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 (WARN_ON(!state))
+		return;
+
 	if (!cstate->base.active) {
 		alloc->start = 0;
 		alloc->end = 0;
@@ -2876,53 +2877,25 @@ 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];
-			*num_active = hweight32(dev_priv->active_crtcs);
-			return;
-		}
-
-		*num_active = hweight32(intel_state->active_crtcs);
-		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;
-		*num_active = config->num_pipes_active;
+	if (!intel_state->active_pipe_changes) {
+		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
+		*num_active = hweight32(dev_priv->active_crtcs);
+		return;
 	}
+
+	*num_active = hweight32(intel_state->active_crtcs);
+	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)
@@ -3021,62 +2994,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 */
@@ -3100,8 +3044,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;
@@ -3115,6 +3057,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,
@@ -3124,8 +3069,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,55 +3083,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 {
-		memset(minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
-		memset(y_minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
-		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++) {
@@ -3738,7 +3658,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)))
@@ -3802,16 +3721,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 v2 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>
---
 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 a54d3c1..0c0a312 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3243,16 +3243,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;
@@ -3267,7 +3265,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);
@@ -3287,7 +3285,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);
@@ -3341,17 +3339,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 v2 11/16] drm/i915/gen9: Allow watermark calculation on in-flight atomic state (v2)

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

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0c0a312..d114375 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3330,14 +3330,17 @@ 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;
@@ -3346,7 +3349,29 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	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);
+		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);
+
 		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
 		result->plane_en[i] = skl_compute_plane_wm(dev_priv,
@@ -3357,6 +3382,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
@@ -3651,14 +3678,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;
@@ -3698,7 +3725,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);
 
 		/*
@@ -3807,7 +3834,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 v2 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 55522b9..6a2bbad 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 d114375..21fde05 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3511,7 +3511,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]);
@@ -3736,7 +3736,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);
 	}
 }
 
@@ -3830,7 +3830,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);
 
@@ -3838,7 +3838,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);
@@ -3997,7 +3997,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 v2 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 21fde05..f07054a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3241,13 +3241,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;
@@ -3259,8 +3260,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;
@@ -3321,13 +3324,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
@@ -3345,6 +3351,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;
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		int i = skl_wm_plane_id(intel_plane);
@@ -3374,13 +3381,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;
@@ -3417,21 +3427,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,
@@ -3678,21 +3693,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,
@@ -3725,8 +3746,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
@@ -3827,14 +3848,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 v2 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=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 f07054a..59d4574 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3224,23 +3224,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,
@@ -3711,66 +3694,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)
 {
@@ -3778,6 +3704,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;
 
@@ -3794,8 +3721,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;
@@ -3804,7 +3733,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;
 	}
@@ -3817,8 +3746,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
@@ -3833,10 +3765,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;
 }
 
@@ -3848,26 +3815,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 v2 15/16] drm/i915/gen9: Reject display updates that exceed wm limitations

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

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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 59d4574..96ffd54 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3309,7 +3309,17 @@ 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");
+			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 v2 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 6a2bbad..90a04ff 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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]

Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160420]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/Pre-calculate-SKL-style-atomic-watermarks/20160420-102913
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-n0-04210735 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_pm.c: In function 'skl_compute_wm':
>> drivers/gpu/drm/i915/intel_pm.c:2903:5: warning: 'num_active' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (num_active == 1)
        ^
   drivers/gpu/drm/i915/intel_pm.c:3057:6: note: 'num_active' was declared here
     int num_active;
         ^

vim +/num_active +2903 drivers/gpu/drm/i915/intel_pm.c

000bcc21 Matt Roper     2016-04-19  2887  	if (!intel_state->active_pipe_changes) {
000bcc21 Matt Roper     2016-04-19  2888  		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
000bcc21 Matt Roper     2016-04-19  2889  		*num_active = hweight32(dev_priv->active_crtcs);
000bcc21 Matt Roper     2016-04-19  2890  		return;
000bcc21 Matt Roper     2016-04-19  2891  	}
000bcc21 Matt Roper     2016-04-19  2892  
000bcc21 Matt Roper     2016-04-19  2893  	*num_active = hweight32(intel_state->active_crtcs);
000bcc21 Matt Roper     2016-04-19  2894  	nth_active_pipe = hweight32(intel_state->active_crtcs &
000bcc21 Matt Roper     2016-04-19  2895  				    (drm_crtc_mask(for_crtc) - 1));
000bcc21 Matt Roper     2016-04-19  2896  	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
000bcc21 Matt Roper     2016-04-19  2897  	alloc->start = nth_active_pipe * ddb_size / *num_active;
000bcc21 Matt Roper     2016-04-19  2898  	alloc->end = alloc->start + pipe_size;
b9cec075 Damien Lespiau 2014-11-04  2899  }
b9cec075 Damien Lespiau 2014-11-04  2900  
000bcc21 Matt Roper     2016-04-19  2901  static unsigned int skl_cursor_allocation(int num_active)
b9cec075 Damien Lespiau 2014-11-04  2902  {
000bcc21 Matt Roper     2016-04-19 @2903  	if (num_active == 1)
b9cec075 Damien Lespiau 2014-11-04  2904  		return 32;
b9cec075 Damien Lespiau 2014-11-04  2905  
b9cec075 Damien Lespiau 2014-11-04  2906  	return 8;
b9cec075 Damien Lespiau 2014-11-04  2907  }
b9cec075 Damien Lespiau 2014-11-04  2908  
a269c583 Damien Lespiau 2014-11-04  2909  static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
a269c583 Damien Lespiau 2014-11-04  2910  {
a269c583 Damien Lespiau 2014-11-04  2911  	entry->start = reg & 0x3ff;

:::::: The code at line 2903 was first introduced by commit
:::::: 000bcc2112fef02e609ad72f6ae43fd027ce8564 drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state (v2)

:::::: TO: Matt Roper <matthew.d.roper@intel.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23648 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

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

[Maarten Lankhorst]

Op 20-04-16 om 04:26 schreef 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).
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 4 ++++
>  drivers/gpu/drm/i915/intel_pm.c  | 6 ++++--
>  2 files changed, 8 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..479a890 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);
> @@ -3088,6 +3088,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	alloc->end -= cursor_blocks;
>  
>  	/* 1. Allocate the mininum required blocks for each active plane */
> +	memset(minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
> +	memset(y_minimum, 0, sizeof(uint16_t) * I915_MAX_PLANES);
The commit doesn't explain why this memset is suddenly needed, especially since this was left uninitialized before.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Hey,

Op 20-04-16 om 04:26 schreef 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.
>
> Signed-off-by: Matt Roper <matthew.d.roper@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 85102ad..e91d39d 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 479a890..640305a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2849,13 +2849,18 @@ 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 (!cstate->base.active) {
>  		alloc->start = 0;
> @@ -2870,25 +2875,59 @@ 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];
> +			*num_active = hweight32(dev_priv->active_crtcs);
> +			return;
> +		}
>  
> -		if (crtc == for_crtc)
> -			break;
> +		*num_active = hweight32(intel_state->active_crtcs);
> +		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;
> +		*num_active = config->num_pipes_active;
> +	}
>  }
>  
> -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 +3093,48 @@ 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;
> +
> +	if (!cstate->base.active) {
> +		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> +		memset(ddb->plane[pipe], 0,
> +		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
> +		memset(ddb->y_plane[pipe], 0,
> +		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
> +		return 0;
>
^Since these are pointers to arrays, memset(x, 0, sizeof(x)); ?

Also there still seems to be a bogus memset for plane[pipe][PLANE_CURSOR], after plane[pipe] was just zero'd. Should probably be fixed while you're touching this function. :-)

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

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

[Maarten Lankhorst]

Op 20-04-16 om 04:26 schreef 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)
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0c0a312..d114375 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3330,14 +3330,17 @@ 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;
> @@ -3346,7 +3349,29 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  	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);
> +		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);
>
You can make this race-free by using using for_each_intel_plane_mask
with crtc_state->plane_mask ¦ old_crtc_state->plane_mask.

Other planes can be assumed to have a data rate of 0, and can unfortunately be updated
in parallel. Since you can only update atomic properties that are not ->crtc or ->fb it wouldn't
change anything visibly on the screen, but it would cause your plane->state to be freed behind
your back if you're not careful.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 20-04-16 om 04:26 schreef 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 21fde05..f07054a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3241,13 +3241,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;
> @@ -3259,8 +3260,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;
> @@ -3321,13 +3324,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
> @@ -3345,6 +3351,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;
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		int i = skl_wm_plane_id(intel_plane);
> @@ -3374,13 +3381,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;
> @@ -3417,21 +3427,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,
> @@ -3678,21 +3693,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,
> @@ -3725,8 +3746,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
> @@ -3827,14 +3848,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);
With the nitpicks fixed, and with CI happy.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lyude Paul]

On a T560, this ends up rejecting valid watermark configurations so the internal
display doesn't switch from fbcon to X properly:

[    5.767383] [drm:intelfb_create] re-using BIOS fb
[    5.767444] [drm] Initialized i915 1.6.0 20160411 for 0000:00:02.0 on minor 0
[    5.767449] [drm:intelfb_create] allocated 1920x1080 fb: 0x00000000, bo ffff88021374c000
[    5.767760] fbcon: inteldrmfb (fb0) is primary device
[    5.768147] [drm:connected_sink_compute_bpp] [CONNECTOR:37:eDP-1] checking for sink bpp constrains
[    5.768150] [drm:connected_sink_compute_bpp] clamping display bpp (was 36) to EDID reported max of 18
[    5.768159] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26] scaler_user index 0.31
[    5.768162] [drm:skl_update_scaler] scaler_user index 0.31: Staged freeing scaler id 0 scaler_users = 0x0
[    5.768166] [drm:intel_dp_compute_config] DP link computation with max lane count 2 max bw 270000 pixel clock 142900KHz
[    5.768172] [drm:intel_dp_compute_config] DP link bw 0a rate select 00 lane count 2 clock 270000 bpp 18
[    5.768174] [drm:intel_dp_compute_config] DP link bw required 257220 available 432000
[    5.768180] [drm:intel_modeset_pipe_config] hw max bpp: 36, pipe bpp: 18, dithering: 1
[    5.768193] [drm:intel_dump_pipe_config] [CRTC:26][modeset] config ffff8800d5c11000 for pipe A
[    5.768200] [drm:intel_dump_pipe_config] cpu_transcoder: EDP
[    5.768202] [drm:intel_dump_pipe_config] pipe bpp: 18, dithering: 1
[    5.768206] [drm:intel_dump_pipe_config] fdi/pch: 0, lanes: 0, gmch_m: 0, gmch_n: 0, link_m: 0, link_n: 0, tu: 0
[    5.768211] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m: 4994717, gmch_n: 8388608, link_m: 277484, link_n: 524288, tu: 64
[    5.768215] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m2: 0, gmch_n2: 0, link_m2: 0, link_n2: 0, tu2: 0
[    5.768217] [drm:intel_dump_pipe_config] audio: 0, infoframes: 0
[    5.768219] [drm:intel_dump_pipe_config] requested mode:
[    5.768226] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
[    5.768228] [drm:intel_dump_pipe_config] adjusted mode:
[    5.768234] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
[    5.768239] [drm:intel_dump_crtc_timings] crtc timings: 142900 1920 1968 2000 2082 1080 1082 1087 1144, type: 0x48 flags: 0xa
[    5.768241] [drm:intel_dump_pipe_config] port clock: 270000
[    5.768244] [drm:intel_dump_pipe_config] pipe src size: 1920x1080
[    5.768247] [drm:intel_dump_pipe_config] num_scalers: 2, scaler_users: 0x0, scaler_id: -1
[    5.768251] [drm:intel_dump_pipe_config] gmch pfit: control: 0x00000000, ratios: 0x00000000, lvds border: 0x00000000
[    5.768254] [drm:intel_dump_pipe_config] pch pfit: pos: 0x00000000, size: 0x00000000, disabled
[    5.768256] [drm:intel_dump_pipe_config] ips: 0
[    5.768258] [drm:intel_dump_pipe_config] double wide: 0
[    5.768263] [drm:intel_dump_pipe_config] ddi_pll_sel: 1; dpll_hw_state: ctrl1: 0x13, cfgcr1: 0x0, cfgcr2: 0x0
[    5.768266] [drm:intel_dump_pipe_config] planes on this crtc
[    5.768277] [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0 idx: 0 enabled
[    5.768284] [drm:intel_dump_pipe_config] 	FB:58, fb = 1920x1080 format = 0x34325258
[    5.768285] [drm:intel_dump_pipe_config] 	scaler:-1 src (0, 0) 1920x1080 dst (0, 0) 1920x1080
[    5.768290] [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disabled, scaler_id = -1
[    5.768295] [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 disabled, scaler_id = -1
[    5.768309] [drm:skl_update_scaler_plane] Updating scaler for [PLANE:23] scaler_user index 0.0
[    5.768319] [drm:intel_find_shared_dpll] CRTC:26 allocated DPLL 0
[    5.768323] [drm:intel_reference_shared_dpll] using DPLL 0 for pipe A
[    5.768327] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26] scaler_user index 0.31
[    5.768339] [drm:skl_compute_plane_wm] Requested display configuration exceeds system watermark limitations
[    5.768359] [drm:drm_fb_helper_hotplug_event] 
[    5.768363] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:37:eDP-1]
[    5.768367] [drm:intel_dp_detect] [CONNECTOR:37:eDP-1]
[    5.768786] [drm:intel_dp_probe_oui] Sink OUI: 0022b9
[    5.769177] [drm:intel_dp_probe_oui] Branch OUI: 444615
[    5.769604] [drm:drm_edid_to_eld] ELD: no CEA Extension found
[    5.769611] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:37:eDP-1] probed modes :
[    5.769618] [drm:drm_mode_debug_printmodeline] Modeline 38:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
[    5.769621] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:45:DP-1]
[    5.769624] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
[    5.769628] [drm:intel_power_well_enable] enabling power well 2
[    5.769633] [drm:skl_set_power_well] Enabling power well 2
[    5.769659] [drm:intel_power_well_disable] disabling power well 2
[    5.769672] [drm:skl_set_power_well] Disabling power well 2

On Tue, 2016-04-19 at 19:26 -0700, Matt Roper wrote:
> 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.
> 
> 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 | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 59d4574..96ffd54 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3309,7 +3309,17 @@ 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");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	*out_blocks = res_blocks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Thu, Apr 21, 2016 at 05:49:52PM -0400, Lyude Paul wrote:
> On a T560, this ends up rejecting valid watermark configurations so the internal
> display doesn't switch from fbcon to X properly:

Hmm.  We can leave this patch out of the series and still have a fix for
the WARN_ON(!wm_changed), but one of the main goals was to identify and
reject configurations that the platform is truly incapable of handling.
I'm still going to include this patch in my next spin of the series on
the mailing list, but we can just omit it during merge if we need more
time to figure out why your configuration is being rejected.  I'll also
add an extra DRM_DEBUG_KMS line to this patch to give us more insight
into what the final calculated values were and how far out of bounds
they were.

If you drop this patch but keep the rest of the series, does the system
still operate properly when it goes into X?  I.e., no underruns reported
and no flickering/corruption on screen?

Could you provide the contents of
/sys/kernel/debug/dri/0/i915_pri_wm_latency ?  I've heard that there are
some SKL pcode (BIOS) versions floating around that give bad memory
latency values which result in us calculating the wrong watermark values
in the end; it would be good to see if that's a contributing factor
here.

Also, I can see your eDP panel in the log below; is that your only
display or do you also have an external display attached?



Matt

> 
> [    5.767383] [drm:intelfb_create] re-using BIOS fb
> [    5.767444] [drm] Initialized i915 1.6.0 20160411 for 0000:00:02.0 on minor 0
> [    5.767449] [drm:intelfb_create] allocated 1920x1080 fb: 0x00000000, bo ffff88021374c000
> [    5.767760] fbcon: inteldrmfb (fb0) is primary device
> [    5.768147] [drm:connected_sink_compute_bpp] [CONNECTOR:37:eDP-1] checking for sink bpp constrains
> [    5.768150] [drm:connected_sink_compute_bpp] clamping display bpp (was 36) to EDID reported max of 18
> [    5.768159] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26] scaler_user index 0.31
> [    5.768162] [drm:skl_update_scaler] scaler_user index 0.31: Staged freeing scaler id 0 scaler_users = 0x0
> [    5.768166] [drm:intel_dp_compute_config] DP link computation with max lane count 2 max bw 270000 pixel clock 142900KHz
> [    5.768172] [drm:intel_dp_compute_config] DP link bw 0a rate select 00 lane count 2 clock 270000 bpp 18
> [    5.768174] [drm:intel_dp_compute_config] DP link bw required 257220 available 432000
> [    5.768180] [drm:intel_modeset_pipe_config] hw max bpp: 36, pipe bpp: 18, dithering: 1
> [    5.768193] [drm:intel_dump_pipe_config] [CRTC:26][modeset] config ffff8800d5c11000 for pipe A
> [    5.768200] [drm:intel_dump_pipe_config] cpu_transcoder: EDP
> [    5.768202] [drm:intel_dump_pipe_config] pipe bpp: 18, dithering: 1
> [    5.768206] [drm:intel_dump_pipe_config] fdi/pch: 0, lanes: 0, gmch_m: 0, gmch_n: 0, link_m: 0, link_n: 0, tu: 0
> [    5.768211] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m: 4994717, gmch_n: 8388608, link_m: 277484, link_n: 524288, tu: 64
> [    5.768215] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m2: 0, gmch_n2: 0, link_m2: 0, link_n2: 0, tu2: 0
> [    5.768217] [drm:intel_dump_pipe_config] audio: 0, infoframes: 0
> [    5.768219] [drm:intel_dump_pipe_config] requested mode:
> [    5.768226] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> [    5.768228] [drm:intel_dump_pipe_config] adjusted mode:
> [    5.768234] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> [    5.768239] [drm:intel_dump_crtc_timings] crtc timings: 142900 1920 1968 2000 2082 1080 1082 1087 1144, type: 0x48 flags: 0xa
> [    5.768241] [drm:intel_dump_pipe_config] port clock: 270000
> [    5.768244] [drm:intel_dump_pipe_config] pipe src size: 1920x1080
> [    5.768247] [drm:intel_dump_pipe_config] num_scalers: 2, scaler_users: 0x0, scaler_id: -1
> [    5.768251] [drm:intel_dump_pipe_config] gmch pfit: control: 0x00000000, ratios: 0x00000000, lvds border: 0x00000000
> [    5.768254] [drm:intel_dump_pipe_config] pch pfit: pos: 0x00000000, size: 0x00000000, disabled
> [    5.768256] [drm:intel_dump_pipe_config] ips: 0
> [    5.768258] [drm:intel_dump_pipe_config] double wide: 0
> [    5.768263] [drm:intel_dump_pipe_config] ddi_pll_sel: 1; dpll_hw_state: ctrl1: 0x13, cfgcr1: 0x0, cfgcr2: 0x0
> [    5.768266] [drm:intel_dump_pipe_config] planes on this crtc
> [    5.768277] [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0 idx: 0 enabled
> [    5.768284] [drm:intel_dump_pipe_config] 	FB:58, fb = 1920x1080 format = 0x34325258
> [    5.768285] [drm:intel_dump_pipe_config] 	scaler:-1 src (0, 0) 1920x1080 dst (0, 0) 1920x1080
> [    5.768290] [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disabled, scaler_id = -1
> [    5.768295] [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 disabled, scaler_id = -1
> [    5.768309] [drm:skl_update_scaler_plane] Updating scaler for [PLANE:23] scaler_user index 0.0
> [    5.768319] [drm:intel_find_shared_dpll] CRTC:26 allocated DPLL 0
> [    5.768323] [drm:intel_reference_shared_dpll] using DPLL 0 for pipe A
> [    5.768327] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26] scaler_user index 0.31
> [    5.768339] [drm:skl_compute_plane_wm] Requested display configuration exceeds system watermark limitations
> [    5.768359] [drm:drm_fb_helper_hotplug_event] 
> [    5.768363] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:37:eDP-1]
> [    5.768367] [drm:intel_dp_detect] [CONNECTOR:37:eDP-1]
> [    5.768786] [drm:intel_dp_probe_oui] Sink OUI: 0022b9
> [    5.769177] [drm:intel_dp_probe_oui] Branch OUI: 444615
> [    5.769604] [drm:drm_edid_to_eld] ELD: no CEA Extension found
> [    5.769611] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:37:eDP-1] probed modes :
> [    5.769618] [drm:drm_mode_debug_printmodeline] Modeline 38:"1920x1080" 60 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> [    5.769621] [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:45:DP-1]
> [    5.769624] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
> [    5.769628] [drm:intel_power_well_enable] enabling power well 2
> [    5.769633] [drm:skl_set_power_well] Enabling power well 2
> [    5.769659] [drm:intel_power_well_disable] disabling power well 2
> [    5.769672] [drm:skl_set_power_well] Disabling power well 2
> 
> On Tue, 2016-04-19 at 19:26 -0700, Matt Roper wrote:
> > 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.
> > 
> > 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 | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 59d4574..96ffd54 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3309,7 +3309,17 @@ 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");
> > +			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	*out_blocks = res_blocks;

-- 
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 v2 13/16] drm/i915/gen9: Propagate watermark calculation failures up the call chain

[Matt Roper]

On Thu, Apr 21, 2016 at 02:19:33PM +0200, Maarten Lankhorst wrote:
> Op 20-04-16 om 04:26 schreef 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 21fde05..f07054a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3241,13 +3241,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;
> > @@ -3259,8 +3260,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;
> > @@ -3321,13 +3324,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
> > @@ -3345,6 +3351,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;
> >  
> >  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> >  		int i = skl_wm_plane_id(intel_plane);
> > @@ -3374,13 +3381,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;
> > @@ -3417,21 +3427,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,
> > @@ -3678,21 +3693,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,
> > @@ -3725,8 +3746,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
> > @@ -3827,14 +3848,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);
> With the nitpicks fixed, and with CI happy.
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I'm interpreting this to refer to the entire series (and not just this
one patch that already had your r-b and no nitpicks); if that's not what
you meant, please let me know.  :-)


Matt

-- 
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 v2 15/16] drm/i915/gen9: Reject display updates that exceed wm limitations

[Lyude Paul]

Unfortunately dropping that patch just results with the screen staying blank
starting from the moment the driver loads. 

As for the external monitors question: the only time this issue seems to happen
with this patchset is when there's no external monitors connected. If I boot
with just the laptop and nothing connected, it gets as far as showing systemd
starting all the services in fbcon up to where it starts GDM, and then the
screen just gets stuck on that framebuffer. Switching to a VT at that point
doesn't fix it.

If I plug in another monitor to the system everything starts working fine again,
even if I unplug the display again afterwards or switch VTs. I've recorded a
sample of dmesg where I boot up the laptop with no displays connected, connect
an external MST display, and then disconnect it (just grep for "rhtest unknown:"
to find where I marked the spot before I connected the monitors, etc. ):

https://paste.fedoraproject.org/358607/61334721/

Also the contents of /sys/kernel/debug/dri/0/i915_pri_wm_latency:
WM0 2 (2.0 usec)
WM1 19 (19.0 usec)
WM2 28 (28.0 usec)
WM3 32 (32.0 usec)
WM4 63 (63.0 usec)
WM5 77 (77.0 usec)
WM6 83 (83.0 usec)
WM7 99 (99.0 usec)

On Thu, 2016-04-21 at 16:14 -0700, Matt Roper wrote:
> On Thu, Apr 21, 2016 at 05:49:52PM -0400, Lyude Paul wrote:
> > 
> > On a T560, this ends up rejecting valid watermark configurations so the
> > internal
> > display doesn't switch from fbcon to X properly:
> Hmm.  We can leave this patch out of the series and still have a fix for
> the WARN_ON(!wm_changed), but one of the main goals was to identify and
> reject configurations that the platform is truly incapable of handling.
> I'm still going to include this patch in my next spin of the series on
> the mailing list, but we can just omit it during merge if we need more
> time to figure out why your configuration is being rejected.  I'll also
> add an extra DRM_DEBUG_KMS line to this patch to give us more insight
> into what the final calculated values were and how far out of bounds
> they were.
> 
> If you drop this patch but keep the rest of the series, does the system
> still operate properly when it goes into X?  I.e., no underruns reported
> and no flickering/corruption on screen?
> 
> Could you provide the contents of
> /sys/kernel/debug/dri/0/i915_pri_wm_latency ?  I've heard that there are
> some SKL pcode (BIOS) versions floating around that give bad memory
> latency values which result in us calculating the wrong watermark values
> in the end; it would be good to see if that's a contributing factor
> here.
> 
> Also, I can see your eDP panel in the log below; is that your only
> display or do you also have an external display attached?
> 
> 
> 
> Matt
> 
> > 
> > 
> > [    5.767383] [drm:intelfb_create] re-using BIOS fb
> > [    5.767444] [drm] Initialized i915 1.6.0 20160411 for 0000:00:02.0 on
> > minor 0
> > [    5.767449] [drm:intelfb_create] allocated 1920x1080 fb: 0x00000000, bo
> > ffff88021374c000
> > [    5.767760] fbcon: inteldrmfb (fb0) is primary device
> > [    5.768147] [drm:connected_sink_compute_bpp] [CONNECTOR:37:eDP-1]
> > checking for sink bpp constrains
> > [    5.768150] [drm:connected_sink_compute_bpp] clamping display bpp (was
> > 36) to EDID reported max of 18
> > [    5.768159] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26]
> > scaler_user index 0.31
> > [    5.768162] [drm:skl_update_scaler] scaler_user index 0.31: Staged
> > freeing scaler id 0 scaler_users = 0x0
> > [    5.768166] [drm:intel_dp_compute_config] DP link computation with max
> > lane count 2 max bw 270000 pixel clock 142900KHz
> > [    5.768172] [drm:intel_dp_compute_config] DP link bw 0a rate select 00
> > lane count 2 clock 270000 bpp 18
> > [    5.768174] [drm:intel_dp_compute_config] DP link bw required 257220
> > available 432000
> > [    5.768180] [drm:intel_modeset_pipe_config] hw max bpp: 36, pipe bpp: 18,
> > dithering: 1
> > [    5.768193] [drm:intel_dump_pipe_config] [CRTC:26][modeset] config
> > ffff8800d5c11000 for pipe A
> > [    5.768200] [drm:intel_dump_pipe_config] cpu_transcoder: EDP
> > [    5.768202] [drm:intel_dump_pipe_config] pipe bpp: 18, dithering: 1
> > [    5.768206] [drm:intel_dump_pipe_config] fdi/pch: 0, lanes: 0, gmch_m: 0,
> > gmch_n: 0, link_m: 0, link_n: 0, tu: 0
> > [    5.768211] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m:
> > 4994717, gmch_n: 8388608, link_m: 277484, link_n: 524288, tu: 64
> > [    5.768215] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m2: 0,
> > gmch_n2: 0, link_m2: 0, link_n2: 0, tu2: 0
> > [    5.768217] [drm:intel_dump_pipe_config] audio: 0, infoframes: 0
> > [    5.768219] [drm:intel_dump_pipe_config] requested mode:
> > [    5.768226] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60
> > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > [    5.768228] [drm:intel_dump_pipe_config] adjusted mode:
> > [    5.768234] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60
> > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > [    5.768239] [drm:intel_dump_crtc_timings] crtc timings: 142900 1920 1968
> > 2000 2082 1080 1082 1087 1144, type: 0x48 flags: 0xa
> > [    5.768241] [drm:intel_dump_pipe_config] port clock: 270000
> > [    5.768244] [drm:intel_dump_pipe_config] pipe src size: 1920x1080
> > [    5.768247] [drm:intel_dump_pipe_config] num_scalers: 2, scaler_users:
> > 0x0, scaler_id: -1
> > [    5.768251] [drm:intel_dump_pipe_config] gmch pfit: control: 0x00000000,
> > ratios: 0x00000000, lvds border: 0x00000000
> > [    5.768254] [drm:intel_dump_pipe_config] pch pfit: pos: 0x00000000, size:
> > 0x00000000, disabled
> > [    5.768256] [drm:intel_dump_pipe_config] ips: 0
> > [    5.768258] [drm:intel_dump_pipe_config] double wide: 0
> > [    5.768263] [drm:intel_dump_pipe_config] ddi_pll_sel: 1; dpll_hw_state:
> > ctrl1: 0x13, cfgcr1: 0x0, cfgcr2: 0x0
> > [    5.768266] [drm:intel_dump_pipe_config] planes on this crtc
> > [    5.768277] [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0
> > idx: 0 enabled
> > [    5.768284] [drm:intel_dump_pipe_config] 	FB:58, fb = 1920x1080
> > format = 0x34325258
> > [    5.768285] [drm:intel_dump_pipe_config] 	scaler:-1 src (0, 0)
> > 1920x1080 dst (0, 0) 1920x1080
> > [    5.768290] [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx:
> > 1 disabled, scaler_id = -1
> > [    5.768295] [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1
> > idx: 2 disabled, scaler_id = -1
> > [    5.768309] [drm:skl_update_scaler_plane] Updating scaler for [PLANE:23]
> > scaler_user index 0.0
> > [    5.768319] [drm:intel_find_shared_dpll] CRTC:26 allocated DPLL 0
> > [    5.768323] [drm:intel_reference_shared_dpll] using DPLL 0 for pipe A
> > [    5.768327] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26]
> > scaler_user index 0.31
> > [    5.768339] [drm:skl_compute_plane_wm] Requested display configuration
> > exceeds system watermark limitations
> > [    5.768359] [drm:drm_fb_helper_hotplug_event] 
> > [    5.768363] [drm:drm_helper_probe_single_connector_modes]
> > [CONNECTOR:37:eDP-1]
> > [    5.768367] [drm:intel_dp_detect] [CONNECTOR:37:eDP-1]
> > [    5.768786] [drm:intel_dp_probe_oui] Sink OUI: 0022b9
> > [    5.769177] [drm:intel_dp_probe_oui] Branch OUI: 444615
> > [    5.769604] [drm:drm_edid_to_eld] ELD: no CEA Extension found
> > [    5.769611] [drm:drm_helper_probe_single_connector_modes]
> > [CONNECTOR:37:eDP-1] probed modes :
> > [    5.769618] [drm:drm_mode_debug_printmodeline] Modeline 38:"1920x1080" 60
> > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > [    5.769621] [drm:drm_helper_probe_single_connector_modes]
> > [CONNECTOR:45:DP-1]
> > [    5.769624] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
> > [    5.769628] [drm:intel_power_well_enable] enabling power well 2
> > [    5.769633] [drm:skl_set_power_well] Enabling power well 2
> > [    5.769659] [drm:intel_power_well_disable] disabling power well 2
> > [    5.769672] [drm:skl_set_power_well] Disabling power well 2
> > 
> > On Tue, 2016-04-19 at 19:26 -0700, Matt Roper wrote:
> > > 
> > > 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.
> > > 
> > > 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 | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 59d4574..96ffd54 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3309,7 +3309,17 @@ 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");
> > > +			return -EINVAL;
> > > +		}
> > >  	}
> > >  
> > >  	*out_blocks = res_blocks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Fri, Apr 22, 2016 at 10:21:20AM -0400, Lyude Paul wrote:
> Unfortunately dropping that patch just results with the screen staying blank
> starting from the moment the driver loads. 
> 
> As for the external monitors question: the only time this issue seems to happen
> with this patchset is when there's no external monitors connected. If I boot
> with just the laptop and nothing connected, it gets as far as showing systemd
> starting all the services in fbcon up to where it starts GDM, and then the
> screen just gets stuck on that framebuffer. Switching to a VT at that point
> doesn't fix it.
> 
> If I plug in another monitor to the system everything starts working fine again,
> even if I unplug the display again afterwards or switch VTs. I've recorded a
> sample of dmesg where I boot up the laptop with no displays connected, connect
> an external MST display, and then disconnect it (just grep for "rhtest unknown:"
> to find where I marked the spot before I connected the monitors, etc. ):
> 
> https://paste.fedoraproject.org/358607/61334721/
> 
> Also the contents of /sys/kernel/debug/dri/0/i915_pri_wm_latency:
> WM0 2 (2.0 usec)
> WM1 19 (19.0 usec)
> WM2 28 (28.0 usec)
> WM3 32 (32.0 usec)
> WM4 63 (63.0 usec)
> WM5 77 (77.0 usec)
> WM6 83 (83.0 usec)
> WM7 99 (99.0 usec)


Okay, I think I've figured this out.  The problem arises because with
this patch set we only re-calculate the DDB (the data buffer that
watermarks refer to) when we need to, otherwise we use the allocation
that was computed on previous modesets.  That pretty much always works
fine, except when we've just booted and we're using the DDB allocations
inherited from the BIOS; those BIOS allocations may fail to meet our
needs/expectations and mislead us into thinking there's no way we can
program watermarks safely.

I think the solution is probably to just add a flag to the driver that
forces our very first real modeset to completely recompute proper DDB
allocations, even if it otherwise wouldn't need to.  I'll put together a
patch for that shortly and see if it helps.


Matt

> 
> On Thu, 2016-04-21 at 16:14 -0700, Matt Roper wrote:
> > On Thu, Apr 21, 2016 at 05:49:52PM -0400, Lyude Paul wrote:
> > > 
> > > On a T560, this ends up rejecting valid watermark configurations so the
> > > internal
> > > display doesn't switch from fbcon to X properly:
> > Hmm.  We can leave this patch out of the series and still have a fix for
> > the WARN_ON(!wm_changed), but one of the main goals was to identify and
> > reject configurations that the platform is truly incapable of handling.
> > I'm still going to include this patch in my next spin of the series on
> > the mailing list, but we can just omit it during merge if we need more
> > time to figure out why your configuration is being rejected.  I'll also
> > add an extra DRM_DEBUG_KMS line to this patch to give us more insight
> > into what the final calculated values were and how far out of bounds
> > they were.
> > 
> > If you drop this patch but keep the rest of the series, does the system
> > still operate properly when it goes into X?  I.e., no underruns reported
> > and no flickering/corruption on screen?
> > 
> > Could you provide the contents of
> > /sys/kernel/debug/dri/0/i915_pri_wm_latency ?  I've heard that there are
> > some SKL pcode (BIOS) versions floating around that give bad memory
> > latency values which result in us calculating the wrong watermark values
> > in the end; it would be good to see if that's a contributing factor
> > here.
> > 
> > Also, I can see your eDP panel in the log below; is that your only
> > display or do you also have an external display attached?
> > 
> > 
> > 
> > Matt
> > 
> > > 
> > > 
> > > [    5.767383] [drm:intelfb_create] re-using BIOS fb
> > > [    5.767444] [drm] Initialized i915 1.6.0 20160411 for 0000:00:02.0 on
> > > minor 0
> > > [    5.767449] [drm:intelfb_create] allocated 1920x1080 fb: 0x00000000, bo
> > > ffff88021374c000
> > > [    5.767760] fbcon: inteldrmfb (fb0) is primary device
> > > [    5.768147] [drm:connected_sink_compute_bpp] [CONNECTOR:37:eDP-1]
> > > checking for sink bpp constrains
> > > [    5.768150] [drm:connected_sink_compute_bpp] clamping display bpp (was
> > > 36) to EDID reported max of 18
> > > [    5.768159] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26]
> > > scaler_user index 0.31
> > > [    5.768162] [drm:skl_update_scaler] scaler_user index 0.31: Staged
> > > freeing scaler id 0 scaler_users = 0x0
> > > [    5.768166] [drm:intel_dp_compute_config] DP link computation with max
> > > lane count 2 max bw 270000 pixel clock 142900KHz
> > > [    5.768172] [drm:intel_dp_compute_config] DP link bw 0a rate select 00
> > > lane count 2 clock 270000 bpp 18
> > > [    5.768174] [drm:intel_dp_compute_config] DP link bw required 257220
> > > available 432000
> > > [    5.768180] [drm:intel_modeset_pipe_config] hw max bpp: 36, pipe bpp: 18,
> > > dithering: 1
> > > [    5.768193] [drm:intel_dump_pipe_config] [CRTC:26][modeset] config
> > > ffff8800d5c11000 for pipe A
> > > [    5.768200] [drm:intel_dump_pipe_config] cpu_transcoder: EDP
> > > [    5.768202] [drm:intel_dump_pipe_config] pipe bpp: 18, dithering: 1
> > > [    5.768206] [drm:intel_dump_pipe_config] fdi/pch: 0, lanes: 0, gmch_m: 0,
> > > gmch_n: 0, link_m: 0, link_n: 0, tu: 0
> > > [    5.768211] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m:
> > > 4994717, gmch_n: 8388608, link_m: 277484, link_n: 524288, tu: 64
> > > [    5.768215] [drm:intel_dump_pipe_config] dp: 1, lanes: 2, gmch_m2: 0,
> > > gmch_n2: 0, link_m2: 0, link_n2: 0, tu2: 0
> > > [    5.768217] [drm:intel_dump_pipe_config] audio: 0, infoframes: 0
> > > [    5.768219] [drm:intel_dump_pipe_config] requested mode:
> > > [    5.768226] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60
> > > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > > [    5.768228] [drm:intel_dump_pipe_config] adjusted mode:
> > > [    5.768234] [drm:drm_mode_debug_printmodeline] Modeline 0:"1920x1080" 60
> > > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > > [    5.768239] [drm:intel_dump_crtc_timings] crtc timings: 142900 1920 1968
> > > 2000 2082 1080 1082 1087 1144, type: 0x48 flags: 0xa
> > > [    5.768241] [drm:intel_dump_pipe_config] port clock: 270000
> > > [    5.768244] [drm:intel_dump_pipe_config] pipe src size: 1920x1080
> > > [    5.768247] [drm:intel_dump_pipe_config] num_scalers: 2, scaler_users:
> > > 0x0, scaler_id: -1
> > > [    5.768251] [drm:intel_dump_pipe_config] gmch pfit: control: 0x00000000,
> > > ratios: 0x00000000, lvds border: 0x00000000
> > > [    5.768254] [drm:intel_dump_pipe_config] pch pfit: pos: 0x00000000, size:
> > > 0x00000000, disabled
> > > [    5.768256] [drm:intel_dump_pipe_config] ips: 0
> > > [    5.768258] [drm:intel_dump_pipe_config] double wide: 0
> > > [    5.768263] [drm:intel_dump_pipe_config] ddi_pll_sel: 1; dpll_hw_state:
> > > ctrl1: 0x13, cfgcr1: 0x0, cfgcr2: 0x0
> > > [    5.768266] [drm:intel_dump_pipe_config] planes on this crtc
> > > [    5.768277] [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0
> > > idx: 0 enabled
> > > [    5.768284] [drm:intel_dump_pipe_config] 	FB:58, fb = 1920x1080
> > > format = 0x34325258
> > > [    5.768285] [drm:intel_dump_pipe_config] 	scaler:-1 src (0, 0)
> > > 1920x1080 dst (0, 0) 1920x1080
> > > [    5.768290] [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx:
> > > 1 disabled, scaler_id = -1
> > > [    5.768295] [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1
> > > idx: 2 disabled, scaler_id = -1
> > > [    5.768309] [drm:skl_update_scaler_plane] Updating scaler for [PLANE:23]
> > > scaler_user index 0.0
> > > [    5.768319] [drm:intel_find_shared_dpll] CRTC:26 allocated DPLL 0
> > > [    5.768323] [drm:intel_reference_shared_dpll] using DPLL 0 for pipe A
> > > [    5.768327] [drm:skl_update_scaler_crtc] Updating scaler for [CRTC:26]
> > > scaler_user index 0.31
> > > [    5.768339] [drm:skl_compute_plane_wm] Requested display configuration
> > > exceeds system watermark limitations
> > > [    5.768359] [drm:drm_fb_helper_hotplug_event] 
> > > [    5.768363] [drm:drm_helper_probe_single_connector_modes]
> > > [CONNECTOR:37:eDP-1]
> > > [    5.768367] [drm:intel_dp_detect] [CONNECTOR:37:eDP-1]
> > > [    5.768786] [drm:intel_dp_probe_oui] Sink OUI: 0022b9
> > > [    5.769177] [drm:intel_dp_probe_oui] Branch OUI: 444615
> > > [    5.769604] [drm:drm_edid_to_eld] ELD: no CEA Extension found
> > > [    5.769611] [drm:drm_helper_probe_single_connector_modes]
> > > [CONNECTOR:37:eDP-1] probed modes :
> > > [    5.769618] [drm:drm_mode_debug_printmodeline] Modeline 38:"1920x1080" 60
> > > 142900 1920 1968 2000 2082 1080 1082 1087 1144 0x48 0xa
> > > [    5.769621] [drm:drm_helper_probe_single_connector_modes]
> > > [CONNECTOR:45:DP-1]
> > > [    5.769624] [drm:intel_dp_detect] [CONNECTOR:45:DP-1]
> > > [    5.769628] [drm:intel_power_well_enable] enabling power well 2
> > > [    5.769633] [drm:skl_set_power_well] Enabling power well 2
> > > [    5.769659] [drm:intel_power_well_disable] disabling power well 2
> > > [    5.769672] [drm:skl_set_power_well] Disabling power well 2
> > > 
> > > On Tue, 2016-04-19 at 19:26 -0700, Matt Roper wrote:
> > > > 
> > > > 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.
> > > > 
> > > > 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 | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 59d4574..96ffd54 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3309,7 +3309,17 @@ 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");
> > > > +			return -EINVAL;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	*out_blocks = res_blocks;

-- 
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 v2 13/16] drm/i915/gen9: Propagate watermark calculation failures up the call chain

[Maarten Lankhorst]

Op 22-04-16 om 01:20 schreef Matt Roper:
> On Thu, Apr 21, 2016 at 02:19:33PM +0200, Maarten Lankhorst wrote:
>> Op 20-04-16 om 04:26 schreef 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 21fde05..f07054a 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3241,13 +3241,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;
>>> @@ -3259,8 +3260,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;
>>> @@ -3321,13 +3324,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
>>> @@ -3345,6 +3351,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;
>>>  
>>>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>>>  		int i = skl_wm_plane_id(intel_plane);
>>> @@ -3374,13 +3381,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;
>>> @@ -3417,21 +3427,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,
>>> @@ -3678,21 +3693,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,
>>> @@ -3725,8 +3746,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
>>> @@ -3827,14 +3848,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);
>> With the nitpicks fixed, and with CI happy.
>>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> I'm interpreting this to refer to the entire series (and not just this
> one patch that already had your r-b and no nitpicks); if that's not what
> you meant, please let me know.  :-)
>
Oops indeed. That's the case. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx