[PATCH v2,1/2] drm/vkms: Use alpha for blending in blend() function
Daniel Vetter
daniel at ffwll.ch
Fri Feb 1 17:15:30 UTC 2019
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 at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list