RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Shankar, Uma]

> -----Original Message-----
> From: Alex Hung <alex.hung@amd.com>
> Sent: Thursday, March 27, 2025 5:17 AM
> To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org
> Cc: wayland-devel@lists.freedesktop.org; harry.wentland@amd.com;
> alex.hung@amd.com; leo.liu@amd.com; ville.syrjala@linux.intel.com;
> pekka.paalanen@collabora.com; contact@emersion.fr; 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; Shankar, Uma <uma.shankar@intel.com>;
> quic_naseer@quicinc.com; quic_cbraga@quicinc.com;
> quic_abhinavk@quicinc.com; marcan@marcan.st; Liviu.Dudau@arm.com;
> sashamcintosh@google.com; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> Subject: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> 
> We've previously introduced DRM_COLOROP_1D_CURVE for pre-defined 1D
> curves. But we also have HW that supports custom curves and userspace needs
> the ability to pass custom curves, aka LUTs.
> 
> This patch introduces a new colorop type, called DRM_COLOROP_1D_LUT that
> provides a SIZE property which is used by a driver to advertise the supported SIZE
> of the LUT, as well as a DATA property which userspace uses to set the LUT.
> 
> DATA and size function in the same way as current drm_crtc GAMMA and
> DEGAMMA LUTs.

Thanks Alex and Harry for driving this forward.
We want to have just one change in the way we expose the hardware capabilities else
all looks good in general.

To expose the capabilities of 1D Lut (of any kind), we can use a generic
implementation which covers the distribution of LUT samples along with size and
precision in a common color op. Something like below:

struct drm_color_lut_range {
	       __u32 flags;
	       __u16 count;
	       __s32 start, end;
	       __u32 norm_factor;

	       struct {
	               __u16 intp;
	               __u16 fracp;
	       } precision;
	};

The idea is to expose the lut distribution covering the entire color range from 0 to 1.0.
The underlying implementation in hardware can have just 1 segment covering the full range
OR a complex LUT distribution in Hardware like PWL, multi segmented etc.

To explain the usage with some real life example
------------------------------------------------

1. Conventional 1D LUT with just one segment

		|---|---|------------------------------------|
		0   1   2                                   1024


	- Hardware Description: A color block with a LUT linearly interpolating and
				covering range from 0 to 1.0
					- Number of segments - 1
					- Number of samples in LUT 1024
					- Precision of LUT samples in HW 0.10
					- Normalization Factor - Max value to represent 1.0
						 in terms of smallest step size which is 1024.

	In this case, it will be represented by the following structure.

	struct drm_color_lut_range lut_1024[] = {
			.start = 0 .end = (1 << 10);
			.normalization_factor = 1024;
			.count = 1024;
			.precision {
				.int_comp = 0;
				.fractional_comp = 10;
			}
		}

2. Piece Wise Linear 1D LUT

	  |---|---|------------------------------------|
	  0   1   2                                                32
                |       \
                |          \
                |             \
                |                \
	  0                   \
                |---|---|--...---|
	 0   1   2            8

	- Hardware Description: A color block with a LUT linearly interpolating and
				covering range from 0 to 1.0
					- Number of segments 2
					- Number of samples
						- segment 1 - 9 (covers range from 0 to 1/32)
						- segment 2 - 30 (covers range from 2/32 to 1.0)
					- Precision of LUT samples in HW 0.24
					- Normalization Factor - Max value to represent 1.0
						 in terms of smallest step size which is 8*32.

		struct drm_color_lut_range lut_pwl[] = {
		        /* segment 1 */
		        {
				.count = 9,
				.start = 0, .end = 8,
				.norm_factor = 8*32,
				.precision = {
					.intp = 0,
					.fracp = 24,
				},
			},
		        /* segment 2 */
			{
				.count = 30,
				.start = 8*2, .end = 8*32,
				.norm_factor = 8*32,
				.precision = {
					.intp = 0,
					.fracp = 24,
				},
		        },
		}

We can add and modify the relevant fields to be exposed in the capability structure as per
the community feedback, but this should be able to cover all the 1d LUT aspects and generically
expose the entire hardware block to userspace.

With this info, userspace will be able to compute the LUT samples for any specific usecase and we
may not need any exclusive "SIZE" or separate 1D lut property and just have this common property
which exposes hardware lut capabilities through "struct drm_color_lut_range"

Please refer to below links explaining the implementation:
[1] https://patchwork.freedesktop.org/patch/642652/?series=129812&rev=4
[2] https://patchwork.freedesktop.org/patch/642591/?series=129812&rev=4
[3] https://patchwork.freedesktop.org/patch/642597/?series=129812&rev=4

Regards,
Uma Shankar

