by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH v5 04/16] drm/vkms: Add typedef and documentation for pixel_read and pixel_write functions

On Wed, 13 Mar 2024 18:44:58 +0100
Louis Chauvet <[email protected]> wrote:

> Introduce two typedefs: pixel_read_t and pixel_write_t. It allows the
> compiler to check if the passed functions take the correct arguments.
> Such typedefs will help ensuring consistency across the code base in
> case of update of these prototypes.
>
> Rename input/output variable in a consistent way between read_line and
> write_line.
>
> A warn has been added in get_pixel_*_function to alert when an unsupported
> pixel format is requested. As those formats are checked before
> atomic_update callbacks, it should never append.

s/append/happen/

Reviewed-by: Pekka Paalanen <[email protected]>

Thanks,
pq

>
> Document for those typedefs.
>
> Signed-off-by: Louis Chauvet <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_drv.h | 23 ++++++-
> drivers/gpu/drm/vkms/vkms_formats.c | 124 +++++++++++++++++++++---------------
> drivers/gpu/drm/vkms/vkms_formats.h | 4 +-
> drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
> 4 files changed, 95 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 18086423a3a7..4bfc62d26f08 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -53,12 +53,31 @@ struct line_buffer {
> struct pixel_argb_u16 *pixels;
> };
>
> +/**
> + * typedef pixel_write_t - These functions are used to read a pixel from a
> + * `struct pixel_argb_u16*`, convert it in a specific format and write it in the @dst_pixels
> + * buffer.
> + *
> + * @out_pixel: destination address to write the pixel
> + * @in_pixel: pixel to write
> + */
> +typedef void (*pixel_write_t)(u8 *out_pixel, struct pixel_argb_u16 *in_pixel);
> +
> struct vkms_writeback_job {
> struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> struct vkms_frame_info wb_frame_info;
> - void (*pixel_write)(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel);
> + pixel_write_t pixel_write;
> };
>
> +/**
> + * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
> + * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> + *
> + * @in_pixel: Pointer to the pixel to read
> + * @out_pixel: Pointer to write the converted pixel
> + */
> +typedef void (*pixel_read_t)(u8 *in_pixel, struct pixel_argb_u16 *out_pixel);
> +
> /**
> * vkms_plane_state - Driver specific plane state
> * @base: base plane state
> @@ -69,7 +88,7 @@ struct vkms_writeback_job {
> struct vkms_plane_state {
> struct drm_shadow_plane_state base;
> struct vkms_frame_info *frame_info;
> - void (*pixel_read)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel);
> + pixel_read_t pixel_read;
> };
>
> struct vkms_plane {
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 6e3dc8682ff9..55a4365d21a4 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -76,7 +76,7 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i
> * They are used in the `vkms_compose_row` function to handle multiple formats.
> */
>
> -static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void ARGB8888_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> {
> /*
> * The 257 is the "conversion ratio". This number is obtained by the
> @@ -84,48 +84,48 @@ static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixe
> * the best color value in a pixel format with more possibilities.
> * A similar idea applies to others RGB color conversions.
> */
> - out_pixel->a = (u16)src_pixels[3] * 257;
> - out_pixel->r = (u16)src_pixels[2] * 257;
> - out_pixel->g = (u16)src_pixels[1] * 257;
> - out_pixel->b = (u16)src_pixels[0] * 257;
> + out_pixel->a = (u16)in_pixel[3] * 257;
> + out_pixel->r = (u16)in_pixel[2] * 257;
> + out_pixel->g = (u16)in_pixel[1] * 257;
> + out_pixel->b = (u16)in_pixel[0] * 257;
> }
>
> -static void XRGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void XRGB8888_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> {
> out_pixel->a = (u16)0xffff;
> - out_pixel->r = (u16)src_pixels[2] * 257;
> - out_pixel->g = (u16)src_pixels[1] * 257;
> - out_pixel->b = (u16)src_pixels[0] * 257;
> + out_pixel->r = (u16)in_pixel[2] * 257;
> + out_pixel->g = (u16)in_pixel[1] * 257;
> + out_pixel->b = (u16)in_pixel[0] * 257;
> }
>
> -static void ARGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void ARGB16161616_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> {
> - u16 *pixels = (u16 *)src_pixels;
> + u16 *pixel = (u16 *)in_pixel;
>
> - out_pixel->a = le16_to_cpu(pixels[3]);
> - out_pixel->r = le16_to_cpu(pixels[2]);
> - out_pixel->g = le16_to_cpu(pixels[1]);
> - out_pixel->b = le16_to_cpu(pixels[0]);
> + out_pixel->a = le16_to_cpu(pixel[3]);
> + out_pixel->r = le16_to_cpu(pixel[2]);
> + out_pixel->g = le16_to_cpu(pixel[1]);
> + out_pixel->b = le16_to_cpu(pixel[0]);
> }
>
> -static void XRGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void XRGB16161616_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> {
> - u16 *pixels = (u16 *)src_pixels;
> + u16 *pixel = (u16 *)in_pixel;
>
> out_pixel->a = (u16)0xffff;
> - out_pixel->r = le16_to_cpu(pixels[2]);
> - out_pixel->g = le16_to_cpu(pixels[1]);
> - out_pixel->b = le16_to_cpu(pixels[0]);
> + out_pixel->r = le16_to_cpu(pixel[2]);
> + out_pixel->g = le16_to_cpu(pixel[1]);
> + out_pixel->b = le16_to_cpu(pixel[0]);
> }
>
> -static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void RGB565_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> {
> - u16 *pixels = (u16 *)src_pixels;
> + u16 *pixel = (u16 *)in_pixel;
>
> s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
>
> - u16 rgb_565 = le16_to_cpu(*pixels);
> + u16 rgb_565 = le16_to_cpu(*pixel);
> s64 fp_r = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> s64 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> s64 fp_b = drm_int2fixp(rgb_565 & 0x1f);
> @@ -169,12 +169,12 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>
> /*
> * The following functions take one argb_u16 pixel and convert it to a specific format. The
> - * result is stored in @dst_pixels.
> + * result is stored in @out_pixel.
> *
> * They are used in the `vkms_writeback_row` to convert and store a pixel from the src_buffer to
> * the writeback buffer.
> */
> -static void argb_u16_to_ARGB8888(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> +static void argb_u16_to_ARGB8888(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> {
> /*
> * This sequence below is important because the format's byte order is
> @@ -186,43 +186,43 @@ static void argb_u16_to_ARGB8888(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel
> * | Addr + 2 | = Red channel
> * | Addr + 3 | = Alpha channel
> */
> - dst_pixels[3] = DIV_ROUND_CLOSEST(in_pixel->a, 257);
> - dst_pixels[2] = DIV_ROUND_CLOSEST(in_pixel->r, 257);
> - dst_pixels[1] = DIV_ROUND_CLOSEST(in_pixel->g, 257);
> - dst_pixels[0] = DIV_ROUND_CLOSEST(in_pixel->b, 257);
> + out_pixel[3] = DIV_ROUND_CLOSEST(in_pixel->a, 257);
> + out_pixel[2] = DIV_ROUND_CLOSEST(in_pixel->r, 257);
> + out_pixel[1] = DIV_ROUND_CLOSEST(in_pixel->g, 257);
> + out_pixel[0] = DIV_ROUND_CLOSEST(in_pixel->b, 257);
> }
>
> -static void argb_u16_to_XRGB8888(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> +static void argb_u16_to_XRGB8888(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> {
> - dst_pixels[3] = 0xff;
> - dst_pixels[2] = DIV_ROUND_CLOSEST(in_pixel->r, 257);
> - dst_pixels[1] = DIV_ROUND_CLOSEST(in_pixel->g, 257);
> - dst_pixels[0] = DIV_ROUND_CLOSEST(in_pixel->b, 257);
> + out_pixel[3] = 0xff;
> + out_pixel[2] = DIV_ROUND_CLOSEST(in_pixel->r, 257);
> + out_pixel[1] = DIV_ROUND_CLOSEST(in_pixel->g, 257);
> + out_pixel[0] = DIV_ROUND_CLOSEST(in_pixel->b, 257);
> }
>
> -static void argb_u16_to_ARGB16161616(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> +static void argb_u16_to_ARGB16161616(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> {
> - u16 *pixels = (u16 *)dst_pixels;
> + u16 *pixel = (u16 *)out_pixel;
>
> - pixels[3] = cpu_to_le16(in_pixel->a);
> - pixels[2] = cpu_to_le16(in_pixel->r);
> - pixels[1] = cpu_to_le16(in_pixel->g);
> - pixels[0] = cpu_to_le16(in_pixel->b);
> + pixel[3] = cpu_to_le16(in_pixel->a);
> + pixel[2] = cpu_to_le16(in_pixel->r);
> + pixel[1] = cpu_to_le16(in_pixel->g);
> + pixel[0] = cpu_to_le16(in_pixel->b);
> }
>
> -static void argb_u16_to_XRGB16161616(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> +static void argb_u16_to_XRGB16161616(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> {
> - u16 *pixels = (u16 *)dst_pixels;
> + u16 *pixel = (u16 *)out_pixel;
>
> - pixels[3] = 0xffff;
> - pixels[2] = cpu_to_le16(in_pixel->r);
> - pixels[1] = cpu_to_le16(in_pixel->g);
> - pixels[0] = cpu_to_le16(in_pixel->b);
> + pixel[3] = 0xffff;
> + pixel[2] = cpu_to_le16(in_pixel->r);
> + pixel[1] = cpu_to_le16(in_pixel->g);
> + pixel[0] = cpu_to_le16(in_pixel->b);
> }
>
> -static void argb_u16_to_RGB565(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> +static void argb_u16_to_RGB565(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> {
> - u16 *pixels = (u16 *)dst_pixels;
> + u16 *pixel = (u16 *)out_pixel;
>
> s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
> @@ -235,7 +235,7 @@ static void argb_u16_to_RGB565(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel)
> u16 g = drm_fixp2int(drm_fixp_div(fp_g, fp_g_ratio));
> u16 b = drm_fixp2int(drm_fixp_div(fp_b, fp_rb_ratio));
>
> - *pixels = cpu_to_le16(r << 11 | g << 5 | b);
> + *pixel = cpu_to_le16(r << 11 | g << 5 | b);
> }
>
> /**
> @@ -266,7 +266,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
> *
> * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
> */
> -void *get_pixel_conversion_function(u32 format)
> +pixel_read_t get_pixel_read_function(u32 format)
> {
> switch (format) {
> case DRM_FORMAT_ARGB8888:
> @@ -280,7 +280,16 @@ void *get_pixel_conversion_function(u32 format)
> case DRM_FORMAT_RGB565:
> return &RGB565_to_argb_u16;
> default:
> - return NULL;
> + /*
> + * This is a bug in vkms_plane_atomic_check. All the supported
> + * format must:
> + * - Be listed in vkms_formats in vkms_plane.c
> + * - Have a pixel_read callback defined here
> + */
> + WARN(true,
> + "Pixel format %p4cc is not supported by VKMS planes. This is a kernel bug, atomic check must forbid this configuration.\n",
> + &format);
> + return (pixel_read_t)NULL;
> }
> }
>
> @@ -291,7 +300,7 @@ void *get_pixel_conversion_function(u32 format)
> *
> * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
> */
> -void *get_pixel_write_function(u32 format)
> +pixel_write_t get_pixel_write_function(u32 format)
> {
> switch (format) {
> case DRM_FORMAT_ARGB8888:
> @@ -305,6 +314,15 @@ void *get_pixel_write_function(u32 format)
> case DRM_FORMAT_RGB565:
> return &argb_u16_to_RGB565;
> default:
> - return NULL;
> + /*
> + * This is a bug in vkms_writeback_atomic_check. All the supported
> + * format must:
> + * - Be listed in vkms_wb_formats in vkms_writeback.c
> + * - Have a pixel_write callback defined here
> + */
> + WARN(true,
> + "Pixel format %p4cc is not supported by VKMS writeback. This is a kernel bug, atomic check must forbid this configuration.\n",
> + &format);
> + return (pixel_write_t)NULL;
> }
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index cf59c2ed8e9a..3ecea4563254 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -5,8 +5,8 @@
>
> #include "vkms_drv.h"
>
> -void *get_pixel_conversion_function(u32 format);
> +pixel_read_t get_pixel_read_function(u32 format);
>
> -void *get_pixel_write_function(u32 format);
> +pixel_write_t get_pixel_write_function(u32 format);
>
> #endif /* _VKMS_FORMATS_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 21b5adfb44aa..10e9b23dab28 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -125,7 +125,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated),
> drm_rect_height(&frame_info->rotated), frame_info->rotation);
>
> - vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
> + vkms_plane_state->pixel_read = get_pixel_read_function(fmt);
> }
>
> static int vkms_plane_atomic_check(struct drm_plane *plane,
>

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-03-25 15:08:42

by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH v5 02/16] drm/vkms: Use drm_frame directly

On Wed, 13 Mar 2024 18:44:56 +0100
Louis Chauvet <[email protected]> wrote:

> From: Arthur Grillo <[email protected]>
>
> Remove intermidiary variables and access the variables directly from
> drm_frame. These changes should be noop.
>
> Signed-off-by: Arthur Grillo <[email protected]>
> Signed-off-by: Louis Chauvet <[email protected]>
> ---

Acked-by: Pekka Paalanen <[email protected]>

Thanks,
pq

> drivers/gpu/drm/vkms/vkms_drv.h | 3 ---
> drivers/gpu/drm/vkms/vkms_formats.c | 12 +++++++-----
> drivers/gpu/drm/vkms/vkms_plane.c | 3 ---
> drivers/gpu/drm/vkms/vkms_writeback.c | 5 -----
> 4 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8f5710debb1e..b4b357447292 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -31,9 +31,6 @@ struct vkms_frame_info {
> struct drm_rect rotated;
> struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> unsigned int rotation;
> - unsigned int offset;
> - unsigned int pitch;
> - unsigned int cpp;
> };
>
> struct pixel_argb_u16 {
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 36046b12f296..172830a3936a 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -11,8 +11,10 @@
>
> static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> {
> - return frame_info->offset + (y * frame_info->pitch)
> - + (x * frame_info->cpp);
> + struct drm_framebuffer *fb = frame_info->fb;
> +
> + return fb->offsets[0] + (y * fb->pitches[0])
> + + (x * fb->format->cpp[0]);
> }
>
> /*
> @@ -131,12 +133,12 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
> u8 *src_pixels = get_packed_src_addr(frame_info, y);
> int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
>
> - for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
> + for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
> int x_pos = get_x_position(frame_info, limit, x);
>
> if (drm_rotation_90_or_270(frame_info->rotation))
> src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
> - + frame_info->cpp * y;
> + + frame_info->fb->format->cpp[0] * y;
>
> plane->pixel_read(src_pixels, &out_pixels[x_pos]);
> }
> @@ -223,7 +225,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
> struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
> int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
>
> - for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->cpp)
> + for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->fb->format->cpp[0])
> wb->pixel_write(dst_pixels, &in_pixels[x]);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 5a8d295e65f2..21b5adfb44aa 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -125,9 +125,6 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated),
> drm_rect_height(&frame_info->rotated), frame_info->rotation);
>
> - frame_info->offset = fb->offsets[0];
> - frame_info->pitch = fb->pitches[0];
> - frame_info->cpp = fb->format->cpp[0];
> vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index bc724cbd5e3a..c8582df1f739 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -149,11 +149,6 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> crtc_state->active_writeback = active_wb;
> crtc_state->wb_pending = true;
> spin_unlock_irq(&output->composer_lock);
> -
> - wb_frame_info->offset = fb->offsets[0];
> - wb_frame_info->pitch = fb->pitches[0];
> - wb_frame_info->cpp = fb->format->cpp[0];
> -
> drm_writeback_queue_job(wb_conn, connector_state);
> active_wb->pixel_write = get_pixel_write_function(wb_format);
> drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
>

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-03-25 15:08:41

[permalink] [raw]

Subject: Re: [PATCH v5 09/16] drm/vkms: Introduce pixel_read_direction enum

On Tue, 26 Mar 2024 16:57:00 +0100
Louis Chauvet <[email protected]> wrote:

> Le 25/03/24 - 15:11, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:03 +0100
> > Louis Chauvet <[email protected]> wrote:
> >
> > > The pixel_read_direction enum is useful to describe the reading direction
> > > in a plane. It avoids using the rotation property of DRM, which not
> > > practical to know the direction of reading.
> > > This patch also introduce two helpers, one to compute the
> > > pixel_read_direction from the DRM rotation property, and one to compute
> > > the step, in byte, between two successive pixel in a specific direction.
> > >
> > > Signed-off-by: Louis Chauvet <[email protected]>
> > > ---
> > > drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/vkms/vkms_drv.h | 11 +++++++++++
> > > drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++++++
> > > 3 files changed, 77 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > index 9254086f23ff..989bcf59f375 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > > @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff
> > > }
> > > }
> > >
> > > +/**
> > > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > > + *
> > > + * This function will use the @rotation setting of a source plane to compute the reading
> > > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > > + * to be written from left to right on the CRTC.
> >
> > That is a well written description.
>
> Thanks
>
> > > + *
> > > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > > + */
> > > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > > +{
> > > + if (rotation & DRM_MODE_ROTATE_0) {
> > > + if (rotation & DRM_MODE_REFLECT_X)
> > > + return READ_RIGHT_TO_LEFT;
> > > + else
> > > + return READ_LEFT_TO_RIGHT;
> > > + } else if (rotation & DRM_MODE_ROTATE_90) {
> > > + if (rotation & DRM_MODE_REFLECT_Y)
> > > + return READ_BOTTOM_TO_TOP;
> > > + else
> > > + return READ_TOP_TO_BOTTOM;
> > > + } else if (rotation & DRM_MODE_ROTATE_180) {
> > > + if (rotation & DRM_MODE_REFLECT_X)
> > > + return READ_LEFT_TO_RIGHT;
> > > + else
> > > + return READ_RIGHT_TO_LEFT;
> > > + } else if (rotation & DRM_MODE_ROTATE_270) {
> > > + if (rotation & DRM_MODE_REFLECT_Y)
> > > + return READ_TOP_TO_BOTTOM;
> > > + else
> > > + return READ_BOTTOM_TO_TOP;
> > > + }
> > > + return READ_LEFT_TO_RIGHT;
> >
> > I'm a little worried seeing REFLECT_X is supported only for some
> > rotations, and REFLECT_Y for other rotations. Why is an analysis of all
> > combinations not necessary?
>
> I don't need to manage all the combination because this is only about
> the "horizontal writing".
>
> So, if you want to write a line in the CRTC, with:
> - ROT_0 || REF_X => You need to read the source line from right to left
> - ROT_0 => You need to read source buffer from left to right
> - ROT_0 || REF_Y => You need to read the source line from left to right

That is true, indeed.

> In this case, REF_Y only have an effect on the "column reading". It is not
> needed here because the new version of the blend function will use the
> drm_rect_* helpers to compute the correct y coordinate.
>
> If you think it's clearer, I can create a big switch(rotation) like this:
>
> switch (rotation) {
> case ROT_0:
> case ROT_0 || REF_X:
> return L2R;
> case ROT_0 || REF_Y:
> return R2L;
> case ROT_90:
> case ROT_90 || REF_X:
> return T2B;
> [...]
> }
>
> So all cases are clearly covered?

I think that would suit my personal taste better. It would not raise
questions nor need a comment. It does become a long function, but I
tend to favour long and clear more than short and needs thinking to
figure out if it works, everything else being equivalent.

I wonder how DRM maintainers feel.

> > I hope IGT uses FB patterns instead of solid color in its tests of
> > rotation to be able to detect the difference.
>
> They use solid colors, and even my new rotation test [3] use solid colors.

That will completely fail to detect rotation and reflection bugs then.
E.g. userspace asks for 180-degree rotation, and the driver does not
rotate at all. Or rotate-180 getting confused with one reflection.

> It is mainly for yuv formats with subsampling: if you have formats with
> subsampling, a "software rotated buffer" and a "hardware rotated buffer"
> will not apply the same subsampling, so the colors will be slightly
> different.

Why would they not use the same subsampling?

The framebuffer contents are defined in its natural orientation, and
the subsampling applies in the natural orientation. If such a FB
is on a rotated plane, one must account for subsampling first, and
rotate second. 90-degree rotation does not change the encoded color.

Getting the subsampling exactly right is going to be necessary sooner
or later. There is no UAPI for setting chroma siting yet, but ideally
there should be.

> > The return values do seem correct to me, assuming I have guessed
> > correctly what "X" and "Y" refer to when combined with rotation. I did
> > not find good documentation about that.
>
> Yes, it is difficult to understand how rotation and reflexion should
> works in drm. I spend half a day testing all the combination in drm_rect_*
> helpers to understand how this works. According to the code:
> - If only rotation or only reflexion, easy as expected
> - If reflexion and rotation are mixed, the source buffer is first
> reflected and then rotated.

Now that you know, you could send a documentation patch. :-)

For me as a userspace developer, the important place is
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties

>
> > Btw. if there are already functions that are able to transform
> > coordinates based on the rotation bitfield, you could alternatively use
> > them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
> > are in plane coordinate system, and vector B - A gives you the
> > direction. The reason I'm mentioning this is that then you don't have
> > to implement yet another copy of the rotation bitfield semantics from
> > scratch.
>
> You are totaly right. I will try this elegant method. Yes, there are some
> helpers (drm_rect_rotate_inv), so I will try to do something.

Cool, thanks,
pq

> >
> > > +}
> > > +
> > > /**
> > > * blend - blend the pixels from all planes and compute crc
> > > * @wb: The writeback frame buffer metadata
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 3ead8b39af4a..985e7a92b7bc 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > > @@ -69,6 +69,17 @@ struct vkms_writeback_job {
> > > pixel_write_t pixel_write;
> > > };
> > >
> > > +/**
> > > + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a
> > > + * plane.
> > > + */
> > > +enum pixel_read_direction {
> > > + READ_BOTTOM_TO_TOP,
> > > + READ_TOP_TO_BOTTOM,
> > > + READ_RIGHT_TO_LEFT,
> > > + READ_LEFT_TO_RIGHT
> > > +};
> > > +
> > > /**
> > > * typedef pixel_read_t - These functions are used to read a pixel in the source frame,
> > > * convert it to `struct pixel_argb_u16` and write it to @out_pixel.
> > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > > index 649d75d05b1f..743b6fd06db5 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > > @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info,
> > > *addr = (u8 *)frame_info->map[0].vaddr + offset;
> > > }
> > >
> > > +/**
> > > + * get_step_next_block() - Common helper to compute the correct step value between each pixel block
> > > + * to read in a certain direction.
> > > + *
> > > + * As the returned offset is the number of bytes between two consecutive blocks in a direction,
> > > + * the caller may have to read multiple pixel before using the next one (for example, to read from
> > > + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used
> > > + * only every 8 pixels.
> > > + *
> > > + * @fb: Framebuffer to iter on
> > > + * @direction: Direction of the reading
> > > + * @plane_index: Plane to get the step from
> > > + */
> > > +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > > + int plane_index)
> > > +{
> >
> > I would have called this something like get_block_step_bytes() for
> > example. That makes it clear it returns bytes (not e.g. pixels). "next"
> > implies to me that I tell the function the current block, and then it
> > gets me the next one. It does not do that, so I'd not use "next".
>
> Nice name, I will took it for the v6.
>
> Thanks,
> Louis Chauvet
>
> > > + switch (direction) {
> > > + case READ_LEFT_TO_RIGHT:
> > > + return fb->format->char_per_block[plane_index];
> > > + case READ_RIGHT_TO_LEFT:
> > > + return -fb->format->char_per_block[plane_index];
> > > + case READ_TOP_TO_BOTTOM:
> > > + return (int)fb->pitches[plane_index];
> > > + case READ_BOTTOM_TO_TOP:
> > > + return -(int)fb->pitches[plane_index];
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Looks good.
> >
> >
> > Thanks,
> > pq
> >
> > > +
> > > static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y,
> > > int plane_index)
> > > {
> > >
> >
>
>
>

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-03-27 15:03:39

On Wed, 13 Mar 2024 18:45:10 +0100
Louis Chauvet <[email protected]> wrote:

> This add the support for:
> - R1/R2/R4/R8
>
> R1 format was tested with [1] and [2].
>
> [1]: https://lore.kernel.org/r/[email protected]
> [2]: https://lore.kernel.org/igt-dev/[email protected]/
>
> Signed-off-by: Louis Chauvet <[email protected]>
> ---
> drivers/gpu/drm/vkms/vkms_formats.c | 100 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_plane.c | 6 ++-
> 2 files changed, 105 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 863fc91d6d48..cbb2ec09564a 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -201,6 +201,11 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const u16 *pixel)
> return out_pixel;
> }
>
> +static struct pixel_argb_u16 argb_u16_from_gray8(u8 gray)
> +{
> + return argb_u16_from_u8888(255, gray, gray, gray);
> +}
> +
> VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> struct conversion_matrix *matrix)
> {
> @@ -269,6 +274,89 @@ static void black_to_argb_u16(const struct vkms_plane_state *plane, int x_start,
> }
> }
>
> +static void Rx_read_line(const struct vkms_plane_state *plane, int x_start,
> + int y_start, enum pixel_read_direction direction, int count,
> + struct pixel_argb_u16 out_pixel[], u8 bit_per_pixel, u8 lum_per_level)
> +{
> + struct pixel_argb_u16 *end = out_pixel + count;
> + u8 *src_pixels;
> + int rem_x, rem_y;
> +
> + packed_pixels_addr(plane->frame_info, x_start, y_start, 0, &src_pixels, &rem_x, &rem_y);

Maybe assert that rem_y = 0? Or block_h = 1.

> + int bit_offset = (int)rem_x * bit_per_pixel;

Why cast rem_x to int when it was defined to be int?

> + int step = get_step_next_block(plane->frame_info->fb, direction, 0);
> + int mask = (0x1 << bit_per_pixel) - 1;

Since mask will interact with u8, it should be unsigned too.

> +
> + if (direction == READ_LEFT_TO_RIGHT || direction == READ_RIGHT_TO_LEFT) {
> + int restart_bit_offset = 0;
> + int step_bit_offset = bit_per_pixel;
> +
> + if (direction == READ_RIGHT_TO_LEFT) {
> + restart_bit_offset = 8 - bit_per_pixel;
> + step_bit_offset = -bit_per_pixel;
> + }
> +
> + while (out_pixel < end) {
> + u8 val = (*src_pixels & (mask << bit_offset)) >> bit_offset;

or shorter: (*src_pixels >> bit_offset) & mask

However, shouldn't the first pixel be on the high bits?

That how I would understand the comments in drm_fourcc.h.

Again a reason to avoid a solid color fill in IGT.

> +
> + *out_pixel = argb_u16_from_gray8(val * lum_per_level);
> +
> + bit_offset += step_bit_offset;
> + if (bit_offset < 0 || 8 <= bit_offset) {
> + bit_offset = restart_bit_offset;
> + src_pixels += step;
> + }
> + out_pixel += 1;
> + }
> + } else if (direction == READ_TOP_TO_BOTTOM || direction == READ_BOTTOM_TO_TOP) {
> + while (out_pixel < end) {
> + u8 val = (*src_pixels & (mask << bit_offset)) >> bit_offset;
> + *out_pixel = argb_u16_from_gray8(val * lum_per_level);
> + src_pixels += step;
> + out_pixel += 1;
> + }
> + }
> +}
> +
> +static void R1_read_line(const struct vkms_plane_state *plane, int x_start,
> + int y_start, enum pixel_read_direction direction, int count,
> + struct pixel_argb_u16 out_pixel[])
> +{
> + Rx_read_line(plane, x_start, y_start, direction, count, out_pixel, 1, 0xFF);
> +}
> +
> +static void R2_read_line(const struct vkms_plane_state *plane, int x_start,
> + int y_start, enum pixel_read_direction direction, int count,
> + struct pixel_argb_u16 out_pixel[])
> +{
> + Rx_read_line(plane, x_start, y_start, direction, count, out_pixel, 2, 0x55);
> +}
> +
> +static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
> + int y_start, enum pixel_read_direction direction, int count,
> + struct pixel_argb_u16 out_pixel[])
> +{
> + Rx_read_line(plane, x_start, y_start, direction, count, out_pixel, 4, 0x11);
> +}
> +
> +static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
> + int y_start, enum pixel_read_direction direction, int count,
> + struct pixel_argb_u16 out_pixel[])
> +{
> + struct pixel_argb_u16 *end = out_pixel + count;
> + u8 *src_pixels;
> + int rem_x, rem_y;
> + int step = get_step_next_block(plane->frame_info->fb, direction, 0);
> +
> + packed_pixels_addr(plane->frame_info, x_start, y_start, 0, &src_pixels, &rem_x, &rem_y);

Assert on block size?

> +
> + while (out_pixel < end) {
> + *out_pixel = argb_u16_from_gray8(*src_pixels);
> + src_pixels += step;
> + out_pixel += 1;
> + }
> +}
> +
> static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> enum pixel_read_direction direction, int count,
> struct pixel_argb_u16 out_pixel[])
> @@ -582,6 +670,14 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> case DRM_FORMAT_YVU422:
> case DRM_FORMAT_YVU444:
> return &planar_yuv_read_line;
> + case DRM_FORMAT_R1:
> + return &R1_read_line;
> + case DRM_FORMAT_R2:
> + return &R2_read_line;
> + case DRM_FORMAT_R4:
> + return &R4_read_line;
> + case DRM_FORMAT_R8:
> + return &R8_read_line;
> default:
> /*
> * This is a bug in vkms_plane_atomic_check. All the supported
> @@ -855,6 +951,10 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
> case DRM_FORMAT_ARGB16161616:
> case DRM_FORMAT_XRGB16161616:
> case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_R1:
> + case DRM_FORMAT_R2:
> + case DRM_FORMAT_R4:
> + case DRM_FORMAT_R8:
> /*
> * Those formats are supported, but they don't need a conversion matrix. Return

It is strange that you need to list irrelevant formats here.

> * a valid pointer to avoid kernel panic in case this matrix is used/checked
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index e21cc92cf497..dc9d62acf350 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -29,7 +29,11 @@ static const u32 vkms_formats[] = {
> DRM_FORMAT_YUV444,
> DRM_FORMAT_YVU420,
> DRM_FORMAT_YVU422,
> - DRM_FORMAT_YVU444
> + DRM_FORMAT_YVU444,
> + DRM_FORMAT_R1,
> + DRM_FORMAT_R2,
> + DRM_FORMAT_R4,
> + DRM_FORMAT_R8
> };
>
> static struct drm_plane_state *
>

Thanks,
pq

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-03-27 14:32:08

On Mon, 8 Apr 2024 09:50:18 +0200
Louis Chauvet <[email protected]> wrote:

> Le 27/03/24 - 14:16, Pekka Paalanen a écrit :
> > On Tue, 26 Mar 2024 16:57:00 +0100
> > Louis Chauvet <[email protected]> wrote:
> >
> > > Le 25/03/24 - 15:11, Pekka Paalanen a écrit :
> > > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > > Louis Chauvet <[email protected]> wrote:
> > > >
> > > > > The pixel_read_direction enum is useful to describe the reading direction
> > > > > in a plane. It avoids using the rotation property of DRM, which not
> > > > > practical to know the direction of reading.
> > > > > This patch also introduce two helpers, one to compute the
> > > > > pixel_read_direction from the DRM rotation property, and one to compute
> > > > > the step, in byte, between two successive pixel in a specific direction.
> > > > >
> > > > > Signed-off-by: Louis Chauvet <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > drivers/gpu/drm/vkms/vkms_drv.h | 11 +++++++++++
> > > > > drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++++++
> > > > > 3 files changed, 77 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > index 9254086f23ff..989bcf59f375 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c

> > > > I hope IGT uses FB patterns instead of solid color in its tests of
> > > > rotation to be able to detect the difference.
> > >
> > > They use solid colors, and even my new rotation test [3] use solid colors.
> >
> > That will completely fail to detect rotation and reflection bugs then.
> > E.g. userspace asks for 180-degree rotation, and the driver does not
> > rotate at all. Or rotate-180 getting confused with one reflection.
>
> I think I missunderstood what you means with "solid colors".
>
> The tests uses a plane with multiple solid colors:
>
> +-------+-------+
> | White | Red |
> +-------+-------+
> | Blue | Green |
> +-------+-------+
>
> But it don't use gradients because of YUV.
>

Oh, that works. No worries then.

> > > It is mainly for yuv formats with subsampling: if you have formats with
> > > subsampling, a "software rotated buffer" and a "hardware rotated buffer"
> > > will not apply the same subsampling, so the colors will be slightly
> > > different.
> >
> > Why would they not use the same subsampling?
>
> YUV422, for each pair of pixels along a horizontal line, the U and V
> components are shared between those two pixels. However, along a vertical
> line, each pixel has its own U and V components.
>
> When you rotate an image by 90 degrees:
> - Hardware Rotation: If you use hardware rotation, the YUV subsampling
> axis will align with what was previously the "White-Red" axis. The
> hardware will handle the rotation.
> - Software Rotation: If you use software rotation, the YUV subsampling
> axis will align with what was previously the "Red-Green" axis.

That would be a bug in the software rotation.

> Because the subsampling compression axis changes depending on whether
> you're using hardware or software rotation, the compression effect on
> colors will differ. Specifically:
> - Hardware rotation, a gradient along the "White-Red" axis may be
> compressed (i.e same UV component for multiple pixels along the
> gradient).
> - Software rotation, the same gradient will not be compressed (i.e, each
> different color in the gradient have dedicated UV component)
>
> The same reasoning also apply for "color borders", and my series [3] avoid
> this issue by choosing the right number of pixels.

What is [3]?

I've used similar tactics in the Weston test suite, when I have no
implementation for chroma siting: the input and reference images
consist of 2x2 equal color pixel groups, so that chroma siting makes no
difference. When chroma siting will be implemented, the tests will be
extended.

Is there a TODO item to fix the software rotation bug and make the
tests more sensitive?

I think documenting this would be an ok intermediate solution.

> > The framebuffer contents are defined in its natural orientation, and
> > the subsampling applies in the natural orientation. If such a FB
> > is on a rotated plane, one must account for subsampling first, and
> > rotate second. 90-degree rotation does not change the encoded color.
> >
> > Getting the subsampling exactly right is going to be necessary sooner
> > or later. There is no UAPI for setting chroma siting yet, but ideally
> > there should be.
> >
> > > > The return values do seem correct to me, assuming I have guessed
> > > > correctly what "X" and "Y" refer to when combined with rotation. I did
> > > > not find good documentation about that.
> > >
> > > Yes, it is difficult to understand how rotation and reflexion should
> > > works in drm. I spend half a day testing all the combination in drm_rect_*
> > > helpers to understand how this works. According to the code:
> > > - If only rotation or only reflexion, easy as expected
> > > - If reflexion and rotation are mixed, the source buffer is first
> > > reflected and then rotated.
> >
> > Now that you know, you could send a documentation patch. :-)
>
> And now I'm not sure about it :)

You'll have people review the patch and confirm your understanding or
point out a mistake. A doc patch it easier to notice and jump in than
this series.

> I was running the tests on my v6, and for the first time ran my new
> rotation [3] on the previous VKMS code. None of the tests for
> ROT_90+reflexion and ROT_270+reflexion are passing...
>
> So, either the previous vkms implementation was wrong, or mine is wrong :)
>
> So, if a DRM expert can explain this, it could be nice.
>
> To have a common example, if I take the same buffer as above
> (white+red+blue+green), if I create a plane with rotation =
> ROTATION_90 | REFLECTION_X, what is the expected result?
>
> 1 - rotation then reflection
>
> +-------+-------+
> | Green | Red |
> +-------+-------+
> | Blue | White |
> +-------+-------+
>
> 2 - reflection then rotation (my vkms implementation)
>
> +-------+-------+
> | White | Blue |
> +-------+-------+
> | Red | Green |
> +-------+-------+
>

I wish I knew. :-)

Thanks,
pq

> > For me as a userspace developer, the important place is
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties
> >

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature

2024-04-09 10:07:21

by Louis Chauvet

[permalink] [raw]

Subject: Re: [PATCH v5 11/16] drm/vkms: Add YUV support

Le 09/04/24 - 10:58, Pekka Paalanen a ?crit :
> On Mon, 8 Apr 2024 09:50:19 +0200
> Louis Chauvet <[email protected]> wrote:
>
> > Le 27/03/24 - 16:23, Pekka Paalanen a ?crit :
> > > On Wed, 13 Mar 2024 18:45:05 +0100
> > > Louis Chauvet <[email protected]> wrote:
> > >
> > > > From: Arthur Grillo <[email protected]>
> > > >
> > > > Add support to the YUV formats bellow:
> > > >
> > > > - NV12/NV16/NV24
> > > > - NV21/NV61/NV42
> > > > - YUV420/YUV422/YUV444
> > > > - YVU420/YVU422/YVU444
> > > >
> > > > The conversion from yuv to rgb is done with fixed-point arithmetic, using
> > > > 32.32 floats and the drm_fixed helpers.
> > >
> > > You mean fixed-point, not floating-point (floats).
> > >
> > > >
> > > > To do the conversion, a specific matrix must be used for each color range
> > > > (DRM_COLOR_*_RANGE) and encoding (DRM_COLOR_*). This matrix is stored in
> > > > the `conversion_matrix` struct, along with the specific y_offset needed.
> > > > This matrix is queried only once, in `vkms_plane_atomic_update` and
> > > > stored in a `vkms_plane_state`. Those conversion matrices of each
> > > > encoding and range were obtained by rounding the values of the original
> > > > conversion matrices multiplied by 2^32. This is done to avoid the use of
> > > > floating point operations.
> > > >
> > > > The same reading function is used for YUV and YVU formats. As the only
> > > > difference between those two category of formats is the order of field, a
> > > > simple swap in conversion matrix columns allows using the same function.
> > >
> > > Sounds good!
> > >
> > > > Signed-off-by: Arthur Grillo <[email protected]>
> > > > [Louis Chauvet:
> > > > - Adapted Arthur's work
> > > > - Implemented the read_line_t callbacks for yuv
> > > > - add struct conversion_matrix
> > > > - remove struct pixel_yuv_u8
> > > > - update the commit message
> > > > - Merge the modifications from Arthur]
> > > > Signed-off-by: Louis Chauvet <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/vkms/vkms_drv.h | 22 ++
> > > > drivers/gpu/drm/vkms/vkms_formats.c | 431 ++++++++++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/vkms/vkms_formats.h | 4 +
> > > > drivers/gpu/drm/vkms/vkms_plane.c | 17 +-
> > > > 4 files changed, 473 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > > index 23e1d247468d..f3116084de5a 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>
> ...
>
> > > > +static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> > > > + struct conversion_matrix *matrix)
> > >
> > > If you are using the "swap the matrix columns" trick, then you cannot
> > > call these cb, cr nor even u,v, because they might be the opposite.
> > > They are simply the first and second chroma channel, and their meaning
> > > depends on the given matrix.
> >
> > I will rename them for v6, channel_1 and channel_2.
> >
> > > > +{
> > > > + u8 r, g, b;
> > > > + s64 fp_y, fp_cb, fp_cr;
> > > > + s64 fp_r, fp_g, fp_b;
> > > > +
> > > > + fp_y = y - matrix->y_offset;
> > > > + fp_cb = cb - 128;
> > > > + fp_cr = cr - 128;
> > >
> > > This looks like an incorrect way to convert u8 to fixed-point, but...
> > >
> > > > +
> > > > + fp_y = drm_int2fixp(fp_y);
> > > > + fp_cb = drm_int2fixp(fp_cb);
> > > > + fp_cr = drm_int2fixp(fp_cr);
> > >
> > > I find it confusing to re-purpose variables like this.
> > >
> > > I'd do just
> > >
> > > fp_c1 = drm_int2fixp((int)c1 - 128);
> >
> > I agree with this remark, I will change it for the v6.
> >
> > > If the function arguments were int to begin with, then the cast would
> > > be obviously unnecessary.
> >
> > For this I'm less sure. The name of the function and the usage is
> > explicit: we want to use u8 as input. As we manipulate pointers in
> > read_line, I don't know how it will works if the pointer is dereferenced
> > to a int instead of a u8.
>
> Dereference operator acts on its input type. What happens to the result
> is irrelevant.
>
> If we have
>
> u8 *p = ...;
>
> void foo(int x);
>
> then you can call
>
> foo(*v);
>
> if that was your question. Dereference acts on u8* which results in u8.
> Then it gets implicitly cast to int.

Thanks for the clear explaination!

> However, you have a semantic reason to keep the argument as u8, and
> that is fine.

So I will keep u8 for the v6.

> > > So, what you have in fp variables at this point is fractional numbers
> > > in the 8-bit integer scale. However, because the target format is
> > > 16-bit, you should not show the extra precision away here. Instead,
> > > multiply by 257 to bring the values to 16-bit scale, and do the RGB
> > > clamping to 16-bit, not 8-bit.
> > >
> > > > +
> > > > + fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
> > > > + drm_fixp_mul(matrix->matrix[0][1], fp_cb) +
> > > > + drm_fixp_mul(matrix->matrix[0][2], fp_cr);
> > > > + fp_g = drm_fixp_mul(matrix->matrix[1][0], fp_y) +
> > > > + drm_fixp_mul(matrix->matrix[1][1], fp_cb) +
> > > > + drm_fixp_mul(matrix->matrix[1][2], fp_cr);
> > > > + fp_b = drm_fixp_mul(matrix->matrix[2][0], fp_y) +
> > > > + drm_fixp_mul(matrix->matrix[2][1], fp_cb) +
> > > > + drm_fixp_mul(matrix->matrix[2][2], fp_cr);
> > > > +
> > > > + fp_r = drm_fixp2int_round(fp_r);
> > > > + fp_g = drm_fixp2int_round(fp_g);
> > > > + fp_b = drm_fixp2int_round(fp_b);
> > > > +
> > > > + r = clamp(fp_r, 0, 0xff);
> > > > + g = clamp(fp_g, 0, 0xff);
> > > > + b = clamp(fp_b, 0, 0xff);
> > > > +
> > > > + return argb_u16_from_u8888(255, r, g, b);
> > >
> > > Going through argb_u16_from_u8888() will throw away precision.
> >
> > I tried to fix it in the v6, IGT tests pass. If something is wrong in the
> > v6, please let me know.
> >
> > > > +}
> > > > +
> > > > /*
> > > > * The following functions are read_line function for each pixel format supported by VKMS.
> > > > *
> > > > @@ -293,6 +367,79 @@ static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> > > > }
> > > > }
> > > >
> > > > +/*
> > > > + * This callback can be used for yuv and yvu formats, given a properly modified conversion matrix
> > > > + * (column inversion)
> > >
> > > Would be nice to explain what semi_planar_yuv means, so that the
> > > documentation for these functions would show how they differ rather
> > > than all saying exactly the same thing.
> >
> > /* This callback can be used for YUV format where each color component is
> > * stored in a different plane (often called planar formats). It will
> > * handle correctly subsampling.
> >
> > /*
> > * This callback can be used for YUV formats where U and V values are
> > * stored in the same plane (often called semi-planar formats). It will
> > * corectly handle subsampling.
> > *
> > * The conversion matrix stored in the @plane is used to:
> > * - Apply the correct color range and encoding
> > * - Convert YUV and YVU with the same function (a simple column swap is
> > * needed)
> > */
>
> Sounds good. I'd just drop the "It will handle correctly subsampling."
> because all code is supposed to be correct by default.

Will do for the v6.

Thanks,
Louis Chauvet

> If there is a function that intentionally overlooks something, that
> certainly should be documented.
>
>
> Thanks,
> pq

--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-04-09 10:11:38

by Louis Chauvet

[permalink] [raw]

Subject: Re: [PATCH v5 09/16] drm/vkms: Introduce pixel_read_direction enum

Le 09/04/24 - 10:35, Pekka Paalanen a ?crit :
> On Mon, 8 Apr 2024 09:50:18 +0200
> Louis Chauvet <[email protected]> wrote:
>
> > Le 27/03/24 - 14:16, Pekka Paalanen a ?crit :
> > > On Tue, 26 Mar 2024 16:57:00 +0100
> > > Louis Chauvet <[email protected]> wrote:
> > >
> > > > Le 25/03/24 - 15:11, Pekka Paalanen a ?crit :
> > > > > On Wed, 13 Mar 2024 18:45:03 +0100
> > > > > Louis Chauvet <[email protected]> wrote:
> > > > >
> > > > > > The pixel_read_direction enum is useful to describe the reading direction
> > > > > > in a plane. It avoids using the rotation property of DRM, which not
> > > > > > practical to know the direction of reading.
> > > > > > This patch also introduce two helpers, one to compute the
> > > > > > pixel_read_direction from the DRM rotation property, and one to compute
> > > > > > the step, in byte, between two successive pixel in a specific direction.
> > > > > >
> > > > > > Signed-off-by: Louis Chauvet <[email protected]>
> > > > > > ---
> > > > > > drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/vkms/vkms_drv.h | 11 +++++++++++
> > > > > > drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++++++
> > > > > > 3 files changed, 77 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > > index 9254086f23ff..989bcf59f375 100644
> > > > > > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > > > > > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>
> > > > > I hope IGT uses FB patterns instead of solid color in its tests of
> > > > > rotation to be able to detect the difference.
> > > >
> > > > They use solid colors, and even my new rotation test [3] use solid colors.
> > >
> > > That will completely fail to detect rotation and reflection bugs then.
> > > E.g. userspace asks for 180-degree rotation, and the driver does not
> > > rotate at all. Or rotate-180 getting confused with one reflection.
> >
> > I think I missunderstood what you means with "solid colors".
> >
> > The tests uses a plane with multiple solid colors:
> >
> > +-------+-------+
> > | White | Red |
> > +-------+-------+
> > | Blue | Green |
> > +-------+-------+
> >
> > But it don't use gradients because of YUV.
> >
>
> Oh, that works. No worries then.
>
> > > > It is mainly for yuv formats with subsampling: if you have formats with
> > > > subsampling, a "software rotated buffer" and a "hardware rotated buffer"
> > > > will not apply the same subsampling, so the colors will be slightly
> > > > different.
> > >
> > > Why would they not use the same subsampling?
> >
> > YUV422, for each pair of pixels along a horizontal line, the U and V
> > components are shared between those two pixels. However, along a vertical
> > line, each pixel has its own U and V components.
> >
> > When you rotate an image by 90 degrees:
> > - Hardware Rotation: If you use hardware rotation, the YUV subsampling
> > axis will align with what was previously the "White-Red" axis. The
> > hardware will handle the rotation.
> > - Software Rotation: If you use software rotation, the YUV subsampling
> > axis will align with what was previously the "Red-Green" axis.
>
> That would be a bug in the software rotation.

Yes, but it is very complex to fix I think, so I did not chose
this path :)

> > Because the subsampling compression axis changes depending on whether
> > you're using hardware or software rotation, the compression effect on
> > colors will differ. Specifically:
> > - Hardware rotation, a gradient along the "White-Red" axis may be
> > compressed (i.e same UV component for multiple pixels along the
> > gradient).
> > - Software rotation, the same gradient will not be compressed (i.e, each
> > different color in the gradient have dedicated UV component)
> >
> > The same reasoning also apply for "color borders", and my series [3] avoid
> > this issue by choosing the right number of pixels.
>
> What is [3]?

I don't know why I put [3] here, I probably mixed references between mails

[3]: https://lore.kernel.org/all/[email protected]/

> I've used similar tactics in the Weston test suite, when I have no
> implementation for chroma siting: the input and reference images
> consist of 2x2 equal color pixel groups, so that chroma siting makes no
> difference. When chroma siting will be implemented, the tests will be
> extended.
>
> Is there a TODO item to fix the software rotation bug and make the
> tests more sensitive?
>
> I think documenting this would be an ok intermediate solution.
>
> > > The framebuffer contents are defined in its natural orientation, and
> > > the subsampling applies in the natural orientation. If such a FB
> > > is on a rotated plane, one must account for subsampling first, and
> > > rotate second. 90-degree rotation does not change the encoded color.
> > >
> > > Getting the subsampling exactly right is going to be necessary sooner
> > > or later. There is no UAPI for setting chroma siting yet, but ideally
> > > there should be.
> > >
> > > > > The return values do seem correct to me, assuming I have guessed
> > > > > correctly what "X" and "Y" refer to when combined with rotation. I did
> > > > > not find good documentation about that.
> > > >
> > > > Yes, it is difficult to understand how rotation and reflexion should
> > > > works in drm. I spend half a day testing all the combination in drm_rect_*
> > > > helpers to understand how this works. According to the code:
> > > > - If only rotation or only reflexion, easy as expected
> > > > - If reflexion and rotation are mixed, the source buffer is first
> > > > reflected and then rotated.
> > >
> > > Now that you know, you could send a documentation patch. :-)
> >
> > And now I'm not sure about it :)
>
> You'll have people review the patch and confirm your understanding or
> point out a mistake. A doc patch it easier to notice and jump in than
> this series.

I just send it [4], you are in copy.

[4]: https://lore.kernel.org/all/[email protected]/

> > I was running the tests on my v6, and for the first time ran my new
> > rotation [3] on the previous VKMS code. None of the tests for
> > ROT_90+reflexion and ROT_270+reflexion are passing...
> >
> > So, either the previous vkms implementation was wrong, or mine is wrong :)
> >
> > So, if a DRM expert can explain this, it could be nice.
> >
> > To have a common example, if I take the same buffer as above
> > (white+red+blue+green), if I create a plane with rotation =
> > ROTATION_90 | REFLECTION_X, what is the expected result?
> >
> > 1 - rotation then reflection
> >
> > +-------+-------+
> > | Green | Red |
> > +-------+-------+
> > | Blue | White |
> > +-------+-------+
> >
> > 2 - reflection then rotation (my vkms implementation)
> >
> > +-------+-------+
> > | White | Blue |
> > +-------+-------+
> > | Red | Green |
> > +-------+-------+
> >
>
> I wish I knew. :-)
>
> Thanks,
> pq
>
>
> > > For me as a userspace developer, the important place is
> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#standard-plane-properties
> > >

2024-04-09 08:00:20

by Pekka Paalanen

[permalink] [raw]

Subject: Re: [PATCH v5 11/16] drm/vkms: Add YUV support

On Mon, 8 Apr 2024 09:50:19 +0200
Louis Chauvet <[email protected]> wrote:

> Le 27/03/24 - 16:23, Pekka Paalanen a écrit :
> > On Wed, 13 Mar 2024 18:45:05 +0100
> > Louis Chauvet <[email protected]> wrote:
> >
> > > From: Arthur Grillo <[email protected]>
> > >
> > > Add support to the YUV formats bellow:
> > >
> > > - NV12/NV16/NV24
> > > - NV21/NV61/NV42
> > > - YUV420/YUV422/YUV444
> > > - YVU420/YVU422/YVU444
> > >
> > > The conversion from yuv to rgb is done with fixed-point arithmetic, using
> > > 32.32 floats and the drm_fixed helpers.
> >
> > You mean fixed-point, not floating-point (floats).
> >
> > >
> > > To do the conversion, a specific matrix must be used for each color range
> > > (DRM_COLOR_*_RANGE) and encoding (DRM_COLOR_*). This matrix is stored in
> > > the `conversion_matrix` struct, along with the specific y_offset needed.
> > > This matrix is queried only once, in `vkms_plane_atomic_update` and
> > > stored in a `vkms_plane_state`. Those conversion matrices of each
> > > encoding and range were obtained by rounding the values of the original
> > > conversion matrices multiplied by 2^32. This is done to avoid the use of
> > > floating point operations.
> > >
> > > The same reading function is used for YUV and YVU formats. As the only
> > > difference between those two category of formats is the order of field, a
> > > simple swap in conversion matrix columns allows using the same function.
> >
> > Sounds good!
> >
> > > Signed-off-by: Arthur Grillo <[email protected]>
> > > [Louis Chauvet:
> > > - Adapted Arthur's work
> > > - Implemented the read_line_t callbacks for yuv
> > > - add struct conversion_matrix
> > > - remove struct pixel_yuv_u8
> > > - update the commit message
> > > - Merge the modifications from Arthur]
> > > Signed-off-by: Louis Chauvet <[email protected]>
> > > ---
> > > drivers/gpu/drm/vkms/vkms_drv.h | 22 ++
> > > drivers/gpu/drm/vkms/vkms_formats.c | 431 ++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/vkms/vkms_formats.h | 4 +
> > > drivers/gpu/drm/vkms/vkms_plane.c | 17 +-
> > > 4 files changed, 473 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > > index 23e1d247468d..f3116084de5a 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h

..

> > > +static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
> > > + struct conversion_matrix *matrix)
> >
> > If you are using the "swap the matrix columns" trick, then you cannot
> > call these cb, cr nor even u,v, because they might be the opposite.
> > They are simply the first and second chroma channel, and their meaning
> > depends on the given matrix.
>
> I will rename them for v6, channel_1 and channel_2.
>
> > > +{
> > > + u8 r, g, b;
> > > + s64 fp_y, fp_cb, fp_cr;
> > > + s64 fp_r, fp_g, fp_b;
> > > +
> > > + fp_y = y - matrix->y_offset;
> > > + fp_cb = cb - 128;
> > > + fp_cr = cr - 128;
> >
> > This looks like an incorrect way to convert u8 to fixed-point, but...
> >
> > > +
> > > + fp_y = drm_int2fixp(fp_y);
> > > + fp_cb = drm_int2fixp(fp_cb);
> > > + fp_cr = drm_int2fixp(fp_cr);
> >
> > I find it confusing to re-purpose variables like this.
> >
> > I'd do just
> >
> > fp_c1 = drm_int2fixp((int)c1 - 128);
>
> I agree with this remark, I will change it for the v6.
>
> > If the function arguments were int to begin with, then the cast would
> > be obviously unnecessary.
>
> For this I'm less sure. The name of the function and the usage is
> explicit: we want to use u8 as input. As we manipulate pointers in
> read_line, I don't know how it will works if the pointer is dereferenced
> to a int instead of a u8.

Dereference operator acts on its input type. What happens to the result
is irrelevant.

If we have

u8 *p = ...;

void foo(int x);

then you can call

foo(*v);

if that was your question. Dereference acts on u8* which results in u8.
Then it gets implicitly cast to int.

However, you have a semantic reason to keep the argument as u8, and
that is fine.

> > So, what you have in fp variables at this point is fractional numbers
> > in the 8-bit integer scale. However, because the target format is
> > 16-bit, you should not show the extra precision away here. Instead,
> > multiply by 257 to bring the values to 16-bit scale, and do the RGB
> > clamping to 16-bit, not 8-bit.
> >
> > > +
> > > + fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
> > > + drm_fixp_mul(matrix->matrix[0][1], fp_cb) +
> > > + drm_fixp_mul(matrix->matrix[0][2], fp_cr);
> > > + fp_g = drm_fixp_mul(matrix->matrix[1][0], fp_y) +
> > > + drm_fixp_mul(matrix->matrix[1][1], fp_cb) +
> > > + drm_fixp_mul(matrix->matrix[1][2], fp_cr);
> > > + fp_b = drm_fixp_mul(matrix->matrix[2][0], fp_y) +
> > > + drm_fixp_mul(matrix->matrix[2][1], fp_cb) +
> > > + drm_fixp_mul(matrix->matrix[2][2], fp_cr);
> > > +
> > > + fp_r = drm_fixp2int_round(fp_r);
> > > + fp_g = drm_fixp2int_round(fp_g);
> > > + fp_b = drm_fixp2int_round(fp_b);
> > > +
> > > + r = clamp(fp_r, 0, 0xff);
> > > + g = clamp(fp_g, 0, 0xff);
> > > + b = clamp(fp_b, 0, 0xff);
> > > +
> > > + return argb_u16_from_u8888(255, r, g, b);
> >
> > Going through argb_u16_from_u8888() will throw away precision.
>
> I tried to fix it in the v6, IGT tests pass. If something is wrong in the
> v6, please let me know.
>
> > > +}
> > > +
> > > /*
> > > * The following functions are read_line function for each pixel format supported by VKMS.
> > > *
> > > @@ -293,6 +367,79 @@ static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> > > }
> > > }
> > >
> > > +/*
> > > + * This callback can be used for yuv and yvu formats, given a properly modified conversion matrix
> > > + * (column inversion)
> >
> > Would be nice to explain what semi_planar_yuv means, so that the
> > documentation for these functions would show how they differ rather
> > than all saying exactly the same thing.
>
> /* This callback can be used for YUV format where each color component is
> * stored in a different plane (often called planar formats). It will
> * handle correctly subsampling.
>
> /*
> * This callback can be used for YUV formats where U and V values are
> * stored in the same plane (often called semi-planar formats). It will
> * corectly handle subsampling.
> *
> * The conversion matrix stored in the @plane is used to:
> * - Apply the correct color range and encoding
> * - Convert YUV and YVU with the same function (a simple column swap is
> * needed)
> */

Sounds good. I'd just drop the "It will handle correctly subsampling."
because all code is supposed to be correct by default.

If there is a function that intentionally overlooks something, that
certainly should be documented.

Thanks,
pq

Attachments:

(No filename) (849.00 B)
OpenPGP digital signature