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

[sashiko-bot@kernel.org]

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