> Signed-off-by: Alex Hung <alex.hung@amd.com>
> Co-developed-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
> v8:
>  - Add DRM_MODE_PROP_ATOMIC to drm_property_create_range (Simon Ser)
>  - Change "1D Curve Custom LUT" to "1D LUT" (Simon Ser)
> 
> v7:
>  - Change "size" to "lut_size" (this affects multiple following commits)
>  - Move "lut_size" from drm_colorop_state to drm_colorop
>  - Modify other files accordingly (i.e. from drm_colorop_state->size
>    to drm_colorop->lut_size)
> 
> v5:
>  - Add kernel doc
>  - Define SIZE in similar manner to GAMMA_SIZE on drm_crtc (Melissa)
> 
>  drivers/gpu/drm/drm_atomic.c      |  4 +++
>  drivers/gpu/drm/drm_atomic_uapi.c |  5 ++++
>  drivers/gpu/drm/drm_colorop.c     | 43 +++++++++++++++++++++++++++++++
>  include/drm/drm_colorop.h         | 16 ++++++++++++
>  include/uapi/drm/drm_mode.h       | 14 ++++++++++
>  5 files changed, 82 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 5223cf363692..f713d177241d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -793,6 +793,10 @@ static void drm_atomic_colorop_print_state(struct
> drm_printer *p,
>  		drm_printf(p, "\tcurve_1d_type=%s\n",
>  			   drm_get_colorop_curve_1d_type_name(state-
> >curve_1d_type));
>  		break;
> +	case DRM_COLOROP_1D_LUT:
> +		drm_printf(p, "\tsize=%d\n", colorop->lut_size);
> +		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data-
> >base.id : 0);
> +		break;
>  	case DRM_COLOROP_CTM_3X4:
>  		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data-
> >base.id : 0);
>  		break;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index dcd12fc0bd8f..dfd88a227da7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -713,6 +713,9 @@ static int drm_atomic_color_set_data_property(struct
> drm_colorop *colorop,
>  	bool replaced = false;
> 
>  	switch (colorop->type) {
> +	case DRM_COLOROP_1D_LUT:
> +		size = colorop->lut_size * sizeof(struct drm_color_lut);
> +		break;
>  	case DRM_COLOROP_CTM_3X4:
>  		size = sizeof(struct drm_color_ctm_3x4);
>  		break;
> @@ -762,6 +765,8 @@ drm_atomic_colorop_get_property(struct drm_colorop
> *colorop,
>  		*val = state->bypass;
>  	} else if (property == colorop->curve_1d_type_property) {
>  		*val = state->curve_1d_type;
> +	} else if (property == colorop->lut_size_property) {
> +		*val = colorop->lut_size;
>  	} else if (property == colorop->data_property) {
>  		*val = (state->data) ? state->data->base.id : 0;
>  	} else {
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index fcb4a8d0e38d..15ffbba60b3d 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -64,6 +64,7 @@
> 
>  static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
>  	{ DRM_COLOROP_1D_CURVE, "1D Curve" },
> +	{ DRM_COLOROP_1D_LUT, "1D LUT" },
>  	{ DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
>  };
> 
> @@ -225,6 +226,47 @@ static int drm_colorop_create_data_prop(struct
> drm_device *dev, struct drm_color
>  	return 0;
>  }
> 
> +/**
> + * drm_colorop_curve_1d_lut_init - Initialize a DRM_COLOROP_1D_LUT
> + *
> + * @dev: DRM device
> + * @colorop: The drm_colorop object to initialize
> + * @plane: The associated drm_plane
> + * @lut_size: LUT size supported by driver
> + * @return zero on success, -E value on failure  */ int
> +drm_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_colorop
> *colorop,
> +				  struct drm_plane *plane, uint32_t lut_size) {
> +	struct drm_property *prop;
> +	int ret;
> +
> +	ret = drm_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_LUT);
> +	if (ret)
> +		return ret;
> +
> +	/* initialize 1D LUT only attribute */
> +	/* LUT size */
> +	prop = drm_property_create_range(dev,
> DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ATOMIC,
> +					 "SIZE", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	colorop->lut_size_property = prop;
> +	drm_object_attach_property(&colorop->base, colorop-
> >lut_size_property, lut_size);
> +	colorop->lut_size = lut_size;
> +
> +	/* data */
> +	ret = drm_colorop_create_data_prop(dev, colorop);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_reset(colorop);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_colorop_curve_1d_lut_init);
> +
>  int drm_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop
> *colorop,
>  			     struct drm_plane *plane)
>  {
> @@ -333,6 +375,7 @@ void drm_colorop_reset(struct drm_colorop *colorop)
> 
>  static const char * const colorop_type_name[] = {
>  	[DRM_COLOROP_1D_CURVE] = "1D Curve",
> +	[DRM_COLOROP_1D_LUT] = "1D LUT",
>  	[DRM_COLOROP_CTM_3X4] = "3x4 Matrix",
>  };
> 
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h index
> d3a60c1beed1..d66c76033343 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -259,6 +259,13 @@ struct drm_colorop {
>  	 */
>  	struct drm_property *bypass_property;
> 
> +	/**
> +	 * @lut_size:
> +	 *
> +	 * Number of entries of the custom LUT. This should be read-only.
> +	 */
> +	uint32_t lut_size;
> +
>  	/**
>  	 * @curve_1d_type_property:
>  	 *
> @@ -266,6 +273,13 @@ struct drm_colorop {
>  	 */
>  	struct drm_property *curve_1d_type_property;
> 
> +	/**
> +	 * @lut_size_property:
> +	 *
> +	 * Size property for custom LUT from userspace.
> +	 */
> +	struct drm_property *lut_size_property;
> +
>  	/**
>  	 * @data_property:
>  	 *
> @@ -310,6 +324,8 @@ static inline struct drm_colorop *drm_colorop_find(struct
> drm_device *dev,
> 
>  int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop
> *colorop,
>  			      struct drm_plane *plane, u64 supported_tfs);
> +int drm_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_colorop
> *colorop,
> +				  struct drm_plane *plane, uint32_t lut_size);
>  int drm_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop
> *colorop,
>  			     struct drm_plane *plane);
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 651bdf48b766..dde250dd7a51 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -890,6 +890,20 @@ enum drm_colorop_type {
>  	 */
>  	DRM_COLOROP_1D_CURVE,
> 
> +	/**
> +	 * @DRM_COLOROP_1D_LUT:
> +	 *
> +	 * enum string "1D LUT"
> +	 *
> +	 * A simple 1D LUT of uniformly spaced &drm_color_lut entries,
> +	 * packed into a blob via the DATA property. The driver's
> +	 * expected LUT size is advertised via the SIZE property.
> +	 *
> +	 * The DATA blob is an array of struct drm_color_lut with size
> +	 * of "lut_size".
> +	 */
> +	DRM_COLOROP_1D_LUT,
> +
>  	/**
>  	 * @DRM_COLOROP_CTM_3X4:
>  	 *
> --
> 2.43.0

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Simon Ser]

On Tuesday, April 15th, 2025 at 08:09, Shankar, Uma <uma.shankar@intel.com> wrote:

> We want to have just one change in the way we expose the hardware capabilities else
> all looks good in general.

I would really recommend leaving this as a follow-up extension. It's a
complicated addition that requires more discussion.

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Shankar, Uma]

> -----Original Message-----
> From: Simon Ser <contact@emersion.fr>
> Sent: Tuesday, April 15, 2025 11:47 AM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: Alex Hung <alex.hung@amd.com>; dri-devel@lists.freedesktop.org; amd-
> gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; wayland-
> devel@lists.freedesktop.org; harry.wentland@amd.com; leo.liu@amd.com;
> ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
> Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> Subject: RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> 
> On Tuesday, April 15th, 2025 at 08:09, Shankar, Uma <uma.shankar@intel.com>
> wrote:
> 
> > We want to have just one change in the way we expose the hardware
> > capabilities else all looks good in general.
> 
> I would really recommend leaving this as a follow-up extension. It's a complicated
> addition that requires more discussion.

Hi Simon,
We have tried to solve the complex part and made it simple to understand and implement
along with a reference implementation [1] (can also help add the same for AMD case as well).
Without this we will end up with up 2 interfaces for 1dL Lut which is not nice where the one above
will be able to cover the current one. Let us know the problems with the proposed interface and we can
work to fix the same. But having a common and single interface is good and the current one will not fit
Intel's color pipeline distribution so the generic one anyways will be needed, and it will benefit userspace
to know the underlying LUT distribution to compute the LUT samples.

[1] https://patchwork.freedesktop.org/series/129812/

Regards,
Uma Shankar

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Harry Wentland]

On 2025-04-15 02:40, Shankar, Uma wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Ser <contact@emersion.fr>
>> Sent: Tuesday, April 15, 2025 11:47 AM
>> To: Shankar, Uma <uma.shankar@intel.com>
>> Cc: Alex Hung <alex.hung@amd.com>; dri-devel@lists.freedesktop.org; amd-
>> gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; wayland-
>> devel@lists.freedesktop.org; harry.wentland@amd.com; leo.liu@amd.com;
>> ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
>> Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
>> Subject: RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
>>
>> On Tuesday, April 15th, 2025 at 08:09, Shankar, Uma <uma.shankar@intel.com>
>> wrote:
>>
>>> We want to have just one change in the way we expose the hardware
>>> capabilities else all looks good in general.
>>
>> I would really recommend leaving this as a follow-up extension. It's a complicated
>> addition that requires more discussion.
> 
> Hi Simon,
> We have tried to solve the complex part and made it simple to understand and implement
> along with a reference implementation [1] (can also help add the same for AMD case as well).
> Without this we will end up with up 2 interfaces for 1dL Lut which is not nice where the one above
> will be able to cover the current one. Let us know the problems with the proposed interface and we can
> work to fix the same. But having a common and single interface is good and the current one will not fit
> Intel's color pipeline distribution so the generic one anyways will be needed, and it will benefit userspace
> to know the underlying LUT distribution to compute the LUT samples.
> 
> [1] https://patchwork.freedesktop.org/series/129812/
> 

I think there is a lot of value in giving userspace a simple LUT
to work with. There are many compositors and many compositor
maintainers. When someone new jumps into color management usually
same thing happens. It starts with "it's not too complicated",
and then over a period of time progresses to "this is very much
non-trivial" as understanding one bit usually opens ten more
questions.

Forcing people to deal with another level of complexity will
discourage implementations and be counterproductive to furthering
adoption of color operations for HW acceleration, IMO.

I'm am not opposed to a complex LUT definition but I don't think
it should replace a simple and well-understood definition.

Harry

> Regards,
> Uma Shankar
> 

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Simon Ser]

On Tuesday, April 15th, 2025 at 17:05, Harry Wentland <harry.wentland@amd.com> wrote:

> > > > We want to have just one change in the way we expose the hardware
> > > > capabilities else all looks good in general.
> > > 
> > > I would really recommend leaving this as a follow-up extension. It's a complicated
> > > addition that requires more discussion.
> > 
> > Hi Simon,
> > We have tried to solve the complex part and made it simple to understand and implement
> > along with a reference implementation [1] (can also help add the same for AMD case as well).
> > Without this we will end up with up 2 interfaces for 1dL Lut which is not nice where the one above
> > will be able to cover the current one. Let us know the problems with the proposed interface and we can
> > work to fix the same. But having a common and single interface is good and the current one will not fit
> > Intel's color pipeline distribution so the generic one anyways will be needed, and it will benefit userspace
> > to know the underlying LUT distribution to compute the LUT samples.
> > 
> > [1] https://patchwork.freedesktop.org/series/129812/
> 
> I think there is a lot of value in giving userspace a simple LUT
> to work with. There are many compositors and many compositor
> maintainers. When someone new jumps into color management usually
> same thing happens. It starts with "it's not too complicated",
> and then over a period of time progresses to "this is very much
> non-trivial" as understanding one bit usually opens ten more
> questions.
> 
> Forcing people to deal with another level of complexity will
> discourage implementations and be counterproductive to furthering
> adoption of color operations for HW acceleration, IMO.
> 
> I'm am not opposed to a complex LUT definition but I don't think
> it should replace a simple and well-understood definition.

Agreed. To add on this, I think shipping many additional features from
day one significantly increases the work load (more code to write,
review, test at once) and we'd also need to go through supplementary
rounds to validate the API design and ensure it's not too
Intel-specific. Also adding this feature as a second step will prove
that the API is as extensible as we desire. 

I don't really understand why it's important to have this feature in
the first version. Intel has been converting simple LUTs into the
fancy distribution for the existing GAMMA_LUT and DEGAMMA_LUT for a
while, so can do it for colorop as well. The upsides of the fancy
distribution is more precise and smaller LUTs, but that doesn't seem
critical?

Simon

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Shankar, Uma]

> -----Original Message-----
> From: wayland-devel <wayland-devel-bounces@lists.freedesktop.org> On Behalf
> Of Simon Ser
> Sent: Tuesday, April 15, 2025 9:55 PM
> To: Harry Wentland <harry.wentland@amd.com>
> Cc: Shankar, Uma <uma.shankar@intel.com>; Alex Hung <alex.hung@amd.com>;
> dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> leo.liu@amd.com; ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
> Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> 
> On Tuesday, April 15th, 2025 at 17:05, Harry Wentland
> <harry.wentland@amd.com> wrote:
> 
> > > > > We want to have just one change in the way we expose the
> > > > > hardware capabilities else all looks good in general.
> > > >
> > > > I would really recommend leaving this as a follow-up extension.
> > > > It's a complicated addition that requires more discussion.
> > >
> > > Hi Simon,
> > > We have tried to solve the complex part and made it simple to
> > > understand and implement along with a reference implementation [1] (can
> also help add the same for AMD case as well).
> > > Without this we will end up with up 2 interfaces for 1dL Lut which
> > > is not nice where the one above will be able to cover the current
> > > one. Let us know the problems with the proposed interface and we can
> > > work to fix the same. But having a common and single interface is
> > > good and the current one will not fit Intel's color pipeline distribution so the
> generic one anyways will be needed, and it will benefit userspace to know the
> underlying LUT distribution to compute the LUT samples.
> > >
> > > [1] https://patchwork.freedesktop.org/series/129812/
> >
> > I think there is a lot of value in giving userspace a simple LUT to
> > work with. There are many compositors and many compositor maintainers.
> > When someone new jumps into color management usually same thing
> > happens. It starts with "it's not too complicated", and then over a
> > period of time progresses to "this is very much non-trivial" as
> > understanding one bit usually opens ten more questions.
> >
> > Forcing people to deal with another level of complexity will
> > discourage implementations and be counterproductive to furthering
> > adoption of color operations for HW acceleration, IMO.
> >
> > I'm am not opposed to a complex LUT definition but I don't think it
> > should replace a simple and well-understood definition.
> 
> Agreed. To add on this, I think shipping many additional features from day one
> significantly increases the work load (more code to write, review, test at once)
> and we'd also need to go through supplementary rounds to validate the API
> design and ensure it's not too Intel-specific. Also adding this feature as a second
> step will prove that the API is as extensible as we desire.

Apologies for getting back late to the thread.  Sure, we can go with this and extend it later.

> I don't really understand why it's important to have this feature in the first
> version. Intel has been converting simple LUTs into the fancy distribution for the
> existing GAMMA_LUT and DEGAMMA_LUT for a while, so can do it for colorop as
> well. The upsides of the fancy distribution is more precise and smaller LUTs, but
> that doesn't seem critical?

Yes Simon, Intel has managed to somehow fit multiple segments with the existing design. It was
manageable where we had lut divided in 2-3 segments, but we have some cases where distribution
of luts is totally non-uniform (in logarithmic fashion as an example). But will add this as follow up once
the basic UAPI is merged.

One request though: Can we enhance the lut samples from existing 16bits to 32bits as lut precision is
going to be more than 16 in certain hardware. While adding the new UAPI, lets extend this to 32 to make it future proof.
Reference: https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4

+/**
+ * struct drm_color_lut_32 - Represents high precision lut values
+ *
+ * Creating 32 bit palette entries for better data
+ * precision. This will be required for HDR and
+ * similar color processing usecases.
+ */
+struct drm_color_lut_32 {
+	/*
+	 * Data for high precision LUTs
+	 */
+	__u32 red;
+	__u32 green;
+	__u32 blue;
+	__u32 reserved;
+};

Regards,
Uma Shankar

> Simon

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Thu, 22 May 2025 11:33:00 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> One request though: Can we enhance the lut samples from existing 16bits to 32bits as lut precision is
> going to be more than 16 in certain hardware. While adding the new UAPI, lets extend this to 32 to make it future proof.
> Reference: https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
> 
> +/**
> + * struct drm_color_lut_32 - Represents high precision lut values
> + *
> + * Creating 32 bit palette entries for better data
> + * precision. This will be required for HDR and
> + * similar color processing usecases.
> + */
> +struct drm_color_lut_32 {
> +	/*
> +	 * Data for high precision LUTs
> +	 */
> +	__u32 red;
> +	__u32 green;
> +	__u32 blue;
> +	__u32 reserved;
> +};

Hi,

I suppose you need this much precision for optical data? If so,
floating-point would be much more appropriate and we could probably
keep 16-bit storage.

What does the "more than 16-bit" hardware actually use? ISTR at least
AMD having some sort of float'ish point internal pipeline?

This sounds the same thing as non-uniformly distributed taps in a LUT.
That mimics floating-point input while this feels like floating-point
output of a LUT.

I've recently decided for myself (and Weston) that I will never store
optical data in an integer format, because it is far too wasteful. That's
why the electrical encodings like power-2.2 are so useful, not just for
emulating a CRT.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Shankar, Uma]

> -----Original Message-----
> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> Sent: Friday, May 30, 2025 7:28 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: Simon Ser <contact@emersion.fr>; Harry Wentland
> <harry.wentland@amd.com>; Alex Hung <alex.hung@amd.com>; dri-
> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> leo.liu@amd.com; ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
> Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> 
> On Thu, 22 May 2025 11:33:00 +0000
> "Shankar, Uma" <uma.shankar@intel.com> wrote:
> 
> > One request though: Can we enhance the lut samples from existing
> > 16bits to 32bits as lut precision is going to be more than 16 in certain hardware.
> While adding the new UAPI, lets extend this to 32 to make it future proof.
> > Reference:
> > https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
> >
> > +/**
> > + * struct drm_color_lut_32 - Represents high precision lut values
> > + *
> > + * Creating 32 bit palette entries for better data
> > + * precision. This will be required for HDR and
> > + * similar color processing usecases.
> > + */
> > +struct drm_color_lut_32 {
> > +	/*
> > +	 * Data for high precision LUTs
> > +	 */
> > +	__u32 red;
> > +	__u32 green;
> > +	__u32 blue;
> > +	__u32 reserved;
> > +};
> 
> Hi,
> 
> I suppose you need this much precision for optical data? If so, floating-point would
> be much more appropriate and we could probably keep 16-bit storage.
> 
> What does the "more than 16-bit" hardware actually use? ISTR at least AMD
> having some sort of float'ish point internal pipeline?
> 
> This sounds the same thing as non-uniformly distributed taps in a LUT.
> That mimics floating-point input while this feels like floating-point output of a LUT.
> 
> I've recently decided for myself (and Weston) that I will never store optical data in
> an integer format, because it is far too wasteful. That's why the electrical
> encodings like power-2.2 are so useful, not just for emulating a CRT.

Hi Pekka,
Internal pipeline in hardware can operate at higher precision than the input framebuffer
to plane engines. So, in case we have optical data of 16bits or 10bits precision, hardware
can scale this up to higher precision in internal pipeline in hardware to take care of rounding
and overflow issues. Even FP16 optical data will be normalized and converted internally for
further processing.

Input to LUT hardware can be 16bits or even higher, so the look up table we program can
be of higher precision than 16 (certain cases 24 in Intel pipeline). This is later truncated to bpc supported
in output formats from sync (10, 12 or 16), mostly for electrical value to be sent to sink.

Hence requesting to increase the container from current u16 to u32, to get advantage of higher
precision luts.

Thanks & Regards,
Uma Shankar

> 
> Thanks,
> pq

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4903 bytes --]

On Tue, 3 Jun 2025 08:30:23 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> > -----Original Message-----
> > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Sent: Friday, May 30, 2025 7:28 PM
> > To: Shankar, Uma <uma.shankar@intel.com>
> > Cc: Simon Ser <contact@emersion.fr>; Harry Wentland
> > <harry.wentland@amd.com>; Alex Hung <alex.hung@amd.com>; dri-
> > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> > leo.liu@amd.com; ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
> > Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> > Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> > 
> > On Thu, 22 May 2025 11:33:00 +0000
> > "Shankar, Uma" <uma.shankar@intel.com> wrote:
> >   
> > > One request though: Can we enhance the lut samples from existing
> > > 16bits to 32bits as lut precision is going to be more than 16 in certain hardware.  
> > While adding the new UAPI, lets extend this to 32 to make it future proof.  
> > > Reference:
> > > https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
> > >
> > > +/**
> > > + * struct drm_color_lut_32 - Represents high precision lut values
> > > + *
> > > + * Creating 32 bit palette entries for better data
> > > + * precision. This will be required for HDR and
> > > + * similar color processing usecases.
> > > + */
> > > +struct drm_color_lut_32 {
> > > +	/*
> > > +	 * Data for high precision LUTs
> > > +	 */
> > > +	__u32 red;
> > > +	__u32 green;
> > > +	__u32 blue;
> > > +	__u32 reserved;
> > > +};  
> > 
> > Hi,
> > 
> > I suppose you need this much precision for optical data? If so, floating-point would
> > be much more appropriate and we could probably keep 16-bit storage.
> > 
> > What does the "more than 16-bit" hardware actually use? ISTR at least AMD
> > having some sort of float'ish point internal pipeline?
> > 
> > This sounds the same thing as non-uniformly distributed taps in a LUT.
> > That mimics floating-point input while this feels like floating-point output of a LUT.
> > 
> > I've recently decided for myself (and Weston) that I will never store optical data in
> > an integer format, because it is far too wasteful. That's why the electrical
> > encodings like power-2.2 are so useful, not just for emulating a CRT.  
> 
> Hi Pekka,
> Internal pipeline in hardware can operate at higher precision than the input framebuffer
> to plane engines. So, in case we have optical data of 16bits or 10bits precision, hardware
> can scale this up to higher precision in internal pipeline in hardware to take care of rounding
> and overflow issues. Even FP16 optical data will be normalized and converted internally for
> further processing.

Is it integer or floating-point?

If we take the full range of PQ as optical and put it into 16-bit
integer format, the luminance step from code 1 to code 2 is 0.15 cd/m².
That seems like a huge step in the dark end. Such a step would
probably need to be divided over several taps in a LUT, which wouldn't
be possible.

In that sense, if a LUT is used for the PQ EOTF, I totally agree that
16-bit integer won't be even nearly enough precision.

This actually points out the caveat that increasing the number of taps
in a LUT can cause the LUT to become non-monotonic when the sample
precision runs out. That is, consecutive taps don't always increase in
value.

> Input to LUT hardware can be 16bits or even higher, so the look up table we program can
> be of higher precision than 16 (certain cases 24 in Intel pipeline). This is later truncated to bpc supported
> in output formats from sync (10, 12 or 16), mostly for electrical value to be sent to sink.
> 
> Hence requesting to increase the container from current u16 to u32, to get advantage of higher
> precision luts.

My argument though is to use a floating-point format for the LUT samples
instead of adding more and more integer bits. That naturally puts more
precision where it is needed: near zero.

A driver can easily convert that to any format the hardware needs.

However, it might make best sense for a driver to expose a LUT with a
format that best matches the hardware precision, especially
floating-point vs. integer.

I guess we may eventually need both 32 bpc integer and 16 (or 32) bpc
floating-point.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Harry Wentland]

On 2025-06-03 06:51, Pekka Paalanen wrote:
> On Tue, 3 Jun 2025 08:30:23 +0000
> "Shankar, Uma" <uma.shankar@intel.com> wrote:
> 
>>> -----Original Message-----
>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Sent: Friday, May 30, 2025 7:28 PM
>>> To: Shankar, Uma <uma.shankar@intel.com>
>>> Cc: Simon Ser <contact@emersion.fr>; Harry Wentland
>>> <harry.wentland@amd.com>; Alex Hung <alex.hung@amd.com>; dri-
>>> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
>>> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
>>> leo.liu@amd.com; ville.syrjala@linux.intel.com; pekka.paalanen@collabora.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; marcan@marcan.st;
>>> Liviu.Dudau@arm.com; sashamcintosh@google.com; Borah, Chaitanya Kumar
>>> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
>>> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
>>>
>>> On Thu, 22 May 2025 11:33:00 +0000
>>> "Shankar, Uma" <uma.shankar@intel.com> wrote:
>>>   
>>>> One request though: Can we enhance the lut samples from existing
>>>> 16bits to 32bits as lut precision is going to be more than 16 in certain hardware.  
>>> While adding the new UAPI, lets extend this to 32 to make it future proof.  
>>>> Reference:
>>>> https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
>>>>
>>>> +/**
>>>> + * struct drm_color_lut_32 - Represents high precision lut values
>>>> + *
>>>> + * Creating 32 bit palette entries for better data
>>>> + * precision. This will be required for HDR and
>>>> + * similar color processing usecases.
>>>> + */
>>>> +struct drm_color_lut_32 {
>>>> +	/*
>>>> +	 * Data for high precision LUTs
>>>> +	 */
>>>> +	__u32 red;
>>>> +	__u32 green;
>>>> +	__u32 blue;
>>>> +	__u32 reserved;
>>>> +};  
>>>
>>> Hi,
>>>
>>> I suppose you need this much precision for optical data? If so, floating-point would
>>> be much more appropriate and we could probably keep 16-bit storage.
>>>
>>> What does the "more than 16-bit" hardware actually use? ISTR at least AMD
>>> having some sort of float'ish point internal pipeline?
>>>
>>> This sounds the same thing as non-uniformly distributed taps in a LUT.
>>> That mimics floating-point input while this feels like floating-point output of a LUT.
>>>
>>> I've recently decided for myself (and Weston) that I will never store optical data in
>>> an integer format, because it is far too wasteful. That's why the electrical
>>> encodings like power-2.2 are so useful, not just for emulating a CRT.  
>>
>> Hi Pekka,
>> Internal pipeline in hardware can operate at higher precision than the input framebuffer
>> to plane engines. So, in case we have optical data of 16bits or 10bits precision, hardware
>> can scale this up to higher precision in internal pipeline in hardware to take care of rounding
>> and overflow issues. Even FP16 optical data will be normalized and converted internally for
>> further processing.
> 
> Is it integer or floating-point?
> 

