Re: [v5 05/14] drm/i915/display: Track CSC mode in intel plane state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unmodified plane color pipeline state (including the newly added CSC_FF configuration) is inadvertently wiped and silently disabled when an atomic commit modifies unrelated plane properties.
--

--- Patch [5]: [v5 05/14] drm/i915/display: Track CSC mode in intel plane state ---
commit ff45547703318a58efbb8be7b806421f22234b15
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

drm/i915/display: Track CSC mode in intel plane state

This commit adds CSC mode to the hardware plane state to enable programming 
the fixed-function CSC block for SDR planes based on the 
DRM_COLOROP_FIXED_MATRIX state. It also explicitly tracks the bypass state 
as a boolean.

> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> --- a/drivers/gpu/drm/i915/display/intel_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
[ ... ]
> @@ -430,11 +430,19 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_atomic_state *state,
>  	while (iter_colorop) {
>  		for_each_new_colorop_in_state(&state->base, colorop, new_colorop_state, i) {

[Severity: High]
This is a pre-existing issue, but I have a question about how unmodified 
colorops are handled here.

Does this cause the hardware colorop configuration to be lost when unrelated
plane properties are updated?

Since for_each_new_colorop_in_state() only iterates over colorops actively
included in the current atomic commit, unmodified colorops won't be in the
state during a simple plane update like a page flip. 

Because the hardware state is wiped earlier by intel_plane_clear_hw_state(),
unmodified plane color pipeline state (including the newly added CSC_FF
configuration) appears to be left zeroed and silently disabled.

Can this lead to incorrect display output, and should this path handle
copying state from unmodified colorops?

>  			if (new_colorop_state->colorop == iter_colorop) {
> -				blob = new_colorop_state->bypass ? NULL : new_colorop_state->data;
>  				intel_colorop = to_intel_colorop(colorop);
> -				changed |= intel_plane_colorop_replace_blob(plane_state,
> +				if (intel_colorop->id == INTEL_PLANE_CB_CSC_FF) {
> +					plane_state->hw.csc_ff_enable =
> +						!new_colorop_state->bypass;
> +					plane_state->hw.csc_ff_type =
> +						new_colorop_state->fixed_matrix_type;
> +				} else {
> +					blob = new_colorop_state->bypass ?
> +						NULL : new_colorop_state->data;
> +					changed |= intel_plane_colorop_replace_blob(plane_state,
>  									    intel_colorop,
>  									    blob);
> +				}
>  			}
>  		}
>  		iter_colorop = iter_colorop->next;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720045131.971650-1-chaitanya.kumar.borah@intel.com?part=5