Re: [CI v4 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] The fixed-function CSC configuration (csc_ff_enable and csc_ff_type) and other color pipeline states (LUTs, CTMs) are lost on atomic commits that do not explicitly modify the colorop (e.g., page flips).
--

--- Patch [5]: [CI v4 05/14] drm/i915/display: Track CSC mode in intel plane state ---
commit d2e5633249bd5c0c74c6412da61ad42bd832980e
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 hw plane state to enable programming the
fixed-function CSC block for SDR planes. It also tracks the bypass state
explicitly as a boolean since it is managed separately from the FIXED_MATRIX
enum value.

> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> index a440f92ff00cd..217c7a84bbfa5 100644
> --- 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) {
>  			if (new_colorop_state->colorop == iter_colorop) {
>  				intel_colorop = to_intel_colorop(colorop);
> +				if (intel_colorop->id == INTEL_PLANE_CB_CSC_FF) {
> +					plane_state->hw.csc_ff_enable =
> +						!new_colorop_state->bypass;

[Severity: High]
This is a pre-existing issue, but does this loop fail to restore the hardware
state of colorops that are active but not modified in the current atomic
commit?

Since intel_plane_clear_hw_state() zeroes plane_state->hw before
intel_plane_color_copy_uapi_to_hw_state() is called, what happens on a simple
page flip that doesn't modify the color pipeline?

It looks like for_each_new_colorop_in_state() will only yield colorops that
were explicitly added to the atomic commit. Since a simple page flip does not
modify the color pipeline, the colorops wouldn't be in the atomic state and
the inner loop body wouldn't execute.

Could this cause the hardware state for unmodified colorops (including the newly
added csc_ff_enable and csc_ff_type) to remain zeroed out, resulting in the
loss of color correction or incorrect gamma during video playback or normal
desktop use?

> +					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/20260713051516.4087420-1-chaitanya.kumar.borah@intel.com?part=5