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)
>
> 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;
>
> 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.
--
Thanks,
Nícolas
