All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.