| Message ID | 1426716287-13092-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 '*'
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
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
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
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;
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(-)