Le 18/09/2025 à 02:45, Alex Hung a écrit :
On 9/17/25 08:47, Nícolas F. R. A. Prado wrote:
On Tue, 2025-09-16 at 19:54 -0600, Alex Hung wrote:
On 9/5/25 11:12, Louis Chauvet wrote:
Le 15/08/2025 à 05:50, Alex Hung a écrit :
From: Harry Wentland <harry.wentl...@amd.com>
This patch introduces a VKMS color pipeline that includes two
drm_colorops for named transfer functions. For now the only ones
supported are sRGB EOTF, sRGB Inverse EOTF, and a Linear TF.
We will expand this in the future but I don't want to do so
without accompanying IGT tests.
We introduce a new vkms_luts.c file that hard-codes sRGB EOTF,
sRGB Inverse EOTF, and a linear EOTF LUT. These have been
generated with 256 entries each as IGT is currently testing
only 8 bpc surfaces. We will likely need higher precision
but I'm reluctant to make that change without clear indication
that we need it. We'll revisit and, if necessary, regenerate
the LUTs when we have IGT tests for higher precision buffers.
Signed-off-by: Harry Wentland <harry.wentl...@amd.com>
Signed-off-by: Alex Hung <alex.h...@amd.com>
Reviewed-by: Daniel Stone <dani...@collabora.com>
---
v11:
- Update drm_colorop_pipeline_destroy from plane to dev
(Nícolas Prado)
- Fix undefined errors by EXPORT_SYMBOL symbols (kernel test
robot)
v9:
- Replace cleanup code by drm_colorop_pipeline_destroy (Simon
Ser)
- Update function names by _plane_ (Chaitanya Kumar Borah)
v8:
- Replace DRM_ERROR by drm_err (Louis Chauvet)
- Replace DRM_WARN_ONCE by drm_WARN_ONCE (Louis Chauvet)
- Fix conflicts with upstream VKMS (Louis Chauvet)
- Add comments for drm_color_lut linear_array (Louis Chauvet)
v7:
- Fix checkpatch warnings (Louis Chauvet)
- Change kzalloc(sizeof(struct drm_colorop) ...) to
kzalloc(sizeof(*ops[i]) ...)
- Remove if (ops[i]) before kfree(ops[i])
- Fix styles by adding and removing spaces (new lines, tabs
and so on)
v6:
- drop 'len' var (Louis Chauvet)
- cleanup if colorop alloc or init fails (Louis Chauvet)
- switch loop in pre_blend_transform (Louis Chauvet)
- drop extraneous if (colorop) inside while (colorop) (Louis
Chauvet)
v5:
- Squash with "Pull apply_colorop out of
pre_blend_color_transform"
(Sebastian)
- Fix warnings
- Fix include
- Drop TODOs
v4:
- Drop _tf_ from color_pipeline init function
- Pass supported TFs into colorop init
- Create bypass pipeline in DRM helper (Pekka)
v2:
- Add commit description
- Fix sRGB EOTF LUT definition
- Add linear and sRGB inverse EOTF LUTs
drivers/gpu/drm/vkms/Makefile | 4 +-
drivers/gpu/drm/vkms/vkms_colorop.c | 81 +++
drivers/gpu/drm/vkms/vkms_composer.c | 51 +-
drivers/gpu/drm/vkms/vkms_drv.h | 3 +
drivers/gpu/drm/vkms/vkms_luts.c | 811
+++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_luts.h | 12 +
drivers/gpu/drm/vkms/vkms_plane.c | 2 +
7 files changed, 962 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_colorop.c
create mode 100644 drivers/gpu/drm/vkms/vkms_luts.c
create mode 100644 drivers/gpu/drm/vkms/vkms_luts.h
diff --git a/drivers/gpu/drm/vkms/Makefile
b/drivers/gpu/drm/vkms/
Makefile
index d657865e573f..0b8936674f69 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -8,7 +8,9 @@ vkms-y := \
vkms_composer.o \
vkms_writeback.o \
vkms_connector.o \
- vkms_config.o
+ vkms_config.o \
+ vkms_colorop.o \
+ vkms_luts.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += tests/
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
b/drivers/gpu/drm/
vkms/vkms_colorop.c
new file mode 100644
index 000000000000..f955ffb0ac84
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/slab.h>
+#include <drm/drm_colorop.h>
+#include <drm/drm_print.h>
+#include <drm/drm_property.h>
+#include <drm/drm_plane.h>
+
+#include "vkms_drv.h"
+
+static const u64 supported_tfs =
+ BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
+ BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
+
+#define MAX_COLOR_PIPELINE_OPS 2
+
+static int vkms_initialize_color_pipeline(struct drm_plane
*plane,
struct drm_prop_enum_list *list)
+{
+ struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
+ struct drm_device *dev = plane->dev;
+ int ret;
+ int i = 0;
+
+ memset(ops, 0, sizeof(ops));
+
+ /* 1st op: 1d curve */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ drm_err(dev, "KMS: Failed to allocate colorop\n");
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
supported_tfs);
+ if (ret)
+ goto cleanup;
+
+ list->type = ops[i]->base.id;
+ list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d",
ops[i]-
base.id);
+
+ i++;
+
+ /* 2nd op: 1d curve */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ drm_err(dev, "KMS: Failed to allocate colorop\n");
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
supported_tfs);
+ if (ret)
+ goto cleanup;
+
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
+
+ return 0;
+
+cleanup:
+ drm_colorop_pipeline_destroy(dev);
If it take a device, it means that it deletes everything, which is
not
what I would expect here: you are curently allocating a specific
plane
pipeline, and deleting all colorop for other planes because of one
failure is counterintuitive.
In this situation I would expect either:
- error propagation to vkms_create or vkms_output_init (it is
already
the case) and "device-wide" cleanup in
vkms_create/vkms_output_init;
- "local" cleanup (i.e only this specific pipeline)
the colorop are now in dev->mode_config->colorop_list, so we can use
"drm_colorop_cleanup" (assuming it is changed to be available here) for
cleanup if removing entire colorop_list by
drm_colorop_pipeline_destroy(dev) is not desireable in vkms. Does the
following code make sense?
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
b/drivers/gpu/drm/vkms/vkms_colorop.c
index 0191ac44dec0..d263e3593ad5 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -19,7 +19,7 @@ static int vkms_initialize_color_pipeline(struct
drm_plane *plane, struct drm_pr
struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
struct drm_device *dev = plane->dev;
int ret;
- int i = 0;
+ int i = 0, j = 0;
memset(ops, 0, sizeof(ops));
@@ -91,7 +91,10 @@ static int vkms_initialize_color_pipeline(struct
drm_plane *plane, struct drm_pr
return 0;
cleanup:
- drm_colorop_pipeline_destroy(dev);
+ for (j = 0; j < i; j++) {
+ if (ops[j])
+ drm_colorop_cleanup(ops[j]);
+ }
return ret;
}
Yes, that could work!
I think you need to add a kfree for ops[j], but this code is better: it
only destroy what was allocated in this function, no more.
BTW, while reviewing this series + post_blend, I noticed that the
pipeline is never freed on device destruction. Did I miss something in
the colorop core? If no, I think it should be added in vkms_destroy or
using automagic drmm_add_action_or_reset in vkms_output_init.
Hi Louis,
Does it make sense to make drm_colorop_pipeline_destroy(drm_plane),
i.e.
in PATCH 13 as in previously V10?
and then drm_colorop_pipeline_destroy should limit to the pipeline in
a
drm_plane and should do something like
drm_colorop_cleanup(colorop);
free(colorop)
drm_plane->colorop = NULL;
This doesn't seem right after digging into it.
We can have same behaviours accross device drivers like amdgpu too.
Hi Simon and Nicolas,
Do you have comments on above proposal?
Hi,
indeed that would make more sense to me.
--
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
