[PATCH v2 0/7] drm/vkms: introduce plane rotation property

[PATCH v2 0/7] drm/vkms: introduce plane rotation property

[Maíra Canal]

This patchset implements all possible rotation value in vkms. All operations
were implemented by software by changing the way the pixels are read. The way
the blending is performed can be depicted as:

- rotate-0:
                (x) ---->
    ----------------------
(y) |                    |
  | |                    |
  | |                    |
  ˇ |                    |
    ----------------------

- rotate-90:
    <---- (y)
    ----------------------
(x) |                    |
  | |                    |
  | |                    |
  ˇ |                    |
    ----------------------

- rotate-180:
    <---- (x)
    ----------------------
(y) |                    |
  ^ |                    |
  | |                    |
  | |                    |
    ----------------------

- rotate-270:
                (y) ---->
    ----------------------
(x) |                    |
  ^ |                    |
  | |                    |
  | |                    |
    ----------------------

- reflect-x:
    <---- (x)
    ----------------------
(y) |                    |
  | |                    |
  | |                    |
  ˇ |                    |
    ----------------------

- reflect-y:
                (x) ---->
    ----------------------
(y) |                    |
  ^ |                    |
  | |                    |
  | |                    |
    ----------------------

The patchset was tested with IGT's kms_rotation_crc tests and also with some
additional tests [1] for the reflection operations.

In order to avoid code duplication, I introduced a patch that isolates the
pixel format convertion and wraps it in a single loop.

v1 -> v2:

* Add patch to isolate pixel format convertion (Arthur Grillo).

[1] https://patchwork.freedesktop.org/series/116025/

Best Regards,
- Maíra Canal

Maíra Canal (7):
  drm/vkms: isolate pixel conversion functionality
  drm/vkms: add rotate-0 and rotate-180 properties
  drm/vkms: add rotate-90 property
  drm/vkms: add rotate-270 property
  drm/vkms: add reflect-x property
  drm/vkms: add reflect-y property
  drm/vkms: drop "Rotation" TODO

 Documentation/gpu/vkms.rst           |   2 +-
 drivers/gpu/drm/vkms/vkms_composer.c |  79 +++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.h      |   5 +-
 drivers/gpu/drm/vkms/vkms_formats.c  | 153 ++++++++++++++-------------
 drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
 drivers/gpu/drm/vkms/vkms_plane.c    |  12 ++-
 6 files changed, 163 insertions(+), 90 deletions(-)

-- 
2.39.2

[PATCH v2 1/7] drm/vkms: isolate pixel conversion functionality

[Maíra Canal]

Currently, the pixel conversion functions repeat the same loop to
iterate the rows. Instead of repeating the same code for each pixel
format, create a function to wrap the loop and isolate the pixel
conversion functionality.

Suggested-by: Arthur Grillo <arthurgrillo@riseup.net>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c |   4 +-
 drivers/gpu/drm/vkms/vkms_drv.h      |   4 +-
 drivers/gpu/drm/vkms/vkms_formats.c  | 136 ++++++++++++---------------
 drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
 drivers/gpu/drm/vkms/vkms_plane.c    |   2 +-
 5 files changed, 65 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 8e53fa80742b..80164e79af00 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -99,7 +99,7 @@ static void blend(struct vkms_writeback_job *wb,
 			if (!check_y_limit(plane[i]->frame_info, y))
 				continue;
 
-			plane[i]->plane_read(stage_buffer, plane[i]->frame_info, y);
+			vkms_compose_row(stage_buffer, plane[i], y);
 			pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
 					    output_buffer);
 		}
@@ -118,7 +118,7 @@ static int check_format_funcs(struct vkms_crtc_state *crtc_state,
 	u32 n_active_planes = crtc_state->num_active_planes;
 
 	for (size_t i = 0; i < n_active_planes; i++)
-		if (!planes[i]->plane_read)
+		if (!planes[i]->pixel_read)
 			return -1;
 
 	if (active_wb && !active_wb->wb_write)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 4a248567efb2..d7ad813d7ae7 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -56,8 +56,7 @@ struct vkms_writeback_job {
 struct vkms_plane_state {
 	struct drm_shadow_plane_state base;
 	struct vkms_frame_info *frame_info;
-	void (*plane_read)(struct line_buffer *buffer,
-			   const struct vkms_frame_info *frame_info, int y);
+	void (*pixel_read)(u16 *src_buffer, struct pixel_argb_u16 *out_pixels, int x);
 };
 
 struct vkms_plane {
@@ -155,6 +154,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
 void vkms_set_composer(struct vkms_output *out, bool enabled);
+void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y);
 
 /* Writeback */
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index d4950688b3f1..02b0fc5a0839 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -42,100 +42,82 @@ static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y
 	return packed_pixels_addr(frame_info, x_src, y_src);
 }
 
-static void ARGB8888_to_argb_u16(struct line_buffer *stage_buffer,
-				 const struct vkms_frame_info *frame_info, int y)
+static void ARGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
-	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u8 *src_pixels = get_packed_src_addr(frame_info, y);
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			    stage_buffer->n_pixels);
-
-	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
-		/*
-		 * The 257 is the "conversion ratio". This number is obtained by the
-		 * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
-		 * the best color value in a pixel format with more possibilities.
-		 * A similar idea applies to others RGB color conversions.
-		 */
-		out_pixels[x].a = (u16)src_pixels[3] * 257;
-		out_pixels[x].r = (u16)src_pixels[2] * 257;
-		out_pixels[x].g = (u16)src_pixels[1] * 257;
-		out_pixels[x].b = (u16)src_pixels[0] * 257;
-	}
+	u8 *pixels = (u8 *)src_pixels;
+
+	/*
+	 * The 257 is the "conversion ratio". This number is obtained by the
+	 * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
+	 * the best color value in a pixel format with more possibilities.
+	 * A similar idea applies to others RGB color conversions.
+	 */
+	out_pixels[x].a = (u16)pixels[3] * 257;
+	out_pixels[x].r = (u16)pixels[2] * 257;
+	out_pixels[x].g = (u16)pixels[1] * 257;
+	out_pixels[x].b = (u16)pixels[0] * 257;
 }
 
-static void XRGB8888_to_argb_u16(struct line_buffer *stage_buffer,
-				 const struct vkms_frame_info *frame_info, int y)
+static void XRGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
-	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u8 *src_pixels = get_packed_src_addr(frame_info, y);
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			    stage_buffer->n_pixels);
+	u8 *pixels = (u8 *)src_pixels;
 
-	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
-		out_pixels[x].a = (u16)0xffff;
-		out_pixels[x].r = (u16)src_pixels[2] * 257;
-		out_pixels[x].g = (u16)src_pixels[1] * 257;
-		out_pixels[x].b = (u16)src_pixels[0] * 257;
-	}
+	out_pixels[x].a = (u16)0xffff;
+	out_pixels[x].r = (u16)pixels[2] * 257;
+	out_pixels[x].g = (u16)pixels[1] * 257;
+	out_pixels[x].b = (u16)pixels[0] * 257;
 }
 
-static void ARGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
-				     const struct vkms_frame_info *frame_info,
-				     int y)
+static void ARGB16161616_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
-	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u16 *src_pixels = get_packed_src_addr(frame_info, y);
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			    stage_buffer->n_pixels);
-
-	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
-		out_pixels[x].a = le16_to_cpu(src_pixels[3]);
-		out_pixels[x].r = le16_to_cpu(src_pixels[2]);
-		out_pixels[x].g = le16_to_cpu(src_pixels[1]);
-		out_pixels[x].b = le16_to_cpu(src_pixels[0]);
-	}
+	out_pixels[x].a = le16_to_cpu(src_pixels[3]);
+	out_pixels[x].r = le16_to_cpu(src_pixels[2]);
+	out_pixels[x].g = le16_to_cpu(src_pixels[1]);
+	out_pixels[x].b = le16_to_cpu(src_pixels[0]);
 }
 
-static void XRGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
-				     const struct vkms_frame_info *frame_info,
-				     int y)
+static void XRGB16161616_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
-	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u16 *src_pixels = get_packed_src_addr(frame_info, y);
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			    stage_buffer->n_pixels);
-
-	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
-		out_pixels[x].a = (u16)0xffff;
-		out_pixels[x].r = le16_to_cpu(src_pixels[2]);
-		out_pixels[x].g = le16_to_cpu(src_pixels[1]);
-		out_pixels[x].b = le16_to_cpu(src_pixels[0]);
-	}
+	out_pixels[x].a = (u16)0xffff;
+	out_pixels[x].r = le16_to_cpu(src_pixels[2]);
+	out_pixels[x].g = le16_to_cpu(src_pixels[1]);
+	out_pixels[x].b = le16_to_cpu(src_pixels[0]);
 }
 
-static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
-			       const struct vkms_frame_info *frame_info, int y)
+static void RGB565_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
-	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u16 *src_pixels = get_packed_src_addr(frame_info, y);
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			       stage_buffer->n_pixels);
-
 	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));
 
-	for (size_t x = 0; x < x_limit; x++, src_pixels++) {
-		u16 rgb_565 = le16_to_cpu(*src_pixels);
-		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);
+	u16 rgb_565 = le16_to_cpu(*src_pixels);
+	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);
 
-		out_pixels[x].a = (u16)0xffff;
-		out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
-		out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
-		out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
-	}
+	out_pixels[x].a = (u16)0xffff;
+	out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
+	out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
+	out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
+}
+
+void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
+{
+	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
+	u16 *src_pixels = get_packed_src_addr(plane->frame_info, y);
+	int limit = min_t(size_t, drm_rect_width(&plane->frame_info->dst),
+			  stage_buffer->n_pixels);
+	int format = plane->frame_info->fb->format->format;
+	int pixel_size;
+
+	if (format == DRM_FORMAT_RGB565)
+		pixel_size = 1;
+	else if (format == DRM_FORMAT_ARGB8888 || format == DRM_FORMAT_XRGB8888)
+		pixel_size = 2;
+	else
+		pixel_size = 4;
+
+	for (size_t x = 0; x < limit; x++, src_pixels += pixel_size)
+		plane->pixel_read(src_pixels, out_pixels, x);
 }
 
 /*
@@ -249,7 +231,7 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
 	}
 }
 
-void *get_frame_to_line_function(u32 format)
+void *get_pixel_conversion_function(u32 format)
 {
 	switch (format) {
 	case DRM_FORMAT_ARGB8888:
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index 43b7c1979018..c5b113495d0c 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -5,7 +5,7 @@
 
 #include "vkms_drv.h"
 
-void *get_frame_to_line_function(u32 format);
+void *get_pixel_conversion_function(u32 format);
 
 void *get_line_to_frame_function(u32 format);
 
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index c41cec7dcb70..0a23875900ec 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -123,7 +123,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info->offset = fb->offsets[0];
 	frame_info->pitch = fb->pitches[0];
 	frame_info->cpp = fb->format->cpp[0];
-	vkms_plane_state->plane_read = get_frame_to_line_function(fmt);
+	vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,
-- 
2.39.2

[PATCH v2 2/7] drm/vkms: add rotate-0 and rotate-180 properties

[Maíra Canal]

Currently, vkms doesn't support any rotation property. Therefore,
improve the vkms IGT test coverage by adding the rotate-0 and rotate-180
properties to vkms. The rotation was implemented by software: invert the
way the blending occurs by reverse reading the x and y axis.

Tested with igt@kms_rotation_crc@primary-rotation-180,
igt@kms_rotation_crc@sprite-rotation-180, and
igt@kms_rotation_crc@cursor-rotation-180.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 39 ++++++++++++++++++++++++----
 drivers/gpu/drm/vkms/vkms_drv.h      |  1 +
 drivers/gpu/drm/vkms/vkms_formats.c  | 16 +++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c    | 12 +++++++++
 4 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 80164e79af00..914d85ac1654 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -11,6 +11,23 @@
 
 #include "vkms_drv.h"
 
+static struct pixel_argb_u16 *get_out_buffer(const struct vkms_frame_info *frame_info,
+					     struct line_buffer *output_buffer)
+{
+	struct pixel_argb_u16 *out = output_buffer->pixels;
+
+	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_180:
+		out -= frame_info->dst.x1;
+		break;
+	default:
+		out += frame_info->dst.x1;
+		break;
+	}
+
+	return out;
+}
+
 static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
 {
 	u32 new_color;
@@ -39,8 +56,7 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 				struct line_buffer *stage_buffer,
 				struct line_buffer *output_buffer)
 {
-	int x_dst = frame_info->dst.x1;
-	struct pixel_argb_u16 *out = output_buffer->pixels + x_dst;
+	struct pixel_argb_u16 *out = get_out_buffer(frame_info, output_buffer);
 	struct pixel_argb_u16 *in = stage_buffer->pixels;
 	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
 			    stage_buffer->n_pixels);
@@ -53,6 +69,16 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 	}
 }
 
+static int get_y_pos(struct vkms_frame_info *frame_info, int y)
+{
+	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_180:
+		return drm_rect_height(&frame_info->dst) - y - 1;
+	default:
+		return y;
+	}
+}
+
 static bool check_y_limit(struct vkms_frame_info *frame_info, int y)
 {
 	if (y >= frame_info->dst.y1 && y < frame_info->dst.y2)
@@ -86,6 +112,7 @@ static void blend(struct vkms_writeback_job *wb,
 {
 	struct vkms_plane_state **plane = crtc_state->active_planes;
 	u32 n_active_planes = crtc_state->num_active_planes;
+	int y_pos;
 
 	const struct pixel_argb_u16 background_color = { .a = 0xffff };
 
@@ -96,10 +123,12 @@ static void blend(struct vkms_writeback_job *wb,
 
 		/* The active planes are composed associatively in z-order. */
 		for (size_t i = 0; i < n_active_planes; i++) {
-			if (!check_y_limit(plane[i]->frame_info, y))
+			y_pos = get_y_pos(plane[i]->frame_info, y);
+
+			if (!check_y_limit(plane[i]->frame_info, y_pos))
 				continue;
 
-			vkms_compose_row(stage_buffer, plane[i], y);
+			vkms_compose_row(stage_buffer, plane[i], y_pos);
 			pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
 					    output_buffer);
 		}
