diff mbox series

[v2,11/13] drm/i915: Commit skl+ planes in an order that avoids ddb overlaps

Message ID 20181114210729.16185-12-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Program SKL+ watermarks/ddb more carefully | expand

Commit Message

Ville Syrjala Nov. 14, 2018, 9:07 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

skl+ can go belly up if there are overlapping ddb allocations between
planes. If we could absolutely guarantee that we can perform the atomic
update within a single frame we shouldn't have to worry about this. But
we can't rely on that so let's steal the ddb overlap check trick from
skl_update_crtcs() and apply it to the plane updates. Since each step
of the sequence is free from ddb overlaps we don't have to worry about
a vblank sneaking up on us in the middle of the sequence. The partial
state that gets latched by the hardware will be safe. And unlike
skl_update_crtcs() we don't have to intoduce any extra vblank waits
on accoung of only having to worry about a single pipe.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 96 ++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c      |  7 +-
 drivers/gpu/drm/i915/intel_drv.h          |  8 +-
 3 files changed, 93 insertions(+), 18 deletions(-)

Comments

Matt Roper Nov. 26, 2018, 11:28 p.m. UTC | #1 
On Wed, Nov 14, 2018 at 11:07:27PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> skl+ can go belly up if there are overlapping ddb allocations between
> planes. If we could absolutely guarantee that we can perform the atomic
> update within a single frame we shouldn't have to worry about this. But
> we can't rely on that so let's steal the ddb overlap check trick from
> skl_update_crtcs() and apply it to the plane updates. Since each step
> of the sequence is free from ddb overlaps we don't have to worry about
> a vblank sneaking up on us in the middle of the sequence. The partial
> state that gets latched by the hardware will be safe. And unlike
> skl_update_crtcs() we don't have to intoduce any extra vblank waits
> on accoung of only having to worry about a single pipe.

Minor typo on 'account' here.

