| Message ID | 1394209951-9963-9-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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. > > 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
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
On Tue, Apr 08, 2014 at 09:55:45PM +0300, Ville Syrjälä wrote: > 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. I'll ignore this one her for now ... -Daniel
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);