> -----Original Message-----
> From: Intel-xe <[email protected]> On Behalf Of
> Kandpal, Suraj
> Sent: Tuesday, October 28, 2025 1:40 PM
> To: Shankar, Uma <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: Borah, Chaitanya Kumar <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Sharma, Swati2 <[email protected]>;
> [email protected]; Shankar, Uma <[email protected]>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Uma Shankar <[email protected]>
> > ---
> > 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.
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
