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
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