For AMD the internal format is floating point with slightly
higher precision than FP16.

> If we take the full range of PQ as optical and put it into 16-bit
> integer format, the luminance step from code 1 to code 2 is 0.15 cd/m².
> That seems like a huge step in the dark end. Such a step would
> probably need to be divided over several taps in a LUT, which wouldn't
> be possible.
> 

Right, and with 32-bpc we'll get a luminance step size of
~0.0000023 cd/m^2, which seems plenty fine-grained.

> In that sense, if a LUT is used for the PQ EOTF, I totally agree that
> 16-bit integer won't be even nearly enough precision.
> 
> This actually points out the caveat that increasing the number of taps
> in a LUT can cause the LUT to become non-monotonic when the sample
> precision runs out. That is, consecutive taps don't always increase in
> value.
> 
>> Input to LUT hardware can be 16bits or even higher, so the look up table we program can
>> be of higher precision than 16 (certain cases 24 in Intel pipeline). This is later truncated to bpc supported
>> in output formats from sync (10, 12 or 16), mostly for electrical value to be sent to sink.
>>
>> Hence requesting to increase the container from current u16 to u32, to get advantage of higher
>> precision luts.
> 
> My argument though is to use a floating-point format for the LUT samples
> instead of adding more and more integer bits. That naturally puts more
> precision where it is needed: near zero.
> 
> A driver can easily convert that to any format the hardware needs.
> 
> However, it might make best sense for a driver to expose a LUT with a
> format that best matches the hardware precision, especially
> floating-point vs. integer.
> 
> I guess we may eventually need both 32 bpc integer and 16 (or 32) bpc
> floating-point.
> 

