| Message ID | 20250326234748.2982010-7-alex.hung@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Color Pipeline API w/ VKMS | expand |
Hi Alex, On Wed, 26 Mar 2025 at 23:50, Alex Hung <alex.hung@amd.com> wrote: > +static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop, > + 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); > + if (ret) > + return ret; > + > + colorop->base.properties = &colorop->properties; > + colorop->dev = dev; > + colorop->type = type; > + colorop->plane = plane; 'plane' seems really incongruous here. The colorop can be created for any number of planes, but we're setting it to always be bound to a single plane at init, and that can only be changed later. I can see the sense in allowing different pipelines to move between different planes, but then it shouldn't be set at init time. I think you want to just drop the 'plane' argument here, and only set the plane during an atomic commit when actually switching the state. It would also be helpful for userspace to know how color pipelines could be used, and to back that up with igt. Questions I have whilst reading this: 1. Is it guaranteed that, if any plane on a device supports the COLOR_PIPELINE property, all planes will support COLOR_PIPELINE? (Given the amdgpu cursor-plane discussion, it looks like no, which is unfortunate but oh well.) 2. Is it guaranteed that, if a COLOR_PIPELINE property exists on a plane, that BYPASS will be one of the supported values? (The current implementation does this, which seems sensible, but if the plan is to not make this a uAPI invariant, e.g. to support planes with mandatory CM steps, this should probably be explicitly documented.) 3. Can a given color pipeline potentially be used on different planes, i.e. a colorop used to represent a separate hardware processing block which may be used on any plane but only one plane at a time? (This should be documented either way, and if they are unique per plane, igt should enforce this.) 3. Can a given color pipeline be active on multiple planes at a time? (If so, the implementation definitely needs rethinking: the colorop would need to have a list of planes.) 4. Can a given color pipeline be active on multiple planes on multiple CRTCs at a time? 5. For a given colorop property, is it an invariant that the colorop will only appear in one color pipeline at a time? (I believe so, but this probably needs documenting and/or igt.) Either way, I suspect that clorop->plane is the wrong thing to do, and that it maybe wants to be a list of planes in the drm_colorop_state? Cheers, Daniel
On Tuesday, April 1st, 2025 at 17:14, Daniel Stone <daniel@fooishbar.org> wrote: > > > Hi Alex, > > On Wed, 26 Mar 2025 at 23:50, Alex Hung alex.hung@amd.com wrote: > > > +static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop, > > + 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); > > + if (ret) > > + return ret; > > + > > + colorop->base.properties = &colorop->properties; > > + colorop->dev = dev; > > + colorop->type = type; > > + colorop->plane = plane; > > 'plane' seems really incongruous here. The colorop can be created for > any number of planes, but we're setting it to always be bound to a > single plane at init, and that can only be changed later. I don't think the current design allows a single colorop to be re-used between planes? I think as-is, drivers create one set of colorops per plane and never share them between different planes? > 1. Is it guaranteed that, if any plane on a device supports the > COLOR_PIPELINE property, all planes will support COLOR_PIPELINE? > (Given the amdgpu cursor-plane discussion, it looks like no, which is > unfortunate but oh well.) I don't think so. (They could all expose a COLOR_PIPELINE with the only choice as the zero bypass pipeline, but that sounds silly.) > 2. Is it guaranteed that, if a COLOR_PIPELINE property exists on a > plane, that BYPASS will be one of the supported values? (The current > implementation does this, which seems sensible, but if the plan is to > not make this a uAPI invariant, e.g. to support planes with mandatory > CM steps, this should probably be explicitly documented.) Yes. This is a hard requirement, mentioned in the design doc IIRC. > 3. Can a given color pipeline potentially be used on different planes, > i.e. a colorop used to represent a separate hardware processing block > which may be used on any plane but only one plane at a time? (This > should be documented either way, and if they are unique per plane, igt > should enforce this.) Right now, I don't think so. Could be a future extension I suppose, but I think we need to properly sit down and think about all of the possible consequences. Maybe using the same pipeline ID isn't the best uAPI here. > 3. Can a given color pipeline be active on multiple planes at a time? > (If so, the implementation definitely needs rethinking: the colorop > would need to have a list of planes.) I don't think so. > 4. Can a given color pipeline be active on multiple planes on multiple > CRTCs at a time? Ditto. > 5. For a given colorop property, is it an invariant that the colorop > will only appear in one color pipeline at a time? (I believe so, but > this probably needs documenting and/or igt.) I don't really understand why that would matter to user-space. > Either way, I suspect that clorop->plane is the wrong thing to do, and > that it maybe wants to be a list of planes in the drm_colorop_state? I don't think so, for a given plane, there can only be a single pipeline active at a time.
On 2025-04-01 15:53, Simon Ser wrote: > > > > > > On Tuesday, April 1st, 2025 at 17:14, Daniel Stone <daniel@fooishbar.org> wrote: > >> >> >> Hi Alex, >> >> On Wed, 26 Mar 2025 at 23:50, Alex Hung alex.hung@amd.com wrote: >> >>> +static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop, >>> + 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); >>> + if (ret) >>> + return ret; >>> + >>> + colorop->base.properties = &colorop->properties; >>> + colorop->dev = dev; >>> + colorop->type = type; >>> + colorop->plane = plane; >> >> 'plane' seems really incongruous here. The colorop can be created for >> any number of planes, but we're setting it to always be bound to a >> single plane at init, and that can only be changed later. > > I don't think the current design allows a single colorop to be re-used > between planes? I think as-is, drivers create one set of colorops per > plane and never share them between different planes? > Yeah, with the current design a colorop always belongs to a plane. In the future when we introduce crtc colorops they could be associated with a single crtc instead. >> 1. Is it guaranteed that, if any plane on a device supports the >> COLOR_PIPELINE property, all planes will support COLOR_PIPELINE? >> (Given the amdgpu cursor-plane discussion, it looks like no, which is >> unfortunate but oh well.) > > I don't think so. (They could all expose a COLOR_PIPELINE with the only > choice as the zero bypass pipeline, but that sounds silly.) > Correct. >> 2. Is it guaranteed that, if a COLOR_PIPELINE property exists on a >> plane, that BYPASS will be one of the supported values? (The current >> implementation does this, which seems sensible, but if the plan is to >> not make this a uAPI invariant, e.g. to support planes with mandatory >> CM steps, this should probably be explicitly documented.) > > Yes. This is a hard requirement, mentioned in the design doc IIRC. > If this wasn't the case then those pipes would be doing undefined things with current implementations. >> 3. Can a given color pipeline potentially be used on different planes, >> i.e. a colorop used to represent a separate hardware processing block >> which may be used on any plane but only one plane at a time? (This >> should be documented either way, and if they are unique per plane, igt >> should enforce this.) > > Right now, I don't think so. Could be a future extension I suppose, but > I think we need to properly sit down and think about all of the possible > consequences. Maybe using the same pipeline ID isn't the best uAPI here. > I think it'd be easier to tie a colorop to a single pipeline, which is tied to a single plane. I'd imagine HW is rarely designed to allow arbitrary routing of individual HW blocks. Muxes are costly. >> 3. Can a given color pipeline be active on multiple planes at a time? >> (If so, the implementation definitely needs rethinking: the colorop >> would need to have a list of planes.) > > I don't think so. > It's tied specifically to a single plane. Harry >> 4. Can a given color pipeline be active on multiple planes on multiple >> CRTCs at a time? > > Ditto. > >> 5. For a given colorop property, is it an invariant that the colorop >> will only appear in one color pipeline at a time? (I believe so, but >> this probably needs documenting and/or igt.) > > I don't really understand why that would matter to user-space. > >> Either way, I suspect that clorop->plane is the wrong thing to do, and >> that it maybe wants to be a list of planes in the drm_colorop_state? > > I don't think so, for a given plane, there can only be a single pipeline > active at a time.
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 5e8ce781fcd7..f75c987f8913 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -649,11 +649,17 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop, struct drm_colorop_state *state, struct drm_file *file_priv, struct drm_property *property, uint64_t val) { - drm_dbg_atomic(colorop->dev, - "[COLOROP:%d] unknown property [PROP:%d:%s]]\n", - colorop->base.id, - property->base.id, property->name); - return -EINVAL; + if (property == colorop->curve_1d_type_property) { + state->curve_1d_type = val; + } else { + drm_dbg_atomic(colorop->dev, + "[COLOROP:%d:%d] unknown property [PROP:%d:%s]]\n", + colorop->base.id, colorop->type, + property->base.id, property->name); + return -EINVAL; + } + + return 0; } static int @@ -663,6 +669,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop, { if (property == colorop->type_property) { *val = colorop->type; + } else if (property == colorop->curve_1d_type_property) { + *val = state->curve_1d_type; } else { return -EINVAL; } diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c index 1459a28c7e7b..a42de0aa48e1 100644 --- a/drivers/gpu/drm/drm_colorop.c +++ b/drivers/gpu/drm/drm_colorop.c @@ -31,6 +31,123 @@ #include "drm_crtc_internal.h" +static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = { + { DRM_COLOROP_1D_CURVE, "1D Curve" }, +}; + +static const char * const colorop_curve_1d_type_names[] = { + [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF", + [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF", +}; + + +/* Init Helpers */ + +static int drm_colorop_init(struct drm_device *dev, struct drm_colorop *colorop, + 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); + if (ret) + return ret; + + 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; +} + +/** + * drm_colorop_curve_1d_init - Initialize a DRM_COLOROP_1D_CURVE + * + * @dev: DRM device + * @colorop: The drm_colorop object to initialize + * @plane: The associated drm_plane + * @supported_tfs: A bitfield of supported drm_colorop_curve_1d_init enum values, + * created using BIT(curve_type) and combined with the OR '|' + * operator. + * @return zero on success, -E value on failure + */ +int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop *colorop, + struct drm_plane *plane, u64 supported_tfs) +{ + struct drm_prop_enum_list enum_list[DRM_COLOROP_1D_CURVE_COUNT]; + int i, len; + + struct drm_property *prop; + int ret; + + if (!supported_tfs) { + drm_err(dev, + "No supported TFs for new 1D curve colorop on [PLANE:%d:%s]\n", + plane->base.id, plane->name); + return -EINVAL; + } + + if ((supported_tfs & -BIT(DRM_COLOROP_1D_CURVE_COUNT)) != 0) { + drm_err(dev, "Unknown TF provided on [PLANE:%d:%s]\n", + plane->base.id, plane->name); + return -EINVAL; + } + + ret = drm_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_CURVE); + if (ret) + return ret; + + len = 0; + for (i = 0; i < DRM_COLOROP_1D_CURVE_COUNT; i++) { + if ((supported_tfs & BIT(i)) == 0) + continue; + + enum_list[len].type = i; + enum_list[len].name = colorop_curve_1d_type_names[i]; + len++; + } + + if (WARN_ON(len <= 0)) + return -EINVAL; + + + /* initialize 1D curve only attribute */ + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, "CURVE_1D_TYPE", + enum_list, len); + if (!prop) + return -ENOMEM; + + colorop->curve_1d_type_property = prop; + drm_object_attach_property(&colorop->base, colorop->curve_1d_type_property, + enum_list[0].type); + drm_colorop_reset(colorop); + + return 0; +} +EXPORT_SYMBOL(drm_colorop_curve_1d_init); + static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop, struct drm_colorop_state *state) { @@ -70,7 +187,16 @@ void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop, static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state, struct drm_colorop *colorop) { + u64 val; + colorop_state->colorop = colorop; + + if (colorop->curve_1d_type_property) { + drm_object_property_get_default_value(&colorop->base, + colorop->curve_1d_type_property, + &val); + colorop_state->curve_1d_type = val; + } } /** @@ -114,3 +240,11 @@ const char *drm_get_colorop_type_name(enum drm_colorop_type type) return colorop_type_name[type]; } + +const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type type) +{ + if (WARN_ON(type >= ARRAY_SIZE(colorop_curve_1d_type_names))) + return "unknown"; + + return colorop_curve_1d_type_names[type]; +} diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h index 9c9698545f63..fc1b2e0f33e0 100644 --- a/include/drm/drm_colorop.h +++ b/include/drm/drm_colorop.h @@ -31,6 +31,42 @@ #include <drm/drm_mode.h> #include <drm/drm_property.h> + +/** + * enum drm_colorop_curve_1d_type - type of 1D curve + * + * Describes a 1D curve to be applied by the DRM_COLOROP_1D_CURVE colorop. + */ +enum drm_colorop_curve_1d_type { + /** + * @DRM_COLOROP_1D_CURVE_SRGB_EOTF: + * + * enum string "sRGB EOTF" + * + * sRGB piece-wise electro-optical transfer function. Transfer + * characteristics as defined by IEC 61966-2-1 sRGB. Equivalent + * to H.273 TransferCharacteristics code point 13 with + * MatrixCoefficients set to 0. + */ + DRM_COLOROP_1D_CURVE_SRGB_EOTF, + + /** + * @DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF: + * + * enum string "sRGB Inverse EOTF" + * + * The inverse of &DRM_COLOROP_1D_CURVE_SRGB_EOTF + */ + DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF, + + /** + * @DRM_COLOROP_1D_CURVE_COUNT: + * + * enum value denoting the size of the enum + */ + DRM_COLOROP_1D_CURVE_COUNT +}; + /** * struct drm_colorop_state - mutable colorop state */ @@ -46,6 +82,13 @@ struct drm_colorop_state { * information. */ + /** + * @curve_1d_type: + * + * Type of 1D curve. + */ + enum drm_colorop_curve_1d_type curve_1d_type; + /** @state: backpointer to global drm_atomic_state */ struct drm_atomic_state *state; }; @@ -127,6 +170,14 @@ struct drm_colorop { * this color operation. The type is enum drm_colorop_type. */ struct drm_property *type_property; + + /** + * @curve_1d_type_property: + * + * Sub-type for DRM_COLOROP_1D_CURVE type. + */ + struct drm_property *curve_1d_type_property; + }; #define obj_to_colorop(x) container_of(x, struct drm_colorop, base) @@ -151,6 +202,9 @@ static inline struct drm_colorop *drm_colorop_find(struct drm_device *dev, return mo ? obj_to_colorop(mo) : NULL; } +int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop *colorop, + struct drm_plane *plane, u64 supported_tfs); + struct drm_colorop_state * drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop); @@ -191,4 +245,13 @@ static inline unsigned int drm_colorop_index(const struct drm_colorop *colorop) */ const char *drm_get_colorop_type_name(enum drm_colorop_type type); +/** + * drm_get_colorop_curve_1d_type_name - return a string for 1D curve type + * @type: 1d curve 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_curve_1d_type_name(enum drm_colorop_curve_1d_type type); + #endif /* __DRM_COLOROP_H__ */