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:42 PM, Kandpal, Suraj wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
>> Kandpal, Suraj
>> Sent: Tuesday, October 28, 2025 1:40 PM
>> To: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
>> intel-xe@lists.freedesktop.org; dri-devel@lists.freedesktop.org
>> Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>;
>> ville.syrjala@linux.intel.com; pekka.paalanen@collabora.com;
>> contact@emersion.fr; harry.wentland@amd.com; mwen@igalia.com;
>> jadahl@redhat.com; sebastian.wick@redhat.com;
>> shashank.sharma@amd.com; Sharma, Swati2 <swati2.sharma@intel.com>;
>> alex.hung@amd.com; Shankar, Uma <uma.shankar@intel.com>
>> Subject: RE: [v5 12/24] drm/i915/color: Add framework to program CSC
>>
>>> 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
>>
>>> +}
>>> +
>>> +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); }
>>> +
> 
> Also the functions above introduced seem to be out of place introduce them later where they are used.
> 

The idea here is to get the plumbing in first and then enable the 
interface later on.

==
Chaitanya

> Regards,
> Suraj Kandpal
> 
>>>   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
>>
>>> *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
>>
>> 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
>