While I like floating point better for representing these things
I don't think it's a great idea to pass floating point values
via IOCTLs but 32 bpc integer values make sense here.

Thanks, Uma, for pushing on this.

Harry

> 
> Thanks,
> pq

RE: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Shankar, Uma]

> -----Original Message-----
> From: Harry Wentland <harry.wentland@amd.com>
> Sent: Wednesday, June 4, 2025 1:57 AM
> To: Pekka Paalanen <pekka.paalanen@collabora.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Cc: Simon Ser <contact@emersion.fr>; Alex Hung <alex.hung@amd.com>; dri-
> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> leo.liu@amd.com; ville.syrjala@linux.intel.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; marcan@marcan.st; Liviu.Dudau@arm.com;
> sashamcintosh@google.com; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> 
> 
> 
> On 2025-06-03 06:51, Pekka Paalanen wrote:
> > On Tue, 3 Jun 2025 08:30:23 +0000
> > "Shankar, Uma" <uma.shankar@intel.com> wrote:
> >
> >>> -----Original Message-----
> >>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>> Sent: Friday, May 30, 2025 7:28 PM
> >>> To: Shankar, Uma <uma.shankar@intel.com>
> >>> Cc: Simon Ser <contact@emersion.fr>; Harry Wentland
> >>> <harry.wentland@amd.com>; Alex Hung <alex.hung@amd.com>; dri-
> >>> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> >>> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> >>> leo.liu@amd.com; ville.syrjala@linux.intel.com;
> >>> pekka.paalanen@collabora.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; marcan@marcan.st; Liviu.Dudau@arm.com;
> >>> sashamcintosh@google.com; Borah, Chaitanya Kumar
> >>> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> >>> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT
> >>> type
> >>>
> >>> On Thu, 22 May 2025 11:33:00 +0000
> >>> "Shankar, Uma" <uma.shankar@intel.com> wrote:
> >>>
> >>>> One request though: Can we enhance the lut samples from existing
> >>>> 16bits to 32bits as lut precision is going to be more than 16 in certain
> hardware.
> >>> While adding the new UAPI, lets extend this to 32 to make it future proof.
> >>>> Reference:
> >>>> https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
> >>>>
> >>>> +/**
> >>>> + * struct drm_color_lut_32 - Represents high precision lut values
> >>>> + *
> >>>> + * Creating 32 bit palette entries for better data
> >>>> + * precision. This will be required for HDR and
> >>>> + * similar color processing usecases.
> >>>> + */
> >>>> +struct drm_color_lut_32 {
> >>>> +	/*
> >>>> +	 * Data for high precision LUTs
> >>>> +	 */
> >>>> +	__u32 red;
> >>>> +	__u32 green;
> >>>> +	__u32 blue;
> >>>> +	__u32 reserved;
> >>>> +};
> >>>
> >>> Hi,
> >>>
> >>> I suppose you need this much precision for optical data? If so,
> >>> floating-point would be much more appropriate and we could probably keep
> 16-bit storage.
> >>>
> >>> What does the "more than 16-bit" hardware actually use? ISTR at
> >>> least AMD having some sort of float'ish point internal pipeline?
> >>>
> >>> This sounds the same thing as non-uniformly distributed taps in a LUT.
> >>> That mimics floating-point input while this feels like floating-point output of a
> LUT.
> >>>
> >>> I've recently decided for myself (and Weston) that I will never
> >>> store optical data in an integer format, because it is far too
> >>> wasteful. That's why the electrical encodings like power-2.2 are so useful, not
> just for emulating a CRT.
> >>
> >> Hi Pekka,
> >> Internal pipeline in hardware can operate at higher precision than
> >> the input framebuffer to plane engines. So, in case we have optical
> >> data of 16bits or 10bits precision, hardware can scale this up to
> >> higher precision in internal pipeline in hardware to take care of
> >> rounding and overflow issues. Even FP16 optical data will be normalized and
> converted internally for further processing.
> >
> > Is it integer or floating-point?
> >
> 
> For AMD the internal format is floating point with slightly higher precision than
> FP16.
> 
> > If we take the full range of PQ as optical and put it into 16-bit
> > integer format, the luminance step from code 1 to code 2 is 0.15 cd/m².
> > That seems like a huge step in the dark end. Such a step would
> > probably need to be divided over several taps in a LUT, which wouldn't
> > be possible.
> >
> 
> Right, and with 32-bpc we'll get a luminance step size of
> ~0.0000023 cd/m^2, which seems plenty fine-grained.
> 
> > In that sense, if a LUT is used for the PQ EOTF, I totally agree that
> > 16-bit integer won't be even nearly enough precision.
> >
> > This actually points out the caveat that increasing the number of taps
> > in a LUT can cause the LUT to become non-monotonic when the sample
> > precision runs out. That is, consecutive taps don't always increase in
> > value.
> >
> >> Input to LUT hardware can be 16bits or even higher, so the look up
> >> table we program can be of higher precision than 16 (certain cases 24
> >> in Intel pipeline). This is later truncated to bpc supported in output formats from
> sync (10, 12 or 16), mostly for electrical value to be sent to sink.
> >>
> >> Hence requesting to increase the container from current u16 to u32,
> >> to get advantage of higher precision luts.
> >
> > My argument though is to use a floating-point format for the LUT
> > samples instead of adding more and more integer bits. That naturally
> > puts more precision where it is needed: near zero.
> >
> > A driver can easily convert that to any format the hardware needs.
> >
> > However, it might make best sense for a driver to expose a LUT with a
> > format that best matches the hardware precision, especially
> > floating-point vs. integer.
> >
> > I guess we may eventually need both 32 bpc integer and 16 (or 32) bpc
> > floating-point.
> >
> 
> While I like floating point better for representing these things I don't think it's a
> great idea to pass floating point values via IOCTLs but 32 bpc integer values make
> sense here.
> 

