[Intel-gfx] [PATCH] drm/i915: Move vblank wait determination to 'check' phase

[Paulo Zanoni]

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



-- 
Paulo Zanoni