Hi, First of all, thanks for your patch :)
I tested your patch against the tests that you implemented in the IGT [1]. All the alpha tests passed, but there was a weird warning that says: $ sudo IGT_FORCE_DRIVER=vkms ./tests/kms_cursor_crc --run-subtest cursor-alpha-opaque IGT-Version: 1.23-g8d81c2c2 (x86_64) (Linux: 5.0.0-rc1-VKMS-RULES+ x86_64) Force option used: Using driver vkms Starting subtest: cursor-alpha-opaque (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. Beginning cursor-alpha-opaque on pipe A, connector Virtual-2 cursor-alpha-opaque on pipe A, connector Virtual-2: PASSED Subtest cursor-alpha-opaque: SUCCESS (0.315s) Do you know the reason for that? Could you detail this issue? Is it possible to fix it? You can see the other comments inline. [1] https://patchwork.freedesktop.org/series/55944/ On 01/30, Mamta Shukla wrote: > Use the alpha value to blend vaddr_src with vaddr_dst instead > of overwriting it in blend(). > > Signed-off-by: Mamta Shukla <mamtashukla...@gmail.com> > --- > changes in v2: > -Use macro to avoid code duplication > -Add spaces around '/' and '-' > -Remove spaces at the end of the line > > drivers/gpu/drm/vkms/vkms_crc.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > index 9d9e8146db90..dc6cb4c2cced 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -5,6 +5,8 @@ > #include <drm/drm_atomic_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > > +#define BITSHIFT(val,i) (u8)((*(u32 *)(val)) >> i & 0xff) - Take care with the macros definition, since you can create a precedence problem. For example, here, you didn't surround “i” with “()”. - At the end of this operation you cast all the value to u8. In this sense, why do you need the 0xff in the end? - I’m worried about the little and big endian issues here. If I understood well, this macro could fail on a big-endian environment. Is it right? Did I miss something? Could you explain to me what going to happen in the big and endian case? - Finally, did you take a look at “include/linux/bitops.h” and “include/linux/kernel.h”? These headers have a bunch of useful macros and functions; probably you can find something useful for you in this file. > + > /** > * compute_crc - Compute CRC value on output frame > * > @@ -71,6 +73,9 @@ static void blend(void *vaddr_dst, void *vaddr_src, > int y_limit = y_src + h_dst; > int x_limit = x_src + w_dst; > > + u8 alpha, r_src, r_dst, g_src, g_dst, b_src, b_dst; > + u8 r_alpha, g_alpha, b_alpha; > + > for (i = y_src, i_dst = y_dst; i < y_limit; ++i) { > for (j = x_src, j_dst = x_dst; j < x_limit; ++j) { > offset_dst = crc_dst->offset > @@ -79,9 +84,25 @@ static void blend(void *vaddr_dst, void *vaddr_src, > offset_src = crc_src->offset > + (i * crc_src->pitch) > + (j * crc_src->cpp); > + > + /*Currently handles alpha values for fully opaque or > fully transparent*/ > + alpha = (u8)((*(u32 *)vaddr_src + offset_src) >> 24); > + alpha = alpha / 255; > + r_src = BITSHIFT(vaddr_src + offset_src, 16); > + g_src = BITSHIFT(vaddr_src + offset_src, 8); > + b_src = BITSHIFT(vaddr_src + offset_src, 0); If I correctly understood, you have an u32 values which gave you 4 bytes; because of this, you have one byte for Red, Green, Blue, and Alpha. The above operations extracts each value, one by one. In this sense, why do we need all of this bitwise operation since you can access it as an array of chars? Something like this (draft alert): char *cursor_addr = (char*)vaddr_src + offset_src; r_src = cursor_addr[2]; g_src = cursor_addr[1]; b_src = cursor_addr[0]; ... There's any restriction for that? Is it related to the big and little endian issue? Finally, is it ok to make pointer operation with void* in the kernel? > + r_dst = BITSHIFT(vaddr_dst + offset_dst, 16); > + g_dst = BITSHIFT(vaddr_dst + offset_dst, 8); > + b_dst = BITSHIFT(vaddr_dst + offset_dst, 0); > + > + /*Pre-multiplied alpha for blending */ > + r_alpha = (r_src) + (r_dst * (1 - alpha)); > + g_alpha = (g_src) + (g_dst * (1 - alpha)); > + b_alpha = (b_src) + (b_dst * (1 - alpha)); > + memset(vaddr_dst + offset_dst, b_alpha, sizeof(u8)); > + memset(vaddr_dst + offset_dst + 1, g_alpha, sizeof(u8)); > + memset(vaddr_dst + offset_dst + 2, r_alpha, sizeof(u8)); IMHO, I prefer to move all above alpha operations for an inline function on top of “blend()” with the goal of improve the code readability. > - memcpy(vaddr_dst + offset_dst, > - vaddr_src + offset_src, sizeof(u32)); > } > i_dst++; > } > -- > 2.17.1 > Finally, your patch introduces some checkpatch warnings and errors. I highly recommend you to use checkpatch in your patches before you send it. In the link below [2], there’s a section named “Git post-commit hooks”; take a look at it. [2] https://kernelnewbies.org/FirstKernelPatch Again, thanks for your patch. Best Regards -- 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
