[PATCH v2 2/2] drm/vkms: Modify memset() in compute_crc function

[Rodrigo Siqueira]

Hi,

On 01/30, Mamta Shukla wrote:
> Replace memset(vaddr_out + src_offset + 24, 0,  8) with
> memset(vaddr_out + src_offset + 3, 0, 1) because memset fills
> memory in bytes and not in bits.
> 
> Signed-off-by: Mamta Shukla <mamtashukla555 at gmail.com>
> ---
> No changes in v2.
> 
>  drivers/gpu/drm/vkms/vkms_crc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index dc6cb4c2cced..5135642fb204 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -31,7 +31,7 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
>  				     + (i * crc_out->pitch)
>  				     + (j * crc_out->cpp);
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
> +			memset(vaddr_out + src_offset + 3, 0, 1);

Nice catch :)

This look like a bug.
The strange part for me is the fact that IGT does not complain about
this operation. Additionally, I expect a buffer overflow here... Why the
current code works without any problem?

Anyway...
As you already knows, this patch makes IGT shows some warnings like
this:

(kms_cursor_crc:423) igt_debugfs-WARNING: Warning on condition all_zero in function crc_sanity_checks, file ../lib/igt_debugfs.c:901
(kms_cursor_crc:423) igt_debugfs-WARNING: Suspicious CRC: All values are 0.

For me, your code makes sense, but I can't understand why we still get
these warnings. After long hours of testing and thinking about this
issue I started to suspect that we have a byte-order problem here.

I suspect that "memset(addr + 3, 0, 1)" does not set 0 in the Alpha
channel because we're using a Little-endian architecture and your code
works like a big-endian. In other words, the correct offset should be 0.

I did a bunch of experiments, and in the end, I wrote this "draft" of
fix:

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 9d9e8146db90..a9876ade619b 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -14,9 +14,23 @@
  * 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_crc_data *crc_out)
+
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define RGBA_ALPHA 0
+#define RGBA_BLUE  1
+#define RGBA_GREEN 2
+#define RGBA_RED   3
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define RGBA_ALPHA 3
+#define RGBA_BLUE  2
+#define RGBA_GREEN 1
+#define RGBA_RED   0
+#endif
+
+static uint32_t compute_crc(char *vaddr_out, struct vkms_crc_data *crc_out)
 {
  int i, j, src_offset;
+ unsigned char *pixel;
  int x_src = crc_out->src.x1 >> 16;
  int y_src = crc_out->src.y1 >> 16;
  int h_src = drm_rect_height(&crc_out->src) >> 16;
@@ -29,9 +43,9 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
          + (i * crc_out->pitch)
          + (j * crc_out->cpp);
    /* XRGB format ignores Alpha channel */
-   memset(vaddr_out + src_offset + 24, 0,  8);
-   crc = crc32_le(crc, vaddr_out + src_offset,
-           sizeof(u32));
+   pixel = vaddr_out + src_offset;
+   pixel[RGBA_ALPHA] = 0;
+   crc = crc32_le(crc, pixel, sizeof(u32));
   }
  }
 
Noticed that I made some extra changes, but the important part
"pixel[RGBA_ALPHA] = 0". Accordingly with the endianness architecture
the macro RGBA will change its value. Anyway, the above code fixed the
warning issue; Could you take a look on this? Or do you have another
idea?

In addition, Daniel suggested taking a look at "drm fourcc" code since
it has fixed endianness and take a look at DRM_FORMAT_HOST_ARGB8888.

Best Regards

>  			crc = crc32_le(crc, vaddr_out + src_offset,
>  				       sizeof(u32));
>  		}
> -- 
> 2.17.1
> 

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190214/b926f058/attachment-0001.sig>