[17/28] drm/i915: Define segmented Lut and add capabilities to colorop

Commit Message

Shankar, Uma Feb. 13, 2024, 6:48 a.m. UTC

This defines the lut segments and create the color pipeline

Signed-off-by: Uma Shankar <[e-mail]>
Signed-off-by: Chaitanya Kumar Borah <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_color.c | 109 +++++++++++++++++++++
 1 file changed, 109 insertions(+)

Comments

Pekka Paalanen Feb. 13, 2024, 9:37 a.m. UTC | #1

On Tue, 13 Feb 2024 12:18:24 +0530
Uma Shankar <[e-mail]> wrote:

> This defines the lut segments and create the color pipeline
> 
> Signed-off-by: Uma Shankar <[e-mail]>
> Signed-off-by: Chaitanya Kumar Borah <[e-mail]>
> ---
>  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;

Shankar, Uma Feb. 14, 2024, 7:28 a.m. UTC | #2

> -----Original Message-----
> From: dri-devel <[e-mail]> On Behalf Of Pekka
> Paalanen
> Sent: Tuesday, February 13, 2024 3:07 PM
> To: Shankar, Uma <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add capabilities
> to colorop
> 
> On Tue, 13 Feb 2024 12:18:24 +0530
> Uma Shankar <[e-mail]> wrote:
> 
> > This defines the lut segments and create the color pipeline
> >
> > Signed-off-by: Uma Shankar <[e-mail]>
> > Signed-off-by: Chaitanya Kumar Borah <[e-mail]>
> > ---
> >  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?

Hi Pekka,
Idea to have these flags is to just have some option in case there are some differences
across segments. Most cases this should not be the case, just helps to future proof
the implementation.

Based on how the community feels on the usability of it, we can take a call on the flags
and the expected interpretation for the same. We are open for suggestions on the same.

> 
> > +		.count = 128,
> > +		.input_bpc = 24, .output_bpc = 16,
> 
> The same question about input_bpc and output_bpc.

Same for these as well, userspace can just ignore these if no usage. However, for some clients
it may help in Lut computations.
The original idea for the structure came from Ville (missed to mention that in cover letter, will get that
updated in next version).

@[e-mail] Please share your inputs on the usability of these attributes.


> > +		.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?

This is just to help give some reference to userspace.  As of now, driver trusts the values
coming from userspace if it sends wrong values its on him and driver can't help much.
However, we surely can have some sanity check like non decreasing luts etc. to driver.

Ideally LUT values should not overlap, but we can indicate this explicitly with flag to
hint the userspace (for overlap or otherwise) and also get a check in driver for the same.

Regards,
Uma Shankar

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

Pekka Paalanen Feb. 14, 2024, 9:03 a.m. UTC | #3

On Wed, 14 Feb 2024 07:28:37 +0000
"Shankar, Uma" <[e-mail]> wrote:

> > -----Original Message-----
> > From: dri-devel <[e-mail]> On Behalf Of Pekka
> > Paalanen
> > Sent: Tuesday, February 13, 2024 3:07 PM
> > To: Shankar, Uma <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add capabilities
> > to colorop
> > 
> > On Tue, 13 Feb 2024 12:18:24 +0530
> > Uma Shankar <[e-mail]> wrote:
> >   
> > > This defines the lut segments and create the color pipeline
> > >
> > > Signed-off-by: Uma Shankar <[e-mail]>
> > > Signed-off-by: Chaitanya Kumar Borah <[e-mail]>
> > > ---
> > >  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?  
> 
> Hi Pekka,
> Idea to have these flags is to just have some option in case there are some differences
> across segments. Most cases this should not be the case, just helps to future proof
> the implementation.
> 
> Based on how the community feels on the usability of it, we can take a call on the flags
> and the expected interpretation for the same. We are open for suggestions on the same.
> 
> >   
> > > +		.count = 128,
> > > +		.input_bpc = 24, .output_bpc = 16,  
> > 
> > The same question about input_bpc and output_bpc.  
> 
> Same for these as well, userspace can just ignore these if no usage. However, for some clients
> it may help in Lut computations.
> The original idea for the structure came from Ville (missed to mention that in cover letter, will get that
> updated in next version).
> 
> @[e-mail] Please share your inputs on the usability of these attributes.

Userspace will always need to evaluate whether each segment is good
enough individually, so maybe it's not that big deal.

Ignoring these is not an option for userspace, because that would mean
userspace does not know what it is getting. If UAPI contains a
parameter, then the onus is on userspace to ensure the value is
acceptable.

> > > +		.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?  
> 
> This is just to help give some reference to userspace.  As of now, driver trusts the values
> coming from userspace if it sends wrong values its on him and driver can't help much.
> However, we surely can have some sanity check like non decreasing luts etc. to driver.

But what will guarantee that the driver provided values are consistent?
That they actually describe a template of a well-formed sampled
curve? If they are not consistent, userspace cannot use the colorop.
Whose responsibility is it to ensure the consistency?

We have a few examples of drivers getting descriptive values like
these simply wrong until DRM common code started sanity-checking them,
the bitmasks of possible_clones and possible_crtcs for example.

There should also be DRM common code to verify that userspace provided
data matches the segmented LUT description rather than drivers just
trusting it. If it doesn't match, the atomic commit must fail rather
than silently malfunction. The same with programming hardware: if
hardware does not produce the intended result from a given segmented
LUT configuration, the atomic commit must fail instead of malfunction.

> 
> Ideally LUT values should not overlap, but we can indicate this explicitly with flag to
> hint the userspace (for overlap or otherwise) and also get a check in driver for the same.

Sorry? How could overlapping segments ever work? Or segments with a gap
between them?

If segments overlap, what's the rule for choosing which segment to use
for an input value hitting both? The segments can disagree on the
result.

If there are gaps, what is the rule how to handle an input value
hitting a gap?


Thanks,
pq

> 
> Regards,
> Uma Shankar
> 
> > 
> > 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;  
>

Shankar, Uma Feb. 19, 2024, 10:34 a.m. UTC | #4

> -----Original Message-----
> From: Pekka Paalanen <[e-mail]>
> Sent: Wednesday, February 14, 2024 2:34 PM
> To: Shankar, Uma <[e-mail]>
> Cc: [e-mail]; [e-mail]; dri-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add capabilities
> to colorop
> 
> On Wed, 14 Feb 2024 07:28:37 +0000
> "Shankar, Uma" <[e-mail]> wrote:
> 
> > > -----Original Message-----
> > > From: dri-devel <[e-mail]> On Behalf
> > > Of Pekka Paalanen
> > > Sent: Tuesday, February 13, 2024 3:07 PM
> > > To: Shankar, Uma <[e-mail]>
> > > Cc: [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add
> > > capabilities to colorop
> > >
> > > On Tue, 13 Feb 2024 12:18:24 +0530
> > > Uma Shankar <[e-mail]> wrote:
> > >
> > > > This defines the lut segments and create the color pipeline
> > > >
> > > > Signed-off-by: Uma Shankar <[e-mail]>
> > > > Signed-off-by: Chaitanya Kumar Borah
> > > > <[e-mail]>
> > > > ---
> > > >  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?
> >
> > Hi Pekka,
> > Idea to have these flags is to just have some option in case there are
> > some differences across segments. Most cases this should not be the
> > case, just helps to future proof the implementation.
> >
> > Based on how the community feels on the usability of it, we can take a
> > call on the flags and the expected interpretation for the same. We are open for
> suggestions on the same.
> >
> > >
> > > > +		.count = 128,
> > > > +		.input_bpc = 24, .output_bpc = 16,
> > >
> > > The same question about input_bpc and output_bpc.
> >
> > Same for these as well, userspace can just ignore these if no usage.
> > However, for some clients it may help in Lut computations.
> > The original idea for the structure came from Ville (missed to mention
> > that in cover letter, will get that updated in next version).
> >
> > @[e-mail] Please share your inputs on the usability of these
> attributes.
> 
> Userspace will always need to evaluate whether each segment is good enough
> individually, so maybe it's not that big deal.
> 
> Ignoring these is not an option for userspace, because that would mean userspace
> does not know what it is getting. If UAPI contains a parameter, then the onus is on
> userspace to ensure the value is acceptable.

Got your point, the parameters, and expectations with it should be clearly defined.
Here it just means what is the bpc which is fed to the color block and at what bpc
results come out after rounding and truncation. This information may help in
computing the LUT co-efficients and get better accuracy.

Having said that, we are not using it as of now in the IGT tests. We can discuss the
usability and usefulness of this attribute for userspace, based on recommendation
we can adopt or drop this.


> > > > +		.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?
> >
> > This is just to help give some reference to userspace.  As of now,
> > driver trusts the values coming from userspace if it sends wrong values its on
> him and driver can't help much.
> > However, we surely can have some sanity check like non decreasing luts etc. to
> driver.
> 
> But what will guarantee that the driver provided values are consistent?
> That they actually describe a template of a well-formed sampled curve? If they
> are not consistent, userspace cannot use the colorop.
> Whose responsibility is it to ensure the consistency?

Since the driver will be privileged, I guess userspace should believe the values are
sane as reported by the properties. This is as for any other hardware capabilities.
Also, its immutable so userspace will not be able to tweak it making it safe.

> We have a few examples of drivers getting descriptive values like these simply
> wrong until DRM common code started sanity-checking them, the bitmasks of
> possible_clones and possible_crtcs for example.

We can implement some helpers to catch basic abnormalities with LUT reporting
while property creation itself.  Like decreasing luts, not matching the reported flags etc.

> There should also be DRM common code to verify that userspace provided data
> matches the segmented LUT description rather than drivers just trusting it. If it
> doesn't match, the atomic commit must fail rather than silently malfunction. The
> same with programming hardware: if hardware does not produce the intended
> result from a given segmented LUT configuration, the atomic commit must fail
> instead of malfunction.

Yes, we can have some checks in driver for sanity of userspace provided values.
Things like LUTs not following the flags and capabilities reported, going beyond
the range etc. However the actual values and computation of the same has to be
userspace responsibility, if the co-efficients go wrong then responsibility of the artifact
should be on the client/compositor who is controlling it (permission can be controlled
so that only allowed userspace can be able to change color setttings)

> >
> > Ideally LUT values should not overlap, but we can indicate this
> > explicitly with flag to hint the userspace (for overlap or otherwise) and also get
> a check in driver for the same.
> 
> Sorry? How could overlapping segments ever work? Or segments with a gap
> between them?

I have not seen overlapping luts in segments, we can take a call if all vendors align.

> If segments overlap, what's the rule for choosing which segment to use for an
> input value hitting both? The segments can disagree on the result.
> 
> If there are gaps, what is the rule how to handle an input value hitting a gap?

This can be brainstormed, if any usescase like this exists.

Regards,
Uma Shankar

> 
> Thanks,
> pq
> 
> >
> > Regards,
> > Uma Shankar
> >
> > >
> > > 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;
> >

Pekka Paalanen Feb. 19, 2024, 12:02 p.m. UTC | #5

On Mon, 19 Feb 2024 10:34:19 +0000
"Shankar, Uma" <[e-mail]> wrote:

> > -----Original Message-----
> > From: Pekka Paalanen <[e-mail]>
> > Sent: Wednesday, February 14, 2024 2:34 PM
> > To: Shankar, Uma <[e-mail]>
> > Cc: [e-mail]; [e-mail]; dri-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add capabilities
> > to colorop
> > 
> > On Wed, 14 Feb 2024 07:28:37 +0000
> > "Shankar, Uma" <[e-mail]> wrote:
> >   
> > > > -----Original Message-----
> > > > From: dri-devel <[e-mail]> On Behalf
> > > > Of Pekka Paalanen
> > > > Sent: Tuesday, February 13, 2024 3:07 PM
> > > > To: Shankar, Uma <[e-mail]>
> > > > Cc: [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Subject: Re: [PATCH 17/28] drm/i915: Define segmented Lut and add
> > > > capabilities to colorop
> > > >
> > > > On Tue, 13 Feb 2024 12:18:24 +0530
> > > > Uma Shankar <[e-mail]> wrote:
> > > >  
> > > > > This defines the lut segments and create the color pipeline
> > > > >
> > > > > Signed-off-by: Uma Shankar <[e-mail]>
> > > > > Signed-off-by: Chaitanya Kumar Borah
> > > > > <[e-mail]>
> > > > > ---
> > > > >  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?  
> > >
> > > Hi Pekka,
> > > Idea to have these flags is to just have some option in case there are
> > > some differences across segments. Most cases this should not be the
> > > case, just helps to future proof the implementation.
> > >
> > > Based on how the community feels on the usability of it, we can take a
> > > call on the flags and the expected interpretation for the same. We are open for  
> > suggestions on the same.  
> > >  
> > > >  
> > > > > +		.count = 128,
> > > > > +		.input_bpc = 24, .output_bpc = 16,  
> > > >
> > > > The same question about input_bpc and output_bpc.  
> > >
> > > Same for these as well, userspace can just ignore these if no usage.
> > > However, for some clients it may help in Lut computations.
> > > The original idea for the structure came from Ville (missed to mention
> > > that in cover letter, will get that updated in next version).
> > >
> > > @[e-mail] Please share your inputs on the usability of these  
> > attributes.
> > 
> > Userspace will always need to evaluate whether each segment is good enough
> > individually, so maybe it's not that big deal.
> > 
> > Ignoring these is not an option for userspace, because that would mean userspace
> > does not know what it is getting. If UAPI contains a parameter, then the onus is on
> > userspace to ensure the value is acceptable.  
> 
> Got your point, the parameters, and expectations with it should be clearly defined.
> Here it just means what is the bpc which is fed to the color block and at what bpc
> results come out after rounding and truncation. This information may help in
> computing the LUT co-efficients and get better accuracy.
> 
> Having said that, we are not using it as of now in the IGT tests. We can discuss the
> usability and usefulness of this attribute for userspace, based on recommendation
> we can adopt or drop this.

Hi Uma,

this discussion applies much more to the flags than bpc.

Bpc is interesting to userspace, so I do think it should be kept, and
extended to all colorops as possible.

> > > > > +		.input_bpc = 24, .output_bpc = 16,  
> > > > > +		.start = 0, .end = (1 << 24) - 1,
> > > > > +		.min = 0, .max = (1 << 24) - 1,

Btw. does this say, that normalized input value range is
[0.0, 1.0], and output values must be in [0.0, 256.0]?

That the divisor is (1 << bpc) - 1, and not (1 << bpc)?

> > > > > +	},
> > > > > +	/* 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?  
> > >
> > > This is just to help give some reference to userspace.  As of now,
> > > driver trusts the values coming from userspace if it sends wrong values its on  
> > him and driver can't help much.  
> > > However, we surely can have some sanity check like non decreasing luts etc. to  
> > driver.
> > 
> > But what will guarantee that the driver provided values are consistent?
> > That they actually describe a template of a well-formed sampled curve? If they
> > are not consistent, userspace cannot use the colorop.
> > Whose responsibility is it to ensure the consistency?  
> 
> Since the driver will be privileged, I guess userspace should believe the values are
> sane as reported by the properties. This is as for any other hardware capabilities.
> Also, its immutable so userspace will not be able to tweak it making it safe.

It is still possible for the driver to give an utter nonsense blob.
Where should be the tests to prevent that, in IGT or DRM core?

> > We have a few examples of drivers getting descriptive values like these simply
> > wrong until DRM common code started sanity-checking them, the bitmasks of
> > possible_clones and possible_crtcs for example.  
> 
> We can implement some helpers to catch basic abnormalities with LUT reporting
> while property creation itself.  Like decreasing luts, not matching the reported flags etc.

I really meant *drivers* getting the *advertisement* values wrong.
Claiming that a connector/encoder is compatible with CRTC index 17 when
such CRTC does not exist is a real example. Or that all connectors can
be clones with all other connectors, even those that do not exist.

In the case of segmented LUTs, there could be these mistakes for a
two-segment { A, B } description:
- segment A has REFLECT_NEGATIVE, but segment B does not
- segment B normalised start is smaller than segment A normalised end
- there is a gap between segment A normalised end and segment B normalised start
- segment A normalised max is smaller than segment B normalised min
- segment sample count is zero (or negative)
- segment max is smaller than segment min

And so on. Something needs to check for these and catch these, before a
driver with them is released or even merged.

Think of it as reversed checks at UAPI boundary where a common
component is checking that drivers get it right.

> 
> > There should also be DRM common code to verify that userspace provided data
> > matches the segmented LUT description rather than drivers just trusting it. If it
> > doesn't match, the atomic commit must fail rather than silently malfunction. The
> > same with programming hardware: if hardware does not produce the intended
> > result from a given segmented LUT configuration, the atomic commit must fail
> > instead of malfunction.  
> 
> Yes, we can have some checks in driver for sanity of userspace provided values.
> Things like LUTs not following the flags and capabilities reported, going beyond
> the range etc. However the actual values and computation of the same has to be
> userspace responsibility, if the co-efficients go wrong then responsibility of the artifact
> should be on the client/compositor who is controlling it (permission can be controlled
> so that only allowed userspace can be able to change color setttings)

Naturally. A driver cannot know if the values are correct for what
userspace intends to do, but the DRM core can check that the values fit
the segmented LUT description that the driver advertised.

Permission? Permissions are already controlled by the DRM master
mechanic.

> > >
> > > Ideally LUT values should not overlap, but we can indicate this
> > > explicitly with flag to hint the userspace (for overlap or otherwise) and also get  
> > a check in driver for the same.
> > 
> > Sorry? How could overlapping segments ever work? Or segments with a gap
> > between them?  
> 
> I have not seen overlapping luts in segments, we can take a call if all vendors align.
> 
> > If segments overlap, what's the rule for choosing which segment to use for an
> > input value hitting both? The segments can disagree on the result.
> > 
> > If there are gaps, what is the rule how to handle an input value hitting a gap?  
> 
> This can be brainstormed, if any usescase like this exists.

These questions were supposed to make you think and realise
that overlapping/gapped segments make absolutely no sense at all by
pointing out the problems they would cause as an interface description.

Even if hardware had literally overlapping segments, then the driver
should advertise non-overlapping segments that match what the hardware
will achieve.

If hardware had literally gaps between segments, then... either the
hardware cannot be fully used, or the driver will manufacture a
description without gaps that matches hardware behaviour.


Thanks,
pq

Patch

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),
+		.count = 128,
+		.input_bpc = 24, .output_bpc = 16,
+		.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,
+		.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;