| Message ID | b7f37c793c9f37cddff6dc548a592412335114c6.1548798107.git.mamtashukla555@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/vkms: Use alpha for blending in blend() function | expand |
On 2019-01-29 5:00 p.m., 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@gmail.com> Series is Reviewed-by: Harry Wentland <harry.wentland@amd.com> Harry > --- > 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); > crc = crc32_le(crc, vaddr_out + src_offset, > sizeof(u32)); > } >
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@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 >
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); crc = crc32_le(crc, vaddr_out + src_offset, sizeof(u32)); }
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@gmail.com> --- No changes in v2. drivers/gpu/drm/vkms/vkms_crc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)