| Message ID | 20240226211100.100108-11-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Color Pipeline API w/ VKMS | expand |
On Mon, 26 Feb 2024 16:10:24 -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. > > v4: > - Use enum property for TYPE (Pekka) > > 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 | 44 ++++++++++++++++++++++++++++++- > include/drm/drm_colorop.h | 17 +++++++++++- > include/uapi/drm/drm_mode.h | 4 +++ > 5 files changed, 72 insertions(+), 5 deletions(-) ... > diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c > index a295ab96aee1..3a07a8fe2842 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,29 @@ 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_enum(dev, > + DRM_MODE_PROP_IMMUTABLE, > + "TYPE", drm_colorop_type_enum_list, > + ARRAY_SIZE(drm_colorop_type_enum_list)); This list here is the list of all values the enum could take, right? Should it not be just the one value it's going to have? It'll never change, and it can never be changed. > + > + if (!prop) > + return -ENOMEM; > + > + colorop->type_property = prop; > + > + drm_object_attach_property(&colorop->base, > + colorop->type_property, > + colorop->type); > + > return ret; > } > EXPORT_SYMBOL(drm_colorop_init); Thanks, pq
On Tuesday, March 12th, 2024 at 16:02, Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > This list here is the list of all values the enum could take, right? > Should it not be just the one value it's going to have? It'll never > change, and it can never be changed. Listing all possible values is how other properties behave. Example: "type" (immutable): enum {Overlay, Primary, Cursor} = Primary
On Tue, 12 Mar 2024 15:15:13 +0000 Simon Ser <contact@emersion.fr> wrote: > On Tuesday, March 12th, 2024 at 16:02, Pekka Paalanen <pekka.paalanen@collabora.com> wrote: > > > This list here is the list of all values the enum could take, right? > > Should it not be just the one value it's going to have? It'll never > > change, and it can never be changed. > > Listing all possible values is how other properties behave. Example: > > "type" (immutable): enum {Overlay, Primary, Cursor} = Primary Yeah, that's weird, now that you pointed it out. Userspace does nothing with the knowledge of what this specific object's property "could" be since it's immutable. Only the kernel internals and UAPI docs need the full list, and userspace needs to hardcode the full list in general so it can recognize each value. But on the property instance on a specific object, I see no use for the full list. Thanks, pq
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 62e87e6a9653..b400e32c9d39 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -634,8 +634,8 @@ drm_atomic_get_colorop_state(struct drm_atomic_state *state, state->colorops[index].new_state = colorop_state; colorop_state->state = state; - drm_dbg_atomic(colorop->dev, "Added [COLOROP:%d] %p state to %p\n", - colorop->base.id, colorop_state, state); + drm_dbg_atomic(colorop->dev, "Added [COLOROP:%d:%d] %p state to %p\n", + colorop->base.id, colorop->type, colorop_state, state); return colorop_state; } diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 1f9b6dfa8ca7..e3067c095c72 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -660,7 +660,13 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop, const struct drm_colorop_state *state, struct drm_property *property, uint64_t *val) { - return -EINVAL; + if (property == colorop->type_property) { + *val = colorop->type; + } else { + return -EINVAL; + } + + return 0; } 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..3a07a8fe2842 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,29 @@ 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_enum(dev, + DRM_MODE_PROP_IMMUTABLE, + "TYPE", drm_colorop_type_enum_list, + ARRAY_SIZE(drm_colorop_type_enum_list)); + + if (!prop) + return -ENOMEM; + + colorop->type_property = prop; + + drm_object_attach_property(&colorop->base, + colorop->type_property, + colorop->type); + return ret; } EXPORT_SYMBOL(drm_colorop_init); @@ -150,3 +172,23 @@ void drm_colorop_reset(struct drm_colorop *colorop) __drm_colorop_reset(colorop, colorop->state); } EXPORT_SYMBOL(drm_colorop_reset); + + +static const char * const colorop_type_name[] = { + [DRM_COLOROP_1D_CURVE] = "1D Curve", +}; + +/** + * drm_get_colorop_type_name - return a string for colorop type + * @type: colorop 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_type_name(enum drm_colorop_type type) +{ + if (WARN_ON(type >= ARRAY_SIZE(colorop_type_name))) + return "unknown"; + + return colorop_type_name[type]; +} diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h index e611f830f986..cb98c55f8387 100644 --- a/include/drm/drm_colorop.h +++ b/include/drm/drm_colorop.h @@ -103,6 +103,21 @@ struct drm_colorop { /** @properties: property tracking for this plane */ struct drm_object_properties properties; + /** + * @type: + * + * Read-only + * Type of color operation + */ + enum drm_colorop_type type; + + /** + * @type_property: + * + * Read-only "TYPE" property for specifying the type of + * this color operation. The type is enum drm_colorop_type. + */ + struct drm_property *type_property; }; #define obj_to_colorop(x) container_of(x, struct drm_colorop, base) @@ -127,7 +142,7 @@ static inline struct drm_colorop *drm_colorop_find(struct drm_device *dev, } 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_colorop_state * drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop); diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 9e8f383935db..0cbd6bef52bc 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -866,6 +866,10 @@ struct drm_color_lut { __u16 reserved; }; +enum drm_colorop_type { + DRM_COLOROP_1D_CURVE +}; + /** * struct hdr_metadata_infoframe - HDR Metadata Infoframe Data. *
Add a read-only TYPE property. The TYPE specifies the colorop type, such as enumerated curve, 1D LUT, CTM, 3D LUT, PWL LUT, etc. v4: - Use enum property for TYPE (Pekka) 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 | 44 ++++++++++++++++++++++++++++++- include/drm/drm_colorop.h | 17 +++++++++++- include/uapi/drm/drm_mode.h | 4 +++ 5 files changed, 72 insertions(+), 5 deletions(-)