Re: [PATCH v5 14/44] drm/vkms: Add enumerated 1D curve colorop

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 19/08/24 - 16:56, Harry Wentland a écrit :

> +static int vkms_initialize_color_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_curve_1d_init(dev, op, plane, supported_tfs);
> +	if (ret)
> +		return ret;
> +
> +	list->type = op->base.id;
> +	list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d", op->base.id);
> +
> +	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;
> +	}

There is no need to cleanup previously created colorop here?

> +	ret = drm_colorop_curve_1d_init(dev, op, plane, supported_tfs);
> +	if (ret)
> +		return ret;

ditto?

> +	drm_colorop_set_next_property(prev_op, op);
> +
> +	return 0;
> +}
> +
> +int vkms_initialize_colorops(struct drm_plane *plane)
> +{
> +	struct drm_prop_enum_list pipelines[MAX_COLOR_PIPELINES];
> +	int len = 0;
> +	int ret;
> +
> +	/* Add color pipeline */
> +	ret = vkms_initialize_color_pipeline(plane, &(pipelines[len]));
> +	if (ret)
> +		return ret;
> +	len++;

The usage of len is not very clear, can you maybe remove it? Or do you 
plan to use it for instanciating multiple pipelines?

	struct drm_prop_enum_list pipeline;
	ret = vkms_initialize_color_pipeline(plane, &pipeline);
	drm_plane_create_color_pipeline_property(plane, &pipeline, 1);

> +	/* Create COLOR_PIPELINE property and attach */
> +	drm_plane_create_color_pipeline_property(plane, pipelines, len);

This function can fail, can you also test the result here?

> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 3ecda70c2b55..bc116d16e378 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,53 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
>  	}
>  }
>  
> +static void apply_colorop(struct pixel_argb_u16 *pixel, struct drm_colorop *colorop)
> +{
> +	struct drm_colorop_state *colorop_state = colorop->state;
> +
> +	if (colorop->type == DRM_COLOROP_1D_CURVE) {
> +		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:
> +				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;
> +			default:
> +				DRM_DEBUG_DRIVER("unkown colorop 1D curve type %d\n", colorop_state->curve_1d_type);

Maybe add a ONCE/RATELIMITED here, this function is called for every 
pixel.

> +				break;
> +		}
> +	}
> +
> +}
> +
> +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;
> +
> +		colorop_state = colorop->state;
> +
> +		if (!colorop_state)
> +			return;
> +
> +		for (size_t x = 0; x < output_buffer->n_pixels; x++)
> +			if (!colorop_state->bypass)

Maybe we can swap the for and the if? I don't know the performance impact 
is huge, but it feel more logical to check bypass before iterating.

> +				apply_colorop(&output_buffer->pixels[x], colorop);
> +
> +		colorop = colorop->next;
> +	}
> +}
> +
>  /**
>   * blend - blend the pixels from all planes and compute crc
>   * @wb: The writeback frame buffer metadata
> @@ -200,6 +248,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_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] = {
> +	{ 0x0, 0x0, 0x0, 0 },
> +	{ 0x101, 0x101, 0x101, 0 },
> +	{ 0x202, 0x202, 0x202, 0 },
> +	{ 0x303, 0x303, 0x303, 0 },
> +	{ 0x404, 0x404, 0x404, 0 },

For this LUT and the other, can you add a comment to explain how the 
values were generated/found?

[...]

Thanks,
Louis Chauvet