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

[Daniel Vetter]

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 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>

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