[PATCH v3 3/4] drm/vkms: add XRGB planes composition

[Daniel Vetter]

On Tue, Apr 13, 2021 at 04:54:52AM -0300, Melissa Wen 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 just copying the RGB values of a
> pixel from src to dst, without the need for alpha blending
> operations for each pixel.
> 
> Signed-off-by: Melissa Wen <melissa.srw at gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 22 ++++++++++++++++++----
>  drivers/gpu/drm/vkms/vkms_plane.c    |  7 ++++---
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index be8f1d33c645..7fe1fdb3af39 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>
> @@ -76,6 +77,11 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
>  	argb_dst[3] = 0xFF;
>  }
>  
> +static void x_blending(const u8 *xrgb_src, u8 *xrgb_dst)
> +{
> +	memcpy(xrgb_dst, xrgb_src, sizeof(u8) * 3);

Don't we need to clear the alpha channel, or is that done already when we
allocate the buffer? Looking at the code we have I think we have a few
more bugs there to fix.

I also just realized that we don't support when the primary plane isn't
exactly matching our output size. So that's another thing to maybe fix -
with your new x_blending here what we could do is essentially blend the
primary plane onto the black background (allocated with all zeros) like we
blend the other planes.

Minimally I'd add a comment above that we rely on alpha being cleared in
the target buffer or something like that for stable crc values.

Anyway, work for another patch set.

> +}
> +
>  /**
>   * blend - blend value at vaddr_src with value at vaddr_dst
>   * @vaddr_dst: destination address
> @@ -91,7 +97,8 @@ static void alpha_blending(const u8 *argb_src, u8 *argb_dst)
>   */
>  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 +126,7 @@ 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);

Since it's all one file, did you check whether gcc inlines the two
functions calls here? If we do actual function calls for each blend it's
going to be a lot slower. Maybe worth checking, and perhaps we need to
throw some ineline hints (like always_inline or something like that) on
both the blend function and the alpha_blending and x_blending helpers.

Cursor is generally tiny, but when we start "blending" the primary plane
into the black background, then performance really starts to matter.

Anyway that's all stuff for later tuning.

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

>  		}
>  		i_dst++;
>  	}
> @@ -131,6 +138,8 @@ static void compose_planes(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 +147,13 @@ static void compose_planes(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_blending;
> +	else
> +		pixel_blend = &x_blending;
> +
> +	blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
> +	      plane_composer, pixel_blend);
>  }
>  
>  static int composite(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;
> -- 
> 2.30.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch