[Intel-gfx] [PATCH 1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

[Intel-gfx] [PATCH 1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Dbuf slice tracking busted across runtime PM. Back to the
drawing board.

This reverts commit 70b1a26f299c729cc1a5099374cc02568b05ec7d.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a21e36ed1a77..d40d22eb65da 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4105,6 +4105,10 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	 */
 	dbuf_slice_mask = skl_compute_dbuf_slices(crtc_state, active_pipes);
 
+	DRM_DEBUG_KMS("DBuf slice mask %x pipe %c active pipes %x\n",
+		      dbuf_slice_mask,
+		      pipe_name(for_pipe), active_pipes);
+
 	/*
 	 * Figure out at which DBuf slice we start, i.e if we start at Dbuf S2
 	 * and slice size is 1024, the offset would be 1024
@@ -4187,10 +4191,8 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	alloc->start = offset + start;
 	alloc->end = offset + end;
 
-	drm_dbg_kms(&dev_priv->drm,
-		    "[CRTC:%d:%s] dbuf slices 0x%x, ddb (%d - %d), active pipes 0x%x\n",
-		    for_crtc->base.id, for_crtc->name,
-		    dbuf_slice_mask, alloc->start, alloc->end, active_pipes);
+	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
+		      alloc->start, alloc->end);
 
 	return 0;
 }
@@ -5704,10 +5706,7 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
 static int
 skl_compute_ddb(struct intel_atomic_state *state)
 {
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_dbuf_state *old_dbuf_state;
-	const struct intel_dbuf_state *new_dbuf_state;
-	const struct intel_crtc_state *old_crtc_state;
+	struct intel_crtc_state *old_crtc_state;
 	struct intel_crtc_state *new_crtc_state;
 	struct intel_crtc *crtc;
 	int ret, i;
@@ -5724,17 +5723,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
 			return ret;
 	}
 
-	old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
-	new_dbuf_state = intel_atomic_get_new_dbuf_state(state);
-
-	if (new_dbuf_state &&
-	    new_dbuf_state->enabled_slices != old_dbuf_state->enabled_slices)
-		drm_dbg_kms(&dev_priv->drm,
-			    "Enabled dbuf slices 0x%x -> 0x%x (out of %d dbuf slices)\n",
-			    old_dbuf_state->enabled_slices,
-			    new_dbuf_state->enabled_slices,
-			    INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
-
 	return 0;
 }
 
-- 
2.26.2

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

[Intel-gfx] [PATCH 2/4] Revert "drm/i915: Move the dbuf pre/post plane update"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Dbuf slice tracking busted across runtime PM. Back to the
drawing board.

This reverts commit c7c0e7ebe4d9963573f81399374e4e95f37fd8e3.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 41 +++++++++++++++++++-
 drivers/gpu/drm/i915/intel_pm.c              | 37 ------------------
 drivers/gpu/drm/i915/intel_pm.h              |  2 -
 3 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e1407dc28ddc..49577f19ff9c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15207,6 +15207,43 @@ static void intel_commit_modeset_enables(struct intel_atomic_state *state)
 	}
 }
 
+static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_dbuf_state *new_dbuf_state =
+		intel_atomic_get_new_dbuf_state(state);
+	const struct intel_dbuf_state *old_dbuf_state =
+		intel_atomic_get_old_dbuf_state(state);
+
+	if (!new_dbuf_state ||
+	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
+		return;
+
+	WARN_ON(!new_dbuf_state->base.changed);
+
+	gen9_dbuf_slices_update(dev_priv,
+				old_dbuf_state->enabled_slices |
+				new_dbuf_state->enabled_slices);
+}
+
+static void icl_dbuf_slice_post_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_dbuf_state *new_dbuf_state =
+		intel_atomic_get_new_dbuf_state(state);
+	const struct intel_dbuf_state *old_dbuf_state =
+		intel_atomic_get_old_dbuf_state(state);
+
+	if (!new_dbuf_state ||
+	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
+		return;
+
+	WARN_ON(!new_dbuf_state->base.changed);
+
+	gen9_dbuf_slices_update(dev_priv,
+				new_dbuf_state->enabled_slices);
+}
+
 static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
@@ -15447,7 +15484,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	if (state->modeset)
 		intel_encoders_update_prepare(state);
 
-	intel_dbuf_pre_plane_update(state);
+	icl_dbuf_slice_pre_update(state);
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.commit_modeset_enables(state);
@@ -15502,7 +15539,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 			dev_priv->display.optimize_watermarks(state, crtc);
 	}
 
-	intel_dbuf_post_plane_update(state);
+	icl_dbuf_slice_post_update(state);
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		intel_post_plane_update(state, crtc);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d40d22eb65da..a92d57d9b759 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7806,40 +7806,3 @@ int intel_dbuf_init(struct drm_i915_private *dev_priv)
 
 	return 0;
 }
-
-void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_dbuf_state *new_dbuf_state =
-		intel_atomic_get_new_dbuf_state(state);
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(state);
-
-	if (!new_dbuf_state ||
-	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
-		return;
-
-	WARN_ON(!new_dbuf_state->base.changed);
-
-	gen9_dbuf_slices_update(dev_priv,
-				old_dbuf_state->enabled_slices |
-				new_dbuf_state->enabled_slices);
-}
-
-void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_dbuf_state *new_dbuf_state =
-		intel_atomic_get_new_dbuf_state(state);
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(state);
-
-	if (!new_dbuf_state ||
-	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
-		return;
-
-	WARN_ON(!new_dbuf_state->base.changed);
-
-	gen9_dbuf_slices_update(dev_priv,
-				new_dbuf_state->enabled_slices);
-}
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 6636d2a057cd..3fcc9b6e2cbf 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -83,7 +83,5 @@ intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
 	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
 
 int intel_dbuf_init(struct drm_i915_private *dev_priv);
-void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
-void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
 
 #endif /* __INTEL_PM_H__ */
-- 
2.26.2

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

Re: [Intel-gfx] [PATCH 2/4] Revert "drm/i915: Move the dbuf pre/post plane update"

[Lisovskiy, Stanislav]

On Mon, May 18, 2020 at 03:23:01PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Dbuf slice tracking busted across runtime PM. Back to the
> drawing board.
> 
> This reverts commit c7c0e7ebe4d9963573f81399374e4e95f37fd8e3.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 41 +++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_pm.c              | 37 ------------------
>  drivers/gpu/drm/i915/intel_pm.h              |  2 -
>  3 files changed, 39 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e1407dc28ddc..49577f19ff9c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15207,6 +15207,43 @@ static void intel_commit_modeset_enables(struct intel_atomic_state *state)
>  	}
>  }
>  

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> +static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	const struct intel_dbuf_state *new_dbuf_state =
> +		intel_atomic_get_new_dbuf_state(state);
> +	const struct intel_dbuf_state *old_dbuf_state =
> +		intel_atomic_get_old_dbuf_state(state);
> +
> +	if (!new_dbuf_state ||
> +	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> +		return;
> +
> +	WARN_ON(!new_dbuf_state->base.changed);
> +
> +	gen9_dbuf_slices_update(dev_priv,
> +				old_dbuf_state->enabled_slices |
> +				new_dbuf_state->enabled_slices);
> +}
> +
> +static void icl_dbuf_slice_post_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	const struct intel_dbuf_state *new_dbuf_state =
> +		intel_atomic_get_new_dbuf_state(state);
> +	const struct intel_dbuf_state *old_dbuf_state =
> +		intel_atomic_get_old_dbuf_state(state);
> +
> +	if (!new_dbuf_state ||
> +	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> +		return;
> +
> +	WARN_ON(!new_dbuf_state->base.changed);
> +
> +	gen9_dbuf_slices_update(dev_priv,
> +				new_dbuf_state->enabled_slices);
> +}
> +
>  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> @@ -15447,7 +15484,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	if (state->modeset)
>  		intel_encoders_update_prepare(state);
>  
> -	intel_dbuf_pre_plane_update(state);
> +	icl_dbuf_slice_pre_update(state);
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.commit_modeset_enables(state);
> @@ -15502,7 +15539,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  			dev_priv->display.optimize_watermarks(state, crtc);
>  	}
>  
> -	intel_dbuf_post_plane_update(state);
> +	icl_dbuf_slice_post_update(state);
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		intel_post_plane_update(state, crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d40d22eb65da..a92d57d9b759 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7806,40 +7806,3 @@ int intel_dbuf_init(struct drm_i915_private *dev_priv)
>  
>  	return 0;
>  }
> -
> -void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_dbuf_state *new_dbuf_state =
> -		intel_atomic_get_new_dbuf_state(state);
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(state);
> -
> -	if (!new_dbuf_state ||
> -	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> -		return;
> -
> -	WARN_ON(!new_dbuf_state->base.changed);
> -
> -	gen9_dbuf_slices_update(dev_priv,
> -				old_dbuf_state->enabled_slices |
> -				new_dbuf_state->enabled_slices);
> -}
> -
> -void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_dbuf_state *new_dbuf_state =
> -		intel_atomic_get_new_dbuf_state(state);
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(state);
> -
> -	if (!new_dbuf_state ||
> -	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> -		return;
> -
> -	WARN_ON(!new_dbuf_state->base.changed);
> -
> -	gen9_dbuf_slices_update(dev_priv,
> -				new_dbuf_state->enabled_slices);
> -}
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 6636d2a057cd..3fcc9b6e2cbf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -83,7 +83,5 @@ intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
>  	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
>  
>  int intel_dbuf_init(struct drm_i915_private *dev_priv);
> -void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
> -void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
>  
>  #endif /* __INTEL_PM_H__ */
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/4] Revert "drm/i915: Nuke skl_ddb_get_hw_state()"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Dbuf slice tracking busted across runtime PM. Back to the
drawing board.

