* [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
@ 2017-09-13 9:25 Maarten Lankhorst
2017-09-13 9:38 ` Lofstedt, Marta
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Maarten Lankhorst @ 2017-09-13 9:25 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed
the call to wait_for_vblanks and replaced it with flip_done.
Unfortunately legacy_cursor_update was unset too late, and the
replacement call drm_atomic_helper_wait_for_flip_done() was
a noop. Make sure that its unset before setup_commit() is
called to fix this issue.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675
Testcase: kms_cursor_crc
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
Cc: Marta Löfstedt <marta.lofstedt@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++++----------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba76b2db939c..c1da48d77c8e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12412,23 +12412,14 @@ static int intel_atomic_commit(struct drm_device *dev,
{
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(dev);
- int ret = 0;
-
- ret = drm_atomic_helper_setup_commit(state, nonblock);
- if (ret)
- return ret;
+ struct drm_crtc_state *new_crtc_state;
+ struct drm_crtc *crtc;
+ int i, ret = 0;
drm_atomic_state_get(state);
i915_sw_fence_init(&intel_state->commit_ready,
intel_atomic_commit_ready);
- ret = intel_atomic_prepare_commit(dev, state);
- if (ret) {
- DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
- i915_sw_fence_commit(&intel_state->commit_ready);
- return ret;
- }
-
/*
* The intel_legacy_cursor_update() fast path takes care
* of avoiding the vblank waits for simple cursor
@@ -12437,19 +12428,32 @@ static int intel_atomic_commit(struct drm_device *dev,
* updates happen during the correct frames. Gen9+ have
* double buffered watermarks and so shouldn't need this.
*
- * Do this after drm_atomic_helper_setup_commit() and
- * intel_atomic_prepare_commit() because we still want
- * to skip the flip and fb cleanup waits. Although that
- * does risk yanking the mapping from under the display
- * engine.
+ * Unset state->legacy_cursor_update before the call to
+ * drm_atomic_helper_setup_commit() because otherwise
+ * drm_atomic_helper_wait_for_flip_done() is a noop and
+ * we get FIFO underruns because we didn't wait
+ * for vblank.
*
* FIXME doing watermarks and fb cleanup from a vblank worker
* (assuming we had any) would solve these problems.
*/
- if (INTEL_GEN(dev_priv) < 9)
- state->legacy_cursor_update = false;
+ if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+ if (to_intel_crtc_state(new_crtc_state)->wm.need_postvbl_update)
+ state->legacy_cursor_update = false;
+ }
+
+ ret = intel_atomic_prepare_commit(dev, state);
+ if (ret) {
+ DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
+ i915_sw_fence_commit(&intel_state->commit_ready);
+ return ret;
+ }
+
+ ret = drm_atomic_helper_setup_commit(state, nonblock);
+ if (!ret)
+ ret = drm_atomic_helper_swap_state(state, true);
- ret = drm_atomic_helper_swap_state(state, true);
if (ret) {
i915_sw_fence_commit(&intel_state->commit_ready);
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
2017-09-13 9:25 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit Maarten Lankhorst
@ 2017-09-13 9:38 ` Lofstedt, Marta
2017-09-13 10:27 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Lofstedt, Marta @ 2017-09-13 9:38 UTC (permalink / raw)
To: Maarten Lankhorst, intel-gfx@lists.freedesktop.org; +Cc: Daniel Vetter
Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> Sent: Wednesday, September 13, 2017 12:26 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Daniel Vetter
> <daniel.vetter@ffwll.ch>; Jani Nikula <jani.nikula@linux.intel.com>; Lofstedt,
> Marta <marta.lofstedt@intel.com>
> Subject: [PATCH] drm/i915: Unset legacy_cursor_update early in
> intel_atomic_commit
>
> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed
> the call to wait_for_vblanks and replaced it with flip_done.
>
> Unfortunately legacy_cursor_update was unset too late, and the
> replacement call drm_atomic_helper_wait_for_flip_done() was a noop.
> Make sure that its unset before setup_commit() is called to fix this issue.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675
> Testcase: kms_cursor_crc
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++++----------
> ------
> 1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ba76b2db939c..c1da48d77c8e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12412,23 +12412,14 @@ static int intel_atomic_commit(struct
> drm_device *dev, {
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(dev);
> - int ret = 0;
> -
> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> - if (ret)
> - return ret;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc *crtc;
> + int i, ret = 0;
>
> drm_atomic_state_get(state);
> i915_sw_fence_init(&intel_state->commit_ready,
> intel_atomic_commit_ready);
>
> - ret = intel_atomic_prepare_commit(dev, state);
> - if (ret) {
> - DRM_DEBUG_ATOMIC("Preparing state failed
> with %i\n", ret);
> - i915_sw_fence_commit(&intel_state-
> >commit_ready);
> - return ret;
> - }
> -
> /*
> * The intel_legacy_cursor_update() fast path takes care
> * of avoiding the vblank waits for simple cursor @@ -12437,19
> +12428,32 @@ static int intel_atomic_commit(struct drm_device *dev,
> * updates happen during the correct frames. Gen9+ have
> * double buffered watermarks and so shouldn't need this.
> *
> - * Do this after drm_atomic_helper_setup_commit() and
> - * intel_atomic_prepare_commit() because we still want
> - * to skip the flip and fb cleanup waits. Although that
> - * does risk yanking the mapping from under the display
> - * engine.
> + * Unset state->legacy_cursor_update before the call to
> + * drm_atomic_helper_setup_commit() because otherwise
> + * drm_atomic_helper_wait_for_flip_done() is a noop and
> + * we get FIFO underruns because we didn't wait
> + * for vblank.
> *
> * FIXME doing watermarks and fb cleanup from a vblank
> worker
> * (assuming we had any) would solve these problems.
> */
> - if (INTEL_GEN(dev_priv) < 9)
> - state->legacy_cursor_update = false;
> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update)
> {
> + for_each_new_crtc_in_state(state, crtc,
> new_crtc_state, i)
> + if
> (to_intel_crtc_state(new_crtc_state)->wm.need_postvbl_update)
> + state-
> >legacy_cursor_update = false;
> + }
> +
> + ret = intel_atomic_prepare_commit(dev, state);
> + if (ret) {
> + DRM_DEBUG_ATOMIC("Preparing state failed
> with %i\n", ret);
> + i915_sw_fence_commit(&intel_state-
> >commit_ready);
> + return ret;
> + }
> +
> + ret = drm_atomic_helper_setup_commit(state, nonblock);
> + if (!ret)
> + ret = drm_atomic_helper_swap_state(state,
> true);
>
> - ret = drm_atomic_helper_swap_state(state, true);
> if (ret) {
> i915_sw_fence_commit(&intel_state-
> >commit_ready);
>
> --
> 2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* ✓ Fi.CI.BAT: success for drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
2017-09-13 9:25 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit Maarten Lankhorst
2017-09-13 9:38 ` Lofstedt, Marta
@ 2017-09-13 10:27 ` Patchwork
2017-09-13 14:03 ` [PATCH] " Ville Syrjälä
2017-09-13 15:39 ` ✓ Fi.CI.IGT: success for " Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-09-13 10:27 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
URL : https://patchwork.freedesktop.org/series/30273/
State : success
== Summary ==
Series 30273v1 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
https://patchwork.freedesktop.org/api/1.0/series/30273/revisions/1/mbox/
Test chamelium:
Subgroup dp-crc-fast:
pass -> FAIL (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
fail -> PASS (fi-snb-2600) fdo#100215
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
fail -> PASS (fi-ivb-3520m) fdo#102699
Test pm_rpm:
Subgroup basic-rte:
pass -> DMESG-WARN (fi-cfl-s) fdo#102294
fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102699 https://bugs.freedesktop.org/show_bug.cgi?id=102699
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:449s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:450s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:375s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:532s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:268s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:507s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:500s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:494s
fi-cfl-s total:289 pass:222 dwarn:35 dfail:0 fail:0 skip:32 time:550s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:447s
fi-glk-2a total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:595s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:427s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:408s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:441s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:481s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:467s
fi-kbl-7500u total:289 pass:263 dwarn:1 dfail:0 fail:1 skip:24 time:478s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:580s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:582s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:547s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:457s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:516s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:502s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:459s
fi-skl-x1585l total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:471s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:569s
fi-snb-2600 total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:429s
29603d09af9af9882a230162fd50674ff94aa05c drm-tip: 2017y-09m-13d-09h-38m-49s UTC integration manifest
9654c6272127 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5677/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
2017-09-13 9:25 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit Maarten Lankhorst
2017-09-13 9:38 ` Lofstedt, Marta
2017-09-13 10:27 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-09-13 14:03 ` Ville Syrjälä
2017-09-13 15:39 ` ✓ Fi.CI.IGT: success for " Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2017-09-13 14:03 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx
On Wed, Sep 13, 2017 at 11:25:54AM +0200, Maarten Lankhorst wrote:
> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed
> the call to wait_for_vblanks and replaced it with flip_done.
>
> Unfortunately legacy_cursor_update was unset too late, and the
> replacement call drm_atomic_helper_wait_for_flip_done() was
> a noop. Make sure that its unset before setup_commit() is
> called to fix this issue.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675
> Testcase: kms_cursor_crc
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba76b2db939c..c1da48d77c8e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12412,23 +12412,14 @@ static int intel_atomic_commit(struct drm_device *dev,
> {
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(dev);
> - int ret = 0;
> -
> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> - if (ret)
> - return ret;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_crtc *crtc;
I'd prefer to see intel_ types.
> + int i, ret = 0;
>
> drm_atomic_state_get(state);
> i915_sw_fence_init(&intel_state->commit_ready,
> intel_atomic_commit_ready);
>
> - ret = intel_atomic_prepare_commit(dev, state);
> - if (ret) {
> - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
> - i915_sw_fence_commit(&intel_state->commit_ready);
> - return ret;
> - }
> -
> /*
> * The intel_legacy_cursor_update() fast path takes care
> * of avoiding the vblank waits for simple cursor
> @@ -12437,19 +12428,32 @@ static int intel_atomic_commit(struct drm_device *dev,
> * updates happen during the correct frames. Gen9+ have
> * double buffered watermarks and so shouldn't need this.
> *
> - * Do this after drm_atomic_helper_setup_commit() and
> - * intel_atomic_prepare_commit() because we still want
> - * to skip the flip and fb cleanup waits. Although that
> - * does risk yanking the mapping from under the display
> - * engine.
> + * Unset state->legacy_cursor_update before the call to
> + * drm_atomic_helper_setup_commit() because otherwise
> + * drm_atomic_helper_wait_for_flip_done() is a noop and
> + * we get FIFO underruns because we didn't wait
> + * for vblank.
> *
> * FIXME doing watermarks and fb cleanup from a vblank worker
> * (assuming we had any) would solve these problems.
> */
> - if (INTEL_GEN(dev_priv) < 9)
> - state->legacy_cursor_update = false;
> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> + if (to_intel_crtc_state(new_crtc_state)->wm.need_postvbl_update)
> + state->legacy_cursor_update = false;
> + }
Hmm. I don't think we can rely on need_postvbl_update at this point.
Pre-g4x code won't set it. We already made the decision to not use the
fast path, so explicitly setting legacy_cursor_update=false should still
be fine, no?
> +
> + ret = intel_atomic_prepare_commit(dev, state);
> + if (ret) {
> + DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
> + i915_sw_fence_commit(&intel_state->commit_ready);
> + return ret;
> + }
> +
> + ret = drm_atomic_helper_setup_commit(state, nonblock);
> + if (!ret)
> + ret = drm_atomic_helper_swap_state(state, true);
>
> - ret = drm_atomic_helper_swap_state(state, true);
> if (ret) {
> i915_sw_fence_commit(&intel_state->commit_ready);
>
> --
> 2.14.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* ✓ Fi.CI.IGT: success for drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
2017-09-13 9:25 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit Maarten Lankhorst
` (2 preceding siblings ...)
2017-09-13 14:03 ` [PATCH] " Ville Syrjälä
@ 2017-09-13 15:39 ` Patchwork
3 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2017-09-13 15:39 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Unset legacy_cursor_update early in intel_atomic_commit
URL : https://patchwork.freedesktop.org/series/30273/
State : success
== Summary ==
Test perf:
Subgroup polling:
pass -> FAIL (shard-hsw) fdo#102252
Test gem_eio:
Subgroup in-flight:
pass -> FAIL (shard-hsw) fdo#102616
Test kms_plane:
Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
skip -> PASS (shard-hsw)
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102616 https://bugs.freedesktop.org/show_bug.cgi?id=102616
shard-hsw total:2419 pass:1321 dwarn:2 dfail:0 fail:15 skip:1081 time:9684s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5677/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-13 15:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-13 9:25 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit Maarten Lankhorst
2017-09-13 9:38 ` Lofstedt, Marta
2017-09-13 10:27 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-09-13 14:03 ` [PATCH] " Ville Syrjälä
2017-09-13 15:39 ` ✓ Fi.CI.IGT: success for " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox