On Fri, Feb 11, 2022 at 11:26:04AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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
>
