[PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[Alex Hung]

LUT1D_INTERPOLATION and LUT3D_INTERPOLATION are read-only properties but
were missing DRM_MODE_PROP_IMMUTABLE and DRM_MODE_PROP_ATOMIC flags.

Add the correct flags and remove the now-unreachable set_property
handlers in drm_atomic_colorop_set_property().

Link: https://lore.kernel.org/amd-gfx/dbb27ec4-cdc6-4ead-9daf-664d97e86cd0@intel.com/
Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Suggested-by: Melissa Wen <mwen@igalia.com>
Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Alex Hung <alex.hung@amd.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
 drivers/gpu/drm/drm_colorop.c     | 6 ++++--
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 713fa9e81732..e831894d5a51 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -758,8 +758,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 			state->bypass = val;
 			*replaced = true;
 		}
-	} else if (property == colorop->lut1d_interpolation_property) {
-		colorop->lut1d_interpolation = val;
 	} else if (property == colorop->curve_1d_type_property) {
 		if (state->curve_1d_type != val) {
 			state->curve_1d_type = val;
@@ -770,8 +768,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 			state->multiplier = val;
 			*replaced = true;
 		}
-	} else if (property == colorop->lut3d_interpolation_property) {
-		colorop->lut3d_interpolation = val;
 	} else if (property == colorop->data_property) {
 		return drm_atomic_color_set_data_property(colorop, state,
 							  property, val,
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 398cc81ae588..34998f393f2e 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -317,7 +317,8 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
 	colorop->size = lut_size;
 
 	/* interpolation */
-	prop = drm_property_create_enum(dev, 0, "LUT1D_INTERPOLATION",
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
+					"LUT1D_INTERPOLATION",
 					drm_colorop_lut1d_interpolation_list,
 					ARRAY_SIZE(drm_colorop_lut1d_interpolation_list));
 	if (!prop)
@@ -413,7 +414,8 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
 	colorop->size = lut_size;
 
 	/* interpolation */
-	prop = drm_property_create_enum(dev, 0, "LUT3D_INTERPOLATION",
+	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
+					"LUT3D_INTERPOLATION",
 					drm_colorop_lut3d_interpolation_list,
 					ARRAY_SIZE(drm_colorop_lut3d_interpolation_list));
 	if (!prop)
-- 
2.43.0

Re: [PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[Melissa Wen]

On 26/03/2026 21:39, Alex Hung wrote:
> LUT1D_INTERPOLATION and LUT3D_INTERPOLATION are read-only properties but
> were missing DRM_MODE_PROP_IMMUTABLE and DRM_MODE_PROP_ATOMIC flags.
+ Xaver

So, I double checked previous conversation about these two interpolation 
properties.
AFAIU, we agreed (?) on not making it immutable because it could become 
mutable if a HW allows it.
- 
https://lore.kernel.org/dri-devel/CAFZQkGyj7=N2UcbBnjv7Az3oHSD2LXaAx5wzccpSt_UEhH3NZw@mail.gmail.com/

So, what's the best approach to inform userspace that they cannot change 
this property?
Perhaps the driver has to reject if in the end the property is immutable (?)

Melissa
>
> Add the correct flags and remove the now-unreachable set_property
> handlers in drm_atomic_colorop_set_property().
>
> Link: https://lore.kernel.org/amd-gfx/dbb27ec4-cdc6-4ead-9daf-664d97e86cd0@intel.com/
> Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
> Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
> Suggested-by: Melissa Wen <mwen@igalia.com>
> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Alex Hung <alex.hung@amd.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
>   drivers/gpu/drm/drm_colorop.c     | 6 ++++--
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 713fa9e81732..e831894d5a51 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -758,8 +758,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>   			state->bypass = val;
>   			*replaced = true;
>   		}
> -	} else if (property == colorop->lut1d_interpolation_property) {
> -		colorop->lut1d_interpolation = val;
>   	} else if (property == colorop->curve_1d_type_property) {
>   		if (state->curve_1d_type != val) {
>   			state->curve_1d_type = val;
> @@ -770,8 +768,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>   			state->multiplier = val;
>   			*replaced = true;
>   		}
> -	} else if (property == colorop->lut3d_interpolation_property) {
> -		colorop->lut3d_interpolation = val;
>   	} else if (property == colorop->data_property) {
>   		return drm_atomic_color_set_data_property(colorop, state,
>   							  property, val,
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 398cc81ae588..34998f393f2e 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -317,7 +317,8 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
>   	colorop->size = lut_size;
>   
>   	/* interpolation */
> -	prop = drm_property_create_enum(dev, 0, "LUT1D_INTERPOLATION",
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
> +					"LUT1D_INTERPOLATION",
>   					drm_colorop_lut1d_interpolation_list,
>   					ARRAY_SIZE(drm_colorop_lut1d_interpolation_list));
>   	if (!prop)
> @@ -413,7 +414,8 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
>   	colorop->size = lut_size;
>   
>   	/* interpolation */
> -	prop = drm_property_create_enum(dev, 0, "LUT3D_INTERPOLATION",
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
> +					"LUT3D_INTERPOLATION",
>   					drm_colorop_lut3d_interpolation_list,
>   					ARRAY_SIZE(drm_colorop_lut3d_interpolation_list));
>   	if (!prop)

Re: [PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[Pekka Paalanen]

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

On Mon, 30 Mar 2026 09:33:53 -0300
Melissa Wen <mwen@igalia.com> wrote:

> On 26/03/2026 21:39, Alex Hung wrote:
> > LUT1D_INTERPOLATION and LUT3D_INTERPOLATION are read-only properties but
> > were missing DRM_MODE_PROP_IMMUTABLE and DRM_MODE_PROP_ATOMIC flags.  
> + Xaver
> 
> So, I double checked previous conversation about these two interpolation 
> properties.
> AFAIU, we agreed (?) on not making it immutable because it could become 
> mutable if a HW allows it.
> - 
> https://lore.kernel.org/dri-devel/CAFZQkGyj7=N2UcbBnjv7Az3oHSD2LXaAx5wzccpSt_UEhH3NZw@mail.gmail.com/

Simon's rationale there makes sense to me.

> So, what's the best approach to inform userspace that they cannot change 
> this property?
> Perhaps the driver has to reject if in the end the property is immutable (?)

If the enum is exposed with a single value only, then obviously
userspace cannot program any other value.

Each driver needs to customize every enum for what they actually
support, anyway.


Thanks,
pq

> 
> Melissa
> >
> > Add the correct flags and remove the now-unreachable set_property
> > handlers in drm_atomic_colorop_set_property().
> >
> > Link: https://lore.kernel.org/amd-gfx/dbb27ec4-cdc6-4ead-9daf-664d97e86cd0@intel.com/
> > Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
> > Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
> > Suggested-by: Melissa Wen <mwen@igalia.com>
> > Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> > Signed-off-by: Alex Hung <alex.hung@amd.com>
> > ---
> >   drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
> >   drivers/gpu/drm/drm_colorop.c     | 6 ++++--
> >   2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 713fa9e81732..e831894d5a51 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -758,8 +758,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
> >   			state->bypass = val;
> >   			*replaced = true;
> >   		}
> > -	} else if (property == colorop->lut1d_interpolation_property) {
> > -		colorop->lut1d_interpolation = val;
> >   	} else if (property == colorop->curve_1d_type_property) {
> >   		if (state->curve_1d_type != val) {
> >   			state->curve_1d_type = val;
> > @@ -770,8 +768,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
> >   			state->multiplier = val;
> >   			*replaced = true;
> >   		}
> > -	} else if (property == colorop->lut3d_interpolation_property) {
> > -		colorop->lut3d_interpolation = val;
> >   	} else if (property == colorop->data_property) {
> >   		return drm_atomic_color_set_data_property(colorop, state,
> >   							  property, val,
> > diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> > index 398cc81ae588..34998f393f2e 100644
> > --- a/drivers/gpu/drm/drm_colorop.c
> > +++ b/drivers/gpu/drm/drm_colorop.c
> > @@ -317,7 +317,8 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
> >   	colorop->size = lut_size;
> >   
> >   	/* interpolation */
> > -	prop = drm_property_create_enum(dev, 0, "LUT1D_INTERPOLATION",
> > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
> > +					"LUT1D_INTERPOLATION",
> >   					drm_colorop_lut1d_interpolation_list,
> >   					ARRAY_SIZE(drm_colorop_lut1d_interpolation_list));
> >   	if (!prop)
> > @@ -413,7 +414,8 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
> >   	colorop->size = lut_size;
> >   
> >   	/* interpolation */
> > -	prop = drm_property_create_enum(dev, 0, "LUT3D_INTERPOLATION",
> > +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
> > +					"LUT3D_INTERPOLATION",
> >   					drm_colorop_lut3d_interpolation_list,
> >   					ARRAY_SIZE(drm_colorop_lut3d_interpolation_list));
> >   	if (!prop)  
> 


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

Re: [PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[Alex Hung]

On 3/31/26 06:19, Pekka Paalanen wrote:
> On Mon, 30 Mar 2026 09:33:53 -0300
> Melissa Wen <mwen@igalia.com> wrote:
> 
>> On 26/03/2026 21:39, Alex Hung wrote:
>>> LUT1D_INTERPOLATION and LUT3D_INTERPOLATION are read-only properties but
>>> were missing DRM_MODE_PROP_IMMUTABLE and DRM_MODE_PROP_ATOMIC flags.
>> + Xaver
>>
>> So, I double checked previous conversation about these two interpolation
>> properties.
>> AFAIU, we agreed (?) on not making it immutable because it could become
>> mutable if a HW allows it.
>> -
>> https://lore.kernel.org/dri-devel/CAFZQkGyj7=N2UcbBnjv7Az3oHSD2LXaAx5wzccpSt_UEhH3NZw@mail.gmail.com/
> 
> Simon's rationale there makes sense to me.

Yes this makes sense and I forgot that we discussed it before. Thanks 
Melissa and pekka

> 
>> So, what's the best approach to inform userspace that they cannot change
>> this property?
>> Perhaps the driver has to reject if in the end the property is immutable (?)
> 
> If the enum is exposed with a single value only, then obviously
> userspace cannot program any other value.
> 
> Each driver needs to customize every enum for what they actually
> support, anyway.

As Melissa pointed out in the other thread, the doc said these 
properties are "Read-only" when they are not. Should we update the 
documents, such as below, to avoid confusion?

diff --git include/drm/drm_colorop.h include/drm/drm_colorop.h
index a3a32f9f918c..86d75babe30d 100644
--- include/drm/drm_colorop.h
+++ include/drm/drm_colorop.h
@@ -312,7 +312,7 @@ struct drm_colorop {
  	/**
  	 * @lut1d_interpolation_property:
  	 *
-	 * Read-only property for DRM_COLOROP_1D_LUT interpolation
+	 * Property for DRM_COLOROP_1D_LUT interpolation
  	 */
  	struct drm_property *lut1d_interpolation_property;

@@ -340,7 +340,7 @@ struct drm_colorop {
  	/**
  	 * @lut3d_interpolation_property:
  	 *
-	 * Read-only property for DRM_COLOROP_3D_LUT interpolation
+	 * Property for DRM_COLOROP_3D_LUT interpolation
  	 */
  	struct drm_property *lut3d_interpolation_property;


> 
> 
> Thanks,
> pq
> 
>>
>> Melissa
>>>
>>> Add the correct flags and remove the now-unreachable set_property
>>> handlers in drm_atomic_colorop_set_property().
>>>
>>> Link: https://lore.kernel.org/amd-gfx/dbb27ec4-cdc6-4ead-9daf-664d97e86cd0@intel.com/
>>> Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
>>> Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
>>> Suggested-by: Melissa Wen <mwen@igalia.com>
>>> Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
>>>    drivers/gpu/drm/drm_colorop.c     | 6 ++++--
>>>    2 files changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>> index 713fa9e81732..e831894d5a51 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -758,8 +758,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>>>    			state->bypass = val;
>>>    			*replaced = true;
>>>    		}
>>> -	} else if (property == colorop->lut1d_interpolation_property) {
>>> -		colorop->lut1d_interpolation = val;
>>>    	} else if (property == colorop->curve_1d_type_property) {
>>>    		if (state->curve_1d_type != val) {
>>>    			state->curve_1d_type = val;
>>> @@ -770,8 +768,6 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>>>    			state->multiplier = val;
>>>    			*replaced = true;
>>>    		}
>>> -	} else if (property == colorop->lut3d_interpolation_property) {
>>> -		colorop->lut3d_interpolation = val;
>>>    	} else if (property == colorop->data_property) {
>>>    		return drm_atomic_color_set_data_property(colorop, state,
>>>    							  property, val,
>>> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
>>> index 398cc81ae588..34998f393f2e 100644
>>> --- a/drivers/gpu/drm/drm_colorop.c
>>> +++ b/drivers/gpu/drm/drm_colorop.c
>>> @@ -317,7 +317,8 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
>>>    	colorop->size = lut_size;
>>>    
>>>    	/* interpolation */
>>> -	prop = drm_property_create_enum(dev, 0, "LUT1D_INTERPOLATION",
>>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
>>> +					"LUT1D_INTERPOLATION",
>>>    					drm_colorop_lut1d_interpolation_list,
>>>    					ARRAY_SIZE(drm_colorop_lut1d_interpolation_list));
>>>    	if (!prop)
>>> @@ -413,7 +414,8 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
>>>    	colorop->size = lut_size;
>>>    
>>>    	/* interpolation */
>>> -	prop = drm_property_create_enum(dev, 0, "LUT3D_INTERPOLATION",
>>> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC | DRM_MODE_PROP_IMMUTABLE,
>>> +					"LUT3D_INTERPOLATION",
>>>    					drm_colorop_lut3d_interpolation_list,
>>>    					ARRAY_SIZE(drm_colorop_lut3d_interpolation_list));
>>>    	if (!prop)
>>
> 

Re: [PATCH] drm/colorop: Mark interpolation properties as immutable and atomic

[Pekka Paalanen]

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

On Wed, 1 Apr 2026 18:53:56 -0600
Alex Hung <alex.hung@amd.com> wrote:

> On 3/31/26 06:19, Pekka Paalanen wrote:
> > On Mon, 30 Mar 2026 09:33:53 -0300
> > Melissa Wen <mwen@igalia.com> wrote:
> >   
> >> On 26/03/2026 21:39, Alex Hung wrote:  
> >>> LUT1D_INTERPOLATION and LUT3D_INTERPOLATION are read-only properties but
> >>> were missing DRM_MODE_PROP_IMMUTABLE and DRM_MODE_PROP_ATOMIC flags.  
> >> + Xaver
> >>
> >> So, I double checked previous conversation about these two interpolation
> >> properties.
> >> AFAIU, we agreed (?) on not making it immutable because it could become
> >> mutable if a HW allows it.
> >> -
> >> https://lore.kernel.org/dri-devel/CAFZQkGyj7=N2UcbBnjv7Az3oHSD2LXaAx5wzccpSt_UEhH3NZw@mail.gmail.com/  
> > 
> > Simon's rationale there makes sense to me.  
> 
> Yes this makes sense and I forgot that we discussed it before. Thanks 
> Melissa and pekka
> 
> >   
> >> So, what's the best approach to inform userspace that they cannot change
> >> this property?
> >> Perhaps the driver has to reject if in the end the property is immutable (?)  
> > 
> > If the enum is exposed with a single value only, then obviously
> > userspace cannot program any other value.
> > 
> > Each driver needs to customize every enum for what they actually
> > support, anyway.  
> 
> As Melissa pointed out in the other thread, the doc said these 
> properties are "Read-only" when they are not. Should we update the 
> documents, such as below, to avoid confusion?

Yes, please!


Thanks,
pq


> diff --git include/drm/drm_colorop.h include/drm/drm_colorop.h
> index a3a32f9f918c..86d75babe30d 100644
> --- include/drm/drm_colorop.h
> +++ include/drm/drm_colorop.h
> @@ -312,7 +312,7 @@ struct drm_colorop {
>   	/**
>   	 * @lut1d_interpolation_property:
>   	 *
> -	 * Read-only property for DRM_COLOROP_1D_LUT interpolation
> +	 * Property for DRM_COLOROP_1D_LUT interpolation
>   	 */
>   	struct drm_property *lut1d_interpolation_property;
> 
> @@ -340,7 +340,7 @@ struct drm_colorop {
>   	/**
>   	 * @lut3d_interpolation_property:
>   	 *
> -	 * Read-only property for DRM_COLOROP_3D_LUT interpolation
> +	 * Property for DRM_COLOROP_3D_LUT interpolation
>   	 */
>   	struct drm_property *lut3d_interpolation_property;
> 

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