Skip to contentSkip to conversation

drm/i915: don't wait_for_vblank if the CRTC is disabled

RFC In review Paulo Zanoni 5 participants 8 messages Mar 18, 2015 – Mar 20, 2015

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Paulo Zanoni

author
#0 lore raw

RFC 1/1 drm/i915: don't wait_for_vblank if the CRTC is disabled

From: Paulo Zanoni <[email protected]>

Otherwise we'll get a WARN from drm_wait_one_vblank() saying that vblanks are not available (since they were already disabled in crtc_disable()).

This is certainly a regresison, but QA couldn't bisect it due to other regressions breaking the bisect.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Cc Ville Syrjälä <[email protected]>
Cc Matt Roper <[email protected]>
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I'm not really sure if this is the best way to fix the regression. Ville and/or Matt should provide some comments here.

drivers/gpu/drm/i915/intel_display.c +1 -1
index f1c0295..f2f7e81 100644
@@ -12193,7 +12193,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_runtime_pm_put(dev_priv);
 
-	if (intel_crtc->atomic.wait_vblank)
+	if (intel_crtc->active && intel_crtc->atomic.wait_vblank)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
Signature · 6 lines
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw
Quoted 17 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Otherwise we'll get a WARN from drm_wait_one_vblank() saying that vblanks are not available (since they were already disabled in crtc_disable()).

This is certainly a regresison, but QA couldn't bisect it due to other regressions breaking the bisect.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Cc Ville Syrjälä <[email protected]>
Cc Matt Roper <[email protected]>
Signed-off-by Paulo Zanoni <[email protected]>
Quoted 8 lines
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I'm not really sure if this is the best way to fix the regression. Ville and/or Matt should provide some comments here.

This will definitely fix the problem (we shouldn't be waiting for vblank with a disabled CRTC!), but I think the true bug in our code is that our sprite commit function is setting some bits that should have been set back in the 'check' phase under the 'if (intel_crtc->active)' branch.

I'll supply a patch shortly that I think should fix how we got into this situation in the first place. The whole 'wait_for_vblank' flag is something we should be able to get rid of completely once I finish the atomic watermark work.

Matt

Quoted 17 lines
drivers/gpu/drm/i915/intel_display.c +1 -1
index f1c0295..f2f7e81 100644
@@ -12193,7 +12193,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_runtime_pm_put(dev_priv);
 
-	if (intel_crtc->atomic.wait_vblank)
+	if (intel_crtc->active && intel_crtc->atomic.wait_vblank)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
Signature · 1 line
2.1.4
Signature · 9 lines
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#2 lore raw

PATCH 1/1 drm/i915: Move vblank wait determination to 'check' phase

Determining whether we'll need to wait for vblanks is something we should determine during the atomic 'check' phase, not the 'commit' phase. Note that we only set these bits in the branch of 'check' where intel_crtc->active is true so that we don't try to wait on a disabled CRTC.

The whole 'wait for vblank after update' flag should go away in the future, once we start handling watermarks in a proper atomic manner.

Cc Paulo Zanoni <[email protected]>
Root-cause-analysis-by Paulo Zanoni <[email protected]>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Signed-off-by Matt Roper <[email protected]>

---
Paulo, can you test this patch and see if it solves the problem? The only platform I have access to (IVB) doesn't have runtime power management, so I can't actually run the relevant testcases myself.

 drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +10 -14
index a828736..fad1d9f 100644
@@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	/*
-	 * Avoid underruns when disabling the sprite.
-	 * FIXME remove once watermark updates are done properly.
-	 */
-	intel_crtc->atomic.wait_vblank = true;
-	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int

@@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 
 	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-	/*
-	 * Avoid underruns when disabling the sprite.
-	 * FIXME remove once watermark updates are done properly.
-	 */
-	intel_crtc->atomic.wait_vblank = true;
-	intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 /**

@@ -1262,6 +1248,16 @@ finish:
 		    plane->state->fb->modifier[0] !=
 		    state->base.fb->modifier[0])
 			intel_crtc->atomic.update_wm = true;
+
+		if (!state->visible) {
+			/*
+			 * Avoid underruns when disabling the sprite.
+			 * FIXME remove once watermark updates are done properly.
+			 */
+			intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.update_sprite_watermarks |=
+				(1 << drm_plane_index(plane));
+		}
 	}
 
 	return 0;
Signature · 6 lines
1.8.5.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-by on v1 1/1
#3 lore raw

Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase

Tested-by PRC QA PRTS (Patch Regression Test System Contact: [email protected])

Task id: 5999 -------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied

PNV                                  272/272              272/272
ILK                                  301/301              301/301
SNB                                  303/303              303/303
IVB                                  342/342              342/342
BYT                                  287/287              287/287
HSW                                  362/362              362/362
BDW                                  308/308              308/308

