Re: [PATCH v2,1/2] drm/vkms: Use alpha for blending in blend() function

[Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5564 bytes --]

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 <mamtashukla555@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel