Re: [PATCH V13 39/51] drm/amd/display: add 3x4 matrix colorop

Fri, 19 Dec 2025 05:10:39 -0800 



On 12/19/2025 6:11 PM, Tvrtko Ursulin wrote:


On 15/11/2025 00:02, Alex Hung wrote:

This adds support for a 3x4 color transformation matrix.

With this change the following IGT tests pass:
kms_colorop --run plane-XR30-XR30-ctm_3x4_50_desat
kms_colorop --run plane-XR30-XR30-ctm_3x4_overdrive
kms_colorop --run plane-XR30-XR30-ctm_3x4_oversaturate
kms_colorop --run plane-XR30-XR30-ctm_3x4_bt709_enc
kms_colorop --run plane-XR30-XR30-ctm_3x4_bt709_dec

The color pipeline now consists of the following colorops:
1. 1D curve colorop
2. 3x4 CTM
3. 1D curve colorop
4. 1D LUT
5. 1D curve colorop
6. 1D LUT

Signed-off-by: Alex Hung <[email protected]>
Signed-off-by: Harry Wentland <[email protected]>
Reviewed-by: Daniel Stone <[email protected]>
Reviewed-by: Melissa Wen <[email protected]>
---
v13:

  - Remove redundant ternary null check for drm_color_ctm_3x4 blob 
(Coverity Scan)


V10:
  - Change %lu to %zu for sizeof() in drm_warn (kernel test robot)
  - Remove redundant DRM_ERROR(...)

V9:
  - Update function names by _plane_ (Chaitanya Kumar Borah)

v8:
  - Return -EINVAL when drm_color_ctm_3x4's size mismatches (Leo Li)

v7:
  - Change %lu to %zu for sizeof() in drm_warn

v6:
  - fix warnings in dbg prints

  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 52 +++++++++++++++++++
  .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 15 ++++++
  2 files changed, 67 insertions(+)


diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c

index b958f9c0a0c2..298f337f0eb4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c

@@ -1378,6 +1378,47 @@ __set_dm_plane_colorop_degamma(struct 
drm_plane_state *plane_state,

      return __set_colorop_in_tf_1d_curve(dc_plane_state, colorop_state);
  }
+static int
+__set_dm_plane_colorop_3x4_matrix(struct drm_plane_state *plane_state,
+                  struct dc_plane_state *dc_plane_state,
+                  struct drm_colorop *colorop)
+{
+    struct drm_colorop *old_colorop;
+    struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+    struct drm_atomic_state *state = plane_state->state;
+    const struct drm_device *dev = colorop->dev;
+    const struct drm_property_blob *blob;
+    struct drm_color_ctm_3x4 *ctm = NULL;
+    int i = 0;
+
+    /* 3x4 matrix */
+    old_colorop = colorop;

+    for_each_new_colorop_in_state(state, colorop, new_colorop_state, 
i) {

+        if (new_colorop_state->colorop == old_colorop &&
+            new_colorop_state->colorop->type == DRM_COLOROP_CTM_3X4) {
+            colorop_state = new_colorop_state;
+            break;
+        }
+    }
+

+    if (colorop_state && !colorop_state->bypass && colorop->type == 
DRM_COLOROP_CTM_3X4) {
+        drm_dbg(dev, "3x4 matrix colorop with ID: %d\n", colorop- 
>base.id);

+        blob = colorop_state->data;
+        if (blob->length == sizeof(struct drm_color_ctm_3x4)) {
+            ctm = (struct drm_color_ctm_3x4 *) blob->data;

+            __drm_ctm_3x4_to_dc_matrix(ctm, dc_plane_state- 
>gamut_remap_matrix.matrix);

+            dc_plane_state->gamut_remap_matrix.enable_remap = true;

+            dc_plane_state->input_csc_color_matrix.enable_adjustment 
= false;

+        } else {

+            drm_warn(dev, "blob->length (%zu) isn't equal to 
drm_color_ctm_3x4 (%zu)\n",

+                 blob->length, sizeof(struct drm_color_ctm_3x4));
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
  static int
  __set_dm_plane_colorop_shaper(struct drm_plane_state *plane_state,
                    struct dc_plane_state *dc_plane_state,

@@ -1581,6 +1622,17 @@ amdgpu_dm_plane_set_colorop_properties(struct 
drm_plane_state *plane_state,

      if (ret)
          return ret;
+    /* 3x4 matrix */
+    colorop = colorop->next;
+    if (!colorop) {
+        drm_dbg(dev, "no 3x4 matrix colorop found\n");
+        return -EINVAL;
+    }
+

+    ret = __set_dm_plane_colorop_3x4_matrix(plane_state, 
dc_plane_state, colorop);

+    if (ret)
+        return ret;
+
      /* 1D Curve & LUT - SHAPER TF & LUT */
      colorop = colorop->next;
      if (!colorop) {

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c

index 4845f26e4a8a..f2be75b9b073 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c

@@ -74,6 +74,21 @@ int amdgpu_dm_initialize_default_pipeline(struct 
drm_plane *plane, struct drm_pr

      i++;
+    /* 3x4 matrix */
+    ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
+    if (!ops[i]) {
+        ret = -ENOMEM;
+        goto cleanup;



Does this cleanup path leak the list->name allocated a few lines above, 
outside the diff? Kmemleak appears to think it can leak from somewhere 
at least:


unreferenced object 0xffff8881143c1e00 (size 32):
   comm "(udev-worker)", pid 588, jiffies 4294888494
   hex dump (first 32 bytes):
     43 6f 6c 6f 72 20 50 69 70 65 6c 69 6e 65 20 33  Color Pipeline 3
     31 33 00 00 00 00 00 00 00 00 00 00 00 00 00 00  13..............
   backtrace (crc a75af242):
     __kmalloc_node_track_caller_noprof+0x525/0x890
     kvasprintf+0xb6/0x130
     kasprintf+0xb2/0xe0
     amdgpu_dm_initialize_default_pipeline+0x1ce/0x840 [amdgpu]
     dm_plane_init_colorops+0x19c/0x2e0 [amdgpu]
     amdgpu_dm_plane_init+0x4c4/0xf10 [amdgpu]
     initialize_plane+0xf1/0x280 [amdgpu]
     amdgpu_dm_init+0x23d0/0x7450 [amdgpu]
     dm_hw_init+0x3d/0x200 [amdgpu]
     amdgpu_device_init+0x67cd/0x9b20 [amdgpu]
     amdgpu_driver_load_kms+0x13/0xf0 [amdgpu]
     amdgpu_pci_probe+0x437/0xf30 [amdgpu]
     local_pci_probe+0xda/0x180
     pci_device_probe+0x381/0x730
     really_probe+0x1da/0x970
     __driver_probe_device+0x18c/0x3e0


Could it be a false positive, or leaking later after the success path, 
or some other path I am not sure since I am not at home in this code.



There is no error checking on list->name either, so I supppose the code 
which can touch that can handle a NULL harmlessly?


I sent out a fix for this (and some) today.

https://lore.kernel.org/intel-gfx/[email protected]/T/#m3893809764c761af38043dddf7cc962b8c3aa44e


We need list->name just for registering the enum. 
drm_property_create_enum() makes it's own copy. Therefore, there is no 
owner of the string after init function's context ends.



As for checking the output of kasprintf, I judged it low probabilty 
enough to skip it. I see it being done both ways (i.e. with and without 
error handling) in the kernel.


==
Chaitanya



Regards,

Tvrtko


+    }
+
+    ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane);
+    if (ret)
+        goto cleanup;
+
+    drm_colorop_set_next_property(ops[i-1], ops[i]);
+
+    i++;
+
      /* 1D curve - SHAPER TF */
      ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
      if (!ops[i]) {


























					Reply via email to