[RFC PATCH v4 17/42] drm/vkms: Add enumerated 1D curve colorop

[Pekka Paalanen]

On Mon, 26 Feb 2024 16:10:31 -0500
Harry Wentland <harry.wentland at 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.
> 
> 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
> 
> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
> Signed-off-by: Alex Hung <alex.hung at amd.com>
> ---
>  drivers/gpu/drm/vkms/Makefile        |   4 +-
>  drivers/gpu/drm/vkms/vkms_colorop.c  |  70 +++
>  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, 938 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/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index b90e446d5954..9493bdb1ba3f 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) {

I think this would be easier to read if you used

	for (; colorop; colorop = colorop->next) {

and

> +		struct drm_colorop_state *colorop_state;
> +
> +		if (!colorop)
> +			return;
> +
> +		/* TODO this is probably wrong */
> +		colorop_state = colorop->state;
> +
> +		if (!colorop_state)
> +			return;

	if (colorop_state->bypass)
		continue;

Something about 'switch (colorop->type)' to pick a function pointer to
call, but hard to see at this point of the series how that would work.

However, you can pick between srgb_inv_eotf and srgb_eotf already here.
Then inside the loop you can just call one set of
apply_lut_to_channel_value() and not need conditionals and avoid
indentation levels.

> +
> +		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) {
> +					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;
> +				}
> +			}

else { aaargh_unknown_colorop(); }

> +		}
> +
> +		colorop = colorop->next;
> +	}
> +}

...

> 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"
> +

Here it would be really nice to explain how the tables were generated.

> +static struct drm_color_lut linear_array[LUT_SIZE] = {
> +	{ 0x0, 0x0, 0x0, 0 },

...

> +	{ 0xffff, 0xffff, 0xffff, 0 },
> +};
> +
> +const struct vkms_color_lut linear_eotf = {
> +	.base = linear_array,
> +	.lut_length = LUT_SIZE,

Why not use just 2 table entries for the linear array?

I didn't see linear_eotf used at all? It could also just skip in the
code, not need an array.

> +	.channel_value2index_ratio = 0xff00ffll
> +};


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20240312/c3d967df/attachment-0001.sig>