diff mbox series

[v2,01/13] drm/i915: Reorganize plane register writes to make them more atomic

Message ID 20181114210729.16185-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Program SKL+ watermarks/ddb more carefully | expand

Commit Message

Ville Syrjala Nov. 14, 2018, 9:07 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some observations about the plane registers:
- the control register will self-arm if the plane is not already
  enabled, thus we want to write it as close to (or ideally after)
  the surface register
- tileoff/linoff/offset/aux_offset are self-arming as well so we want
  them close to the surface register as well
- color keying registers we maybe self arming before SKL. Not 100%
  sure but we can try to keep them near to the surface register
  as well
- chv pipe b csc register are double buffered but self arming so
  moving them down a bit
- the rest should be mostly armed by the surface register so we can
  safely write them first, and to just for some consistency let's try
  to follow keep them in order based on the register offset

None of this will have any effect of course unless the vblank evasion
fails (which it still does sometimes). Another potential future benefit
might be pulling the non-self armings registers outside the vblank
evasion since they won't latch until the arming register has been
written. This would make the critical section a bit lighter and thus
less likely to exceed the deadline.

v2: Rebase due to input CSC
v3: Swap LINOFF/TILEOFF and KEYMSK/KEYMAX to actually follow
    the last rule above (Matt)
    Add a bit more rationale to the commit message (Matt)

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  52 ++++++------
 drivers/gpu/drm/i915/intel_sprite.c  | 118 ++++++++++++++++-----------
 2 files changed, 97 insertions(+), 73 deletions(-)

Comments

Matt Roper Nov. 19, 2018, 11:14 p.m. UTC | #1
On Wed, Nov 14, 2018 at 11:07:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Some observations about the plane registers:
> - the control register will self-arm if the plane is not already
>   enabled, thus we want to write it as close to (or ideally after)
>   the surface register
> - tileoff/linoff/offset/aux_offset are self-arming as well so we want
>   them close to the surface register as well
> - color keying registers we maybe self arming before SKL. Not 100%
>   sure but we can try to keep them near to the surface register
>   as well
> - chv pipe b csc register are double buffered but self arming so
>   moving them down a bit
> - the rest should be mostly armed by the surface register so we can
>   safely write them first, and to just for some consistency let's try
>   to follow keep them in order based on the register offset
> 
> None of this will have any effect of course unless the vblank evasion
> fails (which it still does sometimes). Another potential future benefit
> might be pulling the non-self armings registers outside the vblank
> evasion since they won't latch until the arming register has been
> written. This would make the critical section a bit lighter and thus
> less likely to exceed the deadline.
> 
> v2: Rebase due to input CSC
> v3: Swap LINOFF/TILEOFF and KEYMSK/KEYMAX to actually follow
>     the last rule above (Matt)
>     Add a bit more rationale to the commit message (Matt)
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |  52 ++++++------
>  drivers/gpu/drm/i915/intel_sprite.c  | 118 ++++++++++++++++-----------
>  2 files changed, 97 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 132e978227fb..3c760a2eacc8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3314,7 +3314,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
>  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
>  	u32 linear_offset;
>  	u32 dspcntr = plane_state->ctl;
> -	i915_reg_t reg = DSPCNTR(i9xx_plane);
>  	int x = plane_state->color_plane[0].x;
>  	int y = plane_state->color_plane[0].y;
>  	unsigned long irqflags;
> @@ -3329,41 +3328,45 @@ static void i9xx_update_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> +	I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
> +
>  	if (INTEL_GEN(dev_priv) < 4) {
>  		/* pipesrc and dspsize control the size that is scaled from,
>  		 * which should always be the user's requested size.
>  		 */
> -		I915_WRITE_FW(DSPSIZE(i9xx_plane),
> -			      ((crtc_state->pipe_src_h - 1) << 16) |
> -			      (crtc_state->pipe_src_w - 1));
>  		I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
> +		I915_WRITE_FW(DSPSIZE(i9xx_plane),
> +			      ((crtc_state->pipe_src_h - 1) << 16) |
> +			      (crtc_state->pipe_src_w - 1));
>  	} else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> -		I915_WRITE_FW(PRIMSIZE(i9xx_plane),
> -			      ((crtc_state->pipe_src_h - 1) << 16) |
> -			      (crtc_state->pipe_src_w - 1));
>  		I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
> +		I915_WRITE_FW(PRIMSIZE(i9xx_plane),
> +			      ((crtc_state->pipe_src_h - 1) << 16) |
> +			      (crtc_state->pipe_src_w - 1));
>  		I915_WRITE_FW(PRIMCNSTALPHA(i9xx_plane), 0);
>  	}
>  
> -	I915_WRITE_FW(reg, dspcntr);
> -
> -	I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		I915_WRITE_FW(DSPSURF(i9xx_plane),
> -			      intel_plane_ggtt_offset(plane_state) +
> -			      dspaddr_offset);
>  		I915_WRITE_FW(DSPOFFSET(i9xx_plane), (y << 16) | x);
>  	} else if (INTEL_GEN(dev_priv) >= 4) {
> -		I915_WRITE_FW(DSPSURF(i9xx_plane),
> -			      intel_plane_ggtt_offset(plane_state) +
> -			      dspaddr_offset);
> -		I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
>  		I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> -	} else {
> +		I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
> +	}
> +
> +	/*
> +	 * The control register self-arms if the plane was previously
> +	 * disabled. Try to make the plane enable atomic by writing
> +	 * the control register just before the surface register.
> +	 */
> +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> +	if (INTEL_GEN(dev_priv) >= 4)
> +		I915_WRITE_FW(DSPSURF(i9xx_plane),
> +			      intel_plane_ggtt_offset(plane_state) +
> +			      dspaddr_offset);
> +	else
>  		I915_WRITE_FW(DSPADDR(i9xx_plane),
>  			      intel_plane_ggtt_offset(plane_state) +
>  			      dspaddr_offset);
> -	}
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
> @@ -10056,8 +10059,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>  	 * On some platforms writing CURCNTR first will also
>  	 * cause CURPOS to be armed by the CURBASE write.
>  	 * Without the CURCNTR write the CURPOS write would
> -	 * arm itself. Thus we always start the full update
> -	 * with a CURCNTR write.
> +	 * arm itself. Thus we always update CURCNTR before
> +	 * CURPOS.
>  	 *
>  	 * On other platforms CURPOS always requires the
>  	 * CURBASE write to arm the update. Additonally
> @@ -10067,15 +10070,16 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>  	 * cursor that doesn't appear to move, or even change
>  	 * shape. Thus we always write CURBASE.
>  	 *
> -	 * CURCNTR and CUR_FBC_CTL are always
> -	 * armed by the CURBASE write only.
> +	 * The other registers are armed by by the CURBASE write
> +	 * except when the plane is getting enabled at which time
> +	 * the CURCNTR write arms the update.
>  	 */
>  	if (plane->cursor.base != base ||
>  	    plane->cursor.size != fbc_ctl ||
>  	    plane->cursor.cntl != cntl) {
> -		I915_WRITE_FW(CURCNTR(pipe), cntl);
>  		if (HAS_CUR_FBC(dev_priv))
>  			I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl);
> +		I915_WRITE_FW(CURCNTR(pipe), cntl);
>  		I915_WRITE_FW(CURPOS(pipe), pos);
>  		I915_WRITE_FW(CURBASE(pipe), base);
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 5e0f7b575a50..a80773211265 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -508,28 +508,12 @@ skl_program_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> -			      plane_state->color_ctl);
> -
> -	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> -		icl_program_input_csc_coeff(crtc_state, plane_state);
> -
> -	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> -	I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> -	I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
> -
> -	I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
>  	I915_WRITE_FW(PLANE_STRIDE(pipe, plane_id), stride);
> +	I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
>  	I915_WRITE_FW(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
>  	I915_WRITE_FW(PLANE_AUX_DIST(pipe, plane_id),
>  		      (plane_state->color_plane[1].offset - surf_addr) | aux_stride);
>  
> -	if (INTEL_GEN(dev_priv) < 11)
> -		I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
> -			      (plane_state->color_plane[1].y << 16) |
> -			       plane_state->color_plane[1].x);
> -
>  	if (icl_is_hdr_plane(plane)) {
>  		u32 cus_ctl = 0;
>  
> @@ -551,15 +535,36 @@ skl_program_plane(struct intel_plane *plane,
>  		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
>  	}
>  
> -	if (!slave && plane_state->scaler_id >= 0)
> -		skl_program_scaler(plane, crtc_state, plane_state);
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> +			      plane_state->color_ctl);
>  
> -	I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> +	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> +		icl_program_input_csc_coeff(crtc_state, plane_state);
>  
> +	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> +	I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
> +	I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> +
> +	I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
> +
> +	if (INTEL_GEN(dev_priv) < 11)
> +		I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
> +			      (plane_state->color_plane[1].y << 16) |
> +			      plane_state->color_plane[1].x);
> +
> +	/*
> +	 * The control register self-arms if the plane was previously
> +	 * disabled. Try to make the plane enable atomic by writing
> +	 * the control register just before the surface register.
> +	 */
>  	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
>  	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
>  		      intel_plane_ggtt_offset(plane_state) + surf_addr);
>  
> +	if (!slave && plane_state->scaler_id >= 0)
> +		skl_program_scaler(plane, crtc_state, plane_state);
> +
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> @@ -821,24 +826,29 @@ vlv_update_plane(struct intel_plane *plane,
>  
>  	vlv_update_clrc(plane_state);
>  
> +	I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
> +		      plane_state->color_plane[0].stride);
> +	I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> +	I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> +	I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
> +
>  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
>  		chv_update_csc(plane_state);
>  
>  	if (key->flags) {
>  		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
> -		I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
>  		I915_WRITE_FW(SPKEYMSK(pipe, plane_id), key->channel_mask);
> +		I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
>  	}
> -	I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
> -		      plane_state->color_plane[0].stride);
> -	I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
>  
> -	I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
>  	I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
> +	I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
>  
> -	I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
> -
> -	I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> +	/*
> +	 * The control register self-arms if the plane was previously
> +	 * disabled. Try to make the plane enable atomic by writing
> +	 * the control register just before the surface register.
> +	 */
>  	I915_WRITE_FW(SPCNTR(pipe, plane_id), sprctl);
>  	I915_WRITE_FW(SPSURF(pipe, plane_id),
>  		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
> @@ -980,27 +990,32 @@ ivb_update_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	if (key->flags) {
> -		I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
> -		I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
> -		I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
> -	}
> -
>  	I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
>  	I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> +	I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> +	if (IS_IVYBRIDGE(dev_priv))
> +		I915_WRITE_FW(SPRSCALE(pipe), sprscale);
> +
> +	if (key->flags) {
> +		I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
> +		I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
> +		I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
> +	}
>  
>  	/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
>  	 * register */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		I915_WRITE_FW(SPROFFSET(pipe), (y << 16) | x);
>  	} else {
> -		I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
>  		I915_WRITE_FW(SPRLINOFF(pipe), linear_offset);
> +		I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
>  	}
>  
> -	I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> -	if (IS_IVYBRIDGE(dev_priv))
> -		I915_WRITE_FW(SPRSCALE(pipe), sprscale);
> +	/*
> +	 * The control register self-arms if the plane was previously
> +	 * disabled. Try to make the plane enable atomic by writing
> +	 * the control register just before the surface register.
> +	 */
>  	I915_WRITE_FW(SPRCTL(pipe), sprctl);
>  	I915_WRITE_FW(SPRSURF(pipe),
>  		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
> @@ -1018,7 +1033,7 @@ ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
>  	I915_WRITE_FW(SPRCTL(pipe), 0);
> -	/* Can't leave the scaler enabled... */
> +	/* Disable the scaler */
>  	if (IS_IVYBRIDGE(dev_priv))
>  		I915_WRITE_FW(SPRSCALE(pipe), 0);
>  	I915_WRITE_FW(SPRSURF(pipe), 0);
> @@ -1148,20 +1163,25 @@ g4x_update_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	if (key->flags) {
> -		I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
> -		I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
> -		I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
> -	}
> -
>  	I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
>  	I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> -
> -	I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
> -	I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> -
>  	I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
>  	I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
> +
> +	if (key->flags) {
> +		I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
> +		I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
> +		I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
> +	}
> +
> +	I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> +	I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
> +
> +	/*
> +	 * The control register self-arms if the plane was previously
> +	 * disabled. Try to make the plane enable atomic by writing
> +	 * the control register just before the surface register.
> +	 */
>  	I915_WRITE_FW(DVSCNTR(pipe), dvscntr);
>  	I915_WRITE_FW(DVSSURF(pipe),
>  		      intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
> -- 
> 2.18.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 132e978227fb..3c760a2eacc8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3314,7 +3314,6 @@  static void i9xx_update_plane(struct intel_plane *plane,
 	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
 	u32 linear_offset;
 	u32 dspcntr = plane_state->ctl;
-	i915_reg_t reg = DSPCNTR(i9xx_plane);
 	int x = plane_state->color_plane[0].x;
 	int y = plane_state->color_plane[0].y;
 	unsigned long irqflags;
@@ -3329,41 +3328,45 @@  static void i9xx_update_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
+	I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
+
 	if (INTEL_GEN(dev_priv) < 4) {
 		/* pipesrc and dspsize control the size that is scaled from,
 		 * which should always be the user's requested size.
 		 */
-		I915_WRITE_FW(DSPSIZE(i9xx_plane),
-			      ((crtc_state->pipe_src_h - 1) << 16) |
-			      (crtc_state->pipe_src_w - 1));
 		I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
+		I915_WRITE_FW(DSPSIZE(i9xx_plane),
+			      ((crtc_state->pipe_src_h - 1) << 16) |
+			      (crtc_state->pipe_src_w - 1));
 	} else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
-		I915_WRITE_FW(PRIMSIZE(i9xx_plane),
-			      ((crtc_state->pipe_src_h - 1) << 16) |
-			      (crtc_state->pipe_src_w - 1));
 		I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
+		I915_WRITE_FW(PRIMSIZE(i9xx_plane),
+			      ((crtc_state->pipe_src_h - 1) << 16) |
+			      (crtc_state->pipe_src_w - 1));
 		I915_WRITE_FW(PRIMCNSTALPHA(i9xx_plane), 0);
 	}
 
-	I915_WRITE_FW(reg, dspcntr);
-
-	I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		I915_WRITE_FW(DSPSURF(i9xx_plane),
-			      intel_plane_ggtt_offset(plane_state) +
-			      dspaddr_offset);
 		I915_WRITE_FW(DSPOFFSET(i9xx_plane), (y << 16) | x);
 	} else if (INTEL_GEN(dev_priv) >= 4) {
-		I915_WRITE_FW(DSPSURF(i9xx_plane),
-			      intel_plane_ggtt_offset(plane_state) +
-			      dspaddr_offset);
-		I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
 		I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
-	} else {
+		I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
+	}
+
+	/*
+	 * The control register self-arms if the plane was previously
+	 * disabled. Try to make the plane enable atomic by writing
+	 * the control register just before the surface register.
+	 */
+	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
+	if (INTEL_GEN(dev_priv) >= 4)
+		I915_WRITE_FW(DSPSURF(i9xx_plane),
+			      intel_plane_ggtt_offset(plane_state) +
+			      dspaddr_offset);
+	else
 		I915_WRITE_FW(DSPADDR(i9xx_plane),
 			      intel_plane_ggtt_offset(plane_state) +
 			      dspaddr_offset);
-	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
@@ -10056,8 +10059,8 @@  static void i9xx_update_cursor(struct intel_plane *plane,
 	 * On some platforms writing CURCNTR first will also
 	 * cause CURPOS to be armed by the CURBASE write.
 	 * Without the CURCNTR write the CURPOS write would
-	 * arm itself. Thus we always start the full update
-	 * with a CURCNTR write.
+	 * arm itself. Thus we always update CURCNTR before
+	 * CURPOS.
 	 *
 	 * On other platforms CURPOS always requires the
 	 * CURBASE write to arm the update. Additonally
@@ -10067,15 +10070,16 @@  static void i9xx_update_cursor(struct intel_plane *plane,
 	 * cursor that doesn't appear to move, or even change
 	 * shape. Thus we always write CURBASE.
 	 *
-	 * CURCNTR and CUR_FBC_CTL are always
-	 * armed by the CURBASE write only.
+	 * The other registers are armed by by the CURBASE write
+	 * except when the plane is getting enabled at which time
+	 * the CURCNTR write arms the update.
 	 */
 	if (plane->cursor.base != base ||
 	    plane->cursor.size != fbc_ctl ||
 	    plane->cursor.cntl != cntl) {
-		I915_WRITE_FW(CURCNTR(pipe), cntl);
 		if (HAS_CUR_FBC(dev_priv))
 			I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl);
+		I915_WRITE_FW(CURCNTR(pipe), cntl);
 		I915_WRITE_FW(CURPOS(pipe), pos);
 		I915_WRITE_FW(CURBASE(pipe), base);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 5e0f7b575a50..a80773211265 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -508,28 +508,12 @@  skl_program_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
-			      plane_state->color_ctl);
-
-	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
-		icl_program_input_csc_coeff(crtc_state, plane_state);
-
-	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
-	I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
-	I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
-
-	I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
 	I915_WRITE_FW(PLANE_STRIDE(pipe, plane_id), stride);
+	I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 	I915_WRITE_FW(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
 	I915_WRITE_FW(PLANE_AUX_DIST(pipe, plane_id),
 		      (plane_state->color_plane[1].offset - surf_addr) | aux_stride);
 
-	if (INTEL_GEN(dev_priv) < 11)
-		I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
-			      (plane_state->color_plane[1].y << 16) |
-			       plane_state->color_plane[1].x);
-
 	if (icl_is_hdr_plane(plane)) {
 		u32 cus_ctl = 0;
 
@@ -551,15 +535,36 @@  skl_program_plane(struct intel_plane *plane,
 		I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
 	}
 
-	if (!slave && plane_state->scaler_id >= 0)
-		skl_program_scaler(plane, crtc_state, plane_state);
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
+			      plane_state->color_ctl);
 
-	I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
+	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
+		icl_program_input_csc_coeff(crtc_state, plane_state);
 
+	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
+	I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
+	I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
+
+	I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
+
+	if (INTEL_GEN(dev_priv) < 11)
+		I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
+			      (plane_state->color_plane[1].y << 16) |
+			      plane_state->color_plane[1].x);
+
+	/*
+	 * The control register self-arms if the plane was previously
+	 * disabled. Try to make the plane enable atomic by writing
+	 * the control register just before the surface register.
+	 */
 	I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
 	I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
 		      intel_plane_ggtt_offset(plane_state) + surf_addr);
 
+	if (!slave && plane_state->scaler_id >= 0)
+		skl_program_scaler(plane, crtc_state, plane_state);
+
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
@@ -821,24 +826,29 @@  vlv_update_plane(struct intel_plane *plane,
 
 	vlv_update_clrc(plane_state);
 
+	I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
+		      plane_state->color_plane[0].stride);
+	I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
+	I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+	I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
+
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
 		chv_update_csc(plane_state);
 
 	if (key->flags) {
 		I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
-		I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
 		I915_WRITE_FW(SPKEYMSK(pipe, plane_id), key->channel_mask);
+		I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
 	}
-	I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
-		      plane_state->color_plane[0].stride);
-	I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 
-	I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
 	I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
+	I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
 
-	I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
-
-	I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+	/*
+	 * The control register self-arms if the plane was previously
+	 * disabled. Try to make the plane enable atomic by writing
+	 * the control register just before the surface register.
+	 */
 	I915_WRITE_FW(SPCNTR(pipe, plane_id), sprctl);
 	I915_WRITE_FW(SPSURF(pipe, plane_id),
 		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
@@ -980,27 +990,32 @@  ivb_update_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	if (key->flags) {
-		I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
-		I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
-		I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
-	}
-
 	I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
 	I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
+	I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
+	if (IS_IVYBRIDGE(dev_priv))
+		I915_WRITE_FW(SPRSCALE(pipe), sprscale);
+
+	if (key->flags) {
+		I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
+		I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
+		I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
+	}
 
 	/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
 	 * register */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
 		I915_WRITE_FW(SPROFFSET(pipe), (y << 16) | x);
 	} else {
-		I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
 		I915_WRITE_FW(SPRLINOFF(pipe), linear_offset);
+		I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
 	}
 