@@ -107,7 +136,7 @@ static void blend(struct vkms_writeback_job *wb,
 		*crc32 = crc32_le(*crc32, (void *)output_buffer->pixels, row_size);
 
 		if (wb)
-			wb->wb_write(&wb->wb_frame_info, output_buffer, y);
+			wb->wb_write(&wb->wb_frame_info, output_buffer, y_pos);
 	}
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index d7ad813d7ae7..8344395d7745 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -27,6 +27,7 @@ struct vkms_frame_info {
 	struct drm_framebuffer *fb;
 	struct drm_rect src, dst;
 	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
+	unsigned int rotation;
 	unsigned int offset;
 	unsigned int pitch;
 	unsigned int cpp;
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 02b0fc5a0839..7a98d4d75f17 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -42,6 +42,13 @@ static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y
 	return packed_pixels_addr(frame_info, x_src, y_src);
 }
 
+static int get_x_position(const struct vkms_frame_info *frame_info, int x_limit, int x)
+{
+	if (frame_info->rotation & DRM_MODE_ROTATE_180)
+		return x_limit - x - 1;
+	return x;
+}
+
 static void ARGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
 	u8 *pixels = (u8 *)src_pixels;
@@ -107,7 +114,7 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	int limit = min_t(size_t, drm_rect_width(&plane->frame_info->dst),
 			  stage_buffer->n_pixels);
 	int format = plane->frame_info->fb->format->format;
-	int pixel_size;
+	int pixel_size, x_pos;
 
 	if (format == DRM_FORMAT_RGB565)
 		pixel_size = 1;
@@ -116,8 +123,11 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	else
 		pixel_size = 4;
 
