Re: [v5 11/24] drm/i915/color: Add and attach COLORPIPELINE plane property

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

On 10/28/2025 10:43 AM, Kandpal, Suraj wrote:
>> Subject: [v5 11/24] drm/i915/color: Add and attach COLORPIPELINE plane
>> property
>>
>> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>
>> Add supported color pipelines and attach it to plane.
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_color.c | 42 ++++++++++++++++++++++
>> drivers/gpu/drm/i915/display/intel_color.h |  3 ++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> index 90ac6530d1a5..363c9590c5c1 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -4050,6 +4050,48 @@ int intel_plane_tf_pipeline_init(struct drm_plane
>> *plane, struct drm_prop_enum_l
>>   	return 0;
>>   }
>>
>> +int intel_plane_color_init(struct drm_plane *plane) {
> 
> Again need to rethink the name here
> 

Ack, I have now created a separate file for pipeline related stuff.

>> +	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*/
> 
> Missed a blank space at the end
> 
>> +	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++;
>> +
>> +	/* Add pipeline consisting of transfer functions */
>> +	ret = intel_plane_tf_pipeline_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);
>> +
>> +	/* TODO check if needed */
>> +	if (plane->state)
>> +		plane->state->color_pipeline = NULL;
>> +
>> +	return 0;
>> +}
>> +
>>   void intel_color_crtc_init(struct intel_crtc *crtc)  {
>>   	struct intel_display *display = to_intel_display(crtc); diff --git
>> a/drivers/gpu/drm/i915/display/intel_color.h
>> b/drivers/gpu/drm/i915/display/intel_color.h
>> index ce9db761c6e2..c2561b86bb26 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>> @@ -18,6 +18,8 @@ struct drm_plane;
>>   struct drm_prop_enum_list;
>>   enum intel_color_block;
>>
>> +#define MAX_COLOR_PIPELINES 5
> 
> Here I see you will be initializing a max of 3 pipelines if I am not wrong the number should reflect that so maybe
> 3

We technically have two color pipelines one with 3 colorops and another 
"Bypass". 5 is more a forward looking limit.[1]

==
Chaitanya

[1] Also a bit of monkey see, monkey do
https://lore.kernel.org/dri-devel/20251030034349.2309829-25-alex.hung@amd.com/

> 
> Regards,
> Suraj Kandpal
> 
>> +
>>   void intel_color_init_hooks(struct intel_display *display);  int
>> intel_color_init(struct intel_display *display);  void intel_color_crtc_init(struct
>> intel_crtc *crtc); @@ -46,5 +48,6 @@ void intel_color_assert_luts(const struct
>> intel_crtc_state *crtc_state);  struct intel_plane_colorop
>> *intel_colorop_alloc(void);  struct intel_plane_colorop
>> *intel_plane_colorop_create(enum intel_color_block id);  int
>> intel_plane_tf_pipeline_init(struct drm_plane *plane, struct
>> drm_prop_enum_list *list);
>> +int intel_plane_color_init(struct drm_plane *plane);
>>
>>   #endif /* __INTEL_COLOR_H__ */
>> --
>> 2.42.0
>