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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 02 Jul 2025, Uma Shankar <uma.shankar@intel.com> wrote:
> 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)
> +{
> +	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*/
> +	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

If it does not need to be in the header, it does not belong in the
header.

> +
>  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);

Maybe this all deserves a separate file, intel_plane_color.[ch] or something?

>  
>  #endif /* __INTEL_COLOR_H__ */

-- 
Jani Nikula, Intel