Re: [RFC PATCH v3 08/23] drm/colorop: Add TYPE property

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Wed, 8 Nov 2023 11:36:27 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> Add a read-only TYPE property. The TYPE specifies the colorop
> type, such as enumerated curve, 1D LUT, CTM, 3D LUT, PWL LUT,
> etc.
> 
> v3:
>  - Make TYPE a range property
>  - Move enum drm_colorop_type to uapi header
>  - Fix drm_get_colorop_type_name description
> 
> For now we're only introducing an enumerated 1D LUT type to
> illustrate the concept.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic.c      |  4 +--
>  drivers/gpu/drm/drm_atomic_uapi.c |  8 +++++-
>  drivers/gpu/drm/drm_colorop.c     | 43 ++++++++++++++++++++++++++++++-
>  include/drm/drm_colorop.h         | 17 +++++++++++-
>  include/uapi/drm/drm_mode.h       |  4 +++
>  5 files changed, 71 insertions(+), 5 deletions(-)
> 

...

>  static int drm_atomic_set_writeback_fb_for_connector(
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index a295ab96aee1..902b13e300fd 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -32,12 +32,17 @@
>  
>  /* TODO big colorop doc, including properties, etc. */
>  
> +static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
> +	{ DRM_COLOROP_1D_CURVE, "1D Curve" },
> +};
> +
>  /* Init Helpers */
>  
>  int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
> -		     struct drm_plane *plane)
> +		     struct drm_plane *plane, enum drm_colorop_type type)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_property *prop;
>  	int ret = 0;
>  
>  	ret = drm_mode_object_add(dev, &colorop->base, DRM_MODE_OBJECT_COLOROP);
> @@ -46,12 +51,28 @@ int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
>  
>  	colorop->base.properties = &colorop->properties;
>  	colorop->dev = dev;
> +	colorop->type = type;
>  	colorop->plane = plane;
>  
>  	list_add_tail(&colorop->head, &config->colorop_list);
>  	colorop->index = config->num_colorop++;
>  
>  	/* add properties */
> +
> +	/* type */
> +	prop = drm_property_create_range(dev,
> +					DRM_MODE_PROP_IMMUTABLE,
> +					"TYPE", type, type);

Why was this made a range property?

My review comment for the doc patch is wrong, the doc should mention
enum drm_colorop_type.

> +
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	colorop->type_property = prop;
> +
> +	drm_object_attach_property(&colorop->base,
> +				   colorop->type_property,
> +				   colorop->type);
> +
>  	return ret;
>  }

...

> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 4e3251ff894a..0ebf0f480dc8 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -858,6 +858,10 @@ struct drm_color_lut {
>  	__u16 reserved;
>  };
>  
> +enum drm_colorop_type {
> +	DRM_COLOROP_1D_CURVE
> +};


Thanks,
pq

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