From: Melissa Wen
Sent: 21 March 2020 20:37 The previous memset operation was not correctly setting zero on the alpha channel to compute the crc, and as a result, the pipe-A-cursor-alpha-transparent subtest of the IGT test kms_cursor_crc was crashing. To avoid errors of misinterpretation related to endianness, this solution uses a bitfield operation to extract the RGB values from each pixel and ignores the alpha channel as expected.
Signed-off-by: Melissa Wen melissa.srw@gmail.com
drivers/gpu/drm/vkms/vkms_composer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 4af2f19480f4..8c1c005bb717 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+
#include <linux/crc32.h> +#include <linux/bitfield.h>
#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> @@ -9,6 +10,8 @@
#include "vkms_drv.h"
+#define XRGB_MSK GENMASK(23, 0)
/**
- compute_crc - Compute CRC value on output frame
@@ -26,6 +29,7 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) int h_src = drm_rect_height(&composer->src) >> 16; int w_src = drm_rect_width(&composer->src) >> 16; u32 crc = 0;
u32 *pixel;
for (i = y_src; i < y_src + h_src; ++i) { for (j = x_src; j < x_src + w_src; ++j) {
@@ -33,7 +37,8 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer) + (i * composer->pitch) + (j * composer->cpp); /* XRGB format ignores Alpha channel */
memset(vaddr_out + src_offset + 24, 0, 8);
pixel = vaddr_out + src_offset;*pixel = FIELD_GET(XRGB_MSK, *(u32 *)pixel); crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32));
That looks horrid. I suspect the simplest fix is to change the memset() offset/length to bytes from bits. Or (assuming the alpha channel is last) just: *(u8 *)(vaddr_out + src_offset + 3) = 0; I'm not sure of the options for the crc code, but if you are only passing in 4 bytes there ought to be an option to pass in the value itself (rather than the address).
Do you actually want to 'zap' the alpha channel data from the output buffer? or just exclude it from the crc??
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)