[PATCH v2 0/4] drm/i915: Calculate intermediate watermarks correctly.

[PATCH v2 0/4] drm/i915: Calculate intermediate watermarks correctly.

[Maarten Lankhorst]

Calculate intermediate watermarks with the correct previous optimal watermark values.
This is the same as v1, but with helper macro to get the old state.

Maarten Lankhorst (4):
  drm/i915: Change use get_new_plane_state instead of existing plane
    state
  drm/i915: Change get_existing_crtc_state to old state
  drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
  drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

 drivers/gpu/drm/i915/intel_atomic.c |  4 ++--
 drivers/gpu/drm/i915/intel_drv.h    | 10 +++++-----
 drivers/gpu/drm/i915/intel_pm.c     | 33 ++++++++++++++++++++++-----------
 3 files changed, 29 insertions(+), 18 deletions(-)

-- 
2.9.3

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

[PATCH v2 1/4] drm/i915: Change use get_new_plane_state instead of existing plane state

[Maarten Lankhorst]

The get_existing macros are deprecated and should be replaced by
get_old/new_state for clarity.

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

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index d791b3ef89b5..87b1dd464eee 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -301,8 +301,8 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
 				continue;
 			}
 
-			plane_state = intel_atomic_get_existing_plane_state(drm_state,
-									    intel_plane);
+			plane_state = intel_atomic_get_new_plane_state(drm_state,
+								       intel_plane);
 			scaler_id = &plane_state->scaler_id;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 110d9088be35..054b454a8d07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1929,12 +1929,12 @@ intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
 }
 
 static inline struct intel_plane_state *
-intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
+intel_atomic_get_new_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);
+	plane_state = drm_atomic_get_new_plane_state(state, &plane->base);
 
 	return to_intel_plane_state(plane_state);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cacb65fa2dd5..246e0723e6e9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2561,8 +2561,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		struct intel_plane_state *ps;
 
-		ps = intel_atomic_get_existing_plane_state(state,
-							   intel_plane);
+		ps = intel_atomic_get_new_plane_state(state, intel_plane);
 		if (!ps)
 			continue;
 
@@ -3924,8 +3923,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 
 	if (state)
 		intel_pstate =
-			intel_atomic_get_existing_plane_state(state,
-							      intel_plane);
+			intel_atomic_get_new_plane_state(state, intel_plane);
 
 	/*
 	 * Note: If we start supporting multiple pending atomic commits against
-- 
2.9.3

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

[PATCH v2 2/4] drm/i915: Change get_existing_crtc_state to old state

[Maarten Lankhorst]

get_existing_crtc_state is currently unused, but the next commit
needs to access the old intel state. Rename this and use this
as convenience wrapper.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 054b454a8d07..f69b56d8f7d7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1915,12 +1915,12 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 }
 
 static inline struct intel_crtc_state *
-intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
-				     struct intel_crtc *crtc)
+intel_atomic_get_old_crtc_state(struct drm_atomic_state *state,
+				struct intel_crtc *crtc)
 {
 	struct drm_crtc_state *crtc_state;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state, &crtc->base);
+	crtc_state = drm_atomic_get_old_crtc_state(state, &crtc->base);
 
 	if (crtc_state)
 		return to_intel_crtc_state(crtc_state);
-- 
2.9.3

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

[PATCH v2 3/4] drm/i915: Calculate ironlake intermediate watermarks correctly, v2.

[Maarten Lankhorst]

The watermarks it should calculate against are the old optimal watermarks.
The currently active crtc watermarks are pure fiction, and are invalid in
case of a nonblocking modeset, page flip enabling/disabling planes or any
other reason.

When the crtc is disabled or during a modeset the intermediate watermarks
don't need to be programmed separately, and could be directly assigned
to the optimal watermarks.

Changes since v1:
- Use intel_atomic_get_old_crtc_state. (ville)

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 246e0723e6e9..0f344b1fff45 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2638,7 +2638,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 				       struct intel_crtc_state *newstate)
 {
 	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
-	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
+	const struct intel_crtc_state *oldstate =
+		intel_atomic_get_old_crtc_state(newstate->base.state, intel_crtc);
+	const struct intel_pipe_wm *b = &oldstate->wm.ilk.optimal;
 	int level, max_level = ilk_wm_max_level(to_i915(dev));
 
 	/*
@@ -2647,6 +2649,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
 	 * and after the vblank.
 	 */
 	*a = newstate->wm.ilk.optimal;
+	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
+		return 0;
+
 	a->pipe_enabled |= b->pipe_enabled;
 	a->sprites_enabled |= b->sprites_enabled;
 	a->sprites_scaled |= b->sprites_scaled;
-- 
2.9.3

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

[PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

The watermarks it should calculate against are the old optimal watermarks.
The currently active crtc watermarks are pure fiction, and are invalid in
case of a nonblocking modeset, page flip enabling/disabling planes or any
other reason.

When the crtc is disabled or during a modeset the intermediate watermarks
don't need to be programmed separately, and could be directly assigned
to the optimal watermarks.

Also rename crtc_state to new_crtc_state, to distinguish it from the old state.

Changes since v1:
- Use intel_atomic_get_old_crtc_state. (ville)

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0f344b1fff45..a09396ee1f3d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 
 static int vlv_compute_intermediate_wm(struct drm_device *dev,
 				       struct intel_crtc *crtc,
-				       struct intel_crtc_state *crtc_state)
+				       struct intel_crtc_state *new_crtc_state)
 {
-	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
-	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
-	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
+	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
+	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
+	const struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
+	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
 	int level;
 
+	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
+		*intermediate = *optimal;
+
+		return 0;
+	}
+
 	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
 	intermediate->cxsr = optimal->cxsr && active->cxsr &&
-		!crtc_state->disable_cxsr;
+		!new_crtc_state->disable_cxsr;
 
 	for (level = 0; level < intermediate->num_levels; level++) {
 		enum plane_id plane_id;
@@ -1491,7 +1499,7 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev,
 	 * omit the post-vblank programming; only update if it's different.
 	 */
 	if (memcmp(intermediate, optimal, sizeof(*intermediate)) != 0)
-		crtc_state->wm.need_postvbl_update = true;
+		new_crtc_state->wm.need_postvbl_update = true;
 
 	return 0;
 }
-- 
2.9.3

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

✓ Fi.CI.BAT: success for drm/i915: Calculate intermediate watermarks correctly.

[Patchwork]

== Series Details ==

Series: drm/i915: Calculate intermediate watermarks correctly.
URL   : https://patchwork.freedesktop.org/series/23769/
State : success

== Summary ==

Series 23769v1 drm/i915: Calculate intermediate watermarks correctly.
https://patchwork.freedesktop.org/api/1.0/series/23769/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:429s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:436s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:584s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:509s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:563s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:483s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:412s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:402s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:455s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:567s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:570s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:463s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:497s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:428s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:525s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:399s

1d490e4b6d5324cfbf8dc800cf4a99471252802c drm-tip: 2017y-04m-28d-14h-14m-47s UTC integration manifest
aefcd83 drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.
e039db0 drm/i915: Calculate ironlake intermediate watermarks correctly, v2.
157989e drm/i915: Change get_existing_crtc_state to old state
60a0a96 drm/i915: Change use get_new_plane_state instead of existing plane state

== Logs ==

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
> The watermarks it should calculate against are the old optimal watermarks.
> The currently active crtc watermarks are pure fiction, and are invalid in
> case of a nonblocking modeset, page flip enabling/disabling planes or any
> other reason.
> 
> When the crtc is disabled or during a modeset the intermediate watermarks
> don't need to be programmed separately, and could be directly assigned
> to the optimal watermarks.
> 
> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
> 
> Changes since v1:
> - Use intel_atomic_get_old_crtc_state. (ville)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0f344b1fff45..a09396ee1f3d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  
>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>  				       struct intel_crtc *crtc,
> -				       struct intel_crtc_state *crtc_state)
> +				       struct intel_crtc_state *new_crtc_state)
>  {
> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>  	int level;
>  
> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> +		*intermediate = *optimal;
> +
> +		return 0;
> +	}
> +
>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
> -		!crtc_state->disable_cxsr;
> +		!new_crtc_state->disable_cxsr;

We need to consider disable_cxsr even in the modeset case.

>  
>  	for (level = 0; level < intermediate->num_levels; level++) {
>  		enum plane_id plane_id;
> @@ -1491,7 +1499,7 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev,
>  	 * omit the post-vblank programming; only update if it's different.
>  	 */
>  	if (memcmp(intermediate, optimal, sizeof(*intermediate)) != 0)
> -		crtc_state->wm.need_postvbl_update = true;
> +		new_crtc_state->wm.need_postvbl_update = true;
>  
>  	return 0;
>  }
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 v2 1/4] drm/i915: Change use get_new_plane_state instead of existing plane state

[Ville Syrjälä]

On Mon, May 01, 2017 at 03:34:31PM +0200, Maarten Lankhorst wrote:
> The get_existing macros are deprecated and should be replaced by
> get_old/new_state for clarity.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_atomic.c | 4 ++--
>  drivers/gpu/drm/i915/intel_drv.h    | 4 ++--
>  drivers/gpu/drm/i915/intel_pm.c     | 6 ++----
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index d791b3ef89b5..87b1dd464eee 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -301,8 +301,8 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>  				continue;
>  			}
>  
> -			plane_state = intel_atomic_get_existing_plane_state(drm_state,
> -									    intel_plane);
> +			plane_state = intel_atomic_get_new_plane_state(drm_state,
> +								       intel_plane);
>  			scaler_id = &plane_state->scaler_id;
>  		}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 110d9088be35..054b454a8d07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1929,12 +1929,12 @@ intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
>  }
>  
>  static inline struct intel_plane_state *
> -intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
> +intel_atomic_get_new_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);
> +	plane_state = drm_atomic_get_new_plane_state(state, &plane->base);
>  
>  	return to_intel_plane_state(plane_state);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cacb65fa2dd5..246e0723e6e9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2561,8 +2561,7 @@ static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
>  		struct intel_plane_state *ps;
>  
> -		ps = intel_atomic_get_existing_plane_state(state,
> -							   intel_plane);
> +		ps = intel_atomic_get_new_plane_state(state, intel_plane);
>  		if (!ps)
>  			continue;
>  
> @@ -3924,8 +3923,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  
>  	if (state)
>  		intel_pstate =
> -			intel_atomic_get_existing_plane_state(state,
> -							      intel_plane);
> +			intel_atomic_get_new_plane_state(state, intel_plane);
>  
>  	/*
>  	 * Note: If we start supporting multiple pending atomic commits against
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 v2 2/4] drm/i915: Change get_existing_crtc_state to old state

[Ville Syrjälä]

On Mon, May 01, 2017 at 03:34:32PM +0200, Maarten Lankhorst wrote:
> get_existing_crtc_state is currently unused, but the next commit
> needs to access the old intel state. Rename this and use this
> as convenience wrapper.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 054b454a8d07..f69b56d8f7d7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1915,12 +1915,12 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  }
>  
>  static inline struct intel_crtc_state *
> -intel_atomic_get_existing_crtc_state(struct drm_atomic_state *state,
> -				     struct intel_crtc *crtc)
> +intel_atomic_get_old_crtc_state(struct drm_atomic_state *state,
> +				struct intel_crtc *crtc)
>  {
>  	struct drm_crtc_state *crtc_state;
>  
> -	crtc_state = drm_atomic_get_existing_crtc_state(state, &crtc->base);
> +	crtc_state = drm_atomic_get_old_crtc_state(state, &crtc->base);
>  
>  	if (crtc_state)
>  		return to_intel_crtc_state(crtc_state);
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 v2 3/4] drm/i915: Calculate ironlake intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Mon, May 01, 2017 at 03:34:33PM +0200, Maarten Lankhorst wrote:
> The watermarks it should calculate against are the old optimal watermarks.
> The currently active crtc watermarks are pure fiction, and are invalid in
> case of a nonblocking modeset, page flip enabling/disabling planes or any
> other reason.
> 
> When the crtc is disabled or during a modeset the intermediate watermarks
> don't need to be programmed separately, and could be directly assigned
> to the optimal watermarks.
> 
> Changes since v1:
> - Use intel_atomic_get_old_crtc_state. (ville)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 246e0723e6e9..0f344b1fff45 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2638,7 +2638,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  				       struct intel_crtc_state *newstate)
>  {
>  	struct intel_pipe_wm *a = &newstate->wm.ilk.intermediate;
> -	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> +	const struct intel_crtc_state *oldstate =
> +		intel_atomic_get_old_crtc_state(newstate->base.state, intel_crtc);
> +	const struct intel_pipe_wm *b = &oldstate->wm.ilk.optimal;
>  	int level, max_level = ilk_wm_max_level(to_i915(dev));
>  
>  	/*
> @@ -2647,6 +2649,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  	 * and after the vblank.
>  	 */
>  	*a = newstate->wm.ilk.optimal;
> +	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> +		return 0;
> +
>  	a->pipe_enabled |= b->pipe_enabled;
>  	a->sprites_enabled |= b->sprites_enabled;
>  	a->sprites_scaled |= b->sprites_scaled;
> -- 
> 2.9.3
> 
> _______________________________________________
> 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 v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

