Re: [v6 5/9] drm/i915/display: Program CSC on SDR planes based on Fixed Matrix Colorop

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

Pre-existing issues:
- [High] Plane color pipeline configuration (including `csc_ff_enable` and LUT blobs) is silently discarded if an atomic commit updates the plane but omits the color pipeline objects.
--

--- Patch [5]: [v6 5/9] drm/i915/display: Program CSC on SDR planes based on Fixed Matrix Colorop ---
commit 17128052d77ad1b002034d74a8e48d89696c0df8
Author: Chaitanya Kumar Borah <[email protected]>

drm/i915/display: Program CSC on SDR planes based on Fixed Matrix Colorop

When a color pipeline is active, program the SDR plane fixed-function
CSC based on the Fixed Matrix Colorop's state. Re-use the existing plane
state variables for color_range and color_encoding.

> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> index d0f99a87c42e8..25ca049009efb 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane.c

[ ... ]

> @@ -485,11 +522,23 @@ 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) {
> -				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) {
> +					fm_type = new_colorop_state->fixed_matrix_type;
> +
> +					plane_state->hw.csc_ff_enable =
> +						!new_colorop_state->bypass;

[Severity: High]
This isn't a bug introduced by this patch, but does this iteration macro fail
to preserve the hardware state during page flips?

If userspace performs a simple plane update, such as a page flip to update the
FB, without explicitly including the color pipeline objects in the atomic
commit, for_each_new_colorop_in_state() in
intel_plane_color_copy_uapi_to_hw_state() will skip the unmodified colorops.

Since intel_plane_copy_uapi_to_hw_state() calls intel_plane_clear_hw_state()
prior to this step, the entire hardware state is cleared. As a result, the
new csc_ff_enable flag and other LUT pointers will not be restored,
permanently losing the hardware configuration.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 73be4c6ff1d3b..222c3b6bbe467 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c

[ ... ]

> @@ -1245,9 +1245,11 @@ static u32 glk_plane_color_ctl_input_csc(const struct intel_plane_state *plane_s
>  	struct intel_display *display = to_intel_display(plane_state);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	bool color_pipeline = !!plane_state->uapi.color_pipeline;
> +	bool needs_csc = color_pipeline ? plane_state->hw.csc_ff_enable : fb->format->is_yuv;
>  	u32 ctl = 0;
>  
> -	if (!fb->format->is_yuv)
> +	if (!needs_csc)
>  		return 0;

[Severity: High]
Because of the issue mentioned above, could needs_csc evaluate to false
incorrectly?

When the csc_ff_enable flag is silently dropped during an atomic commit that
does not modify colorops, glk_plane_color_ctl_input_csc() will evaluate
needs_csc to false. This disables the hardware CSC, which can lead to
incorrect colors or visual artifacts on the display.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5