This reverts commit 0cde0e0ff5f5ebd27507069250728c763c14ac81.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
 drivers/gpu/drm/i915/intel_pm.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a92d57d9b759..cb57786fdc9f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4315,6 +4315,12 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
 	intel_display_power_put(dev_priv, power_domain, wakeref);
 }
 
+void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv)
+{
+	dev_priv->dbuf.enabled_slices =
+		intel_enabled_dbuf_slices_mask(dev_priv);
+}
+
 /*
  * Determines the downscale amount of a plane for the purposes of watermark calculations.
  * The bspec defines downscale amount as:
@@ -6175,6 +6181,7 @@ void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
 	struct intel_crtc *crtc;
 	struct intel_crtc_state *crtc_state;
 
+	skl_ddb_get_hw_state(dev_priv);
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		crtc_state = to_intel_crtc_state(crtc->base.state);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 3fcc9b6e2cbf..9f75ac4c2bd1 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -39,6 +39,7 @@ u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *dev_priv);
 void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
 			       struct skl_ddb_entry *ddb_y,
 			       struct skl_ddb_entry *ddb_uv);
+void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
 void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
 			      struct skl_pipe_wm *out);
 void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
-- 
2.26.2

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

Re: [Intel-gfx] [PATCH 3/4] Revert "drm/i915: Nuke skl_ddb_get_hw_state()"

[Lisovskiy, Stanislav]

On Mon, May 18, 2020 at 03:23:02PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Dbuf slice tracking busted across runtime PM. Back to the
> drawing board.
> 
> This reverts commit 0cde0e0ff5f5ebd27507069250728c763c14ac81.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
>  drivers/gpu/drm/i915/intel_pm.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a92d57d9b759..cb57786fdc9f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4315,6 +4315,12 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
>  	intel_display_power_put(dev_priv, power_domain, wakeref);
>  }
>  
> +void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv)
> +{
> +	dev_priv->dbuf.enabled_slices =
> +		intel_enabled_dbuf_slices_mask(dev_priv);
> +}
> +

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  /*
>   * Determines the downscale amount of a plane for the purposes of watermark calculations.
>   * The bspec defines downscale amount as:
> @@ -6175,6 +6181,7 @@ void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
>  	struct intel_crtc *crtc;
>  	struct intel_crtc_state *crtc_state;
>  
> +	skl_ddb_get_hw_state(dev_priv);
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		crtc_state = to_intel_crtc_state(crtc->base.state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 3fcc9b6e2cbf..9f75ac4c2bd1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -39,6 +39,7 @@ u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *dev_priv);
>  void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
>  			       struct skl_ddb_entry *ddb_y,
>  			       struct skl_ddb_entry *ddb_uv);
> +void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
>  void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
>  			      struct skl_pipe_wm *out);
>  void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/4] Revert "drm/i915: Introduce proper dbuf state"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Dbuf slice tracking busted across runtime PM. Back to the
drawing board.

This reverts commit 3cf43cdc63fbc3df19ea8398e9b8717ab44a6304.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  |  67 ++-----
 .../drm/i915/display/intel_display_power.c    |   8 +-
 .../drm/i915/display/intel_display_types.h    |  13 ++
 drivers/gpu/drm/i915/i915_drv.h               |  11 +-
 drivers/gpu/drm/i915/intel_pm.c               | 185 ++++++------------
 drivers/gpu/drm/i915/intel_pm.h               |  22 ---
 6 files changed, 99 insertions(+), 207 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 49577f19ff9c..d6635d649dc8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7579,8 +7579,6 @@ static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
 		to_intel_bw_state(dev_priv->bw_obj.state);
 	struct intel_cdclk_state *cdclk_state =
 		to_intel_cdclk_state(dev_priv->cdclk.obj.state);
-	struct intel_dbuf_state *dbuf_state =
-		to_intel_dbuf_state(dev_priv->dbuf.obj.state);
 	struct intel_crtc_state *crtc_state =
 		to_intel_crtc_state(crtc->base.state);
 	enum intel_display_power_domain domain;
@@ -7654,8 +7652,6 @@ static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
 	cdclk_state->min_voltage_level[pipe] = 0;
 	cdclk_state->active_pipes &= ~BIT(pipe);
 
-	dbuf_state->active_pipes &= ~BIT(pipe);
-
 	bw_state->data_rate[pipe] = 0;
 	bw_state->num_active_planes[pipe] = 0;
 }
@@ -14012,10 +14008,10 @@ static void verify_wm_state(struct intel_crtc *crtc,
 	hw_enabled_slices = intel_enabled_dbuf_slices_mask(dev_priv);
 
 	if (INTEL_GEN(dev_priv) >= 11 &&
-	    hw_enabled_slices != dev_priv->dbuf.enabled_slices)
+	    hw_enabled_slices != dev_priv->enabled_dbuf_slices_mask)
 		drm_err(&dev_priv->drm,
 			"mismatch in DBUF Slices (expected 0x%x, got 0x%x)\n",
-			dev_priv->dbuf.enabled_slices,
+			dev_priv->enabled_dbuf_slices_mask,
 			hw_enabled_slices);
 
 	/* planes */
@@ -14556,7 +14552,9 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
 	state->modeset = true;
 	state->active_pipes = intel_calc_active_pipes(state, dev_priv->active_pipes);
 
-	if (state->active_pipes != dev_priv->active_pipes) {
+	state->active_pipe_changes = state->active_pipes ^ dev_priv->active_pipes;
+
+	if (state->active_pipe_changes) {
 		ret = _intel_atomic_lock_global_state(state);
 		if (ret)
 			return ret;
@@ -15210,38 +15208,22 @@ static void intel_commit_modeset_enables(struct intel_atomic_state *state)
 static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_dbuf_state *new_dbuf_state =
-		intel_atomic_get_new_dbuf_state(state);
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(state);
-
-	if (!new_dbuf_state ||
-	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
-		return;
+	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_mask;
+	u8 required_slices = state->enabled_dbuf_slices_mask;
+	u8 slices_union = hw_enabled_slices | required_slices;
 
-	WARN_ON(!new_dbuf_state->base.changed);
-
-	gen9_dbuf_slices_update(dev_priv,
-				old_dbuf_state->enabled_slices |
-				new_dbuf_state->enabled_slices);
+	if (INTEL_GEN(dev_priv) >= 11 && slices_union != hw_enabled_slices)
+		gen9_dbuf_slices_update(dev_priv, slices_union);
 }
 
 static void icl_dbuf_slice_post_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_dbuf_state *new_dbuf_state =
-		intel_atomic_get_new_dbuf_state(state);
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(state);
-
-	if (!new_dbuf_state ||
-	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
-		return;
+	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_mask;
+	u8 required_slices = state->enabled_dbuf_slices_mask;
 
-	WARN_ON(!new_dbuf_state->base.changed);
-
-	gen9_dbuf_slices_update(dev_priv,
-				new_dbuf_state->enabled_slices);
+	if (INTEL_GEN(dev_priv) >= 11 && required_slices != hw_enabled_slices)
+		gen9_dbuf_slices_update(dev_priv, required_slices);
 }
 
 static void skl_commit_modeset_enables(struct intel_atomic_state *state)
@@ -15484,7 +15466,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	if (state->modeset)
 		intel_encoders_update_prepare(state);
 
-	icl_dbuf_slice_pre_update(state);
+	/* Enable all new slices, we might need */
+	if (state->modeset)
+		icl_dbuf_slice_pre_update(state);
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.commit_modeset_enables(state);
@@ -15539,7 +15523,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 			dev_priv->display.optimize_watermarks(state, crtc);
 	}
 
-	icl_dbuf_slice_post_update(state);
+	/* Disable all slices, we don't need */
+	if (state->modeset)
+		icl_dbuf_slice_post_update(state);
 
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		intel_post_plane_update(state, crtc);
@@ -17433,14 +17419,10 @@ void intel_modeset_init_hw(struct drm_i915_private *i915)
 {
 	struct intel_cdclk_state *cdclk_state =
 		to_intel_cdclk_state(i915->cdclk.obj.state);
-	struct intel_dbuf_state *dbuf_state =
-		to_intel_dbuf_state(i915->dbuf.obj.state);
 
 	intel_update_cdclk(i915);
 	intel_dump_cdclk_config(&i915->cdclk.hw, "Current CDCLK");
 	cdclk_state->logical = cdclk_state->actual = i915->cdclk.hw;
-
-	dbuf_state->enabled_slices = i915->dbuf.enabled_slices;
 }
 
 static int sanitize_watermarks_add_affected(struct drm_atomic_state *state)
@@ -17729,10 +17711,6 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
 	if (ret)
 		return ret;
 
-	ret = intel_dbuf_init(i915);
-	if (ret)
-		return ret;
-
 	ret = intel_bw_init(i915);
 	if (ret)
 		return ret;
@@ -18249,8 +18227,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_cdclk_state *cdclk_state =
 		to_intel_cdclk_state(dev_priv->cdclk.obj.state);
-	struct intel_dbuf_state *dbuf_state =
-		to_intel_dbuf_state(dev_priv->dbuf.obj.state);
 	enum pipe pipe;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
@@ -18281,8 +18257,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			    enableddisabled(crtc_state->hw.active));
 	}
 
-	dev_priv->active_pipes = cdclk_state->active_pipes =
-		dbuf_state->active_pipes = active_pipes;
+	dev_priv->active_pipes = cdclk_state->active_pipes = active_pipes;
 
 	readout_plane_state(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 7a8213993110..a3e581947bec 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -1161,7 +1161,7 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
 static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
 {
 	u8 hw_enabled_dbuf_slices = intel_enabled_dbuf_slices_mask(dev_priv);
-	u8 enabled_dbuf_slices = dev_priv->dbuf.enabled_slices;
+	u8 enabled_dbuf_slices = dev_priv->enabled_dbuf_slices_mask;
 
 	drm_WARN(&dev_priv->drm,
 		 hw_enabled_dbuf_slices != enabled_dbuf_slices,
@@ -4539,14 +4539,14 @@ void gen9_dbuf_slices_update(struct drm_i915_private *dev_priv,
 	for (slice = DBUF_S1; slice < num_slices; slice++)
 		gen9_dbuf_slice_set(dev_priv, slice, req_slices & BIT(slice));
 
-	dev_priv->dbuf.enabled_slices = req_slices;
+	dev_priv->enabled_dbuf_slices_mask = req_slices;
 
 	mutex_unlock(&power_domains->lock);
 }
 
 static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
 {
-	dev_priv->dbuf.enabled_slices =
+	dev_priv->enabled_dbuf_slices_mask =
 		intel_enabled_dbuf_slices_mask(dev_priv);
 
 	/*
@@ -4554,7 +4554,7 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
 	 * figure out later which slices we have and what we need.
 	 */
 	gen9_dbuf_slices_update(dev_priv, BIT(DBUF_S1) |
-				dev_priv->dbuf.enabled_slices);
+				dev_priv->enabled_dbuf_slices_mask);
 }
 
 static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 83e2f0bd2387..2bf3d4cb4ea9 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -479,6 +479,16 @@ struct intel_atomic_state {
 
 	bool dpll_set, modeset;
 
+	/*
+	 * Does this transaction change the pipes that are active?  This mask
+	 * tracks which CRTC's have changed their active state at the end of
+	 * the transaction (not counting the temporary disable during modesets).
+	 * This mask should only be non-zero when intel_state->modeset is true,
+	 * but the converse is not necessarily true; simply changing a mode may
+	 * not flip the final active status of any CRTC's
+	 */
+	u8 active_pipe_changes;
+
 	u8 active_pipes;
 
 	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
@@ -496,6 +506,9 @@ struct intel_atomic_state {
 	 */
 	bool global_state_changed;
 
+	/* Number of enabled DBuf slices */
+	u8 enabled_dbuf_slices_mask;
+
 	struct i915_sw_fence commit_ready;
 
 	struct llist_node freed;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 64798b398482..adb9bf34cf97 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -950,13 +950,6 @@ struct drm_i915_private {
 		struct intel_global_obj obj;
 	} cdclk;
 
-	struct {
-		/* The current hardware dbuf configuration */
-		u8 enabled_slices;
-
-		struct intel_global_obj obj;
-	} dbuf;
-
 	/**
 	 * wq - Driver workqueue for GEM.
 	 *
@@ -1133,12 +1126,12 @@ struct drm_i915_private {
 		 * Set during HW readout of watermarks/DDB.  Some platforms
 		 * need to know when we're still using BIOS-provided values
 		 * (which we don't fully trust).
-		 *
-		 * FIXME get rid of this.
 		 */
 		bool distrust_bios_wm;
 	} wm;
 
+	u8 enabled_dbuf_slices_mask; /* GEN11 has configurable 2 slices */
+
 	struct dram_info {
 		bool valid;
 		bool is_16gb_dimm;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cb57786fdc9f..5c47b893e7b2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4040,7 +4040,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
 static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
 				  u8 active_pipes);
 
-static int
+static void
 skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 				   const struct intel_crtc_state *crtc_state,
 				   const u64 total_data_rate,
@@ -4053,29 +4053,30 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	const struct intel_crtc *crtc;
 	u32 pipe_width = 0, total_width_in_range = 0, width_before_pipe_in_range = 0;
 	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
-	struct intel_dbuf_state *new_dbuf_state =
-		intel_atomic_get_new_dbuf_state(intel_state);
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(intel_state);
-	u8 active_pipes = new_dbuf_state->active_pipes;
 	u16 ddb_size;
 	u32 ddb_range_size;
 	u32 i;
 	u32 dbuf_slice_mask;
+	u32 active_pipes;
 	u32 offset;
 	u32 slice_size;
 	u32 total_slice_mask;
 	u32 start, end;
-	int ret;
-
-	*num_active = hweight8(active_pipes);
 
-	if (!crtc_state->hw.active) {
+	if (drm_WARN_ON(&dev_priv->drm, !state) || !crtc_state->hw.active) {
 		alloc->start = 0;
 		alloc->end = 0;
-		return 0;
+		*num_active = hweight8(dev_priv->active_pipes);
+		return;
 	}
 
+	if (intel_state->active_pipe_changes)
+		active_pipes = intel_state->active_pipes;
+	else
+		active_pipes = dev_priv->active_pipes;
+
+	*num_active = hweight8(active_pipes);
+
 	ddb_size = intel_get_ddb_size(dev_priv);
 
 	slice_size = ddb_size / INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
@@ -4088,16 +4089,13 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	 * that changes the active CRTC list or do modeset would need to
 	 * grab _all_ crtc locks, including the one we currently hold.
 	 */
-	if (old_dbuf_state->active_pipes == new_dbuf_state->active_pipes &&
-	    !dev_priv->wm.distrust_bios_wm) {
+	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
 		/*
 		 * alloc may be cleared by clear_intel_crtc_state,
 		 * copy from old state to be sure
-		 *
-		 * FIXME get rid of this mess
 		 */
 		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
-		return 0;
+		return;
 	}
 
 	/*
@@ -4176,13 +4174,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	 * FIXME: For now we always enable slice S1 as per
 	 * the Bspec display initialization sequence.
 	 */
-	new_dbuf_state->enabled_slices = total_slice_mask | BIT(DBUF_S1);
-
-	if (old_dbuf_state->enabled_slices != new_dbuf_state->enabled_slices) {
-		ret = intel_atomic_serialize_global_state(&new_dbuf_state->base);
-		if (ret)
-			return ret;
-	}
+	intel_state->enabled_dbuf_slices_mask = total_slice_mask | BIT(DBUF_S1);
 
 	start = ddb_range_size * width_before_pipe_in_range / total_width_in_range;
 	end = ddb_range_size *
@@ -4193,8 +4185,9 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 
 	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
 		      alloc->start, alloc->end);
-
-	return 0;
+	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
+		      intel_state->enabled_dbuf_slices_mask,
+		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
 }
 
 static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
@@ -4317,8 +4310,8 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
 
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv)
 {
-	dev_priv->dbuf.enabled_slices =
-		intel_enabled_dbuf_slices_mask(dev_priv);
+	dev_priv->enabled_dbuf_slices_mask =
+				intel_enabled_dbuf_slices_mask(dev_priv);
 }
 
 /*
@@ -4765,7 +4758,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
 	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
 	u32 blocks;
 	int level;
-	int ret;
 
 	/* Clear the partitioning for disabled planes. */
 	memset(crtc_state->wm.skl.plane_ddb_y, 0, sizeof(crtc_state->wm.skl.plane_ddb_y));
@@ -4786,12 +4778,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
 							 plane_data_rate,
 							 uv_plane_data_rate);
 
