[Intel-gfx] [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Daniel Vetter]

On Wed, Jan 15, 2014 at 10:25:13AM -0800, Jesse Barnes wrote:
> On Thu, 19 Dec 2013 19:12:29 -0200
> Paulo Zanoni <przanoni at gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > 
> > Depending on the HW gen and the connector type, the pipe won't start
> > running right after we call intel_enable_pipe, so that
> > intel_wait_for_vblank call we currently have will just sit there for
> > the full 50ms timeout. So this patch adds an argument that will allow
> > us to avoid the vblank wait in case we want. Currently all the callers
> > still request for the vblank wait, so the behavior should still be the
> > same.
> > 
> > We also added a POSTING_READ on the register: previously
> > intel_wait_for_vblank was acting as a POSTING_READ, but now if
> > wait_for_vblank is false we'll stkip it, so we need an explicit
> > POSTING_READ.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 869be78..6865fa2 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1753,7 +1753,7 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv)
> >   * returning.
> >   */
> >  static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> > -			      bool pch_port, bool dsi)
> > +			      bool pch_port, bool dsi, bool wait_for_vblank)
> >  {
> >  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> >  								      pipe);
> > @@ -1796,7 +1796,9 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> >  		return;
> >  
> >  	I915_WRITE(reg, val | PIPECONF_ENABLE);
> > -	intel_wait_for_vblank(dev_priv->dev, pipe);
> > +	POSTING_READ(reg);
> > +	if (wait_for_vblank)
> > +		intel_wait_for_vblank(dev_priv->dev, pipe);
> >  }
> >  
> >  /**
> > @@ -3558,7 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_update_watermarks(crtc);
> >  	intel_enable_pipe(dev_priv, pipe,
> > -			  intel_crtc->config.has_pch_encoder, false);
> > +			  intel_crtc->config.has_pch_encoder, false, true);
> >  	intel_enable_primary_plane(dev_priv, plane, pipe);
> >  	intel_enable_planes(crtc);
> >  	intel_crtc_update_cursor(crtc, true);
> > @@ -3704,7 +3706,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_update_watermarks(crtc);
> >  	intel_enable_pipe(dev_priv, pipe,
> > -			  intel_crtc->config.has_pch_encoder, false);
> > +			  intel_crtc->config.has_pch_encoder, false, true);
> >  
> >  	if (intel_crtc->config.has_pch_encoder)
> >  		lpt_pch_enable(crtc);
> > @@ -4145,7 +4147,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >  	intel_crtc_load_lut(crtc);
> >  
> >  	intel_update_watermarks(crtc);
> > -	intel_enable_pipe(dev_priv, pipe, false, is_dsi);
> > +	intel_enable_pipe(dev_priv, pipe, false, is_dsi, true);
> >  	intel_enable_primary_plane(dev_priv, plane, pipe);
> >  	intel_enable_planes(crtc);
> >  	intel_crtc_update_cursor(crtc, true);
> > @@ -4183,7 +4185,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  	intel_crtc_load_lut(crtc);
> >  
> >  	intel_update_watermarks(crtc);
> > -	intel_enable_pipe(dev_priv, pipe, false, false);
> > +	intel_enable_pipe(dev_priv, pipe, false, false, true);
> >  	intel_enable_primary_plane(dev_priv, plane, pipe);
> >  	intel_enable_planes(crtc);
> >  	/* The fixup needs to happen before cursor is enabled */
> 
> Looks fine, though I echo Jani's comment.  Might be better to have an
> explicitly named intel_enable_pipe_wait variant to make the code more
> readable (likewise for DSI and PCH probably, if we don't need every
> combination), but that can be a separate cleanup.

Yeah, the 3 bool arguments aren't really fun any more imo. Simplest way
would be to move the vblank wait out into the corresponding platform
crtc_enable implementation. The same could be done with the asserts, which
atm are the only reason we have the other two booleans. With that we have
sanity again and a simpel intel_enable_pipe(dev_priv, pipe).
-Daniel

> 
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch