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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 12/2/2025 1:26 PM, Murthy, Arun R wrote:
> 
> 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?

Makes sense. Would be added in the next version.

Regards
Chaitanya

> 
> 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 \