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

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

[Mamta Shukla]

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)
+
 /**
  * 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);
+			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));
 
-			memcpy(vaddr_dst + offset_dst,
-			       vaddr_src + offset_src, sizeof(u32));
 		}
 		i_dst++;
 	}
-- 
2.17.1

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

[PATCH v2 2/2] drm/vkms: Modify memset() in compute_crc function

[Mamta Shukla]

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);
 			crc = crc32_le(crc, vaddr_out + src_offset,
 				       sizeof(u32));
 		}
-- 
2.17.1

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

Re: [PATCH v2 2/2] drm/vkms: Modify memset() in compute_crc function

[Wentland, Harry]

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));
>  		}
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/vkms: Modify memset() in compute_crc function

[Rodrigo Siqueira via dri-devel]

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

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
> 

-- 
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: 159 bytes --]

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

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

[Rodrigo Siqueira]

[-- 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

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

[Daniel Vetter]

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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel