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
