Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 18 Jun 2019 11:27:14 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc:     Brian Starkey <brian.starkey@arm.com>,
        Liviu Dudau <liviu.dudau@arm.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        Simon Ser <contact@emersion.fr>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 1/5] drm/vkms: Move functions from vkms_crc to
 vkms_composer
Message-ID: <20190618092714.GP12905@phenom.ffwll.local>
Mail-Followup-To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
        Brian Starkey <brian.starkey@arm.com>,
        Liviu Dudau <liviu.dudau@arm.com>,
        Haneen Mohammed <hamohammed.sa@gmail.com>,
        Simon Ser <contact@emersion.fr>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <cover.1560820888.git.rodrigosiqueiramelo@gmail.com>
 <408a662d504db1cfe13919688a4e9f7f7a6d8489.1560820888.git.rodrigosiqueiramelo@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <408a662d504db1cfe13919688a4e9f7f7a6d8489.1560820888.git.rodrigosiqueiramelo@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 17, 2019 at 11:42:33PM -0300, Rodrigo Siqueira wrote:
> The vkms_crc file has functions related to compose operations which are
> not directly associated with CRC. This patch, move those function for a
> new file named vkms_composer.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

I guess my suggestion wasn't quite clear: I thought we should rename the
entire file from vkms_crc.c to vkms_composor.c. And then adjust functions
and datastructures to use vkms_composer instead of vkms_crc as the prefix.
-Daniel

