Re: [PATCH V8 40/43] drm/colorop: Add 3D LUT support to color pipeline

[Alex Hung <alex.hung@amd.com>]

On 4/25/25 07:50, Leandro Ribeiro wrote:
> 
> 
> On 3/26/25 20:47, Alex Hung wrote:
>> It is to be used to enable HDR by allowing userpace to create and pass
>> 3D LUTs to kernel and hardware.
>>
>> new drm_colorop_type: DRM_COLOROP_3D_LUT.
>>
>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>> ---
>> v8:
>>   - Fix typo in subject (Simon Ser)
>>   - Update documentation for DRM_COLOROP_3D_LUT (Simon Ser)
>>   - Delete empty lines (Simon Ser)
>>
>> v7:
>>   - Simplify 3D LUT by removing lut_3d_modes and related functions (Simon Ser)
>>
>>   drivers/gpu/drm/drm_atomic.c      |  6 +++
>>   drivers/gpu/drm/drm_atomic_uapi.c |  6 +++
>>   drivers/gpu/drm/drm_colorop.c     | 72 +++++++++++++++++++++++++++++++
>>   include/drm/drm_colorop.h         | 21 +++++++++
>>   include/uapi/drm/drm_mode.h       | 33 ++++++++++++++
>>   5 files changed, 138 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 0efb0ead204a..ef47a06344f3 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -806,6 +806,12 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>>   	case DRM_COLOROP_MULTIPLIER:
>>   		drm_printf(p, "\tmultiplier=%llu\n", state->multiplier);
>>   		break;
>> +	case DRM_COLOROP_3D_LUT:
>> +		drm_printf(p, "\tsize=%d\n", colorop->lut_size);
>> +		drm_printf(p, "\tinterpolation=%s\n",
>> +			   drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
>> +		drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
>> +		break;
>>   	default:
>>   		break;
>>   	}
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 947c18e8bf9b..d5d464b4d0f6 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -719,6 +719,10 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>>   	case DRM_COLOROP_CTM_3X4:
>>   		size = sizeof(struct drm_color_ctm_3x4);
>>   		break;
>> +	case DRM_COLOROP_3D_LUT:
>> +		size = colorop->lut_size * colorop->lut_size * colorop->lut_size *
>> +		       sizeof(struct drm_color_lut);
>> +		break;
>>   	default:
>>   		/* should never get here */
>>   		return -EINVAL;
>> @@ -771,6 +775,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
>>   		*val = state->multiplier;
>>   	} else if (property == colorop->lut_size_property) {
>>   		*val = colorop->lut_size;
>> +	} else if (property == colorop->lut3d_interpolation_property) {
>> +		*val = colorop->lut3d_interpolation;
>>   	} 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 e03706e7179b..224c6be237d2 100644
>> --- a/drivers/gpu/drm/drm_colorop.c
>> +++ b/drivers/gpu/drm/drm_colorop.c
>> @@ -67,6 +67,7 @@ static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
>>   	{ DRM_COLOROP_1D_LUT, "1D LUT" },
>>   	{ DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
>>   	{ DRM_COLOROP_MULTIPLIER, "Multiplier"},
>> +	{ DRM_COLOROP_3D_LUT, "3D LUT"},
>>   };
>>   
>>   static const char * const colorop_curve_1d_type_names[] = {
>> @@ -82,6 +83,11 @@ static const struct drm_prop_enum_list drm_colorop_lut1d_interpolation_list[] =
>>   	{ DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR, "Linear" },
>>   };
>>   
>> +
>> +static const struct drm_prop_enum_list drm_colorop_lut3d_interpolation_list[] = {
>> +	{ DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL, "Tetrahedral" },
>> +};
>> +
>>   /* Init Helpers */
>>   
>>   static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
>> @@ -349,6 +355,51 @@ int drm_colorop_mult_init(struct drm_device *dev, struct drm_colorop *colorop,
>>   }
>>   EXPORT_SYMBOL(drm_colorop_mult_init);
>>   
>> +int drm_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *colorop,
>> +			   struct drm_plane *plane,
>> +			   uint32_t lut_size,
>> +			   enum drm_colorop_lut3d_interpolation_type interpolation,
>> +			   bool allow_bypass)
>> +{
>> +	struct drm_property *prop;
>> +	int ret;
>> +
>> +	ret = drm_colorop_init(dev, colorop, plane, DRM_COLOROP_3D_LUT, allow_bypass);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* 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;
>> +
>> +	/* interpolation */
>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "LUT3D_INTERPOLATION",
>> +					drm_colorop_lut3d_interpolation_list,
>> +					ARRAY_SIZE(drm_colorop_lut3d_interpolation_list));
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	colorop->lut3d_interpolation_property = prop;
>> +	drm_object_attach_property(&colorop->base, prop, interpolation);
>> +	colorop->lut3d_interpolation = interpolation;
>> +
>> +	/* data */
>> +	ret = drm_colorop_create_data_prop(dev, colorop);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_colorop_reset(colorop);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_colorop_3dlut_init);
>> +
>>   static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop,
>>   							struct drm_colorop_state *state)
>>   {
>> @@ -441,7 +492,13 @@ static const char * const colorop_type_name[] = {
>>   	[DRM_COLOROP_1D_LUT] = "1D LUT",
>>   	[DRM_COLOROP_CTM_3X4] = "3x4 Matrix",
>>   	[DRM_COLOROP_MULTIPLIER] = "Multiplier",
>> +	[DRM_COLOROP_3D_LUT] = "3D LUT",
>>   };
>> +
>> +static const char * const colorop_lu3d_interpolation_name[] = {
>> +	[DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL] = "Tetrahedral",
>> +};
>> +
>>   static const char * const colorop_lut1d_interpolation_name[] = {
>>   	[DRM_COLOROP_LUT1D_INTERPOLATION_LINEAR] = "Linear",
>>   };
>> @@ -477,6 +534,21 @@ const char *drm_get_colorop_lut1d_interpolation_name(enum drm_colorop_lut1d_inte
>>   	return colorop_lut1d_interpolation_name[type];
>>   }
>>   
>> +/**
>> + * drm_get_colorop_lut3d_interpolation_name - return a string for interpolation type
>> + * @type: interpolation type to compute name of
>> + *
>> + * In contrast to the other drm_get_*_name functions this one here returns a
>> + * const pointer and hence is threadsafe.
>> + */
>> +const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type)
>> +{
>> +	if (WARN_ON(type >= ARRAY_SIZE(colorop_lu3d_interpolation_name)))
>> +		return "unknown";
>> +
>> +	return colorop_lu3d_interpolation_name[type];
>> +}
>> +
>>   /**
>>    * drm_colorop_set_next_property - sets the next pointer
>>    * @colorop: drm colorop
>> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
>> index c89d5eb44856..e999d5ceb8a5 100644
>> --- a/include/drm/drm_colorop.h
>> +++ b/include/drm/drm_colorop.h
>> @@ -281,6 +281,14 @@ struct drm_colorop {
>>   	 */
>>   	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
>>   
>> +	/**
>> +	 * @lut3d_interpolation:
>> +	 *
>> +	 * Read-only
>> +	 * Interpolation for DRM_COLOROP_3D_LUT
>> +	 */
>> +	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
>> +
>>   	/**
>>   	 * @lut1d_interpolation_property:
>>   	 *
>> @@ -309,6 +317,13 @@ struct drm_colorop {
>>   	 */
>>   	struct drm_property *lut_size_property;
>>   
>> +	/**
>> +	 * @lut3d_interpolation_property:
>> +	 *
>> +	 * Read-only property for DRM_COLOROP_3D_LUT interpolation
>> +	 */
>> +	struct drm_property *lut3d_interpolation_property;
>> +
>>   	/**
>>   	 * @data_property:
>>   	 *
>> @@ -362,6 +377,11 @@ int drm_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop *colorop
>>   			     struct drm_plane *plane, bool allow_bypass);
>>   int drm_colorop_mult_init(struct drm_device *dev, struct drm_colorop *colorop,
>>   			      struct drm_plane *plane, bool allow_bypass);
>> +int drm_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *colorop,
>> +			   struct drm_plane *plane,
>> +			   uint32_t lut_size,
>> +			   enum drm_colorop_lut3d_interpolation_type interpolation,
>> +			   bool allow_bypass);
>>   
>>   struct drm_colorop_state *
>>   drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop);
>> @@ -412,6 +432,7 @@ const char *drm_get_colorop_type_name(enum drm_colorop_type type);
>>    */
>>   const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type type);
>>   const char *drm_get_colorop_lut1d_interpolation_name(enum drm_colorop_lut1d_interpolation_type type);
>> +const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type);
>>   
>>   void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
>>   
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index d76c8ffe5408..88fafbdeb2a2 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -930,6 +930,39 @@ enum drm_colorop_type {
>>   	 * property.
>>   	 */
>>   	DRM_COLOROP_MULTIPLIER,
>> +
>> +	/**
>> +	 * @DRM_COLOROP_3D_LUT:
>> +	 *
>> +	 * enum string "3D LUT"
>> +	 *
>> +	 * A 3D LUT of &drm_color_lut entries,
>> +	 * packed into a blob via the DATA property. The driver's expected
>> +	 * LUT size is advertised via the SIZE property, i.e., a 3D LUT with
>> +	 * 17x17x17 entries will have SIZE set to 17.
>> +	 *
>> +	 * The DATA blob is a 3D array of struct drm_color_lut with dimension
>> +	 * length of "lut_size".
>> +	 * The LUT elements are traversed like so:
>> +	 *
>> +	 *   for R in range 0..n
>> +	 *     for G in range 0..n
>> +	 *       for B in range 0..n
>> +	 *         color = lut3d[R][G][B]
>> +	 */
>> +	DRM_COLOROP_3D_LUT,
>> +};
> 
> Hi,
> 
> I'm experimenting with V7 of the this API on Weston, using the AMD driver,
> and I'm seeing issues with the usage of 3D LUT's: channels R and B being
> swapped.
> On Weston, the 3D LUT is constructed as:
> 
> for B in range 0..n
>      for G in range 0..n
>         for R in range 0..n
>             index = R + n * (G + n * B)
>             lut[index].red   = foo
>             lut[index].green = foo
>             lut[index].blue  = foo
> 
> To map that to DRM_COLOROP_3D_LUT, we do:
> 
> for B in range 0..n
>      for G in range 0..n
>         for R in range 0..n
>             index_weston = R + n * (G + n * B)
>             index_kernel = B + n * (G + n * R)
>             lut_kernel[index_kernel].red   = lut[index_weston].red
>             lut_kernel[index_kernel].green = lut[index_weston].green
>             lut_kernel[index_kernel].blue  = lut[index_weston].blue
> 
> If I ignore the documentation and use the same indices, everything works
> fine regarding the color channels.
> 

I think you are right here. The comment above should state index_kernel 
= R + n * (G + n * B). This also seems to be how corresponding IGT test is.

> Maybe there's a bug in our Weston code, but writing this just to confirm
> that the documentation and the AMD driver are matching.
> 
> Thanks,
> Leandro
> 
>> +
>> +/**
>> + * enum drm_colorop_lut3d_interpolation_type - type of 3DLUT interpolation
>> + */
>> +enum drm_colorop_lut3d_interpolation_type {
>> +	/**
>> +	 * @DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL:
>> +	 *
>> +	 * Tetrahedral 3DLUT interpolation
>> +	 */
>> +	DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL,
>>   };
>>   
>>   /**
>