Otherwise, patch looks good.

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

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 96 ++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c      |  7 +-
>  drivers/gpu/drm/i915/intel_drv.h          |  8 +-
>  3 files changed, 93 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 69fc7010190c..ff8d3e577bbf 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -169,24 +169,75 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  						   to_intel_plane_state(new_plane_state));
>  }
>  
> -void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> -				 struct intel_crtc *crtc,
> -				 struct intel_crtc_state *old_crtc_state,
> -				 struct intel_crtc_state *new_crtc_state)
> +static struct intel_plane *
> +skl_next_plane_to_commit(struct intel_atomic_state *state,
> +			 struct intel_crtc *crtc,
> +			 struct skl_ddb_entry entries_y[I915_MAX_PLANES],
> +			 struct skl_ddb_entry entries_uv[I915_MAX_PLANES],
> +			 unsigned int *update_mask)
>  {
> -	u32 update_mask = new_crtc_state->update_planes;
> -	struct intel_plane_state *new_plane_state;
> +	struct intel_crtc_state *crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct intel_plane_state *plane_state;
>  	struct intel_plane *plane;
>  	int i;
>  
> -	for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
> +	if (*update_mask == 0)
> +		return NULL;
> +
> +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> +		enum plane_id plane_id = plane->id;
> +
>  		if (crtc->pipe != plane->pipe ||
> -		    !(update_mask & BIT(plane->id)))
> +		    !(*update_mask & BIT(plane_id)))
>  			continue;
>  
> +		if (skl_ddb_allocation_overlaps(&crtc_state->wm.skl.plane_ddb_y[plane_id],
> +						entries_y,
> +						I915_MAX_PLANES, plane_id) ||
> +		    skl_ddb_allocation_overlaps(&crtc_state->wm.skl.plane_ddb_uv[plane_id],
> +						entries_uv,
> +						I915_MAX_PLANES, plane_id))
> +			continue;
> +
> +		*update_mask &= ~BIT(plane_id);
> +		entries_y[plane_id] = crtc_state->wm.skl.plane_ddb_y[plane_id];
> +		entries_uv[plane_id] = crtc_state->wm.skl.plane_ddb_uv[plane_id];
> +
> +		return plane;
> +	}
> +
> +	/* should never happen */
> +	WARN_ON(1);
> +
> +	return NULL;
> +}
> +
> +void skl_update_planes_on_crtc(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc)
> +{
> +	struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(state, crtc);
> +	struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	struct skl_ddb_entry entries_y[I915_MAX_PLANES];
> +	struct skl_ddb_entry entries_uv[I915_MAX_PLANES];
> +	u32 update_mask = new_crtc_state->update_planes;
> +	struct intel_plane *plane;
> +
> +	memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y,
> +	       sizeof(old_crtc_state->wm.skl.plane_ddb_y));
> +	memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv,
> +	       sizeof(old_crtc_state->wm.skl.plane_ddb_uv));
> +
> +	while ((plane = skl_next_plane_to_commit(state, crtc,
> +						 entries_y, entries_uv,
> +						 &update_mask))) {
> +		struct intel_plane_state *new_plane_state =
> +			intel_atomic_get_new_plane_state(state, plane);
> +
>  		if (new_plane_state->base.visible) {
>  			trace_intel_update_plane(&plane->base, crtc);
> -
>  			plane->update_plane(plane, new_crtc_state, new_plane_state);
>  		} else if (new_plane_state->slave) {
>  			struct intel_plane *master =
> @@ -202,14 +253,37 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
>  			 * plane_state.
>  			 */
>  			new_plane_state =
> -				intel_atomic_get_new_plane_state(old_state, master);
> +				intel_atomic_get_new_plane_state(state, master);
>  
>  			trace_intel_update_plane(&plane->base, crtc);
> -
>  			plane->update_slave(plane, new_crtc_state, new_plane_state);
>  		} else {
>  			trace_intel_disable_plane(&plane->base, crtc);
> +			plane->disable_plane(plane, new_crtc_state);
> +		}
> +	}
> +}
>  
> +void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
> +				struct intel_crtc *crtc)
> +{
> +	struct intel_crtc_state *new_crtc_state =
> +		intel_atomic_get_new_crtc_state(state, crtc);
> +	u32 update_mask = new_crtc_state->update_planes;
> +	struct intel_plane_state *new_plane_state;
> +	struct intel_plane *plane;
> +	int i;
> +
> +	for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) {
> +		if (crtc->pipe != plane->pipe ||
> +		    !(update_mask & BIT(plane->id)))
> +			continue;
> +
> +		if (new_plane_state->base.visible) {
> +			trace_intel_update_plane(&plane->base, crtc);
> +			plane->update_plane(plane, new_crtc_state, new_plane_state);
> +		} else {
> +			trace_intel_disable_plane(&plane->base, crtc);
>  			plane->disable_plane(plane, new_crtc_state);
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2981cea3704a..114b2f3c6274 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12661,7 +12661,6 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
>  	bool modeset = needs_modeset(new_crtc_state);
>  	struct intel_plane_state *new_plane_state =
> @@ -12684,8 +12683,10 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  
>  	intel_begin_crtc_commit(crtc, old_crtc_state);
>  
> -	intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
> -				    old_intel_cstate, pipe_config);
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
> +	else
> +		i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
>  
>  	intel_finish_crtc_commit(crtc, old_crtc_state);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4a9af09c483a..2af994942fa7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2299,10 +2299,10 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
>  void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> -void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> -				 struct intel_crtc *crtc,
> -				 struct intel_crtc_state *old_crtc_state,
> -				 struct intel_crtc_state *new_crtc_state);
> +void skl_update_planes_on_crtc(struct intel_atomic_state *state,
> +			       struct intel_crtc *crtc);
> +void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
> +				struct intel_crtc *crtc);
>  int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
>  					struct intel_crtc_state *crtc_state,
>  					const struct intel_plane_state *old_plane_state,
> -- 
> 2.18.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 69fc7010190c..ff8d3e577bbf 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -169,24 +169,75 @@  static int intel_plane_atomic_check(struct drm_plane *plane,
 						   to_intel_plane_state(new_plane_state));
 }
 
-void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
-				 struct intel_crtc *crtc,
-				 struct intel_crtc_state *old_crtc_state,
-				 struct intel_crtc_state *new_crtc_state)
+static struct intel_plane *
+skl_next_plane_to_commit(struct intel_atomic_state *state,
+			 struct intel_crtc *crtc,
+			 struct skl_ddb_entry entries_y[I915_MAX_PLANES],
+			 struct skl_ddb_entry entries_uv[I915_MAX_PLANES],
+			 unsigned int *update_mask)
 {
-	u32 update_mask = new_crtc_state->update_planes;
-	struct intel_plane_state *new_plane_state;
+	struct intel_crtc_state *crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct intel_plane_state *plane_state;
 	struct intel_plane *plane;
 	int i;
 
-	for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
+	if (*update_mask == 0)
+		return NULL;
+
+	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
+		enum plane_id plane_id = plane->id;
+
 		if (crtc->pipe != plane->pipe ||
-		    !(update_mask & BIT(plane->id)))
+		    !(*update_mask & BIT(plane_id)))
 			continue;
 
