Re: [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added Fixed Matrix (FM) colorop is unhandled by the AMD atomic check/commit logic, breaking the entire AMD color pipeline feature.
--

--- Patch [6]: [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline ---
commit d87e1fc27fd29f8b37f2f841d69915be8eea3f32
Author: Harry Wentland <harry.wentland@amd.com>

drm/amd/display: Add fixed matrix colorop to color pipeline

This commit inserts a fixed matrix colorop as the first operation in the amdgpu
color pipeline, before the DEGAM 1D curve. This enables userspace to select
YUV-to-RGB conversion via the color pipeline for YCbCr framebuffers.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> index 056a76b88f438..1ed12cdce3172 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
[ ... ]
> @@ -73,6 +81,23 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
>  
>  	memset(ops, 0, sizeof(ops));
>  
> +	/* Fixed Matrix (YUV to RGB) */
> +	ops[i] = kzalloc_obj(*ops[0]);
> +	if (!ops[i]) {
> +		ret = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	ret = drm_plane_colorop_fixed_matrix_init(dev, ops[i], plane, &dm_colorop_funcs,
> +						  amdgpu_dm_supported_fm,
> +						  DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +	if (ret)
> +		goto cleanup;
> +
> +	list->type = ops[i]->base.id;
> +
> +	i++;

[Severity: High]
Will this cause atomic checks to unconditionally fail for the color pipeline?

Because the fixed matrix colorop is now the first operation in the pipeline,
userspace will pass it in plane_state->color_pipeline.

However, looking at amdgpu_dm_plane_set_colorop_properties(), it expects the
first operation to be the DEGAM TF:

amdgpu_dm_color.c:amdgpu_dm_plane_set_colorop_properties() {
    ...
    /* 1D Curve - DEGAM TF */
    if (!colorop)
        return -EINVAL;

    ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state,
                                         colorop);
    if (ret)
        return ret;
    ...
}

When this passes the new fixed matrix colorop to
__set_dm_plane_colorop_degamma(), it ultimately calls
__set_colorop_in_tf_1d_curve() which has a strict type check:

amdgpu_dm_color.c:__set_colorop_in_tf_1d_curve() {
    ...
    if (colorop->type != DRM_COLOROP_1D_CURVE)
        return -EINVAL;
    ...
}

Since the first colorop is now DRM_COLOROP_FIXED_MATRIX, this type check will
fail and return -EINVAL.

Does amdgpu_dm_plane_set_colorop_properties() need to be updated to parse and
handle the new fixed matrix colorop before processing the DEGAM TF?

> +
>  	/* 1D curve - DEGAM TF */
>  	ops[i] = kzalloc_obj(*ops[0]);
>  	if (!ops[i]) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=6