Re: [PATCH 06/14] drm/i915: Reorganize plane register writes to make them more atomic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 01, 2018 at 05:05:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> 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
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  40 +++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  | 114 +++++++++++++++------------
>  2 files changed, 86 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5ce3892d583..9521cff5fb44 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3328,7 +3328,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;
> @@ -3343,41 +3342,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(DSPPOS(i9xx_plane), 0);
>  		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);
>  	} else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> +		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(PRIMPOS(i9xx_plane), 0);
>  		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(DSPTILEOFF(i9xx_plane), (y << 16) | x);
> +		I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> +	}
> +
> +	/*
> +	 * 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);
> -		I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
> -		I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> -	} else {
> +	else
>  		I915_WRITE_FW(DSPADDR(i9xx_plane),
>  			      intel_plane_ggtt_offset(plane_state) +
>  			      dspaddr_offset);
> -	}
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
> @@ -10045,8 +10048,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
> @@ -10056,15 +10059,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 8a40879abe30..455b2d0cbaa6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -404,24 +404,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);
> -
> -	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);
> +		      (plane_state->color_plane[1].offset - surf_addr) |
> +		      aux_stride);
>  
>  	if (icl_is_hdr_plane(plane)) {
>  		u32 cus_ctl = 0;
> @@ -444,15 +432,33 @@ 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);
> +	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);
>  
> +	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);
>  }
>  
> @@ -690,7 +696,6 @@ vlv_update_plane(struct intel_plane *plane,
>  		 const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	enum pipe pipe = plane->pipe;
>  	enum plane_id plane_id = plane->id;
>  	u32 sprctl = plane_state->ctl;
> @@ -715,6 +720,12 @@ 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);
>  
> @@ -723,18 +734,15 @@ vlv_update_plane(struct intel_plane *plane,
>  		I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
>  		I915_WRITE_FW(SPKEYMSK(pipe, plane_id), key->channel_mask);
>  	}
> -	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);
> -
> -	if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> -		I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
> -	else
> -		I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
>  
> -	I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
> +	I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
> +	I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
>  
> -	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);
> @@ -848,7 +856,6 @@ ivb_update_plane(struct intel_plane *plane,
>  		 const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	enum pipe pipe = plane->pipe;
>  	u32 sprctl = plane_state->ctl, sprscale = 0;
>  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> @@ -877,27 +884,32 @@ ivb_update_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> +	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(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);
> -
>  	/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
>  	 * register */
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>  		I915_WRITE_FW(SPROFFSET(pipe), (y << 16) | x);
> -	else if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> +	} else {
>  		I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
> -	else
>  		I915_WRITE_FW(SPRLINOFF(pipe), linear_offset);
> +	}
>  
> -	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);
> @@ -915,7 +927,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);
> @@ -1017,7 +1029,6 @@ g4x_update_plane(struct intel_plane *plane,
>  		 const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	enum pipe pipe = plane->pipe;
>  	u32 dvscntr = plane_state->ctl, dvsscale = 0;
>  	u32 dvssurf_offset = plane_state->color_plane[0].offset;
> @@ -1046,22 +1057,25 @@ g4x_update_plane(struct intel_plane *plane,
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> +	I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
> +	I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> +	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(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);
> -
> -	if (fb->modifier == I915_FORMAT_MOD_X_TILED)

I believe this removed if deserves a separated patch...

the rest was hard to review, but after you pointed out how to check
on bspec the armed-by field I think it all makes sense.

> -		I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
> -	else
> -		I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> +	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);
> +	/*
> +	 * 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux