| Message ID | 20230111162247.2991559-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: implement async_flip mode per plane tracking | expand |
On 11.01.2023 18:40, Patchwork wrote: > *Patch Details* > *Series:* drm/i915: implement async_flip mode per plane tracking (rev2) > *URL:* https://patchwork.freedesktop.org/series/108371/ > <https://patchwork.freedesktop.org/series/108371/> > *State:* failure > *Details:* > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/index.html > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/index.html> > > > CI Bug Log - changes from CI_DRM_12573 -> Patchwork_108371v2 > > > Summary > > *FAILURE* > > Serious unknown changes coming with Patchwork_108371v2 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_108371v2, 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_108371v2/index.html > > > Participating hosts (34 -> 33) > > Additional (1): fi-kbl-soraka > Missing (2): fi-rkl-11600 fi-snb-2520m > > > Possible new issues > > Here are the unknown changes that may have been introduced in > Patchwork_108371v2: > > > IGT changes > > > Possible regressions > > * igt@i915_selftest@live@guc: > o fi-kbl-soraka: NOTRUN -> INCOMPLETE > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@i915_selftest@live@guc.html> Again not related bug in fi-kbl-soraka and GuC [1] or more generally live tests[2]. [1]: https://lore.kernel.org/intel-gfx/?q=b%3A%22fi-kbl-soraka%2Figt%40i915_selftest%40live%40guc.html%22 [2]: https://lore.kernel.org/intel-gfx/?q=b%3A%22fi-kbl-soraka%2Figt%40i915_selftest%40live%40%22 Regards Andrzej > > > Known issues > > Here are the changes found in Patchwork_108371v2 that come from known > issues: > > > IGT changes > > > Issues hit > > * > > igt@gem_huc_copy@huc-copy: > > o fi-kbl-soraka: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#2190 <https://gitlab.freedesktop.org/drm/intel/issues/2190>) > * > > igt@gem_lmem_swapping@basic: > > o fi-kbl-soraka: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271> / i915#4613 <https://gitlab.freedesktop.org/drm/intel/issues/4613>) +3 similar issues > * > > igt@i915_selftest@live@gt_heartbeat: > > o fi-kbl-soraka: NOTRUN -> DMESG-FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html> (i915#5334 <https://gitlab.freedesktop.org/drm/intel/issues/5334>) > * > > igt@i915_selftest@live@gt_pm: > > o fi-kbl-soraka: NOTRUN -> DMESG-FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html> (i915#1886 <https://gitlab.freedesktop.org/drm/intel/issues/1886>) > * > > igt@kms_chamelium_frames@hdmi-crc-fast: > > o fi-kbl-soraka: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +15 similar issues > * > > igt@kms_chamelium_hpd@common-hpd-after-suspend: > > o fi-rkl-guc: NOTRUN -> SKIP > <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-rkl-guc/igt@kms_chamelium_hpd@common-hpd-after-suspend.html> (i915#7828 <https://gitlab.freedesktop.org/drm/intel/issues/7828>) > > > Possible fixes > > * > > igt@i915_selftest@live@gt_lrc: > > o fi-rkl-guc: INCOMPLETE > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html> (i915#4983 <https://gitlab.freedesktop.org/drm/intel/issues/4983>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html> > * > > igt@i915_selftest@live@migrate: > > o {bat-atsm-1}: DMESG-FAIL > <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12573/bat-atsm-1/igt@i915_selftest@live@migrate.html> (i915#7699 <https://gitlab.freedesktop.org/drm/intel/issues/7699>) -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108371v2/bat-atsm-1/igt@i915_selftest@live@migrate.html> > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > > Build changes > > * Linux: CI_DRM_12573 -> Patchwork_108371v2 > > CI-20190529: 20190529 > CI_DRM_12573: 05a39ecbd04e67fb44442617819fc138eca0ac61 @ > git://anongit.freedesktop.org/gfx-ci/linux > IGT_7116: 79eb8984acd309108be713a8831e60667db67e21 @ > https://gitlab.freedesktop.org/drm/igt-gpu-tools.git > Patchwork_108371v2: 05a39ecbd04e67fb44442617819fc138eca0ac61 @ > git://anongit.freedesktop.org/gfx-ci/linux > > > Linux commits > > 01304089b052 drm/i915: implement async_flip mode per plane tracking >
On Wed, Jan 11, 2023 at 05:22:47PM +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 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 > i915 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 > 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) > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 ++++- > drivers/gpu/drm/i915/display/intel_display.c | 7 ++++--- > drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++ > 3 files changed, 11 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 10e1fc9d069827..3f1b1548ede025 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -362,6 +362,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; > @@ -581,8 +582,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); > + } intel_modeset_all_pipes() and intel_color_add_affected_planes() would need to clear this stuff as well. Can't immediately think of other places that need the same treatment (the nv12 plane stuff I believe should be fine without this since we should be rejecting async flips with planar formats). Though I think that is still going to have annoying ping-pong for the the wm/ddb optimizations whenever there are internal sync plane updates injected between async flips. I think to sort that out fully we'd need to start tracking the last uapi async flip state for each plane. > > return 0; > } > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index e75b9b2a0e015a..e1c3b1b0b6a8f1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -1303,7 +1303,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; > @@ -1312,7 +1313,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. > @@ -1429,7 +1430,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, > * WA for platforms where async address update enable bit > * is double buffered and only latched at start of vblank. > */ > - if (old_crtc_state->uapi.async_flip && !new_crtc_state->uapi.async_flip) > + if (old_crtc_state->async_flip_planes & ~new_crtc_state->async_flip_planes) > intel_crtc_async_flip_disable_wa(state, crtc); > } > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 32e8b2fc3cc642..61b1a0ec3dede1 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -1248,6 +1248,9 @@ struct intel_crtc_state { > /* bitmask of planes that will be updated during the commit */ > u8 update_planes; > > + /* bitmask of planes with async flip active */ > + u8 async_flip_planes; > + > u8 framestart_delay; /* 1-4 */ > u8 msa_timing_delay; /* 0-3 */ > > -- > 2.34.1
On 24.01.2023 15:02, Ville Syrjälä wrote: > On Wed, Jan 11, 2023 at 05:22:47PM +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 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 >> i915 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 >> 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) >> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 ++++- >> drivers/gpu/drm/i915/display/intel_display.c | 7 ++++--- >> drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++ >> 3 files changed, 11 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 10e1fc9d069827..3f1b1548ede025 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> @@ -362,6 +362,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; >> @@ -581,8 +582,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); >> + } > > intel_modeset_all_pipes() and intel_color_add_affected_planes() would > need to clear this stuff as well. Can't immediately think of other I am not familiar with the display, so forgive me being verbose in verification of your comments :) 1. In case of intel_modeset_all_pipes for every crtc: crtc_state->async_flip_planes = 0; 2. In case of intel_color_add_affected_planes: for every plane: new_crtc_state->async_flip_planes &= ~BIT(plane->id); Are the changes OK? > places that need the same treatment (the nv12 plane stuff I believe > should be fine without this since we should be rejecting async flips > with planar formats). The driver is quite big and I quickly get lost when lurking into it, so hard to comment it :) > > Though I think that is still going to have annoying ping-pong > for the the wm/ddb optimizations whenever there are internal > sync plane updates injected between async flips. I think to Hmm, I hoped, that since intel_crtc_duplicate_state preserves async_flip_planes ping-pong shouldn't happen. > sort that out fully we'd need to start tracking the last uapi > async flip state for each plane. I hoped async_flip_planes should serve it, modulo it is not fully clear to me when to switch async_flip. Enabling async flip is quite clear (requested by user), but disabling: - full modeset, - sync fb update, - plane disable, - cases mentioned above, - ? Is it OK to send next version with added clearing code in intel_modeset_all_pipes and intel_color_add_affected_planes ? Regards Andrzej > >> >> return 0; >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index e75b9b2a0e015a..e1c3b1b0b6a8f1 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -1303,7 +1303,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; >> @@ -1312,7 +1313,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. >> @@ -1429,7 +1430,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, >> * WA for platforms where async address update enable bit >> * is double buffered and only latched at start of vblank. >> */ >> - if (old_crtc_state->uapi.async_flip && !new_crtc_state->uapi.async_flip) >> + if (old_crtc_state->async_flip_planes & ~new_crtc_state->async_flip_planes) >> intel_crtc_async_flip_disable_wa(state, crtc); >> } >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index 32e8b2fc3cc642..61b1a0ec3dede1 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -1248,6 +1248,9 @@ struct intel_crtc_state { >> /* bitmask of planes that will be updated during the commit */ >> u8 update_planes; >> >> + /* bitmask of planes with async flip active */ >> + u8 async_flip_planes; >> + >> u8 framestart_delay; /* 1-4 */ >> u8 msa_timing_delay; /* 0-3 */ >> >> -- >> 2.34.1 >
On Thu, Jan 26, 2023 at 07:14:30PM +0100, Andrzej Hajda wrote: > On 24.01.2023 15:02, Ville Syrjälä wrote: > > On Wed, Jan 11, 2023 at 05:22:47PM +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 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 > >> i915 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 > >> 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) > >> > >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 ++++- > >> drivers/gpu/drm/i915/display/intel_display.c | 7 ++++--- > >> drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++ > >> 3 files changed, 11 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 10e1fc9d069827..3f1b1548ede025 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> @@ -362,6 +362,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; > >> @@ -581,8 +582,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); > >> + } > > > > intel_modeset_all_pipes() and intel_color_add_affected_planes() would > > need to clear this stuff as well. Can't immediately think of other > > I am not familiar with the display, so forgive me being verbose in > verification of your comments :) > 1. In case of intel_modeset_all_pipes > for every crtc: crtc_state->async_flip_planes = 0; > 2. In case of intel_color_add_affected_planes: > for every plane: new_crtc_state->async_flip_planes &= ~BIT(plane->id); I think I'd do async_flip_planes=0 in both case, and also do_async_flip=false. Guarantees we don't end up in some funny state where some planes would want to do an async update and some other ones a sync update. > Are the changes OK? > > > places that need the same treatment (the nv12 plane stuff I believe > > should be fine without this since we should be rejecting async flips > > with planar formats). > > The driver is quite big and I quickly get lost when lurking into it, so > hard to comment it :) > > > > > Though I think that is still going to have annoying ping-pong > > for the the wm/ddb optimizations whenever there are internal > > sync plane updates injected between async flips. I think to > > Hmm, I hoped, that since intel_crtc_duplicate_state preserves > async_flip_planes ping-pong shouldn't happen. > > > sort that out fully we'd need to start tracking the last uapi > > async flip state for each plane. > > > I hoped async_flip_planes should serve it, modulo it is not fully clear > to me when to switch async_flip. Enabling async flip is quite clear > (requested by user), but disabling: > - full modeset, > - sync fb update, > - plane disable, > - cases mentioned above, > - ? That reminds me that skl_{ddb,wm}_add_affected_planes() should also disable async flips. > > Is it OK to send next version with added clearing code in > intel_modeset_all_pipes and intel_color_add_affected_planes ? Yeah, I think that should be good enough improvement for the moment. > > Regards > Andrzej > > > > >> > >> return 0; > >> } > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> index e75b9b2a0e015a..e1c3b1b0b6a8f1 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -1303,7 +1303,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; > >> @@ -1312,7 +1313,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. > >> @@ -1429,7 +1430,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, > >> * WA for platforms where async address update enable bit > >> * is double buffered and only latched at start of vblank. > >> */ > >> - if (old_crtc_state->uapi.async_flip && !new_crtc_state->uapi.async_flip) > >> + if (old_crtc_state->async_flip_planes & ~new_crtc_state->async_flip_planes) > >> intel_crtc_async_flip_disable_wa(state, crtc); > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > >> index 32e8b2fc3cc642..61b1a0ec3dede1 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > >> @@ -1248,6 +1248,9 @@ struct intel_crtc_state { > >> /* bitmask of planes that will be updated during the commit */ > >> u8 update_planes; > >> > >> + /* bitmask of planes with async flip active */ > >> + u8 async_flip_planes; > >> + > >> u8 framestart_delay; /* 1-4 */ > >> u8 msa_timing_delay; /* 0-3 */ > >> > >> -- > >> 2.34.1 > >
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 10e1fc9d069827..3f1b1548ede025 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -362,6 +362,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; @@ -581,8 +582,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_display.c b/drivers/gpu/drm/i915/display/intel_display.c index e75b9b2a0e015a..e1c3b1b0b6a8f1 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1303,7 +1303,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; @@ -1312,7 +1313,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. @@ -1429,7 +1430,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state, * WA for platforms where async address update enable bit * is double buffered and only latched at start of vblank. */ - if (old_crtc_state->uapi.async_flip && !new_crtc_state->uapi.async_flip) + if (old_crtc_state->async_flip_planes & ~new_crtc_state->async_flip_planes) intel_crtc_async_flip_disable_wa(state, crtc); } diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 32e8b2fc3cc642..61b1a0ec3dede1 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1248,6 +1248,9 @@ struct intel_crtc_state { /* bitmask of planes that will be updated during the commit */ u8 update_planes; + /* bitmask of planes with async flip active */ + u8 async_flip_planes; + u8 framestart_delay; /* 1-4 */ u8 msa_timing_delay; /* 0-3 */
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 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 i915 0000:00:02.0: [drm] *ERROR* Fault errors on pipe A: 0x00000080 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) Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic_plane.c | 5 ++++- drivers/gpu/drm/i915/display/intel_display.c | 7 ++++--- drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++ 3 files changed, 11 insertions(+), 4 deletions(-)