Re: [RFC PATCH v3 15/23] drm/vkms: Add enumerated 1D curve colorop

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9001 bytes --]

On Wed, 8 Nov 2023 11:36:34 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> 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.

VKMS could just implement the sRGB TF formula rather than an opaque
array of numbers that supposedly computes the same'ish.

Why is even the identity curve (which you call linear) encoded as a LUT?

> v2:
>  - Add commit description
>  - Fix sRGB EOTF LUT definition
>  - Add linear and sRGB inverse EOTF LUTs
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/vkms/Makefile        |   4 +-
>  drivers/gpu/drm/vkms/vkms_colorop.c  |  85 +++
>  drivers/gpu/drm/vkms/vkms_composer.c |  45 ++
>  drivers/gpu/drm/vkms/vkms_drv.h      |   4 +
>  drivers/gpu/drm/vkms/vkms_luts.c     | 802 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_luts.h     |  12 +
>  drivers/gpu/drm/vkms/vkms_plane.c    |   2 +
>  7 files changed, 953 insertions(+), 1 deletion(-)
>  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 1b28a6a32948..c38455c46be4 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -6,6 +6,8 @@ vkms-y := \
>  	vkms_formats.o \
>  	vkms_crtc.o \
>  	vkms_composer.o \
> -	vkms_writeback.o
> +	vkms_writeback.o \
> +	vkms_colorop.o \
> +	vkms_luts.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
> new file mode 100644
> index 000000000000..9a26b9fdc4a2
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -0,0 +1,85 @@
> +/* 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>
> +
> +#define MAX_COLOR_PIPELINES 5
> +
> +const int vkms_initialize_tf_pipeline(struct drm_plane *plane, struct drm_prop_enum_list *list)
> +{
> +
> +	struct drm_colorop *op, *prev_op;
> +	struct drm_device *dev = plane->dev;
> +	int ret;
> +
> +	/* 1st op: 1d curve */
> +	op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> +	if (!op) {
> +		DRM_ERROR("KMS: Failed to allocate colorop\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
> +	if (ret)
> +		return ret;
> +
> +	list->type = op->base.id;
> +	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", op->base.id);

This is the string name for the pipeline (enum value).

I don't think including the ID in the name is good, because the ID
might be different on every load of the driver. IIRC you said userspace
is ok to hardcode the pipeline name and look for it, instead of doing
the discovery.

> +
> +	prev_op = op;
> +
> +	/* 2nd op: 1d curve */
> +	op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> +	if (!op) {
> +		DRM_ERROR("KMS: Failed to allocate colorop\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = drm_colorop_init(dev, op, plane, DRM_COLOROP_1D_CURVE);
> +	if (ret)
> +		return ret;
> +
> +	drm_colorop_set_next_property(prev_op, op);
> +
> +	return 0;
> +}
> +
> +int vkms_initialize_colorops(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
> +	int len = 0;
> +	int ret;
> +
> +	/* Add "Bypass" (i.e. NULL) pipeline */
> +	pipelines[len].type = 0;
> +	pipelines[len].name = "Bypass";
> +	len++;

Should there be a helper to replace the drm_property_create_enum() call
below that would always add "Bypass"?

Maybe that would push driver authors towards always supporting bypass
if at all possible.

It could also be called "Identity" pipeline, or is that too strict? I
wonder what happens with an YUV FB.

> +
> +	/* Add pipeline consisting of transfer functions */
> +	ret = vkms_initialize_tf_pipeline(plane, &(pipelines[len]));
> +	if (ret)
> +		return ret;
> +	len++;
> +
> +	/* Create COLOR_PIPELINE property and attach */
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"COLOR_PIPELINE",
> +					pipelines, len);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	plane->color_pipeline_property = prop;
> +
> +	drm_object_attach_property(&plane->base, prop, 0);
> +
> +	/* TODO do we even need this? */
> +	if (plane->state)
> +		plane->state->color_pipeline = NULL;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 25b6b73bece8..be42756e300a 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -12,6 +12,7 @@
>  #include <linux/minmax.h>
>  
>  #include "vkms_drv.h"
> +#include "vkms_luts.h"
>  
>  static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
>  {
> @@ -163,6 +164,47 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
>  	}
>  }
>  
> +static void pre_blend_color_transform(const struct vkms_plane_state *plane_state, struct line_buffer *output_buffer)
> +{
> +	struct drm_colorop *colorop = plane_state->base.base.color_pipeline;
> +
> +	while (colorop) {
> +		struct drm_colorop_state *colorop_state;
> +
> +		if (!colorop)
> +			return;
> +
> +		/* TODO this is probably wrong */
> +		colorop_state = colorop->state;
> +
> +		if (!colorop_state)
> +			return;
> +
> +		for (size_t x = 0; x < output_buffer->n_pixels; x++) {
> +			struct pixel_argb_u16 *pixel = &output_buffer->pixels[x];
> +
> +			if (colorop->type == DRM_COLOROP_1D_CURVE &&
> +				colorop_state->bypass == false) {
> +				switch (colorop_state->curve_1d_type) {

FWIW, for performance reasons, it would probably be better to move all
conditionals outside of the innermost loop.

> +					case DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF:
> +						pixel->r = apply_lut_to_channel_value(&srgb_inv_eotf, pixel->r, LUT_RED);
> +						pixel->g = apply_lut_to_channel_value(&srgb_inv_eotf, pixel->g, LUT_GREEN);
> +						pixel->b = apply_lut_to_channel_value(&srgb_inv_eotf, pixel->b, LUT_BLUE);
> +						break;
> +					case DRM_COLOROP_1D_CURVE_SRGB_EOTF:
> +					default:
> +						pixel->r = apply_lut_to_channel_value(&srgb_eotf, pixel->r, LUT_RED);
> +						pixel->g = apply_lut_to_channel_value(&srgb_eotf, pixel->g, LUT_GREEN);
> +						pixel->b = apply_lut_to_channel_value(&srgb_eotf, pixel->b, LUT_BLUE);
> +						break;
> +				}
> +			}
> +		}
> +
> +		colorop = colorop->next;
> +	}
> +}
> +
>  /**
>   * blend - blend the pixels from all planes and compute crc
>   * @wb: The writeback frame buffer metadata
> @@ -200,6 +242,9 @@ static void blend(struct vkms_writeback_job *wb,
>  				continue;
>  
>  			vkms_compose_row(stage_buffer, plane[i], y_pos);
> +
> +			pre_blend_color_transform(plane[i], stage_buffer);
> +
>  			pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
>  					    output_buffer);
>  		}
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8f5710debb1e..2bcc24c196a2 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -170,4 +170,8 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer
>  /* Writeback */
>  int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
>  
> +/* Colorops */
> +int vkms_initialize_colorops(struct drm_plane *plane);
> +
> +
>  #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_luts.c b/drivers/gpu/drm/vkms/vkms_luts.c
> new file mode 100644
> index 000000000000..6553d6d442b4
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_luts.c
> @@ -0,0 +1,802 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include <drm/drm_mode.h>
> +
> +#include "vkms_drv.h"
> +#include "vkms_luts.h"
> +
> +static struct drm_color_lut linear_array[LUT_SIZE] = {

If I was a maintainer, I would be interested in having it documented how
these tables were generated, if not even generate them at build time.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]