Re: [PATCH 08/24] drm/i915: Shuffle wait_for_vblank out of primary_enable/disable funcs

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

On Mon, Apr 07, 2014 at 05:27:41PM -0300, Paulo Zanoni wrote:
> 2014-03-07 13:32 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rather than have a wait_for_vblank() in the primary plane enable/disable
> > funcs, move the wait_for_vblank() to happen after enabling/disabling all
> > planes.
> 
> Why exactly? What is improved? Are we solving a bug? What are the
> risks? What's the problem with the current code? Did you check the
> modeset sequence documentation of every single platform (since you
> changed them all) to make sure this is safe?

Just another step towards getting all the planes enabled/disabled
atomically during a modeset. I should have probably yanked it from
this series since it shouldn't be strictly needed for the watermark
stuff. At least I can't think of a reason now. I guess I included
it here just to make things a bit more difficult for the new watermark
update mechanism.

In any case there's nothing magical about the primary plane, so we
shouldn't treat it as such. That's my excuse for this patch anyway.

> Please update the commit message with the answers.
> 
> Also, we should probably update the first comment of hsw_enable_ips.
> It seems things have changed since it was written.

I thought I had. Ah no, that's part of the mmio vs. cs flip race series.
Looks like the wait for vblank changes there will conflict with this
stuff anyway. I'll have to revisit that series, and hopefully get it
merged before this series so that we can ignore this patch entirely.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2815351..4986887 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1899,7 +1899,6 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
> >
> >         I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
> >         intel_flush_primary_plane(dev_priv, plane);
> > -       intel_wait_for_vblank(dev_priv->dev, pipe);
> >  }
> >
> >  /**
> > @@ -1927,7 +1926,6 @@ static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
> >
> >         I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
> >         intel_flush_primary_plane(dev_priv, plane);
> > -       intel_wait_for_vblank(dev_priv->dev, pipe);
> >  }
> >
> >  static bool need_vtd_wa(struct drm_device *dev)
> > @@ -3550,6 +3548,7 @@ static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
> >         intel_enable_primary_plane(dev_priv, plane, pipe);
> >         intel_enable_planes(crtc);
> >         intel_crtc_update_cursor(crtc, true);
> > +       intel_wait_for_vblank(dev, pipe);
> >
> >         hsw_enable_ips(intel_crtc);
> >
> > @@ -3579,6 +3578,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
> >         intel_crtc_update_cursor(crtc, false);
> >         intel_disable_planes(crtc);
> >         intel_disable_primary_plane(dev_priv, plane, pipe);
> > +       intel_wait_for_vblank(dev, pipe);
> >  }
> >
> >  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > @@ -4211,6 +4211,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >         intel_enable_primary_plane(dev_priv, plane, pipe);
> >         intel_enable_planes(crtc);
> >         intel_crtc_update_cursor(crtc, true);
> > +       intel_wait_for_vblank(dev, pipe);
> >
> >         intel_update_fbc(dev);
> >
> > @@ -4258,6 +4259,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >
> >         /* Give the overlay scaler a chance to enable if it's on this pipe */
> >         intel_crtc_dpms_overlay(intel_crtc, true);
> > +       intel_wait_for_vblank(dev, pipe);
> >
> >         intel_update_fbc(dev);
> >
> > @@ -4308,6 +4310,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> >         intel_crtc_update_cursor(crtc, false);
> >         intel_disable_planes(crtc);
> >         intel_disable_primary_plane(dev_priv, plane, pipe);
> > +       intel_wait_for_vblank(dev, pipe);
> >
> >         intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
> >         intel_disable_pipe(dev_priv, pipe);
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC