[PATCH 00/10] drm/i915/color: Enable SDR plane color pipeline

[PATCH 00/10] drm/i915/color: Enable SDR plane color pipeline

[Chaitanya Kumar Borah]

This series adds color pipeline support for SDR planes in i915 and
exposes the functionality to userspace through the DRM colorop
framework.

In contrast to HDR planes, SDR planes have LUTs with smaller sizes
and a fixed function CSC block in contrast to a programmable CTM.

The series first introduces a new DRM colorop type,
DRM_COLOROP_CSC_FF, which represents fixed-function CSC blocks where
userspace selects predefined hardware conversion modes instead of
programming arbitrary matrices.

With that, the SDR plane color pipeline looks like.

[1D LUT] -> [CSC_FF] -> [1D LUT]

The series also fixes an issue in HDR pre-CSC LUT programming where the
loop condition prevented the last entries from being programmed.

Chaitanya Kumar Borah (5):
  drm/colorop: Add DRM_COLOROP_CSC_FF
  drm/i915/color: Add CSC on SDR plane color pipeline
  drm/i915/color: Program fixed-function CSC on SDR planes
  drm/i915/color: Add support for 1D LUT in SDR planes
  drm/i915/color: Add color pipeline support for SDR planes

Pranay Samala (5):
  drm/i915/color: Fix HDR pre-CSC LUT programming loop
  drm/i915/color: Extract HDR pre-CSC LUT programming to helper function
  drm/i915/color: Program Pre-CSC registers for SDR
  drm/i915/color: Extract HDR post-CSC LUT programming to helper
    function
  drm/i915/color: Program Plane Post CSC registers for SDR planes

 drivers/gpu/drm/drm_atomic.c                  |   4 +
 drivers/gpu/drm/drm_atomic_uapi.c             |   4 +
 drivers/gpu/drm/drm_colorop.c                 | 105 +++++++
 drivers/gpu/drm/i915/display/intel_color.c    | 288 +++++++++++++-----
 .../drm/i915/display/intel_color_pipeline.c   |  37 ++-
 .../drm/i915/display/intel_display_limits.h   |   1 +
 .../drm/i915/display/intel_display_types.h    |   2 +
 drivers/gpu/drm/i915/display/intel_plane.c    |  12 +-
 .../drm/i915/display/skl_universal_plane.c    |  30 ++
 include/drm/drm_colorop.h                     |  72 +++++
 include/uapi/drm/drm_mode.h                   |  13 +
 11 files changed, 475 insertions(+), 93 deletions(-)

-- 
2.25.1

[PATCH 01/10] drm/colorop: Add DRM_COLOROP_CSC_FF

[Chaitanya Kumar Borah]

Introduce DRM_COLOROP_CSC_FF, a new colorop type representing a
fixed-function Color Space Conversion (CSC) block.

Unlike CTM-based colorops, this block does not expose programmable
coefficients. Instead, userspace selects one of the predefined
hardware modes via a new CSC_FF_TYPE enum property. Supported modes
include common YUV->RGB and RGB709->RGB2020 conversions.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/drm_atomic.c      |   4 ++
 drivers/gpu/drm/drm_atomic_uapi.c |   4 ++
 drivers/gpu/drm/drm_colorop.c     | 105 ++++++++++++++++++++++++++++++
 include/drm/drm_colorop.h         |  72 ++++++++++++++++++++
 include/uapi/drm/drm_mode.h       |  13 ++++
 5 files changed, 198 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 04925166df98..7296b844e3fd 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -844,6 +844,10 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
 			   drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
 		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
 		break;
+	case DRM_COLOROP_CSC_FF:
+		drm_printf(p, "\tcsc_ff_type=%s\n",
+			   drm_get_colorop_csc_ff_type_name(state->csc_ff_type));
+		break;
 	default:
 		break;
 	}
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 87de41fb4459..9af73325aa93 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -757,6 +757,8 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 	} else if (property == colorop->data_property) {
 		return drm_atomic_color_set_data_property(colorop, state,
 							  property, val);
+	} else if (property == colorop->csc_ff_type_property) {
+		state->csc_ff_type = val;
 	} else {
 		drm_dbg_atomic(colorop->dev,
 			       "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
@@ -789,6 +791,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 		*val = colorop->lut3d_interpolation;
 	else if (property == colorop->data_property)
 		*val = (state->data) ? state->data->base.id : 0;
+	else if (property == colorop->csc_ff_type_property)
+		*val = state->csc_ff_type;
 	else
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index f421c623b3f0..49422c625f4d 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -68,6 +68,7 @@ static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
 	{ DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
 	{ DRM_COLOROP_MULTIPLIER, "Multiplier"},
 	{ DRM_COLOROP_3D_LUT, "3D LUT"},
+	{ DRM_COLOROP_CSC_FF, "CSC Fixed-Function"},
 };
 
 static const char * const colorop_curve_1d_type_names[] = {
@@ -90,6 +91,13 @@ static const struct drm_prop_enum_list drm_colorop_lut3d_interpolation_list[] =
 	{ DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL, "Tetrahedral" },
 };
 
+static const char * const colorop_csc_ff_type_names[] = {
+	[DRM_COLOROP_CSC_FF_YUV601_RGB601]   = "YUV601 to RGB601",
+	[DRM_COLOROP_CSC_FF_YUV709_RGB709]   = "YUV709 to RGB709",
+	[DRM_COLOROP_CSC_FF_YUV2020_RGB2020] = "YUV2020 to RGB2020",
+	[DRM_COLOROP_CSC_FF_RGB709_RGB2020]  = "RGB709 to RGB2020",
+};
+
 /* Init Helpers */
 
 static int drm_plane_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
@@ -459,6 +467,80 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
 }
 EXPORT_SYMBOL(drm_plane_colorop_3dlut_init);
 
+/**
+ * drm_plane_colorop_csc_ff_init - Initialize a DRM_COLOROP_CSC_FF
+ *
+ * @dev: DRM device
+ * @colorop: The drm_colorop object to initialize
+ * @plane: The associated drm_plane
+ * @funcs: control functions for the new colorop
+ * @supported_csc_ff: A bitfield of supported drm_plane_colorop_csc_ff_type enum values,
+ *                    created using BIT(csc_ff_type) and combined with the OR '|'
+ *                    operator.
+ * @flags: bitmask of misc, see DRM_COLOROP_FLAG_* defines.
+ * @return zero on success, -E value on failure
+ */
+int drm_plane_colorop_csc_ff_init(struct drm_device *dev, struct drm_colorop *colorop,
+				  struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
+				  u64 supported_csc_ff, uint32_t flags)
+{
+	struct drm_prop_enum_list enum_list[DRM_COLOROP_CSC_FF_COUNT];
+	int i, len;
+
+	struct drm_property *prop;
+	int ret;
+
+	if (!supported_csc_ff) {
+		drm_err(dev,
+			"No supported CSC op for new CSC FF colorop on [PLANE:%d:%s]\n",
+			plane->base.id, plane->name);
+		return -EINVAL;
+	}
+
+	if ((supported_csc_ff & -BIT(DRM_COLOROP_CSC_FF_COUNT)) != 0) {
+		drm_err(dev, "Unknown CSC provided on [PLANE:%d:%s]\n",
+			plane->base.id, plane->name);
+		return -EINVAL;
+	}
+
+	ret = drm_plane_colorop_init(dev, colorop, plane, funcs, DRM_COLOROP_CSC_FF, flags);
+	if (ret)
+		return ret;
+
+	len = 0;
+	for (i = 0; i < DRM_COLOROP_CSC_FF_COUNT; i++) {
+		if ((supported_csc_ff & BIT(i)) == 0)
+			continue;
+
+		enum_list[len].type = i;
+		enum_list[len].name = colorop_csc_ff_type_names[i];
+		len++;
+	}
+
+	if (WARN_ON(len <= 0))
+		return -EINVAL;
+
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, "CSC_FF_TYPE",
+					enum_list, len);
+
+	if (!prop)
+		return -ENOMEM;
+
+	colorop->csc_ff_type_property = prop;
+	/*
+	 * Default to the first supported CSC mode as provided by the driver.
+	 * Intuitively this should be something that keeps the colorop in pixel bypass
+	 * mode but that is already handled via the standard colorop bypass
+	 * property.
+	 */
+	drm_object_attach_property(&colorop->base, colorop->csc_ff_type_property,
+				   enum_list[0].type);
+	drm_colorop_reset(colorop);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_colorop_csc_ff_init);
+
 static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop,
 							struct drm_colorop_state *state)
 {
@@ -513,6 +595,13 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
 						      &val);
 		colorop_state->curve_1d_type = val;
 	}
+
+	if (colorop->csc_ff_type_property) {
+		drm_object_property_get_default_value(&colorop->base,
+						      colorop->csc_ff_type_property,
+						      &val);
+		colorop_state->csc_ff_type = val;
+	}
 }
 
 /**
@@ -551,6 +640,7 @@ static const char * const colorop_type_name[] = {
 	[DRM_COLOROP_CTM_3X4] = "3x4 Matrix",
 	[DRM_COLOROP_MULTIPLIER] = "Multiplier",
 	[DRM_COLOROP_3D_LUT] = "3D LUT",
+	[DRM_COLOROP_CSC_FF] = "CSC Fixed-Function",
 };
 
 static const char * const colorop_lu3d_interpolation_name[] = {
@@ -607,6 +697,21 @@ const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_inte
 	return colorop_lu3d_interpolation_name[type];
 }
 
+/**
+ * drm_get_colorop_csc_ff_type_name: return a string for interpolation type
+ * @type: csc ff type to compute name of
+ *
+ * In contrast to the other drm_get_*_name functions this one here returns a
+ * const pointer and hence is threadsafe.
+ */
+const char *drm_get_colorop_csc_ff_type_name(enum drm_colorop_csc_ff_type type)
+{
+	if (WARN_ON(type >= ARRAY_SIZE(colorop_csc_ff_type_names)))
+		return "unknown";
+
+	return colorop_csc_ff_type_names[type];
+}
+
 /**
  * drm_colorop_set_next_property - sets the next pointer
  * @colorop: drm colorop
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index bd082854ca74..2cd8e0779c2a 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -134,6 +134,60 @@ enum drm_colorop_curve_1d_type {
 	DRM_COLOROP_1D_CURVE_COUNT
 };
 
+/**
+ * enum drm_colorop_csc_ff_type - type of CSC Fixed-Function
+ *
+ * Describes a CSC operation to be applied by the DRM_COLOROP_CSC_FF colorop.
+ */
+enum drm_colorop_csc_ff_type {
+	/**
+	 * @DRM_COLOROP_CSC_FF_YUV601_RGB601
+	 *
+	 * enum string "YUV601 to RGB601"
+	 *
+	 * Selects the fixed-function CSC preset that converts YUV
+	 * (BT.601) colorimetry to RGB (BT.601).
+	 */
+	DRM_COLOROP_CSC_FF_YUV601_RGB601,
+
+	/**
+	 * @DRM_COLOROP_CSC_FF_YUV709_RGB709:
+	 *
+	 * enum string "YUV709 to RGB709"
+	 *
+	 * Selects the fixed-function CSC preset that converts YUV
+	 * (BT.709) colorimetry to RGB (BT.709).
+	 */
+	DRM_COLOROP_CSC_FF_YUV709_RGB709,
+
+	/**
+	 * @DRM_COLOROP_CSC_FF_YUV2020_RGB2020:
+	 *
+	 * enum string "YUV2020 to RGB2020"
+	 *
+	 * Selects the fixed-function CSC preset that converts YUV
+	 * (BT.2020) colorimetry to RGB (BT.2020).
+	 */
+	DRM_COLOROP_CSC_FF_YUV2020_RGB2020,
+
+	/**
+	 * @DRM_COLOROP_CSC_FF_RGB709_RGB2020:
+	 *
+	 * enum string "RGB709 to RGB2020"
+	 *
+	 * Selects the fixed-function CSC preset that converts RGB
+	 * (BT.709) colorimetry to RGB (BT.2020).
+	 */
+	DRM_COLOROP_CSC_FF_RGB709_RGB2020,
+
+	/**
+	 * @DRM_COLOROP_CSC_FF_COUNT:
+	 *
+	 * enum value denoting the size of the enum
+	 */
+	DRM_COLOROP_CSC_FF_COUNT
+};
+
 /**
  * struct drm_colorop_state - mutable colorop state
  */
@@ -183,6 +237,13 @@ struct drm_colorop_state {
 	 */
 	struct drm_property_blob *data;
 
+	/**
+	 * @csc_ff_type:
+	 *
+	 * Type of Fixed function CSC.
+	 */
+	enum drm_colorop_csc_ff_type csc_ff_type;
+
 	/** @state: backpointer to global drm_atomic_state */
 	struct drm_atomic_state *state;
 };
@@ -368,6 +429,13 @@ struct drm_colorop {
 	 */
 	struct drm_property *data_property;
 
+	/**
+	 * @csc_ff_type_property:
+	 *
+	 * Sub-type for DRM_COLOROP_CSC_FF type.
+	 */
+	struct drm_property *csc_ff_type_property;
+
 	/**
 	 * @next_property:
 	 *
@@ -424,6 +492,9 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
 				 uint32_t lut_size,
 				 enum drm_colorop_lut3d_interpolation_type interpolation,
 				 uint32_t flags);
+int drm_plane_colorop_csc_ff_init(struct drm_device *dev, struct drm_colorop *colorop,
+				  struct drm_plane *plane, const struct drm_colorop_funcs *funcs,
+				  u64 supported_csc_ff, uint32_t flags);
 
 struct drm_colorop_state *
 drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop);
@@ -480,6 +551,7 @@ drm_get_colorop_lut1d_interpolation_name(enum drm_colorop_lut1d_interpolation_ty
 
 const char *
 drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type);
+const char *drm_get_colorop_csc_ff_type_name(enum drm_colorop_csc_ff_type type);
 
 void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 3693d82b5279..f7808e7ea984 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -968,6 +968,19 @@ enum drm_colorop_type {
 	 *         color = lut3d[index]
 	 */
 	DRM_COLOROP_3D_LUT,
+
+	/**
+	 * @DRM_COLOROP_CSC_FF:
+	 *
+	 * enum string "CSC Fixed-Function"
+	 *
+	 * A fixed-function Color Space Conversion block where the coefficients
+	 * are not programmable but selected from predefined hardware modes via
+	 * the CSC_FF_TYPE enum property. The driver advertises the supported
+	 * CSC modes through this property.
+	 */
+	DRM_COLOROP_CSC_FF,
+
 };
 
 /**
-- 
2.25.1

Claude review: drm/colorop: Add DRM_COLOROP_CSC_FF

[Claude Code Review Bot]

Patch Review

Good addition to the DRM colorop framework. The implementation follows existing patterns well.

Minor observations:

- The `drm_get_colorop_csc_ff_type_name` doc comment says "return a string for interpolation type" — this is a copy-paste error from the LUT3D interpolation function. It should say "return a string for CSC FF type".

```c
/**
 * drm_get_colorop_csc_ff_type_name: return a string for interpolation type
 * @type: csc ff type to compute name of
```

- In `drm_plane_colorop_csc_ff_init`, if the `drm_plane_colorop_init` call succeeds but `drm_property_create_enum` returns NULL, the function returns `-ENOMEM` without cleaning up the partially initialized colorop. The existing patterns in other `*_init` functions have the same issue, so this is pre-existing, but worth noting.

- The `WARN_ON(len <= 0)` check after the loop is dead code since `supported_csc_ff` was already validated non-zero above. It's a reasonable defensive check though.

---
Generated by Claude Code Patch Reviewer

[PATCH 02/10] drm/i915/color: Add CSC on SDR plane color pipeline

[Chaitanya Kumar Borah]

Add the fixed-function CSC block to color pipeline in SDR planes
as a DRM_COLOROP_CSC_FF colorop.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 .../drm/i915/display/intel_color_pipeline.c   | 22 ++++++++++++++++++-
 .../drm/i915/display/intel_display_limits.h   |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
index 6cf8080ee800..f368a896d2fc 100644
--- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
+++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
@@ -43,6 +43,16 @@ static const enum intel_color_block hdr_plane_pipeline[] = {
 	INTEL_PLANE_CB_POST_CSC_LUT,
 };
 
+static const enum intel_color_block sdr_plane_pipeline[] = {
+	INTEL_PLANE_CB_CSC_FF,
+};
+
+static const u64 intel_plane_supported_csc_ff =
+		BIT(DRM_COLOROP_CSC_FF_YUV601_RGB601) |
+		BIT(DRM_COLOROP_CSC_FF_YUV709_RGB709) |
+		BIT(DRM_COLOROP_CSC_FF_YUV2020_RGB2020) |
+		BIT(DRM_COLOROP_CSC_FF_RGB709_RGB2020);
+
 static bool plane_has_3dlut(struct intel_display *display, enum pipe pipe,
 			    struct drm_plane *plane)
 {
@@ -92,6 +102,12 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
 							  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
 							  DRM_COLOROP_FLAG_ALLOW_BYPASS);
 		break;
+	case INTEL_PLANE_CB_CSC_FF:
+		ret = drm_plane_colorop_csc_ff_init(dev, &colorop->base, plane,
+						    &intel_colorop_funcs,
+						    intel_plane_supported_csc_ff,
+						    DRM_COLOROP_FLAG_ALLOW_BYPASS);
+		break;
 	default:
 		drm_err(plane->dev, "Invalid colorop id [%d]", id);
 		ret = -EINVAL;
@@ -122,13 +138,17 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
 	int pipeline_len;
 	int ret = 0;
 	int i;
+	bool is_hdr = icl_is_hdr_plane(display, to_intel_plane(plane)->id);
 
 	if (plane_has_3dlut(display, pipe, plane)) {
 		pipeline = xe3plpd_primary_plane_pipeline;
 		pipeline_len = ARRAY_SIZE(xe3plpd_primary_plane_pipeline);
-	} else {
+	} else if (is_hdr) {
 		pipeline = hdr_plane_pipeline;
 		pipeline_len = ARRAY_SIZE(hdr_plane_pipeline);
+	} else {
+		pipeline = sdr_plane_pipeline;
+		pipeline_len = ARRAY_SIZE(sdr_plane_pipeline);
 	}
 
 	for (i = 0; i < pipeline_len; i++) {
diff --git a/drivers/gpu/drm/i915/display/intel_display_limits.h b/drivers/gpu/drm/i915/display/intel_display_limits.h
index 453f7b720815..f4aad54472ce 100644
--- a/drivers/gpu/drm/i915/display/intel_display_limits.h
+++ b/drivers/gpu/drm/i915/display/intel_display_limits.h
@@ -167,6 +167,7 @@ enum aux_ch {
 enum intel_color_block {
 	INTEL_PLANE_CB_PRE_CSC_LUT,
 	INTEL_PLANE_CB_CSC,
+	INTEL_PLANE_CB_CSC_FF,
 	INTEL_PLANE_CB_POST_CSC_LUT,
 	INTEL_PLANE_CB_3DLUT,
 
-- 
2.25.1

Claude review: drm/i915/color: Add CSC on SDR plane color pipeline

[Claude Code Review Bot]

Patch Review

Clean patch. Adds the SDR pipeline definition and routes planes correctly based on `icl_is_hdr_plane`.

One concern: the `sdr_plane_pipeline` initially has only `INTEL_PLANE_CB_CSC_FF` — the LUTs are added in patch 4. This means between patches 2-3 and patch 4, the SDR pipeline would be incomplete if bisecting. Not a functional issue since patch 10 is what actually enables the pipeline for userspace, but the intermediate states are worth being aware of for bisection.

---
Generated by Claude Code Patch Reviewer

[PATCH 03/10] drm/i915/color: Program fixed-function CSC on SDR planes

[Chaitanya Kumar Borah]

Program the fixed-function CSC block for SDR planes based on the
DRM_COLOROP_CSC_FF state.

Track the bypass state explicitly as a boolean in the plane hw state
since bypass is managed separately from the CSC_FF enum value in the
colorop framework.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_plane.c    | 12 ++++++--
 .../drm/i915/display/skl_universal_plane.c    | 30 +++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index e189f8c39ccb..02b1cee18e4a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -679,6 +679,8 @@ struct intel_plane_state {
 		enum drm_color_range color_range;
 		enum drm_scaling_filter scaling_filter;
 		struct drm_property_blob *ctm, *degamma_lut, *gamma_lut, *lut_3d;
+		enum drm_colorop_csc_ff_type csc_ff_type; /* For SDR plane */
+		bool csc_ff_enable;
 	} hw;
 
 	struct i915_vma *ggtt_vma;
diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
index e06a0618b4c6..c271a0ceb94e 100644
--- a/drivers/gpu/drm/i915/display/intel_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_plane.c
@@ -378,11 +378,19 @@ intel_plane_color_copy_uapi_to_hw_state(struct intel_plane_state *plane_state,
 	while (iter_colorop) {
 		for_each_new_colorop_in_state(state, 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) {
+					plane_state->hw.csc_ff_enable =
+						!new_colorop_state->bypass;
+					plane_state->hw.csc_ff_type =
+						new_colorop_state->csc_ff_type;
+				} else {
+					blob = new_colorop_state->bypass ?
+						NULL : new_colorop_state->data;
+					changed |= intel_plane_colorop_replace_blob(plane_state,
 									    intel_colorop,
 									    blob);
+				}
 			}
 		}
 		iter_colorop = iter_colorop->next;
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 677f1339b7f8..3d96975f97ae 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1239,6 +1239,32 @@ static u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	return plane_color_ctl;
 }
 
+static u32 intel_csc_ff_type_to_csc_mode(enum drm_colorop_csc_ff_type csc_ff_type,
+					 bool enable)
+{
+	u32 csc_mode = PLANE_COLOR_CSC_MODE_BYPASS;
+
+	if (enable) {
+		switch (csc_ff_type) {
+		case DRM_COLOROP_CSC_FF_YUV601_RGB601:
+			csc_mode = PLANE_COLOR_CSC_MODE_YUV601_TO_RGB601;
+			break;
+		case DRM_COLOROP_CSC_FF_YUV709_RGB709:
+			csc_mode = PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
+			break;
+		case DRM_COLOROP_CSC_FF_YUV2020_RGB2020:
+			csc_mode = PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020;
+			break;
+		case DRM_COLOROP_CSC_FF_RGB709_RGB2020:
+			csc_mode = PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020;
+			break;
+		default:
+			csc_mode = PLANE_COLOR_CSC_MODE_BYPASS;
+		}
+	}
+	return csc_mode;
+}
+
 static u32 glk_plane_color_ctl(const struct intel_plane_state *plane_state)
 {
 	struct intel_display *display = to_intel_display(plane_state);
@@ -1270,6 +1296,10 @@ static u32 glk_plane_color_ctl(const struct intel_plane_state *plane_state)
 			plane_color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
 	}
 
+	if (!icl_is_hdr_plane(display, plane->id))
+		plane_color_ctl |= intel_csc_ff_type_to_csc_mode(plane_state->hw.csc_ff_type,
+								plane_state->hw.csc_ff_enable);
+
 	if (plane_state->force_black)
 		plane_color_ctl |= PLANE_COLOR_PLANE_CSC_ENABLE;
 
-- 
2.25.1

Claude review: drm/i915/color: Program fixed-function CSC on SDR planes

[Claude Code Review Bot]

Patch Review

**BUG (Critical) — CSC mode register corruption for YUV formats on SDR planes.**

In `glk_plane_color_ctl()`, the existing code for SDR planes with YUV formats already sets `PLANE_COLOR_CSC_MODE_MASK` (bits 19:17):

```c
if (fb->format->is_yuv && !icl_is_hdr_plane(display, plane->id)) {
    switch (plane_state->hw.color_encoding) {
    case DRM_COLOR_YCBCR_BT709:
        plane_color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;  // sets bits 19:17
        break;
    ...
    }
}
```

Then the new code unconditionally OR's in another CSC mode for non-HDR planes:

```c
if (!icl_is_hdr_plane(display, plane->id))
    plane_color_ctl |= intel_csc_ff_type_to_csc_mode(plane_state->hw.csc_ff_type,
                                                      plane_state->hw.csc_ff_enable);
```

Since both use the same `PLANE_COLOR_CSC_MODE_MASK` bitfield (bits 19:17, defined via `REG_GENMASK(19, 17)`), OR-ing two non-zero CSC mode values will produce a corrupted value. For example, if the YUV path sets mode 2 (`0b010`) and CSC_FF sets mode 1 (`0b001`), the result is mode 3 (`0b011`), which is the wrong conversion.

This needs to either:
1. Make the two paths mutually exclusive (the CSC_FF colorop should take precedence when enabled, replacing the legacy `color_encoding` path), or
2. Use a mask-and-set pattern to clear the field before writing.

---
Generated by Claude Code Patch Reviewer

[PATCH 04/10] drm/i915/color: Add support for 1D LUT in SDR planes

[Chaitanya Kumar Borah]

Extend the SDR plane color pipeline to include pre- and post-CSC
1D LUT blocks.

SDR planes use a smaller LUT size than HDR planes and therefore
initialize the 1D LUT colorops with the appropriate hardware
capacity.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color_pipeline.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
index f368a896d2fc..47b3bcec7b18 100644
--- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
+++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
@@ -15,6 +15,7 @@
 #define MAX_COLOROP 4
 #define PLANE_DEGAMMA_SIZE 128
 #define PLANE_GAMMA_SIZE 32
+#define PLANE_DEGAMMA_SIZE_SDR 32
 
 static const struct drm_colorop_funcs intel_colorop_funcs = {
 	.destroy = intel_colorop_destroy,
@@ -44,7 +45,9 @@ static const enum intel_color_block hdr_plane_pipeline[] = {
 };
 
 static const enum intel_color_block sdr_plane_pipeline[] = {
+	INTEL_PLANE_CB_PRE_CSC_LUT,
 	INTEL_PLANE_CB_CSC_FF,
+	INTEL_PLANE_CB_POST_CSC_LUT,
 };
 
 static const u64 intel_plane_supported_csc_ff =
@@ -67,8 +70,10 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
 							     enum intel_color_block id)
 {
 	struct drm_device *dev = plane->dev;
+	struct intel_display *display = to_intel_display(dev);
 	struct intel_colorop *colorop;
 	int ret;
+	bool is_hdr = icl_is_hdr_plane(display, to_intel_plane(plane)->id);
 
 	colorop = intel_colorop_create(id);
 
@@ -80,7 +85,9 @@ struct intel_colorop *intel_color_pipeline_plane_add_colorop(struct drm_plane *p
 		ret = drm_plane_colorop_curve_1d_lut_init(dev,
 							  &colorop->base, plane,
 							  &intel_colorop_funcs,
-							  PLANE_DEGAMMA_SIZE,
+							  is_hdr ?
+							  PLANE_DEGAMMA_SIZE :
+							  PLANE_DEGAMMA_SIZE_SDR,
 							  DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR,
 							  DRM_COLOROP_FLAG_ALLOW_BYPASS);
 		break;
-- 
2.25.1

Claude review: drm/i915/color: Add support for 1D LUT in SDR planes

[Claude Code Review Bot]

Patch Review

Clean patch. Extends the SDR pipeline with pre/post CSC LUTs and correctly differentiates LUT sizes between HDR (128) and SDR (32) planes for pre-CSC.

---
Generated by Claude Code Patch Reviewer

[PATCH 05/10] drm/i915/color: Fix HDR pre-CSC LUT programming loop

[Chaitanya Kumar Borah]

From: Pranay Samala <pranay.samala@intel.com>

The integer lut programming loop never executes completely due to
incorrect condition (i++ > 130).

Fix to properly program 129th+ entries for values > 1.0.

Cc: <stable@vger.kernel.org> #v6.19
Fixes: 82caa1c8813f ("drm/i915/color: Program Pre-CSC registers")
Signed-off-by: Pranay Samala <pranay.samala@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index e7950655434b..6d1cffc6d2be 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -3976,7 +3976,7 @@ xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 				intel_de_write_dsb(display, dsb,
 						   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
 						   (1 << 24));
-			} while (i++ > 130);
+			} while (i++ < 130);
 		} else {
 			for (i = 0; i < lut_size; i++) {
 				u32 v = (i * ((1 << 24) - 1)) / (lut_size - 1);
-- 
2.25.1

Claude review: drm/i915/color: Fix HDR pre-CSC LUT programming loop

[Claude Code Review Bot]

Patch Review

Correct fix — `i++ > 130` was a clear typo that should be `i++ < 130`, causing the do-while to exit immediately (only writing one entry instead of the intended 3). The `do { ... } while (i++ < 130)` with `i` starting at 128 writes entries 128, 129, 130 — 3 clamp values, which is correct.

**Ordering issue**: This patch has `Cc: stable` and `Fixes:` tags but depends on no other patch in the series. It should be patch 1 to enable clean backporting. Currently being at position 5 means backporting it requires either cherry-picking (which works since it's independent) or backporting patches 1-4 too (which are features, not fixes).

---
Generated by Claude Code Patch Reviewer

[PATCH 06/10] drm/i915/color: Extract HDR pre-CSC LUT programming to helper function

[Chaitanya Kumar Borah]

From: Pranay Samala <pranay.samala@intel.com>

As we prepare to add support for LUT programming in SDR planes,
refactor HDR plane pre-CSC LUT programming to a helper.

Signed-off-by: Pranay Samala <pranay.samala@intel.com>
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 92 ++++++++++++----------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 6d1cffc6d2be..17ab4364faea 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -3943,6 +3943,55 @@ xelpd_load_plane_csc_matrix(struct intel_dsb *dsb,
 			   ctm_to_twos_complement(input[11], 0, 12));
 }
 
+static void
+xelpd_load_hdr_pre_csc_lut(struct intel_display *display,
+			   struct intel_dsb *dsb,
+			   enum pipe pipe,
+			   enum plane_id plane,
+			   const struct drm_color_lut32 *pre_csc_lut)
+{
+	u32 lut_size = 128;
+	u32 lut_val;
+	int i;
+
+	intel_de_write_dsb(display, dsb,
+			   PLANE_PRE_CSC_GAMC_INDEX_ENH(pipe, plane, 0),
+			   PLANE_PAL_PREC_AUTO_INCREMENT);
+
+	if (pre_csc_lut) {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = drm_color_lut32_extract(pre_csc_lut[i].green, 24);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   lut_val);
+		}
+
+		/* Program the max register to clamp values > 1.0. */
+		/* TODO: Restrict to 0x7ffffff */
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   (1 << 24));
+		} while (i++ < 130);
+	} else {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = (i * ((1 << 24) - 1)) / (lut_size - 1);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0), lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   1 << 24);
+		} while (i++ < 130);
+	}
+
+	intel_de_write_dsb(display, dsb, PLANE_PRE_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
+}
+
 static void
 xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 				const struct intel_plane_state *plane_state)
