Re: [V7 07/45] drm/colorop: Add 1D Curve subtype

2025-02-28 Thread Louis Chauvet




Le 28/02/2025 à 02:07, Alex Hung a écrit :



On 2/25/25 03:13, Louis Chauvet wrote:



Le 20/12/2024 à 05:33, Alex Hung a écrit :

From: Harry Wentland 

Add a new drm_colorop with DRM_COLOROP_1D_CURVE with two subtypes:
DRM_COLOROP_1D_CURVE_SRGB_EOTF and DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF.

Signed-off-by: Harry Wentland 
Co-developed-by: Alex Hung 
Signed-off-by: Alex Hung 
---
v5:
   - Add drm_get_colorop_curve_1d_type_name in header
   - Add drm_colorop_init
   - Set default curve
   - Add kernel docs

v4:
   - Use drm_colorop_curve_1d_type_enum_list to get name (Pekka)
   - Create separate init function for 1D curve
   - Pass supported TFs into 1D curve init function

   drivers/gpu/drm/drm_atomic_uapi.c |  18 ++--
   drivers/gpu/drm/drm_colorop.c | 134 ++
   include/drm/drm_colorop.h |  60 +
   3 files changed, 207 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/
drm_atomic_uapi.c
index 59fc25b59100..9a5dbf0a1306 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -648,11 +648,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
@@ -662,6 +668,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));


I think this function belongs to the previous patch "Add TYPE property".


This function is only called by the first colorop. Some pieces of the
code in this function are introduced with the first colorop (1D curve)
so it makes sense to include it here.


True! I did not saw it, you can keep it here indeed




+
+    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)

Re: [V7 05/45] drm/colorop: Introduce new drm_colorop mode object

2025-02-28 Thread Harry Wentland




On 2025-02-25 05:05, Louis Chauvet wrote:



Le 20/12/2024 à 05:33, Alex Hung a écrit :

From: Harry Wentland 

@@ -249,6 +255,20 @@ void drm_atomic_state_default_clear(struct 
drm_atomic_state *state)

  state->planes[i].new_state = NULL;
  }
+    for (i = 0; i < config->num_colorop; i++) {
+    struct drm_colorop *colorop = state->colorops[i].ptr;
+
+    if (!colorop)
+    continue;
+
+    drm_colorop_atomic_destroy_state(colorop,
+ state->colorops[i].state);
+    state->colorops[i].ptr = NULL;
+    state->colorops[i].state = NULL;


There is no risk of use-after-free between the 
drm_colorop_atomic_destroy_state and the state->colorops[i].state?




This is using the same pattern as all the other DRM objects in this
function. If this was a problem it would've been a problem before this
change.

I don't claim to fully understand the calling code but this is called
from __drm_atomic_state_free and to backoff when an -EDEADLK occurs.
In the latter case it's followed by drm_modeset_backoff which releases
locks in drm_modeset_drop_locks. This seems to indicate that callers
hold the respective locks to protect the state.

Harry