[PATCH v4 0/6] drm/vkms: introduce plane rotation property

[PATCH v4 0/6] 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.

I tried to apply Ville's suggestion to avoid hand rolled coordinate
calculation stuff. Although I couldn't apply all the code suggested by
Ville, I was able to remove all the hardcoded code related to the x-offset.
As VKMS' composition is performed by line, I still need to indicate the
right pixel, which means that I still have some hardcoded code. Thanks for
the suggestion, Ville! It really reduced the code complexity. 

v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/

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

v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/

* Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo).
* Don't use the x coordinate in the pixel_read functions (Arthur Grillo).
* Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä).
* Use u8* to input the src_pixels instead of using u16*.

v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/

* Create a original rectangle and a rotated rectangle and use the original
  rectangle to offset the x-axis (Ville Syrjälä).

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

Best Regards,
- Maíra Canal

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

 Documentation/gpu/vkms.rst           |   2 +-
 drivers/gpu/drm/vkms/vkms_composer.c |  38 ++++++--
 drivers/gpu/drm/vkms/vkms_drv.h      |   6 +-
 drivers/gpu/drm/vkms/vkms_formats.c  | 139 +++++++++++++--------------
 drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
 drivers/gpu/drm/vkms/vkms_plane.c    |  16 ++-
 6 files changed, 117 insertions(+), 86 deletions(-)

-- 
2.39.2

[PATCH v4 1/6] 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>
Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_composer.c |   4 +-
 drivers/gpu/drm/vkms/vkms_drv.h      |   4 +-
 drivers/gpu/drm/vkms/vkms_formats.c  | 125 +++++++++++----------------
 drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
 drivers/gpu/drm/vkms/vkms_plane.c    |   2 +-
 5 files changed, 56 insertions(+), 81 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..f152d54baf76 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)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel);
 };
 
 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..8d948c73741e 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -42,100 +42,75 @@ 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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	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;
