On Thu, Jan 31, 2019 at 10:44:09AM -0200, Rodrigo Siqueira wrote: > 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. If this only happens once then maybe we have a race condition somewhere in our CRC code. Or we need to seed it with something to avoid a silly corner case. If it happens all the time (i.e. every time the test captures a crc) then there's a bug in our crc code. -Daniel > 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 <mamtashukla555@xxxxxxxxx> > > --- > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
