Re: [v5 12/24] drm/i915/color: Add framework to program CSC

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

On 10/28/2025 1:39 PM, Kandpal, Suraj wrote:
>> Subject: [v5 12/24] drm/i915/color: Add framework to program CSC
>>
>> From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>
>> Add framework to program CSC. It enables copying of matrix from uapi to intel
>> plane state. Also adding helper functions which will eventually program values
>> to hardware.
>>
>> 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    | 14 +++++++
>>   drivers/gpu/drm/i915/display/intel_color.h    |  4 +-
>>   .../drm/i915/display/intel_display_types.h    |  1 +
>>   drivers/gpu/drm/i915/display/intel_plane.c    | 39 +++++++++++++++++++
>>   4 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
>> b/drivers/gpu/drm/i915/display/intel_color.c
>> index 363c9590c5c1..7c53572f729b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.c
>> +++ b/drivers/gpu/drm/i915/display/intel_color.c
>> @@ -3967,6 +3967,20 @@ static const struct intel_color_funcs
>> ilk_color_funcs = {  };
>>
>>   /* TODO: Move to another file */
>> +static void
>> +intel_color_load_plane_csc_matrix(struct intel_dsb *dsb,
>> +				  const struct intel_plane_state *plane_state)
>> {
>> +	/* CTM programming */
> 
> Add TODO
> 

Ack.

>> +}
>> +
>> +void intel_color_plane_program_pipeline(struct intel_dsb *dsb,
>> +					const struct intel_plane_state
>> *plane_state) {
>> +	if (plane_state->hw.ctm)
>> +		intel_color_load_plane_csc_matrix(dsb, plane_state); }
>> +
>>   struct intel_plane_colorop *intel_colorop_alloc(void)  {
>>   	struct intel_plane_colorop *colorop;
>> diff --git a/drivers/gpu/drm/i915/display/intel_color.h
>> b/drivers/gpu/drm/i915/display/intel_color.h
>> index c2561b86bb26..420d596dbbae 100644
>> --- a/drivers/gpu/drm/i915/display/intel_color.h
>> +++ b/drivers/gpu/drm/i915/display/intel_color.h
>> @@ -13,6 +13,7 @@ struct intel_crtc_state;  struct intel_crtc;  struct
>> intel_display;  struct intel_dsb;
>> +struct intel_plane_state;
>>   struct drm_property_blob;
>>   struct drm_plane;
>>   struct drm_prop_enum_list;
>> @@ -49,5 +50,6 @@ 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);
>> -
>> +void intel_color_plane_program_pipeline(struct intel_dsb *dsb,
>> +					const struct intel_plane_state
>> *plane_state);
>>   #endif /* __INTEL_COLOR_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
>> b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 4b5124a08cc9..c709df0cea9e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -634,6 +634,7 @@ struct intel_plane_state {
>>   		enum drm_color_encoding color_encoding;
>>   		enum drm_color_range color_range;
>>   		enum drm_scaling_filter scaling_filter;
>> +		struct drm_property_blob *ctm;
>>   	} 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 36fb07471deb..cc8f3e15c82e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_plane.c
>> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
>> @@ -380,6 +380,43 @@ intel_plane_copy_uapi_plane_damage(struct
>> intel_plane_state *new_plane_state,
>>   		*damage = drm_plane_state_src(&new_uapi_plane_state-
>>> uapi);
>>   }
>>
>> +static void
>> +intel_plane_colorop_replace_blob(struct intel_plane_state *plane_state,
>> +				 struct intel_plane_colorop *intel_colorop,
>> +				 struct drm_property_blob *blob)
>> +{
>> +	if (intel_colorop->id == CB_PLANE_CSC)
>> +		drm_property_replace_blob(&plane_state->hw.ctm, blob); }
>> +
>> +static void
>> +intel_plane_copy_uapi_to_hw_state_color(struct intel_plane_state
> 
> This should be intel_plane_color_copy_uapi_to_hw_state
> 

Ack.

>> *plane_state,
>> +					const struct intel_plane_state
>> *from_plane_state,
>> +					struct intel_crtc *crtc)
>> +{
>> +	struct drm_colorop *iter_colorop, *colorop;
>> +	struct drm_colorop_state *new_colorop_state;
>> +	struct drm_atomic_state *state = plane_state->uapi.state;
>> +	struct intel_plane_colorop *intel_colorop;
>> +	struct drm_property_blob *blob;
>> +	int i = 0;
>> +
>> +	iter_colorop = plane_state->uapi.color_pipeline;
>> +
>> +	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_plane_colorop(colorop);
>> +
>> 	intel_plane_colorop_replace_blob(plane_state,
>> +
>> intel_colorop,
>> +								 blob);
> 
> A break here why keep iterating if you have found what you are looking for
> I think I am seeing more and more reason to have a separate file as Jani had said
> Called intel_plane_color.c
> 

We have to iterate over all the colorops in the pipeline/state.

==
Chaitanya

> Regards,
> Suraj Kandpal
>> +			}
>> +		}
>> +		iter_colorop = iter_colorop->next;
>> +	}
>> +}
>> +
>>   void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state
>> *plane_state,
>>   				       const struct intel_plane_state
>> *from_plane_state,
>>   				       struct intel_crtc *crtc)
>> @@ -408,6 +445,8 @@ void intel_plane_copy_uapi_to_hw_state(struct
>> intel_plane_state *plane_state,
>>
>>   	plane_state->uapi.src = drm_plane_state_src(&from_plane_state-
>>> uapi);
>>   	plane_state->uapi.dst = drm_plane_state_dest(&from_plane_state-
>>> uapi);
>> +
>> +	intel_plane_copy_uapi_to_hw_state_color(plane_state,
>> from_plane_state,
>> +crtc);
>>   }
>>
>>   void intel_plane_copy_hw_state(struct intel_plane_state *plane_state,
>> --
>> 2.42.0
>