Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

[Vasilev, Oleg]

On Tue, 2019-06-25 at 22:38 -0300, Rodrigo Siqueira wrote:
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a
> copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying
> the
> input framebuffer.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++---------
> --
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 51a270514219..8126aa0f968f 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -6,33 +6,40 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> +				 const struct vkms_composer *composer)
> +{
> +	int src_offset = composer->offset + (y * composer->pitch)
> +					  + (x * composer->cpp);
> +
> +	return *(u32 *)&buffer[src_offset];
> +}
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
>   * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer
> *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> +			    const struct vkms_composer *composer)
>  {
> -	int i, j, src_offset;
> +	int x, y;
>  	int x_src = composer->src.x1 >> 16;
>  	int y_src = composer->src.y1 >> 16;
>  	int h_src = drm_rect_height(&composer->src) >> 16;
>  	int w_src = drm_rect_width(&composer->src) >> 16;
> -	u32 crc = 0;
> +	u32 crc = 0, pixel = 0;
>  
> -	for (i = y_src; i < y_src + h_src; ++i) {
> -		for (j = x_src; j < x_src + w_src; ++j) {
> -			src_offset = composer->offset
> -				     + (i * composer->pitch)
> -				     + (j * composer->cpp);
> +	for (y = y_src; y < y_src + h_src; ++y) {
> +		for (x = x_src; x < x_src + w_src; ++x) {
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
Hi Rodgrigo,

Do I understand correctly, that previous version with memset was
actually zeroing out the whole fb, except first 24 bytes? On the first
iteration bytes 24..32 would be zeroed, on the second 25..33, etc.

Should we add more CRC tests to IGT, so we can catch such mistakes? For
example, compute CRC, than augment random pixel and assert that CRC
changed.

Oleg
> -			crc = crc32_le(crc, vaddr_out + src_offset,
> -				       sizeof(u32));
> +			pixel = get_pixel_from_buffer(x, y, vaddr,
> composer);
> +			bitmap_clear((void *)&pixel, 0, 8);
> +			crc = crc32_le(crc, (void *)&pixel,
> sizeof(u32));
>  		}
>  	}
>

Attachment: smime.p7s
Description: S/MIME cryptographic signature