-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

author Tested-by on v1 1/1
#4 lore raw

Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase

In reply to Matt Roper

2015-03-18 19:04 GMT-03:00 Matt Roper <[email protected]>:

Quoted 8 lines

Determining whether we'll need to wait for vblanks is something we should determine during the atomic 'check' phase, not the 'commit' phase. Note that we only set these bits in the branch of 'check' where intel_crtc->active is true so that we don't try to wait on a disabled CRTC.

The whole 'wait for vblank after update' flag should go away in the future, once we start handling watermarks in a proper atomic manner.

Add this to the commit message:

Regression introduced by:

commit 2fdd7def16dd7580f297827930126c16b152ec11

Author: Matt Roper <[email protected]>
Date:   Wed Mar 4 10:49:04 2015 -0800

drm/i915: Don't clobber plane state on internal disables

(at least according to QA's bisect)

Quoted 8 lines
Cc Paulo Zanoni <[email protected]>
Root-cause-analysis-by Paulo Zanoni <[email protected]>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Quoted 5 lines
Signed-off-by Matt Roper <[email protected]>

---
Paulo, can you test this patch and see if it solves the problem? The only platform I have access to (IVB) doesn't have runtime power management, so I can't actually run the relevant testcases myself.

Yes, this patch makes the WARN go away on my BDW machine :)

Tested-by Paulo Zanoni <[email protected]>

One of the possible problems with this patch is that, before it, the wait_vblank was only set for ILK-BDW, but now we set it for all platforms. Do we really want this? Otherwise, it looks good, although I haven't been closely following the atomic rework to be able to assert this is really according to the grand plans. Daniel/Ville could comment here :)

Quoted 60 lines
 drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +10 -15
index a828736..fad1d9f 100644
@@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        I915_WRITE(SPRSURF(pipe), 0);
 
        intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-       /*
-        * Avoid underruns when disabling the sprite.
-        * FIXME remove once watermark updates are done properly.
-        */
-       intel_crtc->atomic.wait_vblank = true;
-       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int

@@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        I915_WRITE(DVSSURF(pipe), 0);
 
        intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-       /*
-        * Avoid underruns when disabling the sprite.
-        * FIXME remove once watermark updates are done properly.
-        */
-       intel_crtc->atomic.wait_vblank = true;
-       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 /**

@@ -1262,6 +1248,16 @@ finish:
                    plane->state->fb->modifier[0] !=
                    state->base.fb->modifier[0])
                        intel_crtc->atomic.update_wm = true;
+
+               if (!state->visible) {
+                       /*
+                        * Avoid underruns when disabling the sprite.
+                        * FIXME remove once watermark updates are done properly.
+                        */
+                       intel_crtc->atomic.wait_vblank = true;
+                       intel_crtc->atomic.update_sprite_watermarks |=
+                               (1 << drm_plane_index(plane));
+               }
        }
 
        return 0;
--

1.8.5.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#5 lore raw

Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase

Quoted 36 lines · Paulo Zanoni

2015-03-18 19:04 GMT-03:00 Matt Roper <[email protected]>:

Quoted 8 lines

Determining whether we'll need to wait for vblanks is something we should determine during the atomic 'check' phase, not the 'commit' phase. Note that we only set these bits in the branch of 'check' where intel_crtc->active is true so that we don't try to wait on a disabled CRTC.

The whole 'wait for vblank after update' flag should go away in the future, once we start handling watermarks in a proper atomic manner.

Add this to the commit message:

Regression introduced by:

commit 2fdd7def16dd7580f297827930126c16b152ec11

Author: Matt Roper <[email protected]>
Date:   Wed Mar 4 10:49:04 2015 -0800

drm/i915: Don't clobber plane state on internal disables

(at least according to QA's bisect)

Quoted 8 lines
Cc Paulo Zanoni <[email protected]>
Root-cause-analysis-by Paulo Zanoni <[email protected]>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Quoted 5 lines
Signed-off-by Matt Roper <[email protected]>

---
Paulo, can you test this patch and see if it solves the problem? The only platform I have access to (IVB) doesn't have runtime power management, so I can't actually run the relevant testcases myself.

Yes, this patch makes the WARN go away on my BDW machine :)

Quoted 8 lines
Tested-by Paulo Zanoni <[email protected]>

One of the possible problems with this patch is that, before it, the wait_vblank was only set for ILK-BDW, but now we set it for all platforms. Do we really want this? Otherwise, it looks good, although I haven't been closely following the atomic rework to be able to assert this is really according to the grand plans. Daniel/Ville could comment here :)

Somethins seems a bit off to me if we end up calling .disable_plane() with a disabled pipe. So fixing things so that won't happen might be a better approach.

Quoted 70 lines
Quoted 61 lines

 drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)
