[PATCH 1/2] drm/i915: Allow preservation of watermarks.

[PATCH 1/2] drm/i915: Allow preservation of watermarks.

[Maarten Lankhorst]

As Paulo has noted we can help bisectability by separating computing
watermarks on a noop in 2 separate commits.

This patch no longer clears the crtc watermark state, but recalculates
it completely. Regardless whether a level is used the full values for
each level are calculated. If a level is invalid wm[level].enable is
unset.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d33de954a2e4..1b8ba777d2b3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
-	int level, max_level = ilk_wm_max_level(dev);
+	int level, max_level = ilk_wm_max_level(dev), usable_level;
 	struct ilk_wm_maximums max;
 
 	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 		return PTR_ERR(cstate);
 
 	pipe_wm = &cstate->wm.optimal.ilk;
-	memset(pipe_wm, 0, sizeof(*pipe_wm));
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		ps = drm_atomic_get_plane_state(state,
@@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
 		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
 
+	usable_level = max_level;
+
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
 	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
-		max_level = 1;
+		usable_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
 	if (pipe_wm->sprites_scaled)
-		max_level = 0;
+		usable_level = 0;
 
 	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
 			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
@@ -2350,20 +2351,21 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
-				     pristate, sprstate, curstate, &wm);
+				     pristate, sprstate, curstate, wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (!ilk_validate_wm_level(level, &max, &wm))
-			break;
-
-		pipe_wm->wm[level] = wm;
+		if (!ilk_validate_wm_level(level, &max, wm)) {
+			usable_level = level;
+			wm->enable = false;
+		} else if (level > usable_level)
+			wm->enable = false;
 	}
 
 	return 0;
-- 
2.1.0

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

[PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.

[Maarten Lankhorst]

Only planes that are part of the state should be used for recalculating
watermarks. For planes not part of the state the previous patch allows
us to re-use the old values since they're calculated even for levels
that are not actively used.

Changes since v1:
- Remove big if from intel_crtc_atomic_check.
- Remove extra newline.
- Remove memset in ilk_compute_pipe_wm.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 671295523317..4b8f446cdf44 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,8 +629,7 @@ struct drm_i915_display_funcs {
 			  int target, int refclk,
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
-	int (*compute_pipe_wm)(struct intel_crtc *crtc,
-			       struct drm_atomic_state *state);
+	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
 	int (*compute_intermediate_wm)(struct drm_device *dev,
 				       struct intel_crtc *intel_crtc,
 				       struct intel_crtc_state *newstate);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 79bf527e0a73..20a4cb3f69d1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11994,7 +11994,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 
 	ret = 0;
 	if (dev_priv->display.compute_pipe_wm) {
-		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
+		ret = dev_priv->display.compute_pipe_wm(pipe_config);
 		if (ret) {
 			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
 			return ret;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5daf53c080e1..03b7d031e910 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1630,6 +1630,18 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
+				      struct intel_plane *plane)
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
+
+	return to_intel_plane_state(plane_state);
+}
+
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1b8ba777d2b3..dc1e1683f4a3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1996,11 +1996,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
-					     pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
-	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
-	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	if (pristate) {
+		result->pri_val = ilk_compute_pri_wm(cstate, pristate,
+						     pri_latency, level);
+		result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
+	}
+
+	if (sprstate)
+		result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
+
+	if (curstate)
+		result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
+
 	result->enable = true;
 }
 
@@ -2288,51 +2295,51 @@ static bool ilk_validate_pipe_wm(struct drm_device *dev,
 }
 
 /* Compute new watermarks for the pipe */
-static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
-			       struct drm_atomic_state *state)
+static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 {
+	struct drm_atomic_state *state = cstate->base.state;
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 	struct intel_pipe_wm *pipe_wm;
-	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_device *dev = state->dev;
 	const struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc_state *cstate = NULL;
 	struct intel_plane *intel_plane;
-	struct drm_plane_state *ps;
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
 	int level, max_level = ilk_wm_max_level(dev), usable_level;
 	struct ilk_wm_maximums max;
 
-	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
-	if (IS_ERR(cstate))
-		return PTR_ERR(cstate);
-
 	pipe_wm = &cstate->wm.optimal.ilk;
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		ps = drm_atomic_get_plane_state(state,
-						&intel_plane->base);
-		if (IS_ERR(ps))
-			return PTR_ERR(ps);
+		struct intel_plane_state *ps;
+
+		ps = intel_atomic_get_existing_plane_state(state,
+							   intel_plane);
+		if (!ps)
+			continue;
 
 		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
-			pristate = to_intel_plane_state(ps);
+			pristate = ps;
 		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
-			sprstate = to_intel_plane_state(ps);
+			sprstate = ps;
 		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
-			curstate = to_intel_plane_state(ps);
+			curstate = ps;
 	}
 
 	pipe_wm->pipe_enabled = cstate->base.active;
-	pipe_wm->sprites_enabled = sprstate->visible;
-	pipe_wm->sprites_scaled = sprstate->visible &&
-		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
-		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
+	if (sprstate) {
+		pipe_wm->sprites_enabled = sprstate->visible;
+		pipe_wm->sprites_scaled = sprstate->visible &&
+			(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
+			 drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
+	}
+
 
 	usable_level = max_level;
 
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
-	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
+	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
 		usable_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
-- 
2.1.0

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Allow preservation of watermarks.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Allow preservation of watermarks.
URL   : https://patchwork.freedesktop.org/series/3958/
State : failure

== Summary ==

Series 3958v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/3958/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (hsw-brixbox)
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:169  pass:137  dwarn:1   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:144  dwarn:0   dfail:0   fail:0   skip:25 
hsw-brixbox      total:169  pass:153  dwarn:1   dfail:0   fail:0   skip:15 
hsw-gt2          total:169  pass:157  dwarn:1   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:169  pass:117  dwarn:1   dfail:0   fail:1   skip:50 
ivb-t430s        total:169  pass:152  dwarn:0   dfail:0   fail:1   skip:16 
skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
snb-dellxps      total:169  pass:144  dwarn:1   dfail:0   fail:1   skip:23 

Results at /archive/results/CI_IGT_test/Patchwork_1500/

deb861ea08c6d5bbf682fb40ea1f0471a448aafd drm-intel-nightly: 2016y-03m-01d-11h-12m-16s UTC integration manifest
e8ceac9f787ce5e1c2514ecf18544e33ea510956 drm/i915: Only recalculate wm's for planes part of the state, v2.
4b4e856bf5a2b802a78af6d206faf7e3050b7ab5 drm/i915: Allow preservation of watermarks.

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

Re: [PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.

[Matt Roper]

On Tue, Mar 01, 2016 at 11:07:22AM +0100, Maarten Lankhorst wrote:
> Only planes that are part of the state should be used for recalculating
> watermarks. For planes not part of the state the previous patch allows
> us to re-use the old values since they're calculated even for levels
> that are not actively used.
> 
> Changes since v1:
> - Remove big if from intel_crtc_atomic_check.
> - Remove extra newline.
> - Remove memset in ilk_compute_pipe_wm.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>

I haven't thought through this too carefully yet, but off the top of my
head I'm not sure if this will work for SKL once we transition it to
also use a more atomic style.  Changes to other state might result in
changes to DDB allocation, making our previously-calculated values
invalid.

I think this is okay for the current code (where only ILK is atomic), so

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

for the time being.  I'll be back to looking at SKL-style watermarks in
the next day or two and I might have to backtrack somewhat in cases
where a DDB partitioning changes results in a full recompute, but I need
to think through the details a bit more about how best to handle that.


Matt


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 12 ++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 59 ++++++++++++++++++++----------------
>  4 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 671295523317..4b8f446cdf44 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -629,8 +629,7 @@ struct drm_i915_display_funcs {
>  			  int target, int refclk,
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
> -	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> -			       struct drm_atomic_state *state);
> +	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *newstate);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 79bf527e0a73..20a4cb3f69d1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,7 +11994,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  	ret = 0;
>  	if (dev_priv->display.compute_pipe_wm) {
> -		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
> +		ret = dev_priv->display.compute_pipe_wm(pipe_config);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5daf53c080e1..03b7d031e910 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1630,6 +1630,18 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  
>  	return to_intel_crtc_state(crtc_state);
>  }
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
> +				      struct intel_plane *plane)
> +{
> +	struct drm_plane_state *plane_state;
> +
> +	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
> +
> +	return to_intel_plane_state(plane_state);
> +}
> +
>  int intel_atomic_setup_scalers(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1b8ba777d2b3..dc1e1683f4a3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1996,11 +1996,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  		cur_latency *= 5;
>  	}
>  
> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> -					     pri_latency, level);
> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
> +	if (pristate) {
> +		result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> +						     pri_latency, level);
> +		result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
> +	}
> +
> +	if (sprstate)
> +		result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> +
> +	if (curstate)
> +		result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> +
>  	result->enable = true;
>  }
>  
> @@ -2288,51 +2295,51 @@ static bool ilk_validate_pipe_wm(struct drm_device *dev,
>  }
>  
>  /* Compute new watermarks for the pipe */
> -static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> -			       struct drm_atomic_state *state)
> +static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  {
> +	struct drm_atomic_state *state = cstate->base.state;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  	struct intel_pipe_wm *pipe_wm;
> -	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_device *dev = state->dev;
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc_state *cstate = NULL;
>  	struct intel_plane *intel_plane;
> -	struct drm_plane_state *ps;
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
> -	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> -	if (IS_ERR(cstate))
> -		return PTR_ERR(cstate);
> -
>  	pipe_wm = &cstate->wm.optimal.ilk;
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		ps = drm_atomic_get_plane_state(state,
> -						&intel_plane->base);
> -		if (IS_ERR(ps))
> -			return PTR_ERR(ps);
> +		struct intel_plane_state *ps;
> +
> +		ps = intel_atomic_get_existing_plane_state(state,
> +							   intel_plane);
> +		if (!ps)
> +			continue;
>  
>  		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -			pristate = to_intel_plane_state(ps);
> +			pristate = ps;
>  		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> -			sprstate = to_intel_plane_state(ps);
> +			sprstate = ps;
>  		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> -			curstate = to_intel_plane_state(ps);
> +			curstate = ps;
>  	}
>  
>  	pipe_wm->pipe_enabled = cstate->base.active;
> -	pipe_wm->sprites_enabled = sprstate->visible;
> -	pipe_wm->sprites_scaled = sprstate->visible &&
> -		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> -		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> +	if (sprstate) {
> +		pipe_wm->sprites_enabled = sprstate->visible;
> +		pipe_wm->sprites_scaled = sprstate->visible &&
> +			(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> +			 drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> +	}
> +
>  
>  	usable_level = max_level;
>  
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
>  		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> -- 
> 2.1.0
> 

-- 
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 1/2] drm/i915: Allow preservation of watermarks.

[Ville Syrjälä]

On Tue, Mar 01, 2016 at 11:07:21AM +0100, Maarten Lankhorst wrote:
> As Paulo has noted we can help bisectability by separating computing
> watermarks on a noop in 2 separate commits.
> 
> This patch no longer clears the crtc watermark state, but recalculates
> it completely. Regardless whether a level is used the full values for
> each level are calculated. If a level is invalid wm[level].enable is
> unset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d33de954a2e4..1b8ba777d2b3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		return PTR_ERR(cstate);
>  
>  	pipe_wm = &cstate->wm.optimal.ilk;
> -	memset(pipe_wm, 0, sizeof(*pipe_wm));
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		ps = drm_atomic_get_plane_state(state,
> @@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
>  
> +	usable_level = max_level;
> +
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> -		max_level = 1;
> +		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>  	if (pipe_wm->sprites_scaled)
> -		max_level = 0;
> +		usable_level = 0;
>  
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>  			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
> @@ -2350,20 +2351,21 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level wm = {};
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> -				     pristate, sprstate, curstate, &wm);
> +				     pristate, sprstate, curstate, wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (!ilk_validate_wm_level(level, &max, &wm))
> -			break;
> -
> -		pipe_wm->wm[level] = wm;
> +		if (!ilk_validate_wm_level(level, &max, wm)) {
> +			usable_level = level;
> +			wm->enable = false;
> +		} else if (level > usable_level)
> +			wm->enable = false;

That seems to be the wrong way around. I don't think there's
any point in calling ilk_validate_wm_level() if the level
already exceeds the max.

>  	}
>  
>  	return 0;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm/i915: Allow preservation of watermarks.

[Maarten Lankhorst]

Op 02-03-16 om 12:17 schreef Ville Syrjälä:
> On Tue, Mar 01, 2016 at 11:07:21AM +0100, Maarten Lankhorst wrote:
>> As Paulo has noted we can help bisectability by separating computing
>> watermarks on a noop in 2 separate commits.
>>
>> This patch no longer clears the crtc watermark state, but recalculates
>> it completely. Regardless whether a level is used the full values for
>> each level are calculated. If a level is invalid wm[level].enable is
>> unset.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index d33de954a2e4..1b8ba777d2b3 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>>  	struct intel_plane_state *pristate = NULL;
>>  	struct intel_plane_state *sprstate = NULL;
>>  	struct intel_plane_state *curstate = NULL;
>> -	int level, max_level = ilk_wm_max_level(dev);
>> +	int level, max_level = ilk_wm_max_level(dev), usable_level;
>>  	struct ilk_wm_maximums max;
>>  
>>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
>> @@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>>  		return PTR_ERR(cstate);
>>  
>>  	pipe_wm = &cstate->wm.optimal.ilk;
>> -	memset(pipe_wm, 0, sizeof(*pipe_wm));
>>  
>>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>>  		ps = drm_atomic_get_plane_state(state,
>> @@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>>  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
>>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
>>  
>> +	usable_level = max_level;
>> +
>>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
>> -		max_level = 1;
>> +		usable_level = 1;
>>  
>>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>>  	if (pipe_wm->sprites_scaled)
>> -		max_level = 0;
>> +		usable_level = 0;
>>  
>>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>>  			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
>> @@ -2350,20 +2351,21 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>>  
>>  	for (level = 1; level <= max_level; level++) {
>> -		struct intel_wm_level wm = {};
>> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>>  
>>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
>> -				     pristate, sprstate, curstate, &wm);
>> +				     pristate, sprstate, curstate, wm);
>>  
>>  		/*
>>  		 * Disable any watermark level that exceeds the
>>  		 * register maximums since such watermarks are
>>  		 * always invalid.
>>  		 */
>> -		if (!ilk_validate_wm_level(level, &max, &wm))
>> -			break;
>> -
>> -		pipe_wm->wm[level] = wm;
>> +		if (!ilk_validate_wm_level(level, &max, wm)) {
>> +			usable_level = level;
>> +			wm->enable = false;
>> +		} else if (level > usable_level)
>> +			wm->enable = false;
> That seems to be the wrong way around. I don't think there's
> any point in calling ilk_validate_wm_level() if the level
> already exceeds the max.
>
Indeed! Will fix.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v1.1 1/2] drm/i915: Allow preservation of watermarks, v2.

[Maarten Lankhorst]

As Paulo has noted we can help bisectability by separating computing
watermarks on a noop in 2 separate commits.

This patch no longer clears the crtc watermark state, but recalculates
it completely. Regardless whether a level is used the full values for
each level are calculated. If a level is invalid wm[level].enable is
unset.

Changes since v1:
- Only call ilk_validate_wm_level when level <= usable_level.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 61dcac9dea37..4c21b98e5a5c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
 	struct intel_plane_state *curstate = NULL;
-	int level, max_level = ilk_wm_max_level(dev);
+	int level, max_level = ilk_wm_max_level(dev), usable_level;
 	struct ilk_wm_maximums max;
 
 	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 		return PTR_ERR(cstate);
 
 	pipe_wm = &cstate->wm.optimal.ilk;
-	memset(pipe_wm, 0, sizeof(*pipe_wm));
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		ps = drm_atomic_get_plane_state(state,
@@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
 		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
 
+	usable_level = max_level;
+
 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
 	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
-		max_level = 1;
+		usable_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
 	if (pipe_wm->sprites_scaled)
-		max_level = 0;
+		usable_level = 0;
 
 	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
 			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
@@ -2350,20 +2351,22 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
-				     pristate, sprstate, curstate, &wm);
+				     pristate, sprstate, curstate, wm);
 
 		/*
 		 * Disable any watermark level that exceeds the
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (!ilk_validate_wm_level(level, &max, &wm))
-			break;
-
-		pipe_wm->wm[level] = wm;
+		if (level > usable_level) {
+			wm->enable = false;
+		} else if (!ilk_validate_wm_level(level, &max, wm)) {
+			wm->enable = false;
+			usable_level = level;
+		}
 	}
 
 	return 0;
-- 
2.1.0


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

Re: [PATCH v1.1 1/2] drm/i915: Allow preservation of watermarks, v2.

[Zanoni, Paulo R]

Em Qua, 2016-03-02 às 12:38 +0100, Maarten Lankhorst escreveu:
> As Paulo has noted we can help bisectability by separating computing
> watermarks on a noop in 2 separate commits.
> 
> This patch no longer clears the crtc watermark state, but
> recalculates
> it completely. Regardless whether a level is used the full values for
> each level are calculated. If a level is invalid wm[level].enable is
> unset.
> 
> Changes since v1:
> - Only call ilk_validate_wm_level when level <= usable_level.

LGTM.

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

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 61dcac9dea37..4c21b98e5a5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  		return PTR_ERR(cstate);
>  
>  	pipe_wm = &cstate->wm.optimal.ilk;
> -	memset(pipe_wm, 0, sizeof(*pipe_wm));
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		ps = drm_atomic_get_plane_state(state,
> @@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  		(drm_rect_width(&sprstate->dst) !=
> drm_rect_width(&sprstate->src) >> 16 ||
>  		drm_rect_height(&sprstate->dst) !=
> drm_rect_height(&sprstate->src) >> 16);
>  
> +	usable_level = max_level;
> +
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> -		max_level = 1;
> +		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>  	if (pipe_wm->sprites_scaled)
> -		max_level = 0;
> +		usable_level = 0;
>  
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>  			     pristate, sprstate, curstate, &pipe_wm-
> >wm[0]);
> @@ -2350,20 +2351,22 @@ static int ilk_compute_pipe_wm(struct
> intel_crtc *intel_crtc,
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level wm = {};
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level,
> cstate,
> -				     pristate, sprstate, curstate,
> &wm);
> +				     pristate, sprstate, curstate,
> wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (!ilk_validate_wm_level(level, &max, &wm))
> -			break;
> -
> -		pipe_wm->wm[level] = wm;
> +		if (level > usable_level) {
> +			wm->enable = false;
> +		} else if (!ilk_validate_wm_level(level, &max, wm))
> {
> +			wm->enable = false;
> +			usable_level = level;
> +		}
>  	}
>  
>  	return 0;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.

[Zanoni, Paulo R]

Em Ter, 2016-03-01 às 14:28 -0800, Matt Roper escreveu:
> On Tue, Mar 01, 2016 at 11:07:22AM +0100, Maarten Lankhorst wrote:
> > Only planes that are part of the state should be used for
> > recalculating
> > watermarks. For planes not part of the state the previous patch
> > allows
> > us to re-use the old values since they're calculated even for
> > levels
> > that are not actively used.
> > 
> > Changes since v1:
> > - Remove big if from intel_crtc_atomic_check.
> > - Remove extra newline.
> > - Remove memset in ilk_compute_pipe_wm.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > >
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> 
> I haven't thought through this too carefully yet, but off the top of
> my
> head I'm not sure if this will work for SKL once we transition it to
> also use a more atomic style.  Changes to other state might result in
> changes to DDB allocation, making our previously-calculated values
> invalid.
> 
> I think this is okay for the current code (where only ILK is atomic),
> so
> 
>     Acknowledged-by: Matt Roper <matthew.d.roper@intel.com>
> 
> for the time being.  I'll be back to looking at SKL-style watermarks
> in
> the next day or two and I might have to backtrack somewhat in cases
> where a DDB partitioning changes results in a full recompute, but I
> need
> to think through the details a bit more about how best to handle
> that.

I spent some time looking at that early return from invalid pipe
watermarks, but I suppose that return means we'll completely discard
anything just computed by the function, right?

The patch seems to do what it says on the box, so if we assume we
actually want the patch:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'll let you and Matt decide whether we actually want the patch or not.



> Matt
> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
> >  drivers/gpu/drm/i915/intel_display.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h     | 12 ++++++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 59 ++++++++++++++++++++--
> > --------------
> >  4 files changed, 47 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 671295523317..4b8f446cdf44 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -629,8 +629,7 @@ struct drm_i915_display_funcs {
> >  			  int target, int refclk,
> >  			  struct dpll *match_clock,
> >  			  struct dpll *best_clock);
> > -	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> > -			       struct drm_atomic_state *state);
> > +	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
> >  	int (*compute_intermediate_wm)(struct drm_device *dev,
> >  				       struct intel_crtc
> > *intel_crtc,
> >  				       struct intel_crtc_state
> > *newstate);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 79bf527e0a73..20a4cb3f69d1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11994,7 +11994,7 @@ static int intel_crtc_atomic_check(struct
> > drm_crtc *crtc,
> >  
> >  	ret = 0;
> >  	if (dev_priv->display.compute_pipe_wm) {
> > -		ret = dev_priv-
> > >display.compute_pipe_wm(intel_crtc, state);
> > +		ret = dev_priv-
> > >display.compute_pipe_wm(pipe_config);
> >  		if (ret) {
> >  			DRM_DEBUG_KMS("Target pipe watermarks are
> > invalid\n");
> >  			return ret;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 5daf53c080e1..03b7d031e910 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1630,6 +1630,18 @@ intel_atomic_get_crtc_state(struct
> > drm_atomic_state *state,
> >  
> >  	return to_intel_crtc_state(crtc_state);
> >  }
> > +
> > +static inline struct intel_plane_state *
> > +intel_atomic_get_existing_plane_state(struct drm_atomic_state
> > *state,
> > +				      struct intel_plane *plane)
> > +{
> > +	struct drm_plane_state *plane_state;
> > +
> > +	plane_state = drm_atomic_get_existing_plane_state(state,
> > &plane->base);
> > +
> > +	return to_intel_plane_state(plane_state);
> > +}
> > +
> >  int intel_atomic_setup_scalers(struct drm_device *dev,
> >  	struct intel_crtc *intel_crtc,
> >  	struct intel_crtc_state *crtc_state);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 1b8ba777d2b3..dc1e1683f4a3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1996,11 +1996,18 @@ static void ilk_compute_wm_level(const
> > struct drm_i915_private *dev_priv,
> >  		cur_latency *= 5;
> >  	}
> >  
> > -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> > -					     pri_latency, level);
> > -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
> > spr_latency);
> > -	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
> > cur_latency);
> > -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
> > result->pri_val);
> > +	if (pristate) {
> > +		result->pri_val = ilk_compute_pri_wm(cstate,
> > pristate,
> > +						     pri_latency,
> > level);
> > +		result->fbc_val = ilk_compute_fbc_wm(cstate,
> > pristate, result->pri_val);
> > +	}
> > +
> > +	if (sprstate)
> > +		result->spr_val = ilk_compute_spr_wm(cstate,
> > sprstate, spr_latency);
> > +
> > +	if (curstate)
> > +		result->cur_val = ilk_compute_cur_wm(cstate,
> > curstate, cur_latency);
> > +
> >  	result->enable = true;
> >  }
> >  
> > @@ -2288,51 +2295,51 @@ static bool ilk_validate_pipe_wm(struct
> > drm_device *dev,
> >  }
> >  
> >  /* Compute new watermarks for the pipe */
> > -static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> > -			       struct drm_atomic_state *state)
> > +static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
> >  {
> > +	struct drm_atomic_state *state = cstate->base.state;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > >base.crtc);
> >  	struct intel_pipe_wm *pipe_wm;
> > -	struct drm_device *dev = intel_crtc->base.dev;
> > +	struct drm_device *dev = state->dev;
> >  	const struct drm_i915_private *dev_priv = dev-
> > >dev_private;
> > -	struct intel_crtc_state *cstate = NULL;
> >  	struct intel_plane *intel_plane;
> > -	struct drm_plane_state *ps;
> >  	struct intel_plane_state *pristate = NULL;
> >  	struct intel_plane_state *sprstate = NULL;
> >  	struct intel_plane_state *curstate = NULL;
> >  	int level, max_level = ilk_wm_max_level(dev),
> > usable_level;
> >  	struct ilk_wm_maximums max;
> >  
> > -	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> > -	if (IS_ERR(cstate))
> > -		return PTR_ERR(cstate);
> > -
> >  	pipe_wm = &cstate->wm.optimal.ilk;
> >  
> >  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)
> > {
> > -		ps = drm_atomic_get_plane_state(state,
> > -						&intel_plane-
> > >base);
> > -		if (IS_ERR(ps))
> > -			return PTR_ERR(ps);
> > +		struct intel_plane_state *ps;
> > +
> > +		ps = intel_atomic_get_existing_plane_state(state,
> > +							   intel_p
> > lane);
> > +		if (!ps)
> > +			continue;
> >  
> >  		if (intel_plane->base.type ==
> > DRM_PLANE_TYPE_PRIMARY)
> > -			pristate = to_intel_plane_state(ps);
> > +			pristate = ps;
> >  		else if (intel_plane->base.type ==
> > DRM_PLANE_TYPE_OVERLAY)
> > -			sprstate = to_intel_plane_state(ps);
> > +			sprstate = ps;
> >  		else if (intel_plane->base.type ==
> > DRM_PLANE_TYPE_CURSOR)
> > -			curstate = to_intel_plane_state(ps);
> > +			curstate = ps;
> >  	}
> >  
> >  	pipe_wm->pipe_enabled = cstate->base.active;
> > -	pipe_wm->sprites_enabled = sprstate->visible;
> > -	pipe_wm->sprites_scaled = sprstate->visible &&
> > -		(drm_rect_width(&sprstate->dst) !=
> > drm_rect_width(&sprstate->src) >> 16 ||
> > -		drm_rect_height(&sprstate->dst) !=
> > drm_rect_height(&sprstate->src) >> 16);
> > +	if (sprstate) {
> > +		pipe_wm->sprites_enabled = sprstate->visible;
> > +		pipe_wm->sprites_scaled = sprstate->visible &&
> > +			(drm_rect_width(&sprstate->dst) !=
> > drm_rect_width(&sprstate->src) >> 16 ||
> > +			 drm_rect_height(&sprstate->dst) !=
> > drm_rect_height(&sprstate->src) >> 16);
> > +	}
> > +
> >  
> >  	usable_level = max_level;
> >  
> >  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> > -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> > +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
> >  		usable_level = 1;
> >  
> >  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> > -- 
> > 2.1.0
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.

[Maarten Lankhorst]

Op 02-03-16 om 22:08 schreef Zanoni, Paulo R:
> Em Ter, 2016-03-01 às 14:28 -0800, Matt Roper escreveu:
>> On Tue, Mar 01, 2016 at 11:07:22AM +0100, Maarten Lankhorst wrote:
>>> Only planes that are part of the state should be used for
>>> recalculating
>>> watermarks. For planes not part of the state the previous patch
>>> allows
>>> us to re-use the old values since they're calculated even for
>>> levels
>>> that are not actively used.
>>>
>>> Changes since v1:
>>> - Remove big if from intel_crtc_atomic_check.
>>> - Remove extra newline.
>>> - Remove memset in ilk_compute_pipe_wm.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> I haven't thought through this too carefully yet, but off the top of
>> my
>> head I'm not sure if this will work for SKL once we transition it to
>> also use a more atomic style.  Changes to other state might result in
>> changes to DDB allocation, making our previously-calculated values
>> invalid.
>>
>> I think this is okay for the current code (where only ILK is atomic),
>> so
>>
>>     Acknowledged-by: Matt Roper <matthew.d.roper@intel.com>
>>
>> for the time being.  I'll be back to looking at SKL-style watermarks
>> in
>> the next day or two and I might have to backtrack somewhat in cases
>> where a DDB partitioning changes results in a full recompute, but I
>> need
>> to think through the details a bit more about how best to handle
>> that.
> I spent some time looking at that early return from invalid pipe
> watermarks, but I suppose that return means we'll completely discard
> anything just computed by the function, right?
>
> The patch seems to do what it says on the box, so if we assume we
> actually want the patch:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I'll let you and Matt decide whether we actually want the patch or not.
>
This was a bug. I sent a fix for that one. When wm calculation fails -EINVAL is returned now,
and the invalid wm's discarded.

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

Re: [PATCH v1.1 1/2] drm/i915: Allow preservation of watermarks, v2.

[Ville Syrjälä]

On Wed, Mar 02, 2016 at 12:38:06PM +0100, Maarten Lankhorst wrote:
> As Paulo has noted we can help bisectability by separating computing
> watermarks on a noop in 2 separate commits.
> 
> This patch no longer clears the crtc watermark state, but recalculates
> it completely. Regardless whether a level is used the full values for
> each level are calculated. If a level is invalid wm[level].enable is
> unset.
> 
> Changes since v1:
> - Only call ilk_validate_wm_level when level <= usable_level.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

After thinking a bit more on this I think we need to do this
differently.

What should happen is each plane's watermarks are computed entirely
indpendently, then we would collect them up when coming up with the
optimal wms, limiting the resulting set to the max usable level,
leaving all higher levels at 0. This part is what was lost with the
current method, and it can even result in totally invalid watermark
values ending up merged into the final result.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 61dcac9dea37..4c21b98e5a5c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2300,7 +2300,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
> -	int level, max_level = ilk_wm_max_level(dev);
> +	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2308,7 +2308,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		return PTR_ERR(cstate);
>  
>  	pipe_wm = &cstate->wm.optimal.ilk;
> -	memset(pipe_wm, 0, sizeof(*pipe_wm));
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		ps = drm_atomic_get_plane_state(state,
> @@ -2330,13 +2329,15 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
>  
> +	usable_level = max_level;
> +
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
>  	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> -		max_level = 1;
> +		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
>  	if (pipe_wm->sprites_scaled)
> -		max_level = 0;
> +		usable_level = 0;
>  
>  	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
>  			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
> @@ -2350,20 +2351,22 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
> -		struct intel_wm_level wm = {};
> +		struct intel_wm_level *wm = &pipe_wm->wm[level];
>  
>  		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> -				     pristate, sprstate, curstate, &wm);
> +				     pristate, sprstate, curstate, wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
>  		 * register maximums since such watermarks are
>  		 * always invalid.
>  		 */
> -		if (!ilk_validate_wm_level(level, &max, &wm))
> -			break;
> -
> -		pipe_wm->wm[level] = wm;
> +		if (level > usable_level) {
> +			wm->enable = false;
> +		} else if (!ilk_validate_wm_level(level, &max, wm)) {
> +			wm->enable = false;
> +			usable_level = level;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.1.0
> 

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