[PATCH 0/3] Rework of the WM flush (for the DDB allocation)

[PATCH 0/3] Rework of the WM flush (for the DDB allocation)

[Damien Lespiau]

Ville found that the sequencing I had to re-program the DDB wasn't quite
correct and so this is an attempt to do better.

This series reworks patch:

  [PATCH 78/89] drm/i915/skl: Flush the WM configuration

of the initial SKL Stage 1 series. The core of the issue is now documented in a
comment, so I won't repeat it here again.

-- 
Damien

Damien Lespiau (3):
  drm/i915/skl: Stage the pipe DDB allocation
  drm/i915/skl: Flush the WM configuration
  drm/i915/skl: Log the order in which we flush the pipes in the WM code

 drivers/gpu/drm/i915/i915_drv.h |   1 +
 drivers/gpu/drm/i915/intel_pm.c | 152 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 146 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] drm/i915/skl: Stage the pipe DDB allocation

[Damien Lespiau]

To correctly flush the new DDB allocation we need to know about the pipe
allocation layout inside the DDB in order to sequence the re-allocation
to not cause a newly allocated pipe to fetch from a space that was
previously allocated to another pipe.

This patch preserves the per-pipe (start,end) allocation to be used in
the flush.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2e800db..075be77 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1436,6 +1436,7 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
 }
 
 struct skl_ddb_allocation {
+	struct skl_ddb_entry pipe[I915_MAX_PIPES];
 	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
 	struct skl_ddb_entry cursor[I915_MAX_PIPES];
 };
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5129d6b..118eb95 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3117,13 +3117,13 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	struct skl_ddb_entry alloc;
+	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
 	uint16_t alloc_size, start, cursor_blocks;
 	unsigned int total_data_rate;
 	int plane;
 
-	skl_ddb_get_pipe_allocation_limits(dev, crtc, config, params, &alloc);
-	alloc_size = skl_ddb_entry_size(&alloc);
+	skl_ddb_get_pipe_allocation_limits(dev, crtc, config, params, alloc);
+	alloc_size = skl_ddb_entry_size(alloc);
 	if (alloc_size == 0) {
 		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
 		memset(&ddb->cursor[pipe], 0, sizeof(ddb->cursor[pipe]));
@@ -3131,11 +3131,11 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	}
 
 	cursor_blocks = skl_cursor_allocation(config);
-	ddb->cursor[pipe].start = alloc.end - cursor_blocks;
-	ddb->cursor[pipe].end = alloc.end;
+	ddb->cursor[pipe].start = alloc->end - cursor_blocks;
+	ddb->cursor[pipe].end = alloc->end;
 
 	alloc_size -= cursor_blocks;
-	alloc.end -= cursor_blocks;
+	alloc->end -= cursor_blocks;
 
 	/*
 	 * Each active plane get a portion of the remaining space, in
@@ -3145,7 +3145,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	 */
 	total_data_rate = skl_get_total_relative_data_rate(intel_crtc, params);
 
-	start = alloc.start;
+	start = alloc->start;
 	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
 		const struct intel_plane_wm_parameters *p;
 		unsigned int data_rate;
-- 
1.8.3.1

[PATCH 2/3] drm/i915/skl: Flush the WM configuration

[Damien Lespiau]

When we write new values for the DDB allocation and WM parameters, we now
need to trigger the double buffer update for the pipe to take the new
configuration into account.

As the DDB is a global resource shared between planes, enabling or
disabling one plane will result in changes for all planes that are
currently in use, thus the need write PLANE_SURF/CUR_BASE for more than
the plane we're touching.

v2: Don't wait for pipes that are off

v3: Split the staging results structure to not exceed the 1Kb stack
    allocation in skl_update_wm()

v4: Rework and document the algorithm after Ville found that it was all
    wrong.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 135 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 118eb95..99f7c40 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3507,6 +3507,140 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	}
 }
 
+/*
+ * When setting up a new DDB allocation arrangement, we need to correctly
+ * sequence the times at which the new allocations for the pipes are taken into
+ * account or we'll have pipes fetching from space previously allocated to
+ * another pipe.
+ *
+ * Roughly the sequence looks like:
+ *  1. re-allocate the pipe(s) with the allocation being reduced and not
+ *     overlapping with a previous light-up pipe (another way to put it is:
+ *     pipes with their new allocation strickly included into their old ones).
+ *  2. re-allocate the other pipes that get their allocation reduced
+ *  3. allocate the pipes having their allocation increased
+ *
+ * Steps 1. and 2. are here to take care of the following case:
+ * - Initially DDB looks like this:
+ *     |   B    |   C    |
+ * - enable pipe A.
+ * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
+ *   allocation
+ *     |  A  |  B  |  C  |
+ *
+ * We need to sequence the re-allocation: C, B, A (and not B, C, A).
+ */
+
+static void skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int plane;
+
+	for_each_plane(pipe, plane) {
+		I915_WRITE(PLANE_SURF(pipe, plane),
+			   I915_READ(PLANE_SURF(pipe, plane)));
+	}
+	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
+}
+
+static bool
+skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
+			    const struct skl_ddb_allocation *new,
+			    enum pipe pipe)
+{
+	uint16_t old_size, new_size;
+
+	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
+	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
+
+	return old_size != new_size &&
+	       new->pipe[pipe].start >= old->pipe[pipe].start &&
+	       new->pipe[pipe].end <= old->pipe[pipe].end;
+}
+
+static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
+				struct skl_wm_values *new_values)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct skl_ddb_allocation *cur_ddb, *new_ddb;
+	bool reallocated[I915_MAX_PIPES] = {false, false, false};
+	struct intel_crtc *crtc;
+	enum pipe pipe;
+
+	new_ddb = &new_values->ddb;
+	cur_ddb = &dev_priv->wm.skl_hw.ddb;
+
+	/*
+	 * First pass: flush the pipes with the new allocation contained into
+	 * the old space.
+	 *
+	 * We'll wait for the vblank on those pipes to ensure we can safely
+	 * re-allocate the freed space without this pipe fetching from it.
+	 */
+	for_each_intel_crtc(dev, crtc) {
+		if (!crtc->active)
+			continue;
+
+		pipe = crtc->pipe;
+
+		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
+			continue;
+
+		skl_wm_flush_pipe(dev_priv, pipe);
+		intel_wait_for_vblank(dev, pipe);
+
+		reallocated[pipe] = true;
+	}
+
+
+	/*
+	 * Second pass: flush the pipes that are having their allocation
+	 * reduced, but overlapping with a previous allocation.
+	 *
+	 * Here as well we need to wait for the vblank to make sure the freed
+	 * space is not used anymore.
+	 */
+	for_each_intel_crtc(dev, crtc) {
+		if (!crtc->active)
+			continue;
+
+		pipe = crtc->pipe;
+
+		if (reallocated[pipe])
+			continue;
+
+		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
+		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
+			skl_wm_flush_pipe(dev_priv, pipe);
+			intel_wait_for_vblank(dev, pipe);
+		}
+
+		reallocated[pipe] = true;
+	}
+
+	/*
+	 * Third pass: flush the pipes that got more space allocated.
+	 *
+	 * We don't need to actively wait for the update here, next vblank
+	 * will just get more DDB space with the correct WM values.
+	 */
+	for_each_intel_crtc(dev, crtc) {
+		if (!crtc->active)
+			continue;
+
+		pipe = crtc->pipe;
+
+		/*
+		 * At this point, only the pipes more space than before are
+		 * left to re-allocate.
+		 */
+		if (reallocated[pipe])
+			continue;
+
+		skl_wm_flush_pipe(dev_priv, pipe);
+	}
+}
+
 static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 			       struct skl_pipe_wm_parameters *params,
 			       struct intel_wm_config *config,
@@ -3598,6 +3732,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	skl_update_other_pipe_wm(dev, crtc, &config, results);
 	skl_write_wm_values(dev_priv, results);
+	skl_flush_wm_values(dev_priv, results);
 
 	/* store the new configuration */
 	dev_priv->wm.skl_hw = *results;
-- 
1.8.3.1

Re: [PATCH 2/3] drm/i915/skl: Flush the WM configuration

[Ville Syrjälä]

On Wed, Oct 15, 2014 at 06:34:43PM +0100, Damien Lespiau wrote:
> When we write new values for the DDB allocation and WM parameters, we now
> need to trigger the double buffer update for the pipe to take the new
> configuration into account.
> 
> As the DDB is a global resource shared between planes, enabling or
> disabling one plane will result in changes for all planes that are
> currently in use, thus the need write PLANE_SURF/CUR_BASE for more than
> the plane we're touching.
> 
> v2: Don't wait for pipes that are off
> 
> v3: Split the staging results structure to not exceed the 1Kb stack
>     allocation in skl_update_wm()
> 
> v4: Rework and document the algorithm after Ville found that it was all
>     wrong.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 135 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 135 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 118eb95..99f7c40 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3507,6 +3507,140 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +/*
> + * When setting up a new DDB allocation arrangement, we need to correctly
> + * sequence the times at which the new allocations for the pipes are taken into
> + * account or we'll have pipes fetching from space previously allocated to
> + * another pipe.
> + *
> + * Roughly the sequence looks like:
> + *  1. re-allocate the pipe(s) with the allocation being reduced and not
> + *     overlapping with a previous light-up pipe (another way to put it is:
> + *     pipes with their new allocation strickly included into their old ones).
> + *  2. re-allocate the other pipes that get their allocation reduced
> + *  3. allocate the pipes having their allocation increased
> + *
> + * Steps 1. and 2. are here to take care of the following case:
> + * - Initially DDB looks like this:
> + *     |   B    |   C    |
> + * - enable pipe A.
> + * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
> + *   allocation
> + *     |  A  |  B  |  C  |
> + *
> + * We need to sequence the re-allocation: C, B, A (and not B, C, A).
> + */
> +
> +static void skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int plane;
> +
> +	for_each_plane(pipe, plane) {
> +		I915_WRITE(PLANE_SURF(pipe, plane),
> +			   I915_READ(PLANE_SURF(pipe, plane)));
> +	}
> +	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
> +}
> +
> +static bool
> +skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
> +			    const struct skl_ddb_allocation *new,
> +			    enum pipe pipe)
> +{
> +	uint16_t old_size, new_size;
> +
> +	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
> +	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
> +
> +	return old_size != new_size &&

First I was thinking this size check is redundant, but it's actually
needed to avoid the vblank wait when things didn't change.

The algorithm itself looks solid to me. Well, assuming that the
allocated chunks are always sorted the same way. Which they since the
allocation uses a for_each_pipe() loop. Anything else would be mad
anyway because then we might not be able to reallocate without
temporarily disabling some planes.

The only issue with this is that it actually shouldn't be hiding in the
wm code AFAICS, and instead we should drive the whole atomic commit
from this same algorithm so that the DDB allocation, watermarks and plane
config all agree with each other, all the time. So basically
skl_wm_flush_pipe() should eventually become something like:

{
 for_each_plane_on_crtc()
 	plane.commit();
 crtc.commit(); // for global pipe properties
}

But it seems good enough for now given that we're not yet living in the
nuclear age.

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

> +	       new->pipe[pipe].start >= old->pipe[pipe].start &&
> +	       new->pipe[pipe].end <= old->pipe[pipe].end;
> +}
> +
> +static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
> +				struct skl_wm_values *new_values)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct skl_ddb_allocation *cur_ddb, *new_ddb;
> +	bool reallocated[I915_MAX_PIPES] = {false, false, false};
> +	struct intel_crtc *crtc;
> +	enum pipe pipe;
> +
> +	new_ddb = &new_values->ddb;
> +	cur_ddb = &dev_priv->wm.skl_hw.ddb;
> +
> +	/*
> +	 * First pass: flush the pipes with the new allocation contained into
> +	 * the old space.
> +	 *
> +	 * We'll wait for the vblank on those pipes to ensure we can safely
> +	 * re-allocate the freed space without this pipe fetching from it.
> +	 */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (!crtc->active)
> +			continue;
> +
> +		pipe = crtc->pipe;
> +
> +		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
> +			continue;
> +
> +		skl_wm_flush_pipe(dev_priv, pipe);
> +		intel_wait_for_vblank(dev, pipe);
> +
> +		reallocated[pipe] = true;
> +	}
> +
> +
> +	/*
> +	 * Second pass: flush the pipes that are having their allocation
> +	 * reduced, but overlapping with a previous allocation.
> +	 *
> +	 * Here as well we need to wait for the vblank to make sure the freed
> +	 * space is not used anymore.
> +	 */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (!crtc->active)
> +			continue;
> +
> +		pipe = crtc->pipe;
> +
> +		if (reallocated[pipe])
> +			continue;
> +
> +		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
> +		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
> +			skl_wm_flush_pipe(dev_priv, pipe);
> +			intel_wait_for_vblank(dev, pipe);
> +		}
> +
> +		reallocated[pipe] = true;
> +	}
> +
> +	/*
> +	 * Third pass: flush the pipes that got more space allocated.
> +	 *
> +	 * We don't need to actively wait for the update here, next vblank
> +	 * will just get more DDB space with the correct WM values.
> +	 */
> +	for_each_intel_crtc(dev, crtc) {
> +		if (!crtc->active)
> +			continue;
> +
> +		pipe = crtc->pipe;
> +
> +		/*
> +		 * At this point, only the pipes more space than before are
> +		 * left to re-allocate.
> +		 */
> +		if (reallocated[pipe])
> +			continue;
> +
> +		skl_wm_flush_pipe(dev_priv, pipe);
> +	}
> +}
> +
>  static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  			       struct skl_pipe_wm_parameters *params,
>  			       struct intel_wm_config *config,
> @@ -3598,6 +3732,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  
>  	skl_update_other_pipe_wm(dev, crtc, &config, results);
>  	skl_write_wm_values(dev_priv, results);
> +	skl_flush_wm_values(dev_priv, results);
>  
>  	/* store the new configuration */
>  	dev_priv->wm.skl_hw = *results;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 3/3] drm/i915/skl: Log the order in which we flush the pipes in the WM code

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99f7c40..178b35f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3531,11 +3531,14 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
  * We need to sequence the re-allocation: C, B, A (and not B, C, A).
  */
 
-static void skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
+static void
+skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
 {
 	struct drm_device *dev = dev_priv->dev;
 	int plane;
 
+	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
+
 	for_each_plane(pipe, plane) {
 		I915_WRITE(PLANE_SURF(pipe, plane),
 			   I915_READ(PLANE_SURF(pipe, plane)));
@@ -3586,7 +3589,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
 			continue;
 
-		skl_wm_flush_pipe(dev_priv, pipe);
+		skl_wm_flush_pipe(dev_priv, pipe, 1);
 		intel_wait_for_vblank(dev, pipe);
 
 		reallocated[pipe] = true;
@@ -3611,7 +3614,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 
 		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
 		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
-			skl_wm_flush_pipe(dev_priv, pipe);
+			skl_wm_flush_pipe(dev_priv, pipe, 2);
 			intel_wait_for_vblank(dev, pipe);
 		}
 
@@ -3637,7 +3640,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 		if (reallocated[pipe])
 			continue;
 
-		skl_wm_flush_pipe(dev_priv, pipe);
+		skl_wm_flush_pipe(dev_priv, pipe, 3);
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH 0/3] Rework of the WM flush (for the DDB allocation)

[Ville Syrjälä]

On Wed, Oct 15, 2014 at 06:34:41PM +0100, Damien Lespiau wrote:
> Ville found that the sequencing I had to re-program the DDB wasn't quite
> correct and so this is an attempt to do better.
> 
> This series reworks patch:
> 
>   [PATCH 78/89] drm/i915/skl: Flush the WM configuration
> 
> of the initial SKL Stage 1 series. The core of the issue is now documented in a
> comment, so I won't repeat it here again.
> 
> -- 
> Damien
> 
> Damien Lespiau (3):
>   drm/i915/skl: Stage the pipe DDB allocation
>   drm/i915/skl: Flush the WM configuration

Replied to this with r-b and some comments.

>   drm/i915/skl: Log the order in which we flush the pipes in the WM code

And the other two look good too, so for the entire series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  drivers/gpu/drm/i915/i915_drv.h |   1 +
>  drivers/gpu/drm/i915/intel_pm.c | 152 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 146 insertions(+), 7 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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