2024-01-10 17:46:16

by Arthur Grillo

[permalink] [raw]
Subject: [PATCH v2 4/7] drm/vkms: Add chroma subsampling

Add support to chroma subsampling. This should be noop, as all supported
formats do not have chroma subsampling.

Signed-off-by: Arthur Grillo <[email protected]>
---
drivers/gpu/drm/vkms/vkms_formats.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 0156372aa1ef..098ed16f2104 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -26,12 +26,15 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
* @index: The index of the plane on the 2D buffer
*
* Takes the information stored in the frame_info, a pair of coordinates, and
- * returns the address of the first color channel on the desired index.
+ * returns the address of the first color channel on the desired index. The
+ * format's specific subsample is applied.
*/
static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
int x, int y, size_t index)
{
- size_t offset = pixel_offset(frame_info, x, y, index);
+ int vsub = index == 0 ? 1 : frame_info->fb->format->vsub;
+ int hsub = index == 0 ? 1 : frame_info->fb->format->hsub;
+ size_t offset = pixel_offset(frame_info, x / hsub, y / vsub, index);

return (u8 *)frame_info->map[0].vaddr + offset;
}
@@ -146,18 +149,23 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
for (size_t x = 0; x < limit; x++) {
int x_pos = get_x_position(frame_info, limit, x);

+ bool shoud_inc = !((x + 1) % frame_format->num_planes);
+
if (drm_rotation_90_or_270(frame_info->rotation)) {
for (size_t i = 0; i < frame_format->num_planes; i++) {
src_pixels[i] = get_packed_src_addr(frame_info,
x + frame_info->rotated.y1, i);
- src_pixels[i] += frame_format->cpp[i] * y;
+ if (!i || shoud_inc)
+ src_pixels[i] += frame_format->cpp[i] * y;
}
}

plane->pixel_read(src_pixels, &out_pixels[x_pos], encoding, range);

- for (size_t i = 0; i < frame_format->num_planes; i++)
- src_pixels[i] += frame_format->cpp[i];
+ for (size_t i = 0; i < frame_format->num_planes; i++) {
+ if (!i || shoud_inc)
+ src_pixels[i] += frame_format->cpp[i];
+ }
}
}


--
2.43.0



2024-02-01 18:06:08

by Louis Chauvet

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] drm/vkms: Add chroma subsampling

[...]

> @@ -146,18 +149,23 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
> for (size_t x = 0; x < limit; x++) {
> int x_pos = get_x_position(frame_info, limit, x);
>
> + bool shoud_inc = !((x + 1) % frame_format->num_planes);

I think this line will break if the subsampling is not the same as the
plane count. For NV12 it works only because there are two planes and
hsub=2/vsub=2, but I believe NV24 will not work because of plane 2, as
we need to increment x at the same speed on all planes.

I have a proposal to solve this issue (see my patchset applying on top of
yours). You probably at least need to use .hsub/vsub to
increment/decrement properly src_pixels pointer.

Currently the tests pass for it because it only use "horizontal
lines" and "full color" pictures.

In the series [1] I proposed to change the pattern to detect this kind of
issue.

[...]

[1]: https://lore.kernel.org/dri-devel/[email protected]/T/#t

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