-	for (size_t x = 0; x < limit; x++, src_pixels += pixel_size)
-		plane->pixel_read(src_pixels, out_pixels, x);
+	for (size_t x = 0; x < limit; x++, src_pixels += pixel_size) {
+		x_pos = get_x_position(plane->frame_info, limit, x);
+
+		plane->pixel_read(src_pixels, out_pixels, x_pos);
+	}
 }
 
 /*
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0a23875900ec..f6a884b5872c 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -4,6 +4,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_blend.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
@@ -120,6 +121,13 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info->fb = fb;
 	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
 	drm_framebuffer_get(frame_info->fb);
+	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
+						     DRM_MODE_ROTATE_0 |
+						     DRM_MODE_ROTATE_180);
+
+	drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst),
+			drm_rect_height(&frame_info->dst), frame_info->rotation);
+
 	frame_info->offset = fb->offsets[0];
 	frame_info->pitch = fb->pitches[0];
 	frame_info->cpp = fb->format->cpp[0];
@@ -229,5 +237,9 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 
 	drm_plane_helper_add(&plane->base, funcs);
 
+	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
+					   DRM_MODE_ROTATE_0 |
+					   DRM_MODE_ROTATE_180);
+
 	return plane;
 }
-- 
2.39.2

[PATCH v2 3/7] drm/vkms: add rotate-90 property

[Maíra Canal]

Currently, vkms only supports the rotate-180 property. Therefore,
improve the vkms IGT test coverage by adding the rotate-90 property to
vkms. The rotation was implement by software: rotate the way the
blending occurs by making the source x axis be the destination y axis and
the source y axis be the destination x axis.

Tested with igt@kms_rotation_crc@primary-rotation-90,
igt@kms_rotation_crc@sprite-rotation-90, and
igt@kms_rotation_crc@sprite-rotation-90-pos-100-0.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 29 ++++++++++++++++++++--------
 drivers/gpu/drm/vkms/vkms_formats.c  | 28 ++++++++++++++++++++-------
 drivers/gpu/drm/vkms/vkms_plane.c    |  2 ++
 3 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 914d85ac1654..824bb65badb7 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -17,6 +17,9 @@ static struct pixel_argb_u16 *get_out_buffer(const struct vkms_frame_info *frame
 	struct pixel_argb_u16 *out = output_buffer->pixels;
 
 	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_90:
+		out -= frame_info->dst.y1;
+		break;
 	case DRM_MODE_ROTATE_180:
 		out -= frame_info->dst.x1;
 		break;
@@ -58,10 +61,12 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 {
 	struct pixel_argb_u16 *out = get_out_buffer(frame_info, output_buffer);
 	struct pixel_argb_u16 *in = stage_buffer->pixels;
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
-			    stage_buffer->n_pixels);
+	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
+
+	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+		limit = min_t(size_t, drm_rect_height(&frame_info->dst), stage_buffer->n_pixels);
 
-	for (int x = 0; x < x_limit; x++) {
+	for (int x = 0; x < limit; x++) {
 		out[x].a = (u16)0xffff;
 		out[x].r = pre_mul_blend_channel(in[x].r, out[x].r, in[x].a);
 		out[x].g = pre_mul_blend_channel(in[x].g, out[x].g, in[x].a);
@@ -72,6 +77,10 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 {
 	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_90:
+		if (y - frame_info->dst.x1 < 0)
+			return -1;
+		return frame_info->dst.x2 - y - 1;
 	case DRM_MODE_ROTATE_180:
 		return drm_rect_height(&frame_info->dst) - y - 1;
 	default:
@@ -79,11 +88,15 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 	}
 }
 
-static bool check_y_limit(struct vkms_frame_info *frame_info, int y)
+static bool check_limit(struct vkms_frame_info *frame_info, int pos)
 {
-	if (y >= frame_info->dst.y1 && y < frame_info->dst.y2)
-		return true;
-
+	if (frame_info->rotation & DRM_MODE_ROTATE_90) {
+		if (pos >= 0 && pos < drm_rect_width(&frame_info->dst))
+			return true;
+	} else {
+		if (pos >= frame_info->dst.y1 && pos < frame_info->dst.y2)
+			return true;
+	}
 	return false;
 }
 
@@ -125,7 +138,7 @@ static void blend(struct vkms_writeback_job *wb,
 		for (size_t i = 0; i < n_active_planes; i++) {
 			y_pos = get_y_pos(plane[i]->frame_info, y);
 
-			if (!check_y_limit(plane[i]->frame_info, y_pos))
+			if (!check_limit(plane[i]->frame_info, y_pos))
 				continue;
 
 			vkms_compose_row(stage_buffer, plane[i], y_pos);
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 7a98d4d75f17..90b72b0ff8c9 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -49,6 +49,20 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int x_limit,
 	return x;
 }
 
+static int get_limit(const struct vkms_frame_info *frame_info, struct line_buffer *stage_buffer)
+{
+	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+		return min_t(size_t, drm_rect_height(&frame_info->dst), stage_buffer->n_pixels);
+	return min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
+}
+
+static void *get_src_pixels(const struct vkms_frame_info *frame_info, int x, int y, int pixel_size)
+{
+	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+		return get_packed_src_addr(frame_info, x + frame_info->dst.y1) + pixel_size * y;
+	return get_packed_src_addr(frame_info, y) + pixel_size * x;
+}
+
 static void ARGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
 {
 	u8 *pixels = (u8 *)src_pixels;
@@ -110,21 +124,21 @@ static void RGB565_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixel
 void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
 {
 	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
-	u16 *src_pixels = get_packed_src_addr(plane->frame_info, y);
-	int limit = min_t(size_t, drm_rect_width(&plane->frame_info->dst),
-			  stage_buffer->n_pixels);
+	int limit = get_limit(plane->frame_info, stage_buffer);
 	int format = plane->frame_info->fb->format->format;
 	int pixel_size, x_pos;
+	u16 *src_pixels;
 
 	if (format == DRM_FORMAT_RGB565)
-		pixel_size = 1;
-	else if (format == DRM_FORMAT_ARGB8888 || format == DRM_FORMAT_XRGB8888)
 		pixel_size = 2;
-	else
+	else if (format == DRM_FORMAT_ARGB8888 || format == DRM_FORMAT_XRGB8888)
 		pixel_size = 4;
+	else
+		pixel_size = 8;
 
-	for (size_t x = 0; x < limit; x++, src_pixels += pixel_size) {
+	for (size_t x = 0; x < limit; x++) {
 		x_pos = get_x_position(plane->frame_info, limit, x);
+		src_pixels = get_src_pixels(plane->frame_info, x, y, pixel_size);
 
 		plane->pixel_read(src_pixels, out_pixels, x_pos);
 	}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index f6a884b5872c..ce2ebe56f6e4 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -123,6 +123,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	drm_framebuffer_get(frame_info->fb);
 	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
 						     DRM_MODE_ROTATE_0 |
+						     DRM_MODE_ROTATE_90 |
 						     DRM_MODE_ROTATE_180);
 
 	drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst),
@@ -239,6 +240,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 
 	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
 					   DRM_MODE_ROTATE_0 |
+					   DRM_MODE_ROTATE_90 |
 					   DRM_MODE_ROTATE_180);
 
 	return plane;
-- 
2.39.2

[PATCH v2 4/7] drm/vkms: add rotate-270 property

[Maíra Canal]

Currently, vkms only supports the rotate-90 and rotate-180 properties.
Therefore, improve the vkms IGT test coverage by adding the rotate-270
property to vkms. The rotation was implement by software: rotate the way
the blending occurs by making the source x axis be the destination y axis
and the source y axis be the destination x axis and reverse-read the axis.

Tested with igt@kms_rotation_crc@primary-rotation-270, and
igt@kms_rotation_crc@sprite-rotation-270.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 12 ++++++++++--
 drivers/gpu/drm/vkms/vkms_formats.c  |  7 ++++---
 drivers/gpu/drm/vkms/vkms_plane.c    |  6 ++++--
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 824bb65badb7..b05bd008aeab 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -4,6 +4,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_blend.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_vblank.h>
@@ -23,6 +24,9 @@ static struct pixel_argb_u16 *get_out_buffer(const struct vkms_frame_info *frame
 	case DRM_MODE_ROTATE_180:
 		out -= frame_info->dst.x1;
 		break;
+	case DRM_MODE_ROTATE_270:
+		out += frame_info->dst.y1;
+		break;
 	default:
 		out += frame_info->dst.x1;
 		break;
@@ -63,7 +67,7 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 	struct pixel_argb_u16 *in = stage_buffer->pixels;
 	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
 
-	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+	if (drm_rotation_90_or_270(frame_info->rotation))
 		limit = min_t(size_t, drm_rect_height(&frame_info->dst), stage_buffer->n_pixels);
 
 	for (int x = 0; x < limit; x++) {
@@ -83,6 +87,10 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 		return frame_info->dst.x2 - y - 1;
 	case DRM_MODE_ROTATE_180:
 		return drm_rect_height(&frame_info->dst) - y - 1;
+	case DRM_MODE_ROTATE_270:
+		if (y + frame_info->dst.x1 < 0)
+			return -1;
+		return y + frame_info->dst.x1;
 	default:
 		return y;
 	}
@@ -90,7 +98,7 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 
 static bool check_limit(struct vkms_frame_info *frame_info, int pos)
 {
-	if (frame_info->rotation & DRM_MODE_ROTATE_90) {
+	if (drm_rotation_90_or_270(frame_info->rotation)) {
 		if (pos >= 0 && pos < drm_rect_width(&frame_info->dst))
 			return true;
 	} else {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 90b72b0ff8c9..c4513f3d6876 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -2,6 +2,7 @@
 
 #include <linux/kernel.h>
 #include <linux/minmax.h>
+#include <drm/drm_blend.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_fixed.h>
 
@@ -44,21 +45,21 @@ static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y
 
 static int get_x_position(const struct vkms_frame_info *frame_info, int x_limit, int x)
 {
-	if (frame_info->rotation & DRM_MODE_ROTATE_180)
+	if (frame_info->rotation & (DRM_MODE_ROTATE_180 | DRM_MODE_ROTATE_270))
 		return x_limit - x - 1;
 	return x;
 }
 
 static int get_limit(const struct vkms_frame_info *frame_info, struct line_buffer *stage_buffer)
 {
-	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+	if (drm_rotation_90_or_270(frame_info->rotation))
 		return min_t(size_t, drm_rect_height(&frame_info->dst), stage_buffer->n_pixels);
 	return min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
 }
 
 static void *get_src_pixels(const struct vkms_frame_info *frame_info, int x, int y, int pixel_size)
 {
-	if (frame_info->rotation & DRM_MODE_ROTATE_90)
+	if (drm_rotation_90_or_270(frame_info->rotation))
 		return get_packed_src_addr(frame_info, x + frame_info->dst.y1) + pixel_size * y;
 	return get_packed_src_addr(frame_info, y) + pixel_size * x;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index ce2ebe56f6e4..5f69e0efd85f 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -124,7 +124,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
 						     DRM_MODE_ROTATE_0 |
 						     DRM_MODE_ROTATE_90 |
-						     DRM_MODE_ROTATE_180);
+						     DRM_MODE_ROTATE_180 |
+						     DRM_MODE_ROTATE_270);
 
 	drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst),
 			drm_rect_height(&frame_info->dst), frame_info->rotation);
@@ -241,7 +242,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
 					   DRM_MODE_ROTATE_0 |
 					   DRM_MODE_ROTATE_90 |
-					   DRM_MODE_ROTATE_180);
+					   DRM_MODE_ROTATE_180 |
+					   DRM_MODE_ROTATE_270);
 
 	return plane;
 }
-- 
2.39.2

[PATCH v2 5/7] drm/vkms: add reflect-x property

[Maíra Canal]

Currently, vkms doesn't support any reflection property. Therefore, add
the reflect-x property to vkms through a software implementation of the
operation. This is possible by reverse reading the x axis during the
blending.

Tested with igt@kms_rotation_crc@primary-reflect-x and
igt@kms_rotation_crc@sprite-reflect-x [1].

[1] https://patchwork.freedesktop.org/series/116025/

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 2 +-
 drivers/gpu/drm/vkms/vkms_plane.c   | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index c4513f3d6876..2f8b41bf9ff9 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -45,7 +45,7 @@ static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y
 
 static int get_x_position(const struct vkms_frame_info *frame_info, int x_limit, int x)
 {
-	if (frame_info->rotation & (DRM_MODE_ROTATE_180 | DRM_MODE_ROTATE_270))
+	if (frame_info->rotation & (DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_180 | DRM_MODE_ROTATE_270))
 		return x_limit - x - 1;
 	return x;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 5f69e0efd85f..11662afa9fe4 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -125,7 +125,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 						     DRM_MODE_ROTATE_0 |
 						     DRM_MODE_ROTATE_90 |
 						     DRM_MODE_ROTATE_180 |
-						     DRM_MODE_ROTATE_270);
+						     DRM_MODE_ROTATE_270 |
+						     DRM_MODE_REFLECT_X);
 
 	drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst),
 			drm_rect_height(&frame_info->dst), frame_info->rotation);
@@ -243,7 +244,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 					   DRM_MODE_ROTATE_0 |
 					   DRM_MODE_ROTATE_90 |
 					   DRM_MODE_ROTATE_180 |
-					   DRM_MODE_ROTATE_270);
+					   DRM_MODE_ROTATE_270 |
+					   DRM_MODE_REFLECT_X);
 
 	return plane;
 }
-- 
2.39.2

[PATCH v2 6/7] drm/vkms: add reflect-y property

[Maíra Canal]

Currently, vkms only support the reflect-x property. Therefore, add the
reflect-y property to vkms through a software implementation of the
operation. This is possible by reverse reading the y axis during the
blending.

Now, vkms support all possible rotation values.

Tested with igt@kms_rotation_crc@primary-reflect-y and
igt@kms_rotation_crc@sprite-reflect-y [1].

[1] https://patchwork.freedesktop.org/series/116025/

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
 drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index b05bd008aeab..19d1078e9d34 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 			return -1;
 		return y + frame_info->dst.x1;
 	default:
-		return y;
+		break;
 	}
+
+	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
+		return drm_rect_height(&frame_info->dst) - y - 1;
+
+	return y;
 }
 
 static bool check_limit(struct vkms_frame_info *frame_info, int pos)
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 11662afa9fe4..d08bda869a24 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info->fb = fb;
 	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
 	drm_framebuffer_get(frame_info->fb);
-	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
-						     DRM_MODE_ROTATE_0 |
-						     DRM_MODE_ROTATE_90 |
-						     DRM_MODE_ROTATE_180 |
-						     DRM_MODE_ROTATE_270 |
-						     DRM_MODE_REFLECT_X);
+	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
+						     DRM_MODE_REFLECT_MASK);
 
 	drm_rect_rotate(&frame_info->dst, drm_rect_width(&frame_info->dst),
 			drm_rect_height(&frame_info->dst), frame_info->rotation);
@@ -240,12 +236,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 
 	drm_plane_helper_add(&plane->base, funcs);
 
-	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
-					   DRM_MODE_ROTATE_0 |
-					   DRM_MODE_ROTATE_90 |
-					   DRM_MODE_ROTATE_180 |
-					   DRM_MODE_ROTATE_270 |
-					   DRM_MODE_REFLECT_X);
+	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0, DRM_MODE_ROTATE_MASK |
+					   DRM_MODE_REFLECT_MASK);
 
 	return plane;
 }
-- 
2.39.2

[PATCH v2 7/7] drm/vkms: drop "Rotation" TODO

[Maíra Canal]

Now that VKMS supports all values of rotation and reflection, drop the
"Rotation" task from the TODO list.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 Documentation/gpu/vkms.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 49db221c0f52..413e6815b9bc 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -125,7 +125,7 @@ There's lots of plane features we could add support for:
 
 - Full alpha blending on all planes.
 
-- Rotation, scaling.
+- Scaling.
 
 - Additional buffer formats, especially YUV formats for video like NV12.
   Low/high bpp RGB formats would also be interesting.
-- 
2.39.2

Re: [PATCH v2 6/7] drm/vkms: add reflect-y property

[Ville Syrjälä]

On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
> Currently, vkms only support the reflect-x property. Therefore, add the
> reflect-y property to vkms through a software implementation of the
> operation. This is possible by reverse reading the y axis during the
> blending.
> 
> Now, vkms support all possible rotation values.
> 
> Tested with igt@kms_rotation_crc@primary-reflect-y and
> igt@kms_rotation_crc@sprite-reflect-y [1].
> 
> [1] https://patchwork.freedesktop.org/series/116025/
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
>  drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
>  2 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index b05bd008aeab..19d1078e9d34 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
>  			return -1;
>  		return y + frame_info->dst.x1;
>  	default:
> -		return y;
> +		break;
>  	}
> +
> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
> +		return drm_rect_height(&frame_info->dst) - y - 1;
> +
> +	return y;
>  }
>  
>  static bool check_limit(struct vkms_frame_info *frame_info, int pos)
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 11662afa9fe4..d08bda869a24 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	frame_info->fb = fb;
>  	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>  	drm_framebuffer_get(frame_info->fb);
> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
> -						     DRM_MODE_ROTATE_0 |
> -						     DRM_MODE_ROTATE_90 |
> -						     DRM_MODE_ROTATE_180 |
> -						     DRM_MODE_ROTATE_270 |
> -						     DRM_MODE_REFLECT_X);
> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
> +						     DRM_MODE_REFLECT_MASK);

What are you trying to achieve with that?

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 6/7] drm/vkms: add reflect-y property

[Maíra Canal]

On 4/14/23 11:24, Ville Syrjälä wrote:
> On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
>> Currently, vkms only support the reflect-x property. Therefore, add the
>> reflect-y property to vkms through a software implementation of the
>> operation. This is possible by reverse reading the y axis during the
>> blending.
>>
>> Now, vkms support all possible rotation values.
>>
>> Tested with igt@kms_rotation_crc@primary-reflect-y and
>> igt@kms_rotation_crc@sprite-reflect-y [1].
>>
>> [1] https://patchwork.freedesktop.org/series/116025/
>>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>> ---
>>   drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
>>   drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
>>   2 files changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index b05bd008aeab..19d1078e9d34 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
>>   			return -1;
>>   		return y + frame_info->dst.x1;
>>   	default:
>> -		return y;
>> +		break;
>>   	}
>> +
>> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
>> +		return drm_rect_height(&frame_info->dst) - y - 1;
>> +
>> +	return y;
>>   }
>>   
>>   static bool check_limit(struct vkms_frame_info *frame_info, int pos)
>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>> index 11662afa9fe4..d08bda869a24 100644
>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>   	frame_info->fb = fb;
>>   	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>>   	drm_framebuffer_get(frame_info->fb);
>> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
>> -						     DRM_MODE_ROTATE_0 |
>> -						     DRM_MODE_ROTATE_90 |
>> -						     DRM_MODE_ROTATE_180 |
>> -						     DRM_MODE_ROTATE_270 |
>> -						     DRM_MODE_REFLECT_X);
>> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
>> +						     DRM_MODE_REFLECT_MASK);
> 
> What are you trying to achieve with that?

Yeah, seeing it right now I can see that this is not achieving anything. 
I will remove it in the next version.

Best Regards,
- Maíra Canal

> 

Re: [PATCH v2 6/7] drm/vkms: add reflect-y property

[Ville Syrjälä]

On Fri, Apr 14, 2023 at 11:37:17AM -0300, Maíra Canal wrote:
> On 4/14/23 11:24, Ville Syrjälä wrote:
> > On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
> >> Currently, vkms only support the reflect-x property. Therefore, add the
> >> reflect-y property to vkms through a software implementation of the
> >> operation. This is possible by reverse reading the y axis during the
> >> blending.
> >>
> >> Now, vkms support all possible rotation values.
> >>
> >> Tested with igt@kms_rotation_crc@primary-reflect-y and
> >> igt@kms_rotation_crc@sprite-reflect-y [1].
> >>
> >> [1] https://patchwork.freedesktop.org/series/116025/
> >>
> >> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >> ---
> >>   drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
> >>   drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
> >>   2 files changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> >> index b05bd008aeab..19d1078e9d34 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> >> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> >> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
> >>   			return -1;
> >>   		return y + frame_info->dst.x1;
> >>   	default:
> >> -		return y;
> >> +		break;
> >>   	}
> >> +
> >> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
> >> +		return drm_rect_height(&frame_info->dst) - y - 1;
> >> +
> >> +	return y;
> >>   }
> >>   
> >>   static bool check_limit(struct vkms_frame_info *frame_info, int pos)
> >> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >> index 11662afa9fe4..d08bda869a24 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> >> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> >>   	frame_info->fb = fb;
> >>   	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
> >>   	drm_framebuffer_get(frame_info->fb);
> >> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
> >> -						     DRM_MODE_ROTATE_0 |
> >> -						     DRM_MODE_ROTATE_90 |
> >> -						     DRM_MODE_ROTATE_180 |
> >> -						     DRM_MODE_ROTATE_270 |
> >> -						     DRM_MODE_REFLECT_X);
> >> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
> >> +						     DRM_MODE_REFLECT_MASK);
> > 
> > What are you trying to achieve with that?
> 
> Yeah, seeing it right now I can see that this is not achieving anything. 
> I will remove it in the next version.

I think using it might still make sense to eg. remove the 180/270
cases from your actual rendering code.

I'm also a bit uneasy about all that hand rolled coordinate calculation
stuff. Ideally drm_rect_rotate*() should handle all that for you, and
make sure the rotate vs. reflect actually get applied in the correct
order.

-- 
Ville Syrjälä
Intel

Re: [PATCH v2 6/7] drm/vkms: add reflect-y property

[Maíra Canal]

On 4/14/23 11:46, Ville Syrjälä wrote:
> On Fri, Apr 14, 2023 at 11:37:17AM -0300, Maíra Canal wrote:
>> On 4/14/23 11:24, Ville Syrjälä wrote:
>>> On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
>>>> Currently, vkms only support the reflect-x property. Therefore, add the
>>>> reflect-y property to vkms through a software implementation of the
>>>> operation. This is possible by reverse reading the y axis during the
>>>> blending.
>>>>
>>>> Now, vkms support all possible rotation values.
>>>>
>>>> Tested with igt@kms_rotation_crc@primary-reflect-y and
>>>> igt@kms_rotation_crc@sprite-reflect-y [1].
>>>>
>>>> [1] https://patchwork.freedesktop.org/series/116025/
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
>>>>    drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
>>>>    2 files changed, 10 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> index b05bd008aeab..19d1078e9d34 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>>>> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
>>>>    			return -1;
>>>>    		return y + frame_info->dst.x1;
>>>>    	default:
>>>> -		return y;
>>>> +		break;
>>>>    	}
>>>> +
>>>> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
>>>> +		return drm_rect_height(&frame_info->dst) - y - 1;
>>>> +
>>>> +	return y;
>>>>    }
>>>>    
>>>>    static bool check_limit(struct vkms_frame_info *frame_info, int pos)
>>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> index 11662afa9fe4..d08bda869a24 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>>> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>>>>    	frame_info->fb = fb;
>>>>    	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
>>>>    	drm_framebuffer_get(frame_info->fb);
>>>> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
>>>> -						     DRM_MODE_ROTATE_0 |
>>>> -						     DRM_MODE_ROTATE_90 |
>>>> -						     DRM_MODE_ROTATE_180 |
>>>> -						     DRM_MODE_ROTATE_270 |
>>>> -						     DRM_MODE_REFLECT_X);
>>>> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
>>>> +						     DRM_MODE_REFLECT_MASK);
>>>
>>> What are you trying to achieve with that?
>>
>> Yeah, seeing it right now I can see that this is not achieving anything.
>> I will remove it in the next version.
> 
> I think using it might still make sense to eg. remove the 180/270
> cases from your actual rendering code.

I will remove it on the next version.

> 
> I'm also a bit uneasy about all that hand rolled coordinate calculation
> stuff. Ideally drm_rect_rotate*() should handle all that for you, and
> make sure the rotate vs. reflect actually get applied in the correct
> order.
> 

At first, I had a similar idea that drm_rect_rotate() would handle all the
rotation. But, this turn out to be untrue. From what I can see, although
the drm_rect_rotate() helps by performing the coordinates rotation, we also
need to change the way the blending occurs. Although the coordinates have
changed, the vmap stays the same and we will still start reading the vmap
by the first line (y = 0). That's why we need to change the iteration
boundaries in the blending loops.

So, drm_rect_rotate() is a part of the solution, but it is not able to handle
it completely.

If you have any suggestions on how to perform the rotation without changing
the way the blending occurs, I would be glad to hear. Unfortunately, just
performing drm_rect_rotate() doesn't seem to be a solution.

Best Regards,
- Maíra Canal

Re: [PATCH v2 1/7] drm/vkms: isolate pixel conversion functionality

[Arthur Grillo Queiroz Cabral]

On 14/04/23 10:51, Maíra Canal wrote:
> Currently, the pixel conversion functions repeat the same loop to
> iterate the rows. Instead of repeating the same code for each pixel
> format, create a function to wrap the loop and isolate the pixel
> conversion functionality.
> 
> Suggested-by: Arthur Grillo <arthurgrillo@riseup.net>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c |   4 +-
>  drivers/gpu/drm/vkms/vkms_drv.h      |   4 +-
>  drivers/gpu/drm/vkms/vkms_formats.c  | 136 ++++++++++++---------------
>  drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
>  drivers/gpu/drm/vkms/vkms_plane.c    |   2 +-
>  5 files changed, 65 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 8e53fa80742b..80164e79af00 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -99,7 +99,7 @@ static void blend(struct vkms_writeback_job *wb,
>  			if (!check_y_limit(plane[i]->frame_info, y))
>  				continue;
>  
> -			plane[i]->plane_read(stage_buffer, plane[i]->frame_info, y);
> +			vkms_compose_row(stage_buffer, plane[i], y);
>  			pre_mul_alpha_blend(plane[i]->frame_info, stage_buffer,
>  					    output_buffer);
>  		}
> @@ -118,7 +118,7 @@ static int check_format_funcs(struct vkms_crtc_state *crtc_state,
>  	u32 n_active_planes = crtc_state->num_active_planes;
>  
>  	for (size_t i = 0; i < n_active_planes; i++)
> -		if (!planes[i]->plane_read)
> +		if (!planes[i]->pixel_read)
>  			return -1;
>  
>  	if (active_wb && !active_wb->wb_write)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 4a248567efb2..d7ad813d7ae7 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -56,8 +56,7 @@ struct vkms_writeback_job { struct vkms_plane_state { struct drm_shadow_plane_state base;
>  	struct vkms_frame_info *frame_info;
> -	void (*plane_read)(struct line_buffer *buffer,
> -			   const struct vkms_frame_info *frame_info, int y);
> +	void (*pixel_read)(u16 *src_buffer, struct pixel_argb_u16 *out_pixels, int x);
>  };
>  
>  struct vkms_plane {
> @@ -155,6 +154,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  /* Composer Support */
>  void vkms_composer_worker(struct work_struct *work);
>  void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y);
>  
>  /* Writeback */
>  int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index d4950688b3f1..02b0fc5a0839 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -42,100 +42,82 @@ static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y
>  	return packed_pixels_addr(frame_info, x_src, y_src);
>  }
>  
> -static void ARGB8888_to_argb_u16(struct line_buffer *stage_buffer,
> -				 const struct vkms_frame_info *frame_info, int y)
> +static void ARGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)

Do you need to pass the x coordinate? You don't change it on those
functions. By not passing it, the conversion function would be even more
isolated.

>  {
> -	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> -	u8 *src_pixels = get_packed_src_addr(frame_info, y);
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> -			    stage_buffer->n_pixels);
> -
> -	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> -		/*
> -		 * The 257 is the "conversion ratio". This number is obtained by the
> -		 * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
> -		 * the best color value in a pixel format with more possibilities.
> -		 * A similar idea applies to others RGB color conversions.
> -		 */
> -		out_pixels[x].a = (u16)src_pixels[3] * 257;
> -		out_pixels[x].r = (u16)src_pixels[2] * 257;
> -		out_pixels[x].g = (u16)src_pixels[1] * 257;
> -		out_pixels[x].b = (u16)src_pixels[0] * 257;
> -	}
> +	u8 *pixels = (u8 *)src_pixels;
> +
> +	/*
> +	 * The 257 is the "conversion ratio". This number is obtained by the
> +	 * (2^16 - 1) / (2^8 - 1) division. Which, in this case, tries to get
> +	 * the best color value in a pixel format with more possibilities.
> +	 * A similar idea applies to others RGB color conversions.
> +	 */
> +	out_pixels[x].a = (u16)pixels[3] * 257;
> +	out_pixels[x].r = (u16)pixels[2] * 257;
> +	out_pixels[x].g = (u16)pixels[1] * 257;
> +	out_pixels[x].b = (u16)pixels[0] * 257;
>  }
>  
> -static void XRGB8888_to_argb_u16(struct line_buffer *stage_buffer,
> -				 const struct vkms_frame_info *frame_info, int y)
> +static void XRGB8888_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
>  {
> -	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> -	u8 *src_pixels = get_packed_src_addr(frame_info, y);
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> -			    stage_buffer->n_pixels);
> +	u8 *pixels = (u8 *)src_pixels;
>  
> -	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> -		out_pixels[x].a = (u16)0xffff;
> -		out_pixels[x].r = (u16)src_pixels[2] * 257;
> -		out_pixels[x].g = (u16)src_pixels[1] * 257;
> -		out_pixels[x].b = (u16)src_pixels[0] * 257;
> -	}
> +	out_pixels[x].a = (u16)0xffff;
> +	out_pixels[x].r = (u16)pixels[2] * 257;
> +	out_pixels[x].g = (u16)pixels[1] * 257;
> +	out_pixels[x].b = (u16)pixels[0] * 257;
>  }
>  
> -static void ARGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
> -				     const struct vkms_frame_info *frame_info,
> -				     int y)
> +static void ARGB16161616_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
>  {
> -	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> -	u16 *src_pixels = get_packed_src_addr(frame_info, y);
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> -			    stage_buffer->n_pixels);
> -
> -	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> -		out_pixels[x].a = le16_to_cpu(src_pixels[3]);
> -		out_pixels[x].r = le16_to_cpu(src_pixels[2]);
> -		out_pixels[x].g = le16_to_cpu(src_pixels[1]);
> -		out_pixels[x].b = le16_to_cpu(src_pixels[0]);
> -	}
> +	out_pixels[x].a = le16_to_cpu(src_pixels[3]);
> +	out_pixels[x].r = le16_to_cpu(src_pixels[2]);
> +	out_pixels[x].g = le16_to_cpu(src_pixels[1]);
> +	out_pixels[x].b = le16_to_cpu(src_pixels[0]);
>  }
>  
> -static void XRGB16161616_to_argb_u16(struct line_buffer *stage_buffer,
> -				     const struct vkms_frame_info *frame_info,
> -				     int y)
> +static void XRGB16161616_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
>  {
> -	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> -	u16 *src_pixels = get_packed_src_addr(frame_info, y);
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> -			    stage_buffer->n_pixels);
> -
> -	for (size_t x = 0; x < x_limit; x++, src_pixels += 4) {
> -		out_pixels[x].a = (u16)0xffff;
> -		out_pixels[x].r = le16_to_cpu(src_pixels[2]);
> -		out_pixels[x].g = le16_to_cpu(src_pixels[1]);
> -		out_pixels[x].b = le16_to_cpu(src_pixels[0]);
> -	}
> +	out_pixels[x].a = (u16)0xffff;
> +	out_pixels[x].r = le16_to_cpu(src_pixels[2]);
> +	out_pixels[x].g = le16_to_cpu(src_pixels[1]);
> +	out_pixels[x].b = le16_to_cpu(src_pixels[0]);
>  }
>  
> -static void RGB565_to_argb_u16(struct line_buffer *stage_buffer,
> -			       const struct vkms_frame_info *frame_info, int y)
> +static void RGB565_to_argb_u16(u16 *src_pixels, struct pixel_argb_u16 *out_pixels, int x)
>  {
> -	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> -	u16 *src_pixels = get_packed_src_addr(frame_info, y);
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst),
> -			       stage_buffer->n_pixels);
> -
>  	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));
>  
> -	for (size_t x = 0; x < x_limit; x++, src_pixels++) {
> -		u16 rgb_565 = le16_to_cpu(*src_pixels);
> -		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);
> +	u16 rgb_565 = le16_to_cpu(*src_pixels);
> +	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);
>  
> -		out_pixels[x].a = (u16)0xffff;
> -		out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> -		out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> -		out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> -	}
> +	out_pixels[x].a = (u16)0xffff;
> +	out_pixels[x].r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> +	out_pixels[x].g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> +	out_pixels[x].b = drm_fixp2int(drm_fixp_mul(fp_b, fp_rb_ratio));
> +}
> +
> +void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state *plane, int y)
> +{
> +	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
> +	u16 *src_pixels = get_packed_src_addr(plane->frame_info, y);
> +	int limit = min_t(size_t, drm_rect_width(&plane->frame_info->dst),
> +			  stage_buffer->n_pixels);
> +	int format = plane->frame_info->fb->format->format;
> +	int pixel_size;
> +
> +	if (format == DRM_FORMAT_RGB565)
> +		pixel_size = 1;
> +	else if (format == DRM_FORMAT_ARGB8888 || format == DRM_FORMAT_XRGB8888)
> +		pixel_size = 2;
> +	else
> +		pixel_size = 4;

Instead of hardcoding the pixel_size, I think it would be better to use
drm_format_info_bpp() and DIV_ROUND_UP to get it.

> +
> +	for (size_t x = 0; x < limit; x++, src_pixels += pixel_size)
> +		plane->pixel_read(src_pixels, out_pixels, x);
>  }
>  
>  /*
> @@ -249,7 +231,7 @@ static void argb_u16_to_RGB565(struct vkms_frame_info *frame_info,
>  	}
>  }
>  

Maybe it would be good to make the same modifications on the conversions
from ARGB16161616. The writeback side would benefit from it.

Best Regards.
~Arthur Grillo

> -void *get_frame_to_line_function(u32 format)
> +void *get_pixel_conversion_function(u32 format)
>  {
>  	switch (format) {
>  	case DRM_FORMAT_ARGB8888:
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index 43b7c1979018..c5b113495d0c 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -5,7 +5,7 @@
>  
>  #include "vkms_drv.h"
>  
> -void *get_frame_to_line_function(u32 format);
> +void *get_pixel_conversion_function(u32 format);
>  
>  void *get_line_to_frame_function(u32 format);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index c41cec7dcb70..0a23875900ec 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -123,7 +123,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	frame_info->offset = fb->offsets[0];
>  	frame_info->pitch = fb->pitches[0];
>  	frame_info->cpp = fb->format->cpp[0];
> -	vkms_plane_state->plane_read = get_frame_to_line_function(fmt);
> +	vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
>  }
>  
>  static int vkms_plane_atomic_check(struct drm_plane *plane,

Re: [PATCH v2 6/7] drm/vkms: add reflect-y property

[Ville Syrjälä]

On Fri, Apr 14, 2023 at 02:53:55PM -0300, Maíra Canal wrote:
> On 4/14/23 11:46, Ville Syrjälä wrote:
> > On Fri, Apr 14, 2023 at 11:37:17AM -0300, Maíra Canal wrote:
> >> On 4/14/23 11:24, Ville Syrjälä wrote:
> >>> On Fri, Apr 14, 2023 at 10:51:50AM -0300, Maíra Canal wrote:
> >>>> Currently, vkms only support the reflect-x property. Therefore, add the
> >>>> reflect-y property to vkms through a software implementation of the
> >>>> operation. This is possible by reverse reading the y axis during the
> >>>> blending.
> >>>>
> >>>> Now, vkms support all possible rotation values.
> >>>>
> >>>> Tested with igt@kms_rotation_crc@primary-reflect-y and
> >>>> igt@kms_rotation_crc@sprite-reflect-y [1].
> >>>>
> >>>> [1] https://patchwork.freedesktop.org/series/116025/
> >>>>
> >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >>>> ---
> >>>>    drivers/gpu/drm/vkms/vkms_composer.c |  7 ++++++-
> >>>>    drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++------------
> >>>>    2 files changed, 10 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> index b05bd008aeab..19d1078e9d34 100644
> >>>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> >>>> @@ -92,8 +92,13 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
> >>>>    			return -1;
> >>>>    		return y + frame_info->dst.x1;
> >>>>    	default:
> >>>> -		return y;
> >>>> +		break;
> >>>>    	}
> >>>> +
> >>>> +	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
> >>>> +		return drm_rect_height(&frame_info->dst) - y - 1;
> >>>> +
> >>>> +	return y;
> >>>>    }
> >>>>    
> >>>>    static bool check_limit(struct vkms_frame_info *frame_info, int pos)
> >>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> >>>> index 11662afa9fe4..d08bda869a24 100644
> >>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> >>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> >>>> @@ -121,12 +121,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> >>>>    	frame_info->fb = fb;
> >>>>    	memcpy(&frame_info->map, &shadow_plane_state->data, sizeof(frame_info->map));
> >>>>    	drm_framebuffer_get(frame_info->fb);
> >>>> -	frame_info->rotation = drm_rotation_simplify(new_state->rotation,
> >>>> -						     DRM_MODE_ROTATE_0 |
> >>>> -						     DRM_MODE_ROTATE_90 |
> >>>> -						     DRM_MODE_ROTATE_180 |
> >>>> -						     DRM_MODE_ROTATE_270 |
> >>>> -						     DRM_MODE_REFLECT_X);
> >>>> +	frame_info->rotation = drm_rotation_simplify(new_state->rotation, DRM_MODE_ROTATE_MASK |
> >>>> +						     DRM_MODE_REFLECT_MASK);
> >>>
> >>> What are you trying to achieve with that?
> >>
> >> Yeah, seeing it right now I can see that this is not achieving anything.
> >> I will remove it in the next version.
> > 
> > I think using it might still make sense to eg. remove the 180/270
> > cases from your actual rendering code.
> 
> I will remove it on the next version.
> 
> > 
> > I'm also a bit uneasy about all that hand rolled coordinate calculation
> > stuff. Ideally drm_rect_rotate*() should handle all that for you, and
> > make sure the rotate vs. reflect actually get applied in the correct
> > order.
> > 
> 
> At first, I had a similar idea that drm_rect_rotate() would handle all the
> rotation. But, this turn out to be untrue. From what I can see, although
> the drm_rect_rotate() helps by performing the coordinates rotation, we also
> need to change the way the blending occurs. Although the coordinates have
> changed, the vmap stays the same and we will still start reading the vmap
> by the first line (y = 0). That's why we need to change the iteration
> boundaries in the blending loops.
> 
> So, drm_rect_rotate() is a part of the solution, but it is not able to handle
> it completely.
> 
> If you have any suggestions on how to perform the rotation without changing
> the way the blending occurs, I would be glad to hear. Unfortunately, just
> performing drm_rect_rotate() doesn't seem to be a solution.

Dunno how vkms does this currently, but the most straightforward
solution I can think of would be something like this:

struct drm_rect src;

drm_rect_fp_to_int(&src, plane_state->src);

drm_rect_rotate(&src, fb->width, fb->height, rotation);

for (dy = dst->y1; dy < dst->y2; dy++) {
	for (dx = dst->x1; dx < dst->x2; dx++) {
		int _sx = dx - dst->x1 + src.x1;
		int _sy = dy - dst->y1 + src.y1;
		struct drm_rect s;

		drm_rect_init(&s, _sx, _sy, 1, 1);

		drm_rect_rotate_inv(&s, fb->width, fb->height, rotation);

		output_pixel(dx, dy, s.x1, s.y1);
	}
}

-- 
Ville Syrjälä
Intel