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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915/fbc: collapsed FBC dirty rect for panned or rotated planes
--

commit 688db83891029bf71adf9b522ab593681a2edb50
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 c6f9e6506f630..3915dea9edb1e 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -1556,11 +1556,14 @@ __intel_fbc_prepare_dirty_rect(const struct intel_plane_state *plane_state,
>  			       const struct intel_crtc_state *crtc_state)
>  {

[ ... ]

> -	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 subtracting an absolute coordinate from a relative size here break the
dirty rect for rotated planes?

The damage rectangle coordinates are in absolute framebuffer space, while
height and y_offset are relative. If an absolute coordinate like damage->y2
exceeds the relative height, inv_y1 becomes negative.

Could this cause y_offset + inv_y1 to fall below y_offset, forcing clamp()
to always return y_offset and collapse the FBC dirty rect to zero height?

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

[Severity: High]
Are absolute coordinates being incorrectly clamped against tile-relative bounds
here in __intel_fbc_prepare_dirty_rect()?

For panned planes where the source Y offset is non-zero, damage->y1 might be
much larger than the relative y_end. For example, if damage->y1 is 500 and
y_end is 120, wouldn't clamping an absolute damage coordinate against y_end
force both y1 and y2 to y_end?

Could this result in the dirty rect collapsing to zero height and halting
compressed framebuffer updates?

> +		}
> +
> +		/*
> +		 * 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: High]
Since absolute damage coordinates will naturally differ from tile-relative
clamped coordinates, will this condition evaluate to true incorrectly?

If so, does this falsely trigger the debug message and spam the kernel logs
whenever panned or rotated planes are configured by a compositor?

> +
> +		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);
> +	}
>  }

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