Re: [PATCH v5 16/16] drm/vkms: Add support for DRM_FORMAT_R*
Le 28/03/24 - 16:00, Pekka Paalanen a écrit :
> On Wed, 13 Mar 2024 18:45:10 +0100
> Louis Chauvet wrote:
>
> > This add the support for:
> > - R1/R2/R4/R8
> >
> > R1 format was tested with [1] and [2].
> >
> > [1]:
> > https://lore.kernel.org/r/20240313-new_rotation-v2-0-6230fd5ca...@bootlin.com
> > [2]:
> > https://lore.kernel.org/igt-dev/20240306-b4-kms_tests-v1-0-8fe451efd...@bootlin.com/
> >
> > Signed-off-by: Louis Chauvet
> > ---
> > 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_u(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, _pixels,
> > _x, _y);
>
> Maybe assert that rem_y = 0? Or block_h = 1.
Done for the v6.
> > + int bit_offset = (int)rem_x * bit_per_pixel;
>
> Why cast rem_x to int when it was defined to be int?
Because it was not the case for my first implementation, and I miss this
cast... Thanks.
> > + 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.
Ok, I will change it for the v6.
> > +
> > + 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?
Obviously yes... I missunderstood it... fixed in the v6 (and it will be
fixed in the next iteration of my igt series).
> That how I would understand the comments in drm_fourcc.h.
>
> Again a reason to avoid a solid color fill in IGT.
Yes, but in this case I wrote the IGT test too... So "wrong + wrong =
test SUCCESS" :)
There are some patterns in IGT, but they are only for "color". None of
them are available for "black and white"/"gray" formats.
> > +
> > + *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[])
> > +{
> > +
Re: [PATCH v5 16/16] drm/vkms: Add support for DRM_FORMAT_R*
On Wed, 13 Mar 2024 18:45:10 +0100
Louis Chauvet wrote:
> This add the support for:
> - R1/R2/R4/R8
>
> R1 format was tested with [1] and [2].
>
> [1]:
> https://lore.kernel.org/r/20240313-new_rotation-v2-0-6230fd5ca...@bootlin.com
> [2]:
> https://lore.kernel.org/igt-dev/20240306-b4-kms_tests-v1-0-8fe451efd...@bootlin.com/
>
> Signed-off-by: Louis Chauvet
> ---
> 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_u(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, _pixels,
> _x, _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
[PATCH v5 16/16] drm/vkms: Add support for DRM_FORMAT_R*
This add the support for:
- R1/R2/R4/R8
R1 format was tested with [1] and [2].
[1]:
https://lore.kernel.org/r/20240313-new_rotation-v2-0-6230fd5ca...@bootlin.com
[2]:
https://lore.kernel.org/igt-dev/20240306-b4-kms_tests-v1-0-8fe451efd...@bootlin.com/
Signed-off-by: Louis Chauvet
---
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_u(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, _pixels,
_x, _y);
+ int bit_offset = (int)rem_x * bit_per_pixel;
+ int step = get_step_next_block(plane->frame_info->fb, direction, 0);
+ int mask = (0x1 << bit_per_pixel) - 1;
+
+ 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;
+
+ *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, _pixels,
_x, _y);
+
+ while (out_pixel < end) {
+ *out_pixel = argb_u16_from_gray8(*src_pixels);
+ src_pixels += step;
+ out_pixel += 1;
+ }
+}
+
static void ARGB_read_line(const struct vkms_plane_state *plane, int
x_start, int y_start,
enum pixel_read_direction direction, int count,
