Re: [PATCH] drm/i915: Move vblank wait determination to 'check' phase

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx