[PATCH 0/2] drm/i915/fbc: Dirty rectangle bounds enforcement and CFB nuke handling

[PATCH 0/2] drm/i915/fbc: Dirty rectangle bounds enforcement and CFB nuke handling

[Austin Hu]

This series improves Frame Buffer Compression (FBC) dirty rectangle
handling in compliance with hardware requirements.

The first patch restricts dirty rectangle vertical coordinates to the
valid frame buffer range [y_offset, y_end], adding explicit coordinate
inversion handling for 180-degree plane rotation and debug logging when
clamping occurs[cite: 10].

The second patch tracks plane state attribute changes during atomic commits[cite: 9].
When FBC Dirty Rectangle mode is active, partial updates only apply to
surface address changes (PLANE_SURF)[cite: 9]. Modifications to any other
plane state parameters require fetching full-plane pixel data from memory to
re-compress and update the entire CFB in stolen memory[cite: 9]. Once fully
re-compressed, subsequent atomic commits can resume selective dirty
rectangle updates[cite: 9].

Austin Hu (1):
  drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes[cite: 9]

Charlton Lin (1):
  drm/i915/fbc: fbc_dirty_rect restrictions and logging[cite: 10]

 drivers/gpu/drm/i915/display/intel_fbc.c | 158 +++++++++++++++++++++-[cite: 9, 10]
 1 file changed, 151 insertions(+), 7 deletions(-)[cite: 9, 10]

[PATCH 1/2] drm/i915/fbc: fbc_dirty_rect restrictions and logging

[Austin Hu]

From: Charlton Lin <charlton.lin@intel.com>

Clamp the FBC dirty rectangle coordinates to the valid frame buffer
range [y_offset, y_end] to prevent out-of-bounds coordinates, and
log a debug message when clamping occurs.

Signed-off-by: Charlton Lin <charlton.lin@intel.com>
Signed-off-by: Austin Hu <austin.hu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 39 ++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index f61b4a218d6e..c0fed695af0d 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1521,11 +1521,14 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 			       const struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	struct intel_display *display = to_intel_display(plane_state);
 	struct intel_fbc *fbc = plane->fbc;
 	struct drm_rect *fbc_dirty_rect = &fbc->state.dirty_rect;
 	int width = drm_rect_width(&plane_state->uapi.src) >> 16;
+	int height = drm_rect_height(&plane_state->uapi.src) >> 16;
 	const struct drm_rect *damage = &plane_state->damage;
 	int y_offset = plane_state->view.color_plane[0].y;
+	int y_end = y_offset + height;
 
 	lockdep_assert_held(&fbc->lock);
 
@@ -1535,11 +1538,41 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 		return;
 	}
 
-	if (drm_rect_visible(damage))
-		*fbc_dirty_rect = *damage;
-	else
+	if (drm_rect_visible(damage)) {
+		int y1, y2;
+
+		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
+			/* Under 180 degree rotation, coordinate system is inverted */
+			int inv_y1 = height - damage->y2;
+			int inv_y2 = height - damage->y1;
+
+			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
+			y2 = clamp(y_offset + inv_y2, y_offset, y_end);
+		} else {
+			y1 = clamp(damage->y1, y_offset, y_end);
+			y2 = clamp(damage->y2, y_offset, y_end);
+		}
+
+		/*
+		 * Clamp dirty rect to the valid FB range [y_offset, y_end].
+		 * Per Bspec:
+		 *   start_line >= y_offset
+		 *   end_line <= y_offset + plane_height
+		 */
+		if (y1 != damage->y1 || y2 != damage->y2)
+			drm_dbg_kms(display->drm,
+				    "[PLANE:%d:%s] FBC dirty rect out of range: y1=%d y2=%d clamped to y1=%d y2=%d (y_offset=%d y_end=%d)\n",
+				    plane->base.base.id, plane->base.name,
+				    damage->y1, damage->y2, y1, y2, y_offset, y_end);
+
+		fbc_dirty_rect->x1 = damage->x1;
+		fbc_dirty_rect->x2 = damage->x2;
+		fbc_dirty_rect->y1 = y1;
+		fbc_dirty_rect->y2 = y2;
+	} else {
 		/* dirty rect must cover at least one line */
 		*fbc_dirty_rect = DRM_RECT_INIT(0, y_offset, width, 1);
+	}
 }
 
 void
-- 
2.34.1

[PATCH 2/2] drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

[Austin Hu]

When FBC Dirty Rectangle mode is enabled, any non-PLANE_SURF register
update requires fetching the full plane frame buffer from memory to
re-compress and update the CFB in stolen memory.

Check plane state attributes during atomic commits and force a full
CFB nuke if any setting other than the surface address changes.

Signed-off-by: Austin Hu <austin.hu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 126 ++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index c0fed695af0d..6bf5c0947671 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1516,8 +1516,124 @@ static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
 		intel_fbc_is_cfb_ok(plane_state);
 }
 
+/*
+ * From BSpec about "FBC Dirty Rectangle", when Dirty Rectangle mode is active,
+ * partial updates only apply to surface address changes. Any other plane state
+ * modification requires Plane to fetch full-plane pixel data from memory to
+ * re-compress and update the entire CFB in stolen memory. Once fully
+ * re-compressed, subsequent atomic commits go ahead with FBC dirty rectangle
+ * updates for smooth visual updates.
+ *
+ * So check any Plane attribute changed except for its surface address by
+ * referring to intel_async_flip_check_hw() which also checks async flip.
+ */
+static bool intel_fbc_dirty_rect_needs_nuke(struct intel_atomic_state *state,
+					    struct intel_plane *plane)
+{
+	struct intel_display *display = to_intel_display(state);
+	const struct intel_plane_state *old_plane_state =
+		intel_atomic_get_old_plane_state(state, plane);
+	const struct intel_plane_state *new_plane_state =
+		intel_atomic_get_new_plane_state(state, plane);
+
+	/*
+	 * If the plane state isn't part of this atomic transaction or during
+	 * initial plane setup, conservatively force a full CFB nuke.
+	 */
+	if (!old_plane_state || !new_plane_state)
+		return true;
+
+	if (old_plane_state->view.color_plane[0].mapping_stride !=
+	    new_plane_state->view.color_plane[0].mapping_stride) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Stride changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->hw.fb->modifier !=
+	    new_plane_state->hw.fb->modifier) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Modifier changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->hw.fb->format != new_plane_state->hw.fb->format) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Pixel format changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->hw.rotation != new_plane_state->hw.rotation) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Rotation changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (skl_plane_aux_dist(old_plane_state, 0) !=
+	    skl_plane_aux_dist(new_plane_state, 0)) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] AUX_DIST changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (!drm_rect_equals(&old_plane_state->uapi.src,
+			     &new_plane_state->uapi.src) ||
+	    !drm_rect_equals(&old_plane_state->uapi.dst,
+			     &new_plane_state->uapi.dst)) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Size/coords changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->hw.alpha != new_plane_state->hw.alpha) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Alpha changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->decrypt != new_plane_state->decrypt) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Decryption changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	/* Includes pixel_blend_mode, color_encoding & color_range checking. */
+	if (old_plane_state->color_ctl != new_plane_state->color_ctl) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Color ctl changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->cus_ctl != new_plane_state->cus_ctl) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] CUS ctl changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	if (old_plane_state->view.color_plane[0].x !=
+	    new_plane_state->view.color_plane[0].x) {
+		drm_dbg_kms(display->drm,
+			    "[PLANE:%d:%s] Color plane 0 x changed in FBC DIRTY RECT\n",
+			    plane->base.base.id, plane->base.name);
+		return true;
+	}
+
+	return false;
+}
+
 static void
-__intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
+__intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
+			       const struct intel_plane_state *plane_state,
 			       const struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
@@ -1538,7 +1654,10 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 		return;
 	}
 
-	if (drm_rect_visible(damage)) {
+	if (intel_fbc_dirty_rect_needs_nuke(state, plane)) {
+		/* compress the entire region due to non PLANE_SURF updating. */
+		*fbc_dirty_rect = DRM_RECT_INIT(0, y_offset, width, height);
+	} else if (drm_rect_visible(damage)) {
 		int y1, y2;
 
 		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
@@ -1598,8 +1717,7 @@ intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
 		mutex_lock(&fbc->lock);
 
 		if (fbc->state.plane == plane)
-			__intel_fbc_prepare_dirty_rect(plane_state,
-						       crtc_state);
+			__intel_fbc_prepare_dirty_rect(state, plane_state, crtc_state);
 
 		mutex_unlock(&fbc->lock);
 	}
-- 
2.34.1

Re: [PATCH 1/2] drm/i915/fbc: fbc_dirty_rect restrictions and logging

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect coordinate space math in the 180-degree rotation branch causes the FBC dirty rectangle to collapse, breaking FBC tracking.
- [Medium] The 'FBC dirty rect out of range' debug warning will trigger spuriously on almost every frame when the plane is rotated 180 degrees.
--

--- Patch [1]: [PATCH 1/2] drm/i915/fbc: fbc_dirty_rect restrictions and logging ---
commit 234fb7237db4bb8e467458e88cc6ed3f6e850511
Author: Charlton Lin <charlton.lin@intel.com>

drm/i915/fbc: fbc_dirty_rect restrictions and logging

Clamp the FBC dirty rectangle coordinates to the valid frame buffer
range [y_offset, y_end] to prevent out-of-bounds coordinates, and
log a debug message when clamping occurs.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index f61b4a218d6ef..c0fed695af0da 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c

[ ... ]

> @@ -1535,11 +1538,41 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
>  		return;
>  	}
>  
> -	if (drm_rect_visible(damage))
> -		*fbc_dirty_rect = *damage;
> -	else
> +	if (drm_rect_visible(damage)) {
> +		int y1, y2;
> +
> +		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
> +			/* Under 180 degree rotation, coordinate system is inverted */
> +			int inv_y1 = height - damage->y2;
> +			int inv_y2 = height - damage->y1;
> +
> +			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
> +			y2 = clamp(y_offset + inv_y2, y_offset, y_end);

[Severity: High]
Can this math result in collapsed dirty rectangles?

Because damage->y2 already includes the absolute y_offset, height - damage->y2
yields a large negative number. When y_offset is added back, the result falls
well below y_offset, causing both y1 and y2 to unconditionally clamp to
y_offset.

This collapses the FBC dirty rectangle to a 0-height region starting at
y_offset, entirely missing the actual damaged pixels.

> +		} else {
> +			y1 = clamp(damage->y1, y_offset, y_end);
> +			y2 = clamp(damage->y2, y_offset, y_end);
> +		}
> +
> +		/*
> +		 * Clamp dirty rect to the valid FB range [y_offset, y_end].
> +		 * Per Bspec:
> +		 *   start_line >= y_offset
> +		 *   end_line <= y_offset + plane_height
> +		 */
> +		if (y1 != damage->y1 || y2 != damage->y2)
> +			drm_dbg_kms(display->drm,
> +				    "[PLANE:%d:%s] FBC dirty rect out of range: y1=%d y2=%d clamped to y1=%d y2=%d (y_offset=%d y_end=%d)\n",
> +				    plane->base.base.id, plane->base.name,
> +				    damage->y1, damage->y2, y1, y2, y_offset, y_end);

[Severity: Medium]
Will this warning trigger spuriously on almost every frame when the plane is
rotated 180 degrees?

For 180-degree planes, the code intentionally alters y1 and y2 to represent
inverted coordinates. The warning condition compares these new inverted
coordinates directly against the original, non-inverted coordinates.

Because an inverted coordinate will inherently mismatch a non-inverted one,
this condition evaluates to true on nearly every atomic update involving damage
for rotated planes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812215350.3753102-1-austin.hu@intel.com?part=1

Re: [PATCH 2/2] drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Strict equality check on `uapi.src` forces a full FBC CFB nuke on vertical panning (Y-offset changes), defeating dirty rectangle optimizations for scrolling.

Pre-existing issues:
- [High] Incorrect damage coordinate inversion math for 180-degree rotated planes results in 0-height dirty rectangles, freezing screen updates.
--

--- Patch [2]: [PATCH 2/2] drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes ---
commit 50b0481138fde60ac48db330dd972dfba35b3f10
Author: Austin Hu <austin.hu@intel.com>

drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes
    
