| Message ID | 1442937302-8211-10-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; + As I commented in the patch 08/13, I think that direction is related with flip. If I am right, these two if statements looks awkward. Best regards, Hyungwon Hwang > + 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,
Hello Hyungwon, Hyungwon Hwang wrote: > 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; + > > As I commented in the patch 08/13, I think that direction is related > with flip. If I am right, these two if statements looks awkward. I hope my other mail clear things up. The code above does exactly what it's supposed to do. You can also check this with the test application I incuded. E.g. you can remove the setting of the direction registers and quickly observe image corruption of the moved region. With best wishes, Tobias > > Best regards, > Hyungwon Hwang > >> + 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, >
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. 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. 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. 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,
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, >
Hello Tobias, On Mon, 09 Nov 2015 10:47:02 +0100 Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > 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 In that manner following glibc, I agree that the naming is reasonable. I commented like that previously, because at the first time when I run the test, I think that the result seems like a bug. The test program said that it was a move test, but the operation seemed copying. It would be just OK if it is well documented or printed when runs the test that the test does not do anything about the previous area intentionally. BRs, Hyungwon Hwang > > > > > 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, > > > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-samsung-soc" in the body of a message to > majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
Hello Hyungwon, Hyungwon Hwang wrote: > Hello Tobias, > > On Mon, 09 Nov 2015 10:47:02 +0100 > Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > >> 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 > > In that manner following glibc, I agree that the naming is reasonable. well, that was just my way of thinking. But I guess most people have experience using the libc, so the naming should look at least 'familiar'. > I commented like that previously, because at the first time when I run > the test, I think that the result seems like a bug. The test program > said that it was a move test, but the operation seemed copying. Ok, so just that I understand this correctly. Your issue is with the commit the description of the test or with the commit description of the patch that introduces g2d_move()? Because I don't see what you point out in the test commit description: " tests/exynos: add test for g2d_move To check if g2d_move() works properly we create a small checkerboard pattern in the center of the screen and then shift this pattern around with g2d_move(). The pattern should be properly preserved by the operation. " I intentionally avoid to write "...move this pattern around...", so instead I choose "shift". I'm not a native speaker, so I'm clueless how to formulate this in a more clear way. > It > would be just OK if it is well documented or printed when runs the test > that the test does not do anything about the previous area > intentionally. I could add solid fills of the 'empty' areas after each move() operation. Would that be more in line what you think the test should do? With best wishes, Tobias > > > BRs, > Hyungwon Hwang >
Hello Tobias, On Tue, 10 Nov 2015 14:24:11 +0100 Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > Hello Hyungwon, > > > Hyungwon Hwang wrote: > > Hello Tobias, > > > > On Mon, 09 Nov 2015 10:47:02 +0100 > > Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > > > >> 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 > > > > In that manner following glibc, I agree that the naming is > > reasonable. > well, that was just my way of thinking. But I guess most people have > experience using the libc, so the naming should look at least > 'familiar'. > > > > > I commented like that previously, because at the first time when I > > run the test, I think that the result seems like a bug. The test > > program said that it was a move test, but the operation seemed > > copying. > Ok, so just that I understand this correctly. Your issue is with the > commit the description of the test or with the commit description of > the patch that introduces g2d_move()? > > Because I don't see what you point out in the test commit description: > > " > tests/exynos: add test for g2d_move > > To check if g2d_move() works properly we create a small checkerboard > pattern in the center of the screen and then shift this pattern > around with g2d_move(). The pattern should be properly preserved > by the operation. > " > > I intentionally avoid to write "...move this pattern around...", so > instead I choose "shift". > > I'm not a native speaker, so I'm clueless how to formulate this in a > more clear way. I'm also not a native speaker, so maybe I could not figure out the subtle difference between move and shift. I just thought that 'shift' was just the synonym of 'move'. > > > > It > > would be just OK if it is well documented or printed when runs the > > test that the test does not do anything about the previous area > > intentionally. > I could add solid fills of the 'empty' areas after each move() > operation. Would that be more in line what you think the test should > do? No. Because g2d_move() is to g2d_copy() what memmove() is to memcpy(), the current implementation seems enough. What about change 'move' to 'copy' in the function description? * g2d_move - copy content inside single buffer. * Similar to 'memmove' this copies a rectangular region * of the provided buffer to another location (the source * and destination region potentially overlapping) Best regards, Hyungwon Hwang > > > With best wishes, > Tobias > > > > > > > > > > BRs, > > Hyungwon Hwang > > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-samsung-soc" in the body of a message to > majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
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,
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(+)