From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Siqueira via dri-devel Subject: Re: [PATCH v2 2/2] drm/vkms: Modify memset() in compute_crc function Date: Thu, 14 Feb 2019 20:31:58 -0200 Message-ID: <20190214223158.ris2sgueygkwxzoh@smtp.gmail.com> References: <48ecdf48d153d0ca3b954ba737ae9a0b35ef3ad5.1548798106.git.mamtashukla555@gmail.com> Reply-To: Rodrigo Siqueira Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0484440166==" Return-path: Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) by gabe.freedesktop.org (Postfix) with ESMTPS id 383DD6EAC2 for ; Thu, 14 Feb 2019 22:32:04 +0000 (UTC) Received: by mail-qk1-x744.google.com with SMTP id o125so4626190qkf.3 for ; Thu, 14 Feb 2019 14:32:04 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Mamta Shukla Cc: hamohammed.sa@gmail.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0484440166== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="qfbe2326uhx7im55" Content-Disposition: inline --qfbe2326uhx7im55 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Mamta Shukla > --- > No changes in v2. >=20 > drivers/gpu/drm/vkms/vkms_crc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > 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 vkm= s_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_cr= c.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__ =3D=3D __ORDER_LITTLE_ENDIAN__ +#define RGBA_ALPHA 0 +#define RGBA_BLUE 1 +#define RGBA_GREEN 2 +#define RGBA_RED 3 +#elif __BYTE_ORDER__ =3D=3D __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 =3D crc_out->src.x1 >> 16; int y_src =3D crc_out->src.y1 >> 16; int h_src =3D 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 =3D crc32_le(crc, vaddr_out + src_offset, - sizeof(u32)); + pixel =3D vaddr_out + src_offset; + pixel[RGBA_ALPHA] =3D 0; + crc =3D crc32_le(crc, pixel, sizeof(u32)); } } =20 Noticed that I made some extra changes, but the important part "pixel[RGBA_ALPHA] =3D 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 =3D crc32_le(crc, vaddr_out + src_offset, > sizeof(u32)); > } > --=20 > 2.17.1 >=20 --=20 Rodrigo Siqueira https://siqueira.tech Graduate Student Department of Computer Science University of S=E3o Paulo --qfbe2326uhx7im55 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE4tZ+ii1mjMCMQbfkWJzP/comvP8FAlxl7F4ACgkQWJzP/com vP8/mRAAycY0vPsVaxmRUp+d5soVNO0TN73wl3u03G5Hk3QW8fnMuWZ1aqGLYp2R VcfPx1gnUWWdn/jvRQa6o0+SCMn44yCNv4qZ7duL0k7kiO7cM5RMy+HmpxMjKEJV /AUeRAW2CtBqL7VLN/5yKc0IbEw9Rf/q/uzMTZBVe2x4uFg+5QrTsqxtxmEU7eJp Nl6/6bazY93GSNv2rxNXpQ29Ft2g+hm+3RiCc4dT6LbBs4+YLN5wpkvy8C+kzNS3 4/Xe+1o2nYaPtW4Ot510aEajY6U7nvkzVZpKyUAvE7ic3mBITcoVvqCL+vjqWqGI 7WwJEiqEQoZTnA4ur2p7Jkwj2ZhouJCbQWc7wKRqEVTe9WTg8+G63aj0qDHzFMfq S/JYactg4Q1g/UAaaPSF1Ya0XhjOazNv5AeRM977Vxh+3UNHYK4HbOE5+5UbQNtm IdL4Uocrj4FyYVLQwVnLEmTL82FKEqNz5HtfdT5SYGNSJvKLT++IgzjePsbYuViL s01KwcNp3yRKOVDuk9QS/5LSRfDkpVOQJrug6uDA0F8h+u6OWhzgt2yiDYcA2980 0yeB3jLXjPIxDQa7bqGMFTR2P5WFn2o1rTGcC9sLiAIequxF04laxV0C+o7q2pbi dNhHYTHwIiVatuijLcLfH07FxCfGwu/hbFJvF9h6YyxZBPwZ4p8= =7DMc -----END PGP SIGNATURE----- --qfbe2326uhx7im55-- --===============0484440166== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============0484440166==--