Re: [Intel-gfx] [PATCH v3 3/5] drm/i915: Make cursor plane registers unlocked

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, Feb 11, 2022 at 11:26:04AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Drop the locks around cursor plane register writes. The
> lock isn't needed since each plane's register are neatly
> contained on their own cachelines.
> 
> The locking did have a secondary effect of disabling
> interrupts around the cursor registers writes though.
> If we drop that then we open outselves up for sceduling
> delays and whatnot while on the middle of the register
> writes. That increases the chance of not all the register
> writes land during the same frame. For normal atomic
> commits this is not a concern as the vblank evade mechanism
> anyway disables interrupts around the update, but the legacy
> cursor codepath does not. Technically we should do a vblank
> evade there as well, but so far no one has bothered to hook
> that up. So in the meantime let's put an explicit local irq
> disable/enable around the legacy cursor update to keep the
> race window minimal.
> 
> v2: local_irq_{disable,enable}() for legacy cursor ioctl

Guess, this will help our infamous atomic update evasion time exceeeded.
I think I've even checked with similar patch.
Good that its finally will make its way into kernel.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 2ade8fdd9bdd..b648be744cf2 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -255,7 +255,6 @@ static void i845_cursor_update_arm(struct intel_plane *plane,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	u32 cntl = 0, base = 0, pos = 0, size = 0;
> -	unsigned long irqflags;
>  
>  	if (plane_state && plane_state->uapi.visible) {
>  		unsigned int width = drm_rect_width(&plane_state->uapi.dst);
> @@ -270,8 +269,6 @@ static void i845_cursor_update_arm(struct intel_plane *plane,
>  		pos = intel_cursor_position(plane_state);
>  	}
>  
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	/* On these chipsets we can only modify the base/size/stride
>  	 * whilst the cursor is disabled.
>  	 */
> @@ -290,8 +287,6 @@ static void i845_cursor_update_arm(struct intel_plane *plane,
>  	} else {
>  		intel_de_write_fw(dev_priv, CURPOS(PIPE_A), pos);
>  	}
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void i845_cursor_disable_arm(struct intel_plane *plane,
> @@ -492,7 +487,6 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
>  	u32 cntl = 0, base = 0, pos = 0, fbc_ctl = 0;
> -	unsigned long irqflags;
>  
>  	if (plane_state && plane_state->uapi.visible) {
>  		int width = drm_rect_width(&plane_state->uapi.dst);
> @@ -508,8 +502,6 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
>  		pos = intel_cursor_position(plane_state);
>  	}
>  
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	/*
>  	 * On some platforms writing CURCNTR first will also
>  	 * cause CURPOS to be armed by the CURBASE write.
> @@ -555,8 +547,6 @@ static void i9xx_cursor_update_arm(struct intel_plane *plane,
>  		intel_de_write_fw(dev_priv, CURPOS(pipe), pos);
>  		intel_de_write_fw(dev_priv, CURBASE(pipe), base);
>  	}
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void i9xx_cursor_disable_arm(struct intel_plane *plane,
> @@ -715,6 +705,14 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  	 */
>  	crtc_state->active_planes = new_crtc_state->active_planes;
>  
> +	/*
> +	 * Technically we should do a vblank evasion here to make
> +	 * sure all the cursor registers update on the same frame.
> +	 * For now just make sure the register writes happen as
> +	 * quickly as possible to minimize the race window.
> +	 */
> +	local_irq_disable();
> +
>  	if (new_plane_state->uapi.visible) {
>  		intel_plane_update_noarm(plane, crtc_state, new_plane_state);
>  		intel_plane_update_arm(plane, crtc_state, new_plane_state);
> @@ -722,6 +720,8 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>  		intel_plane_disable_arm(plane, crtc_state);
>  	}
>  
> +	local_irq_enable();
> +
>  	intel_plane_unpin_fb(old_plane_state);
>  
>  out_free:
> -- 
> 2.34.1
>