Re: [PATCH 09/13] exynos/fimg2d: add g2d_move

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
To: Hyungwon Hwang <human.hwang@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org, emil.l.velikov@gmail.com,
	dri-devel@lists.freedesktop.org, gustavo.padovan@collabora.co.uk
Subject: Re: [PATCH 09/13] exynos/fimg2d: add g2d_move
Date: Mon, 9 Nov 2015 10:47:02 +0100	[thread overview]
Message-ID: <56406B96.1050701@math.uni-bielefeld.de> (raw)
In-Reply-To: <20151109163013.011f4cb6@hwh-ubuntu>

Hello Hyungwon,

Hyungwon Hwang wrote:
> Hello Tobias,
> 
> I was in vacation last week, so I could run your code today. I found
> that what g2d_move() does is actually copying not moving, because the
> operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc.
Like in libc 'moving' memory doesn't actually move it, like you would
move a real object, since it's undefined what 'empty' memory should be.

The same applies here. It's not defined what 'empty' areas of the buffer
should be.

> Would it be possible to
> generalize g2d_copy() works better, so it could works well in case of
> the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.

I also want the user to explicitly request this. The user should make
sure what requirements he has for the buffers in question. Are the
buffers disjoint or not?

> If it is possible, I think it
> would be better way to do that. If it is not, at least chaning the
> function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?

I still think the naming is fine though, since it mirrors libc's naming.
And the user is supposed to read the documentation anyway.

With best wishes,
Tobias

> 
> Best regards,
> Hyungwon Hwang
> 
> On Tue, 22 Sep 2015 17:54:58 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> We already have g2d_copy() which implements G2D copy
>> operations from one buffer to another. However we can't
>> do a overlapping copy operation in one buffer.
>>
>> Add g2d_move() which acts like the standard memmove()
>> and properly handles overlapping copies.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  exynos/exynos_fimg2d.c | 94
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> exynos/exynos_fimg2d.h |  3 ++ 2 files changed, 97 insertions(+)
>>
>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>> index 4d5419c..8703629 100644
>> --- a/exynos/exynos_fimg2d.c
>> +++ b/exynos/exynos_fimg2d.c
>> @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct
>> g2d_image *src, }
>>  
>>  /**
>> + * g2d_move - move content inside single buffer.
>> + *	Similar to 'memmove' this moves a rectangular region
>> + *	of the provided buffer to another location (the source
>> + *	and destination region potentially overlapping).
>> + *
>> + * @ctx: a pointer to g2d_context structure.
>> + * @img: a pointer to g2d_image structure providing
>> + *	buffer information.
>> + * @src_x: x position of source rectangle.
>> + * @src_y: y position of source rectangle.
>> + * @dst_x: x position of destination rectangle.
>> + * @dst_y: y position of destination rectangle.
>> + * @w: width of rectangle to move.
>> + * @h: height of rectangle to move.
>> + */
>> +int
>> +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
>> +		unsigned int src_x, unsigned int src_y,
>> +		unsigned int dst_x, unsigned dst_y, unsigned int w,
>> +		unsigned int h)
>> +{
>> +	union g2d_rop4_val rop4;
>> +	union g2d_point_val pt;
>> +	union g2d_direction_val dir;
>> +	unsigned int src_w, src_h, dst_w, dst_h;
>> +
>> +	src_w = w;
>> +	src_h = h;
>> +	if (src_x + img->width > w)
>> +		src_w = img->width - src_x;
>> +	if (src_y + img->height > h)
>> +		src_h = img->height - src_y;
>> +
>> +	dst_w = w;
>> +	dst_h = w;
>> +	if (dst_x + img->width > w)
>> +		dst_w = img->width - dst_x;
>> +	if (dst_y + img->height > h)
>> +		dst_h = img->height - dst_y;
>> +
>> +	w = MIN(src_w, dst_w);
>> +	h = MIN(src_h, dst_h);
>> +
>> +	if (w == 0 || h == 0) {
>> +		fprintf(stderr, MSG_PREFIX "invalid width or
>> height.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (g2d_check_space(ctx, 13, 2))
>> +		return -ENOSPC;
>> +
>> +	g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR);
>> +	g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL);
>> +
>> +	g2d_add_cmd(ctx, DST_COLOR_MODE_REG, img->color_mode);
>> +	g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, img->color_mode);
>> +
>> +	g2d_add_base_addr(ctx, img, g2d_dst);
>> +	g2d_add_base_addr(ctx, img, g2d_src);
>> +
>> +	g2d_add_cmd(ctx, DST_STRIDE_REG, img->stride);
>> +	g2d_add_cmd(ctx, SRC_STRIDE_REG, img->stride);
>> +
>> +	dir.val[0] = dir.val[1] = 0;
>> +
>> +	if (dst_x >= src_x)
>> +		dir.data.src_x_direction = dir.data.dst_x_direction
>> = 1;
>> +	if (dst_y >= src_y)
>> +		dir.data.src_y_direction = dir.data.dst_y_direction
>> = 1; +
>> +	g2d_set_direction(ctx, &dir);
>> +
>> +	pt.data.x = src_x;
>> +	pt.data.y = src_y;
>> +	g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val);
>> +	pt.data.x = src_x + w;
>> +	pt.data.y = src_y + h;
>> +	g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
>> +
>> +	pt.data.x = dst_x;
>> +	pt.data.y = dst_y;
>> +	g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val);
>> +	pt.data.x = dst_x + w;
>> +	pt.data.y = dst_y + h;
>> +	g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
>> +
>> +	rop4.val = 0;
>> +	rop4.data.unmasked_rop3 = G2D_ROP3_SRC;
>> +	g2d_add_cmd(ctx, ROP4_REG, rop4.val);
>> +
>> +	return g2d_flush(ctx);
>> +}
>> +
>> +/**
>>   * g2d_copy_with_scale - copy contents in source buffer to
>> destination buffer
>>   *	scaling up or down properly.
>>   *
>> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
>> index 9eee7c0..2700686 100644
>> --- a/exynos/exynos_fimg2d.h
>> +++ b/exynos/exynos_fimg2d.h
>> @@ -343,6 +343,9 @@ int g2d_copy(struct g2d_context *ctx, struct
>> g2d_image *src, struct g2d_image *dst, unsigned int src_x,
>>  		unsigned int src_y, unsigned int dst_x, unsigned int
>> dst_y, unsigned int w, unsigned int h);
>> +int g2d_move(struct g2d_context *ctx, struct g2d_image *img,
>> +		unsigned int src_x, unsigned int src_y, unsigned int
>> dst_x,
>> +		unsigned dst_y, unsigned int w, unsigned int h);
>>  int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image
>> *src, struct g2d_image *dst, unsigned int src_x,
>>  				unsigned int src_y, unsigned int
>> src_w,
> 

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