dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Pekka Paalanen <ppaalanen@gmail.com>
To: Melissa Wen <melissa.srw@gmail.com>
Cc: David Airlie <airlied@linux.ie>,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	Sumera Priyadarsini <sylphrenadin@gmail.com>,
	Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 3/4] drm/vkms: add XRGB planes composition
Date: Mon, 26 Apr 2021 11:03:15 +0300	[thread overview]
Message-ID: <20210426110315.4e64d589@eldfell> (raw)
In-Reply-To: <07bcf4643d11da9480599fe1b165e478bff58b25.1619250933.git.melissa.srw@gmail.com>


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

On Sat, 24 Apr 2021 05:25:31 -0300
Melissa Wen <melissa.srw@gmail.com> wrote:

> Add support for composing XRGB888 planes in addition to the ARGB8888
> format. In the case of an XRGB plane at the top, the composition consists
> of copying the RGB values of a pixel from src to dst and clearing alpha
> channel, without the need for alpha blending operations for each pixel.
> 
> Blend equations assume a completely opaque background, i.e., primary plane
> is not cleared before pixel blending but alpha channel is explicitly
> opaque (a = 0xff). Also, there is room for performance evaluation in
> switching pixel blend operation according to the plane format.
> 
> v4:
> - clear alpha channel (0xff) after blend color values by pixel
> - improve comments on blend ops to reflect the current state
> - describe in the commit message future improvements for plane composition
> 
> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 56 ++++++++++++++++++++++------
>  drivers/gpu/drm/vkms/vkms_plane.c    |  7 ++--
>  2 files changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 02642801735d..7e01bc39d2a1 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -4,6 +4,7 @@
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -64,7 +65,17 @@ static u8 blend_channel(u8 src, u8 dst, u8 alpha)
>  	return new_color;
>  }
>  
> -static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
> +/**
> + * alpha_blend - alpha blending equation
> + * @argb_src: src pixel on premultiplied alpha mode
> + * @argb_dst: dst pixel completely opaque
> + *
> + * blend pixels using premultiplied blend formula. The current DRM assumption
> + * is that pixel color values have been already pre-multiplied with the alpha
> + * channel values. See more drm_plane_create_blend_mode_property(). Also, this
> + * formula assumes a completely opaque background.
> + */
> +static void alpha_blend(const u8 *argb_src, u8 *argb_dst)
>  {
>  	u8 alpha;
>  
> @@ -72,8 +83,16 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
>  	argb_dst[0] = blend_channel(argb_src[0], argb_dst[0], alpha);
>  	argb_dst[1] = blend_channel(argb_src[1], argb_dst[1], alpha);
>  	argb_dst[2] = blend_channel(argb_src[2], argb_dst[2], alpha);
> -	/* Opaque primary */
> -	argb_dst[3] = 0xFF;
> +}
> +
> +/**
> + * x_blend - blending equation that ignores the pixel alpha
> + *
> + * overwrites RGB color value from src pixel to dst pixel.
> + */
> +static void x_blend(const u8 *xrgb_src, u8 *xrgb_dst)
> +{
> +	memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);

Hi,

this function very clearly assumes a very specific pixel format on both
source and destination. I think it would be good if the code comments
called out exactly which DRM_FORMAT_* they assume. This would be good
to do on almost every function that makes such assumptions. I believe that
would help code readability, and also point out explicitly which things
need to be fixed when you add support for even more pixel formats.

"xrgb" and "argb" are IMO too vague. You might be referring to
DRM_FORMAT_XRGB* and DRM_FORMAT_ARGB*, or maybe you are referring to any
pixel format that happens to have or not have an alpha channel in
addition to the three RGB channels in some order and width.

Being explicit that these refer to specific DRM_FORMAT_* should also
help understanding how things work on big-endian CPUs. My current
understanding is that this memcpy is correct also on big-endian, given
DRM_FORMAT_XRGB8888.

Hmm, or rather, is this particular function intended to be general in
the sense that the order of RGB channels does not matter as long as it's
the same in both source and destination? Which would mean I had a wrong
assumption from the start.

>  }
>  
>  /**
> @@ -82,16 +101,20 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
>   * @vaddr_src: source address
>   * @dst_composer: destination framebuffer's metadata
>   * @src_composer: source framebuffer's metadata
> + * @pixel_blend: blending equation based on plane format
>   *
> - * Blend the vaddr_src value with the vaddr_dst value using the pre-multiplied
> - * alpha blending equation, since DRM currently assumes that the pixel color
> - * values have already been pre-multiplied with the alpha channel values. See
> - * more drm_plane_create_blend_mode_property(). This function uses buffer's
> - * metadata to locate the new composite values at vaddr_dst.
> + * Blend the vaddr_src value with the vaddr_dst value using a pixel blend
> + * equation according to the plane format and clearing alpha channel to an
> + * completely opaque background. This function uses buffer's metadata to locate
> + * the new composite values at vaddr_dst.
> + *
> + * TODO: completely clear the primary plane (a = 0xff) before starting to blend
> + * pixel color values
>   */
>  static void blend(void *vaddr_dst, void *vaddr_src,
>  		  struct vkms_composer *dst_composer,
> -		  struct vkms_composer *src_composer)
> +		  struct vkms_composer *src_composer,
> +		  void (*pixel_blend)(const u8 *, u8 *))
>  {
>  	int i, j, j_dst, i_dst;
>  	int offset_src, offset_dst;
> @@ -119,7 +142,9 @@ static void blend(void *vaddr_dst, void *vaddr_src,
>  
>  			pixel_src = (u8 *)(vaddr_src + offset_src);
>  			pixel_dst = (u8 *)(vaddr_dst + offset_dst);
> -			alpha_blending(pixel_src, pixel_dst);
> +			pixel_blend(pixel_src, pixel_dst);
> +			/* clearing alpha channel (0xff)*/
> +			memset(vaddr_dst + offset_dst + 3, 0xff, 1);

A one byte memset?

Wouldn't pixel_dst[3] = 0xff; be more clear?


Thanks,
pq

>  		}
>  		i_dst++;
>  	}
> @@ -131,6 +156,8 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  {
>  	struct drm_gem_object *plane_obj;
>  	struct drm_gem_shmem_object *plane_shmem_obj;
> +	struct drm_framebuffer *fb = &plane_composer->fb;
> +	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
>  
>  	plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0);
>  	plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
> @@ -138,8 +165,13 @@ static void compose_plane(struct vkms_composer *primary_composer,
>  	if (WARN_ON(!plane_shmem_obj->vaddr))
>  		return;
>  
> -	blend(vaddr_out, plane_shmem_obj->vaddr,
> -	      primary_composer, plane_composer);
> +	if (fb->format->format == DRM_FORMAT_ARGB8888)
> +		pixel_blend = &alpha_blend;
> +	else
> +		pixel_blend = &x_blend;
> +
> +	blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
> +	      plane_composer, pixel_blend);
>  }
>  
>  static int compose_active_planes(void **vaddr_out,
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 135140f8e87a..da4251aff67f 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -16,8 +16,9 @@ static const u32 vkms_formats[] = {
>  	DRM_FORMAT_XRGB8888,
>  };
>  
> -static const u32 vkms_cursor_formats[] = {
> +static const u32 vkms_plane_formats[] = {
>  	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XRGB8888
>  };
>  
>  static struct drm_plane_state *
> @@ -200,8 +201,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  	int nformats;
>  
>  	if (type == DRM_PLANE_TYPE_CURSOR) {
> -		formats = vkms_cursor_formats;
> -		nformats = ARRAY_SIZE(vkms_cursor_formats);
> +		formats = vkms_plane_formats;
> +		nformats = ARRAY_SIZE(vkms_plane_formats);
>  		funcs = &vkms_primary_helper_funcs;
>  	} else {
>  		formats = vkms_formats;


[-- Attachment #1.2: OpenPGP digital signature --]
[-- 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

  reply	other threads:[~2021-04-26  8:03 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-24  8:22 [PATCH v4 0/4] drm/vkms: add overlay plane support Melissa Wen
2021-04-24  8:23 ` [PATCH v4 1/4] drm/vkms: init plane using drmm_universal_plane_alloc Melissa Wen
2021-04-24  8:24 ` [PATCH v4 2/4] drm/vkms: rename cursor to plane on ops of planes composition Melissa Wen
2021-04-24  8:25 ` [PATCH v4 3/4] drm/vkms: add XRGB " Melissa Wen
2021-04-26  8:03   ` Pekka Paalanen [this message]
2021-04-26 16:28     ` Daniel Vetter
2021-04-26 17:31       ` Melissa Wen
2021-04-27  8:10         ` Pekka Paalanen
2021-04-27  8:31           ` Melissa Wen
2021-04-27  9:04           ` Daniel Vetter
2021-04-24  8:26 ` [PATCH v4 4/4] drm/vkms: add overlay support Melissa Wen
2021-04-26  8:07   ` Pekka Paalanen
2021-04-26 17:07     ` Melissa Wen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210426110315.4e64d589@eldfell \
    --to=ppaalanen@gmail.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hamohammed.sa@gmail.com \
    --cc=melissa.srw@gmail.com \
    --cc=rodrigosiqueiramelo@gmail.com \
    --cc=sylphrenadin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).