On Mon, 26 Feb 2024 16:10:31 -0500
Harry Wentland <harry.wentland@xxxxxxx> 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@xxxxxxx>
> Signed-off-by: Alex Hung <alex.hung@xxxxxxx>
> ---
> 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
Attachment:
pgpqxjREYfl_v.pgp
Description: OpenPGP digital signature