-	}
+	/*
+	 * 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_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;
 }
 
-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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	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) {
-		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_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;
 }
 
-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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	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);
+	u16 *pixels = (u16 *)src_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_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]);
 }
 
-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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	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);
+	u16 *pixels = (u16 *)src_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_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]);
 }
 
-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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	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);
+	u16 *pixels = (u16 *)src_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(*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_pixel->a = (u16)0xffff;
+	out_pixel->r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
+	out_pixel->g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
+	out_pixel->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;
+	struct vkms_frame_info *frame_info = plane->frame_info;
+	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)
+		plane->pixel_read(src_pixels, &out_pixels[x]);
 }
 
 /*
@@ -249,7 +224,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 v4 2/6] drm/vkms: add rotate-0 and 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_composer.c |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h      |  2 ++
 drivers/gpu/drm/vkms/vkms_formats.c  | 16 +++++++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c    | 12 ++++++++++++
 4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 80164e79af00..a4436981cbcd 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -55,7 +55,7 @@ static void pre_mul_alpha_blend(struct vkms_frame_info *frame_info,
 
 static bool check_y_limit(struct vkms_frame_info *frame_info, int y)
 {
-	if (y >= frame_info->dst.y1 && y < frame_info->dst.y2)
+	if (y >= frame_info->rotated.y1 && y < frame_info->rotated.y2)
 		return true;
 
 	return false;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f152d54baf76..5f1a0a44a78c 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -26,7 +26,9 @@
 struct vkms_frame_info {
 	struct drm_framebuffer *fb;
 	struct drm_rect src, dst;
+	struct drm_rect rotated;
 	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 8d948c73741e..f59b1c48a563 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -37,11 +37,18 @@ static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
 static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y)
 {
 	int x_src = frame_info->src.x1 >> 16;
-	int y_src = y - frame_info->dst.y1 + (frame_info->src.y1 >> 16);
+	int y_src = y - frame_info->rotated.y1 + (frame_info->src.y1 >> 16);
 
 	return packed_pixels_addr(frame_info, x_src, y_src);
 }
 
+static int get_x_position(const struct vkms_frame_info *frame_info, int limit, int x)
+{
+	if (frame_info->rotation & DRM_MODE_REFLECT_X)
+		return limit - x - 1;
+	return x;
+}
+
 static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
 {
 	/*
@@ -109,8 +116,11 @@ 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)
-		plane->pixel_read(src_pixels, &out_pixels[x]);
+	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
+		int x_pos = get_x_position(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..3639afd7da47 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>
@@ -117,9 +118,16 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info = vkms_plane_state->frame_info;
 	memcpy(&frame_info->src, &new_state->src, sizeof(struct drm_rect));
 	memcpy(&frame_info->dst, &new_state->dst, sizeof(struct drm_rect));
+	memcpy(&frame_info->rotated, &new_state->dst, sizeof(struct drm_rect));
 	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_REFLECT_X);
+
+	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];
@@ -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_REFLECT_X);
+
 	return plane;
 }
-- 
2.39.2

[PATCH v4 3/6] drm/vkms: add reflect-y and rotate-180 property

[Maíra Canal]

Currently, vkms only supports 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.

Note that, by implementing the reflect-x and reflect-y properties, it is
also possible to add the rotate-180 property, as it is a combination
of those two properties.

Tested with igt@kms_rotation_crc@primary-reflect-y [1],
igt@kms_rotation_crc@sprite-reflect-y [1],
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 | 16 +++++++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c    |  7 +++++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index a4436981cbcd..6c5ef11b3943 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -53,6 +53,13 @@ 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)
+{
+	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
+		return drm_rect_height(&frame_info->rotated) - y - 1;
+	return y;
+}
+
 static bool check_y_limit(struct vkms_frame_info *frame_info, int y)
 {
 	if (y >= frame_info->rotated.y1 && y < frame_info->rotated.y2)
@@ -86,6 +93,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 +104,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 +117,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_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 3639afd7da47..904a278e07d7 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -123,7 +123,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	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_REFLECT_X);
+						     DRM_MODE_REFLECT_X |
+						     DRM_MODE_REFLECT_Y);
 
 	drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated),
 			drm_rect_height(&frame_info->rotated), frame_info->rotation);
@@ -239,7 +240,9 @@ 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_REFLECT_X);
+					   DRM_MODE_ROTATE_180 |
+					   DRM_MODE_REFLECT_X |
+					   DRM_MODE_REFLECT_Y);
 
 	return plane;
 }
-- 
2.39.2

[PATCH v4 4/6] drm/vkms: add rotate-90 property

[Maíra Canal]

Currently, vkms only supports the rotate-180, reflect-x and reflect-y
properties. 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 | 21 ++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_formats.c  |  4 ++++
 drivers/gpu/drm/vkms/vkms_plane.c    |  2 ++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 6c5ef11b3943..491850ffeac9 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -57,13 +57,24 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
 {
 	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
 		return drm_rect_height(&frame_info->rotated) - y - 1;
-	return y;
+
+	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
+	case DRM_MODE_ROTATE_90:
+		return frame_info->rotated.x2 - y - 1;
+	default:
+		return 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->rotated.y1 && y < frame_info->rotated.y2)
-		return true;
+	if (frame_info->rotation & DRM_MODE_ROTATE_90) {
+		if (pos >= 0 && pos < drm_rect_width(&frame_info->rotated))
+			return true;
+	} else {
+		if (pos >= frame_info->rotated.y1 && pos < frame_info->rotated.y2)
+			return true;
+	}
 
 	return false;
 }
@@ -106,7 +117,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 f59b1c48a563..f56f359e0d2d 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -119,6 +119,10 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
 		int x_pos = get_x_position(frame_info, limit, x);
 
+		if (frame_info->rotation & DRM_MODE_ROTATE_90)
+			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
+				+ frame_info->cpp * y;
+
 		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 904a278e07d7..9e451ce97099 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,
 	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_REFLECT_X |
 						     DRM_MODE_REFLECT_Y);
 
@@ -240,6 +241,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 |
 					   DRM_MODE_REFLECT_X |
 					   DRM_MODE_REFLECT_Y);
-- 
2.39.2

[PATCH v4 5/6] drm/vkms: add rotate-270 property

[Maíra Canal]

Currently, vkms supports the rotate-90, rotate-180, reflect-x and
reflect-y 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.

Now, vkms supports all possible rotation values.

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 | 5 ++++-
 drivers/gpu/drm/vkms/vkms_formats.c  | 6 ++++--
 drivers/gpu/drm/vkms/vkms_plane.c    | 7 ++-----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 491850ffeac9..906d3df40cdb 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>
@@ -61,6 +62,8 @@ 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:
 		return frame_info->rotated.x2 - y - 1;
+	case DRM_MODE_ROTATE_270:
+		return y + frame_info->rotated.x1;
 	default:
 		return y;
 	}
@@ -68,7 +71,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->rotated))
 			return true;
 	} else {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index f56f359e0d2d..ebacb8efa055 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -2,6 +2,8 @@
 
 #include <linux/kernel.h>
 #include <linux/minmax.h>
+
+#include <drm/drm_blend.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_fixed.h>
 
@@ -44,7 +46,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 limit, int x)
 {
-	if (frame_info->rotation & DRM_MODE_REFLECT_X)
+	if (frame_info->rotation & (DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_270))
 		return limit - x - 1;
 	return x;
 }
@@ -119,7 +121,7 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
 		int x_pos = get_x_position(frame_info, limit, x);
 
-		if (frame_info->rotation & DRM_MODE_ROTATE_90)
+		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;
 
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 9e451ce97099..66263081639d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -124,6 +124,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_270 |
 						     DRM_MODE_REFLECT_X |
 						     DRM_MODE_REFLECT_Y);
 
@@ -240,11 +241,7 @@ 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_REFLECT_X |
-					   DRM_MODE_REFLECT_Y);
+					   DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
 
 	return plane;
 }
-- 
2.39.2

[PATCH v4 6/6] 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 v4 1/6] drm/vkms: isolate pixel conversion functionality

[Maíra Canal]

On 4/18/23 10:05, 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>
> Reviewed-by: Arthur Grillo <arthurgrillo@riseup.net>

Applied to drm/drm-misc (drm-misc-next).

Best Regards,
- Maíra Canal

> ---
>   drivers/gpu/drm/vkms/vkms_composer.c |   4 +-
>   drivers/gpu/drm/vkms/vkms_drv.h      |   4 +-
>   drivers/gpu/drm/vkms/vkms_formats.c  | 125 +++++++++++----------------
>   drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
>   drivers/gpu/drm/vkms/vkms_plane.c    |   2 +-
>   5 files changed, 56 insertions(+), 81 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..f152d54baf76 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)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel);
>   };
>   
>   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..8d948c73741e 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -42,100 +42,75 @@ 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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>   {
> -	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;
> -	}
> +	/*
> +	 * 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_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;
>   }
>   
> -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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>   {
> -	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) {
> -		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_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;
>   }
>   
> -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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>   {
> -	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);
> +	u16 *pixels = (u16 *)src_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_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]);
>   }
>   
> -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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>   {
> -	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);
> +	u16 *pixels = (u16 *)src_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_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]);
>   }
>   
> -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(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
>   {
> -	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);
> +	u16 *pixels = (u16 *)src_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(*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_pixel->a = (u16)0xffff;
> +	out_pixel->r = drm_fixp2int(drm_fixp_mul(fp_r, fp_rb_ratio));
> +	out_pixel->g = drm_fixp2int(drm_fixp_mul(fp_g, fp_g_ratio));
> +	out_pixel->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;
> +	struct vkms_frame_info *frame_info = plane->frame_info;
> +	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)
> +		plane->pixel_read(src_pixels, &out_pixels[x]);
>   }
>   
>   /*
> @@ -249,7 +224,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,

Re: [PATCH v4 4/6] drm/vkms: add rotate-90 property

[Melissa Wen]

On 04/18, Maíra Canal wrote:
> Currently, vkms only supports the rotate-180, reflect-x and reflect-y
> properties. 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.

In the same way you did for reflect-x, could you add the link for your
IGT patchset here to avoid misunderstanding between VKMS and IGT tests
status? Double-check if it's necessary for other commits in this series.

Thanks,

Melissa

> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 21 ++++++++++++++++-----
>  drivers/gpu/drm/vkms/vkms_formats.c  |  4 ++++
>  drivers/gpu/drm/vkms/vkms_plane.c    |  2 ++
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 6c5ef11b3943..491850ffeac9 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -57,13 +57,24 @@ static int get_y_pos(struct vkms_frame_info *frame_info, int y)
>  {
>  	if (frame_info->rotation & DRM_MODE_REFLECT_Y)
>  		return drm_rect_height(&frame_info->rotated) - y - 1;
> -	return y;
> +
> +	switch (frame_info->rotation & DRM_MODE_ROTATE_MASK) {
> +	case DRM_MODE_ROTATE_90:
> +		return frame_info->rotated.x2 - y - 1;
> +	default:
> +		return 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->rotated.y1 && y < frame_info->rotated.y2)
> -		return true;
> +	if (frame_info->rotation & DRM_MODE_ROTATE_90) {
> +		if (pos >= 0 && pos < drm_rect_width(&frame_info->rotated))
> +			return true;
> +	} else {
> +		if (pos >= frame_info->rotated.y1 && pos < frame_info->rotated.y2)
> +			return true;
> +	}
>  
>  	return false;
>  }
> @@ -106,7 +117,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 f59b1c48a563..f56f359e0d2d 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -119,6 +119,10 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
>  		int x_pos = get_x_position(frame_info, limit, x);
>  
> +		if (frame_info->rotation & DRM_MODE_ROTATE_90)
> +			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
> +				+ frame_info->cpp * y;
> +
>  		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 904a278e07d7..9e451ce97099 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,
>  	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_REFLECT_X |
>  						     DRM_MODE_REFLECT_Y);
>  
> @@ -240,6 +241,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 |
>  					   DRM_MODE_REFLECT_X |
>  					   DRM_MODE_REFLECT_Y);
> -- 
> 2.39.2
> 

Re: [PATCH v4 0/6] drm/vkms: introduce plane rotation property

[Melissa Wen]

On 04/18, Maíra Canal wrote:
> 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.
> 
> I tried to apply Ville's suggestion to avoid hand rolled coordinate
> calculation stuff. Although I couldn't apply all the code suggested by
> Ville, I was able to remove all the hardcoded code related to the x-offset.
> As VKMS' composition is performed by line, I still need to indicate the
> right pixel, which means that I still have some hardcoded code. Thanks for
> the suggestion, Ville! It really reduced the code complexity. 
> 
> v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/
> 
> * Add patch to isolate pixel format conversion (Arthur Grillo).
> 
> v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/
> 
> * Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo).
> * Don't use the x coordinate in the pixel_read functions (Arthur Grillo).
> * Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä).
> * Use u8* to input the src_pixels instead of using u16*.
> 
> v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/
> 
> * Create a original rectangle and a rotated rectangle and use the original
>   rectangle to offset the x-axis (Ville Syrjälä).
> 
> [1] https://patchwork.freedesktop.org/series/116025/

Hi Maíra,

Thanks for adding rotation properties to VKMS!
Overall, LGTM and this series is:

Reviewed-by: Melissa Wen <mwen@igalia.com>

As you already applied the first patch, not a big issue, but I see new
function documentation is missing. Could you send a follow-up patch
documenting "vkms_compose_row()"? Also, would be good to apply the same
improvement for the remaining conversion functions too.

Thanks,

Melissa

> 
> Best Regards,
> - Maíra Canal
> 
> Maíra Canal (6):
>   drm/vkms: isolate pixel conversion functionality
>   drm/vkms: add rotate-0 and reflect-x property
>   drm/vkms: add reflect-y and rotate-180 property
>   drm/vkms: add rotate-90 property
>   drm/vkms: add rotate-270 property
>   drm/vkms: drop "Rotation" TODO
> 
>  Documentation/gpu/vkms.rst           |   2 +-
>  drivers/gpu/drm/vkms/vkms_composer.c |  38 ++++++--
>  drivers/gpu/drm/vkms/vkms_drv.h      |   6 +-
>  drivers/gpu/drm/vkms/vkms_formats.c  | 139 +++++++++++++--------------
>  drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
>  drivers/gpu/drm/vkms/vkms_plane.c    |  16 ++-
>  6 files changed, 117 insertions(+), 86 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v4 0/6] drm/vkms: introduce plane rotation property

[Maíra Canal]

On 5/7/23 17:54, Melissa Wen wrote:
> On 04/18, Maíra Canal wrote:
>> 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.
>>
>> I tried to apply Ville's suggestion to avoid hand rolled coordinate
>> calculation stuff. Although I couldn't apply all the code suggested by
>> Ville, I was able to remove all the hardcoded code related to the x-offset.
>> As VKMS' composition is performed by line, I still need to indicate the
>> right pixel, which means that I still have some hardcoded code. Thanks for
>> the suggestion, Ville! It really reduced the code complexity.
>> v1 -> v2: https://lore.kernel.org/dri-devel/20230406130138.70752-1-mcanal@igalia.com/T/
>>
>> * Add patch to isolate pixel format conversion (Arthur Grillo).
>>
>> v2 -> v3: https://lore.kernel.org/dri-devel/20230414135151.75975-1-mcanal@igalia.com/T/
>>
>> * Use cpp to calculate pixel size instead of hard-coding (Arthur Grillo).
>> * Don't use the x coordinate in the pixel_read functions (Arthur Grillo).
>> * Use drm_rotate_simplify() to avoid hard-coding rotate-180 (Ville Syrjälä).
>> * Use u8* to input the src_pixels instead of using u16*.
>>
>> v3 -> v4: https://lore.kernel.org/dri-devel/20230417121056.63917-1-mcanal@igalia.com/T/
>>
>> * Create a original rectangle and a rotated rectangle and use the original
>>   rectangle to offset the x-axis (Ville Syrjälä).
>>
>> [1] https://patchwork.freedesktop.org/series/116025/
> 
> Hi Maíra,
> 
> Thanks for adding rotation properties to VKMS!
> Overall, LGTM and this series is:
> 
> Reviewed-by: Melissa Wen <mwen@igalia.com>

Thanks for the review, Melissa! Applied to drm/drm-misc (drm-misc-next).

> 
> As you already applied the first patch, not a big issue, but I see new
> function documentation is missing. Could you send a follow-up patch
> documenting "vkms_compose_row()"? Also, would be good to apply the same
> improvement for the remaining conversion functions too.

I'll try to work on some follow-up patches ASAP.

Best Regards,
- Maíra Canal

> 
> Thanks,
> 
> Melissa
> 
>>
>> Best Regards,
>> - Maíra Canal
>>
>> Maíra Canal (6):
>>   drm/vkms: isolate pixel conversion functionality
>>   drm/vkms: add rotate-0 and reflect-x property
>>   drm/vkms: add reflect-y and rotate-180 property
>>   drm/vkms: add rotate-90 property
>>   drm/vkms: add rotate-270 property
>>   drm/vkms: drop "Rotation" TODO
>>
>>  Documentation/gpu/vkms.rst           |   2 +-
>>  drivers/gpu/drm/vkms/vkms_composer.c |  38 ++++++--
>>  drivers/gpu/drm/vkms/vkms_drv.h      |   6 +-
>>  drivers/gpu/drm/vkms/vkms_formats.c  | 139 +++++++++++++--------------
>>  drivers/gpu/drm/vkms/vkms_formats.h  |   2 +-
>>  drivers/gpu/drm/vkms/vkms_plane.c    |  16 ++-
>>  6 files changed, 117 insertions(+), 86 deletions(-)
>>
>> -- 
>> 2.39.2
>>