Hi,
I tested your patch with kms_cursor_crc, and everything worked as
expected! Really nice patch :)
I just have some tiny comments inline.
On 01/24, 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>
> ---
> drivers/gpu/drm/vkms/vkms_crc.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index 9d9e8146db90..13d5ffed9135 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -70,7 +70,8 @@ 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
> @@ -80,8 +81,23 @@ static void blend(void *vaddr_dst, void *vaddr_src,
> + (i * crc_src->pitch)
> + (j * crc_src->cpp);
>
> - memcpy(vaddr_dst + offset_dst,
> - vaddr_src + offset_src, sizeof(u32));
> + /*Currently handles alpha values for fully opaque or fully transparent */
> + alpha = (u8)((*(u32 *)(vaddr_src + offset_src)) >> 24);
> + alpha = alpha/255;
Add spaces between '/'.
> + r_src = (u8)((*(u32 *)(vaddr_src + offset_src)) >> 16 & 0xff);
> + g_src = (u8)((*(u32 *)(vaddr_src + offset_src)) >> 8 & 0xff);
> + b_src = (u8)((*(u32 *)(vaddr_src + offset_src)) & 0xff);
> + r_dst = (u8)((*(u32 *)(vaddr_dst + offset_dst)) >> 16 & 0xff);
> + g_dst = (u8)((*(u32 *)(vaddr_dst + offset_dst)) >> 8 & 0xff);
> + b_dst = (u8)((*(u32 *)(vaddr_dst + offset_dst)) & 0xff);
Since these operations are very similar, it could be better to create
one inline function or macro to reduce the code duplication. I'm not
sure, but I suspect that Linux already provides some macros for this
sort of operations (again, I'm not sure).
> +
> + /*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));
Add space between '-'.
> + memset(vaddr_dst + offset_dst, b_alpha, sizeof(u8));
> + memset(vaddr_dst + offset_dst + 1, g_alpha, sizeof(u8));
Take care with extra spaces at the end.
Finally, remember to run checkpatch in your patches.
> + memset(vaddr_dst + offset_dst + 2, r_alpha, sizeof(u8));
> }
> i_dst++;
> }
> --
> 2.17.1
>
--
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
