On Tue, 13 Feb 2024 12:18:24 +0530
Uma Shankar <uma.shankar@intel.com> wrote:

> This defines the lut segments and create the color pipeline
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 109 +++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index e223edbe4c13..223cd1ff7291 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -3811,6 +3811,105 @@ static const struct intel_color_funcs ilk_color_funcs = {
>  	.get_config = ilk_get_config,
>  };
>  
> +static const struct drm_color_lut_range xelpd_degamma_hdr[] = {
> +	/* segment 1 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_NON_DECREASING),

Hi Uma,

is it a good idea to have these flags per-segment?

I would find it very strange, unusable really, if REFLECT_NEGATIVE
applied on some but not all segments, for example. Is such flexibility
really necessary in the hardware description?

> +		.count = 128,
> +		.input_bpc = 24, .output_bpc = 16,

The same question about input_bpc and output_bpc.

> +		.start = 0, .end = (1 << 24) - 1,
> +		.min = 0, .max = (1 << 24) - 1,
> +	},
> +	/* segment 2 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = (1 << 24) - 1, .end = 1 << 24,

What if there is a gap or overlap between the previous segment .end and
the next segment .start? Is it forbidden? Will the kernel common code
verify that drivers don't make mistakes? Or IGT?


Thanks,
pq

> +		.min = 0, .max = (1 << 27) - 1,
> +	},
> +	/* Segment 3 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = 1 << 24, .end = 3 << 24,
> +		.min = 0, .max = (1 << 27) - 1,
> +	},
> +	/* Segment 4 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = 3 << 24, .end = 7 << 24,
> +		.min = 0, .max = (1 << 27) - 1,
> +	}
> +};
> +
> +/* FIXME input bpc? */
> +static const struct drm_color_lut_range xelpd_gamma_hdr[] = {
> +	/*
> +	 * ToDo: Add Segment 1
> +	 * There is an optional fine segment added with 9 lut values
> +	 * Will be added later
> +	 */
> +
> +	/* segment 2 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 32,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = 0, .end = (1 << 24) - 1,
> +		.min = 0, .max = (1 << 24) - 1,
> +	},
> +	/* segment 3 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = (1 << 24) - 1, .end = 1 << 24,
> +		.min = 0, .max = 1 << 24,
> +	},
> +	/* Segment 4 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = 1 << 24, .end = 3 << 24,
> +		.min = 0, .max = (3 << 24),
> +	},
> +	/* Segment 5 */
> +	{
> +		.flags = (DRM_MODE_LUT_REFLECT_NEGATIVE |
> +				DRM_MODE_LUT_INTERPOLATE |
> +				DRM_MODE_LUT_REUSE_LAST |
> +				DRM_MODE_LUT_NON_DECREASING),
> +		.count = 1,
> +		.input_bpc = 24, .output_bpc = 16,
> +		.start = 3 << 24, .end = 7 << 24,
> +		.min = 0, .max = (7 << 24),
> +	},
> +};
> +
>  /* TODO: Move to another file */
>  struct intel_plane_colorop *intel_colorop_alloc(void)
>  {
> @@ -3865,6 +3964,11 @@ int intel_plane_tf_pipeline_init(struct drm_plane *plane, struct drm_prop_enum_l
>  	if (ret)
>  		return ret;
>  
> +	if (icl_is_hdr_plane(i915, to_intel_plane(plane)->id)) {
> +		drm_colorop_lutcaps_init(&colorop->base, plane, xelpd_degamma_hdr,
> +					 sizeof(xelpd_degamma_hdr));
> +	}
> +
>  	list->type = colorop->base.base.id;
>  	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", colorop->base.base.id);
>  
> @@ -3886,6 +3990,11 @@ int intel_plane_tf_pipeline_init(struct drm_plane *plane, struct drm_prop_enum_l
>  	if (ret)
>  		return ret;
>  
> +	if (icl_is_hdr_plane(i915, to_intel_plane(plane)->id)) {
> +		drm_colorop_lutcaps_init(&colorop->base, plane, xelpd_gamma_hdr,
> +					 sizeof(xelpd_gamma_hdr));
> +	}
> +
>  	drm_colorop_set_next_property(prev_op, &colorop->base);
>  
>  	return 0;