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

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Pekka
> Paalanen
> Sent: Tuesday, February 13, 2024 3:07 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> ville.syrjala@linux.intel.com; contact@emersion.fr; harry.wentland@amd.com;
> mwen@igalia.com; jadahl@redhat.com; sebastian.wick@redhat.com;
> shashank.sharma@amd.com; agoins@nvidia.com; joshua@froggi.es;
> mdaenzer@redhat.com; aleixpol@kde.org; xaver.hugl@gmail.com;
> victoria@system76.com; daniel@ffwll.ch; quic_naseer@quicinc.com;
> quic_cbraga@quicinc.com; quic_abhinavk@quicinc.com; arthurgrillo@riseup.net;
> marcan@marcan.st; Liviu.Dudau@arm.com; sashamcintosh@google.com;
> sean@poorly.run
> 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 <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?

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).

@ville.syrjala@linux.intel.com 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;