* [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
@ 2017-09-18 10:12 Maarten Lankhorst
2017-09-18 11:31 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (6 more replies)
0 siblings, 7 replies; 11+ messages in thread
From: Maarten Lankhorst @ 2017-09-18 10:12 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.
Changes since v1:
- Force vblank wait for watermarks not yet converted to atomic too. (Ville)
- Use for_each_new_intel_crtc_in_state. (Ville)
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>
Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8599e425abb1..8d051256da1e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
struct drm_i915_private *dev_priv = to_i915(dev);
int ret = 0;
- ret = drm_atomic_helper_setup_commit(state, nonblock);
- if (ret)
- return ret;
-
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
@@ -12540,19 +12529,37 @@ 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) {
+ struct intel_crtc_state *new_crtc_state;
+ struct intel_crtc *crtc;
+ int i;
+
+ for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
+ if (new_crtc_state->wm.need_postvbl_update ||
+ new_crtc_state->update_wm_post)
+ 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] 11+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
@ 2017-09-18 11:31 ` Patchwork
2017-09-18 14:48 ` ✓ Fi.CI.BAT: success " Patchwork
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-09-18 11:31 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
URL : https://patchwork.freedesktop.org/series/30503/
State : failure
== Summary ==
Series 30503v1 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
https://patchwork.freedesktop.org/api/1.0/series/30503/revisions/1/mbox/
Test gem_exec_reloc:
Subgroup basic-cpu-gtt-noreloc:
pass -> DMESG-WARN (fi-kbl-7500u)
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> INCOMPLETE (fi-kbl-7500u)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-legacy:
fail -> PASS (fi-snb-2600) fdo#100215
Test pm_rpm:
Subgroup basic-rte:
dmesg-warn -> PASS (fi-cfl-s) fdo#102294
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
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:448s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:469s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:418s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:514s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:277s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:506s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:496s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:488s
fi-cfl-s total:286 pass:223 dwarn:31 dfail:0 fail:0 skip:31
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:412s
fi-glk-2a total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:604s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:426s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:410s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:429s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:485s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:467s
fi-kbl-7500u total:118 pass:99 dwarn:2 dfail:0 fail:0 skip:16
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:599s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:544s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:452s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:749s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:496s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:474s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:564s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:1 skip:39 time:417s
5299e24e48f3c24e612bcdb997d0dc477cdde0d0 drm-tip: 2017y-09m-18d-08h-44m-15s UTC integration manifest
52dcc46ca752 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5726/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
2017-09-18 11:31 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-09-18 14:48 ` Patchwork
2017-09-18 15:03 ` [PATCH] " Ville Syrjälä
` (4 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-09-18 14:48 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
URL : https://patchwork.freedesktop.org/series/30503/
State : success
== Summary ==
Series 30503v1 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
https://patchwork.freedesktop.org/api/1.0/series/30503/revisions/1/mbox/
Test gem_exec_reloc:
Subgroup basic-cpu-read:
dmesg-warn -> PASS (fi-kbl-7500u)
Subgroup basic-gtt-read:
dmesg-warn -> PASS (fi-kbl-7500u)
Test gem_exec_store:
Subgroup basic-all:
dmesg-warn -> PASS (fi-kbl-7500u)
Test gem_exec_suspend:
Subgroup basic-s3:
incomplete -> PASS (fi-kbl-7500u)
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> FAIL (fi-snb-2600) fdo#100215 +1
Test pm_rpm:
Subgroup basic-rte:
dmesg-warn -> PASS (fi-cfl-s) fdo#102294
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
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:467s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:422s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:517s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:278s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:521s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:491s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:501s
fi-cfl-s total:289 pass:223 dwarn:34 dfail:0 fail:0 skip:32 time:541s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:417s
fi-glk-2a total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:598s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:428s
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:427s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:491s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:466s
fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:474s
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:588s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:593s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:543s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:455s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:746s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:494s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:472s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:563s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:1 skip:39 time:416s
0bbe2f0d6a1e69d0ee72ea0c1dcdd93269419024 drm-tip: 2017y-09m-18d-13h-52m-45s UTC integration manifest
45883bf694b6 drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5731/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
2017-09-18 11:31 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-09-18 14:48 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2017-09-18 15:03 ` Ville Syrjälä
2017-09-19 9:06 ` Maarten Lankhorst
2017-09-18 16:19 ` kbuild test robot
` (3 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2017-09-18 15:03 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx
On Mon, Sep 18, 2017 at 12:12:50PM +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.
>
> Changes since v1:
> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
> - Use for_each_new_intel_crtc_in_state. (Ville)
>
> 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>
> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e425abb1..8d051256da1e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
> struct drm_i915_private *dev_priv = to_i915(dev);
> int ret = 0;
>
> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> - if (ret)
> - return ret;
> -
> 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
> @@ -12540,19 +12529,37 @@ 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) {
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_crtc *crtc;
> + int i;
> +
> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
> + if (new_crtc_state->wm.need_postvbl_update ||
> + new_crtc_state->update_wm_post)
> + state->legacy_cursor_update = false;
Hmm. I guess that's better. But I still don't see why you want to change
this bit of code in this patch. AFAICS it's got nothing to do with the fix
itself, and instead it's just trying to optimize some cursor updates
that were kicked over to the slow path. Or am I missing something?
> + }
> +
> + 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
--
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] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
` (2 preceding siblings ...)
2017-09-18 15:03 ` [PATCH] " Ville Syrjälä
@ 2017-09-18 16:19 ` kbuild test robot
2017-09-18 17:24 ` kbuild test robot
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2017-09-18 16:19 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4992 bytes --]
Hi Maarten,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.14-rc1 next-20170915]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-i915-Unset-legacy_cursor_update-early-in-intel_atomic_commit-v2/20170918-223150
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x003-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'intel_atomic_commit':
>> drivers/gpu/drm/i915/intel_display.c:12608:3: error: implicit declaration of function 'for_each_new_intel_crtc_in_state' [-Werror=implicit-function-declaration]
for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/intel_display.c:12609:4: error: expected ';' before 'if'
if (new_crtc_state->wm.need_postvbl_update ||
^~
cc1: some warnings being treated as errors
vim +/for_each_new_intel_crtc_in_state +12608 drivers/gpu/drm/i915/intel_display.c
12561
12562 /**
12563 * intel_atomic_commit - commit validated state object
12564 * @dev: DRM device
12565 * @state: the top-level driver state object
12566 * @nonblock: nonblocking commit
12567 *
12568 * This function commits a top-level state object that has been validated
12569 * with drm_atomic_helper_check().
12570 *
12571 * RETURNS
12572 * Zero for success or -errno.
12573 */
12574 static int intel_atomic_commit(struct drm_device *dev,
12575 struct drm_atomic_state *state,
12576 bool nonblock)
12577 {
12578 struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
12579 struct drm_i915_private *dev_priv = to_i915(dev);
12580 int ret = 0;
12581
12582 drm_atomic_state_get(state);
12583 i915_sw_fence_init(&intel_state->commit_ready,
12584 intel_atomic_commit_ready);
12585
12586 /*
12587 * The intel_legacy_cursor_update() fast path takes care
12588 * of avoiding the vblank waits for simple cursor
12589 * movement and flips. For cursor on/off and size changes,
12590 * we want to perform the vblank waits so that watermark
12591 * updates happen during the correct frames. Gen9+ have
12592 * double buffered watermarks and so shouldn't need this.
12593 *
12594 * Unset state->legacy_cursor_update before the call to
12595 * drm_atomic_helper_setup_commit() because otherwise
12596 * drm_atomic_helper_wait_for_flip_done() is a noop and
12597 * we get FIFO underruns because we didn't wait
12598 * for vblank.
12599 *
12600 * FIXME doing watermarks and fb cleanup from a vblank worker
12601 * (assuming we had any) would solve these problems.
12602 */
12603 if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
12604 struct intel_crtc_state *new_crtc_state;
12605 struct intel_crtc *crtc;
12606 int i;
12607
12608 for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
12609 if (new_crtc_state->wm.need_postvbl_update ||
12610 new_crtc_state->update_wm_post)
12611 state->legacy_cursor_update = false;
12612 }
12613
12614 ret = intel_atomic_prepare_commit(dev, state);
12615 if (ret) {
12616 DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
12617 i915_sw_fence_commit(&intel_state->commit_ready);
12618 return ret;
12619 }
12620
12621 ret = drm_atomic_helper_setup_commit(state, nonblock);
12622 if (!ret)
12623 ret = drm_atomic_helper_swap_state(state, true);
12624
12625 if (ret) {
12626 i915_sw_fence_commit(&intel_state->commit_ready);
12627
12628 drm_atomic_helper_cleanup_planes(dev, state);
12629 return ret;
12630 }
12631 dev_priv->wm.distrust_bios_wm = false;
12632 intel_shared_dpll_swap_state(state);
12633 intel_atomic_track_fbs(state);
12634
12635 if (intel_state->modeset) {
12636 memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
12637 sizeof(intel_state->min_pixclk));
12638 dev_priv->active_crtcs = intel_state->active_crtcs;
12639 dev_priv->cdclk.logical = intel_state->cdclk.logical;
12640 dev_priv->cdclk.actual = intel_state->cdclk.actual;
12641 }
12642
12643 drm_atomic_state_get(state);
12644 INIT_WORK(&state->commit_work, intel_atomic_commit_work);
12645
12646 i915_sw_fence_commit(&intel_state->commit_ready);
12647 if (nonblock)
12648 queue_work(system_unbound_wq, &state->commit_work);
12649 else
12650 intel_atomic_commit_tail(state);
12651
12652
12653 return 0;
12654 }
12655
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29253 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
` (3 preceding siblings ...)
2017-09-18 16:19 ` kbuild test robot
@ 2017-09-18 17:24 ` kbuild test robot
2017-09-18 18:17 ` ✗ Fi.CI.IGT: warning for " Patchwork
2017-09-26 11:55 ` [PATCH] " Daniel Vetter
6 siblings, 0 replies; 11+ messages in thread
From: kbuild test robot @ 2017-09-18 17:24 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 10631 bytes --]
Hi Maarten,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.14-rc1 next-20170918]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-i915-Unset-legacy_cursor_update-early-in-intel_atomic_commit-v2/20170918-223150
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x012-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'intel_atomic_commit':
drivers/gpu/drm/i915/intel_display.c:12608:3: error: implicit declaration of function 'for_each_new_intel_crtc_in_state' [-Werror=implicit-function-declaration]
for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/dmi.h:4,
from drivers/gpu/drm/i915/intel_display.c:27:
include/linux/compiler.h:156:2: error: expected ';' before 'if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers/gpu/drm/i915/intel_display.c:12609:4: note: in expansion of macro 'if'
if (new_crtc_state->wm.need_postvbl_update ||
^~
drivers/gpu/drm/i915/intel_display.c: At top level:
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'strcpy' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:390:2: note: in expansion of macro 'if'
if (p_size == (size_t)-1 && q_size == (size_t)-1)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:380:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:378:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:369:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:367:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:358:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:356:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:348:2: note: in expansion of macro 'if'
if (p_size < size || q_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:345:3: note: in expansion of macro 'if'
if (q_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:343:3: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:342:2: note: in expansion of macro 'if'
vim +/if +12609 drivers/gpu/drm/i915/intel_display.c
12561
12562 /**
12563 * intel_atomic_commit - commit validated state object
12564 * @dev: DRM device
12565 * @state: the top-level driver state object
12566 * @nonblock: nonblocking commit
12567 *
12568 * This function commits a top-level state object that has been validated
12569 * with drm_atomic_helper_check().
12570 *
12571 * RETURNS
12572 * Zero for success or -errno.
12573 */
12574 static int intel_atomic_commit(struct drm_device *dev,
12575 struct drm_atomic_state *state,
12576 bool nonblock)
12577 {
12578 struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
12579 struct drm_i915_private *dev_priv = to_i915(dev);
12580 int ret = 0;
12581
12582 drm_atomic_state_get(state);
12583 i915_sw_fence_init(&intel_state->commit_ready,
12584 intel_atomic_commit_ready);
12585
12586 /*
12587 * The intel_legacy_cursor_update() fast path takes care
12588 * of avoiding the vblank waits for simple cursor
12589 * movement and flips. For cursor on/off and size changes,
12590 * we want to perform the vblank waits so that watermark
12591 * updates happen during the correct frames. Gen9+ have
12592 * double buffered watermarks and so shouldn't need this.
12593 *
12594 * Unset state->legacy_cursor_update before the call to
12595 * drm_atomic_helper_setup_commit() because otherwise
12596 * drm_atomic_helper_wait_for_flip_done() is a noop and
12597 * we get FIFO underruns because we didn't wait
12598 * for vblank.
12599 *
12600 * FIXME doing watermarks and fb cleanup from a vblank worker
12601 * (assuming we had any) would solve these problems.
12602 */
12603 if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
12604 struct intel_crtc_state *new_crtc_state;
12605 struct intel_crtc *crtc;
12606 int i;
12607
12608 for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
12609 if (new_crtc_state->wm.need_postvbl_update ||
12610 new_crtc_state->update_wm_post)
12611 state->legacy_cursor_update = false;
12612 }
12613
12614 ret = intel_atomic_prepare_commit(dev, state);
12615 if (ret) {
12616 DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
12617 i915_sw_fence_commit(&intel_state->commit_ready);
12618 return ret;
12619 }
12620
12621 ret = drm_atomic_helper_setup_commit(state, nonblock);
12622 if (!ret)
12623 ret = drm_atomic_helper_swap_state(state, true);
12624
12625 if (ret) {
12626 i915_sw_fence_commit(&intel_state->commit_ready);
12627
12628 drm_atomic_helper_cleanup_planes(dev, state);
12629 return ret;
12630 }
12631 dev_priv->wm.distrust_bios_wm = false;
12632 intel_shared_dpll_swap_state(state);
12633 intel_atomic_track_fbs(state);
12634
12635 if (intel_state->modeset) {
12636 memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
12637 sizeof(intel_state->min_pixclk));
12638 dev_priv->active_crtcs = intel_state->active_crtcs;
12639 dev_priv->cdclk.logical = intel_state->cdclk.logical;
12640 dev_priv->cdclk.actual = intel_state->cdclk.actual;
12641 }
12642
12643 drm_atomic_state_get(state);
12644 INIT_WORK(&state->commit_work, intel_atomic_commit_work);
12645
12646 i915_sw_fence_commit(&intel_state->commit_ready);
12647 if (nonblock)
12648 queue_work(system_unbound_wq, &state->commit_work);
12649 else
12650 intel_atomic_commit_tail(state);
12651
12652
12653 return 0;
12654 }
12655
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24861 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✗ Fi.CI.IGT: warning for drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
` (4 preceding siblings ...)
2017-09-18 17:24 ` kbuild test robot
@ 2017-09-18 18:17 ` Patchwork
2017-09-26 11:55 ` [PATCH] " Daniel Vetter
6 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-09-18 18:17 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
URL : https://patchwork.freedesktop.org/series/30503/
State : warning
== Summary ==
Test perf:
Subgroup blocking:
pass -> FAIL (shard-hsw) fdo#102252
Test drv_module_reload:
Subgroup basic-reload-inject:
dmesg-warn -> PASS (shard-hsw) fdo#102707
Test kms_setmode:
Subgroup basic:
fail -> PASS (shard-hsw) fdo#99912
Test kms_flip:
Subgroup blt-wf_vblank-vs-modeset-interruptible:
pass -> DMESG-WARN (shard-hsw)
Test kms_plane:
Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
skip -> PASS (shard-hsw)
Test pm_rpm:
Subgroup gem-mmap-gtt:
pass -> SKIP (shard-hsw)
Test kms_atomic_interruptible:
Subgroup legacy-pageflip:
pass -> SKIP (shard-hsw)
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
shard-hsw total:2313 pass:1242 dwarn:1 dfail:0 fail:13 skip:1057 time:9693s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5731/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 15:03 ` [PATCH] " Ville Syrjälä
@ 2017-09-19 9:06 ` Maarten Lankhorst
2017-09-19 10:24 ` Ville Syrjälä
0 siblings, 1 reply; 11+ messages in thread
From: Maarten Lankhorst @ 2017-09-19 9:06 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx
Op 18-09-17 om 17:03 schreef Ville Syrjälä:
> On Mon, Sep 18, 2017 at 12:12:50PM +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.
>>
>> Changes since v1:
>> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
>> - Use for_each_new_intel_crtc_in_state. (Ville)
>>
>> 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>
>> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
>> 1 file changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8599e425abb1..8d051256da1e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>> struct drm_i915_private *dev_priv = to_i915(dev);
>> int ret = 0;
>>
>> - ret = drm_atomic_helper_setup_commit(state, nonblock);
>> - if (ret)
>> - return ret;
>> -
>> 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
>> @@ -12540,19 +12529,37 @@ 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) {
>> + struct intel_crtc_state *new_crtc_state;
>> + struct intel_crtc *crtc;
>> + int i;
>> +
>> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
>> + if (new_crtc_state->wm.need_postvbl_update ||
>> + new_crtc_state->update_wm_post)
>> + state->legacy_cursor_update = false;
> Hmm. I guess that's better. But I still don't see why you want to change
> this bit of code in this patch. AFAICS it's got nothing to do with the fix
> itself, and instead it's just trying to optimize some cursor updates
> that were kicked over to the slow path. Or am I missing something?
We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it..
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-19 9:06 ` Maarten Lankhorst
@ 2017-09-19 10:24 ` Ville Syrjälä
2017-09-19 11:58 ` Maarten Lankhorst
0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2017-09-19 10:24 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx
On Tue, Sep 19, 2017 at 11:06:52AM +0200, Maarten Lankhorst wrote:
> Op 18-09-17 om 17:03 schreef Ville Syrjälä:
> > On Mon, Sep 18, 2017 at 12:12:50PM +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.
> >>
> >> Changes since v1:
> >> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
> >> - Use for_each_new_intel_crtc_in_state. (Ville)
> >>
> >> 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>
> >> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
> >> 1 file changed, 26 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8599e425abb1..8d051256da1e 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
> >> struct drm_i915_private *dev_priv = to_i915(dev);
> >> int ret = 0;
> >>
> >> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> >> - if (ret)
> >> - return ret;
> >> -
> >> 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
> >> @@ -12540,19 +12529,37 @@ 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) {
> >> + struct intel_crtc_state *new_crtc_state;
> >> + struct intel_crtc *crtc;
> >> + int i;
> >> +
> >> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
> >> + if (new_crtc_state->wm.need_postvbl_update ||
> >> + new_crtc_state->update_wm_post)
> >> + state->legacy_cursor_update = false;
> > Hmm. I guess that's better. But I still don't see why you want to change
> > this bit of code in this patch. AFAICS it's got nothing to do with the fix
> > itself, and instead it's just trying to optimize some cursor updates
> > that were kicked over to the slow path. Or am I missing something?
>
> We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it..
IMO any regression fix should ideally get us back exactly where we were.
--
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] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-19 10:24 ` Ville Syrjälä
@ 2017-09-19 11:58 ` Maarten Lankhorst
0 siblings, 0 replies; 11+ messages in thread
From: Maarten Lankhorst @ 2017-09-19 11:58 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx
Op 19-09-17 om 12:24 schreef Ville Syrjälä:
> On Tue, Sep 19, 2017 at 11:06:52AM +0200, Maarten Lankhorst wrote:
>> Op 18-09-17 om 17:03 schreef Ville Syrjälä:
>>> On Mon, Sep 18, 2017 at 12:12:50PM +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.
>>>>
>>>> Changes since v1:
>>>> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
>>>> - Use for_each_new_intel_crtc_in_state. (Ville)
>>>>
>>>> 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>
>>>> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
>>>> 1 file changed, 26 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 8599e425abb1..8d051256da1e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>>> int ret = 0;
>>>>
>>>> - ret = drm_atomic_helper_setup_commit(state, nonblock);
>>>> - if (ret)
>>>> - return ret;
>>>> -
>>>> 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
>>>> @@ -12540,19 +12529,37 @@ 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) {
>>>> + struct intel_crtc_state *new_crtc_state;
>>>> + struct intel_crtc *crtc;
>>>> + int i;
>>>> +
>>>> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
>>>> + if (new_crtc_state->wm.need_postvbl_update ||
>>>> + new_crtc_state->update_wm_post)
>>>> + state->legacy_cursor_update = false;
>>> Hmm. I guess that's better. But I still don't see why you want to change
>>> this bit of code in this patch. AFAICS it's got nothing to do with the fix
>>> itself, and instead it's just trying to optimize some cursor updates
>>> that were kicked over to the slow path. Or am I missing something?
>> We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it..
> IMO any regression fix should ideally get us back exactly where we were.
>
Ok I'll send it out as separate patch then..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
` (5 preceding siblings ...)
2017-09-18 18:17 ` ✗ Fi.CI.IGT: warning for " Patchwork
@ 2017-09-26 11:55 ` Daniel Vetter
6 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2017-09-26 11:55 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Daniel Vetter, intel-gfx
On Mon, Sep 18, 2017 at 12:12:50PM +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.
>
> Changes since v1:
> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
> - Use for_each_new_intel_crtc_in_state. (Ville)
>
> 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>
> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
I think the proper fix is switching over to the async plane helpers and
unconditionally always clearing the legacy cursor hack. Maybe even
outright removing it, since it doesn't really work all that well with most
drivers.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e425abb1..8d051256da1e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
> struct drm_i915_private *dev_priv = to_i915(dev);
> int ret = 0;
>
> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> - if (ret)
> - return ret;
> -
> 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
> @@ -12540,19 +12529,37 @@ 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) {
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_crtc *crtc;
> + int i;
> +
> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
> + if (new_crtc_state->wm.need_postvbl_update ||
> + new_crtc_state->update_wm_post)
> + 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-09-26 11:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-18 10:12 [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2 Maarten Lankhorst
2017-09-18 11:31 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-09-18 14:48 ` ✓ Fi.CI.BAT: success " Patchwork
2017-09-18 15:03 ` [PATCH] " Ville Syrjälä
2017-09-19 9:06 ` Maarten Lankhorst
2017-09-19 10:24 ` Ville Syrjälä
2017-09-19 11:58 ` Maarten Lankhorst
2017-09-18 16:19 ` kbuild test robot
2017-09-18 17:24 ` kbuild test robot
2017-09-18 18:17 ` ✗ Fi.CI.IGT: warning for " Patchwork
2017-09-26 11:55 ` [PATCH] " Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.