Re: [PATCH 1/3] drm/i915: add wait_for_vblank argument to intel_enable_pipe

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@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@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);

A general bikeshed: adding plenty of boolean parameters makes the code
annoying to read. A bit mask with named constants makes it nicer. I'm
not saying you need to do this now; just something to keep in mind.

Cheers,
Jani.



>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center