-	ret = skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state,
-						 total_data_rate,
-						 alloc, &num_active);
-	if (ret)
-		return ret;
-
+	skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate,
+					   alloc, &num_active);
 	alloc_size = skl_ddb_entry_size(alloc);
 	if (alloc_size == 0)
 		return 0;
@@ -5712,11 +5700,14 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
 static int
 skl_compute_ddb(struct intel_atomic_state *state)
 {
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct intel_crtc_state *old_crtc_state;
 	struct intel_crtc_state *new_crtc_state;
 	struct intel_crtc *crtc;
 	int ret, i;
 
+	state->enabled_dbuf_slices_mask = dev_priv->enabled_dbuf_slices_mask;
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		ret = skl_allocate_pipe_ddb(new_crtc_state);
@@ -5864,8 +5855,7 @@ skl_print_wm_changes(struct intel_atomic_state *state)
 	}
 }
 
-static int intel_add_affected_pipes(struct intel_atomic_state *state,
-				    u8 pipe_mask)
+static int intel_add_all_pipes(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
 	struct intel_crtc *crtc;
@@ -5873,9 +5863,6 @@ static int intel_add_affected_pipes(struct intel_atomic_state *state,
 	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		struct intel_crtc_state *crtc_state;
 
-		if ((pipe_mask & BIT(crtc->pipe)) == 0)
-			continue;
-
 		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
@@ -5888,54 +5875,49 @@ static int
 skl_ddb_add_affected_pipes(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_crtc_state *crtc_state;
-	struct intel_crtc *crtc;
-	int i, ret;
+	int ret;
 
+	/*
+	 * If this is our first atomic update following hardware readout,
+	 * we can't trust the DDB that the BIOS programmed for us.  Let's
+	 * pretend that all pipes switched active status so that we'll
+	 * ensure a full DDB recompute.
+	 */
 	if (dev_priv->wm.distrust_bios_wm) {
-		/*
-		 * skl_ddb_get_pipe_allocation_limits() currently requires
-		 * all active pipes to be included in the state so that
-		 * it can redistribute the dbuf among them, and it really
-		 * wants to recompute things when distrust_bios_wm is set
-		 * so we add all the pipes to the state.
-		 */
-		ret = intel_add_affected_pipes(state, ~0);
+		ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
+				       state->base.acquire_ctx);
 		if (ret)
 			return ret;
-	}
 
-	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
-		struct intel_dbuf_state *new_dbuf_state;
-		const struct intel_dbuf_state *old_dbuf_state;
-
-		new_dbuf_state = intel_atomic_get_dbuf_state(state);
-		if (IS_ERR(new_dbuf_state))
-			return ret;
-
-		old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
-
-		new_dbuf_state->active_pipes =
-			intel_calc_active_pipes(state, old_dbuf_state->active_pipes);
-
-		if (old_dbuf_state->active_pipes == new_dbuf_state->active_pipes)
-			break;
-
-		ret = intel_atomic_lock_global_state(&new_dbuf_state->base);
-		if (ret)
-			return ret;
+		state->active_pipe_changes = INTEL_INFO(dev_priv)->pipe_mask;
 
 		/*
-		 * skl_ddb_get_pipe_allocation_limits() currently requires
-		 * all active pipes to be included in the state so that
-		 * it can redistribute the dbuf among them.
+		 * We usually only initialize state->active_pipes if we
+		 * we're doing a modeset; make sure this field is always
+		 * initialized during the sanitization process that happens
+		 * on the first commit too.
 		 */
-		ret = intel_add_affected_pipes(state,
-					       new_dbuf_state->active_pipes);
+		if (!state->modeset)
+			state->active_pipes = dev_priv->active_pipes;
+	}
+
+	/*
+	 * If the modeset changes which CRTC's are active, we need to
+	 * recompute the DDB allocation for *all* active pipes, even
+	 * those that weren't otherwise being modified in any way by this
+	 * atomic commit.  Due to the shrinking of the per-pipe allocations
+	 * when new active CRTC's are added, it's possible for a pipe that
+	 * we were already using and aren't changing at all here to suddenly
+	 * become invalid if its DDB needs exceeds its new allocation.
+	 *
+	 * Note that if we wind up doing a full DDB recompute, we can't let
+	 * any other display updates race with this transaction, so we need
+	 * to grab the lock on *all* CRTC's.
+	 */
+	if (state->active_pipe_changes || state->modeset) {
+		ret = intel_add_all_pipes(state);
 		if (ret)
 			return ret;
-
-		break;
 	}
 
 	return 0;
@@ -7764,52 +7746,3 @@ void intel_pm_setup(struct drm_i915_private *dev_priv)
 	dev_priv->runtime_pm.suspended = false;
 	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);
 }
