Re: [PATCH 13/15] drm/i915: Eliminate rmw from .update_primary_plane()

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Jun 05, 2014 at 07:16:02PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Move the entire DSPCNTR register setup into the .update_primary_plane()
> functions. That's where it belongs anyway and it'll also help 830M which
> has the extra problem that plane registers reads will return the value
> latched at the last vblank, not the value that was last written.
> 
> Also move DSPPOS and DSPSIZE setup there.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The code changes here look good to me.  You may want to reword/clarify
in the commit message exactly what "move entire register setup" means
since I didn't really know what you meant by that phrase until I looked
at the code and realized you were talking about starting to build the
new value for DSPCNTR which had previously started in *_crtc_enable().  

Looking at these *_update_primary_plane() functions makes me wonder
whether it might help future review if we were to add another patch for
these functions that reorders the bit setting to match the order they
appear in the docs.  Maybe also drop in a code comment in for each of
the bits that are described in the PRM, but that we're purposely just
leaving to a default value of 0 (180 rotation, async update, etc.) so
that it's clear to the reader that we expect the implicit assignment of
0 rather than overlooking overlooking we should have been programming.

Anyway, the code changes here look good, so with or without my
suggestions this is
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_display.c | 110 +++++++++++------------------------
>  1 file changed, 34 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4f02464..74bbab9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2431,20 +2431,31 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb;
> -	struct drm_i915_gem_object *obj;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
>  	u32 dspcntr;
> -	u32 reg;
> +	u32 reg = DSPCNTR(plane);
>  
> -	intel_fb = to_intel_framebuffer(fb);
> -	obj = intel_fb->obj;
> +	dspcntr = DISPPLANE_GAMMA_ENABLE;
> +
> +	if (intel_crtc->primary_enabled)
> +		dspcntr |= DISPLAY_PLANE_ENABLE;
> +
> +	if (INTEL_INFO(dev)->gen < 4) {
> +		if (intel_crtc->pipe == PIPE_B)
> +			dspcntr |= DISPPLANE_SEL_PIPE_B;
> +
> +		/* pipesrc and dspsize control the size that is scaled from,
> +		 * which should always be the user's requested size.
> +		 */
> +		I915_WRITE(DSPSIZE(plane),
> +			   ((intel_crtc->config.pipe_src_h - 1) << 16) |
> +			   (intel_crtc->config.pipe_src_w - 1));
> +		I915_WRITE(DSPPOS(plane), 0);
> +	}
>  
> -	reg = DSPCNTR(plane);
> -	dspcntr = I915_READ(reg);
> -	/* Mask out pixel format bits in case we change it */
> -	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -2476,12 +2487,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  		BUG();
>  	}
>  
> -	if (INTEL_INFO(dev)->gen >= 4) {
> -		if (obj->tiling_mode != I915_TILING_NONE)
> -			dspcntr |= DISPPLANE_TILED;
> -		else
> -			dspcntr &= ~DISPPLANE_TILED;
> -	}
> +	if (INTEL_INFO(dev)->gen >= 4 &&
> +	    obj->tiling_mode != I915_TILING_NONE)
> +		dspcntr |= DISPPLANE_TILED;
>  
>  	if (IS_G4X(dev))
>  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> @@ -2521,20 +2529,21 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_framebuffer *intel_fb;
> -	struct drm_i915_gem_object *obj;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	int plane = intel_crtc->plane;
>  	unsigned long linear_offset;
>  	u32 dspcntr;
> -	u32 reg;
> +	u32 reg = DSPCNTR(plane);
>  
> -	intel_fb = to_intel_framebuffer(fb);
> -	obj = intel_fb->obj;
> +	dspcntr = DISPPLANE_GAMMA_ENABLE;
> +
> +	if (intel_crtc->primary_enabled)
> +		dspcntr |= DISPLAY_PLANE_ENABLE;
> +
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
>  
> -	reg = DSPCNTR(plane);
> -	dspcntr = I915_READ(reg);
> -	/* Mask out pixel format bits in case we change it */
> -	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -2564,12 +2573,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  
>  	if (obj->tiling_mode != I915_TILING_NONE)
>  		dspcntr |= DISPPLANE_TILED;
> -	else
> -		dspcntr &= ~DISPPLANE_TILED;
>  
> -	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> -		dspcntr &= ~DISPPLANE_TRICKLE_FEED_DISABLE;
> -	else
> +	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
>  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>  
>  	I915_WRITE(reg, dspcntr);
> @@ -4000,7 +4005,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	enum plane plane = intel_crtc->plane;
>  
>  	WARN_ON(!crtc->enabled);
>  
> @@ -4022,10 +4026,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	ironlake_set_pipeconf(crtc);
>  
> -	/* Set up the display plane register */
> -	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
> -	POSTING_READ(DSPCNTR(plane));
> -
>  	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
>  					       crtc->x, crtc->y);
>  
> @@ -4113,7 +4113,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	enum plane plane = intel_crtc->plane;
>  
>  	WARN_ON(!crtc->enabled);
>  
> @@ -4134,10 +4133,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_set_pipe_csc(crtc);
>  
> -	/* Set up the display plane register */
> -	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE | DISPPLANE_PIPE_CSC_ENABLE);
> -	POSTING_READ(DSPCNTR(plane));
> -
>  	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
>  					       crtc->x, crtc->y);
>  
> @@ -4623,9 +4618,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	int plane = intel_crtc->plane;
>  	bool is_dsi;
> -	u32 dspcntr;
>  
>  	WARN_ON(!crtc->enabled);
>  
> @@ -4634,27 +4627,13 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	vlv_prepare_pll(intel_crtc);
>  
> -	/* Set up the display plane register */
> -	dspcntr = DISPPLANE_GAMMA_ENABLE;
> -
>  	if (intel_crtc->config.has_dp_encoder)
>  		intel_dp_set_m_n(intel_crtc);
>  
>  	intel_set_pipe_timings(intel_crtc);
>  
> -	/* pipesrc and dspsize control the size that is scaled from,
> -	 * which should always be the user's requested size.
> -	 */
> -	I915_WRITE(DSPSIZE(plane),
> -		   ((intel_crtc->config.pipe_src_h - 1) << 16) |
> -		   (intel_crtc->config.pipe_src_w - 1));
> -	I915_WRITE(DSPPOS(plane), 0);
> -
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	I915_WRITE(DSPCNTR(plane), dspcntr);
> -	POSTING_READ(DSPCNTR(plane));
> -
>  	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
>  					       crtc->x, crtc->y);
>  
> @@ -4711,8 +4690,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	int plane = intel_crtc->plane;
> -	u32 dspcntr;
>  
>  	WARN_ON(!crtc->enabled);
>  
> @@ -4721,32 +4698,13 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	i9xx_set_pll_dividers(intel_crtc);
>  
> -	/* Set up the display plane register */
> -	dspcntr = DISPPLANE_GAMMA_ENABLE;
> -
> -	if (pipe == 0)
> -		dspcntr &= ~DISPPLANE_SEL_PIPE_MASK;
> -	else
> -		dspcntr |= DISPPLANE_SEL_PIPE_B;
> -
>  	if (intel_crtc->config.has_dp_encoder)
>  		intel_dp_set_m_n(intel_crtc);
>  
>  	intel_set_pipe_timings(intel_crtc);
>  
> -	/* pipesrc and dspsize control the size that is scaled from,
> -	 * which should always be the user's requested size.
> -	 */
> -	I915_WRITE(DSPSIZE(plane),
> -		   ((intel_crtc->config.pipe_src_h - 1) << 16) |
> -		   (intel_crtc->config.pipe_src_w - 1));
> -	I915_WRITE(DSPPOS(plane), 0);
> -
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	I915_WRITE(DSPCNTR(plane), dspcntr);
> -	POSTING_READ(DSPCNTR(plane));
> -
>  	dev_priv->display.update_primary_plane(crtc, crtc->primary->fb,
>  					       crtc->x, crtc->y);
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795