@@ -3952,48 +4001,9 @@ xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 	enum pipe pipe = to_intel_plane(state->plane)->pipe;
 	enum plane_id plane = to_intel_plane(state->plane)->id;
 	const struct drm_color_lut32 *pre_csc_lut = plane_state->hw.degamma_lut->data;
-	u32 i, lut_size;
 
-	if (icl_is_hdr_plane(display, plane)) {
-		lut_size = 128;
-
-		intel_de_write_dsb(display, dsb,
-				   PLANE_PRE_CSC_GAMC_INDEX_ENH(pipe, plane, 0),
-				   PLANE_PAL_PREC_AUTO_INCREMENT);
-
-		if (pre_csc_lut) {
-			for (i = 0; i < lut_size; i++) {
-				u32 lut_val = drm_color_lut32_extract(pre_csc_lut[i].green, 24);
-
-				intel_de_write_dsb(display, dsb,
-						   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   lut_val);
-			}
-
-			/* Program the max register to clamp values > 1.0. */
-			/* TODO: Restrict to 0x7ffffff */
-			do {
-				intel_de_write_dsb(display, dsb,
-						   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   (1 << 24));
-			} while (i++ < 130);
-		} else {
-			for (i = 0; i < lut_size; i++) {
-				u32 v = (i * ((1 << 24) - 1)) / (lut_size - 1);
-
-				intel_de_write_dsb(display, dsb,
-						   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0), v);
-			}
-
-			do {
-				intel_de_write_dsb(display, dsb,
-						   PLANE_PRE_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   1 << 24);
-			} while (i++ < 130);
-		}
-
-		intel_de_write_dsb(display, dsb, PLANE_PRE_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
-	}
+	if (icl_is_hdr_plane(display, plane))
+		xelpd_load_hdr_pre_csc_lut(display, dsb, pipe, plane, pre_csc_lut);
 }
 
 static void
-- 
2.25.1

Claude review: drm/i915/color: Extract HDR pre-CSC LUT programming to helper function

[Claude Code Review Bot]

Patch Review

Mechanical refactoring — moves the HDR pre-CSC LUT programming into `xelpd_load_hdr_pre_csc_lut()`. The code is a faithful copy of the original with the fix from patch 5 applied.

---
Generated by Claude Code Patch Reviewer

[PATCH 07/10] drm/i915/color: Program Pre-CSC registers for SDR

[Chaitanya Kumar Borah]

From: Pranay Samala <pranay.samala@intel.com>

Implement plane pre-CSC LUT support for SDR planes.

Signed-off-by: Pranay Samala <pranay.samala@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 17ab4364faea..9f7c2a328868 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -3992,6 +3992,59 @@ xelpd_load_hdr_pre_csc_lut(struct intel_display *display,
 	intel_de_write_dsb(display, dsb, PLANE_PRE_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
 }
 
+static void
+xelpd_load_sdr_pre_csc_lut(struct intel_display *display,
+			   struct intel_dsb *dsb,
+			   enum pipe pipe,
+			   enum plane_id plane,
+			   const struct drm_color_lut32 *pre_csc_lut)
+{
+	u32 lut_size = 32;
+	u32 lut_val;
+	int i;
+
+	/*
+	 * First 3 planes are HDR, so reduce by 3 to get to the right
+	 * SDR plane offset
+	 */
+	plane = plane - 3;
+
+	intel_de_write_dsb(display, dsb,
+			   PLANE_PRE_CSC_GAMC_INDEX(pipe, plane, 0),
+			   PLANE_PAL_PREC_AUTO_INCREMENT);
+
+	if (pre_csc_lut) {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = drm_color_lut_extract(pre_csc_lut[i].green, 16);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA(pipe, plane, 0),
+					   lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA(pipe, plane, 0),
+					   (1 << 16));
+		} while (i++ < 34);
+	} else {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = (i * ((1 << 16) - 1)) / (lut_size - 1);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA(pipe, plane, 0), lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_PRE_CSC_GAMC_DATA(pipe, plane, 0),
+					   1 << 16);
+		} while (i++ < 34);
+	}
+
+	intel_de_write_dsb(display, dsb, PLANE_PRE_CSC_GAMC_INDEX(pipe, plane, 0), 0);
+}
+
 static void
 xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 				const struct intel_plane_state *plane_state)
@@ -4004,6 +4057,8 @@ xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 
 	if (icl_is_hdr_plane(display, plane))
 		xelpd_load_hdr_pre_csc_lut(display, dsb, pipe, plane, pre_csc_lut);
+	else
+		xelpd_load_sdr_pre_csc_lut(display, dsb, pipe, plane, pre_csc_lut);
 }
 
 static void
-- 
2.25.1

Claude review: drm/i915/color: Program Pre-CSC registers for SDR

[Claude Code Review Bot]

Patch Review

**BUG — Wrong LUT extract function for SDR pre-CSC.** The function receives `const struct drm_color_lut32 *pre_csc_lut` (32-bit per-channel values) but uses `drm_color_lut_extract()`:

```c
lut_val = drm_color_lut_extract(pre_csc_lut[i].green, 16);
```

`drm_color_lut_extract` expects a 16-bit input (from `struct drm_color_lut`), scaling from `[0, 2^16-1]` to `[0, 2^precision-1]`. But `drm_color_lut32.green` is a `u32` in the range `[0, 2^32-1]`. This will produce incorrect values for any input above 65535. Should use `drm_color_lut32_extract(pre_csc_lut[i].green, 16)` instead.

**Concern — Hardcoded plane offset `plane = plane - 3`:**

```c
/*
 * First 3 planes are HDR, so reduce by 3 to get to the right
 * SDR plane offset
 */
plane = plane - 3;
```

This hardcodes the assumption that exactly 3 HDR planes precede SDR planes. While this matches current hardware (`PLANE_1`, `PLANE_2`, `PLANE_3` are HDR per `icl_hdr_plane_mask()`), it's fragile. The `_MMIO_PLANE_GAMC` macro uses `_PIPE(plane, a, b)` which only supports two planes (plane 0 and plane 1). After subtracting 3, `PLANE_4` becomes index 1, `PLANE_5` becomes index 2, etc. But `_PIPE` only handles index 0 and 1. If there are more than 2 SDR planes, this will access wrong registers. The code should add a comment explaining which SDR planes this supports and add a `WARN_ON` for unexpected plane IDs.

---
Generated by Claude Code Patch Reviewer

[PATCH 08/10] drm/i915/color: Extract HDR post-CSC LUT programming to helper function

[Chaitanya Kumar Borah]

From: Pranay Samala <pranay.samala@intel.com>

Move HDR plane post-CSC LUT programming to improve code organization.

Also removes the segment 0 index register writes as it is not currently
programmed.

Signed-off-by: Pranay Samala <pranay.samala@intel.com>
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 92 +++++++++++-----------
 1 file changed, 48 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 9f7c2a328868..3578606e0ed4 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -4061,6 +4061,52 @@ xelpd_program_plane_pre_csc_lut(struct intel_dsb *dsb,
 		xelpd_load_sdr_pre_csc_lut(display, dsb, pipe, plane, pre_csc_lut);
 }
 
+static void
+xelpd_load_hdr_post_csc_lut(struct intel_display *display,
+			    struct intel_dsb *dsb,
+			    enum pipe pipe,
+			    enum plane_id plane,
+			    const struct drm_color_lut32 *post_csc_lut)
+{
+	u32 lut_size = 32;
+	u32 lut_val;
+	int i;
+
+	intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX_ENH(pipe, plane, 0),
+			   PLANE_PAL_PREC_AUTO_INCREMENT);
+
+	if (post_csc_lut) {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = drm_color_lut32_extract(post_csc_lut[i].green, 24);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   (1 << 24));
+		} while (i++ < 34);
+	} else {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = (i * ((1 << 24) - 1)) / (lut_size - 1);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0), lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
+					   1 << 24);
+		} while (i++ < 34);
+	}
+
+	intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
+}
+
 static void
 xelpd_program_plane_post_csc_lut(struct intel_dsb *dsb,
 				 const struct intel_plane_state *plane_state)
@@ -4070,51 +4116,9 @@ xelpd_program_plane_post_csc_lut(struct intel_dsb *dsb,
 	enum pipe pipe = to_intel_plane(state->plane)->pipe;
 	enum plane_id plane = to_intel_plane(state->plane)->id;
 	const struct drm_color_lut32 *post_csc_lut = plane_state->hw.gamma_lut->data;
-	u32 i, lut_size, lut_val;
-
-	if (icl_is_hdr_plane(display, plane)) {
-		intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX_ENH(pipe, plane, 0),
-				   PLANE_PAL_PREC_AUTO_INCREMENT);
-		/* TODO: Add macro */
-		intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_SEG0_INDEX_ENH(pipe, plane, 0),
-				   PLANE_PAL_PREC_AUTO_INCREMENT);
-		if (post_csc_lut) {
-			lut_size = 32;
-			for (i = 0; i < lut_size; i++) {
-				lut_val = drm_color_lut32_extract(post_csc_lut[i].green, 24);
-
-				intel_de_write_dsb(display, dsb,
-						   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   lut_val);
-			}
-
-			/* Segment 2 */
-			do {
-				intel_de_write_dsb(display, dsb,
-						   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   (1 << 24));
-			} while (i++ < 34);
-		} else {
-			/*TODO: Add for segment 0 */
-			lut_size = 32;
-			for (i = 0; i < lut_size; i++) {
-				u32 v = (i * ((1 << 24) - 1)) / (lut_size - 1);
-
-				intel_de_write_dsb(display, dsb,
-						   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0), v);
-			}
-
-			do {
-				intel_de_write_dsb(display, dsb,
-						   PLANE_POST_CSC_GAMC_DATA_ENH(pipe, plane, 0),
-						   1 << 24);
-			} while (i++ < 34);
-		}
 
-		intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
-		intel_de_write_dsb(display, dsb,
-				   PLANE_POST_CSC_GAMC_SEG0_INDEX_ENH(pipe, plane, 0), 0);
-	}
+	if (icl_is_hdr_plane(display, plane))
+		xelpd_load_hdr_post_csc_lut(display, dsb, pipe, plane, post_csc_lut);
 }
 
 static void
-- 
2.25.1

Claude review: drm/i915/color: Extract HDR post-CSC LUT programming to helper function

[Claude Code Review Bot]

Patch Review

The refactoring extracts HDR post-CSC into `xelpd_load_hdr_post_csc_lut()`. The commit message notes that it "removes the segment 0 index register writes as it is not currently programmed."

The removed code was:
```c
intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_SEG0_INDEX_ENH(pipe, plane, 0),
                   PLANE_PAL_PREC_AUTO_INCREMENT);
...
intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_SEG0_INDEX_ENH(pipe, plane, 0), 0);
```

This is a functional change (removing SEG0 register writes) bundled into a "refactoring" patch. The old code's TODO said `/*TODO: Add for segment 0 */`, indicating these writes were placeholders. Still, mixing functional changes with pure refactoring makes bisection harder if the SEG0 removal causes issues. Consider splitting this into a separate commit.

---
Generated by Claude Code Patch Reviewer

[PATCH 09/10] drm/i915/color: Program Plane Post CSC registers for SDR planes

[Chaitanya Kumar Borah]

From: Pranay Samala <pranay.samala@intel.com>

Implement plane post-CSC LUT support for SDR planes.

Signed-off-by: Pranay Samala <pranay.samala@intel.com>
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 3578606e0ed4..9ff10be40f10 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -4107,6 +4107,57 @@ xelpd_load_hdr_post_csc_lut(struct intel_display *display,
 	intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX_ENH(pipe, plane, 0), 0);
 }
 
