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 <mamtashukla...@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
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
