Re: [RFC PATCH v2 4/9] drm/vkms: Add atomic check and matrix handling for CSC FF colorop

[Alex Hung <alex.hung@amd.com>]

On 3/30/26 09:34, Harry Wentland wrote:
> In vkms_plane_atomic_check(), validate that when a color pipeline
> is active and the framebuffer format is YUV:
>    - The first colorop must be DRM_COLOROP_CSC_FF
>    - The CSC FF colorop must not be bypassed
> 
> In vkms_plane_atomic_update(), when a color pipeline is active,
> read the csc_ff_type from the first colorop state and map it to
> the appropriate (color_encoding, color_range) pair for the existing
> get_conversion_matrix_to_argb_u16() function.
> 
> In apply_colorop(), add a no-op case for DRM_COLOROP_CSC_FF since
> the actual YUV-to-RGB conversion is already performed at format
> read time via the conversion matrix.
> 
> Assisted-by Claude:claude-opus-4.6
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>   drivers/gpu/drm/vkms/vkms_composer.c |  6 +++
>   drivers/gpu/drm/vkms/vkms_plane.c    | 70 +++++++++++++++++++++++++++-
>   2 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad0..e29376f1cbd7 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -187,6 +187,12 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
>   		if (colorop_state->data)
>   			apply_3x4_matrix(pixel,
>   					 (struct drm_color_ctm_3x4 *)colorop_state->data->data);
> +	} else if (colorop->type == DRM_COLOROP_CSC_FF) {
> +		/*
> +		 * CSC FF is a no-op here. YUV-to-RGB conversion is handled
> +		 * at format read time via the conversion_matrix set in
> +		 * vkms_plane_atomic_update().
> +		 */
>   	}
>   }
>   
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index ca7aee101a95..0f1ebdd9e391 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -6,6 +6,7 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_blend.h>
> +#include <drm/drm_colorop.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_gem_atomic_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
> @@ -148,8 +149,58 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>   	frame_info->rotation = new_state->rotation;
>   
>   	vkms_plane_state->pixel_read_line = get_pixel_read_line_function(fmt);
> -	get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding, new_state->color_range,
> -					  &vkms_plane_state->conversion_matrix);
> +
> +	if (new_state->color_pipeline) {
> +		struct drm_colorop *colorop = new_state->color_pipeline;
> +		struct drm_colorop_state *colorop_state;
> +
> +		drm_WARN_ON(plane->dev, colorop->type != DRM_COLOROP_CSC_FF);
Is this drm_WARN_ON necessary? How about a drm_dbg?

Either way, this patch is
Reviewed-by: Alex Hung <alex.hung@amd.com>
> +
> +		colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
> +
> +		if (colorop_state && !colorop_state->bypass) {
> +			enum drm_color_encoding encoding;
> +			enum drm_color_range range;
> +
> +			switch (colorop_state->csc_ff_type) {
> +			case DRM_COLOROP_CSC_FF_YUV601_RGB601:
> +				encoding = DRM_COLOR_YCBCR_BT601;
> +				range = DRM_COLOR_YCBCR_FULL_RANGE;
> +				break;
> +			case DRM_COLOROP_CSC_FF_YUV601_LIMITED_RGB601:
> +				encoding = DRM_COLOR_YCBCR_BT601;
> +				range = DRM_COLOR_YCBCR_LIMITED_RANGE;
> +				break;
> +			case DRM_COLOROP_CSC_FF_YUV709_RGB709:
> +				encoding = DRM_COLOR_YCBCR_BT709;
> +				range = DRM_COLOR_YCBCR_FULL_RANGE;
> +				break;
> +			case DRM_COLOROP_CSC_FF_YUV709_LIMITED_RGB709:
> +				encoding = DRM_COLOR_YCBCR_BT709;
> +				range = DRM_COLOR_YCBCR_LIMITED_RANGE;
> +				break;
> +			case DRM_COLOROP_CSC_FF_YUV2020_RGB2020:
> +				encoding = DRM_COLOR_YCBCR_BT2020;
> +				range = DRM_COLOR_YCBCR_FULL_RANGE;
> +				break;
> +			case DRM_COLOROP_CSC_FF_YUV2020_LIMITED_RGB2020:
> +				encoding = DRM_COLOR_YCBCR_BT2020;
> +				range = DRM_COLOR_YCBCR_LIMITED_RANGE;
> +				break;
> +			default:
> +				encoding = DRM_COLOR_YCBCR_BT709;
> +				range = DRM_COLOR_YCBCR_LIMITED_RANGE;
> +				break;
> +			}
> +
> +			get_conversion_matrix_to_argb_u16(fmt, encoding, range,
> +							  &vkms_plane_state->conversion_matrix);
> +		}
> +	} else {
> +		get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding,
> +						  new_state->color_range,
> +						  &vkms_plane_state->conversion_matrix);
> +	}
>   }
>   
>   static int vkms_plane_atomic_check(struct drm_plane *plane,
> @@ -175,6 +226,21 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
>   	if (ret != 0)
>   		return ret;
>   
> +	if (new_plane_state->color_pipeline && new_plane_state->fb->format->is_yuv) {
> +		struct drm_colorop *colorop = new_plane_state->color_pipeline;
> +		struct drm_colorop_state *colorop_state;
> +
> +		if (colorop->type != DRM_COLOROP_CSC_FF)
> +			return -EINVAL;
> +
> +		colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
> +		if (!colorop_state)
> +			return -EINVAL;
> +
> +		if (colorop_state->bypass)
> +			return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>