| Message ID | 1426706107-1695-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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);