Re: [PATCH V11 35/47] drm/colorop: Add 1D Curve Custom LUT type

["Sebastian Wick" <sebastian.wick@redhat.com>]

On Wed Aug 20, 2025 at 8:16 PM CEST, Alex Hung wrote:
>
>
> On 8/19/25 09:31, Sebastian Wick wrote:
>>> +/**
>>> + * drm_plane_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_plane_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_plane_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;
>> I'm a bit confused here. The property itself is just called "SIZE" which
>> looks very similar to the generic "DATA" property. However, it is
>> assigned to `lut_size_property`.
>> 
>> Is this meant to be to be a generic property where the exact usage
>> depends on the type of the color op (like "DATA"), or is this meant to
>> be specific to LUTs (in which case the generic name is misleading)?
>> 
>> I also tried to find the user space documentation for all the properties
>> but could not find them. The only thing I could find was the kernel
>> documentation of
>> 
>>      struct drm_property *lut_size_property;
>> 
>> Which says "Size property for custom LUT from userspace."
>
> In earlier version, this lut_size was specific to 1D LUT and 3D LUT has 
> a dedicate lut size field in another struct, and lut_size_property is 
> now used for both 1D and 3D LUTs.
>
> Do you mean we clarify by making either changes?
> 1) "SIZE" to "LUT_SIZE"
>      -> specific to LUTs
> 2) "lut_size" and "lut_size_property" "size" and "size_property", 
> respectively
>     -> Can be reused in the future, if any.

Yes! I would prefer "SIZE" as I can see other color op types which use
the "DATA" prop to require this as well.

The documentation should also be adjusted to say that the meaning of the
"SIZE" property depends on the color op type (same as with "DATA").