Nice, we all are on same page here.

> Thanks, Uma, for pushing on this.

Thanks Harry and Pekka for valuable inputs.

Regards,
Uma Shankar

> Harry
> 
> >
> > Thanks,
> > pq

Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7169 bytes --]

On Wed, 4 Jun 2025 18:59:22 +0000
"Shankar, Uma" <uma.shankar@intel.com> wrote:

> > -----Original Message-----
> > From: Harry Wentland <harry.wentland@amd.com>
> > Sent: Wednesday, June 4, 2025 1:57 AM
> > To: Pekka Paalanen <pekka.paalanen@collabora.com>; Shankar, Uma
> > <uma.shankar@intel.com>
> > Cc: Simon Ser <contact@emersion.fr>; Alex Hung <alex.hung@amd.com>; dri-
> > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> > leo.liu@amd.com; ville.syrjala@linux.intel.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; marcan@marcan.st; Liviu.Dudau@arm.com;
> > sashamcintosh@google.com; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> > Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT type
> > 
> > 
> > 
> > On 2025-06-03 06:51, Pekka Paalanen wrote:  
> > > On Tue, 3 Jun 2025 08:30:23 +0000
> > > "Shankar, Uma" <uma.shankar@intel.com> wrote:
> > >  
> > >>> -----Original Message-----
> > >>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >>> Sent: Friday, May 30, 2025 7:28 PM
> > >>> To: Shankar, Uma <uma.shankar@intel.com>
> > >>> Cc: Simon Ser <contact@emersion.fr>; Harry Wentland
> > >>> <harry.wentland@amd.com>; Alex Hung <alex.hung@amd.com>; dri-
> > >>> devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; intel-
> > >>> gfx@lists.freedesktop.org; wayland-devel@lists.freedesktop.org;
> > >>> leo.liu@amd.com; ville.syrjala@linux.intel.com;
> > >>> pekka.paalanen@collabora.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; marcan@marcan.st; Liviu.Dudau@arm.com;
> > >>> sashamcintosh@google.com; Borah, Chaitanya Kumar
> > >>> <chaitanya.kumar.borah@intel.com>; louis.chauvet@bootlin.com
> > >>> Subject: Re: [PATCH V8 32/43] drm/colorop: Add 1D Curve Custom LUT
> > >>> type
> > >>>
> > >>> On Thu, 22 May 2025 11:33:00 +0000
> > >>> "Shankar, Uma" <uma.shankar@intel.com> wrote:
> > >>>  
> > >>>> One request though: Can we enhance the lut samples from existing
> > >>>> 16bits to 32bits as lut precision is going to be more than 16 in certain  
> > hardware.  
> > >>> While adding the new UAPI, lets extend this to 32 to make it future proof.  
> > >>>> Reference:
> > >>>> https://patchwork.freedesktop.org/patch/642592/?series=129811&rev=4
> > >>>>
> > >>>> +/**
> > >>>> + * struct drm_color_lut_32 - Represents high precision lut values
> > >>>> + *
> > >>>> + * Creating 32 bit palette entries for better data
> > >>>> + * precision. This will be required for HDR and
> > >>>> + * similar color processing usecases.
> > >>>> + */
> > >>>> +struct drm_color_lut_32 {
> > >>>> +	/*
> > >>>> +	 * Data for high precision LUTs
> > >>>> +	 */
> > >>>> +	__u32 red;
> > >>>> +	__u32 green;
> > >>>> +	__u32 blue;
> > >>>> +	__u32 reserved;
> > >>>> +};  
> > >>>
> > >>> Hi,
> > >>>
> > >>> I suppose you need this much precision for optical data? If so,
> > >>> floating-point would be much more appropriate and we could probably keep  
> > 16-bit storage.  
> > >>>
> > >>> What does the "more than 16-bit" hardware actually use? ISTR at
> > >>> least AMD having some sort of float'ish point internal pipeline?
> > >>>
> > >>> This sounds the same thing as non-uniformly distributed taps in a LUT.
> > >>> That mimics floating-point input while this feels like floating-point output of a  
> > LUT.  
> > >>>
> > >>> I've recently decided for myself (and Weston) that I will never
> > >>> store optical data in an integer format, because it is far too
> > >>> wasteful. That's why the electrical encodings like power-2.2 are so useful, not  
> > just for emulating a CRT.  
> > >>
> > >> Hi Pekka,
> > >> Internal pipeline in hardware can operate at higher precision than
> > >> the input framebuffer to plane engines. So, in case we have optical
> > >> data of 16bits or 10bits precision, hardware can scale this up to
> > >> higher precision in internal pipeline in hardware to take care of
> > >> rounding and overflow issues. Even FP16 optical data will be normalized and  
> > converted internally for further processing.  
> > >
> > > Is it integer or floating-point?
> > >  
> > 
> > For AMD the internal format is floating point with slightly higher precision than
> > FP16.
> >   
> > > If we take the full range of PQ as optical and put it into 16-bit
> > > integer format, the luminance step from code 1 to code 2 is 0.15 cd/m².
> > > That seems like a huge step in the dark end. Such a step would
> > > probably need to be divided over several taps in a LUT, which wouldn't
> > > be possible.
> > >  
> > 
> > Right, and with 32-bpc we'll get a luminance step size of
> > ~0.0000023 cd/m^2, which seems plenty fine-grained.
> >   
> > > In that sense, if a LUT is used for the PQ EOTF, I totally agree that
> > > 16-bit integer won't be even nearly enough precision.
> > >
> > > This actually points out the caveat that increasing the number of taps
> > > in a LUT can cause the LUT to become non-monotonic when the sample
> > > precision runs out. That is, consecutive taps don't always increase in
> > > value.
> > >  
> > >> Input to LUT hardware can be 16bits or even higher, so the look up
> > >> table we program can be of higher precision than 16 (certain cases 24
> > >> in Intel pipeline). This is later truncated to bpc supported in output formats from  
> > sync (10, 12 or 16), mostly for electrical value to be sent to sink.  
> > >>
> > >> Hence requesting to increase the container from current u16 to u32,
> > >> to get advantage of higher precision luts.  
> > >
> > > My argument though is to use a floating-point format for the LUT
> > > samples instead of adding more and more integer bits. That naturally
> > > puts more precision where it is needed: near zero.
> > >
> > > A driver can easily convert that to any format the hardware needs.
> > >
> > > However, it might make best sense for a driver to expose a LUT with a
> > > format that best matches the hardware precision, especially
> > > floating-point vs. integer.
> > >
> > > I guess we may eventually need both 32 bpc integer and 16 (or 32) bpc
> > > floating-point.
> > >  
> > 
> > While I like floating point better for representing these things I don't think it's a
> > great idea to pass floating point values via IOCTLs but 32 bpc integer values make
> > sense here.
> >   
> 
> Nice, we all are on same page here.

Fine by me!

Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]