-	I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
-	if (IS_IVYBRIDGE(dev_priv))
-		I915_WRITE_FW(SPRSCALE(pipe), sprscale);
+	/*
+	 * The control register self-arms if the plane was previously
+	 * disabled. Try to make the plane enable atomic by writing
+	 * the control register just before the surface register.
+	 */
 	I915_WRITE_FW(SPRCTL(pipe), sprctl);
 	I915_WRITE_FW(SPRSURF(pipe),
 		      intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
@@ -1018,7 +1033,7 @@  ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
 	I915_WRITE_FW(SPRCTL(pipe), 0);
-	/* Can't leave the scaler enabled... */
+	/* Disable the scaler */
 	if (IS_IVYBRIDGE(dev_priv))
 		I915_WRITE_FW(SPRSCALE(pipe), 0);
 	I915_WRITE_FW(SPRSURF(pipe), 0);
@@ -1148,20 +1163,25 @@  g4x_update_plane(struct intel_plane *plane,
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	if (key->flags) {
-		I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
-		I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
-		I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
-	}
-
 	I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
 	I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
-
-	I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
-	I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
-
 	I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
 	I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
+
+	if (key->flags) {
+		I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
+		I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
+		I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
+	}
+
+	I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
+	I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
+
+	/*
+	 * The control register self-arms if the plane was previously
+	 * disabled. Try to make the plane enable atomic by writing
+	 * the control register just before the surface register.
+	 */
 	I915_WRITE_FW(DVSCNTR(pipe), dvscntr);
 	I915_WRITE_FW(DVSSURF(pipe),
 		      intel_plane_ggtt_offset(plane_state) + dvssurf_offset);