When FBC Dirty Rectangle mode is enabled, any non-PLANE_SURF register
update requires fetching the full plane frame buffer from memory to
re-compress and update the CFB in stolen memory. This commit checks
plane state attributes during atomic commits and forces a full CFB
nuke if any setting other than the surface address changes.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index c0fed695af0da..6bf5c09476713 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1516,8 +1516,124 @@ static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
>  		intel_fbc_is_cfb_ok(plane_state);
>  }
>  
> +/*
> + * From BSpec about "FBC Dirty Rectangle", when Dirty Rectangle mode is active,

[ ... ]

> +static bool intel_fbc_dirty_rect_needs_nuke(struct intel_atomic_state *state,
> +					    struct intel_plane *plane)
> +{
> +	struct intel_display *display = to_intel_display(state);

[ ... ]

> +	if (skl_plane_aux_dist(old_plane_state, 0) !=
> +	    skl_plane_aux_dist(new_plane_state, 0)) {
> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] AUX_DIST changed in FBC DIRTY RECT\n",
> +			    plane->base.base.id, plane->base.name);
> +		return true;
> +	}
> +
> +	if (!drm_rect_equals(&old_plane_state->uapi.src,
> +			     &new_plane_state->uapi.src) ||
> +	    !drm_rect_equals(&old_plane_state->uapi.dst,
> +			     &new_plane_state->uapi.dst)) {

[Severity: Medium]
Does this strict equality check on uapi.src accidentally force a full CFB
nuke during vertical panning?

A vertical pan (scrolling) changes uapi.src.y1 to shift the viewport, which
translates to a surface address update (PLANE_OFFSET / PLANE_SURF registers).
Because drm_rect_equals() checks the coordinates (x1 and y1), it fails when
panning, erroneously triggering a full CFB nuke on every frame of the scroll.

Should this perhaps verify drm_rect_width() and drm_rect_height() instead
to allow pure viewport shifts?

> +		drm_dbg_kms(display->drm,
> +			    "[PLANE:%d:%s] Size/coords changed in FBC DIRTY RECT\n",
> +			    plane->base.base.id, plane->base.name);
> +		return true;
> +	}
> +
> +	if (old_plane_state->hw.alpha != new_plane_state->hw.alpha) {

[ ... ]

>  static void
> -__intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
> +__intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
> +			       const struct intel_plane_state *plane_state,
>  			       const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> @@ -1538,7 +1654,10 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
>  		return;
>  	}
>  
> -	if (drm_rect_visible(damage)) {
> +	if (intel_fbc_dirty_rect_needs_nuke(state, plane)) {
> +		/* compress the entire region due to non PLANE_SURF updating. */
> +		*fbc_dirty_rect = DRM_RECT_INIT(0, y_offset, width, height);
> +	} else if (drm_rect_visible(damage)) {
>  		int y1, y2;
>  
>  		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
>  			/* Under 180 degree rotation, coordinate system is inverted */
>  			int inv_y1 = height - damage->y2;
>  			int inv_y2 = height - damage->y1;
>  
>  			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
>  			y2 = clamp(y_offset + inv_y2, y_offset, y_end);

[Severity: High]
This is a pre-existing issue, but does this math incorrectly treat absolute
framebuffer coordinates as relative coordinates, resulting in zero-height
dirty rectangles for 180-degree rotated displays?

The damage clip is already in absolute framebuffer coordinates, meaning
damage->y2 resides between y_offset and y_end. Subtracting this from the
plane's height produces a value that, when added to y_offset, is almost
always less than y_offset. 

The clamp() function then pins both y1 and y2 strictly to y_offset,
programming a zero-height dirty rectangle and freezing screen updates on
rotated displays.

Could this be corrected to account for the absolute offset, perhaps using
something like y_end - (damage->y2 - y_offset)?

>  		} else {
>  			y1 = clamp(damage->y1, y_offset, y_end);
>  			y2 = clamp(damage->y2, y_offset, y_end);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812215350.3753102-1-austin.hu@intel.com?part=2

✗ i915.CI.BAT: failure for drm/i915/fbc: Dirty rectangle bounds enforcement and CFB nuke handling

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

== Series Details ==

Series: drm/i915/fbc: Dirty rectangle bounds enforcement and CFB nuke handling
URL   : https://patchwork.freedesktop.org/series/172104/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_18985 -> Patchwork_172104v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_172104v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_172104v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172104v1/index.html

Participating hosts (37 -> 34)
------------------------------

  Missing    (3): bat-dg2-13 bat-rpls-4 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_172104v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-bsw-n3050:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18985/fi-bsw-n3050/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172104v1/fi-bsw-n3050/igt@i915_module_load@load.html

  


Build changes
-------------

  * Linux: CI_DRM_18985 -> Patchwork_172104v1

  CI-20190529: 20190529
  CI_DRM_18985: 707b81710802330f08e561e54d174f74abdb10e5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9054: 3d819a8f511b2bcd844647bcf7e433b9407d058e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_172104v1: 707b81710802330f08e561e54d174f74abdb10e5 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172104v1/index.html

[-- Attachment #2: Type: text/html, Size: 2528 bytes --]

[PATCH v2 0/2] drm/i915/fbc: Dirty rectangle bounding and CFB nuke handling

[Austin Hu]

This series fixes two issue areas in Intel FBC (Framebuffer Compression)
dirty rectangle handling:

1. Clamps FBC dirty rectangle bounds to valid surface limits [y_offset, y_end]
   and adds logging for out-of-bounds coordinates, including handling for 
   180-degree plane rotation.
2. Tracks plane state attribute changes during atomic commits. When FBC Dirty
   Rectangle mode is active, partial updates only apply to surface address
   changes (PLANE_SURF). Modifications to any other plane state parameters
   require fetching full-plane pixel data from memory to re-compress and
   update the entire CFB in stolen memory. Once fully re-compressed,
   subsequent atomic commits can resume selective dirty rectangle updates.

v1 -> v2 changes:
  - Reused intel_fbc_can_flip_nuke() for plane update state checks as suggested.
  - Added plane Y-coordinate plane view checking (color_plane[0].y) alongside X.

Austin Hu (1):
  drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

Charlton Lin (1):
  drm/i915/fbc: fbc_dirty_rect restrictions and logging

 drivers/gpu/drm/i915/display/intel_fbc.c | 192 +++++++++++++++++------
 1 file changed, 141 insertions(+), 51 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] drm/i915/fbc: fbc_dirty_rect restrictions and logging

[Austin Hu]

From: Charlton Lin <charlton.lin@intel.com>

Clamp the FBC dirty rectangle coordinates to the valid frame buffer
range [y_offset, y_end] to prevent out-of-bounds coordinates, and
log a debug message when clamping occurs.

Signed-off-by: Charlton Lin <charlton.lin@intel.com>
Signed-off-by: Austin Hu <austin.hu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 39 ++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index f61b4a218d6e..c0fed695af0d 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1521,11 +1521,14 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 			       const struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	struct intel_display *display = to_intel_display(plane_state);
 	struct intel_fbc *fbc = plane->fbc;
 	struct drm_rect *fbc_dirty_rect = &fbc->state.dirty_rect;
 	int width = drm_rect_width(&plane_state->uapi.src) >> 16;
+	int height = drm_rect_height(&plane_state->uapi.src) >> 16;
 	const struct drm_rect *damage = &plane_state->damage;
 	int y_offset = plane_state->view.color_plane[0].y;
+	int y_end = y_offset + height;
 
 	lockdep_assert_held(&fbc->lock);
 
@@ -1535,11 +1538,41 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 		return;
 	}
 
