Re: [v7,04/15] drm/i915/color: Create a transfer function color pipeline

["Murthy, Arun R" <arun.r.murthy@intel.com>]

On 01-12-2025 12:16, Uma Shankar wrote:
> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>
> Add a color pipeline with three colorops in the sequence
>
>          1D LUT - 3x4 CTM - 1D LUT
>
> This pipeline can be used to do any color space conversion or HDR
> tone mapping
>
> v2: Change namespace to drm_plane_colorop*
> v3: Use simpler/pre-existing colorops for first iteration
> v4:
>   - s/*_tf_*/*_color_* (Jani)
>   - Refactor to separate files (Jani)
>   - Add missing space in comment (Suraj)
>   - Consolidate patch that adds/attaches pipeline property
> v5:
>   - Limit MAX_COLOR_PIPELINES to 2.(Suraj)
> 	Increase it as and when we add more pipelines.
>   - Remove redundant initialization code (Suraj)
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   drivers/gpu/drm/i915/Makefile                 |  1 +
>   .../drm/i915/display/intel_color_pipeline.c   | 97 +++++++++++++++++++
>   .../drm/i915/display/intel_color_pipeline.h   | 13 +++
>   drivers/gpu/drm/xe/Makefile                   |  1 +
>   4 files changed, 112 insertions(+)
>   create mode 100644 drivers/gpu/drm/i915/display/intel_color_pipeline.c
>   create mode 100644 drivers/gpu/drm/i915/display/intel_color_pipeline.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 7c19d5345d88..ca5c69d1cb08 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -241,6 +241,7 @@ i915-y += \
>   	display/intel_cmtg.o \
>   	display/intel_color.o \
>   	display/intel_colorop.o \
> +	display/intel_color_pipeline.o \
>   	display/intel_combo_phy.o \
>   	display/intel_connector.o \
>   	display/intel_crtc.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> new file mode 100644
> index 000000000000..1415f94dd3e3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +#include "intel_colorop.h"
> +#include "intel_color_pipeline.h"
> +#include "intel_de.h"
> +#include "intel_display_types.h"
> +#include "skl_universal_plane.h"
> +
> +#define MAX_COLOR_PIPELINES 2
Should this be same as INTEL_CB_MAX defined in intel_display_limits.h ?
> +#define PLANE_DEGAMMA_SIZE 128
> +#define PLANE_GAMMA_SIZE 32
> +
> +static
> +int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_enum_list *list)
> +{
> +	struct intel_colorop *colorop;
> +	struct drm_device *dev = plane->dev;
> +	int ret;
> +	struct drm_colorop *prev_op;
> +
> +	colorop = intel_colorop_create(INTEL_PLANE_CB_PRE_CSC_LUT);
> +
> +	ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, plane,
> +						  PLANE_DEGAMMA_SIZE,
> +						  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
> +						  DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +
> +	if (ret)
> +		return ret;
> +
> +	list->type = colorop->base.base.id;
> +	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", colorop->base.base.id);
> +
> +	/* TODO: handle failures and clean up */
> +	prev_op = &colorop->base;
> +
> +	colorop = intel_colorop_create(INTEL_PLANE_CB_CSC);
> +	ret = drm_plane_colorop_ctm_3x4_init(dev, &colorop->base, plane,
> +					     DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_set_next_property(prev_op, &colorop->base);
> +	prev_op = &colorop->base;
> +
> +	colorop = intel_colorop_create(INTEL_PLANE_CB_POST_CSC_LUT);
> +	ret = drm_plane_colorop_curve_1d_lut_init(dev, &colorop->base, plane,
> +						  PLANE_GAMMA_SIZE,
> +						  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
> +						  DRM_COLOROP_FLAG_ALLOW_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_set_next_property(prev_op, &colorop->base);
> +
> +	return 0;
> +}
> +
> +int intel_color_pipeline_plane_init(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_display *display = to_intel_display(dev);
> +	struct drm_property *prop;
> +	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
> +	int len = 0;
> +	int ret;
> +
> +	/* Currently expose pipeline only for HDR planes */
> +	if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
> +		return 0;
> +
> +	/* Add "Bypass" (i.e. NULL) pipeline */
> +	pipelines[len].type = 0;
> +	pipelines[len].name = "Bypass";
> +	len++;
Code for creating Bypass and COLOR_PIPELINE property is there in 
drm_core and is exported as drm_plane_create_color_pipeline_property()

Can this function be used to avoid code duplication?

Thanks and Regards,
Arun R Murthy
-------------------

> +
> +	/* Add pipeline consisting of transfer functions */
> +	ret = _intel_color_pipeline_plane_init(plane, &pipelines[len]);
> +	if (ret)
> +		return ret;
> +	len++;
> +
> +	/* Create COLOR_PIPELINE property and attach */
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_PIPELINE",
> +					pipelines, len);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	plane->color_pipeline_property = prop;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.h b/drivers/gpu/drm/i915/display/intel_color_pipeline.h
> new file mode 100644
> index 000000000000..7f1d32bc9202
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef __INTEL_COLOR_PIPELINE_H__
> +#define __INTEL_COLOR_PIPELINE_H__
> +
> +struct drm_plane;
> +
> +int intel_color_pipeline_plane_init(struct drm_plane *plane);
> +
> +#endif /* __INTEL_COLOR_PIPELINE_H__ */
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 3420725c4ba8..89f922d745ba 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -235,6 +235,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>   	i915-display/intel_cmtg.o \
>   	i915-display/intel_color.o \
>   	i915-display/intel_colorop.o \
> +	i915-display/intel_color_pipeline.o \
>   	i915-display/intel_combo_phy.o \
>   	i915-display/intel_connector.o \
>   	i915-display/intel_crtc.o \