Re: [Intel-gfx] [PATCH v4] drm/i915: implement async_flip mode per plane tracking
On Fri, Jan 27, 2023 at 04:30:02PM +0100, Andrzej Hajda wrote:
> Current implementation of async flip w/a relies on assumption that
> previous atomic commit contains valid information if async_flip is still
> enabled on the plane. It is incorrect. If previous commit did not modify
> the plane its state->uapi.async_flip can be false. As a result DMAR/PIPE
> errors can be observed:
> i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
> i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
> DMAR: DRHD: handling fault status reg 2
> DMAR: [DMA Read NO_PASID] Request device [00:02.0] fault addr 0x0 [fault
> reason 0x06] PTE Read access is not set
>
> v2: update async_flip_planes in more reliable places (Ville)
> v3: reset async_flip_planes and do_async_flip in more scenarios (Ville)
> v4: move all resets to plane loops (Ville)
>
> Signed-off-by: Andrzej Hajda
> ---
> Hi Ville,
>
> I am not sure about this change. I wonder if in case of
> for*plane loops code could be like:
>
> new_crtc_state->async_flip_planes &= ~BIT(plane->id);
> if (!new_crtc_state->async_flip_planes)
> new_crtc_state->do_async_flip = false;
>
> But let's see what CI says.
>
> Regards
> Andrzej
> ---
> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 -
> drivers/gpu/drm/i915/display/intel_color.c | 3 +++
> drivers/gpu/drm/i915/display/intel_display.c | 9 ++---
> drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
> drivers/gpu/drm/i915/display/skl_watermark.c | 4
> 5 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 1409bcfb6fd3d9..3bd8f7eb75a60b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -363,6 +363,7 @@ void intel_plane_set_invisible(struct intel_crtc_state
> *crtc_state,
> crtc_state->scaled_planes &= ~BIT(plane->id);
> crtc_state->nv12_planes &= ~BIT(plane->id);
> crtc_state->c8_planes &= ~BIT(plane->id);
> + crtc_state->async_flip_planes &= ~BIT(plane->id);
> crtc_state->data_rate[plane->id] = 0;
> crtc_state->data_rate_y[plane->id] = 0;
> crtc_state->rel_data_rate[plane->id] = 0;
> @@ -582,8 +583,10 @@ static int intel_plane_atomic_calc_changes(const struct
> intel_crtc_state *old_cr
>intel_plane_is_scaled(new_plane_state
> new_crtc_state->disable_lp_wm = true;
>
> - if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state))
> + if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state)) {
> new_crtc_state->do_async_flip = true;
> + new_crtc_state->async_flip_planes |= BIT(plane->id);
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 8d97c299e6577b..2ca7a016a9d9d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1500,12 +1500,15 @@ intel_color_add_affected_planes(struct
> intel_crtc_state *new_crtc_state)
> return PTR_ERR(plane_state);
>
> new_crtc_state->update_planes |= BIT(plane->id);
> + new_crtc_state->async_flip_planes = 0;
> + new_crtc_state->do_async_flip = false;
>
> /* plane control register changes blocked by CxSR */
> if (HAS_GMCH(i915))
> new_crtc_state->disable_cxsr = true;
> }
>
> +
> return 0;
> }
Thanks. Pushed now. I nuked that bogus extra newline while pushing.
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 717ca3d7890d34..fcd3f1c7af3291 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1252,7 +1252,8 @@ static void intel_crtc_async_flip_disable_wa(struct
> intel_atomic_state *state,
> intel_atomic_get_old_crtc_state(state, crtc);
> const struct intel_crtc_state *new_crtc_state =
> intel_atomic_get_new_crtc_state(state, crtc);
> - u8 update_planes = new_crtc_state->update_planes;
> + u8 disable_async_flip_planes = old_crtc_state->async_flip_planes &
> +~new_crtc_state->async_flip_planes;
> const struct intel_plane_state *old_plane_state;
> struct intel_plane *plane;
> bool need_vbl_wait = false;
> @@ -1261,7 +1262,7 @@ static void intel_crtc_async_flip_disable_wa(struct
> intel_atomic_state *state,
> for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
> if (plane->need_async_flip_disable_wa &&
> plane->pipe == crtc->pipe &&
> - update_planes & BIT(plane->id)) {
> +
Re: [Intel-gfx] [PATCH v4] drm/i915: implement async_flip mode per plane tracking
On Fri, Jan 27, 2023 at 04:30:02PM +0100, Andrzej Hajda wrote:
> Current implementation of async flip w/a relies on assumption that
> previous atomic commit contains valid information if async_flip is still
> enabled on the plane. It is incorrect. If previous commit did not modify
> the plane its state->uapi.async_flip can be false. As a result DMAR/PIPE
> errors can be observed:
> i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
> i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
> DMAR: DRHD: handling fault status reg 2
> DMAR: [DMA Read NO_PASID] Request device [00:02.0] fault addr 0x0 [fault
> reason 0x06] PTE Read access is not set
>
> v2: update async_flip_planes in more reliable places (Ville)
> v3: reset async_flip_planes and do_async_flip in more scenarios (Ville)
> v4: move all resets to plane loops (Ville)
>
> Signed-off-by: Andrzej Hajda
> ---
> Hi Ville,
>
> I am not sure about this change. I wonder if in case of
> for*plane loops code could be like:
>
> new_crtc_state->async_flip_planes &= ~BIT(plane->id);
> if (!new_crtc_state->async_flip_planes)
> new_crtc_state->do_async_flip = false;
The current code is all geared towards all planes doing the
same kind of update (async vs. sync). Maybe we could rework
things so that some planes could remain in async mode while
the rest (and the whole pipe) really does a sync update.
But that will involve some real work.
This one is
Reviewed-by: Ville Syrjälä
>
> But let's see what CI says.
>
> Regards
> Andrzej
> ---
> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 -
> drivers/gpu/drm/i915/display/intel_color.c | 3 +++
> drivers/gpu/drm/i915/display/intel_display.c | 9 ++---
> drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
> drivers/gpu/drm/i915/display/skl_watermark.c | 4
> 5 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 1409bcfb6fd3d9..3bd8f7eb75a60b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -363,6 +363,7 @@ void intel_plane_set_invisible(struct intel_crtc_state
> *crtc_state,
> crtc_state->scaled_planes &= ~BIT(plane->id);
> crtc_state->nv12_planes &= ~BIT(plane->id);
> crtc_state->c8_planes &= ~BIT(plane->id);
> + crtc_state->async_flip_planes &= ~BIT(plane->id);
> crtc_state->data_rate[plane->id] = 0;
> crtc_state->data_rate_y[plane->id] = 0;
> crtc_state->rel_data_rate[plane->id] = 0;
> @@ -582,8 +583,10 @@ static int intel_plane_atomic_calc_changes(const struct
> intel_crtc_state *old_cr
>intel_plane_is_scaled(new_plane_state
> new_crtc_state->disable_lp_wm = true;
>
> - if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state))
> + if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state)) {
> new_crtc_state->do_async_flip = true;
> + new_crtc_state->async_flip_planes |= BIT(plane->id);
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 8d97c299e6577b..2ca7a016a9d9d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1500,12 +1500,15 @@ intel_color_add_affected_planes(struct
> intel_crtc_state *new_crtc_state)
> return PTR_ERR(plane_state);
>
> new_crtc_state->update_planes |= BIT(plane->id);
> + new_crtc_state->async_flip_planes = 0;
> + new_crtc_state->do_async_flip = false;
>
> /* plane control register changes blocked by CxSR */
> if (HAS_GMCH(i915))
> new_crtc_state->disable_cxsr = true;
> }
>
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 717ca3d7890d34..fcd3f1c7af3291 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1252,7 +1252,8 @@ static void intel_crtc_async_flip_disable_wa(struct
> intel_atomic_state *state,
> intel_atomic_get_old_crtc_state(state, crtc);
> const struct intel_crtc_state *new_crtc_state =
> intel_atomic_get_new_crtc_state(state, crtc);
> - u8 update_planes = new_crtc_state->update_planes;
> + u8 disable_async_flip_planes = old_crtc_state->async_flip_planes &
> +~new_crtc_state->async_flip_planes;
> const struct intel_plane_state *old_plane_state;
> struct intel_plane *plane;
> bool need_vbl_wait = false;
> @@ -1261,7 +1262,7 @@ static void intel_crtc_async_flip_disable_wa(struct
> intel_atomic_state
[Intel-gfx] [PATCH v4] drm/i915: implement async_flip mode per plane tracking
Current implementation of async flip w/a relies on assumption that
previous atomic commit contains valid information if async_flip is still
enabled on the plane. It is incorrect. If previous commit did not modify
the plane its state->uapi.async_flip can be false. As a result DMAR/PIPE
errors can be observed:
i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
i915 :00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x0080
DMAR: DRHD: handling fault status reg 2
DMAR: [DMA Read NO_PASID] Request device [00:02.0] fault addr 0x0 [fault reason
0x06] PTE Read access is not set
v2: update async_flip_planes in more reliable places (Ville)
v3: reset async_flip_planes and do_async_flip in more scenarios (Ville)
v4: move all resets to plane loops (Ville)
Signed-off-by: Andrzej Hajda
---
Hi Ville,
I am not sure about this change. I wonder if in case of
for*plane loops code could be like:
new_crtc_state->async_flip_planes &= ~BIT(plane->id);
if (!new_crtc_state->async_flip_planes)
new_crtc_state->do_async_flip = false;
But let's see what CI says.
Regards
Andrzej
---
drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 -
drivers/gpu/drm/i915/display/intel_color.c | 3 +++
drivers/gpu/drm/i915/display/intel_display.c | 9 ++---
drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
drivers/gpu/drm/i915/display/skl_watermark.c | 4
5 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 1409bcfb6fd3d9..3bd8f7eb75a60b 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -363,6 +363,7 @@ void intel_plane_set_invisible(struct intel_crtc_state
*crtc_state,
crtc_state->scaled_planes &= ~BIT(plane->id);
crtc_state->nv12_planes &= ~BIT(plane->id);
crtc_state->c8_planes &= ~BIT(plane->id);
+ crtc_state->async_flip_planes &= ~BIT(plane->id);
crtc_state->data_rate[plane->id] = 0;
crtc_state->data_rate_y[plane->id] = 0;
crtc_state->rel_data_rate[plane->id] = 0;
@@ -582,8 +583,10 @@ static int intel_plane_atomic_calc_changes(const struct
intel_crtc_state *old_cr
intel_plane_is_scaled(new_plane_state
new_crtc_state->disable_lp_wm = true;
- if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state))
+ if (intel_plane_do_async_flip(plane, old_crtc_state, new_crtc_state)) {
new_crtc_state->do_async_flip = true;
+ new_crtc_state->async_flip_planes |= BIT(plane->id);
+ }
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 8d97c299e6577b..2ca7a016a9d9d1 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1500,12 +1500,15 @@ intel_color_add_affected_planes(struct intel_crtc_state
*new_crtc_state)
return PTR_ERR(plane_state);
new_crtc_state->update_planes |= BIT(plane->id);
+ new_crtc_state->async_flip_planes = 0;
+ new_crtc_state->do_async_flip = false;
/* plane control register changes blocked by CxSR */
if (HAS_GMCH(i915))
new_crtc_state->disable_cxsr = true;
}
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 717ca3d7890d34..fcd3f1c7af3291 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1252,7 +1252,8 @@ static void intel_crtc_async_flip_disable_wa(struct
intel_atomic_state *state,
intel_atomic_get_old_crtc_state(state, crtc);
const struct intel_crtc_state *new_crtc_state =
intel_atomic_get_new_crtc_state(state, crtc);
- u8 update_planes = new_crtc_state->update_planes;
+ u8 disable_async_flip_planes = old_crtc_state->async_flip_planes &
+ ~new_crtc_state->async_flip_planes;
const struct intel_plane_state *old_plane_state;
struct intel_plane *plane;
bool need_vbl_wait = false;
@@ -1261,7 +1262,7 @@ static void intel_crtc_async_flip_disable_wa(struct
intel_atomic_state *state,
for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
if (plane->need_async_flip_disable_wa &&
plane->pipe == crtc->pipe &&
- update_planes & BIT(plane->id)) {
+ disable_async_flip_planes & BIT(plane->id)) {
/*
* Apart from the async flip bit we want to
* preserve the old state for the plane.
@@ -1378,7 +1379,7 @@ static void