-	if (drm_rect_visible(damage))
-		*fbc_dirty_rect = *damage;
-	else
+	if (drm_rect_visible(damage)) {
+		int y1, y2;
+
+		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
+			/* Under 180 degree rotation, coordinate system is inverted */
+			int inv_y1 = height - damage->y2;
+			int inv_y2 = height - damage->y1;
+
+			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
+			y2 = clamp(y_offset + inv_y2, y_offset, y_end);
+		} else {
+			y1 = clamp(damage->y1, y_offset, y_end);
+			y2 = clamp(damage->y2, y_offset, y_end);
+		}
+
+		/*
+		 * Clamp dirty rect to the valid FB range [y_offset, y_end].
+		 * Per Bspec:
+		 *   start_line >= y_offset
+		 *   end_line <= y_offset + plane_height
+		 */
+		if (y1 != damage->y1 || y2 != damage->y2)
+			drm_dbg_kms(display->drm,
+				    "[PLANE:%d:%s] FBC dirty rect out of range: y1=%d y2=%d clamped to y1=%d y2=%d (y_offset=%d y_end=%d)\n",
+				    plane->base.base.id, plane->base.name,
+				    damage->y1, damage->y2, y1, y2, y_offset, y_end);
+
+		fbc_dirty_rect->x1 = damage->x1;
+		fbc_dirty_rect->x2 = damage->x2;
+		fbc_dirty_rect->y1 = y1;
+		fbc_dirty_rect->y2 = y2;
+	} else {
 		/* dirty rect must cover at least one line */
 		*fbc_dirty_rect = DRM_RECT_INIT(0, y_offset, width, 1);
+	}
 }
 
 void
-- 
2.34.1

[PATCH v2 2/2] drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

[Austin Hu]

When FBC Dirty Rectangle mode is enabled, any non-PLANE_SURF register
update requires fetching the full plane frame buffer from memory to
re-compress and update the CFB in stolen memory.

Check plane state attributes during atomic commits and force a full
CFB nuke if any setting other than the surface address changes.

---
v2:
 - Address reviewer feedback to reuse intel_fbc_can_flip_nuke().
---
Signed-off-by: Austin Hu <austin.hu@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 155 ++++++++++++++++-------
 1 file changed, 106 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index c0fed695af0d..34c0e42dfdab 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1516,11 +1516,106 @@ static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
 		intel_fbc_is_cfb_ok(plane_state);
 }
 
+static bool intel_fbc_can_flip_nuke(struct intel_atomic_state *state,
+				    struct intel_crtc *crtc,
+				    struct intel_plane *plane)
+{
+	struct intel_display *display = to_intel_display(state);
+	const struct intel_crtc_state *new_crtc_state =
+		intel_atomic_get_new_crtc_state(state, crtc);
+	const struct intel_plane_state *old_plane_state =
+		intel_atomic_get_old_plane_state(state, plane);
+	const struct intel_plane_state *new_plane_state =
+		intel_atomic_get_new_plane_state(state, plane);
+	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
+	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
+
+	if (intel_crtc_needs_modeset(new_crtc_state))
+		return false;
+
+	if (!intel_fbc_is_ok(old_plane_state) ||
+	    !intel_fbc_is_ok(new_plane_state))
+		return false;
+
+	if (old_fb->format->format != new_fb->format->format)
+		return false;
+
+	if (old_fb->modifier != new_fb->modifier)
+		return false;
+
+	if (intel_fbc_plane_stride(old_plane_state) !=
+	    intel_fbc_plane_stride(new_plane_state))
+		return false;
+
+	if (intel_fbc_cfb_stride(old_plane_state) !=
+	    intel_fbc_cfb_stride(new_plane_state))
+		return false;
+
+	if (intel_fbc_cfb_size(old_plane_state) !=
+	    intel_fbc_cfb_size(new_plane_state))
+		return false;
+
+	if (intel_fbc_override_cfb_stride(old_plane_state) !=
+	    intel_fbc_override_cfb_stride(new_plane_state))
+		return false;
+
+	if (!HAS_FBC_DIRTY_RECT(display))
+		return true;
+
+	/*
+	 * From BSpec about "FBC Dirty Rectangle", when Dirty Rectangle mode is
+	 * active, partial updates only apply to surface address changes. Any other
+	 * plane state modification requires Plane to fetch full-plane pixel data
+	 * from memory to re-compress and update the entire CFB in stolen memory.
+	 * Once fully re-compressed, subsequent atomic commits go ahead with FBC
+	 * dirty rectangle updates for smooth visual updates.
+	 *
+	 * So check other Plane attribute changed except for its surface address by
+	 * referring to intel_async_flip_check_hw() which also checks async flip.
+	 */
+
+	/* Includes pixel_blend_mode, color_encoding & color_range checking. */
+	if (old_plane_state->color_ctl != new_plane_state->color_ctl)
+		return false;
+
+	if (!drm_rect_equals(&old_plane_state->uapi.src,
+			     &new_plane_state->uapi.src) ||
+	    !drm_rect_equals(&old_plane_state->uapi.dst,
+			     &new_plane_state->uapi.dst))
+		return false;
+
+	if ((old_plane_state->view.color_plane[0].x !=
+	    new_plane_state->view.color_plane[0].x) ||
+	    (old_plane_state->view.color_plane[0].y !=
+	    new_plane_state->view.color_plane[0].y))
+		return false;
+
+	if (old_plane_state->hw.rotation != new_plane_state->hw.rotation)
+		return false;
+
+	if (old_plane_state->hw.alpha != new_plane_state->hw.alpha)
+		return false;
+
+	if (skl_plane_aux_dist(old_plane_state, 0) !=
+	    skl_plane_aux_dist(new_plane_state, 0))
+		return false;
+
+	if (old_plane_state->decrypt != new_plane_state->decrypt)
+		return false;
+
+	if (old_plane_state->cus_ctl != new_plane_state->cus_ctl)
+		return false;
+
+	return true;
+}
+
 static void
-__intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
+__intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
+			       const struct intel_plane_state *plane_state,
 			       const struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct intel_display *display = to_intel_display(plane_state);
 	struct intel_fbc *fbc = plane->fbc;
 	struct drm_rect *fbc_dirty_rect = &fbc->state.dirty_rect;
@@ -1538,7 +1633,15 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
 		return;
 	}
 
-	if (drm_rect_visible(damage)) {
+	if (!intel_fbc_can_flip_nuke(state, crtc, plane)) {
+		if (HAS_FBC_DIRTY_RECT(display))
+			drm_dbg_kms(display->drm,
+				    "[PLANE:%d:%s] Non surf addr changed for FBC DIRTY RECT\n",
+				    plane->base.base.id, plane->base.name);
+
+		/* compress the entire region due to non PLANE_SURF updating. */
+		*fbc_dirty_rect = DRM_RECT_INIT(0, y_offset, width, height);
+	} else if (drm_rect_visible(damage)) {
 		int y1, y2;
 
 		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
@@ -1598,8 +1701,7 @@ intel_fbc_prepare_dirty_rect(struct intel_atomic_state *state,
 		mutex_lock(&fbc->lock);
 
 		if (fbc->state.plane == plane)
-			__intel_fbc_prepare_dirty_rect(plane_state,
-						       crtc_state);
+			__intel_fbc_prepare_dirty_rect(state, plane_state, crtc_state);
 
 		mutex_unlock(&fbc->lock);
 	}
@@ -1798,51 +1900,6 @@ int intel_fbc_min_cdclk(const struct intel_crtc_state *crtc_state)
 	return min_cdclk;
 }
 
-static bool intel_fbc_can_flip_nuke(struct intel_atomic_state *state,
-				    struct intel_crtc *crtc,
-				    struct intel_plane *plane)
-{
-	const struct intel_crtc_state *new_crtc_state =
-		intel_atomic_get_new_crtc_state(state, crtc);
-	const struct intel_plane_state *old_plane_state =
-		intel_atomic_get_old_plane_state(state, plane);
-	const struct intel_plane_state *new_plane_state =
-		intel_atomic_get_new_plane_state(state, plane);
-	const struct drm_framebuffer *old_fb = old_plane_state->hw.fb;
-	const struct drm_framebuffer *new_fb = new_plane_state->hw.fb;
-
-	if (intel_crtc_needs_modeset(new_crtc_state))
-		return false;
-
-	if (!intel_fbc_is_ok(old_plane_state) ||
-	    !intel_fbc_is_ok(new_plane_state))
-		return false;
-
-	if (old_fb->format->format != new_fb->format->format)
-		return false;
-
-	if (old_fb->modifier != new_fb->modifier)
-		return false;
-
-	if (intel_fbc_plane_stride(old_plane_state) !=
-	    intel_fbc_plane_stride(new_plane_state))
-		return false;
-
-	if (intel_fbc_cfb_stride(old_plane_state) !=
-	    intel_fbc_cfb_stride(new_plane_state))
-		return false;
-
-	if (intel_fbc_cfb_size(old_plane_state) !=
-	    intel_fbc_cfb_size(new_plane_state))
-		return false;
-
-	if (intel_fbc_override_cfb_stride(old_plane_state) !=
-	    intel_fbc_override_cfb_stride(new_plane_state))
-		return false;
-
-	return true;
-}
-
 static bool __intel_fbc_pre_update(struct intel_atomic_state *state,
 				   struct intel_crtc *crtc,
 				   struct intel_plane *plane)
-- 
2.34.1

Re: [PATCH v2 2/2] drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect coordinate inversion for 180-degree rotation causes empty or truncated FBC dirty rectangles.
- [Medium] intel_fbc_can_flip_nuke fails to detect color_encoding changes on ICL+ HDR planes.
- [Medium] intel_fbc_can_flip_nuke fails to check for Color Key (ckey) changes.
- [Medium] intel_fbc_can_flip_nuke fails to check for Fast Clear Color Value (ccval) changes.
--

commit 10fd2eb7ca0c8e663bc8d78356bb0bf9afd6b14b
Author: Austin Hu <austin.hu@intel.com>

drm/i915/fbc: nuke CFB if Plane setting (except for surf addr) changes

This commit forces a full compressed framebuffer (CFB) nuke when any plane
state attribute other than the surface address changes while FBC Dirty
Rectangle mode is enabled. It checks various plane state attributes during
atomic commits to determine if the full plane pixel data needs to be
re-compressed.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index c0fed695af0da..34c0e42dfdab6 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
[ ... ]
> @@ -1516,11 +1516,106 @@ static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
[ ... ]
> +	/* Includes pixel_blend_mode, color_encoding & color_range checking. */
> +	if (old_plane_state->color_ctl != new_plane_state->color_ctl)
> +		return false;

[Severity: Medium]
Is checking color_ctl sufficient for detecting color encoding changes on ICL+
HDR planes? For these planes, the color encoding (like BT.2020 vs BT.709) is
not stored in color_ctl, but is directly programmed via
plane_state->hw.color_encoding. Will this cause the display to show stale FBC
data if the color encoding changes but color_ctl remains the same?

[ ... ]
> +	if (old_plane_state->decrypt != new_plane_state->decrypt)
> +		return false;
> +
> +	if (old_plane_state->cus_ctl != new_plane_state->cus_ctl)
> +		return false;
> +
> +	return true;
> +}

[Severity: Medium]
Are we missing checks for Color Key changes here? If userspace updates
the color key properties, plane_state->ckey and plane_state->ctl are modified.
Since they are omitted from this check, could this update the PLANE_KEYVAL
registers without triggering the necessary FBC nuke?

[Severity: Medium]
Similarly, does this function need to compare the fast clear color values
(old_plane_state->ccval and new_plane_state->ccval)? If the clear color value
changes for a compressed framebuffer, FBC dirty rect updates might proceed
without full CFB re-compression, potentially corrupting display content.

[ ... ]
> @@ -1538,7 +1633,15 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
[ ... ]
> -	if (drm_rect_visible(damage)) {
> +	if (!intel_fbc_can_flip_nuke(state, crtc, plane)) {
[ ... ]
> +	} else if (drm_rect_visible(damage)) {
>  		int y1, y2;
>  
>  		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
>  			/* Under 180 degree rotation, coordinate system is inverted */
>  			int inv_y1 = height - damage->y2;
>  			int inv_y2 = height - damage->y1;
>  
>  			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
>  			y2 = clamp(y_offset + inv_y2, y_offset, y_end);

[Severity: High]
Does this coordinate inversion for 180-degree rotation correctly account for
the y_offset? Since the damage->y1 and damage->y2 coordinates are already in
absolute memory coordinates (and include the y_offset), calculating
inv_y1 = height - damage->y2 and then adding y_offset back appears to
mathematically subtract y_offset entirely. Could this result in empty or
mis-clamped dirty rectangles when the screen is rotated 180 degrees?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812215350.3753102-1-austin.hu@intel.com?part=2

Re: [PATCH v2 1/2] drm/i915/fbc: fbc_dirty_rect restrictions and logging

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect 180-degree rotation math negates `y_offset` instead of maintaining it, resulting in a 0-height dirty rectangle that breaks FBC updates.
- [Medium] Spurious 'FBC dirty rect out of range' debug logging on every frame update during 180-degree rotation.
--

commit 4adeca8228048877f976377febbd0e77586bd42c
Author: Charlton Lin <charlton.lin@intel.com>

drm/i915/fbc: fbc_dirty_rect restrictions and logging

This commit clamps the fbc dirty rectangle coordinates to the valid frame
buffer range to prevent out-of-bounds access. It also adds a debug message
to log when clamping occurs.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
[ ... ]
> @@ -1535,11 +1538,41 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
>  		return;
>  	}
>  
> -	if (drm_rect_visible(damage))
> -		*fbc_dirty_rect = *damage;
> -	else
> +	if (drm_rect_visible(damage)) {
> +		int y1, y2;
> +
> +		if (plane_state->hw.rotation & DRM_MODE_ROTATE_180) {
> +			/* Under 180 degree rotation, coordinate system is inverted */
> +			int inv_y1 = height - damage->y2;
> +			int inv_y2 = height - damage->y1;
> +
> +			y1 = clamp(y_offset + inv_y1, y_offset, y_end);
> +			y2 = clamp(y_offset + inv_y2, y_offset, y_end);

[Severity: High]
Does this calculation erroneously mix relative sizes with absolute
coordinates?

The math `height - damage->y2` seems to assume `damage` is viewport-relative,
but it is actually in absolute framebuffer coordinates (which contains
`y_offset`). When `y_offset` is added back here, it effectively cancels out.

If `y_offset >= height`, this strictly clamps to `y_offset`, resulting in
a 0-height dirty rectangle (`y1 = y2 = y_offset`), which can cause a
regression for fbc screen updates when 180-degree rotation is active.

This issue does not appear to be fixed in the subsequent patch in the series.

> +		} else {
> +			y1 = clamp(damage->y1, y_offset, y_end);
> +			y2 = clamp(damage->y2, y_offset, y_end);
> +		}
> +
> +		/*
> +		 * Clamp dirty rect to the valid FB range [y_offset, y_end].
> +		 * Per Bspec:
> +		 *   start_line >= y_offset
> +		 *   end_line <= y_offset + plane_height
> +		 */
> +		if (y1 != damage->y1 || y2 != damage->y2)

[Severity: Medium]
Will this condition trigger a false positive on every frame update during
180-degree rotation?

The check `y1 != damage->y1` directly compares the newly inverted coordinate
(`y1`) against the original non-inverted coordinate (`damage->y1`). This
will almost always evaluate to true regardless of whether any actual
clamping occurred.

This can potentially spam the kernel log with false positive 'fbc dirty
rect out of range' messages whenever kms debugging is enabled.

This issue also does not appear to be fixed in the subsequent patch in the
series.

> +			drm_dbg_kms(display->drm,
> +				    "[PLANE:%d:%s] FBC dirty rect out of range: y1=%d y2=%d clamped to y1=%d y2=%d (y_offset=%d y_end=%d)\n",
> +				    plane->base.base.id, plane->base.name,
> +				    damage->y1, damage->y2, y1, y2, y_offset, y_end);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812215350.3753102-1-austin.hu@intel.com?part=1