Re: [PATCH v2 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 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