> ---
>  drivers/gpu/drm/vkms/Makefile        |  9 +++-
>  drivers/gpu/drm/vkms/vkms_composer.c | 69 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_composer.h | 12 +++++
>  drivers/gpu/drm/vkms/vkms_crc.c      | 65 +-------------------------
>  4 files changed, 90 insertions(+), 65 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_composer.c
>  create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
> 
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 89f09bec7b23..b4c040854bd6 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -1,4 +1,11 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
> +vkms-y := \
> +	vkms_drv.o \
> +	vkms_plane.o \
> +	vkms_output.o \
> +	vkms_crtc.o \
> +	vkms_gem.o \
> +	vkms_composer.o \
> +	vkms_crc.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> new file mode 100644
> index 000000000000..3d7c5e316d6e
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "vkms_drv.h"
> +#include "vkms_composer.h"
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +/**
> + * blend - belnd value at vaddr_src with value at vaddr_dst
> + * @vaddr_dst: destination address
> + * @vaddr_src: source address
> + * @dest: destination framebuffer's metadata
> + * @src: source framebuffer's metadata
> + *
> + * Blend value at vaddr_src with value at vaddr_dst.
> + * Currently, this function write value at vaddr_src on value
> + * at vaddr_dst using buffer's metadata to locate the new values
> + * from vaddr_src and their distenation at vaddr_dst.
> + *
> + * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> + *	 instead of overwriting it.
> + */
> +void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
> +	   struct vkms_crc_data *src)
> +{
> +	int i, j, j_dst, i_dst;
> +	int offset_src, offset_dst;
> +
> +	int x_src = src->src.x1 >> 16;
> +	int y_src = src->src.y1 >> 16;
> +
> +	int x_dst = src->dst.x1;
> +	int y_dst = src->dst.y1;
> +	int h_dst = drm_rect_height(&src->dst);
> +	int w_dst = drm_rect_width(&src->dst);
> +
> +	int y_limit = y_src + h_dst;
> +	int x_limit = x_src + w_dst;
> +
> +	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 = dest->offset
> +				     + (i_dst * dest->pitch)
> +				     + (j_dst++ * dest->cpp);
> +			offset_src = src->offset
> +				     + (i * src->pitch)
> +				     + (j * src->cpp);
> +
> +			memcpy(vaddr_dst + offset_dst,
> +			       vaddr_src + offset_src, sizeof(u32));
> +		}
> +		i_dst++;
> +	}
> +}
> +
> +void compose_cursor(struct vkms_crc_data *cursor,
> +		    struct vkms_crc_data *primary, void *vaddr_out)
> +{
> +	struct drm_gem_object *cursor_obj;
> +	struct vkms_gem_object *cursor_vkms_obj;
> +
> +	cursor_obj = drm_gem_fb_get_obj(&cursor->fb, 0);
> +	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> +
> +	if (WARN_ON(!cursor_vkms_obj->vaddr))
> +		return;
> +
> +	blend(vaddr_out, cursor_vkms_obj->vaddr, primary, cursor);
> +}
> +
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
> new file mode 100644
> index 000000000000..53fdee17a632
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_composer.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _VKMS_COMPOSER_H_
> +#define _VKMS_COMPOSER_H_
> +
> +void blend(void *vaddr_dst, void *vaddr_src, struct vkms_crc_data *dest,
> +	   struct vkms_crc_data *src);
> +
> +void compose_cursor(struct vkms_crc_data *cursor,
> +		    struct vkms_crc_data *primary, void *vaddr_out);
> +
> +#endif /* _VKMS_COMPOSER_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index 4b3146d83265..3c6a35aba494 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include "vkms_drv.h"
> +#include "vkms_composer.h"
>  #include <linux/crc32.h>
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -39,70 +40,6 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_crc_data *crc_out)
>  	return crc;
>  }
>  
> -/**
> - * blend - belnd value at vaddr_src with value at vaddr_dst
> - * @vaddr_dst: destination address
> - * @vaddr_src: source address
> - * @crc_dst: destination framebuffer's metadata
> - * @crc_src: source framebuffer's metadata
> - *
> - * Blend value at vaddr_src with value at vaddr_dst.
> - * Currently, this function write value at vaddr_src on value
> - * at vaddr_dst using buffer's metadata to locate the new values
> - * from vaddr_src and their distenation at vaddr_dst.
> - *
> - * Todo: Use the alpha value to blend vaddr_src with vaddr_dst
> - *	 instead of overwriting it.
> - */
> -static void blend(void *vaddr_dst, void *vaddr_src,
> -		  struct vkms_crc_data *crc_dst,
> -		  struct vkms_crc_data *crc_src)
> -{
> -	int i, j, j_dst, i_dst;
> -	int offset_src, offset_dst;
> -
> -	int x_src = crc_src->src.x1 >> 16;
> -	int y_src = crc_src->src.y1 >> 16;
> -
> -	int x_dst = crc_src->dst.x1;
> -	int y_dst = crc_src->dst.y1;
> -	int h_dst = drm_rect_height(&crc_src->dst);
> -	int w_dst = drm_rect_width(&crc_src->dst);
> -
> -	int y_limit = y_src + h_dst;
> -	int x_limit = x_src + w_dst;
> -
> -	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
> -				     + (i_dst * crc_dst->pitch)
> -				     + (j_dst++ * crc_dst->cpp);
> -			offset_src = crc_src->offset
> -				     + (i * crc_src->pitch)
> -				     + (j * crc_src->cpp);
> -
> -			memcpy(vaddr_dst + offset_dst,
> -			       vaddr_src + offset_src, sizeof(u32));
> -		}
> -		i_dst++;
> -	}
> -}
> -
> -static void compose_cursor(struct vkms_crc_data *cursor_crc,
> -			   struct vkms_crc_data *primary_crc, void *vaddr_out)
> -{
> -	struct drm_gem_object *cursor_obj;
> -	struct vkms_gem_object *cursor_vkms_obj;
> -
> -	cursor_obj = drm_gem_fb_get_obj(&cursor_crc->fb, 0);
> -	cursor_vkms_obj = drm_gem_to_vkms_gem(cursor_obj);
> -
> -	if (WARN_ON(!cursor_vkms_obj->vaddr))
> -		return;
> -
> -	blend(vaddr_out, cursor_vkms_obj->vaddr, primary_crc, cursor_crc);
> -}
> -
>  static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc,
>  			      struct vkms_crc_data *cursor_crc)
>  {
> -- 
> 2.21.0

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