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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

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 <alex.hung@amd.com>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>> Reviewed-by: Melissa Wen <mwen@igalia.com>
>> ---
>> 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/20251219065614.190834-1-chaitanya.kumar.borah@intel.com/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]) {
>