drivers/gpu/drm/i915/intel_sprite.c +10 -15
index a828736..fad1d9f 100644
@@ -747,13 +747,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        I915_WRITE(SPRSURF(pipe), 0);
 
        intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-       /*
-        * Avoid underruns when disabling the sprite.
-        * FIXME remove once watermark updates are done properly.
-        */
-       intel_crtc->atomic.wait_vblank = true;
-       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 static int

@@ -937,13 +930,6 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
        I915_WRITE(DVSSURF(pipe), 0);
 
        intel_flush_primary_plane(dev_priv, intel_crtc->plane);
-
-       /*
-        * Avoid underruns when disabling the sprite.
-        * FIXME remove once watermark updates are done properly.
-        */
-       intel_crtc->atomic.wait_vblank = true;
-       intel_crtc->atomic.update_sprite_watermarks |= (1 << drm_plane_index(plane));
 }
 
 /**

@@ -1262,6 +1248,16 @@ finish:
                    plane->state->fb->modifier[0] !=
                    state->base.fb->modifier[0])
                        intel_crtc->atomic.update_wm = true;
+
+               if (!state->visible) {
+                       /*
+                        * Avoid underruns when disabling the sprite.
+                        * FIXME remove once watermark updates are done properly.
+                        */
+                       intel_crtc->atomic.wait_vblank = true;
+                       intel_crtc->atomic.update_sprite_watermarks |=
+                               (1 << drm_plane_index(plane));
+               }
        }
 
        return 0;
--

1.8.5.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Applied
#6 lore raw

Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase

Quoted 36 lines · Ville Syrjälä
Quoted 35 lines · Paulo Zanoni

2015-03-18 19:04 GMT-03:00 Matt Roper <[email protected]>:

Quoted 8 lines

Determining whether we'll need to wait for vblanks is something we should determine during the atomic 'check' phase, not the 'commit' phase. Note that we only set these bits in the branch of 'check' where intel_crtc->active is true so that we don't try to wait on a disabled CRTC.

The whole 'wait for vblank after update' flag should go away in the future, once we start handling watermarks in a proper atomic manner.

Add this to the commit message:

Regression introduced by:

commit 2fdd7def16dd7580f297827930126c16b152ec11

Author: Matt Roper <[email protected]>
Date:   Wed Mar 4 10:49:04 2015 -0800

drm/i915: Don't clobber plane state on internal disables

(at least according to QA's bisect)

Quoted 13 lines
Cc Paulo Zanoni <[email protected]>
Root-cause-analysis-by Paulo Zanoni <[email protected]>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Signed-off-by Matt Roper <[email protected]>

---
Paulo, can you test this patch and see if it solves the problem? The only platform I have access to (IVB) doesn't have runtime power management, so I can't actually run the relevant testcases myself.

Quoted 2 lines
Quoted 2 lines

Yes, this patch makes the WARN go away on my BDW machine :)

Tested-by Paulo Zanoni <[email protected]>

Queued for -next, thanks for the patch.

Quoted 10 lines
Quoted 6 lines

One of the possible problems with this patch is that, before it, the wait_vblank was only set for ILK-BDW, but now we set it for all platforms. Do we really want this? Otherwise, it looks good, although I haven't been closely following the atomic rework to be able to assert this is really according to the grand plans. Daniel/Ville could comment here :)

Somethins seems a bit off to me if we end up calling .disable_plane() with a disabled pipe. So fixing things so that won't happen might be a better approach.

Well we have some serious trouble still in these paths with the set_base stuck somewhere in the middle where it shouldn't really hang out when everything is off. So definitely more work to do in any case. -Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#7 lore raw
In reply to Matt Roper
Quoted 32 lines · Matt Roper
Quoted 17 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Otherwise we'll get a WARN from drm_wait_one_vblank() saying that vblanks are not available (since they were already disabled in crtc_disable()).

This is certainly a regresison, but QA couldn't bisect it due to other regressions breaking the bisect.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89550
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms

Cc Ville Syrjälä <[email protected]>
Cc Matt Roper <[email protected]>
Signed-off-by Paulo Zanoni <[email protected]>
Quoted 8 lines
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I'm not really sure if this is the best way to fix the regression. Ville and/or Matt should provide some comments here.

This will definitely fix the problem (we shouldn't be waiting for vblank with a disabled CRTC!), but I think the true bug in our code is that our sprite commit function is setting some bits that should have been set back in the 'check' phase under the 'if (intel_crtc->active)' branch.

Quoted 4 lines

I'll supply a patch shortly that I think should fix how we got into this situation in the first place. The whole 'wait_for_vblank' flag is something we should be able to get rid of completely once I finish the atomic watermark work.

Yeah because of runtime PM we shouldn't do anything really for plane updates when the crtc is off. Unnecessary vblank waits here really smell like the canary, not the cause.
-Daniel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx