Re: [Intel-gfx] [PATCH v2 11/13] drm/i915: Commit skl+ planes in an order that avoids ddb overlaps
On Wed, Nov 14, 2018 at 11:07:27PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
>
> Signed-off-by: Ville Syrjälä
> ---
> 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(_state->wm.skl.plane_ddb_y[plane_id],
> + entries_y,
> + I915_MAX_PLANES, plane_id) ||
> +
> skl_ddb_allocation_overlaps(_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,
> + _mask))) {
> + struct intel_plane_state *new_plane_state =
> + intel_atomic_get_new_plane_state(state, plane);
> +
[Intel-gfx] [PATCH v2 11/13] drm/i915: Commit skl+ planes in an order that avoids ddb overlaps
From: Ville Syrjälä
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ä
---
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(_state->wm.skl.plane_ddb_y[plane_id],
+ entries_y,
+ I915_MAX_PLANES, plane_id) ||
+
skl_ddb_allocation_overlaps(_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,
+_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(>base, crtc);
-
plane->update_plane(plane, new_crtc_state,
new_plane_state);
} else if (new_plane_state->slave) {
