Re: [PATCH v5 02/44] drm/vkms: Round fixp2int conversion in lerp_u16
Le 19/08/24 - 16:56, Harry Wentland a écrit : > fixp2int always rounds down, fixp2int_ceil rounds up. We need > the new fixp2int_round. > > Signed-off-by: Harry Wentland > --- > drivers/gpu/drm/vkms/vkms_composer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c > b/drivers/gpu/drm/vkms/vkms_composer.c > index e7441b227b3c..3d6785d081f2 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -98,7 +98,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t) > > s64 delta = drm_fixp_mul(b_fp - a_fp, t); > > - return drm_fixp2int(a_fp + delta); > + return drm_fixp2int_round(a_fp + delta); > } > > static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value) > -- > 2.46.0 > Reviewed-by: Louis Chauvet
Re: [PATCH v5 14/44] drm/vkms: Add enumerated 1D curve colorop
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 ..6553d6d442b4
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_luts.c
> @@ -0,0 +1,802 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include
> +
> +#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
Re: [PATCH v5 03/44] drm/vkms: Add kunit tests for VKMS LUT handling
Le 19/08/24 - 16:56, Harry Wentland a écrit : [...] > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c > b/drivers/gpu/drm/vkms/vkms_composer.c > index 3d6785d081f2..3ecda70c2b55 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -435,3 +435,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char > *src_name) diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 3d6785d081f2..3ecda70c2b55 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -435,3 +435,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) return ret; } + +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS +#include "tests/vkms_color_tests.c" +#endif> > return ret; > } > + > +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS > +#include "tests/vkms_color_tests.c" > +#endif This is very strange to include a .c in this file, is it something done a lot in the kernel? I can find only one occurence of this pattern in the kernel [1], the other tests are in their own modules. In addition it crate many warning during compilations: warning: symbol 'test_*' was not declared. Should it be static? As other tests will be introduced (yuv [2], config [3]), it is maybe interesting to introduce a new module as [2] is doing? [1]: https://elixir.bootlin.com/linux/v6.11-rc5/source/fs/ext4/mballoc.c#L7047 [2]: https://lore.kernel.org/all/20240809-yuv-v10-14-1a7c76416...@bootlin.com/ [3]: https://lore.kernel.org/all/20240814-google-remove-crtc-index-from-parameter-v1-15-6e179abf9...@bootlin.com/
Re: [PATCH v5 05/44] drm/colorop: Introduce new drm_colorop mode object
Le 19/08/24 - 16:56, Harry Wentland a écrit :
[...]
> +#ifndef __DRM_COLOROP_H__
> +#define __DRM_COLOROP_H__
> +
> +#include
> +#include
> +#include
> +
> +/**
> + * struct drm_colorop_state - mutable colorop state
> + */
> +struct drm_colorop_state {
> + /** @colorop: backpointer to the colorop */
> + struct drm_colorop *colorop;
> +
> + /* colorop properties */
Can you add a comment like this, so it is clear that not all the fields
are valid and only a few of them will be used.
/*
* Color properties
*
* The following fields are not always valid, their usage depends
* on the colorop type. See their associated comment for more
* information.
*/
> +
> + /** @state: backpointer to global drm_atomic_state */
> + struct drm_atomic_state *state;
> +};
> +
> +/**
> + * struct drm_colorop - DRM color operation control structure
> + *
> + * A colorop represents one color operation. They can be chained via
> + * the 'next' pointer to build a color pipeline.
> + */
> +struct drm_colorop {
> + /** @dev: parent DRM device */
> + struct drm_device *dev;
> +
> + /**
> + * @head:
> + *
> + * List of all colorops on @dev, linked from
> &drm_mode_config.colorop_list.
> + * Invariant over the lifetime of @dev and therefore does not need
> + * locking.
> + */
> + struct list_head head;
> +
> + /**
> + * @index: Position inside the mode_config.list, can be used as an array
> + * index. It is invariant over the lifetime of the colorop.
> + */
> + unsigned index;
> +
> + /** @base: base mode object */
> + struct drm_mode_object base;
> +
> + /**
> + * @plane:
> + *
> + * The plane on which the colorop sits. A drm_colorop is always unique
> + * to a plane.
> + */
> + struct drm_plane *plane;
> +
> + /**
> + * @state:
> + *
> + * Current atomic state for this colorop.
> + *
> + * This is protected by @mutex. Note that nonblocking atomic commits
> + * access the current colorop state without taking locks.
> + */
> + struct drm_colorop_state *state;
> +
> + /* colorop properties */
Maybe the same kind of comment here?
> + /** @properties: property tracking for this colorop */
> + struct drm_object_properties properties;
> +
> +};
> +
[...]
Louis Chauvet
Re: [PATCH v5 00/44] Color Pipeline API w/ VKMS
Le 19/08/24 - 16:56, Harry Wentland a écrit : > This is an RFC set for a color pipeline API, along with implementations > in VKMS and amdgpu. It is tested with a set of IGT tests that can be > found at [1]. The IGT tests run a pixel-by-pixel comparison with an > allowable delta variation as the goal for these transformations is > perceptual correctness, not complete pixel accuracy. > > v5 of this patchset fleshed out documentation for colorops and the > various defines that are being introduced. > > VKMS supports two named transfer function colorops and two matrix > colorops. > > Amdgpu advertises the following pipeline for GPUs with DCN 3 or newer: > > 1. 1D Curve EOTF > 2. 3x4 CTM > 3. Multiplier > 4. 1D Curve Inverse EOTF > 5. 1D LUT > 6. 3D LUT > 7. 1D Curve EOTF > 8. 1D LUT > > The supported curves for the 1D Curve type are: > - sRGB EOTF and its inverse > - PQ EOTF, scaled to [0.0, 125.0] and its inverse > - BT.2020/BT.709 OETF and its inverse > > Note that the 1st and 5th colorops take the EOTF or Inverse > OETF while the 3rd colorop takes the Inverse EOTF or OETF. > > The 3D LUT is a 17^3 tetrahedrally interpolated LUT but the mechanism > exists for other drivers to describe their own 3D LUT capability. > > This mirrors the color pipeline used by gamescope and presented by > Melissa Wen, with the exception of the DEGAM LUT, which is not currently > used. See [1] > https://indico.freedesktop.org/event/4/contributions/186/attachments/138/218/xdc2023-TheRainbowTreasureMap-MelissaWen.pdf > > At this point we're hoping to see gamescope and kwin implementations > take shape. The existing pipeline should be enough to satisfy the > gamescope use-cases on the drm_plane. > > In order to support YUV we'll need to add COLOR_ENCODING and COLOR_RANGE > support to the color pipeline. I have sketched these out already but > don't have it all hooked up yet. This should not hinder adoption of this > API for gaming use-cases. > > We'll also want to advertise IN_FORMATS on a color pipeline as some > color pipelines won't be able to work for all IN_FORMATS on a plane. > Again, I have a sketch but no full implementation yet. This is not > currently required by the AMD color pipeline and could be added after > the merge of this set. > > VKMS patches could still be improved in a few ways, though the > payoff might be limited and I would rather focus on other work > at the moment. The most obvious thing to improve would be to > eliminate the hard-coded LUTs for identity, and sRGB, and replace > them with fixed-point math instead. Hi! I reviewed your VKMS patches and added a few comments in your series. This series looks very good. Thanks for this work, Louis Chauvet > There are plenty of things that I would like to see, but they could > be added after the merge of this patchset: > - COLOR_ENCODING and COLOR_RANGE > - IN_FORMATS for a color pipeline > - Is it possible to support HW which can't bypass entire pipeline? > - Can we do a LOAD / COMMIT model for LUTs (and other properties)? > - read-only scaling colorop which defines scaling taps and position > - named matrices, for things like converting YUV to RGB > - Add custom LUT colorops to VKMS > > IGT tests can be found at [1] or on the igt-dev mailing list. > > A kernel branch can be found at [2]. > > I've also rebased Uma and Chaitanya's patches for the Intel color > pipeline on top of this to show how I envision them to mesh with > my changes. The relevant branches can be found at [3] for the kernel > and [4] for IGT. There were some rebase conflicts in i915 and I'm > not entirely sure I've resolved all of them correctly, but the branch > compiles and shows my thoughts for the new DRM concepts to support > Intel's pipeline. > > [1] > https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/amd-color-pipeline-v5 > [2] > https://gitlab.freedesktop.org/hwentland/linux/-/tree/amd-color-pipeline-v5 > [3] > https://gitlab.freedesktop.org/hwentland/linux/-/tree/amd-intel-color-pipeline-v5 > [4] > https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/amd-intel-color-pipeline-v5 > > > v5: > - amdgpu 3D LUT > - Don't require BYPASS > - update RFC docs and add to TOC tree > - add drm_colorop and COLOR_PIPELINE kernel docs (non-RFC) > - add amdgpu color pipeline doc > - define SIZE property similar to drm_crtc's GAMMA_SIZE > - various minor fixes and cleanups > > v4: > - Add amdgpu color pipeline (WIP) > - Don't block setting of deprecated properties, instead pass client cap >to atomic check so drivers can ignore these props > - Dro
Re: [PATCH v5 18/44] drm/vkms: Use s32 for internal color pipeline precision
Le 19/08/24 - 16:56, Harry Wentland a écrit :
> Certain operations require us to preserve values below 0.0 and
> above 1.0 (0x0 and 0x respectively in 16 bpc unorm). One
> such operation is a BT709 encoding operation followed by its
> decoding operation, or the reverse.
>
> We'll use s32 values as intermediate in and outputs of our
> color operations, for the operations where it matters.
>
> For now this won't apply to LUT operations. We might want to
> update those to work on s32 as well, but it's unclear how
> that should work for unorm LUT definitions. We'll revisit
> that once we add LUT + CTM tests.
>
> In order to allow for this we'll also invert the nesting of our
> colorop processing loops. We now use the pixel iteration loop
> on the outside and the colorop iteration on the inside.
Maybe you can do this inversion on your first commit so it will reduce the
code change here?
> v4:
> - Clarify that we're packing 16-bit UNORM into s32, not
>converting values to a different representation (Pekka)
>
> v3:
> - Use new colorop->next pointer
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/vkms_composer.c | 55 ++--
> drivers/gpu/drm/vkms/vkms_drv.h | 4 ++
> 2 files changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index bc116d16e378..6e939d3a6d5c 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -164,7 +164,7 @@ 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)
> +static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
> {
> struct drm_colorop_state *colorop_state = colorop->state;
>
> @@ -190,24 +190,55 @@ static void apply_colorop(struct pixel_argb_u16 *pixel,
> struct drm_colorop *colo
>
> 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;
> + struct drm_colorop *colorop;
> + struct pixel_argb_s32 pixel;
>
> - while (colorop) {
> - struct drm_colorop_state *colorop_state;
> + for (size_t x = 0; x < output_buffer->n_pixels; x++) {
>
> - if (!colorop)
> - return;
> + /*
> + * Some operations, such as applying a BT709 encoding matrix,
> + * followed by a decoding matrix, require that we preserve
> + * values above 1.0 and below 0.0 until the end of the pipeline.
> + *
> + * Pack the 16-bit UNORM values into s32 to give us head-room to
> + * avoid clipping until we're at the end of the pipeline. Clip
> + * intentionally at the end of the pipeline before packing
> + * UNORM values back into u16.
> + */
> + pixel.a = output_buffer->pixels[x].a;
> + pixel.r = output_buffer->pixels[x].r;
> + pixel.g = output_buffer->pixels[x].g;
> + pixel.b = output_buffer->pixels[x].b;
>
> - colorop_state = colorop->state;
> + colorop = plane_state->base.base.color_pipeline;
> + while (colorop) {
> + struct drm_colorop_state *colorop_state;
>
> - if (!colorop_state)
> - return;
> + if (!colorop)
> + return;
I think this is useless, the while should be sufficient.
> +
> + colorop_state = colorop->state;
> +
> + if (!colorop_state)
> + return;
>
> - for (size_t x = 0; x < output_buffer->n_pixels; x++)
> if (!colorop_state->bypass)
> - apply_colorop(&output_buffer->pixels[x],
> colorop);
> + apply_colorop(&pixel, colorop);
>
> - colorop = colorop->next;
> + colorop = colorop->next;
> + }
> +
> + /* clamp pixel */
> + pixel.a = max(min(pixel.a, 0x), 0x0);
> + pixel.r = max(min(pixel.r, 0x), 0x0);
> + pixel.g = max(min(pixel.g, 0x), 0x0);
> + pixel.b = max(min(pixel.b, 0x), 0x0);
clamp can't be used here? And can't we store the result directly in
output_buffer?
> +
> + /* put back to output_buffer */
> + output_buffer->pixels[x].a = pixel.a;
> + output_buffer->pixels[x].r = pixel.r;
> + output_buffer->pixels[x].g = pixel.g;
> + output_buffer->pixels[x].b = pixel.b;
>
> }
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 278cf35
Re: [PATCH v5 41/44] drm/colorop: allow non-bypass colorops
Le 19/08/24 - 16:57, Harry Wentland a écrit : > Not all HW will be able to do bypass on all color > operations. Introduce an 'allow_bypass' boolean for > all colorop init functions and only create the BYPASS > property when it's true. You did not change the documentation of struct drm_colorop_state, so can we expect state->bypass to be always valid (ie. false when bypass is not possible)? I don't think so, because drm_atomic_helper_colorop_duplicate_state / drm_colorop_reset are setting the bypass to true. Maybe you can add something like this? /** * @bypass: * * When the property BYPASS exists on this colorop, this stores * the requested bypass state: true if colorop shall be bypassed, * false if colorop is enabled. */ [...]
Re: [PATCH v5 19/44] drm/vkms: add 3x4 matrix in color pipeline
Le 19/08/24 - 16:56, Harry Wentland a écrit :
> We add two 3x4 matrices into the VKMS color pipeline. The reason
> we're adding matrices is so that we can test that application
> of a matrix and its inverse yields an output equal to the input
> image.
>
> One complication with the matrix implementation has to do with
> the fact that the matrix entries are in signed-magnitude fixed
> point, whereas the drm_fixed.h implementation uses 2s-complement.
> The latter one is the one that we want for easy addition and
> subtraction, so we convert all entries to 2s-complement.
Is there a reason to use signed-magnitude and not 2s-complement here? I
did not read the whole amd driver, but it seems that the matrix is always
converted to fixed point notation (amdgpu_dm_fixpt_from_s3132 in
amdgpu_dm_color.c). It may reduce the complexity here and in the amd
driver too.
>
> Signed-off-by: Harry Wentland
> ---
> drivers/gpu/drm/vkms/vkms_colorop.c | 32 +++-
> drivers/gpu/drm/vkms/vkms_composer.c | 27 +++
> 2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> index f61dfde47156..adcb08153a09 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -37,7 +37,37 @@ static int vkms_initialize_color_pipeline(struct drm_plane
> *plane, struct drm_pr
>
> prev_op = op;
>
> - /* 2nd op: 1d curve */
> + /* 2nd op: 3x4 matrix */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
Same as before, don't we leak memory/properties here?
> + ret = drm_colorop_ctm_3x4_init(dev, op, plane);
> + if (ret)
> + return ret;
> +
> + drm_colorop_set_next_property(prev_op, op);
> +
> + prev_op = op;
> +
> + /* 3rd op: 3x4 matrix */
> + op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!op) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + return -ENOMEM;
> + }
> +
> + ret = drm_colorop_ctm_3x4_init(dev, op, plane);
> + if (ret)
> + return ret;
> +
> + drm_colorop_set_next_property(prev_op, op);
> +
> + prev_op = op;
> +
> + /* 4th op: 1d curve */
> op = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> if (!op) {
> DRM_ERROR("KMS: Failed to allocate colorop\n");
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 6e939d3a6d5c..bd1df06ced85 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -164,6 +164,30 @@ static void apply_lut(const struct vkms_crtc_state
> *crtc_state, struct line_buff
> }
> }
>
> +static void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct
> drm_color_ctm_3x4 *matrix)
> +{
> + s64 rf, gf, bf;
> +
> + rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[3]);
> +
> + gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[7]);
> +
> + bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]),
> drm_int2fixp(pixel->r)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]),
> drm_int2fixp(pixel->g)) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]),
> drm_int2fixp(pixel->b)) +
> + drm_sm2fixp(matrix->matrix[11]);
> +
> + pixel->r = drm_fixp2int(rf);
> + pixel->g = drm_fixp2int(gf);
> + pixel->b = drm_fixp2int(bf);
There is no need to round here, like done in [1]?
I don't know if the performance improvment is huge, bug maybe you can
precompute drm_int2fixp(pixel->r/g/b)?
[1]: https://lore.kernel.org/all/20240802-yuv-v9-12-08a706669...@bootlin.com/
> +}
> +
> static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
> {
> struct drm_colorop_state *colorop_state = colorop->state;
> @@ -184,6 +208,9 @@ static void apply_colorop(struct pixel_argb_s32 *pixel,
> struct drm_colorop *colo
> DRM_DEBUG_DRIVER("unkown colorop 1D curve type
> %d\n", colorop_state->curve_1d_type);
> break;
> }
> + } else if (colorop->type == DRM_COLOROP_CTM_3X4) {
> + if (colorop_state->data)
> + apply_3x4_matrix(pixel, (struct drm_color_ctm_3x4 *)
> colorop_state->data->data);
> }
>
Re: [PATCH v6 18/44] drm/vkms: Use s32 for internal color pipeline precision
On 03/10/24 - 16:01, Harry Wentland wrote:
> Certain operations require us to preserve values below 0.0 and
> above 1.0 (0x0 and 0x respectively in 16 bpc unorm). One
> such operation is a BT709 encoding operation followed by its
> decoding operation, or the reverse.
>
> We'll use s32 values as intermediate in and outputs of our
> color operations, for the operations where it matters.
>
> For now this won't apply to LUT operations. We might want to
> update those to work on s32 as well, but it's unclear how
> that should work for unorm LUT definitions. We'll revisit
> that once we add LUT + CTM tests.
>
> In order to allow for this we'll also invert the nesting of our
> colorop processing loops. We now use the pixel iteration loop
> on the outside and the colorop iteration on the inside.
>
> Signed-off-by: Harry Wentland
> ---
> v6:
> - use clamp_val instead of manual clamping (Louis Chauvet)
Perfect!
> v4:
> - Clarify that we're packing 16-bit UNORM into s32, not
>converting values to a different representation (Pekka)
>
> v3:
> - Use new colorop->next pointer
>
> drivers/gpu/drm/vkms/vkms_composer.c | 27 +--
> drivers/gpu/drm/vkms/vkms_drv.h | 4
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index b4aaad2bf45f..01fa81ebc93b 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -159,7 +159,7 @@ 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)
> +static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
I agree with this change, but I think we may face conversion issues
in apply_lut_to_channel_value, as it takes u16 and returns u16, but with
your change, you pass s32 and expect s32.
If it is not an issue: Reviewed-by: Louis Chauvet
> {
> struct drm_colorop_state *colorop_state = colorop->state;
>
> @@ -185,9 +185,26 @@ static void apply_colorop(struct pixel_argb_u16 *pixel,
> struct drm_colorop *colo
>
> static void pre_blend_color_transform(const struct vkms_plane_state
> *plane_state, struct line_buffer *output_buffer)
> {
> + struct pixel_argb_s32 pixel;
> +
> for (size_t x = 0; x < output_buffer->n_pixels; x++) {
> struct drm_colorop *colorop =
> plane_state->base.base.color_pipeline;
>
> + /*
> + * Some operations, such as applying a BT709 encoding matrix,
> + * followed by a decoding matrix, require that we preserve
> + * values above 1.0 and below 0.0 until the end of the pipeline.
> + *
> + * Pack the 16-bit UNORM values into s32 to give us head-room to
> + * avoid clipping until we're at the end of the pipeline. Clip
> + * intentionally at the end of the pipeline before packing
> + * UNORM values back into u16.
> + */
> + pixel.a = output_buffer->pixels[x].a;
> + pixel.r = output_buffer->pixels[x].r;
> + pixel.g = output_buffer->pixels[x].g;
> + pixel.b = output_buffer->pixels[x].b;
> +
> while (colorop) {
> struct drm_colorop_state *colorop_state;
>
> @@ -197,10 +214,16 @@ static void pre_blend_color_transform(const struct
> vkms_plane_state *plane_state
> return;
>
> if (!colorop_state->bypass)
> - apply_colorop(&output_buffer->pixels[x],
> colorop);
> + apply_colorop(&pixel, colorop);
>
> colorop = colorop->next;
> }
> +
> + /* clamp values */
> + output_buffer->pixels[x].a = clamp_val(pixel.a, 0, 0x);
> + output_buffer->pixels[x].r = clamp_val(pixel.r, 0, 0x);
> + output_buffer->pixels[x].g = clamp_val(pixel.g, 0, 0x);
> + output_buffer->pixels[x].b = clamp_val(pixel.b, 0, 0x);
> }
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 278cf3533f58..b78bc2611746 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -36,6 +36,10 @@ struct vkms_frame_info {
> unsigned int cpp;
> };
>
> +struct pixel_argb_s32 {
> + s32 a, r, g, b;
> +};
> +
> struct pixel_argb_u16 {
> u16 a, r, g, b;
> };
> --
> 2.46.2
>
Re: [PATCH v6 21/44] drm/vkms: Add tests for CTM handling
On 03/10/24 - 16:01, Harry Wentland wrote:
> A whole slew of tests for CTM handling that greatly helped in
> debugging the CTM code. The extent of tests might seem a bit
> silly but they're fast and might someday help save someone
> else's day when debugging this.
>
> Signed-off-by: Harry Wentland
Reviewed-by: Louis Chauvet
> ---
>
> v6:
> - update reference values since we're now rounding
>
> v5:
> - Make apply_3x4_matrix static
>
> v4:
> - Comment on origin of bt709_enc matrix (Pekka)
> - Use full opaque alpha (Pekka)
> - Add additional check for Y < 0x (Pekka)
> - Remove unused code (Pekka)
> - Rename red, green, blue to Y, U, V where applicable
>
> drivers/gpu/drm/vkms/tests/vkms_color_test.c | 250 +++
> drivers/gpu/drm/vkms/vkms_composer.c | 3 +-
> drivers/gpu/drm/vkms/vkms_composer.h | 1 +
> 3 files changed, 253 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> index c36e67c7909e..d5eb1e4e9b67 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> @@ -187,11 +187,261 @@ static void vkms_color_srgb_inv_srgb(struct kunit
> *test)
> }
> }
>
> +#define FIXPT_HALF(DRM_FIXED_ONE >> 1)
> +#define FIXPT_QUARTER (DRM_FIXED_ONE >> 2)
> +
> +const struct drm_color_ctm_3x4 test_matrix_3x4_50_desat = { {
> + FIXPT_HALF, FIXPT_QUARTER, FIXPT_QUARTER, 0,
> + FIXPT_QUARTER, FIXPT_HALF, FIXPT_QUARTER, 0,
> + FIXPT_QUARTER, FIXPT_QUARTER, FIXPT_HALF, 0
> +} };
> +
> +static void vkms_color_ctm_3x4_50_desat(struct kunit *test)
> +{
> + struct pixel_argb_s32 ref, out;
> +
> + /* full white */
> + ref.a = 0x;
> + ref.r = 0x;
> + ref.g = 0x;
> + ref.b = 0x;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* full black */
> + ref.a = 0x;
> + ref.r = 0x0;
> + ref.g = 0x0;
> + ref.b = 0x0;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* 50% grey */
> + ref.a = 0x;
> + ref.r = 0x8000;
> + ref.g = 0x8000;
> + ref.b = 0x8000;
> +
> + memcpy(&out, &ref, sizeof(out));
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +
> + /* full red to 50% desat */
> + ref.a = 0x;
> + ref.r = 0x8000;
> + ref.g = 0x4000;
> + ref.b = 0x4000;
> +
> + out.a = 0x;
> + out.r = 0x;
> + out.g = 0x0;
> + out.b = 0x0;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_50_desat);
> +
> + KUNIT_EXPECT_MEMEQ(test, &ref, &out, sizeof(out));
> +}
> +
> +/*
> + * BT.709 encoding matrix
> + *
> + * Values printed from within IGT when converting
> + * igt_matrix_3x4_bt709_enc to the fixed-point format expected
> + * by DRM/KMS.
> + */
> +const struct drm_color_ctm_3x4 test_matrix_3x4_bt709_enc = { {
> + 0x366cf400ull, 0xb7175900ull, 0x000127bb300ull, 0,
> + 0x80001993b3a0ull, 0x80005609fe80ull, 0x6f9db200ull, 0,
> + 0x9d70a400ull, 0x80008f011100ull, 0x8e6f9330ull, 0
> +} };
> +
> +static void vkms_color_ctm_3x4_bt709(struct kunit *test)
> +{
> + struct pixel_argb_s32 out;
> +
> + /* full white to bt709 */
> + out.a = 0x;
> + out.r = 0x;
> + out.g = 0x;
> + out.b = 0x;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_bt709_enc);
> +
> + /* Y 255 */
> + KUNIT_EXPECT_GT(test, out.r, 0xfe00);
> + KUNIT_EXPECT_LT(test, out.r, 0x1);
> +
> + /* U 0 */
> + KUNIT_EXPECT_LT(test, out.g, 0x0100);
> +
> + /* V 0 */
> + KUNIT_EXPECT_LT(test, out.b, 0x0100);
> +
> + /* full black to bt709 */
> + out.a = 0x;
> + out.r = 0x0;
> + out.g = 0x0;
> + out.b = 0x0;
> +
> + apply_3x4_matrix(&out, &test_matrix_3x4_bt709_enc);
> +
> + /* Y 0 */
> + KUNIT_EXPECT_LT(test, out.r, 0x100);
> +
> + /* U 0 */
> + KUNIT_EXPECT_LT(test, out.g, 0x0100);
> +
> + /* V 0 */
> + KUNIT_EXPECT_LT(test, out.b, 0x01
Re: [PATCH v6 41/44] drm/colorop: allow non-bypass colorops
On 03/10/24 - 16:01, Harry Wentland wrote: > Not all HW will be able to do bypass on all color > operations. Introduce an 'allow_bypass' boolean for > all colorop init functions and only create the BYPASS > property when it's true. > > Signed-off-by: Harry Wentland > --- > .../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 22 +--- > drivers/gpu/drm/drm_atomic.c | 3 +- > drivers/gpu/drm/drm_colorop.c | 51 --- > drivers/gpu/drm/vkms/vkms_colorop.c | 8 +-- > include/drm/drm_colorop.h | 10 ++-- > 5 files changed, 60 insertions(+), 34 deletions(-) > [...] > --- a/drivers/gpu/drm/vkms/vkms_colorop.c > +++ b/drivers/gpu/drm/vkms/vkms_colorop.c > @@ -31,7 +31,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane > *plane, struct drm_pr > goto cleanup; > } > > - ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs); > + ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs, > true); > if (ret) > goto cleanup; > > @@ -48,7 +48,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane > *plane, struct drm_pr > goto cleanup; > } > > - ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane); > + ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane, true); > if (ret) > goto cleanup; > > @@ -64,7 +64,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane > *plane, struct drm_pr > goto cleanup; > } > > - ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane); > + ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane, true); > if (ret) > goto cleanup; > > @@ -80,7 +80,7 @@ static int vkms_initialize_color_pipeline(struct drm_plane > *plane, struct drm_pr > goto cleanup; > } > > - ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs); > + ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs, > true); > if (ret) > goto cleanup; You allow the bypass here, but you forgot to add a check in apply_colorop to bypass the colorop when this is set. It seems to be the case in the AMD driver too. Or maybe you wanted to pass false in "allow_bypass"? > > diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h > index d3a00296973d..b8c1c4da3444 100644 > --- a/include/drm/drm_colorop.h > +++ b/include/drm/drm_colorop.h > @@ -333,14 +333,16 @@ static inline struct drm_colorop > *drm_colorop_find(struct drm_device *dev, > } > > int drm_colorop_curve_1d_init(struct drm_device *dev, struct drm_colorop > *colorop, > - struct drm_plane *plane, u64 supported_tfs); > + struct drm_plane *plane, u64 supported_tfs, > + bool allow_bypass); > int drm_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_colorop > *colorop, > struct drm_plane *plane, uint32_t lut_size, > - enum drm_colorop_lut1d_interpolation_type > lut1d_interpolation); > + enum drm_colorop_lut1d_interpolation_type > lut1d_interpolation, > + bool allow_bypass); > int drm_colorop_ctm_3x4_init(struct drm_device *dev, struct drm_colorop > *colorop, > - struct drm_plane *plane); > + struct drm_plane *plane, bool allow_bypass); > int drm_colorop_mult_init(struct drm_device *dev, struct drm_colorop > *colorop, > - struct drm_plane *plane); > + struct drm_plane *plane, bool allow_bypass); > > struct drm_colorop_state * > drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop); > -- > 2.46.2 >
Re: [PATCH v6 03/44] drm/vkms: Add kunit tests for VKMS LUT handling
On 03/10/24 - 16:00, Harry Wentland wrote:
> Debugging LUT math is much easier when we can unit test
> it. Add kunit functionality to VKMS and add tests for
> - get_lut_index
> - lerp_u16
>
> Signed-off-by: Harry Wentland
> Cc: Arthur Grillo
> ---
> v6:
> - Eliminate need to include test as .c file (Louis Chauvet)
Thanks for the modification!
With the checkpatch warning fixed:
Reviewed-by: Louis Chauvet
> v5:
> - Bring back static for lerp_u16 and get_lut_index (Arthur)
>
> v4:
> - Test the critical points of the lerp function (Pekka)
>
> v3:
> - Use include way of testing static functions (Arthur)
>
> drivers/gpu/drm/vkms/Kconfig | 15 ++
> drivers/gpu/drm/vkms/Makefile| 1 +
> drivers/gpu/drm/vkms/tests/.kunitconfig | 4 +
> drivers/gpu/drm/vkms/tests/Makefile | 3 +
> drivers/gpu/drm/vkms/tests/vkms_color_test.c | 168 +++
> drivers/gpu/drm/vkms/vkms_composer.c | 10 +-
> drivers/gpu/drm/vkms/vkms_composer.h | 13 ++
> 7 files changed, 211 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
> create mode 100644 drivers/gpu/drm/vkms/tests/Makefile
> create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_test.c
> create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
>
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..59c4a32adb9d 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -13,3 +13,18 @@ config DRM_VKMS
> a VKMS.
>
> If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> + tristate "KUnit tests for VKMS." if !KUNIT_ALL_TESTS
> + depends on DRM_VKMS=y && KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds unit tests for VKMS. This option is not useful for
> + distributions or general kernels, but only for kernel
> + developers working on VKMS.
> +
> + For more information on KUnit and unit tests in general,
> + please refer to the KUnit documentation in
> + Documentation/dev-tools/kunit/.
> +
> + If in doubt, say "N".
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 1b28a6a32948..8d3e46dde635 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -9,3 +9,4 @@ vkms-y := \
> vkms_writeback.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
> diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig
> b/drivers/gpu/drm/vkms/tests/.kunitconfig
> new file mode 100644
> index ..70e378228cbd
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_VKMS=y
> +CONFIG_DRM_VKMS_KUNIT_TESTS=y
> diff --git a/drivers/gpu/drm/vkms/tests/Makefile
> b/drivers/gpu/drm/vkms/tests/Makefile
> new file mode 100644
> index ..7876ca7a3c42
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_color_test.o
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> new file mode 100644
> index ..efe139978860
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
checkpatch ask for a // comment here for the SPDX license (If you know,
why checkpatch ask for a // in .c but /* */ in .h?)
> +
> +#include
> +
> +#include
> +#include
> +#include "../vkms_drv.h"
> +#include "../vkms_composer.h"
> +
> +#define TEST_LUT_SIZE 16
> +
> +MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
> +
> +static struct drm_color_lut test_linear_array[TEST_LUT_SIZE] = {
> + { 0x0, 0x0, 0x0, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> + { 0x, 0x, 0x, 0 },
> +};
> +
> +const struct vkms
Re: [PATCH v6 20/44] drm/tests: Add a few tests around drm_fixed.h
Few checkpatch warnings:
- line length of 103 exceeds 100 columns
- Blank lines aren't necessary before a close brace '}'
- adding a line without newline at end of file
With those fixed:
Reviewd-by: Louis Chauvet
On 03/10/24 - 16:01, Harry Wentland wrote:
> While working on the CTM implementation of VKMS I had to ascertain
> myself of a few assumptions. One of those is whether drm_fixed.h
> treats its numbers using signed-magnitude or twos-complement. It is
> twos-complement.
>
> In order to make someone else's day easier I am adding the
> drm_test_int2fixp test that validates the above assumption.
>
> I am also adding a test for the new sm2fixp function that converts
> from a signed-magnitude fixed point to the twos-complement fixed
> point.
>
> Signed-off-by: Harry Wentland
> ---
>
> v6:
> - add missing MODULE_DESCRIPTION (Jeff Johnson)
> - fix buffer overflow
>
> drivers/gpu/drm/tests/Makefile| 3 +-
> drivers/gpu/drm/tests/drm_fixp_test.c | 70 +++
> 2 files changed, 72 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/tests/drm_fixp_test.c
>
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index 56dab563abd7..bd69df0eee33 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> drm_modes_test.o \
> drm_plane_helper_test.o \
> drm_probe_helper_test.o \
> - drm_rect_test.o
> + drm_rect_test.o \
> + drm_fixp_test.o
>
> CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/gpu/drm/tests/drm_fixp_test.c
> b/drivers/gpu/drm/tests/drm_fixp_test.c
> new file mode 100644
> index ..24a801cf38be
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_fixp_test.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + */
> +
> +#include
> +#include
> +
> +static void drm_test_sm2fixp(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test, 0x7fffll, ((1ull << 63) - 1));
> +
> + /* 1 */
> + KUNIT_EXPECT_EQ(test, drm_int2fixp(1), drm_sm2fixp(1ull <<
> DRM_FIXED_POINT));
> +
> + /* -1 */
> + KUNIT_EXPECT_EQ(test, drm_int2fixp(-1), drm_sm2fixp((1ull << 63) |
> (1ull << DRM_FIXED_POINT)));
> +
> + /* 0.5 */
> + KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(1, 2), drm_sm2fixp(1ull <<
> (DRM_FIXED_POINT - 1)));
> +
> + /* -0.5 */
> + KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(-1, 2), drm_sm2fixp((1ull
> << 63) | (1ull << (DRM_FIXED_POINT - 1;
> +
> +}
> +
> +static void drm_test_int2fixp(struct kunit *test)
> +{
> + /* 1 */
> + KUNIT_EXPECT_EQ(test, 1ll << 32, drm_int2fixp(1));
> +
> + /* -1 */
> + KUNIT_EXPECT_EQ(test, -(1ll << 32), drm_int2fixp(-1));
> +
> + /* 1 + (-1) = 0 */
> + KUNIT_EXPECT_EQ(test, 0, drm_int2fixp(1) + drm_int2fixp(-1));
> +
> + /* 1 / 2 */
> + KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(1, 2));
> +
> + /* -0.5 */
> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(-1, 2));
> +
> + /* (1 / 2) + (-1) = 0.5 */
> + KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(-1, 2) +
> drm_int2fixp(1));
> +
> + /* (1 / 2) - 1) = 0.5 */
> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2) +
> drm_int2fixp(-1));
> +
> + /* (1 / 2) - 1) = 0.5 */
> + KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2) -
> drm_int2fixp(1));
> +
> +}
> +
> +static struct kunit_case drm_fixp_tests[] = {
> + KUNIT_CASE(drm_test_int2fixp),
> + KUNIT_CASE(drm_test_sm2fixp),
> + { }
> +};
> +
> +static struct kunit_suite drm_rect_test_suite = {
> + .name = "drm_fixp",
> + .test_cases = drm_fixp_tests,
> +};
> +
> +kunit_test_suite(drm_rect_test_suite);
> +
> +MODULE_AUTHOR("AMD");
> +MODULE_LICENSE("GPL and additional rights");
> +MODULE_DESCRIPTION("Unit tests for drm_fixed.h");
> \ No newline at end of file
> --
> 2.46.2
>
Re: [PATCH v6 19/44] drm/vkms: add 3x4 matrix in color pipeline
On 03/10/24 - 16:01, Harry Wentland wrote:
> We add two 3x4 matrices into the VKMS color pipeline. The reason
> we're adding matrices is so that we can test that application
> of a matrix and its inverse yields an output equal to the input
> image.
>
> One complication with the matrix implementation has to do with
> the fact that the matrix entries are in signed-magnitude fixed
> point, whereas the drm_fixed.h implementation uses 2s-complement.
> The latter one is the one that we want for easy addition and
> subtraction, so we convert all entries to 2s-complement.
>
> Signed-off-by: Harry Wentland
> ---
>
> v6:
> - pre-compute colors (Louis Chauvet)
> - round matrix output (Louis Chauvet)
>
> drivers/gpu/drm/vkms/vkms_colorop.c | 34 +++-
> drivers/gpu/drm/vkms/vkms_composer.c | 32 ++
> 2 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> index aebd467c4e61..f0c264820a81 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -12,7 +12,7 @@ static const u64 supported_tfs =
> BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
> BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
>
> -#define MAX_COLOR_PIPELINE_OPS 2
> +#define MAX_COLOR_PIPELINE_OPS 4
>
> static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct
> drm_prop_enum_list *list)
> {
> @@ -48,6 +48,38 @@ static int vkms_initialize_color_pipeline(struct drm_plane
> *plane, struct drm_pr
> goto cleanup;
> }
>
> + ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane);
> + if (ret)
> + goto cleanup;
> +
> + drm_colorop_set_next_property(ops[i-1], ops[i]);
checkpatch: spaces preferred around that '-' (ctx:VxV)
> +
> + i++;
> +
> + /* 3rd op: 3x4 matrix */
> + ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
checkpatch: Prefer kzalloc(sizeof(*ops[i])...) over kzalloc(sizeof(struct
drm_colorop)...)
> + if (!ops[i]) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> + ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane);
> + if (ret)
> + goto cleanup;
> +
> + drm_colorop_set_next_property(ops[i-1], ops[i]);
checkpatch: preferred around that '-' (ctx:VxV)
> +
> + i++;
> +
> + /* 4th op: 1d curve */
> + ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
checkpatch: Prefer kzalloc(sizeof(*ops[i])...) over kzalloc(sizeof(struct
drm_colorop)...)
> + if (!ops[i]) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
> if (ret)
> goto cleanup;
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 01fa81ebc93b..c8b9b9d7f78f 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -159,6 +159,35 @@ static void apply_lut(const struct vkms_crtc_state
> *crtc_state, struct line_buff
> }
> }
>
> +static void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct
> drm_color_ctm_3x4 *matrix)
> +{
> + s64 rf, gf, bf;
> + s64 r, g, b;
> +
> + r = drm_int2fixp(pixel->r);
> + g = drm_int2fixp(pixel->g);
> + b = drm_int2fixp(pixel->b);
> +
> + rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]), r) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]), g) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]), b) +
> + drm_sm2fixp(matrix->matrix[3]);
> +
> + gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]), r) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]), g) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]), b) +
> + drm_sm2fixp(matrix->matrix[7]);
> +
> + bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]), r) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]), g) +
> + drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]), b) +
> + drm_sm2fixp(matrix->matrix[11]);
> +
> + pixel->r = drm_fixp2int_round(rf);
> + pixel->g = drm_fixp2int_round(gf);
> + pixel->b = drm_fixp2int_round(bf);
> +}
> +
> static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
> *colorop)
> {
> struct drm_colorop_state
Re: [PATCH v6 15/44] drm/vkms: Add kunit tests for linear and sRGB LUTs
hesis
> {
> s64 lut_index = get_lut_index(lut, channel_value);
> @@ -150,6 +139,8 @@ static u16 apply_lut_to_channel_value(const struct
> vkms_color_lut *lut, u16 chan
> return lerp_u16(floor_channel_value, ceil_channel_value,
> lut_index & DRM_FIXED_DECIMAL_MASK);
> }
> +EXPORT_SYMBOL_IF_KUNIT(apply_lut_to_channel_value);
> +
>
> static void apply_lut(const struct vkms_crtc_state *crtc_state, struct
> line_buffer *output_buffer)
> {
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.h
> b/drivers/gpu/drm/vkms/vkms_composer.h
> index 9316a053e7d7..67ae09913460 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.h
> +++ b/drivers/gpu/drm/vkms/vkms_composer.h
> @@ -5,9 +5,22 @@
>
> #include
>
> +/*
> + * This enum is related to the positions of the variables inside
> + * `struct drm_color_lut`, so the order of both needs to be the same.
> + */
> +enum lut_channel {
> + LUT_RED = 0,
> + LUT_GREEN,
> + LUT_BLUE,
> + LUT_RESERVED
> +};
> +
Can you declare this enum here in your previous patch, so you don't have
to move it here?
With or without this and the checkpatch warning fixed:
Reviewed-by: Louis Chauvet
> #if IS_ENABLED(CONFIG_KUNIT)
> u16 lerp_u16(u16 a, u16 b, s64 t);
> s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value);
> +u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16
> channel_value,
> +enum lut_channel channel);
> #endif
>
> #endif /* _VKMS_COMPOSER_H_ */
> --
> 2.46.2
>
Re: [PATCH v6 14/44] drm/vkms: Add enumerated 1D curve colorop
On 03/10/24 - 16:00, Harry Wentland 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.
>
> Signed-off-by: Harry Wentland
> Signed-off-by: Alex Hung
> ---
>
> v6:
> - drop 'len' var (Louis Chauvet)
> - cleanup if colorop alloc or init fails (Louis Chauvet)
> - switch loop in pre_blend_transform (Louis Chauvet)
> - drop extraneous if (colorop) inside while (colorop) (Louis Chauvet)
thanks!
> v5:
> - Squash with "Pull apply_colorop out of pre_blend_color_transform"
>(Sebastian)
> - Fix warnings
> - Fix include
> - Drop TODOs
>
> 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
>
> drivers/gpu/drm/vkms/Makefile| 4 +-
> drivers/gpu/drm/vkms/vkms_colorop.c | 81 +++
> drivers/gpu/drm/vkms/vkms_composer.c | 49 ++
> 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 8d3e46dde635..0bf3c116f1ae 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -6,7 +6,9 @@ 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
> obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> new file mode 100644
> index ..aebd467c4e61
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
checkpatch: Improper SPDX comment style for
'drivers/gpu/drm/vkms/vkms_colorop.c', please use '//' instead
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "vkms_drv.h"
> +
> +static const u64 supported_tfs =
> + BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
> + BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
> +
> +#define MAX_COLOR_PIPELINE_OPS 2
> +
> +static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct
> drm_prop_enum_list *list)
> +{
> + struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
> + struct drm_device *dev = plane->dev;
> + int ret;
> + int i = 0;
> +
> + memset(ops, 0, sizeof(ops));
> +
> + /* 1st op: 1d curve */
> + ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
checkpatch: Prefer kzalloc(sizeof(*ops[i])...) over kzalloc(sizeof(struct
drm_colorop)...)
> + if (!ops[i]) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> + ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
> + if (ret)
> + goto cleanup;
> +
> + list->type = ops[i]->base.id;
> + list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d",
> ops[i]->base.id);
> +
> + i++;
> +
> + /* 2nd op: 1d curve */
> + ops[i] = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
ditto
> + if (!ops[i]) {
> + DRM_ERROR("KMS: Failed to allocate colorop\n");
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> + ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
> + if
Re: [V7 07/45] drm/colorop: Add 1D Curve subtype
Le 28/02/2025 à 02:07, Alex Hung a écrit :
On 2/25/25 03:13, Louis Chauvet wrote:
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
Add a new drm_colorop with DRM_COLOROP_1D_CURVE with two subtypes:
DRM_COLOROP_1D_CURVE_SRGB_EOTF and DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF.
Signed-off-by: Harry Wentland
Co-developed-by: Alex Hung
Signed-off-by: Alex Hung
---
v5:
- Add drm_get_colorop_curve_1d_type_name in header
- Add drm_colorop_init
- Set default curve
- Add kernel docs
v4:
- Use drm_colorop_curve_1d_type_enum_list to get name (Pekka)
- Create separate init function for 1D curve
- Pass supported TFs into 1D curve init function
drivers/gpu/drm/drm_atomic_uapi.c | 18 ++--
drivers/gpu/drm/drm_colorop.c | 134 ++
include/drm/drm_colorop.h | 60 +
3 files changed, 207 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/
drm_atomic_uapi.c
index 59fc25b59100..9a5dbf0a1306 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -648,11 +648,17 @@ static int
drm_atomic_colorop_set_property(struct drm_colorop *colorop,
struct drm_colorop_state *state, struct drm_file *file_priv,
struct drm_property *property, uint64_t val)
{
- drm_dbg_atomic(colorop->dev,
- "[COLOROP:%d] unknown property [PROP:%d:%s]]\n",
- colorop->base.id,
- property->base.id, property->name);
- return -EINVAL;
+ if (property == colorop->curve_1d_type_property) {
+ state->curve_1d_type = val;
+ } else {
+ drm_dbg_atomic(colorop->dev,
+ "[COLOROP:%d:%d] unknown property [PROP:%d:%s]]\n",
+ colorop->base.id, colorop->type,
+ property->base.id, property->name);
+ return -EINVAL;
+ }
+
+ return 0;
}
static int
@@ -662,6 +668,8 @@ drm_atomic_colorop_get_property(struct drm_colorop
*colorop,
{
if (property == colorop->type_property) {
*val = colorop->type;
+ } else if (property == colorop->curve_1d_type_property) {
+ *val = state->curve_1d_type;
} else {
return -EINVAL;
}
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/
drm_colorop.c
index 1459a28c7e7b..a42de0aa48e1 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -31,6 +31,123 @@
#include "drm_crtc_internal.h"
+static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
+ { DRM_COLOROP_1D_CURVE, "1D Curve" },
+};
+
+static const char * const colorop_curve_1d_type_names[] = {
+ [DRM_COLOROP_1D_CURVE_SRGB_EOTF] = "sRGB EOTF",
+ [DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF] = "sRGB Inverse EOTF",
+};
+
+
+/* Init Helpers */
+
+static int drm_colorop_init(struct drm_device *dev, struct
drm_colorop *colorop,
+ struct drm_plane *plane, enum drm_colorop_type type)
+{
+ struct drm_mode_config *config = &dev->mode_config;
+ struct drm_property *prop;
+ int ret = 0;
+
+ ret = drm_mode_object_add(dev, &colorop->base,
DRM_MODE_OBJECT_COLOROP);
+ if (ret)
+ return ret;
+
+ colorop->base.properties = &colorop->properties;
+ colorop->dev = dev;
+ colorop->type = type;
+ colorop->plane = plane;
+
+ list_add_tail(&colorop->head, &config->colorop_list);
+ colorop->index = config->num_colorop++;
+
+ /* add properties */
+
+ /* type */
+ prop = drm_property_create_enum(dev,
+ DRM_MODE_PROP_IMMUTABLE,
+ "TYPE", drm_colorop_type_enum_list,
+ ARRAY_SIZE(drm_colorop_type_enum_list));
I think this function belongs to the previous patch "Add TYPE property".
This function is only called by the first colorop. Some pieces of the
code in this function are introduced with the first colorop (1D curve)
so it makes sense to include it here.
True! I did not saw it, you can keep it here indeed
+
+ if (!prop)
+ return -ENOMEM;
+
+ colorop->type_property = prop;
+
+ drm_object_attach_property(&colorop->base,
+ colorop->type_property,
+ colorop->type);
+
+ return ret;
+}
+
+/**
+ * drm_colorop_curve_1d_init - Initialize a DRM_COLOROP_1D_CURVE
+ *
+ * @dev: DRM device
+ * @colorop: The drm_colorop object to initialize
+ * @plane: The associated drm_plane
+ * @supported_tfs: A bitfield of supported drm_colorop_curve_1d_init
enum values,
+ * created using BIT(curve_type) and combined with
the OR '|'
+ * operator.
+ * @return zero on success, -E value on failure
+ */
+int drm_colorop_curve_1d_init(struct drm_device *dev, struct
drm_colorop *colorop,
+ str
Re: [V7 02/45] drm/vkms: Round fixp2int conversion in lerp_u16
Le 20/12/2024 à 05:33, Alex Hung a écrit : From: Harry Wentland fixp2int always rounds down, fixp2int_ceil rounds up. We need the new fixp2int_round. Signed-off-by: Alex Hung Signed-off-by: Harry Wentland Reviewed-by: Louis Chauvet Hi, Can I extract this patch from the series and apply it on drm-misc-next? Thanks, Louis Chauvet --- drivers/gpu/drm/vkms/vkms_composer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index e7441b227b3c..3d6785d081f2 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -98,7 +98,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t) s64 delta = drm_fixp_mul(b_fp - a_fp, t); - return drm_fixp2int(a_fp + delta); + return drm_fixp2int_round(a_fp + delta); } static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value) -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [V7 03/45] drm/vkms: Add kunit tests for VKMS LUT handling
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
Debugging LUT math is much easier when we can unit test
it. Add kunit functionality to VKMS and add tests for
- get_lut_index
- lerp_u16
Reviewed-by: Louis Chauvet
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
Cc: Arthur Grillo
Hi,
I would like to take this patch too. Can I take it with the
modifications below:
---
v7:
- Fix checkpatch warnings and errors (Louis Chauvet)
- Change SPDX-License-Identifier: GPL-2.0+ from /* */ to //
- Fix checkpatch errors and warnings (new line at EOF, redundant spaces, and
long lines)
- Add static to const struct vkms_color_lut test_linear_lut
- Add "MODULE_DESCRIPTION" (Jeff Johnson)
v6:
- Eliminate need to include test as .c file (Louis Chauvet)
v5:
- Bring back static for lerp_u16 and get_lut_index (Arthur)
v4:
- Test the critical points of the lerp function (Pekka)
v3:
- Use include way of testing static functions (Arthur)
drivers/gpu/drm/vkms/Kconfig | 15 ++
drivers/gpu/drm/vkms/Makefile| 1 +
drivers/gpu/drm/vkms/tests/.kunitconfig | 4 +
drivers/gpu/drm/vkms/tests/Makefile | 3 +
drivers/gpu/drm/vkms/tests/vkms_color_test.c | 172 +++
drivers/gpu/drm/vkms/vkms_composer.c | 8 +-
drivers/gpu/drm/vkms/vkms_composer.h | 13 ++
7 files changed, 214 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
create mode 100644 drivers/gpu/drm/vkms/tests/Makefile
create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_test.c
create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index b9ecdebecb0b..59c4a32adb9d 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -13,3 +13,18 @@ config DRM_VKMS
a VKMS.
If M is selected the module will be called vkms.
+
+config DRM_VKMS_KUNIT_TESTS
Can I change to:
config DRM_VKMS_KUNIT_TEST
+ tristate "KUnit tests for VKMS." if !KUNIT_ALL_TESTS
Can I change to:
tristate "KUnit tests for VKMS" if !KUNIT_ALL_TESTS
+ depends on DRM_VKMS=y && KUNIT
Can I change to:
depends on DRM_VKMS && KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds unit tests for VKMS. This option is not useful for
+ distributions or general kernels, but only for kernel
+ developers working on VKMS.
+
+ For more information on KUnit and unit tests in general,
+ please refer to the KUnit documentation in
+ Documentation/dev-tools/kunit/.
+
+ If in doubt, say "N".
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 1b28a6a32948..8d3e46dde635 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -9,3 +9,4 @@ vkms-y := \
vkms_writeback.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig
b/drivers/gpu/drm/vkms/tests/.kunitconfig
new file mode 100644
index ..70e378228cbd
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_VKMS=y
+CONFIG_DRM_VKMS_KUNIT_TESTS=y
diff --git a/drivers/gpu/drm/vkms/tests/Makefile
b/drivers/gpu/drm/vkms/tests/Makefile
new file mode 100644
index ..7876ca7a3c42
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_color_test.o
\ No newline at end of file
diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
new file mode 100644
index ..b53beaac2703
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+
+#include
+#include
+#include "../vkms_drv.h"
+#include "../vkms_composer.h"
+
+#define TEST_LUT_SIZE 16
+
+MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
Needs to be changed to: (Can I do it?)
MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
Thanks,
Louis Chauvet
+
+static struct drm_color_lut test_linear_array[TEST_LUT_SIZE] = {
+ { 0x0, 0x0, 0x0, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x, 0x, 0x, 0 },
+ { 0x,
Re: [V7 01/45] drm: Add helper for conversion from signed-magnitude
Le 24/02/2025 à 19:50, Alex Hung a écrit :
On 2/24/25 09:07, Louis Chauvet wrote:
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
CTM values are defined as signed-magnitude values. Add
a helper that converts from CTM signed-magnitude fixed
point value to the twos-complement value used by
drm_fixed.
Signed-off-by: Harry Wentland
Reviewed-by: Louis Chauvet
Hi Louis,
Thanks for reviewing.
The replies to other patches (2, 3, 5, 6, 7, 9, 10) seem to be empty (I
checked on my inbox and on
https://lore.kernel.org/all/44edbdfb-5e23-4c19-8c80-e7acb8b49...@amd.com/T/#m2232bab7c543229a057123c5e762bf49c86a4148)
Did you try to send something which didn't go through?
Hi!
Sorry for this, I clearly don't know what happened, they are also empty
in my Sent folder...
Sorry for this, I will resend them.
Louis Chauvet
---
include/drm/drm_fixed.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 1922188f00e8..0b44f2f294ce 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -78,6 +78,24 @@ static inline u32 dfixed_div(fixed20_12 A,
fixed20_12 B)
#define DRM_FIXED_EPSILON 1LL
#define DRM_FIXED_ALMOST_ONE (DRM_FIXED_ONE - DRM_FIXED_EPSILON)
+/**
+ * @drm_sm2fixp
+ *
+ * Convert a 1.31.32 signed-magnitude fixed point to 32.32
+ * 2s-complement fixed point
+ *
+ * @return s64 2s-complement fixed point
+ */
+static inline s64 drm_sm2fixp(__u64 a)
+{
+ if ((a & (1LL << 63))) {
+ return -(a & 0x7fffll);
+ } else {
+ return a;
+ }
+
+}
+
static inline s64 drm_int2fixp(int a)
{
return ((s64)a) << DRM_FIXED_POINT;
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [V7 05/45] drm/colorop: Introduce new drm_colorop mode object
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
This patches introduces a new drm_colorop mode object. This
object represents color transformations and can be used to
define color pipelines.
We also introduce the drm_colorop_state here, as well as
various helpers and state tracking bits.
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
---
v7:
- Fix checkpatch warnings and errors
- Add a tab to for_each_oldnew_colorop_in_state definition
- Change unsigned index to unsigned int index
- Fix a checkpatch warning - a new line after variable declaration
v6:
- Comment that properties validity depends on type (Louis Chauvet)
v5:
- Add comment to drm_atomic_state.colorops
- Replace a misplaced 'plane' with 'colorop' in comment
- Fix colorop_list kernel doc
- Add kernel doc for color_pipeline
- drop unused drm_colorop_destroy_state
- drop drm_colorop_init, to be introduced in later patch
when used
- Add kernel docs
- Drop TODOs
v4:
- Drop IOCTL definitions (Pekka)
- add missing declaration (Chaitanya Kumar Borah)
v3:
- Drop TODO for lock (it's handled in drm_modeset_drop_locks)
(Melissa)
- Don't get plane state when getting colorop state
- Make some functions static (kernel test robot)
drivers/gpu/drm/Makefile| 1 +
drivers/gpu/drm/drm_atomic.c| 70
drivers/gpu/drm/drm_atomic_helper.c | 12 ++
drivers/gpu/drm/drm_atomic_uapi.c | 48
drivers/gpu/drm/drm_colorop.c | 104 +
drivers/gpu/drm/drm_mode_config.c | 7 ++
include/drm/drm_atomic.h| 89 +++
include/drm/drm_atomic_uapi.h | 1 +
include/drm/drm_colorop.h | 166
include/drm/drm_mode_config.h | 18 +++
include/drm/drm_plane.h | 8 ++
include/uapi/drm/drm_mode.h | 1 +
12 files changed, 525 insertions(+)
create mode 100644 drivers/gpu/drm/drm_colorop.c
create mode 100644 include/drm/drm_colorop.h
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 784229d4504d..055f3e535d15 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -44,6 +44,7 @@ drm-y := \
drm_client.o \
drm_client_modeset.o \
drm_color_mgmt.o \
+ drm_colorop.o \
drm_connector.o \
drm_crtc.o \
drm_displayid.o \
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 0fc99da93afe..327d906c48c5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -42,6 +42,7 @@
#include
#include
#include
+#include
#include "drm_crtc_internal.h"
#include "drm_internal.h"
@@ -107,6 +108,7 @@ void drm_atomic_state_default_release(struct
drm_atomic_state *state)
kfree(state->connectors);
kfree(state->crtcs);
kfree(state->planes);
+ kfree(state->colorops);
kfree(state->private_objs);
}
EXPORT_SYMBOL(drm_atomic_state_default_release);
@@ -138,6 +140,10 @@ drm_atomic_state_init(struct drm_device *dev, struct
drm_atomic_state *state)
sizeof(*state->planes), GFP_KERNEL);
if (!state->planes)
goto fail;
+ state->colorops = kcalloc(dev->mode_config.num_colorop,
+ sizeof(*state->colorops), GFP_KERNEL);
+ if (!state->colorops)
+ goto fail;
/*
* Because drm_atomic_state can be committed asynchronously we need our
@@ -249,6 +255,20 @@ void drm_atomic_state_default_clear(struct
drm_atomic_state *state)
state->planes[i].new_state = NULL;
}
+ for (i = 0; i < config->num_colorop; i++) {
+ struct drm_colorop *colorop = state->colorops[i].ptr;
+
+ if (!colorop)
+ continue;
+
+ drm_colorop_atomic_destroy_state(colorop,
+state->colorops[i].state);
+ state->colorops[i].ptr = NULL;
+ state->colorops[i].state = NULL;
There is no risk of use-after-free between the
drm_colorop_atomic_destroy_state and the state->colorops[i].state?
+ state->colorops[i].old_state = NULL;
+ state->colorops[i].new_state = NULL;
+ }
+
for (i = 0; i < state->num_private_objs; i++) {
struct drm_private_obj *obj = state->private_objs[i].ptr;
@@ -568,6 +588,56 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
}
EXPORT_SYMBOL(drm_atomic_get_plane_state);
+
+/**
+ * drm_atomic_get_colorop_state - get colorop state
+ * @state: global atomic state object
+ * @colorop: colorop to get state object for
+ *
+ * This function returns the colorop state for the given colorop, allocating it
+ * if needed. It
Re: [V7 06/45] drm/colorop: Add TYPE property
Le 20/12/2024 à 05:33, Alex Hung a écrit : From: Harry Wentland Add a read-only TYPE property. The TYPE specifies the colorop type, such as enumerated curve, 1D LUT, CTM, 3D LUT, PWL LUT, etc. For now we're only introducing an enumerated 1D LUT type to illustrate the concept. Signed-off-by: Alex Hung Signed-off-by: Harry Wentland Reviewed-by: Louis Chauvet -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [V7 07/45] drm/colorop: Add 1D Curve subtype
ruct drm_plane *plane, u64 supported_tfs)
+{
+ struct drm_prop_enum_list enum_list[DRM_COLOROP_1D_CURVE_COUNT];
+ int i, len;
+
+ struct drm_property *prop;
+ int ret;
+
+ if (!supported_tfs) {
+ drm_err(dev,
+ "No supported TFs for new 1D curve colorop on
[PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+ return -EINVAL;
+ }
+
+ if ((supported_tfs & -BIT(DRM_COLOROP_1D_CURVE_COUNT)) != 0) {
+ drm_err(dev, "Unknown TF provided on [PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+ return -EINVAL;
+ }
+
+ ret = drm_colorop_init(dev, colorop, plane, DRM_COLOROP_1D_CURVE);
+ if (ret)
+ return ret;
+
+ len = 0;
+ for (i = 0; i < DRM_COLOROP_1D_CURVE_COUNT; i++) {
+ if ((supported_tfs & BIT(i)) == 0)
+ continue;
+
+ enum_list[len].type = i;
+ enum_list[len].name = colorop_curve_1d_type_names[i];
+ len++;
+ }
+
+ if (WARN_ON(len <= 0))
+ return -EINVAL;
+
+
+ /* initialize 1D curve only attribute */
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
"CURVE_1D_TYPE",
+ enum_list, len);
+ if (!prop)
+ return -ENOMEM;
+
+ colorop->curve_1d_type_property = prop;
+ drm_object_attach_property(&colorop->base,
colorop->curve_1d_type_property,
+ enum_list[0].type);
+ drm_colorop_reset(colorop);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_colorop_curve_1d_init);
+
static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop
*colorop,
struct
drm_colorop_state *state)
{
@@ -70,7 +187,16 @@ void drm_colorop_atomic_destroy_state(struct drm_colorop
*colorop,
static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
struct drm_colorop *colorop)
{
+ u64 val;
+
colorop_state->colorop = colorop;
+
+ if (colorop->curve_1d_type_property) {
+ drm_object_property_get_default_value(&colorop->base,
+ colorop->curve_1d_type_property,
+ &val);
+ colorop_state->curve_1d_type = val;
+ }
}
/**
@@ -114,3 +240,11 @@ const char *drm_get_colorop_type_name(enum
drm_colorop_type type
return colorop_type_name[type];
Probably a dumb question: can't we use drm_colorop_type_enum_list
instead of colorop_type_name in the drm_get_colorop_type_name function?
So we will avoid duplicating the string "1D Curve".
}
Thanks,
Louis Chauvet
[...]
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [V7 09/45] drm/colorop: Add BYPASS property
Le 20/12/2024 à 05:33, Alex Hung a écrit : From: Harry Wentland We want to be able to bypass each colorop at all times. Introduce a new BYPASS boolean property for this. Signed-off-by: Alex Hung Signed-off-by: Harry Wentland Reviewed-by: Louis Chauvet -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [V7 10/45] drm/colorop: Add NEXT property
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
We'll construct color pipelines out of drm_colorop by
chaining them via the NEXT pointer. NEXT will point to
the next drm_colorop in the pipeline, or by 0 if we're
at the end of the pipeline.
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
---
v5:
- move next comment here from Add 3x4 CTM patch (Sebastian)
- Add kernel doc
v4:
- Allow setting of NEXT property to NULL (Chaitanya Kumar Borah)
v3:
- Add next pointer to colorop to be used by drivers
and in DRM core
drivers/gpu/drm/drm_colorop.c | 30 ++
include/drm/drm_colorop.h | 20
2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 01cbe90635e8..954acd09673a 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -89,6 +89,7 @@ static int drm_colorop_init(struct drm_device *dev, struct
drm_colorop *colorop,
colorop->dev = dev;
colorop->type = type;
colorop->plane = plane;
+ colorop->next = NULL;
list_add_tail(&colorop->head, &config->colorop_list);
colorop->index = config->num_colorop++;
@@ -121,6 +122,16 @@ static int drm_colorop_init(struct drm_device *dev, struct
drm_colorop *colorop,
colorop->bypass_property,
1);
+ /* next */
+ prop = drm_property_create_object(dev, DRM_MODE_PROP_IMMUTABLE |
DRM_MODE_PROP_ATOMIC,
+ "NEXT", DRM_MODE_OBJECT_COLOROP);
+ if (!prop)
+ return -ENOMEM;
+ colorop->next_property = prop;
+ drm_object_attach_property(&colorop->base,
+ colorop->next_property,
+ 0);
+
return ret;
}
@@ -294,3 +305,22 @@ const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type ty
return colorop_curve_1d_type_names[type];
}
+
+/**
+ * drm_colorop_set_next_property - sets the next pointer
+ * @colorop: drm colorop
+ * @next: next colorop
+ *
+ * Should be used when constructing the color pipeline
+ */
+void drm_colorop_set_next_property(struct drm_colorop *colorop, struct
drm_colorop *next)
+{
+ if (!colorop->next_property)
+ return;
I agree with Simon, a WARN_ON seems appropriate here, the
colorop->next_property should always exist, and if not, it is a bug.
+
+ drm_object_property_set_value(&colorop->base,
+ colorop->next_property,
+ next ? next->base.id : 0);
+ colorop->next = next;
+}
+EXPORT_SYMBOL(drm_colorop_set_next_property);
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 5ed24d60a99e..2f0572be37bb 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -165,6 +165,14 @@ struct drm_colorop {
*/
enum drm_colorop_type type;
+ /**
+* @next:
+*
+* Read-only
+* Pointer to next drm_colorop in pipeline
+*/
+ struct drm_colorop *next;
+
/**
* @type_property:
*
@@ -192,10 +200,20 @@ struct drm_colorop {
*/
struct drm_property *curve_1d_type_property;
+ /**
+* @next_property:
+*
+* Read-only property to next colorop in the pipeline
+*/
+ struct drm_property *next_property;
+
};
#define obj_to_colorop(x) container_of(x, struct drm_colorop, base)
+
+
+
Strange useless lines
Thanks,
Louis Chauvet
/**
* drm_colorop_find - look up a Colorop object from its ID
* @dev: DRM device
@@ -268,4 +286,6 @@ const char *drm_get_colorop_type_name(enum drm_colorop_type
type);
*/
const char *drm_get_colorop_curve_1d_type_name(enum drm_colorop_curve_1d_type
type);
+void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
+
#endif /* __DRM_COLOROP_H__ */
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [V7 06/45] drm/colorop: Add TYPE property
Re: [V7 02/45] drm/vkms: Round fixp2int conversion in lerp_u16
Re: [V7 05/45] drm/colorop: Introduce new drm_colorop mode object
Re: [V7 03/45] drm/vkms: Add kunit tests for VKMS LUT handling
Re: [V7 10/45] drm/colorop: Add NEXT property
Re: [V7 01/45] drm: Add helper for conversion from signed-magnitude
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
CTM values are defined as signed-magnitude values. Add
a helper that converts from CTM signed-magnitude fixed
point value to the twos-complement value used by
drm_fixed.
Signed-off-by: Harry Wentland
Reviewed-by: Louis Chauvet
---
include/drm/drm_fixed.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 1922188f00e8..0b44f2f294ce 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -78,6 +78,24 @@ static inline u32 dfixed_div(fixed20_12 A, fixed20_12 B)
#define DRM_FIXED_EPSILON 1LL
#define DRM_FIXED_ALMOST_ONE (DRM_FIXED_ONE - DRM_FIXED_EPSILON)
+/**
+ * @drm_sm2fixp
+ *
+ * Convert a 1.31.32 signed-magnitude fixed point to 32.32
+ * 2s-complement fixed point
+ *
+ * @return s64 2s-complement fixed point
+ */
+static inline s64 drm_sm2fixp(__u64 a)
+{
+ if ((a & (1LL << 63))) {
+ return -(a & 0x7fffll);
+ } else {
+ return a;
+ }
+
+}
+
static inline s64 drm_int2fixp(int a)
{
return ((s64)a) << DRM_FIXED_POINT;
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [V7 09/45] drm/colorop: Add BYPASS property
Re: [V7 07/45] drm/colorop: Add 1D Curve subtype
Re: [V7 02/45] drm/vkms: Round fixp2int conversion in lerp_u16
Le 25/02/2025 à 12:28, Simon Ser a écrit : On Tuesday, February 25th, 2025 at 10:37, Louis Chauvet wrote: Can I extract this patch from the series and apply it on drm-misc-next? That sounds completely fine by me, and TBH it sounds like it could even be drm-misc-fixes material? Probably yes, I can take it now. We need to be a bit careful when merging patches from the same series via multiple trees. Maybe we'll merge the colorop stuff via the amd tree? I don't remember the rules around trees, and I don't know if it would be fine to merge the vkms changes in that tree as well. (I only remember Simona recommending against merging via multiple trees, because it's painful.) If we can't merge the vkms changes via the amd tree, they will likely need to wait for the amd tree to be merged back in drm-next? If we merge some changes via drm-misc-next, then we won't be able to merge the rest via amd, if the rest depends on these changes. If the drm-*-next branches are synchronized often, I propose to split into 4 series: - 2/45, in drm-misc-fixes (it is a bug) - 3/45, in drm-misc-next (only the vkms part needs it, can be merged just after [1] with minor conflict resolution, which I can do) - 1, 4..13, 21..45, in drm-amd-next - 14..20, in drm-misc-next, once drm-amd-next is merged in drm-misc-next (I don't expect huge conflicts if this is merged "soon", afaik nobody is currently working on the composition algorithm) [1]:https://lore.kernel.org/all/20250218101214.5790-1-jose.exposit...@gmail.com/ Thanks, Louis Chauvet Simon -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [V7 15/45] drm/vkms: Add kunit tests for linear and sRGB LUTs
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
Two tests are added to VKMS LUT handling:
- linear
- inv_srgb
Reviewed-by: Louis Chauvet
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
---
v7:
- Fix checkpatch warnings (Louis Chauvet)
- Adde a commit messages
- Fix code styles by adding and removing spaces (new lines, tabs and so on)
drivers/gpu/drm/vkms/tests/vkms_color_test.c | 39 +++-
drivers/gpu/drm/vkms/vkms_composer.c | 17 ++---
drivers/gpu/drm/vkms/vkms_composer.h | 13 +++
3 files changed, 55 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
index b53beaac2703..d765c5eb5d88 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
@@ -6,6 +6,7 @@
#include
#include "../vkms_drv.h"
#include "../vkms_composer.h"
+#include "../vkms_luts.h"
#define TEST_LUT_SIZE 16
@@ -36,7 +37,6 @@ static const struct vkms_color_lut test_linear_lut = {
.channel_value2index_ratio = 0xf000fll
};
-
static void vkms_color_test_get_lut_index(struct kunit *test)
{
s64 lut_index;
@@ -49,6 +49,19 @@ static void vkms_color_test_get_lut_index(struct kunit *test)
lut_index = get_lut_index(&test_linear_lut,
test_linear_array[i].red);
KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(lut_index), i);
}
+
+ KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&srgb_eotf, 0x0)),
0x0);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf,
0x0)), 0x0);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf,
0x101)), 0x1);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf,
0x202)), 0x2);
+
+ KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&srgb_inv_eotf, 0x0)),
0x0);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf,
0x0)), 0x0);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf,
0x101)), 0x1);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_inv_eotf,
0x202)), 0x2);
+
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf,
0xfefe)), 0xfe);
+ KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&srgb_eotf,
0x)), 0xff);
Did you see the kernel bot warning? I think you can simply add
EXPORT_SYMBOL_IF_KUNIT(srgb_eotf) in vkms_lut.h.
}
static void vkms_color_test_lerp(struct kunit *test)
@@ -155,9 +168,33 @@ static void vkms_color_test_lerp(struct kunit *test)
KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x1, 0x8000), 0x1);
}
+static void vkms_color_test_linear(struct kunit *test)
+{
+ for (int i = 0; i < LUT_SIZE; i++) {
+ int linear = apply_lut_to_channel_value(&linear_eotf, i *
0x101, LUT_RED);
+
+ KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(linear, 0x101), i);
+ }
+}
+
+static void vkms_color_srgb_inv_srgb(struct kunit *test)
+{
+ u16 srgb, final;
+
+ for (int i = 0; i < LUT_SIZE; i++) {
+ srgb = apply_lut_to_channel_value(&srgb_eotf, i * 0x101,
LUT_RED);
+ final = apply_lut_to_channel_value(&srgb_inv_eotf, srgb,
LUT_RED);
+
+ KUNIT_EXPECT_GE(test, final / 0x101, i - 1);
+ KUNIT_EXPECT_LE(test, final / 0x101, i + 1);
+ }
+}
+
static struct kunit_case vkms_color_test_cases[] = {
KUNIT_CASE(vkms_color_test_get_lut_index),
KUNIT_CASE(vkms_color_test_lerp),
+ KUNIT_CASE(vkms_color_test_linear),
+ KUNIT_CASE(vkms_color_srgb_inv_srgb),
{}
};
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 983654540ee5..ee3cfe153d8f 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -113,19 +113,8 @@ VISIBLE_IF_KUNIT s64 get_lut_index(const struct
vkms_color_lut *lut, u16 channel
}
EXPORT_SYMBOL_IF_KUNIT(get_lut_index);
-/*
- * This enum is related to the positions of the variables inside
- * `struct drm_color_lut`, so the order of both needs to be the same.
- */
-enum lut_channel {
- LUT_RED = 0,
- LUT_GREEN,
- LUT_BLUE,
- LUT_RESERVED
-};
-
-static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16
channel_value,
- enum lut_channel channel)
+VISIBLE_IF_KUNIT u16 apply_lut_to_channel_value(const struct vkms_color_lut
*lut, u16 channel_value,
+ enum lut_channel channel)
{
s64 lut_index = get_lut_index(lut, channel_value);
u16 *floor_lut_value, *ceil_lut_value;
@@ -150,6 +139,8 @@ static u16 apply_lut_to_channel_value(const struct
vkms_color_lut *lut, u16 chan
Re: [V7 14/45] drm/vkms: Add enumerated 1D curve colorop
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
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.
Signed-off-by: Harry Wentland
Signed-off-by: Alex Hung
---
v7:
- Fix checkpatch warnings (Louis Chauvet)
- Change kzalloc(sizeof(struct drm_colorop) ...) to kzalloc(sizeof(*ops[i])
...)
- Remove if (ops[i]) before kfree(ops[i])
- Fix styles by adding and removing spaces (new lines, tabs and so on)
v6:
- drop 'len' var (Louis Chauvet)
- cleanup if colorop alloc or init fails (Louis Chauvet)
- switch loop in pre_blend_transform (Louis Chauvet)
- drop extraneous if (colorop) inside while (colorop) (Louis Chauvet)
v5:
- Squash with "Pull apply_colorop out of pre_blend_color_transform"
(Sebastian)
- Fix warnings
- Fix include
- Drop TODOs
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
drivers/gpu/drm/vkms/Makefile| 4 +-
drivers/gpu/drm/vkms/vkms_colorop.c | 82 +++
drivers/gpu/drm/vkms/vkms_composer.c | 50 ++
drivers/gpu/drm/vkms/vkms_drv.h | 3 +
drivers/gpu/drm/vkms/vkms_luts.c | 801 +++
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 8d3e46dde635..0bf3c116f1ae 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -6,7 +6,9 @@ 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
obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
b/drivers/gpu/drm/vkms/vkms_colorop.c
new file mode 100644
index ..af319cd3de23
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+#include
+#include
+#include
+#include
+
+#include "vkms_drv.h"
+
+static const u64 supported_tfs =
+ BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
+ BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
+
+#define MAX_COLOR_PIPELINE_OPS 2
+
+static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct
drm_prop_enum_list *list)
+{
+ struct drm_colorop *ops[MAX_COLOR_PIPELINE_OPS];
+ struct drm_device *dev = plane->dev;
+ int ret;
+ int i = 0;
+
+ memset(ops, 0, sizeof(ops));
+
+ /* 1st op: 1d curve */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ DRM_ERROR("KMS: Failed to allocate colorop\n");
drm_err(plane->dev, "KMS...");
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
+ if (ret)
+ goto cleanup;
+
+ list->type = ops[i]->base.id;
+ list->name = kasprintf(GFP_KERNEL, "Color Pipeline %d",
ops[i]->base.id);
+
+ i++;
+
+ /* 2nd op: 1d curve */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ DRM_ERROR("KMS: Failed to allocate colorop\n");
drm_err
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
+ if (ret)
+ goto cleanup;
+
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
+
+ return 0;
+
+cleanup:
+ for (; i >= 0; i--)
+ kfree(ops[i]);
+
+ return ret;
+}
+
+int vkms_initialize_colorops(struct drm_plane *plane)
+{
+ struct drm_prop_enum_list pipeline;
+ int ret;
+
+ /* Add color pipeline */
+ ret = vkms_initialize_color_pipeline(plane, &pipeline);
+
Re: [V7 02/45] drm/vkms: Round fixp2int conversion in lerp_u16
Le 25/02/2025 à 15:45, Simon Ser a écrit : On Tuesday, February 25th, 2025 at 15:43, Harry Wentland wrote: We need to be a bit careful when merging patches from the same series via multiple trees. Maybe we'll merge the colorop stuff via the amd tree? I don't remember the rules around trees, and I don't know if it would be fine to merge the vkms changes in that tree as well. (I only remember Simona recommending against merging via multiple trees, because it's painful.) If we can't merge the vkms changes via the amd tree, they will likely need to wait for the amd tree to be merged back in drm-next? If we merge some changes via drm-misc-next, then we won't be able to merge the rest via amd, if the rest depends on these changes. If the drm-*-next branches are synchronized often, I propose to split into 4 series: - 2/45, in drm-misc-fixes (it is a bug) - 3/45, in drm-misc-next (only the vkms part needs it, can be merged just after 1 with minor conflict resolution, which I can do) - 1, 4..13, 21..45, in drm-amd-next - 14..20, in drm-misc-next, once drm-amd-next is merged in drm-misc-next (I don't expect huge conflicts if this is merged "soon", afaik nobody is currently working on the composition algorithm) I would think it's easier if it all goes in via drm-misc next, except for patch 2 which can be part of drm-misc-fixes. Alex and I based our branches on drm-misc-next. There shouldn't be major conflicts with drm/amd code but we can check that before merging. That sounds like the simplest solution to me :) So let's go this way! I just applied PATCH 2. I don't feel comfortable merging the whole colorop and AMD parts without broader reviews, but I can definitely apply the VKMS part. -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [V7 16/45] drm/colorop: Add 3x4 CTM type
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
This type is used to support a 3x4 matrix in colorops. A 3x4
matrix uses the last column as a "bias" column. Some HW exposes
support for 3x4. The calculation looks like:
out matrixin
|R| |0 1 2 3 | | R |
|G| = |4 5 6 7 | x | G |
|B| |8 9 10 11| | B |
|1.0|
This is also the first colorop where we need a blob property to
program the property. For that we'll introduce a new DATA
property that can be used by all colorop TYPEs requiring a
blob. The way a DATA blob is read depends on the TYPE of
the colorop.
We only create the DATA property for property types that
need it.
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
---
v6:
- take ref for DATA blob in duplicate_state func (Xaver Hugl)
v5:
- Add function signature for init (Sebastian)
- Fix kernel-doc
v4:
- Create helper function for creating 3x4 CTM colorop
- Fix CTM indexes in comment (Pekka)
drivers/gpu/drm/drm_atomic.c | 14 ++-
drivers/gpu/drm/drm_atomic_uapi.c | 29 +
drivers/gpu/drm/drm_colorop.c | 42 +++
include/drm/drm_colorop.h | 21
include/uapi/drm/amdgpu_drm.h | 9 ---
include/uapi/drm/drm_mode.h | 24 +-
6 files changed, 128 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 35136987d5e8..c58663327e6b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -787,7 +787,19 @@ static void drm_atomic_colorop_print_state(struct
drm_printer *p,
drm_printf(p, "colorop[%u]:\n", colorop->base.id);
drm_printf(p, "\ttype=%s\n", drm_get_colorop_type_name(colorop->type));
drm_printf(p, "\tbypass=%u\n", state->bypass);
- drm_printf(p, "\tcurve_1d_type=%s\n",
drm_get_colorop_curve_1d_type_name(state->curve_1d_type));
+
+ switch (colorop->type) {
+ case DRM_COLOROP_1D_CURVE:
+ drm_printf(p, "\tcurve_1d_type=%s\n",
+
drm_get_colorop_curve_1d_type_name(state->curve_1d_type));
+ break;
+ case DRM_COLOROP_CTM_3X4:
+ drm_printf(p, "\tdata blob id=%d\n", state->data ?
state->data->base.id : 0);
+ break;
+ default:
+ break;
+ }
+
As suggested by Simon, you could add this switch in a previous patch, so
you could avoid editing the same line twice.
With or without this change:
Reviewed-by: Louis Chauvet
drm_printf(p, "\tnext=%d\n", colorop->next ? colorop->next->base.id :
0);
}
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index f0c22abcc28f..7bc4978e5441 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -692,6 +692,30 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
return 0;
}
+static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
+ struct drm_colorop_state *state,
+ struct drm_property *property, uint64_t val)
+{
+ ssize_t elem_size = -1;
+ ssize_t size = -1;
+ bool replaced = false;
+
+ switch (colorop->type) {
+ case DRM_COLOROP_CTM_3X4:
+ size = sizeof(struct drm_color_ctm_3x4);
+ break;
+ default:
+ /* should never get here */
+ return -EINVAL;
+ }
+
+ return drm_property_replace_blob_from_id(colorop->dev,
+ &state->data,
+ val,
+ size,
+ elem_size,
+ &replaced);
+}
static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
struct drm_colorop_state *state, struct drm_file *file_priv,
@@ -701,6 +725,9 @@ static int drm_atomic_colorop_set_property(struct
drm_colorop *colorop,
state->bypass = val;
} else if (property == colorop->curve_1d_type_property) {
state->curve_1d_type = val;
+ } else if (property == colorop->data_property) {
+ return drm_atomic_color_set_data_property(colorop,
+ state, property, val);
} else {
drm_dbg_atomic(colorop->dev,
"[COLOROP:%d:%d] unknown property
[PROP:%d:%s]]\n",
@@ -723,6 +750,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
*val = state->bypass;
} else if (property == colorop->curve_1d_type_property) {
*val = state->curve_1d_type;
+ } els
Re: [V7 18/45] drm/vkms: add 3x4 matrix in color pipeline
Le 20/12/2024 à 05:33, Alex Hung a écrit :
From: Harry Wentland
We add two 3x4 matrices into the VKMS color pipeline. The reason
we're adding matrices is so that we can test that application
of a matrix and its inverse yields an output equal to the input
image.
One complication with the matrix implementation has to do with
the fact that the matrix entries are in signed-magnitude fixed
point, whereas the drm_fixed.h implementation uses 2s-complement.
The latter one is the one that we want for easy addition and
subtraction, so we convert all entries to 2s-complement.
Signed-off-by: Alex Hung
Signed-off-by: Harry Wentland
---
v7:
- Fix checkpatch warnings
- Change kzalloc(sizeof(struct drm_colorop) ...) to kzalloc(sizeof(*ops[i])
...)
- Change i-1to i - 1
- Add a new line at EOF
v6:
- pre-compute colors (Louis Chauvet)
- round matrix output (Louis Chauvet)
drivers/gpu/drm/vkms/vkms_colorop.c | 34 +++-
drivers/gpu/drm/vkms/vkms_composer.c | 33 +++
2 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
b/drivers/gpu/drm/vkms/vkms_colorop.c
index af319cd3de23..b407f8ae758d 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -12,7 +12,7 @@ static const u64 supported_tfs =
BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
-#define MAX_COLOR_PIPELINE_OPS 2
+#define MAX_COLOR_PIPELINE_OPS 4
static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_prop_enum_list *list)
{
@@ -48,6 +48,38 @@ static int vkms_initialize_color_pipeline(struct drm_plane
*plane, struct drm_pr
goto cleanup;
}
+ ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane);
+ if (ret)
+ goto cleanup;
+
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
+
+ i++;
+
+ /* 3rd op: 3x4 matrix */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ DRM_ERROR("KMS: Failed to allocate colorop\n");
drm_err
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_colorop_ctm_3x4_init(dev, ops[i], plane);
+ if (ret)
+ goto cleanup;
+
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
+
+ i++;
+
+ /* 4th op: 1d curve */
+ ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
+ if (!ops[i]) {
+ DRM_ERROR("KMS: Failed to allocate colorop\n");
drm_err
With this change:
Reviewed-by: Louis Chauvet
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
ret = drm_colorop_curve_1d_init(dev, ops[i], plane, supported_tfs);
if (ret)
goto cleanup;
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index b009e607a310..cdcaaf8cdb68 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -159,6 +159,35 @@ static void apply_lut(const struct vkms_crtc_state
*crtc_state, struct line_buff
}
}
+static void apply_3x4_matrix(struct pixel_argb_s32 *pixel, const struct drm_color_ctm_3x4 *matrix)
+{
+ s64 rf, gf, bf;
+ s64 r, g, b;
+
+ r = drm_int2fixp(pixel->r);
+ g = drm_int2fixp(pixel->g);
+ b = drm_int2fixp(pixel->b);
+
+ rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]), r) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]), g) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]), b) +
+drm_sm2fixp(matrix->matrix[3]);
+
+ gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]), r) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]), g) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]), b) +
+drm_sm2fixp(matrix->matrix[7]);
+
+ bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]), r) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]), g) +
+drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]), b) +
+drm_sm2fixp(matrix->matrix[11]);
+
+ pixel->r = drm_fixp2int_round(rf);
+ pixel->g = drm_fixp2int_round(gf);
+ pixel->b = drm_fixp2int_round(bf);
+}
+
static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop
*colorop)
{
struct drm_colorop_state *colorop_state = colorop->state;
@@ -180,6 +209,10 @@ static void apply_colorop(struct pixel_argb_s32 *pixel,
struct drm_colorop *colo
colorop_state->curve_1d_type);
break;
}
+ } else if (colorop->type == DRM_COLOROP_CTM_3X4) {
+ if (colorop_state->data)
+ apply_3x4_matrix(pixel,
+ (struct drm_color_ctm_3x4 *)
colorop_state->data->data);
}
}
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