+static void
+xelpd_load_sdr_post_csc_lut(struct intel_display *display,
+			    struct intel_dsb *dsb,
+			    enum pipe pipe,
+			    enum plane_id plane,
+			    const struct drm_color_lut32 *post_csc_lut)
+{
+	u32 lut_size = 32;
+	u32 lut_val;
+	int i;
+
+	/*
+	 * First 3 planes are HDR, so reduce by 3 to get to the right
+	 * SDR plane offset
+	 */
+	plane = plane - 3;
+	intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX(pipe, plane, 0),
+			   PLANE_PAL_PREC_AUTO_INCREMENT);
+
+	if (post_csc_lut) {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = drm_color_lut32_extract(post_csc_lut[i].green, 16);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA(pipe, plane, 0),
+					   lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA(pipe, plane, 0),
+					   (1 << 16));
+		} while (i++ < 34);
+	} else {
+		for (i = 0; i < lut_size; i++) {
+			lut_val = (i * ((1 << 16) - 1)) / (lut_size - 1);
+
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA(pipe, plane, 0), lut_val);
+		}
+
+		do {
+			intel_de_write_dsb(display, dsb,
+					   PLANE_POST_CSC_GAMC_DATA(pipe, plane, 0),
+					   1 << 16);
+		} while (i++ < 34);
+	}
+
+	intel_de_write_dsb(display, dsb, PLANE_POST_CSC_GAMC_INDEX(pipe, plane, 0), 0);
+}
+
 static void
 xelpd_program_plane_post_csc_lut(struct intel_dsb *dsb,
 				 const struct intel_plane_state *plane_state)
@@ -4119,6 +4170,8 @@ xelpd_program_plane_post_csc_lut(struct intel_dsb *dsb,
 
 	if (icl_is_hdr_plane(display, plane))
 		xelpd_load_hdr_post_csc_lut(display, dsb, pipe, plane, post_csc_lut);
+	else
+		xelpd_load_sdr_post_csc_lut(display, dsb, pipe, plane, post_csc_lut);
 }
 
 static void
-- 
2.25.1

Claude review: drm/i915/color: Program Plane Post CSC registers for SDR planes

[Claude Code Review Bot]

Patch Review

**Same bugs as patch 7:**

1. **Wrong extract function**: Uses `drm_color_lut32_extract(post_csc_lut[i].green, 16)` — wait, this one actually uses `drm_color_lut32_extract`, which is correct. But wait, let me re-check:

```c
lut_val = drm_color_lut32_extract(post_csc_lut[i].green, 16);
```

Actually, looking again at the diff: patch 9 uses `drm_color_lut32_extract` — this is **correct** for the post-CSC SDR function. But patch 7 (pre-CSC SDR) uses `drm_color_lut_extract` — that's the inconsistency and bug.

2. **Same hardcoded `plane = plane - 3`** concern as patch 7.

---
Generated by Claude Code Patch Reviewer

[PATCH 10/10] drm/i915/color: Add color pipeline support for SDR planes

[Chaitanya Kumar Borah]

Now that everything is in place expose the SDR plane color pipeline
to user-space.

Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/i915/display/intel_color_pipeline.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color_pipeline.c b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
index 47b3bcec7b18..ec1c60bd8bc2 100644
--- a/drivers/gpu/drm/i915/display/intel_color_pipeline.c
+++ b/drivers/gpu/drm/i915/display/intel_color_pipeline.c
@@ -182,17 +182,11 @@ int _intel_color_pipeline_plane_init(struct drm_plane *plane, struct drm_prop_en
 
 int intel_color_pipeline_plane_init(struct drm_plane *plane, enum pipe pipe)
 {
-	struct drm_device *dev = plane->dev;
-	struct intel_display *display = to_intel_display(dev);
 	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES] = {};
 	int len = 0;
 	int ret = 0;
 	int i;
 
-	/* Currently expose pipeline only for HDR planes */
-	if (!icl_is_hdr_plane(display, to_intel_plane(plane)->id))
-		return 0;
-
 	/* Add pipeline consisting of transfer functions */
 	ret = _intel_color_pipeline_plane_init(plane, &pipelines[len], pipe);
 	if (ret)
-- 
2.25.1

Claude review: drm/i915/color: Add color pipeline support for SDR planes

[Claude Code Review Bot]

Patch Review

Clean enablement patch — removes the HDR-only guard in `intel_color_pipeline_plane_init`. This is the gate that makes SDR pipeline visible to userspace. Logically correct as the final patch.

---

## Summary of Issues by Severity

1. **Critical**: Patch 3 — CSC mode register corruption when YUV format + CSC_FF are both active on SDR planes
2. **Bug**: Patch 7 — `drm_color_lut_extract` used instead of `drm_color_lut32_extract` for 32-bit LUT data
3. **Fragile**: Patches 7/9 — Hardcoded `plane - 3` offset for SDR plane register mapping with no bounds checking
4. **Series ordering**: Patch 5 (bugfix with Cc:stable) should be first in series for clean backporting
5. **Minor**: Patch 1 — copy-paste doc comment error ("interpolation type" → "CSC FF type")
6. **Minor**: Patch 8 — functional change (SEG0 removal) mixed into refactoring patch

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915/color: Enable SDR plane color pipeline

[Claude Code Review Bot]

Overall Series Review

Subject: drm/i915/color: Enable SDR plane color pipeline
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Patches: 11
Reviewed: 2026-03-09T08:26:52.283357

---

This 10-patch series adds SDR plane color pipeline support to i915, introducing a new `DRM_COLOROP_CSC_FF` type in the DRM colorop framework and wiring it up along with pre/post-CSC 1D LUTs for SDR planes. The series also includes a bugfix for HDR pre-CSC LUT programming and refactoring to extract common HDR LUT programming into helpers.

The architecture is sound — exposing fixed-function CSC as a dedicated colorop type rather than overloading the existing CTM is the right approach. However, there are several bugs ranging from a **critical register corruption issue** in patch 3 to a **wrong extraction function** in patches 7/9, and some fragile hardcoded assumptions.

**Series ordering concern**: The bugfix (patch 5) with `Cc: stable` and `Fixes:` tag should be at the front of the series, not buried after feature patches. It has no dependency on patches 1-4 and should be independently backportable.

---

---
Generated by Claude Code Patch Reviewer