+		if (skl_ddb_allocation_overlaps(&crtc_state->wm.skl.plane_ddb_y[plane_id],
+						entries_y,
+						I915_MAX_PLANES, plane_id) ||
+		    skl_ddb_allocation_overlaps(&crtc_state->wm.skl.plane_ddb_uv[plane_id],
+						entries_uv,
+						I915_MAX_PLANES, plane_id))
+			continue;
+
+		*update_mask &= ~BIT(plane_id);
+		entries_y[plane_id] = crtc_state->wm.skl.plane_ddb_y[plane_id];
+		entries_uv[plane_id] = crtc_state->wm.skl.plane_ddb_uv[plane_id];
+
+		return plane;
+	}
+
+	/* should never happen */
+	WARN_ON(1);
+
+	return NULL;
+}
+
+void skl_update_planes_on_crtc(struct intel_atomic_state *state,
+			       struct intel_crtc *crtc)
+{
+	struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(state, crtc);
+	struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	struct skl_ddb_entry entries_y[I915_MAX_PLANES];
+	struct skl_ddb_entry entries_uv[I915_MAX_PLANES];
+	u32 update_mask = new_crtc_state->update_planes;
+	struct intel_plane *plane;
+
+	memcpy(entries_y, old_crtc_state->wm.skl.plane_ddb_y,
+	       sizeof(old_crtc_state->wm.skl.plane_ddb_y));
+	memcpy(entries_uv, old_crtc_state->wm.skl.plane_ddb_uv,
+	       sizeof(old_crtc_state->wm.skl.plane_ddb_uv));
+
+	while ((plane = skl_next_plane_to_commit(state, crtc,
+						 entries_y, entries_uv,
+						 &update_mask))) {
+		struct intel_plane_state *new_plane_state =
+			intel_atomic_get_new_plane_state(state, plane);
+
 		if (new_plane_state->base.visible) {
 			trace_intel_update_plane(&plane->base, crtc);
-
 			plane->update_plane(plane, new_crtc_state, new_plane_state);
 		} else if (new_plane_state->slave) {
 			struct intel_plane *master =
@@ -202,14 +253,37 @@  void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
 			 * plane_state.
 			 */
 			new_plane_state =
-				intel_atomic_get_new_plane_state(old_state, master);
+				intel_atomic_get_new_plane_state(state, master);
 
 			trace_intel_update_plane(&plane->base, crtc);
-
 			plane->update_slave(plane, new_crtc_state, new_plane_state);
 		} else {
 			trace_intel_disable_plane(&plane->base, crtc);
+			plane->disable_plane(plane, new_crtc_state);
+		}
+	}
+}
 
+void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
+				struct intel_crtc *crtc)
+{
+	struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	u32 update_mask = new_crtc_state->update_planes;
+	struct intel_plane_state *new_plane_state;
+	struct intel_plane *plane;
+	int i;
+
+	for_each_new_intel_plane_in_state(state, plane, new_plane_state, i) {
+		if (crtc->pipe != plane->pipe ||
+		    !(update_mask & BIT(plane->id)))
+			continue;
+
+		if (new_plane_state->base.visible) {
+			trace_intel_update_plane(&plane->base, crtc);
+			plane->update_plane(plane, new_crtc_state, new_plane_state);
+		} else {
+			trace_intel_disable_plane(&plane->base, crtc);
 			plane->disable_plane(plane, new_crtc_state);
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2981cea3704a..114b2f3c6274 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12661,7 +12661,6 @@  static void intel_update_crtc(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
 	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
 	bool modeset = needs_modeset(new_crtc_state);
 	struct intel_plane_state *new_plane_state =
@@ -12684,8 +12683,10 @@  static void intel_update_crtc(struct drm_crtc *crtc,
 
 	intel_begin_crtc_commit(crtc, old_crtc_state);
 
-	intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
-				    old_intel_cstate, pipe_config);
+	if (INTEL_GEN(dev_priv) >= 9)
+		skl_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
+	else
+		i9xx_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc);
 
 	intel_finish_crtc_commit(crtc, old_crtc_state);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4a9af09c483a..2af994942fa7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2299,10 +2299,10 @@  struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
 void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
-void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
-				 struct intel_crtc *crtc,
-				 struct intel_crtc_state *old_crtc_state,
-				 struct intel_crtc_state *new_crtc_state);
+void skl_update_planes_on_crtc(struct intel_atomic_state *state,
+			       struct intel_crtc *crtc);
+void i9xx_update_planes_on_crtc(struct intel_atomic_state *state,
+				struct intel_crtc *crtc);
 int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
 					struct intel_crtc_state *crtc_state,
 					const struct intel_plane_state *old_plane_state,