RE: [PATCH] drm/vkms: use bitfield op to get xrgb on compute crc

[David Laight <David.Laight@ACULAB.COM>]

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)