-
-static struct intel_global_state *intel_dbuf_duplicate_state(struct intel_global_obj *obj)
-{
-	struct intel_dbuf_state *dbuf_state;
-
-	dbuf_state = kmemdup(obj->state, sizeof(*dbuf_state), GFP_KERNEL);
-	if (!dbuf_state)
-		return NULL;
-
-	return &dbuf_state->base;
-}
-
-static void intel_dbuf_destroy_state(struct intel_global_obj *obj,
-				     struct intel_global_state *state)
-{
-	kfree(state);
-}
-
-static const struct intel_global_state_funcs intel_dbuf_funcs = {
-	.atomic_duplicate_state = intel_dbuf_duplicate_state,
-	.atomic_destroy_state = intel_dbuf_destroy_state,
-};
-
-struct intel_dbuf_state *
-intel_atomic_get_dbuf_state(struct intel_atomic_state *state)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_global_state *dbuf_state;
-
-	dbuf_state = intel_atomic_get_global_obj_state(state, &dev_priv->dbuf.obj);
-	if (IS_ERR(dbuf_state))
-		return ERR_CAST(dbuf_state);
-
-	return to_intel_dbuf_state(dbuf_state);
-}
-
-int intel_dbuf_init(struct drm_i915_private *dev_priv)
-{
-	struct intel_dbuf_state *dbuf_state;
-
-	dbuf_state = kzalloc(sizeof(*dbuf_state), GFP_KERNEL);
-	if (!dbuf_state)
-		return -ENOMEM;
-
-	intel_atomic_global_obj_init(dev_priv, &dev_priv->dbuf.obj,
-				     &dbuf_state->base, &intel_dbuf_funcs);
-
-	return 0;
-}
diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
index 9f75ac4c2bd1..614ac7f8d4cc 100644
--- a/drivers/gpu/drm/i915/intel_pm.h
+++ b/drivers/gpu/drm/i915/intel_pm.h
@@ -8,8 +8,6 @@
 
 #include <linux/types.h>
 
-#include "display/intel_global_state.h"
-
 #include "i915_reg.h"
 #include "display/intel_bw.h"
 
@@ -65,24 +63,4 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv);
 
 bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable);
 
-struct intel_dbuf_state {
-	struct intel_global_state base;
-
-	u8 enabled_slices;
-	u8 active_pipes;
-};
-
-int intel_dbuf_init(struct drm_i915_private *dev_priv);
-
-struct intel_dbuf_state *
-intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
-
-#define to_intel_dbuf_state(x) container_of((x), struct intel_dbuf_state, base)
-#define intel_atomic_get_old_dbuf_state(state) \
-	to_intel_dbuf_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
-#define intel_atomic_get_new_dbuf_state(state) \
-	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
-
-int intel_dbuf_init(struct drm_i915_private *dev_priv);
-
 #endif /* __INTEL_PM_H__ */
-- 
2.26.2

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

Re: [Intel-gfx] [PATCH 4/4] Revert "drm/i915: Introduce proper dbuf state"

[Lisovskiy, Stanislav]

On Mon, May 18, 2020 at 03:23:03PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Dbuf slice tracking busted across runtime PM. Back to the
> drawing board.
> 
> This reverts commit 3cf43cdc63fbc3df19ea8398e9b8717ab44a6304.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  67 ++-----
>  .../drm/i915/display/intel_display_power.c    |   8 +-
>  .../drm/i915/display/intel_display_types.h    |  13 ++
>  drivers/gpu/drm/i915/i915_drv.h               |  11 +-
>  drivers/gpu/drm/i915/intel_pm.c               | 185 ++++++------------
>  drivers/gpu/drm/i915/intel_pm.h               |  22 ---
>  6 files changed, 99 insertions(+), 207 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 49577f19ff9c..d6635d649dc8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7579,8 +7579,6 @@ static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
>  		to_intel_bw_state(dev_priv->bw_obj.state);
>  	struct intel_cdclk_state *cdclk_state =
>  		to_intel_cdclk_state(dev_priv->cdclk.obj.state);
> -	struct intel_dbuf_state *dbuf_state =
> -		to_intel_dbuf_state(dev_priv->dbuf.obj.state);
>  	struct intel_crtc_state *crtc_state =
>  		to_intel_crtc_state(crtc->base.state);
>  	enum intel_display_power_domain domain;
> @@ -7654,8 +7652,6 @@ static void intel_crtc_disable_noatomic(struct intel_crtc *crtc,
>  	cdclk_state->min_voltage_level[pipe] = 0;
>  	cdclk_state->active_pipes &= ~BIT(pipe);
>  
> -	dbuf_state->active_pipes &= ~BIT(pipe);
> -
>  	bw_state->data_rate[pipe] = 0;
>  	bw_state->num_active_planes[pipe] = 0;
>  }
> @@ -14012,10 +14008,10 @@ static void verify_wm_state(struct intel_crtc *crtc,
>  	hw_enabled_slices = intel_enabled_dbuf_slices_mask(dev_priv);
>  
>  	if (INTEL_GEN(dev_priv) >= 11 &&
> -	    hw_enabled_slices != dev_priv->dbuf.enabled_slices)
> +	    hw_enabled_slices != dev_priv->enabled_dbuf_slices_mask)
>  		drm_err(&dev_priv->drm,
>  			"mismatch in DBUF Slices (expected 0x%x, got 0x%x)\n",
> -			dev_priv->dbuf.enabled_slices,
> +			dev_priv->enabled_dbuf_slices_mask,
>  			hw_enabled_slices);
>  
>  	/* planes */
> @@ -14556,7 +14552,9 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
>  	state->modeset = true;
>  	state->active_pipes = intel_calc_active_pipes(state, dev_priv->active_pipes);
>  
> -	if (state->active_pipes != dev_priv->active_pipes) {
> +	state->active_pipe_changes = state->active_pipes ^ dev_priv->active_pipes;
> +
> +	if (state->active_pipe_changes) {
>  		ret = _intel_atomic_lock_global_state(state);
>  		if (ret)
>  			return ret;
> @@ -15210,38 +15208,22 @@ static void intel_commit_modeset_enables(struct intel_atomic_state *state)
>  static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_dbuf_state *new_dbuf_state =
> -		intel_atomic_get_new_dbuf_state(state);
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(state);
> -
> -	if (!new_dbuf_state ||
> -	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> -		return;
> +	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_mask;
> +	u8 required_slices = state->enabled_dbuf_slices_mask;
> +	u8 slices_union = hw_enabled_slices | required_slices;
>  
> -	WARN_ON(!new_dbuf_state->base.changed);
> -
> -	gen9_dbuf_slices_update(dev_priv,
> -				old_dbuf_state->enabled_slices |
> -				new_dbuf_state->enabled_slices);
> +	if (INTEL_GEN(dev_priv) >= 11 && slices_union != hw_enabled_slices)
> +		gen9_dbuf_slices_update(dev_priv, slices_union);
>  }
>  
>  static void icl_dbuf_slice_post_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_dbuf_state *new_dbuf_state =
> -		intel_atomic_get_new_dbuf_state(state);
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(state);
> -
> -	if (!new_dbuf_state ||
> -	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
> -		return;
> +	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_mask;
> +	u8 required_slices = state->enabled_dbuf_slices_mask;

I guess that one opened the pandora box.. May be we shouldn't
even stick to checking against the old_dbuf_state? I mean,
this paradigm would work well only if we suppose that the last
state doesn't change in the HW by itself, meanwhile now
you have an old dbuf state, which might change in the HW.

As I understand you try to readout hw and access dbuf object
from dev_priv rightaway - can this also bring some sync issues?

I think we were discussing at some point why we can't access
global state objects from dev_priv rightaway, as those 
constantly change.

Anyways, regarding this particular revert,

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Stan

>  
> -	WARN_ON(!new_dbuf_state->base.changed);
> -
> -	gen9_dbuf_slices_update(dev_priv,
> -				new_dbuf_state->enabled_slices);
> +	if (INTEL_GEN(dev_priv) >= 11 && required_slices != hw_enabled_slices)
> +		gen9_dbuf_slices_update(dev_priv, required_slices);
>  }
>  
>  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> @@ -15484,7 +15466,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	if (state->modeset)
>  		intel_encoders_update_prepare(state);
>  
> -	icl_dbuf_slice_pre_update(state);
> +	/* Enable all new slices, we might need */
> +	if (state->modeset)
> +		icl_dbuf_slice_pre_update(state);
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.commit_modeset_enables(state);
> @@ -15539,7 +15523,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  			dev_priv->display.optimize_watermarks(state, crtc);
>  	}
>  
> -	icl_dbuf_slice_post_update(state);
> +	/* Disable all slices, we don't need */
> +	if (state->modeset)
> +		icl_dbuf_slice_post_update(state);
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		intel_post_plane_update(state, crtc);
> @@ -17433,14 +17419,10 @@ void intel_modeset_init_hw(struct drm_i915_private *i915)
>  {
>  	struct intel_cdclk_state *cdclk_state =
>  		to_intel_cdclk_state(i915->cdclk.obj.state);
> -	struct intel_dbuf_state *dbuf_state =
> -		to_intel_dbuf_state(i915->dbuf.obj.state);
>  
>  	intel_update_cdclk(i915);
>  	intel_dump_cdclk_config(&i915->cdclk.hw, "Current CDCLK");
>  	cdclk_state->logical = cdclk_state->actual = i915->cdclk.hw;
> -
> -	dbuf_state->enabled_slices = i915->dbuf.enabled_slices;
>  }
>  
>  static int sanitize_watermarks_add_affected(struct drm_atomic_state *state)
> @@ -17729,10 +17711,6 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
>  	if (ret)
>  		return ret;
>  
> -	ret = intel_dbuf_init(i915);
> -	if (ret)
> -		return ret;
> -
>  	ret = intel_bw_init(i915);
>  	if (ret)
>  		return ret;
> @@ -18249,8 +18227,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_cdclk_state *cdclk_state =
>  		to_intel_cdclk_state(dev_priv->cdclk.obj.state);
> -	struct intel_dbuf_state *dbuf_state =
> -		to_intel_dbuf_state(dev_priv->dbuf.obj.state);
>  	enum pipe pipe;
>  	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
> @@ -18281,8 +18257,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			    enableddisabled(crtc_state->hw.active));
>  	}
>  
> -	dev_priv->active_pipes = cdclk_state->active_pipes =
> -		dbuf_state->active_pipes = active_pipes;
> +	dev_priv->active_pipes = cdclk_state->active_pipes = active_pipes;
>  
>  	readout_plane_state(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 7a8213993110..a3e581947bec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -1161,7 +1161,7 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
>  static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
>  {
>  	u8 hw_enabled_dbuf_slices = intel_enabled_dbuf_slices_mask(dev_priv);
> -	u8 enabled_dbuf_slices = dev_priv->dbuf.enabled_slices;
> +	u8 enabled_dbuf_slices = dev_priv->enabled_dbuf_slices_mask;
>  
>  	drm_WARN(&dev_priv->drm,
>  		 hw_enabled_dbuf_slices != enabled_dbuf_slices,
> @@ -4539,14 +4539,14 @@ void gen9_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  	for (slice = DBUF_S1; slice < num_slices; slice++)
>  		gen9_dbuf_slice_set(dev_priv, slice, req_slices & BIT(slice));
>  
> -	dev_priv->dbuf.enabled_slices = req_slices;
> +	dev_priv->enabled_dbuf_slices_mask = req_slices;
>  
>  	mutex_unlock(&power_domains->lock);
>  }
>  
>  static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->dbuf.enabled_slices =
> +	dev_priv->enabled_dbuf_slices_mask =
>  		intel_enabled_dbuf_slices_mask(dev_priv);
>  
>  	/*
> @@ -4554,7 +4554,7 @@ static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
>  	 * figure out later which slices we have and what we need.
>  	 */
>  	gen9_dbuf_slices_update(dev_priv, BIT(DBUF_S1) |
> -				dev_priv->dbuf.enabled_slices);
> +				dev_priv->enabled_dbuf_slices_mask);
>  }
>  
>  static void gen9_dbuf_disable(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83e2f0bd2387..2bf3d4cb4ea9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -479,6 +479,16 @@ struct intel_atomic_state {
>  
>  	bool dpll_set, modeset;
>  
> +	/*
> +	 * Does this transaction change the pipes that are active?  This mask
> +	 * tracks which CRTC's have changed their active state at the end of
> +	 * the transaction (not counting the temporary disable during modesets).
> +	 * This mask should only be non-zero when intel_state->modeset is true,
> +	 * but the converse is not necessarily true; simply changing a mode may
> +	 * not flip the final active status of any CRTC's
> +	 */
> +	u8 active_pipe_changes;
> +
>  	u8 active_pipes;
>  
>  	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> @@ -496,6 +506,9 @@ struct intel_atomic_state {
>  	 */
>  	bool global_state_changed;
>  
> +	/* Number of enabled DBuf slices */
> +	u8 enabled_dbuf_slices_mask;
> +
>  	struct i915_sw_fence commit_ready;
>  
>  	struct llist_node freed;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 64798b398482..adb9bf34cf97 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -950,13 +950,6 @@ struct drm_i915_private {
>  		struct intel_global_obj obj;
>  	} cdclk;
>  
> -	struct {
> -		/* The current hardware dbuf configuration */
> -		u8 enabled_slices;
> -
> -		struct intel_global_obj obj;
> -	} dbuf;
> -
>  	/**
>  	 * wq - Driver workqueue for GEM.
>  	 *
> @@ -1133,12 +1126,12 @@ struct drm_i915_private {
>  		 * Set during HW readout of watermarks/DDB.  Some platforms
>  		 * need to know when we're still using BIOS-provided values
>  		 * (which we don't fully trust).
> -		 *
> -		 * FIXME get rid of this.
>  		 */
>  		bool distrust_bios_wm;
>  	} wm;
>  
> +	u8 enabled_dbuf_slices_mask; /* GEN11 has configurable 2 slices */
> +
>  	struct dram_info {
>  		bool valid;
>  		bool is_16gb_dimm;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cb57786fdc9f..5c47b893e7b2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4040,7 +4040,7 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv)
>  static u8 skl_compute_dbuf_slices(const struct intel_crtc_state *crtc_state,
>  				  u8 active_pipes);
>  
> -static int
> +static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const u64 total_data_rate,
> @@ -4053,29 +4053,30 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	const struct intel_crtc *crtc;
>  	u32 pipe_width = 0, total_width_in_range = 0, width_before_pipe_in_range = 0;
>  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> -	struct intel_dbuf_state *new_dbuf_state =
> -		intel_atomic_get_new_dbuf_state(intel_state);
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(intel_state);
> -	u8 active_pipes = new_dbuf_state->active_pipes;
>  	u16 ddb_size;
>  	u32 ddb_range_size;
>  	u32 i;
>  	u32 dbuf_slice_mask;
> +	u32 active_pipes;
>  	u32 offset;
>  	u32 slice_size;
>  	u32 total_slice_mask;
>  	u32 start, end;
> -	int ret;
> -
> -	*num_active = hweight8(active_pipes);
>  
> -	if (!crtc_state->hw.active) {
> +	if (drm_WARN_ON(&dev_priv->drm, !state) || !crtc_state->hw.active) {
>  		alloc->start = 0;
>  		alloc->end = 0;
> -		return 0;
> +		*num_active = hweight8(dev_priv->active_pipes);
> +		return;
>  	}
>  
> +	if (intel_state->active_pipe_changes)
> +		active_pipes = intel_state->active_pipes;
> +	else
> +		active_pipes = dev_priv->active_pipes;
> +
> +	*num_active = hweight8(active_pipes);
> +
>  	ddb_size = intel_get_ddb_size(dev_priv);
>  
>  	slice_size = ddb_size / INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
> @@ -4088,16 +4089,13 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	 * that changes the active CRTC list or do modeset would need to
>  	 * grab _all_ crtc locks, including the one we currently hold.
>  	 */
> -	if (old_dbuf_state->active_pipes == new_dbuf_state->active_pipes &&
> -	    !dev_priv->wm.distrust_bios_wm) {
> +	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
>  		/*
>  		 * alloc may be cleared by clear_intel_crtc_state,
>  		 * copy from old state to be sure
> -		 *
> -		 * FIXME get rid of this mess
>  		 */
>  		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
> -		return 0;
> +		return;
>  	}
>  
>  	/*
> @@ -4176,13 +4174,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	 * FIXME: For now we always enable slice S1 as per
>  	 * the Bspec display initialization sequence.
>  	 */
> -	new_dbuf_state->enabled_slices = total_slice_mask | BIT(DBUF_S1);
> -
> -	if (old_dbuf_state->enabled_slices != new_dbuf_state->enabled_slices) {
> -		ret = intel_atomic_serialize_global_state(&new_dbuf_state->base);
> -		if (ret)
> -			return ret;
> -	}
> +	intel_state->enabled_dbuf_slices_mask = total_slice_mask | BIT(DBUF_S1);
>  
>  	start = ddb_range_size * width_before_pipe_in_range / total_width_in_range;
>  	end = ddb_range_size *
> @@ -4193,8 +4185,9 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  
>  	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
>  		      alloc->start, alloc->end);
> -
> -	return 0;
> +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> +		      intel_state->enabled_dbuf_slices_mask,
> +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
>  }
>  
>  static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> @@ -4317,8 +4310,8 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
>  
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->dbuf.enabled_slices =
> -		intel_enabled_dbuf_slices_mask(dev_priv);
> +	dev_priv->enabled_dbuf_slices_mask =
> +				intel_enabled_dbuf_slices_mask(dev_priv);
>  }
>  
>  /*
> @@ -4765,7 +4758,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  	u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
>  	u32 blocks;
>  	int level;
> -	int ret;
>  
>  	/* Clear the partitioning for disabled planes. */
>  	memset(crtc_state->wm.skl.plane_ddb_y, 0, sizeof(crtc_state->wm.skl.plane_ddb_y));
> @@ -4786,12 +4778,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  							 plane_data_rate,
>  							 uv_plane_data_rate);
>  
> -	ret = skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state,
> -						 total_data_rate,
> -						 alloc, &num_active);
> -	if (ret)
> -		return ret;
> -
> +	skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate,
> +					   alloc, &num_active);
>  	alloc_size = skl_ddb_entry_size(alloc);
>  	if (alloc_size == 0)
>  		return 0;
> @@ -5712,11 +5700,14 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
>  static int
>  skl_compute_ddb(struct intel_atomic_state *state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc_state *old_crtc_state;
>  	struct intel_crtc_state *new_crtc_state;
>  	struct intel_crtc *crtc;
>  	int ret, i;
>  
> +	state->enabled_dbuf_slices_mask = dev_priv->enabled_dbuf_slices_mask;
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		ret = skl_allocate_pipe_ddb(new_crtc_state);
> @@ -5864,8 +5855,7 @@ skl_print_wm_changes(struct intel_atomic_state *state)
>  	}
>  }
>  
> -static int intel_add_affected_pipes(struct intel_atomic_state *state,
> -				    u8 pipe_mask)
> +static int intel_add_all_pipes(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc *crtc;
> @@ -5873,9 +5863,6 @@ static int intel_add_affected_pipes(struct intel_atomic_state *state,
>  	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		struct intel_crtc_state *crtc_state;
>  
> -		if ((pipe_mask & BIT(crtc->pipe)) == 0)
> -			continue;
> -
>  		crtc_state = intel_atomic_get_crtc_state(&state->base, crtc);
>  		if (IS_ERR(crtc_state))
>  			return PTR_ERR(crtc_state);
> @@ -5888,54 +5875,49 @@ static int
>  skl_ddb_add_affected_pipes(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	struct intel_crtc_state *crtc_state;
> -	struct intel_crtc *crtc;
> -	int i, ret;
> +	int ret;
>  
> +	/*
> +	 * If this is our first atomic update following hardware readout,
> +	 * we can't trust the DDB that the BIOS programmed for us.  Let's
> +	 * pretend that all pipes switched active status so that we'll
> +	 * ensure a full DDB recompute.
> +	 */
>  	if (dev_priv->wm.distrust_bios_wm) {
> -		/*
> -		 * skl_ddb_get_pipe_allocation_limits() currently requires
> -		 * all active pipes to be included in the state so that
> -		 * it can redistribute the dbuf among them, and it really
> -		 * wants to recompute things when distrust_bios_wm is set
> -		 * so we add all the pipes to the state.
> -		 */
> -		ret = intel_add_affected_pipes(state, ~0);
> +		ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
> +				       state->base.acquire_ctx);
>  		if (ret)
>  			return ret;
> -	}
>  
> -	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> -		struct intel_dbuf_state *new_dbuf_state;
> -		const struct intel_dbuf_state *old_dbuf_state;
> -
> -		new_dbuf_state = intel_atomic_get_dbuf_state(state);
> -		if (IS_ERR(new_dbuf_state))
> -			return ret;
> -
> -		old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
> -
> -		new_dbuf_state->active_pipes =
> -			intel_calc_active_pipes(state, old_dbuf_state->active_pipes);
> -
> -		if (old_dbuf_state->active_pipes == new_dbuf_state->active_pipes)
> -			break;
> -
> -		ret = intel_atomic_lock_global_state(&new_dbuf_state->base);
> -		if (ret)
> -			return ret;
> +		state->active_pipe_changes = INTEL_INFO(dev_priv)->pipe_mask;
>  
>  		/*
> -		 * skl_ddb_get_pipe_allocation_limits() currently requires
> -		 * all active pipes to be included in the state so that
> -		 * it can redistribute the dbuf among them.
> +		 * We usually only initialize state->active_pipes if we
> +		 * we're doing a modeset; make sure this field is always
> +		 * initialized during the sanitization process that happens
> +		 * on the first commit too.
>  		 */
> -		ret = intel_add_affected_pipes(state,
> -					       new_dbuf_state->active_pipes);
> +		if (!state->modeset)
> +			state->active_pipes = dev_priv->active_pipes;
> +	}
> +
> +	/*
> +	 * If the modeset changes which CRTC's are active, we need to
> +	 * recompute the DDB allocation for *all* active pipes, even
> +	 * those that weren't otherwise being modified in any way by this
> +	 * atomic commit.  Due to the shrinking of the per-pipe allocations
> +	 * when new active CRTC's are added, it's possible for a pipe that
> +	 * we were already using and aren't changing at all here to suddenly
> +	 * become invalid if its DDB needs exceeds its new allocation.
> +	 *
> +	 * Note that if we wind up doing a full DDB recompute, we can't let
> +	 * any other display updates race with this transaction, so we need
> +	 * to grab the lock on *all* CRTC's.
> +	 */
> +	if (state->active_pipe_changes || state->modeset) {
> +		ret = intel_add_all_pipes(state);
>  		if (ret)
>  			return ret;
> -
> -		break;
>  	}
>  
>  	return 0;
> @@ -7764,52 +7746,3 @@ void intel_pm_setup(struct drm_i915_private *dev_priv)
>  	dev_priv->runtime_pm.suspended = false;
>  	atomic_set(&dev_priv->runtime_pm.wakeref_count, 0);
>  }
> -
> -static struct intel_global_state *intel_dbuf_duplicate_state(struct intel_global_obj *obj)
> -{
> -	struct intel_dbuf_state *dbuf_state;
> -
> -	dbuf_state = kmemdup(obj->state, sizeof(*dbuf_state), GFP_KERNEL);
> -	if (!dbuf_state)
> -		return NULL;
> -
> -	return &dbuf_state->base;
> -}
> -
> -static void intel_dbuf_destroy_state(struct intel_global_obj *obj,
> -				     struct intel_global_state *state)
> -{
> -	kfree(state);
> -}
> -
> -static const struct intel_global_state_funcs intel_dbuf_funcs = {
> -	.atomic_duplicate_state = intel_dbuf_duplicate_state,
> -	.atomic_destroy_state = intel_dbuf_destroy_state,
> -};
> -
> -struct intel_dbuf_state *
> -intel_atomic_get_dbuf_state(struct intel_atomic_state *state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	struct intel_global_state *dbuf_state;
> -
> -	dbuf_state = intel_atomic_get_global_obj_state(state, &dev_priv->dbuf.obj);
> -	if (IS_ERR(dbuf_state))
> -		return ERR_CAST(dbuf_state);
> -
> -	return to_intel_dbuf_state(dbuf_state);
> -}
> -
> -int intel_dbuf_init(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_dbuf_state *dbuf_state;
> -
> -	dbuf_state = kzalloc(sizeof(*dbuf_state), GFP_KERNEL);
> -	if (!dbuf_state)
> -		return -ENOMEM;
> -
> -	intel_atomic_global_obj_init(dev_priv, &dev_priv->dbuf.obj,
> -				     &dbuf_state->base, &intel_dbuf_funcs);
> -
> -	return 0;
> -}
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index 9f75ac4c2bd1..614ac7f8d4cc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -8,8 +8,6 @@
>  
>  #include <linux/types.h>
>  
> -#include "display/intel_global_state.h"
> -
>  #include "i915_reg.h"
>  #include "display/intel_bw.h"
>  
> @@ -65,24 +63,4 @@ void intel_enable_ipc(struct drm_i915_private *dev_priv);
>  
>  bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable);
>  
> -struct intel_dbuf_state {
> -	struct intel_global_state base;
> -
> -	u8 enabled_slices;
> -	u8 active_pipes;
> -};
> -
> -int intel_dbuf_init(struct drm_i915_private *dev_priv);
> -
> -struct intel_dbuf_state *
> -intel_atomic_get_dbuf_state(struct intel_atomic_state *state);
> -
> -#define to_intel_dbuf_state(x) container_of((x), struct intel_dbuf_state, base)
> -#define intel_atomic_get_old_dbuf_state(state) \
> -	to_intel_dbuf_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
> -#define intel_atomic_get_new_dbuf_state(state) \
> -	to_intel_dbuf_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->dbuf.obj))
> -
> -int intel_dbuf_init(struct drm_i915_private *dev_priv);
> -
>  #endif /* __INTEL_PM_H__ */
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

[Lisovskiy, Stanislav]

On Mon, May 18, 2020 at 03:23:00PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Dbuf slice tracking busted across runtime PM. Back to the
> drawing board.
> 
> This reverts commit 70b1a26f299c729cc1a5099374cc02568b05ec7d.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 26 +++++++-------------------
>  1 file changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a21e36ed1a77..d40d22eb65da 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4105,6 +4105,10 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	 */
>  	dbuf_slice_mask = skl_compute_dbuf_slices(crtc_state, active_pipes);
>  
> +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %c active pipes %x\n",
> +		      dbuf_slice_mask,
> +		      pipe_name(for_pipe), active_pipes);
> +

Just wanted to say as a joke that if I would be adding those, you
would say that those are redundant debugs.. and then figured out
that those were mine initially :)

Ok - back to the drawing board!

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  	/*
>  	 * Figure out at which DBuf slice we start, i.e if we start at Dbuf S2
>  	 * and slice size is 1024, the offset would be 1024
> @@ -4187,10 +4191,8 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	alloc->start = offset + start;
>  	alloc->end = offset + end;
>  
> -	drm_dbg_kms(&dev_priv->drm,
> -		    "[CRTC:%d:%s] dbuf slices 0x%x, ddb (%d - %d), active pipes 0x%x\n",
> -		    for_crtc->base.id, for_crtc->name,
> -		    dbuf_slice_mask, alloc->start, alloc->end, active_pipes);
> +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> +		      alloc->start, alloc->end);
>  
>  	return 0;
>  }
> @@ -5704,10 +5706,7 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
>  static int
>  skl_compute_ddb(struct intel_atomic_state *state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_dbuf_state *old_dbuf_state;
> -	const struct intel_dbuf_state *new_dbuf_state;
> -	const struct intel_crtc_state *old_crtc_state;
> +	struct intel_crtc_state *old_crtc_state;
>  	struct intel_crtc_state *new_crtc_state;
>  	struct intel_crtc *crtc;
>  	int ret, i;
> @@ -5724,17 +5723,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> -	old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
> -	new_dbuf_state = intel_atomic_get_new_dbuf_state(state);
> -
> -	if (new_dbuf_state &&
> -	    new_dbuf_state->enabled_slices != old_dbuf_state->enabled_slices)
> -		drm_dbg_kms(&dev_priv->drm,
> -			    "Enabled dbuf slices 0x%x -> 0x%x (out of %d dbuf slices)\n",
> -			    old_dbuf_state->enabled_slices,
> -			    new_dbuf_state->enabled_slices,
> -			    INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
> -
>  	return 0;
>  }
>  
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

[Patchwork]

== Series Details ==

Series: series starting with [1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"
URL   : https://patchwork.freedesktop.org/series/77358/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
4de007ca5a0f Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"
3358451aea83 Revert "drm/i915: Move the dbuf pre/post plane update"
77a3d0811ea1 Revert "drm/i915: Nuke skl_ddb_get_hw_state()"
0ede4a7df848 Revert "drm/i915: Introduce proper dbuf state"
-:174: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#174: FILE: drivers/gpu/drm/i915/display/intel_display.c:18260:
+	dev_priv->active_pipes = cdclk_state->active_pipes = active_pipes;

total: 0 errors, 0 warnings, 1 checks, 506 lines checked

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

[Patchwork]

== Series Details ==

Series: series starting with [1/4] Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"
URL   : https://patchwork.freedesktop.org/series/77358/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8494 -> Patchwork_17691
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_17691 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_17691, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_17691:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - fi-cml-s:           [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8494/fi-cml-s/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/fi-cml-s/igt@i915_selftest@live@hangcheck.html

  
Known issues
------------

  Here are the changes found in Patchwork_17691 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@execlists:
    - fi-tgl-y:           [PASS][3] -> [INCOMPLETE][4] ([i915#1803])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8494/fi-tgl-y/igt@i915_selftest@live@execlists.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/fi-tgl-y/igt@i915_selftest@live@execlists.html
    - fi-skl-6700k2:      [PASS][5] -> [INCOMPLETE][6] ([i915#1795] / [i915#656])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8494/fi-skl-6700k2/igt@i915_selftest@live@execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/fi-skl-6700k2/igt@i915_selftest@live@execlists.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - fi-whl-u:           [INCOMPLETE][7] ([i915#656]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8494/fi-whl-u/igt@i915_selftest@live@execlists.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/fi-whl-u/igt@i915_selftest@live@execlists.html

  
#### Warnings ####

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-x1275:       [SKIP][9] ([fdo#109271]) -> [FAIL][10] ([i915#62] / [i915#95])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8494/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1795]: https://gitlab.freedesktop.org/drm/intel/issues/1795
  [i915#1803]: https://gitlab.freedesktop.org/drm/intel/issues/1803
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (51 -> 44)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_8494 -> Patchwork_17691

  CI-20190529: 20190529
  CI_DRM_8494: 3d15348fde9b998e754da0b0655baf02b98e7f17 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5657: 649eae5c905a7460b44305800f95db83a6dd47cb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17691: 0ede4a7df84889e374e588f8cf93260914a21975 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0ede4a7df848 Revert "drm/i915: Introduce proper dbuf state"
77a3d0811ea1 Revert "drm/i915: Nuke skl_ddb_get_hw_state()"
3358451aea83 Revert "drm/i915: Move the dbuf pre/post plane update"
4de007ca5a0f Revert "drm/i915: Clean up dbuf debugs during .atomic_check()"

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17691/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx