[PATCH] drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm

[PATCH] drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm

[George Karagounis]

There was a problem with the i845 and i9xx cursor update functions
they handled both position updates and control/base/size updates in
a single armed sequence. This prevented the cursor position from
being updated asynchronously, which can make the cursor feel laggy

To fix this i split the updates into two phases
1. noarm Calculates and writes the CURPOS register immediately.
2. arm Calculates and writes CURCNTR, CURBASE, and CURSIZE to
   latch the control and memory updates at vblank

Also handled a hardware quirk on i9xx platforms where CURPOS requires
a CURBASE write to arm the update. The arm phase now writes CURBASE
even if only the position changed

This resolves two inline TODOs and improves cursor responsiveness

Signed-off-by: George Karagounis <mail@taterr.org>
---
 drivers/gpu/drm/i915/display/intel_cursor.c | 44 +++++++++++++++------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index 0673f16f6fd0..c12a7222f2f6 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -270,14 +270,27 @@ static int i845_check_cursor(struct intel_crtc_state *crtc_state,
 	return 0;
 }
 
-/* TODO: split into noarm+arm pair */
+static void i845_cursor_update_noarm(struct intel_dsb *dsb,
+				     struct intel_plane *plane,
+				     const struct intel_crtc_state *crtc_state,
+				     const struct intel_plane_state *plane_state)
+{
+	struct intel_display *display = to_intel_display(plane);
+	u32 pos = 0;
+
+	if (plane_state && plane_state->uapi.visible)
+		pos = intel_cursor_position(crtc_state, plane_state, false);
+
+	intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
+}
+
 static void i845_cursor_update_arm(struct intel_dsb *dsb,
 				   struct intel_plane *plane,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct intel_plane_state *plane_state)
 {
 	struct intel_display *display = to_intel_display(plane);
-	u32 cntl = 0, base = 0, pos = 0, size = 0;
+	u32 cntl = 0, base = 0, size = 0;
 
 	if (plane_state && plane_state->uapi.visible) {
 		unsigned int width = drm_rect_width(&plane_state->uapi.dst);
@@ -289,7 +302,6 @@ static void i845_cursor_update_arm(struct intel_dsb *dsb,
 		size = CURSOR_HEIGHT(height) | CURSOR_WIDTH(width);
 
 		base = plane_state->surf;
-		pos = intel_cursor_position(crtc_state, plane_state, false);
 	}
 
 	/* On these chipsets we can only modify the base/size/stride
@@ -301,14 +313,11 @@ static void i845_cursor_update_arm(struct intel_dsb *dsb,
 		intel_de_write_fw(display, CURCNTR(display, PIPE_A), 0);
 		intel_de_write_fw(display, CURBASE(display, PIPE_A), base);
 		intel_de_write_fw(display, CURSIZE(display, PIPE_A), size);
-		intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
 		intel_de_write_fw(display, CURCNTR(display, PIPE_A), cntl);
 
 		plane->cursor.base = base;
 		plane->cursor.size = size;
 		plane->cursor.cntl = cntl;
-	} else {
-		intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
 	}
 }
 
@@ -645,7 +654,21 @@ static void skl_write_cursor_wm(struct intel_dsb *dsb,
 			   skl_cursor_ddb_reg_val(ddb));
 }
 
-/* TODO: split into noarm+arm pair */
+static void i9xx_cursor_update_noarm(struct intel_dsb *dsb,
+				     struct intel_plane *plane,
+				     const struct intel_crtc_state *crtc_state,
+				     const struct intel_plane_state *plane_state)
+{
+	struct intel_display *display = to_intel_display(plane);
+	enum pipe pipe = plane->pipe;
+	u32 pos = 0;
+
+	if (plane_state && plane_state->uapi.visible)
+		pos = intel_cursor_position(crtc_state, plane_state, false);
+
+	intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
+}
+
 static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
 				   struct intel_plane *plane,
 				   const struct intel_crtc_state *crtc_state,
@@ -653,7 +676,7 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
 {
 	struct intel_display *display = to_intel_display(plane);
 	enum pipe pipe = plane->pipe;
-	u32 cntl = 0, base = 0, pos = 0, fbc_ctl = 0;
+	u32 cntl = 0, base = 0, fbc_ctl = 0;
 
 	if (plane_state && plane_state->uapi.visible) {
 		int width = drm_rect_width(&plane_state->uapi.dst);
@@ -666,7 +689,6 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
 			fbc_ctl = CUR_FBC_EN | CUR_FBC_HEIGHT(height - 1);
 
 		base = plane_state->surf;
-		pos = intel_cursor_position(crtc_state, plane_state, false);
 	}
 
 	/*
@@ -703,14 +725,12 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
 		if (HAS_CUR_FBC(display))
 			intel_de_write_dsb(display, dsb, CUR_FBC_CTL(display, pipe), fbc_ctl);
 		intel_de_write_dsb(display, dsb, CURCNTR(display, pipe), cntl);
-		intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
 		intel_de_write_dsb(display, dsb, CURBASE(display, pipe), base);
 
 		plane->cursor.base = base;
 		plane->cursor.size = fbc_ctl;
 		plane->cursor.cntl = cntl;
 	} else {
-		intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
 		intel_de_write_dsb(display, dsb, CURBASE(display, pipe), base);
 	}
 }
@@ -1019,6 +1039,7 @@ intel_cursor_plane_create(struct intel_display *display,
 	if (display->platform.i845g || display->platform.i865g) {
 		cursor->max_stride = i845_cursor_max_stride;
 		cursor->min_alignment = i845_cursor_min_alignment;
+		cursor->update_noarm = i845_cursor_update_noarm;
 		cursor->update_arm = i845_cursor_update_arm;
 		cursor->disable_arm = i845_cursor_disable_arm;
 		cursor->get_hw_state = i845_cursor_get_hw_state;
@@ -1036,6 +1057,7 @@ intel_cursor_plane_create(struct intel_display *display,
 		if (intel_scanout_needs_vtd_wa(display))
 			cursor->vtd_guard = 2;
 
+		cursor->update_noarm = i9xx_cursor_update_noarm;
 		cursor->update_arm = i9xx_cursor_update_arm;
 		cursor->disable_arm = i9xx_cursor_disable_arm;
 		cursor->get_hw_state = i9xx_cursor_get_hw_state;
-- 
2.55.0

Re: [PATCH] drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915: atomic KMS violation and i9xx hardware quirk trigger in cursor update
- [Medium] drm/i915: missing CURPOS reprogram during i845 cursor shape change
--

commit 80d531685d42e74f0f8ac2b8f9867c83e1f1a6b8
Author: George Karagounis <taterraster@gmail.com>
Date:   Sat Sep 19 17:39:14 2026 +0000

    drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm
    
    There was a problem with the i845 and i9xx cursor update functions
    they handled both position updates and control/base/size updates in
    a single armed sequence. This prevented the cursor position from
    being updated asynchronously, which can make the cursor feel laggy
    
    To fix this i split the updates into two phases
    1. noarm Calculates and writes the CURPOS register immediately.
    2. arm Calculates and writes CURCNTR, CURBASE, and CURSIZE to
       latch the control and memory updates at vblank

> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 4afd275de86ea..bcf00b78a71b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -271,14 +271,27 @@ static int i845_check_cursor(struct intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> -/* TODO: split into noarm+arm pair */
> +static void i845_cursor_update_noarm(struct intel_dsb *dsb,
> +				     struct intel_plane *plane,
> +				     const struct intel_crtc_state *crtc_state,
> +				     const struct intel_plane_state *plane_state)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	u32 pos = 0;
> +
> +	if (plane_state && plane_state->uapi.visible)
> +		pos = intel_cursor_position(crtc_state, plane_state, false);
> +
> +	intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
> +}
> +

[Severity: High]
Does writing CURPOS in the noarm phase break atomic KMS synchronization?

Writing self-arming registers in the noarm phase can cause visible tearing
when updating the cursor position and image simultaneously.

[ ... ]

>  static void i845_cursor_update_arm(struct intel_dsb *dsb,
>  				   struct intel_plane *plane,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state)
>  {
>  	struct intel_display *display = to_intel_display(plane);
> -	u32 cntl = 0, base = 0, pos = 0, size = 0;
> +	u32 cntl = 0, base = 0, size = 0;

[ ... ]

>  	/* On these chipsets we can only modify the base/size/stride
>  	 * whilst the cursor is disabled.
>  	 */
>  	if (plane->cursor.base != base ||
>  	    plane->cursor.size != size ||
>  	    plane->cursor.cntl != cntl) {
>  		intel_de_write_fw(display, CURCNTR(display, PIPE_A), 0);
>  		intel_de_write_fw(display, CURBASE(display, PIPE_A), base);
>  		intel_de_write_fw(display, CURSIZE(display, PIPE_A), size);
> -		intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
>  		intel_de_write_fw(display, CURCNTR(display, PIPE_A), cntl);

[Severity: Medium]
Can the hardware lose the cursor position here?

The CURPOS write was removed from this atomic disabled-update sequence.
Updating the cursor base, size, or control on an i845 platform triggers a
temporary disable/enable cycle, and omitting the CURPOS reprogramming while
the cursor is temporarily disabled might cause the position to be lost.

[ ... ]

> +static void i9xx_cursor_update_noarm(struct intel_dsb *dsb,
> +				     struct intel_plane *plane,
> +				     const struct intel_crtc_state *crtc_state,
> +				     const struct intel_plane_state *plane_state)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	enum pipe pipe = plane->pipe;
> +	u32 pos = 0;
> +
> +	if (plane_state && plane_state->uapi.visible)
> +		pos = intel_cursor_position(crtc_state, plane_state, false);
> +
> +	intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
> +}

[Severity: High]
Does prematurely writing CURPOS trigger the i9xx self-arming quirk?

Because CURCNTR has not yet been written in the arm phase, writing CURPOS
here causes it to arm itself prematurely. It gets canceled later by writes
to CURCNTR during the arm phase, resulting in dropped cursor position
updates.

[ ... ]

>  static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
>  				   struct intel_plane *plane,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state)
>  {

[ ... ]

>  	/*
>  	 * 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 update CURCNTR before
>  	 * CURPOS.
>  	 *
>  	 * On other platforms CURPOS always requires the
>  	 * CURBASE write to arm the update. Additionally
>  	 * a write to any of the cursor register will cancel
>  	 * an already armed cursor update. Thus leaving out
>  	 * the CURBASE write after CURPOS could lead to a
>  	 * cursor that doesn't appear to move, or even change
>  	 * shape. Thus we always write CURBASE.

[Severity: High]
Does the new update sequence violate the documented hardware quirk?

The hardware quirk documentation warns against the exact sequence introduced
by this patch. Since the commit message explicitly states the intent to make
cursor position updates asynchronous, this appears to be a violation of the
atomic KMS design.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919173914.111150-1-mail@taterr.org?part=1

Re: [PATCH] drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm

[Ville Syrjälä]

On Sat, Sep 19, 2026 at 08:39:14PM +0300, George Karagounis wrote:
> There was a problem with the i845 and i9xx cursor update functions
> they handled both position updates and control/base/size updates in
> a single armed sequence. This prevented the cursor position from
> being updated asynchronously, which can make the cursor feel laggy

The noarm vs. arm split has nothing to do with that. What it does is
(slightly) reduce the amount of work we have to do inside the vblank
evasion critical section. And in order to do the split one has to
evaluate each an every register to confirm whether they are self
arming or not.

And as for the cursor we can't really do that because of the mailbox
updates being performed from the legacy cursor path. That is, when
performing mailbox updates the non-arming registers must also be
updated during the vblank evasion critical section or else they might
disarm the arming that was done by a previous update in the same frame.
The full legacy cursor fastpath would actually work fine there because
it does both the noarm+arm inside the critical section, but the
non-fastpath route for legacy_cursor_update==true does not so it
would need additional changes.

> 
> To fix this i split the updates into two phases
> 1. noarm Calculates and writes the CURPOS register immediately.
> 2. arm Calculates and writes CURCNTR, CURBASE, and CURSIZE to
>    latch the control and memory updates at vblank
> 
> Also handled a hardware quirk on i9xx platforms where CURPOS requires
> a CURBASE write to arm the update. The arm phase now writes CURBASE
> even if only the position changed
> 
> This resolves two inline TODOs and improves cursor responsiveness
> 
> Signed-off-by: George Karagounis <mail@taterr.org>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c | 44 +++++++++++++++------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 0673f16f6fd0..c12a7222f2f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -270,14 +270,27 @@ static int i845_check_cursor(struct intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> -/* TODO: split into noarm+arm pair */
> +static void i845_cursor_update_noarm(struct intel_dsb *dsb,
> +				     struct intel_plane *plane,
> +				     const struct intel_crtc_state *crtc_state,
> +				     const struct intel_plane_state *plane_state)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	u32 pos = 0;
> +
> +	if (plane_state && plane_state->uapi.visible)
> +		pos = intel_cursor_position(crtc_state, plane_state, false);
> +
> +	intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
> +}
> +
>  static void i845_cursor_update_arm(struct intel_dsb *dsb,
>  				   struct intel_plane *plane,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state)
>  {
>  	struct intel_display *display = to_intel_display(plane);
> -	u32 cntl = 0, base = 0, pos = 0, size = 0;
> +	u32 cntl = 0, base = 0, size = 0;
>  
>  	if (plane_state && plane_state->uapi.visible) {
>  		unsigned int width = drm_rect_width(&plane_state->uapi.dst);
> @@ -289,7 +302,6 @@ static void i845_cursor_update_arm(struct intel_dsb *dsb,
>  		size = CURSOR_HEIGHT(height) | CURSOR_WIDTH(width);
>  
>  		base = plane_state->surf;
> -		pos = intel_cursor_position(crtc_state, plane_state, false);
>  	}
>  
>  	/* On these chipsets we can only modify the base/size/stride
> @@ -301,14 +313,11 @@ static void i845_cursor_update_arm(struct intel_dsb *dsb,
>  		intel_de_write_fw(display, CURCNTR(display, PIPE_A), 0);
>  		intel_de_write_fw(display, CURBASE(display, PIPE_A), base);
>  		intel_de_write_fw(display, CURSIZE(display, PIPE_A), size);
> -		intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
>  		intel_de_write_fw(display, CURCNTR(display, PIPE_A), cntl);
>  
>  		plane->cursor.base = base;
>  		plane->cursor.size = size;
>  		plane->cursor.cntl = cntl;
> -	} else {
> -		intel_de_write_fw(display, CURPOS(display, PIPE_A), pos);
>  	}
>  }
>  
> @@ -645,7 +654,21 @@ static void skl_write_cursor_wm(struct intel_dsb *dsb,
>  			   skl_cursor_ddb_reg_val(ddb));
>  }
>  
> -/* TODO: split into noarm+arm pair */
> +static void i9xx_cursor_update_noarm(struct intel_dsb *dsb,
> +				     struct intel_plane *plane,
> +				     const struct intel_crtc_state *crtc_state,
> +				     const struct intel_plane_state *plane_state)
> +{
> +	struct intel_display *display = to_intel_display(plane);
> +	enum pipe pipe = plane->pipe;
> +	u32 pos = 0;
> +
> +	if (plane_state && plane_state->uapi.visible)
> +		pos = intel_cursor_position(crtc_state, plane_state, false);
> +
> +	intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
> +}
> +
>  static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
>  				   struct intel_plane *plane,
>  				   const struct intel_crtc_state *crtc_state,
> @@ -653,7 +676,7 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
>  {
>  	struct intel_display *display = to_intel_display(plane);
>  	enum pipe pipe = plane->pipe;
> -	u32 cntl = 0, base = 0, pos = 0, fbc_ctl = 0;
> +	u32 cntl = 0, base = 0, fbc_ctl = 0;
>  
>  	if (plane_state && plane_state->uapi.visible) {
>  		int width = drm_rect_width(&plane_state->uapi.dst);
> @@ -666,7 +689,6 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
>  			fbc_ctl = CUR_FBC_EN | CUR_FBC_HEIGHT(height - 1);
>  
>  		base = plane_state->surf;
> -		pos = intel_cursor_position(crtc_state, plane_state, false);
>  	}
>  
>  	/*
> @@ -703,14 +725,12 @@ static void i9xx_cursor_update_arm(struct intel_dsb *dsb,
>  		if (HAS_CUR_FBC(display))
>  			intel_de_write_dsb(display, dsb, CUR_FBC_CTL(display, pipe), fbc_ctl);
>  		intel_de_write_dsb(display, dsb, CURCNTR(display, pipe), cntl);
> -		intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
>  		intel_de_write_dsb(display, dsb, CURBASE(display, pipe), base);
>  
>  		plane->cursor.base = base;
>  		plane->cursor.size = fbc_ctl;
>  		plane->cursor.cntl = cntl;
>  	} else {
> -		intel_de_write_dsb(display, dsb, CURPOS(display, pipe), pos);
>  		intel_de_write_dsb(display, dsb, CURBASE(display, pipe), base);
>  	}
>  }
> @@ -1019,6 +1039,7 @@ intel_cursor_plane_create(struct intel_display *display,
>  	if (display->platform.i845g || display->platform.i865g) {
>  		cursor->max_stride = i845_cursor_max_stride;
>  		cursor->min_alignment = i845_cursor_min_alignment;
> +		cursor->update_noarm = i845_cursor_update_noarm;
>  		cursor->update_arm = i845_cursor_update_arm;
>  		cursor->disable_arm = i845_cursor_disable_arm;
>  		cursor->get_hw_state = i845_cursor_get_hw_state;
> @@ -1036,6 +1057,7 @@ intel_cursor_plane_create(struct intel_display *display,
>  		if (intel_scanout_needs_vtd_wa(display))
>  			cursor->vtd_guard = 2;
>  
> +		cursor->update_noarm = i9xx_cursor_update_noarm;
>  		cursor->update_arm = i9xx_cursor_update_arm;
>  		cursor->disable_arm = i9xx_cursor_disable_arm;
>  		cursor->get_hw_state = i9xx_cursor_get_hw_state;
> -- 
> 2.55.0

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/i915/display: Split i845 and i9xx cursor updates into arm/noarm

[George Karagounis]

Hi Ville,

Thank you for the detailed explanation. i completely misunderstood
the purpose of the noarm/arm split regarding vblank evasion, and
i missed the interaction with the mailbox updates in the legacy
cursor path.

Given the architectural complexity required to safely split this
without disarming pending updates, I will drop this patch for now 
and leave the TODO in place.

Thanks,
George