2015-03-18 19:04 GMT-03:00 Matt Roper <matthew.d.roper@xxxxxxxxx>:
> 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@xxxxxxxxx>
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@xxxxxxxxx>
> Root-cause-analysis-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
