[V7 33/45] drm/colorop: Add 1D Curve Custom LUT type

[Simon Ser]

On Friday, January 17th, 2025 at 00:33, Alex Hung <alex.hung at amd.com> wrote:

> On 1/15/25 01:14, Simon Ser wrote:
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index a3e1fcad47ad..4744c12e429d 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -701,6 +701,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);
> >
> > Should we set the element size and the number of elements instead of
> > multiplying? Or is that only useful when either of these are controlled by
> > user-space to avoid integer overflows?
> 
> This multiplication here is to calculate the total size for the data blob.
> 
> The user-space communicates the lut_size (which is read-only) without
> multiplying sizeof(drm_color_lut) in drm_atomic_colorop_get_property, i.e.,
> 
> +	} else if (property == colorop->lut_size_property) {
> +		*val = colorop->lut_size;
> 
> Is this what you meant?

I mean that drm_property_replace_blob_from_id() takes two parameters:
expected_size and expected_elem_size.

But it seems expected_elem_size is just used for checking whether the size of
the blob set by user-space is divisible by the element size, and nothing more.
In particular, drm_property_replace_blob_from_id() doesn't internally multiply
expected_size and expected_elem_size to check the total size when both of these
parameters are provided. In other words, it's useless to provide both
expected_size and expected_elem_size.

tl;dr my comment can be ignored.

> >> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> >> index 665b23900cc0..e6dea2713490 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 Curve Custom LUT" },
> >
> > Since we now have both a "normal" 1D curve, and a "special" one… Would it make
> > sense to change our minds regarding the naming of the former? For instance, we
> > could rename it to DRM_COLOROP_FIXED_1D_CURVE. Or is the current name clear
> > enough (and only the human-readable name can be switched to "1D Fixed Curve")?
> 
> How about keeping "1D Curve" and simplifying "1D Curve Custom LUT" to
> "1D LUT" such as the following?

Yeah, that sounds good to me!