Op 03-05-17 om 15:45 schreef Ville Syrjälä:
> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
>> The watermarks it should calculate against are the old optimal watermarks.
>> The currently active crtc watermarks are pure fiction, and are invalid in
>> case of a nonblocking modeset, page flip enabling/disabling planes or any
>> other reason.
>>
>> When the crtc is disabled or during a modeset the intermediate watermarks
>> don't need to be programmed separately, and could be directly assigned
>> to the optimal watermarks.
>>
>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
>>
>> Changes since v1:
>> - Use intel_atomic_get_old_crtc_state. (ville)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 0f344b1fff45..a09396ee1f3d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>>  
>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>>  				       struct intel_crtc *crtc,
>> -				       struct intel_crtc_state *crtc_state)
>> +				       struct intel_crtc_state *new_crtc_state)
>>  {
>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
>> +	const struct intel_crtc_state *old_crtc_state =
>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>>  	int level;
>>  
>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>> +		*intermediate = *optimal;
>> +
>> +		return 0;
>> +	}
>> +
>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
>> -		!crtc_state->disable_cxsr;
>> +		!new_crtc_state->disable_cxsr;
> We need to consider disable_cxsr even in the modeset case.
Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
> > On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
> >> The watermarks it should calculate against are the old optimal watermarks.
> >> The currently active crtc watermarks are pure fiction, and are invalid in
> >> case of a nonblocking modeset, page flip enabling/disabling planes or any
> >> other reason.
> >>
> >> When the crtc is disabled or during a modeset the intermediate watermarks
> >> don't need to be programmed separately, and could be directly assigned
> >> to the optimal watermarks.
> >>
> >> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
> >>
> >> Changes since v1:
> >> - Use intel_atomic_get_old_crtc_state. (ville)
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 0f344b1fff45..a09396ee1f3d 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >>  
> >>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
> >>  				       struct intel_crtc *crtc,
> >> -				       struct intel_crtc_state *crtc_state)
> >> +				       struct intel_crtc_state *new_crtc_state)
> >>  {
> >> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
> >> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
> >> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
> >> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
> >> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> >> +	const struct intel_crtc_state *old_crtc_state =
> >> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
> >> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
> >>  	int level;
> >>  
> >> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> >> +		*intermediate = *optimal;
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
> >>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
> >> -		!crtc_state->disable_cxsr;
> >> +		!new_crtc_state->disable_cxsr;
> > We need to consider disable_cxsr even in the modeset case.
> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.

It's set if any plane is enabling/disabling, which should be quite
typical during a modeset.

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

Op 03-05-17 om 16:11 schreef Ville Syrjälä:
> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
>>>> The watermarks it should calculate against are the old optimal watermarks.
>>>> The currently active crtc watermarks are pure fiction, and are invalid in
>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
>>>> other reason.
>>>>
>>>> When the crtc is disabled or during a modeset the intermediate watermarks
>>>> don't need to be programmed separately, and could be directly assigned
>>>> to the optimal watermarks.
>>>>
>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
>>>>
>>>> Changes since v1:
>>>> - Use intel_atomic_get_old_crtc_state. (ville)
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 0f344b1fff45..a09396ee1f3d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>>>>  
>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>>>>  				       struct intel_crtc *crtc,
>>>> -				       struct intel_crtc_state *crtc_state)
>>>> +				       struct intel_crtc_state *new_crtc_state)
>>>>  {
>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
>>>> +	const struct intel_crtc_state *old_crtc_state =
>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>>>>  	int level;
>>>>  
>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>>>> +		*intermediate = *optimal;
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
>>>> -		!crtc_state->disable_cxsr;
>>>> +		!new_crtc_state->disable_cxsr;
>>> We need to consider disable_cxsr even in the modeset case.
>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
> It's set if any plane is enabling/disabling, which should be quite
> typical during a modeset.

Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.

Maybe I'm missing something, but why are the intermediate watermarks and optimize_watermarks needed here?

~Maarten

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
> > On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
> >> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
> >>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
> >>>> The watermarks it should calculate against are the old optimal watermarks.
> >>>> The currently active crtc watermarks are pure fiction, and are invalid in
> >>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
> >>>> other reason.
> >>>>
> >>>> When the crtc is disabled or during a modeset the intermediate watermarks
> >>>> don't need to be programmed separately, and could be directly assigned
> >>>> to the optimal watermarks.
> >>>>
> >>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
> >>>>
> >>>> Changes since v1:
> >>>> - Use intel_atomic_get_old_crtc_state. (ville)
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
> >>>>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>> index 0f344b1fff45..a09396ee1f3d 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >>>>  
> >>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
> >>>>  				       struct intel_crtc *crtc,
> >>>> -				       struct intel_crtc_state *crtc_state)
> >>>> +				       struct intel_crtc_state *new_crtc_state)
> >>>>  {
> >>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
> >>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
> >>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
> >>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
> >>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> >>>> +	const struct intel_crtc_state *old_crtc_state =
> >>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
> >>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
> >>>>  	int level;
> >>>>  
> >>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> >>>> +		*intermediate = *optimal;
> >>>> +
> >>>> +		return 0;
> >>>> +	}
> >>>> +
> >>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
> >>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
> >>>> -		!crtc_state->disable_cxsr;
> >>>> +		!new_crtc_state->disable_cxsr;
> >>> We need to consider disable_cxsr even in the modeset case.
> >> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
> > It's set if any plane is enabling/disabling, which should be quite
> > typical during a modeset.
> 
> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.

Which is not what we want. CxSR must stay off until the planes have been
enabled.

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

Op 03-05-17 om 18:07 schreef Ville Syrjälä:
> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
>> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
>>>>>> The watermarks it should calculate against are the old optimal watermarks.
>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in
>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
>>>>>> other reason.
>>>>>>
>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks
>>>>>> don't need to be programmed separately, and could be directly assigned
>>>>>> to the optimal watermarks.
>>>>>>
>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
>>>>>>
>>>>>> Changes since v1:
>>>>>> - Use intel_atomic_get_old_crtc_state. (ville)
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>>>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> index 0f344b1fff45..a09396ee1f3d 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>>>>>>  
>>>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>>>>>>  				       struct intel_crtc *crtc,
>>>>>> -				       struct intel_crtc_state *crtc_state)
>>>>>> +				       struct intel_crtc_state *new_crtc_state)
>>>>>>  {
>>>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
>>>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
>>>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
>>>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>>>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
>>>>>> +	const struct intel_crtc_state *old_crtc_state =
>>>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
>>>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>>>>>>  	int level;
>>>>>>  
>>>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>>>>>> +		*intermediate = *optimal;
>>>>>> +
>>>>>> +		return 0;
>>>>>> +	}
>>>>>> +
>>>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>>>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
>>>>>> -		!crtc_state->disable_cxsr;
>>>>>> +		!new_crtc_state->disable_cxsr;
>>>>> We need to consider disable_cxsr even in the modeset case.
>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
>>> It's set if any plane is enabling/disabling, which should be quite
>>> typical during a modeset.
>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.
> Which is not what we want. CxSR must stay off until the planes have been
> enabled.
>
In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote:
> Op 03-05-17 om 18:07 schreef Ville Syrjälä:
> > On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
> >> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
> >>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
> >>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
> >>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
> >>>>>> The watermarks it should calculate against are the old optimal watermarks.
> >>>>>> The currently active crtc watermarks are pure fiction, and are invalid in
> >>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
> >>>>>> other reason.
> >>>>>>
> >>>>>> When the crtc is disabled or during a modeset the intermediate watermarks
> >>>>>> don't need to be programmed separately, and could be directly assigned
> >>>>>> to the optimal watermarks.
> >>>>>>
> >>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
> >>>>>>
> >>>>>> Changes since v1:
> >>>>>> - Use intel_atomic_get_old_crtc_state. (ville)
> >>>>>>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
> >>>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> index 0f344b1fff45..a09396ee1f3d 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >>>>>>  
> >>>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
> >>>>>>  				       struct intel_crtc *crtc,
> >>>>>> -				       struct intel_crtc_state *crtc_state)
> >>>>>> +				       struct intel_crtc_state *new_crtc_state)
> >>>>>>  {
> >>>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
> >>>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
> >>>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
> >>>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
> >>>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> >>>>>> +	const struct intel_crtc_state *old_crtc_state =
> >>>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
> >>>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
> >>>>>>  	int level;
> >>>>>>  
> >>>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> >>>>>> +		*intermediate = *optimal;
> >>>>>> +
> >>>>>> +		return 0;
> >>>>>> +	}
> >>>>>> +
> >>>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
> >>>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
> >>>>>> -		!crtc_state->disable_cxsr;
> >>>>>> +		!new_crtc_state->disable_cxsr;
> >>>>> We need to consider disable_cxsr even in the modeset case.
> >>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
> >>> It's set if any plane is enabling/disabling, which should be quite
> >>> typical during a modeset.
> >> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.
> > Which is not what we want. CxSR must stay off until the planes have been
> > enabled.
> >
> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..

Because we can keep it enabled across the update unless planes are
getting enabled or disabled.

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

Op 03-05-17 om 20:03 schreef Ville Syrjälä:
> On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote:
>> Op 03-05-17 om 18:07 schreef Ville Syrjälä:
>>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
>>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
>>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
>>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
>>>>>>>> The watermarks it should calculate against are the old optimal watermarks.
>>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in
>>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
>>>>>>>> other reason.
>>>>>>>>
>>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks
>>>>>>>> don't need to be programmed separately, and could be directly assigned
>>>>>>>> to the optimal watermarks.
>>>>>>>>
>>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
>>>>>>>>
>>>>>>>> Changes since v1:
>>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville)
>>>>>>>>
>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>>>>>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>>>>>>>>  
>>>>>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>>>>>>>>  				       struct intel_crtc *crtc,
>>>>>>>> -				       struct intel_crtc_state *crtc_state)
>>>>>>>> +				       struct intel_crtc_state *new_crtc_state)
>>>>>>>>  {
>>>>>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
>>>>>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
>>>>>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
>>>>>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>>>>>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
>>>>>>>> +	const struct intel_crtc_state *old_crtc_state =
>>>>>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
>>>>>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>>>>>>>>  	int level;
>>>>>>>>  
>>>>>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>>>>>>>> +		*intermediate = *optimal;
>>>>>>>> +
>>>>>>>> +		return 0;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>>>>>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
>>>>>>>> -		!crtc_state->disable_cxsr;
>>>>>>>> +		!new_crtc_state->disable_cxsr;
>>>>>>> We need to consider disable_cxsr even in the modeset case.
>>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
>>>>> It's set if any plane is enabling/disabling, which should be quite
>>>>> typical during a modeset.
>>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.
>>> Which is not what we want. CxSR must stay off until the planes have been
>>> enabled.
>>>
>> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..
> Because we can keep it enabled across the update unless planes are
> getting enabled or disabled.
In that case the watermarks still need some fixing to handle this correctly, and optimize watermarks should be called on modeset too.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Maarten Lankhorst]

Op 03-05-17 om 20:03 schreef Ville Syrjälä:
> On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote:
>> Op 03-05-17 om 18:07 schreef Ville Syrjälä:
>>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
>>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
>>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
>>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
>>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
>>>>>>>> The watermarks it should calculate against are the old optimal watermarks.
>>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in
>>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
>>>>>>>> other reason.
>>>>>>>>
>>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks
>>>>>>>> don't need to be programmed separately, and could be directly assigned
>>>>>>>> to the optimal watermarks.
>>>>>>>>
>>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
>>>>>>>>
>>>>>>>> Changes since v1:
>>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville)
>>>>>>>>
>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
>>>>>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>>>>>>>>  
>>>>>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
>>>>>>>>  				       struct intel_crtc *crtc,
>>>>>>>> -				       struct intel_crtc_state *crtc_state)
>>>>>>>> +				       struct intel_crtc_state *new_crtc_state)
>>>>>>>>  {
>>>>>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
>>>>>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
>>>>>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
>>>>>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
>>>>>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
>>>>>>>> +	const struct intel_crtc_state *old_crtc_state =
>>>>>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
>>>>>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
>>>>>>>>  	int level;
>>>>>>>>  
>>>>>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
>>>>>>>> +		*intermediate = *optimal;
>>>>>>>> +
>>>>>>>> +		return 0;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
>>>>>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
>>>>>>>> -		!crtc_state->disable_cxsr;
>>>>>>>> +		!new_crtc_state->disable_cxsr;
>>>>>>> We need to consider disable_cxsr even in the modeset case.
>>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
>>>>> It's set if any plane is enabling/disabling, which should be quite
>>>>> typical during a modeset.
>>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.
>>> Which is not what we want. CxSR must stay off until the planes have been
>>> enabled.
>>>
>> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..
> Because we can keep it enabled across the update unless planes are
> getting enabled or disabled.
>
So for the modeset case, computing intermediate watermarks:

*intermediate = *optimal;
if (needs_modeset)
	intermediate->cxsr = false;

if (optimal->cxsr && !intermediate->cxsr)
	new_crtc_state->wm.need_postvbl_update = true;

?

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

Re: [PATCH v2 4/4] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v2.

[Ville Syrjälä]

On Thu, May 04, 2017 at 10:12:52AM +0200, Maarten Lankhorst wrote:
> Op 03-05-17 om 20:03 schreef Ville Syrjälä:
> > On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote:
> >> Op 03-05-17 om 18:07 schreef Ville Syrjälä:
> >>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote:
> >>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä:
> >>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote:
> >>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä:
> >>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote:
> >>>>>>>> The watermarks it should calculate against are the old optimal watermarks.
> >>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in
> >>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any
> >>>>>>>> other reason.
> >>>>>>>>
> >>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks
> >>>>>>>> don't need to be programmed separately, and could be directly assigned
> >>>>>>>> to the optimal watermarks.
> >>>>>>>>
> >>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state.
> >>>>>>>>
> >>>>>>>> Changes since v1:
> >>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville)
> >>>>>>>>
> >>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------
> >>>>>>>>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644
> >>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
> >>>>>>>>  
> >>>>>>>>  static int vlv_compute_intermediate_wm(struct drm_device *dev,
> >>>>>>>>  				       struct intel_crtc *crtc,
> >>>>>>>> -				       struct intel_crtc_state *crtc_state)
> >>>>>>>> +				       struct intel_crtc_state *new_crtc_state)
> >>>>>>>>  {
> >>>>>>>> -	struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate;
> >>>>>>>> -	const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal;
> >>>>>>>> -	const struct vlv_wm_state *active = &crtc->wm.active.vlv;
> >>>>>>>> +	struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate;
> >>>>>>>> +	const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal;
> >>>>>>>> +	const struct intel_crtc_state *old_crtc_state =
> >>>>>>>> +		intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc);
> >>>>>>>> +	const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal;
> >>>>>>>>  	int level;
> >>>>>>>>  
> >>>>>>>> +	if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) {
> >>>>>>>> +		*intermediate = *optimal;
> >>>>>>>> +
> >>>>>>>> +		return 0;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>>  	intermediate->num_levels = min(optimal->num_levels, active->num_levels);
> >>>>>>>>  	intermediate->cxsr = optimal->cxsr && active->cxsr &&
> >>>>>>>> -		!crtc_state->disable_cxsr;
> >>>>>>>> +		!new_crtc_state->disable_cxsr;
> >>>>>>> We need to consider disable_cxsr even in the modeset case.
> >>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already.
> >>>>> It's set if any plane is enabling/disabling, which should be quite
> >>>>> typical during a modeset.
> >>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway.
> >>> Which is not what we want. CxSR must stay off until the planes have been
> >>> enabled.
> >>>
> >> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..
> > Because we can keep it enabled across the update unless planes are
> > getting enabled or disabled.
> >
> So for the modeset case, computing intermediate watermarks:
> 
> *intermediate = *optimal;
> if (needs_modeset)
> 	intermediate->cxsr = false;
> 
> if (optimal->cxsr && !intermediate->cxsr)
> 	new_crtc_state->wm.need_postvbl_update = true;
> 
> ?


Or maybe

	if (blah) {
		*intermediate = *optimal;
		goto out;
	}

	// min/max stuff

out:
	if (disable_cxsr)
		intermediate->cxsr = false;
	if (memcmp(...


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