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

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 <[email protected]>

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/[email protected]?part=1