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

[sashiko-bot@kernel.org]

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