* [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled @ 2015-03-18 19:15 Paulo Zanoni 2015-03-18 22:01 ` Matt Roper 0 siblings, 1 reply; 8+ messages in thread From: Paulo Zanoni @ 2015-03-18 19:15 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni From: Paulo Zanoni <paulo.r.zanoni@intel.com> 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ä <ville.syrjala@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- 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. diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f1c0295..f2f7e81 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -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); -- 2.1.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled 2015-03-18 19:15 [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Paulo Zanoni @ 2015-03-18 22:01 ` Matt Roper 2015-03-18 22:04 ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper 2015-03-19 16:31 ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter 0 siblings, 2 replies; 8+ messages in thread From: Matt Roper @ 2015-03-18 22:01 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Wed, Mar 18, 2015 at 04:15:07PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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ä <ville.syrjala@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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 > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f1c0295..f2f7e81 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -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); > -- > 2.1.4 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/i915: Move vblank wait determination to 'check' phase 2015-03-18 22:01 ` Matt Roper @ 2015-03-18 22:04 ` Matt Roper 2015-03-19 11:43 ` shuang.he 2015-03-19 19:16 ` Paulo Zanoni 2015-03-19 16:31 ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter 1 sibling, 2 replies; 8+ messages in thread From: Matt Roper @ 2015-03-18 22:04 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni 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 <paulo.r.zanoni@intel.com> Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com> 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 <matthew.d.roper@intel.com> --- 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(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index a828736..fad1d9f 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -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 Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase 2015-03-18 22:04 ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper @ 2015-03-19 11:43 ` shuang.he 2015-03-19 19:16 ` Paulo Zanoni 1 sibling, 0 replies; 8+ messages in thread From: shuang.he @ 2015-03-19 11:43 UTC (permalink / raw) To: shuang.he, ethan.gao, intel-gfx, matthew.d.roper Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) 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 Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase 2015-03-18 22:04 ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper 2015-03-19 11:43 ` shuang.he @ 2015-03-19 19:16 ` Paulo Zanoni 2015-03-19 19:36 ` Ville Syrjälä 1 sibling, 1 reply; 8+ messages in thread From: Paulo Zanoni @ 2015-03-19 19:16 UTC (permalink / raw) To: Matt Roper; +Cc: Intel Graphics Development, Paulo Zanoni 2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>: > 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 <matthew.d.roper@intel.com> 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) > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > 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 <matthew.d.roper@intel.com> > --- > 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 <paulo.r.zanoni@intel.com> 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 :) > > drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index a828736..fad1d9f 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -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 > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase 2015-03-19 19:16 ` Paulo Zanoni @ 2015-03-19 19:36 ` Ville Syrjälä 2015-03-20 10:22 ` Daniel Vetter 0 siblings, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2015-03-19 19:36 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni On Thu, Mar 19, 2015 at 04:16:41PM -0300, Paulo Zanoni wrote: > 2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>: > > 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 <matthew.d.roper@intel.com> > 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) > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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 <matthew.d.roper@intel.com> > > --- > > 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 <paulo.r.zanoni@intel.com> > > 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. > > > > > drivers/gpu/drm/i915/intel_sprite.c | 24 ++++++++++-------------- > > 1 file changed, 10 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index a828736..fad1d9f 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -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 > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase 2015-03-19 19:36 ` Ville Syrjälä @ 2015-03-20 10:22 ` Daniel Vetter 0 siblings, 0 replies; 8+ messages in thread From: Daniel Vetter @ 2015-03-20 10:22 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Intel Graphics Development, Paulo Zanoni On Thu, Mar 19, 2015 at 09:36:28PM +0200, Ville Syrjälä wrote: > On Thu, Mar 19, 2015 at 04:16:41PM -0300, Paulo Zanoni wrote: > > 2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>: > > > 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 <matthew.d.roper@intel.com> > > 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) > > > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > 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 <matthew.d.roper@intel.com> > > > --- > > > 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 <paulo.r.zanoni@intel.com> Queued for -next, thanks for the patch. > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled 2015-03-18 22:01 ` Matt Roper 2015-03-18 22:04 ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper @ 2015-03-19 16:31 ` Daniel Vetter 1 sibling, 0 replies; 8+ messages in thread From: Daniel Vetter @ 2015-03-19 16:31 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx, Paulo Zanoni On Wed, Mar 18, 2015 at 03:01:22PM -0700, Matt Roper wrote: > On Wed, Mar 18, 2015 at 04:15:07PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > 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ä <ville.syrjala@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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. 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-03-20 10:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-18 19:15 [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Paulo Zanoni 2015-03-18 22:01 ` Matt Roper 2015-03-18 22:04 ` [PATCH] drm/i915: Move vblank wait determination to 'check' phase Matt Roper 2015-03-19 11:43 ` shuang.he 2015-03-19 19:16 ` Paulo Zanoni 2015-03-19 19:36 ` Ville Syrjälä 2015-03-20 10:22 ` Daniel Vetter 2015-03-19 16:31 ` [RFC] drm/i915: don't wait_for_vblank if the CRTC is disabled Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox