[PATCH 00/13] drm/exynos: async G2D and g2d_move()

[PATCH 00/13] drm/exynos: async G2D and g2d_move()

[Tobias Jakobi]

Hello,

this series mostly touches G2D code. It introduces the following:

(1) drmHandleEvent2() is added to enable processing of vendor-specific
    events. This will be used to expose asynchronous operation of the
    G2D. The necessary kernel infrastructure is already there since
    a lot of kernel versions. [This touches libdrm core code!]

(2) The necessary infrastructure to handle G2D events. This includes
    adding g2d_config_event() and g2d_exec2() to the public API.
    A test application is provided to ensure that everything works
    as expected.

(3) A small performance test application which can be used to measure
    the speed of solid color clear operations. Interesting for
    benchmarking and plotting colorful graphs (e.g. through
    Mathematica).

(4) g2d_move() which works similar to g2d_copy() but like the C
    memmove() properly handles overlapping buffer copies.
    Again a test application is present to check that this
    indeed does what it should.

(5) Various small changes. A framebuffer colorformat fix for the
    general G2D test application. Moving the currently unused
    g2d_reset() to the public API. Adding a counterpart to
    exynos_bo_map() to unmap buffers again.

(6) Last but not least a small bump of the Exynos version number.

Please review and let me know what I should change/improve.


With best wishes,
Tobias

P.S.: Most patches were submitted already some time ago but never
made it upstream. So if something looks familiar, don't worry! ;)

Tobias Jakobi (13):
  drm: Implement drmHandleEvent2()
  exynos: Introduce exynos_handle_event()
  tests/exynos: add fimg2d performance analysis
  exynos/fimg2d: add g2d_config_event
  exynos: fimg2d: add g2d_exec2
  tests/exynos: add fimg2d event test
  tests/exynos: use XRGB8888 for framebuffer
  exynos: fimg2d: add g2d_set_direction
  exynos/fimg2d: add g2d_move
  tests/exynos: add test for g2d_move
  exynos/fimg2d: add exynos_bo_unmap()
  exynos/fimg2d: add g2d_reset() to public API
  exynos: bump version number

 exynos/exynos-symbol-check         |   5 +
 exynos/exynos_drm.c                |  48 ++++++
 exynos/exynos_drm.h                |  12 ++
 exynos/exynos_drmif.h              |  27 +++
 exynos/exynos_fimg2d.c             | 164 +++++++++++++++++--
 exynos/exynos_fimg2d.h             |  49 ++++++
 exynos/libdrm_exynos.pc.in         |   2 +-
 tests/exynos/Makefile.am           |  26 ++-
 tests/exynos/exynos_fimg2d_event.c | 326 +++++++++++++++++++++++++++++++++++++
 tests/exynos/exynos_fimg2d_perf.c  | 320 ++++++++++++++++++++++++++++++++++++
 tests/exynos/exynos_fimg2d_test.c  | 134 ++++++++++++++-
 xf86drm.h                          |  21 +++
 xf86drmMode.c                      |  10 +-
 13 files changed, 1128 insertions(+), 16 deletions(-)
 create mode 100644 tests/exynos/exynos_fimg2d_event.c
 create mode 100644 tests/exynos/exynos_fimg2d_perf.c

-- 
2.0.5

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

[PATCH 01/13] drm: Implement drmHandleEvent2()

[Tobias Jakobi]

Basically this is an extended version of drmHandleEvent().

drmHandleEvent() only handles core events (like e.g. page flips),
but since kernel DRM drivers might use vendor-specific events
to signal userspace the completion of pending jobs, etc., its
desirable to provide a way to handle these without putting
vendor-specific code in the core libdrm.

To use this you provide drmHandleEvent2() with a function that
handles your non-core events.

The signature of that function looks like this:
void vendor(int fd, struct drm_event *e, void *ctx);

'fd' is the DRM file descriptor, 'e' the non-core event
and 'ctx' the event context (casted to void).

This way we don't have to maintain a copy of drmHandleEvent()
in the vendor code.

v2: Remove the opaque pointer, since this can be better
    handled with a container approach.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 xf86drm.h     | 21 +++++++++++++++++++++
 xf86drmMode.c | 10 +++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/xf86drm.h b/xf86drm.h
index 481d882..1e474a3 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -750,8 +750,29 @@ typedef struct _drmEventContext {
 
 } drmEventContext, *drmEventContextPtr;
 
+typedef void (*drmEventVendorHandler)(int fd, struct drm_event *e, void *ctx);
+
 extern int drmHandleEvent(int fd, drmEventContextPtr evctx);
 
+/*
+ * drmHandleEvent2() is an extended variant of drmHandleEvent() which
+ * allows handling of vendor-specific/non-core events.
+ * The function pointer 'vendorhandler' is used (if non-zero) to
+ * process non-core events. Users of have to prepare a container struct
+ * in the following way:
+ *
+ * struct vendor_event_context {
+ *		drmEventContext base;
+ *		int vendor_specific_data[num];
+ * };
+ *
+ * And then call:
+ * struct vendor_event_context ctx = {0};
+ * drmHandleEvent2(fd, &ctx.base, handler);
+ */
+extern int drmHandleEvent2(int fd, drmEventContextPtr evctx,
+	drmEventVendorHandler vendorhandler);
+
 extern char *drmGetDeviceNameFromFd(int fd);
 extern int drmGetNodeTypeFromFd(int fd);
 
diff --git a/xf86drmMode.c b/xf86drmMode.c
index ab6b519..89698da 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -867,7 +867,8 @@ int drmModeCrtcSetGamma(int fd, uint32_t crtc_id, uint32_t size,
 	return DRM_IOCTL(fd, DRM_IOCTL_MODE_SETGAMMA, &l);
 }
 
-int drmHandleEvent(int fd, drmEventContextPtr evctx)
+int drmHandleEvent2(int fd, drmEventContextPtr evctx,
+			drmEventVendorHandler vendorhandler)
 {
 	char buffer[1024];
 	int len, i;
@@ -910,6 +911,8 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 						 U642VOID (vblank->user_data));
 			break;
 		default:
+			if (vendorhandler)
+				vendorhandler(fd, e, evctx);
 			break;
 		}
 		i += e->length;
@@ -918,6 +921,11 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 	return 0;
 }
 
+int drmHandleEvent(int fd, drmEventContextPtr evctx)
+{
+	return drmHandleEvent2(fd, evctx, NULL);
+}
+
 int drmModePageFlip(int fd, uint32_t crtc_id, uint32_t fb_id,
 		    uint32_t flags, void *user_data)
 {
-- 
2.0.5

[PATCH 02/13] exynos: Introduce exynos_handle_event()

[Tobias Jakobi]

Used to handle kernel events specific to the Exynos platform.
Currently only G2D events are handled.

v2: Adapt to container approach.
v3: Add exynos_handle_event() to Exynos symbol test.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos-symbol-check |  1 +
 exynos/exynos_drm.c        | 28 ++++++++++++++++++++++++++++
 exynos/exynos_drm.h        | 12 ++++++++++++
 exynos/exynos_drmif.h      | 26 ++++++++++++++++++++++++++
 4 files changed, 67 insertions(+)

diff --git a/exynos/exynos-symbol-check b/exynos/exynos-symbol-check
index 1a1be89..c3ddbe4 100755
--- a/exynos/exynos-symbol-check
+++ b/exynos/exynos-symbol-check
@@ -22,6 +22,7 @@ exynos_device_destroy
 exynos_prime_fd_to_handle
 exynos_prime_handle_to_fd
 exynos_vidi_connection
+exynos_handle_event
 g2d_blend
 g2d_copy
 g2d_copy_with_scale
diff --git a/exynos/exynos_drm.c b/exynos/exynos_drm.c
index df9b8ed..7a400ad 100644
--- a/exynos/exynos_drm.c
+++ b/exynos/exynos_drm.c
@@ -42,6 +42,8 @@
 #include "exynos_drm.h"
 #include "exynos_drmif.h"
 
+#define U642VOID(x) ((void *)(unsigned long)(x))
+
 /*
  * Create exynos drm device object.
  *
@@ -374,3 +376,29 @@ exynos_vidi_connection(struct exynos_device *dev, uint32_t connect,
 
 	return 0;
 }
+
+static void
+exynos_handle_vendor(int fd, struct drm_event *e, void *ctx)
+{
+	struct drm_exynos_g2d_event *g2d;
+	struct exynos_event_context *ectx = ctx;
+
+	switch (e->type) {
+		case DRM_EXYNOS_G2D_EVENT:
+			if (ectx->version < 1 || ectx->g2d_event_handler == NULL)
+				break;
+			g2d = (struct drm_exynos_g2d_event *)e;
+			ectx->g2d_event_handler(fd, g2d->cmdlist_no, g2d->tv_sec,
+						g2d->tv_usec, U642VOID(g2d->user_data));
+			break;
+
+		default:
+			break;
+	}
+}
+
+int
+exynos_handle_event(struct exynos_device *dev, struct exynos_event_context *ctx)
+{
+	return drmHandleEvent2(dev->fd, &ctx->base, exynos_handle_vendor);
+}
diff --git a/exynos/exynos_drm.h b/exynos/exynos_drm.h
index 256c02f..c3af0ac 100644
--- a/exynos/exynos_drm.h
+++ b/exynos/exynos_drm.h
@@ -157,4 +157,16 @@ struct drm_exynos_g2d_exec {
 #define DRM_IOCTL_EXYNOS_G2D_EXEC		DRM_IOWR(DRM_COMMAND_BASE + \
 		DRM_EXYNOS_G2D_EXEC, struct drm_exynos_g2d_exec)
 
+/* EXYNOS specific events */
+#define DRM_EXYNOS_G2D_EVENT		0x80000000
+
+struct drm_exynos_g2d_event {
+	struct drm_event	base;
+	__u64				user_data;
+	__u32				tv_sec;
+	__u32				tv_usec;
+	__u32				cmdlist_no;
+	__u32				reserved;
+};
+
 #endif
diff --git a/exynos/exynos_drmif.h b/exynos/exynos_drmif.h
index c7c1d44..626e399 100644
--- a/exynos/exynos_drmif.h
+++ b/exynos/exynos_drmif.h
@@ -54,6 +54,25 @@ struct exynos_bo {
 	uint32_t		name;
 };
 
+#define EXYNOS_EVENT_CONTEXT_VERSION 1
+
+/*
+ * Exynos Event Context structure.
+ *
+ * @base: base context (for core events).
+ * @version: version info similar to the one in 'drmEventContext'.
+ * @g2d_event_handler: handler for G2D events.
+ */
+struct exynos_event_context {
+	drmEventContext base;
+
+	int version;
+
+	void (*g2d_event_handler)(int fd, unsigned int cmdlist_no,
+							  unsigned int tv_sec, unsigned int tv_usec,
+							  void *user_data);
+};
+
 /*
  * device related functions:
  */
@@ -83,4 +102,11 @@ int exynos_prime_fd_to_handle(struct exynos_device *dev, int fd,
 int exynos_vidi_connection(struct exynos_device *dev, uint32_t connect,
 				uint32_t ext, void *edid);
 
+/*
+ * event handling related functions:
+ */
+int exynos_handle_event(struct exynos_device *dev,
+				struct exynos_event_context *ctx);
+
+
 #endif /* EXYNOS_DRMIF_H_ */
-- 
2.0.5

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

[PATCH 03/13] tests/exynos: add fimg2d performance analysis

[Tobias Jakobi]

Currently only fast solid color clear performance is measured.
A large buffer is allocated and solid color clear operations
are executed on it with randomly chosen properties (position
and size of the region, clear color). Execution time is
measured and output together with the amount of pixels
processed.

The 'simple' variant only executes one G2D command buffer at
a time, while the 'multi' variant executes multiple ones. This
can be used to measure setup/exec overhead.

The test also serves a stability check. If clocks/voltages are
too high or low respectively, the test quickly reveals this.

v2: Add GPLv2 header, argument handling and documentation.
    Tool is only installed when requested.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/exynos/Makefile.am          |  19 ++-
 tests/exynos/exynos_fimg2d_perf.c | 320 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 337 insertions(+), 2 deletions(-)
 create mode 100644 tests/exynos/exynos_fimg2d_perf.c

diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
index b21d016..e82d199 100644
--- a/tests/exynos/Makefile.am
+++ b/tests/exynos/Makefile.am
@@ -5,16 +5,31 @@ AM_CFLAGS = \
 	-I $(top_srcdir)/exynos \
 	-I $(top_srcdir)
 
+bin_PROGRAMS =
+noinst_PROGRAMS =
+
 if HAVE_LIBKMS
 if HAVE_INSTALL_TESTS
-bin_PROGRAMS = \
+bin_PROGRAMS += \
 	exynos_fimg2d_test
 else
-noinst_PROGRAMS = \
+noinst_PROGRAMS += \
 	exynos_fimg2d_test
 endif
 endif
 
+if HAVE_INSTALL_TESTS
+bin_PROGRAMS += \
+	exynos_fimg2d_perf
+else
+noinst_PROGRAMS += \
+	exynos_fimg2d_perf
+endif
+
+exynos_fimg2d_perf_LDADD = \
+	$(top_builddir)/libdrm.la \
+	$(top_builddir)/exynos/libdrm_exynos.la
+
 exynos_fimg2d_test_LDADD = \
 	$(top_builddir)/libdrm.la \
 	$(top_builddir)/libkms/libkms.la \
diff --git a/tests/exynos/exynos_fimg2d_perf.c b/tests/exynos/exynos_fimg2d_perf.c
new file mode 100644
index 0000000..73d28ea
--- /dev/null
+++ b/tests/exynos/exynos_fimg2d_perf.c
@@ -0,0 +1,320 @@
+/*
+ * Copyright (C) 2015 - Tobias Jakobi
+ *
+ * This is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * It is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ * You should have received a copy of the GNU General Public License
+ * along with it. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <time.h>
+#include <getopt.h>
+
+#include <xf86drm.h>
+
+#include "exynos_drm.h"
+#include "exynos_drmif.h"
+#include "exynos_fimg2d.h"
+
+static int output_mathematica = 0;
+
+static int fimg2d_perf_simple(struct exynos_bo *bo, struct g2d_context *ctx,
+			unsigned buf_width, unsigned buf_height, unsigned iterations)
+{
+	struct timespec tspec = { 0 };
+	struct g2d_image img = { 0 };
+
+	unsigned long long g2d_time;
+	unsigned i;
+	int ret = 0;
+
+	img.width = buf_width;
+	img.height = buf_height;
+	img.stride = buf_width * 4;
+	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
+	img.buf_type = G2D_IMGBUF_GEM;
+	img.bo[0] = bo->handle;
+
+	srand(time(NULL));
+
+	printf("starting simple G2D performance test\n");
+	printf("buffer width = %u, buffer height = %u, iterations = %u\n",
+		buf_width, buf_height, iterations);
+
+	if (output_mathematica)
+		putchar('{');
+
+	for (i = 0; i < iterations; ++i) {
+		unsigned x, y, w, h;
+
+		x = rand() % buf_width;
+		y = rand() % buf_height;
+
+		if (x == (buf_width - 1))
+			x -= 1;
+		if (y == (buf_height - 1))
+			y -= 1;
+
+		w = rand() % (buf_width - x);
+		h = rand() % (buf_height - y);
+
+		if (w == 0) w = 1;
+		if (h == 0) h = 1;
+
+		img.color = rand();
+
+		ret = g2d_solid_fill(ctx, &img, x, y, w, h);
+
+		clock_gettime(CLOCK_MONOTONIC, &tspec);
+
+		if (ret == 0)
+			ret = g2d_exec(ctx);
+
+		if (ret != 0) {
+			fprintf(stderr, "error: iteration %u failed (x = %u, y = %u, w = %u, h = %u)\n",
+				i, x, y, w, h);
+			break;
+		} else {
+			struct timespec end = { 0 };
+			clock_gettime(CLOCK_MONOTONIC, &end);
+
+			g2d_time = (end.tv_sec - tspec.tv_sec) * 1000000000ULL;
+			g2d_time += (end.tv_nsec - tspec.tv_nsec);
+
+			if (output_mathematica) {
+				if (i != 0) putchar(',');
+				printf("{%u,%llu}", w * h, g2d_time);
+			} else {
+				printf("num_pixels = %u, usecs = %llu\n", w * h, g2d_time);
+			}
+		}
+	}
+
+	if (output_mathematica)
+		printf("}\n");
+
+	return ret;
+}
+
+static int fimg2d_perf_multi(struct exynos_bo *bo, struct g2d_context *ctx,
+			unsigned buf_width, unsigned buf_height, unsigned iterations, unsigned batch)
+{
+	struct timespec tspec = { 0 };
+	struct g2d_image *images;
+
+	unsigned long long g2d_time;
+	unsigned i, j;
+	int ret = 0;
+
+	images = calloc(batch, sizeof(struct g2d_image));
+	for (i = 0; i < batch; ++i) {
+		images[i].width = buf_width;
+		images[i].height = buf_height;
+		images[i].stride = buf_width * 4;
+		images[i].color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
+		images[i].buf_type = G2D_IMGBUF_GEM;
+		images[i].bo[0] = bo->handle;
+	}
+
+	srand(time(NULL));
+
+	printf("starting multi G2D performance test (batch size = %u)\n", batch);
+	printf("buffer width = %u, buffer height = %u, iterations = %u\n",
+		buf_width, buf_height, iterations);
+
+	if (output_mathematica)
+		putchar('{');
+
+	for (i = 0; i < iterations; ++i) {
+		unsigned num_pixels = 0;
+
+		for (j = 0; j < batch; ++j) {
+			unsigned x, y, w, h;
+
+			x = rand() % buf_width;
+			y = rand() % buf_height;
+
+			if (x == (buf_width - 1))
+				x -= 1;
+			if (y == (buf_height - 1))
+				y -= 1;
+
+			w = rand() % (buf_width - x);
+			h = rand() % (buf_height - y);
+
+			if (w == 0) w = 1;
+			if (h == 0) h = 1;
+
+			images[j].color = rand();
+
+			num_pixels += w * h;
+
+			ret = g2d_solid_fill(ctx, &images[j], x, y, w, h);
+			if (ret != 0)
+				break;
+		}
+
+		clock_gettime(CLOCK_MONOTONIC, &tspec);
+
+		if (ret == 0)
+			ret = g2d_exec(ctx);
+
+		if (ret != 0) {
+			fprintf(stderr, "error: iteration %u failed (num_pixels = %u)\n", i, num_pixels);
+			break;
+			break;
+		} else {
+			struct timespec end = { 0 };
+			clock_gettime(CLOCK_MONOTONIC, &end);
+
+			g2d_time = (end.tv_sec - tspec.tv_sec) * 1000000000ULL;
+			g2d_time += (end.tv_nsec - tspec.tv_nsec);
+
+			if (output_mathematica) {
+				if (i != 0) putchar(',');
+				printf("{%u,%llu}", num_pixels, g2d_time);
+			} else {
+				printf("num_pixels = %u, usecs = %llu\n", num_pixels, g2d_time);
+			}
+		}
+	}
+
+	if (output_mathematica)
+		printf("}\n");
+
+	return ret;
+}
+
+static void usage(const char *name)
+{
+	fprintf(stderr, "usage: %s [-ibwh]\n\n", name);
+
+	fprintf(stderr, "\t-i <number of iterations>\n");
+	fprintf(stderr, "\t-b <size of a batch> (default = 3)\n\n");
+
+	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
+	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n\n");
+
+	fprintf(stderr, "\t-M <enable Mathematica styled output>\n");
+
+	exit(0);
+}
+
+int main(int argc, char **argv)
+{
+	int fd, ret, c, parsefail;
+
+	struct exynos_device *dev;
+	struct g2d_context *ctx;
+	struct exynos_bo *bo;
+
+	unsigned int iters = 0, batch = 3;
+	unsigned int bufw = 4096, bufh = 4096;
+
+	ret = 0;
+	parsefail = 0;
+
+	while ((c = getopt(argc, argv, "i:b:w:h:M")) != -1) {
+		switch (c) {
+		case 'i':
+			if (sscanf(optarg, "%u", &iters) != 1)
+				parsefail = 1;
+			break;
+		case 'b':
+			if (sscanf(optarg, "%u", &batch) != 1)
+				parsefail = 1;
+			break;
+		case 'w':
+			if (sscanf(optarg, "%u", &bufw) != 1)
+				parsefail = 1;
+			break;
+		case 'h':
+			if (sscanf(optarg, "%u", &bufh) != 1)
+				parsefail = 1;
+			break;
+		case 'M':
+			output_mathematica = 1;
+			break;
+		default:
+			parsefail = 1;
+			break;
+		}
+	}
+
+	if (parsefail || (argc == 1) || (iters == 0))
+		usage(argv[0]);
+
+	if (bufw < 2 || bufw > 4096 || bufh < 2 || bufh > 4096) {
+		fprintf(stderr, "error: buffer width/height should be in the range 2 to 4096.\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	if (bufw == 0 || bufh == 0) {
+		fprintf(stderr, "error: buffer width/height should be non-zero.\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	fd = drmOpen("exynos", NULL);
+	if (fd < 0) {
+		fprintf(stderr, "error: failed to open drm\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	dev = exynos_device_create(fd);
+	if (dev == NULL) {
+		fprintf(stderr, "error: failed to create device\n");
+		ret = -2;
+
+		goto fail;
+	}
+
+	ctx = g2d_init(fd);
+	if (ctx == NULL) {
+		fprintf(stderr, "error: failed to init G2D\n");
+		ret = -3;
+
+		goto g2d_fail;
+	}
+
+	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
+	if (bo == NULL) {
+		fprintf(stderr, "error: failed to create bo\n");
+		ret = -4;
+
+		goto bo_fail;
+	}
+
+	ret = fimg2d_perf_simple(bo, ctx, bufw, bufh, iters);
+
+	if (ret == 0)
+		ret = fimg2d_perf_multi(bo, ctx, bufw, bufh, iters, batch);
+
+	exynos_bo_destroy(bo);
+
+bo_fail:
+	g2d_fini(ctx);
+
+g2d_fail:
+	exynos_device_destroy(dev);
+
+fail:
+	drmClose(fd);
+
+out:
+	return ret;
+}
-- 
2.0.5

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

[PATCH 04/13] exynos/fimg2d: add g2d_config_event

[Tobias Jakobi]

This enables us to pass command buffers to the kernel which
trigger an event on the DRM fd upon completion.
The final goal is to enable asynchronous operation of the
G2D engine, similar to async page flips.

Passing the event userdata pointer through the G2D context
was chosen to not change the current API (e.g. by adding
a userdata argument to each public functions).

v2: Add g2d_config_event() to Exynos symbol test.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos-symbol-check |  1 +
 exynos/exynos_fimg2d.c     | 28 ++++++++++++++++++++++++++--
 exynos/exynos_fimg2d.h     |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/exynos/exynos-symbol-check b/exynos/exynos-symbol-check
index c3ddbe4..bb12315 100755
--- a/exynos/exynos-symbol-check
+++ b/exynos/exynos-symbol-check
@@ -27,6 +27,7 @@ g2d_blend
 g2d_copy
 g2d_copy_with_scale
 g2d_exec
+g2d_config_event
 g2d_fini
 g2d_init
 g2d_scale_and_blend
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
index e734144..b10620b 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -57,6 +57,7 @@ struct g2d_context {
 	unsigned int			cmd_nr;
 	unsigned int			cmd_buf_nr;
 	unsigned int			cmdlist_nr;
+	void				*event_userdata;
 };
 
 enum g2d_base_addr_reg {
@@ -280,8 +281,15 @@ static int g2d_flush(struct g2d_context *ctx)
 	cmdlist.cmd_buf = (uint64_t)(uintptr_t)&ctx->cmd_buf[0];
 	cmdlist.cmd_nr = ctx->cmd_nr;
 	cmdlist.cmd_buf_nr = ctx->cmd_buf_nr;
-	cmdlist.event_type = G2D_EVENT_NOT;
-	cmdlist.user_data = 0;
+
+	if (ctx->event_userdata) {
+		cmdlist.event_type = G2D_EVENT_NONSTOP;
+		cmdlist.user_data = (uint64_t)(uintptr_t)(ctx->event_userdata);
+		ctx->event_userdata = NULL;
+	} else {
+		cmdlist.event_type = G2D_EVENT_NOT;
+		cmdlist.user_data = 0;
+	}
 
 	ctx->cmd_nr = 0;
 	ctx->cmd_buf_nr = 0;
@@ -336,6 +344,22 @@ void g2d_fini(struct g2d_context *ctx)
 }
 
 /**
+ * g2d_config_event - setup userdata configuration for a g2d event.
+ *		The next invocation of a g2d call (e.g. g2d_solid_fill) is
+ *		then going to flag the command buffer as 'nonstop'.
+ *		Completion of the command buffer execution can then be
+ *		determined by using drmHandleEvent on the DRM fd.
+ *		The userdata is 'consumed' in the process.
+ *
+ * @ctx: a pointer to g2d_context structure.
+ * @userdata: a pointer to the user data
+ */
+void g2d_config_event(struct g2d_context *ctx, void *userdata)
+{
+	ctx->event_userdata = userdata;
+}
+
+/**
  * g2d_exec - start the dma to process all commands summited by g2d_flush().
  *
  * @ctx: a pointer to g2d_context structure.
diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
index 4aa1568..a2c22da 100644
--- a/exynos/exynos_fimg2d.h
+++ b/exynos/exynos_fimg2d.h
@@ -290,6 +290,7 @@ struct g2d_context;
 
 struct g2d_context *g2d_init(int fd);
 void g2d_fini(struct g2d_context *ctx);
+void g2d_config_event(struct g2d_context *ctx, void *userdata);
 int g2d_exec(struct g2d_context *ctx);
 int g2d_solid_fill(struct g2d_context *ctx, struct g2d_image *img,
 			unsigned int x, unsigned int y, unsigned int w,
-- 
2.0.5

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

[PATCH 05/13] exynos: fimg2d: add g2d_exec2

[Tobias Jakobi]

This is a more 'flexible' version of g2d_exec allowing to pass
some flags which modify the behaviour of g2d_exec. Currently
only the 'async' operation flag is supported.

v2: Add g2d_exec2() to Exynos symbol test.
Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos-symbol-check |  1 +
 exynos/exynos_fimg2d.c     | 15 +++++++++++++--
 exynos/exynos_fimg2d.h     |  6 ++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/exynos/exynos-symbol-check b/exynos/exynos-symbol-check
index bb12315..2bb7718 100755
--- a/exynos/exynos-symbol-check
+++ b/exynos/exynos-symbol-check
@@ -27,6 +27,7 @@ g2d_blend
 g2d_copy
 g2d_copy_with_scale
 g2d_exec
+g2d_exec2
 g2d_config_event
 g2d_fini
 g2d_init
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
index b10620b..e997d4b 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -366,13 +366,24 @@ void g2d_config_event(struct g2d_context *ctx, void *userdata)
  */
 int g2d_exec(struct g2d_context *ctx)
 {
-	struct drm_exynos_g2d_exec exec;
+	return g2d_exec2(ctx, G2D_EXEC_FLAG_NORMAL);
+}
+
+/**
+ * g2d_exec2 - same as 'g2d_exec' but allow to pass some flags.
+ *
+ * @ctx: a pointer to g2d_context structure.
+ */
+int g2d_exec2(struct g2d_context *ctx, unsigned int flags)
+{
+	struct drm_exynos_g2d_exec exec = {0};
 	int ret;
 
 	if (ctx->cmdlist_nr == 0)
 		return -EINVAL;
 
-	exec.async = 0;
+	if (flags & G2D_EXEC_FLAG_ASYNC)
+		exec.async = 1;
 
 	ret = drmIoctl(ctx->fd, DRM_IOCTL_EXYNOS_G2D_EXEC, &exec);
 	if (ret < 0) {
diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
index a2c22da..c6b58ac 100644
--- a/exynos/exynos_fimg2d.h
+++ b/exynos/exynos_fimg2d.h
@@ -184,6 +184,11 @@ enum e_g2d_acoeff_mode {
 	G2D_ACOEFF_MODE_MASK
 };
 
+enum e_g2d_exec_flag {
+	G2D_EXEC_FLAG_NORMAL = (0 << 0),
+	G2D_EXEC_FLAG_ASYNC = (1 << 0)
+};
+
 union g2d_point_val {
 	unsigned int val;
 	struct {
@@ -292,6 +297,7 @@ struct g2d_context *g2d_init(int fd);
 void g2d_fini(struct g2d_context *ctx);
 void g2d_config_event(struct g2d_context *ctx, void *userdata);
 int g2d_exec(struct g2d_context *ctx);
+int g2d_exec2(struct g2d_context *ctx, unsigned int flags);
 int g2d_solid_fill(struct g2d_context *ctx, struct g2d_image *img,
 			unsigned int x, unsigned int y, unsigned int w,
 			unsigned int h);
-- 
2.0.5

[PATCH 06/13] tests/exynos: add fimg2d event test

[Tobias Jakobi]

This tests async processing of G2D jobs. A separate thread is spawned
to monitor the DRM fd for events and check whether a G2D job was
completed.

v2: Add GPLv2 header, argument handling and documentation.
    Test is only installed when requested.
v3: Allocate G2D jobs with calloc which fixes 'busy' being
    potentially uninitialized. Also enable timeout for poll()
    in the monitor thread. This fixes pthread_join() not working
    because of poll() not returning.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/exynos/Makefile.am           |  11 +-
 tests/exynos/exynos_fimg2d_event.c | 326 +++++++++++++++++++++++++++++++++++++
 2 files changed, 335 insertions(+), 2 deletions(-)
 create mode 100644 tests/exynos/exynos_fimg2d_event.c

diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
index e82d199..357d6b8 100644
--- a/tests/exynos/Makefile.am
+++ b/tests/exynos/Makefile.am
@@ -20,16 +20,23 @@ endif
 
 if HAVE_INSTALL_TESTS
 bin_PROGRAMS += \
-	exynos_fimg2d_perf
+	exynos_fimg2d_perf \
+	exynos_fimg2d_event
 else
 noinst_PROGRAMS += \
-	exynos_fimg2d_perf
+	exynos_fimg2d_perf \
+	exynos_fimg2d_event
 endif
 
 exynos_fimg2d_perf_LDADD = \
 	$(top_builddir)/libdrm.la \
 	$(top_builddir)/exynos/libdrm_exynos.la
 
+exynos_fimg2d_event_LDADD = \
+	$(top_builddir)/libdrm.la \
+	$(top_builddir)/exynos/libdrm_exynos.la \
+	-lpthread
+
 exynos_fimg2d_test_LDADD = \
 	$(top_builddir)/libdrm.la \
 	$(top_builddir)/libkms/libkms.la \
diff --git a/tests/exynos/exynos_fimg2d_event.c b/tests/exynos/exynos_fimg2d_event.c
new file mode 100644
index 0000000..c03dcff
--- /dev/null
+++ b/tests/exynos/exynos_fimg2d_event.c
@@ -0,0 +1,326 @@
+/*
+ * Copyright (C) 2015 - Tobias Jakobi
+ *
+ * This is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * It is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ * You should have received a copy of the GNU General Public License
+ * along with it. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <unistd.h>
+#include <poll.h>
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <time.h>
+#include <getopt.h>
+
+#include <pthread.h>
+
+#include <xf86drm.h>
+
+#include "exynos_drm.h"
+#include "exynos_drmif.h"
+#include "exynos_fimg2d.h"
+
+struct g2d_job {
+	unsigned int id;
+	unsigned int busy;
+};
+
+struct exynos_evhandler {
+	struct pollfd fds;
+	struct exynos_event_context evctx;
+};
+
+struct threaddata {
+	unsigned int stop;
+	struct exynos_device *dev;
+	struct exynos_evhandler evhandler;
+};
+
+static void g2d_event_handler(int fd, unsigned int cmdlist_no, unsigned int tv_sec,
+							unsigned int tv_usec, void *user_data)
+{
+	struct g2d_job *job = user_data;
+
+	fprintf(stderr, "info: g2d job (id = %u, cmdlist number = %u) finished!\n",
+			job->id, cmdlist_no);
+
+	job->busy = 0;
+}
+
+static void setup_g2d_event_handler(struct exynos_evhandler *evhandler, int fd)
+{
+	evhandler->fds.fd = fd;
+	evhandler->fds.events = POLLIN;
+	evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
+	evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
+	evhandler->evctx.g2d_event_handler = g2d_event_handler;
+}
+
+static void* threadfunc(void *arg) {
+	const int timeout = 0;
+	struct threaddata *data;
+
+	data = arg;
+
+	while (1) {
+		if (data->stop) break;
+
+		usleep(500);
+
+		data->evhandler.fds.revents = 0;
+
+		if (poll(&data->evhandler.fds, 1, timeout) < 0)
+			continue;
+
+		if (data->evhandler.fds.revents & (POLLHUP | POLLERR))
+			continue;
+
+		if (data->evhandler.fds.revents & POLLIN)
+			exynos_handle_event(data->dev, &data->evhandler.evctx);
+	}
+
+	pthread_exit(0);
+}
+
+/*
+ * We need to wait until all G2D jobs are finished, otherwise we
+ * potentially remove a BO which the engine still operates on.
+ * This results in the following kernel message:
+ * [drm:exynos_drm_gem_put_dma_addr] *ERROR* failed to lookup gem object.
+ * Also any subsequent BO allocations fail then with:
+ * [drm:exynos_drm_alloc_buf] *ERROR* failed to allocate buffer.
+ */
+static void wait_all_jobs(struct g2d_job* jobs, unsigned num_jobs)
+{
+	unsigned i;
+
+	for (i = 0; i < num_jobs; ++i) {
+		while (jobs[i].busy)
+			usleep(500);
+	}
+
+}
+
+static struct g2d_job* free_job(struct g2d_job* jobs, unsigned num_jobs)
+{
+	unsigned i;
+
+	for (i = 0; i < num_jobs; ++i) {
+		if (jobs[i].busy == 0)
+			return &jobs[i];
+	}
+
+	return NULL;
+}
+
+static int g2d_work(struct g2d_context *ctx, struct g2d_image *img,
+					unsigned num_jobs, unsigned iterations)
+{
+	struct g2d_job *jobs = calloc(num_jobs, sizeof(struct g2d_job));
+	int ret;
+	unsigned i;
+
+	/* setup job ids */
+	for (i = 0; i < num_jobs; ++i)
+		jobs[i].id = i;
+
+	for (i = 0; i < iterations; ++i) {
+		unsigned x, y, w, h;
+
+		struct g2d_job *j = NULL;
+
+		while (1) {
+			j = free_job(jobs, num_jobs);
+
+			if (j)
+				break;
+			else
+				usleep(500);
+		}
+
+		x = rand() % img->width;
+		y = rand() % img->height;
+
+		if (x == (img->width - 1))
+			x -= 1;
+		if (y == (img->height - 1))
+			y -= 1;
+
+		w = rand() % (img->width - x);
+		h = rand() % (img->height - y);
+
+		if (w == 0) w = 1;
+		if (h == 0) h = 1;
+
+		img->color = rand();
+
+		j->busy = 1;
+		g2d_config_event(ctx, j);
+
+		ret = g2d_solid_fill(ctx, img, x, y, w, h);
+
+		if (ret == 0)
+			g2d_exec2(ctx, G2D_EXEC_FLAG_ASYNC);
+
+		if (ret != 0) {
+			fprintf(stderr, "error: iteration %u (x = %u, x = %u, x = %u, x = %u) failed\n",
+					i, x, y, w, h);
+			break;
+		}
+	}
+
+	wait_all_jobs(jobs, num_jobs);
+	free(jobs);
+
+	return 0;
+}
+
+static void usage(const char *name)
+{
+	fprintf(stderr, "usage: %s [-ijwh]\n\n", name);
+
+	fprintf(stderr, "\t-i <number of iterations>\n");
+	fprintf(stderr, "\t-j <number of G2D jobs> (default = 4)\n\n");
+
+	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
+	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n");
+
+	exit(0);
+}
+
+int main(int argc, char **argv)
+{
+	int fd, ret, c, parsefail;
+
+	pthread_t event_thread;
+	struct threaddata event_data = {0};
+
+	struct exynos_device *dev;
+	struct g2d_context *ctx;
+	struct exynos_bo *bo;
+
+	struct g2d_image img = {0};
+
+	unsigned int iters = 0, njobs = 4;
+	unsigned int bufw = 4096, bufh = 4096;
+
+	ret = 0;
+	parsefail = 0;
+
+	while ((c = getopt(argc, argv, "i:j:w:h:")) != -1) {
+		switch (c) {
+		case 'i':
+			if (sscanf(optarg, "%u", &iters) != 1)
+				parsefail = 1;
+			break;
+		case 'j':
+			if (sscanf(optarg, "%u", &njobs) != 1)
+				parsefail = 1;
+			break;
+		case 'w':
+			if (sscanf(optarg, "%u", &bufw) != 1)
+				parsefail = 1;
+			break;
+		case 'h':
+			if (sscanf(optarg, "%u", &bufh) != 1)
+				parsefail = 1;
+			break;
+		default:
+			parsefail = 1;
+			break;
+		}
+	}
+
+	if (parsefail || (argc == 1) || (iters == 0))
+		usage(argv[0]);
+
+	if (bufw > 4096 || bufh > 4096) {
+		fprintf(stderr, "error: buffer width/height should be less than 4096.\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	if (bufw == 0 || bufh == 0) {
+		fprintf(stderr, "error: buffer width/height should be non-zero.\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	fd = drmOpen("exynos", NULL);
+	if (fd < 0) {
+		fprintf(stderr, "error: failed to open drm\n");
+		ret = -1;
+
+		goto out;
+	}
+
+	dev = exynos_device_create(fd);
+	if (dev == NULL) {
+		fprintf(stderr, "error: failed to create device\n");
+		ret = -2;
+
+		goto fail;
+	}
+
+	ctx = g2d_init(fd);
+	if (ctx == NULL) {
+		fprintf(stderr, "error: failed to init G2D\n");
+		ret = -3;
+
+		goto g2d_fail;
+	}
+
+	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
+	if (bo == NULL) {
+		fprintf(stderr, "error: failed to create bo\n");
+		ret = -4;
+
+		goto bo_fail;
+	}
+
+	/* setup g2d image object */
+	img.width = bufw;
+	img.height = bufh;
+	img.stride = bufw * 4;
+	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
+	img.buf_type = G2D_IMGBUF_GEM;
+	img.bo[0] = bo->handle;
+
+	event_data.dev = dev;
+	setup_g2d_event_handler(&event_data.evhandler, fd);
+
+	pthread_create(&event_thread, NULL, threadfunc, &event_data);
+
+	ret = g2d_work(ctx, &img, njobs, iters);
+	if (ret != 0)
+		fprintf(stderr, "error: g2d_work failed\n");
+
+	event_data.stop = 1;
+	pthread_join(event_thread, NULL);
+
+	exynos_bo_destroy(bo);
+
+bo_fail:
+	g2d_fini(ctx);
+
+g2d_fail:
+	exynos_device_destroy(dev);
+
+fail:
+	drmClose(fd);
+
+out:
+	return ret;
+}
-- 
2.0.5

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

[PATCH 07/13] tests/exynos: use XRGB8888 for framebuffer

[Tobias Jakobi]

This matches the G2D color mode that is used in the entire code.
The previous (incorrect) RGBA8888 would only work since the
Exynos mixer did its configuration based on the bpp, and not
based on the actual pixelformat.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/exynos/exynos_fimg2d_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
index 8794dac..dfb00a0 100644
--- a/tests/exynos/exynos_fimg2d_test.c
+++ b/tests/exynos/exynos_fimg2d_test.c
@@ -675,7 +675,7 @@ int main(int argc, char **argv)
 	offsets[0] = 0;
 
 	ret = drmModeAddFB2(dev->fd, screen_width, screen_height,
-				DRM_FORMAT_RGBA8888, handles,
+				DRM_FORMAT_XRGB8888, handles,
 				pitches, offsets, &fb_id, 0);
 	if (ret < 0)
 		goto err_destroy_buffer;
-- 
2.0.5

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

[PATCH 08/13] exynos: fimg2d: add g2d_set_direction

[Tobias Jakobi]

This allows setting the two direction registers, which specify how
the engine blits pixels. This can be used for overlapping blits,
which happen e.g. when 'moving' a rectangular region inside a
fixed buffer.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos_fimg2d.c | 13 +++++++++++++
 exynos/exynos_fimg2d.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
index e997d4b..4d5419c 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct g2d_context *ctx, struct g2d_image *img,
 }
 
 /*
+ * g2d_set_direction - setup direction register (useful for overlapping blits).
+ *
+ * @ctx: a pointer to g2d_context structure.
+ * @dir: a pointer to the g2d_direction_val structure.
+ */
+static void g2d_set_direction(struct g2d_context *ctx,
+			const union g2d_direction_val *dir)
+{
+	g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
+	g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
+}
+
+/*
  * g2d_reset - reset fimg2d hardware.
  *
  * @ctx: a pointer to g2d_context structure.
diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
index c6b58ac..9eee7c0 100644
--- a/exynos/exynos_fimg2d.h
+++ b/exynos/exynos_fimg2d.h
@@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
 	G2D_EXEC_FLAG_ASYNC = (1 << 0)
 };
 
+enum e_g2d_dir_mode {
+	G2D_DIR_MODE_POSITIVE = 0,
+	G2D_DIR_MODE_NEGATIVE = 1
+};
+
 union g2d_point_val {
 	unsigned int val;
 	struct {
@@ -269,6 +274,39 @@ union g2d_blend_func_val {
 	} data;
 };
 
+union g2d_direction_val {
+	unsigned int val[2];
+	struct {
+		/* SRC_MSK_DIRECT_REG [0:1] (source) */
+		enum e_g2d_dir_mode		src_x_direction:1;
+		enum e_g2d_dir_mode		src_y_direction:1;
+
+		/* SRC_MSK_DIRECT_REG [2:3] */
+		unsigned int			reversed1:2;
+
+		/* SRC_MSK_DIRECT_REG [4:5] (mask) */
+		enum e_g2d_dir_mode		mask_x_direction:1;
+		enum e_g2d_dir_mode		mask_y_direction:1;
+
+		/* SRC_MSK_DIRECT_REG [6:31] */
+		unsigned int			padding1:26;
+
+		/* DST_PAT_DIRECT_REG [0:1] (destination) */
+		enum e_g2d_dir_mode		dst_x_direction:1;
+		enum e_g2d_dir_mode		dst_y_direction:1;
+
+		/* DST_PAT_DIRECT_REG [2:3] */
+		unsigned int			reversed2:2;
+
+		/* DST_PAT_DIRECT_REG [4:5] (pattern) */
+		enum e_g2d_dir_mode		pat_x_direction:1;
+		enum e_g2d_dir_mode		pat_y_direction:1;
+
+		/* DST_PAT_DIRECT_REG [6:31] */
+		unsigned int			padding2:26;
+	} data;
+};
+
 struct g2d_image {
 	enum e_g2d_select_mode		select_mode;
 	enum e_g2d_color_mode		color_mode;
-- 
2.0.5

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

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

[Tobias Jakobi]

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,
-- 
2.0.5

[PATCH 10/13] tests/exynos: add test for g2d_move

[Tobias Jakobi]

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.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/exynos/exynos_fimg2d_test.c | 132 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
index dfb00a0..797fb6e 100644
--- a/tests/exynos/exynos_fimg2d_test.c
+++ b/tests/exynos/exynos_fimg2d_test.c
@@ -313,6 +313,130 @@ fail:
 	return ret;
 }
 
+static int g2d_move_test(struct exynos_device *dev,
+				struct exynos_bo *tmp,
+				struct exynos_bo *buf,
+				enum e_g2d_buf_type type)
+{
+	struct g2d_context *ctx;
+	struct g2d_image img = {0}, tmp_img = {0};
+	unsigned int img_w, img_h, count;
+	int cur_x, cur_y;
+	void *checkerboard;
+	int ret;
+
+	static const struct g2d_step {
+		int x, y;
+	} steps[] = {
+		{ 1,  0}, { 0,  1},
+		{-1,  0}, { 0, -1},
+		{ 1,  1}, {-1, -1},
+		{ 1, -1}, {-1,  1},
+		{ 2,  1}, { 1,  2},
+		{-2, -1}, {-1, -2},
+		{ 2, -1}, { 1, -2},
+		{-2,  1}, {-1,  2}
+	};
+	static const unsigned int num_steps =
+		sizeof(steps) / sizeof(struct g2d_step);
+
+	ctx = g2d_init(dev->fd);
+	if (!ctx)
+		return -EFAULT;
+
+	img.bo[0] = buf->handle;
+
+	/* create pattern of half the screen size */
+	checkerboard = create_checkerboard_pattern(screen_width / 64, screen_height / 64, 32);
+	if (!checkerboard) {
+		ret = -EFAULT;
+		goto fail;
+	}
+
+	img_w = (screen_width / 64) * 32;
+	img_h = (screen_height / 64) * 32;
+
+	switch (type) {
+	case G2D_IMGBUF_GEM:
+		memcpy(tmp->vaddr, checkerboard, img_w * img_h * 4);
+		tmp_img.bo[0] = tmp->handle;
+		break;
+	case G2D_IMGBUF_USERPTR:
+		tmp_img.user_ptr[0].userptr = (unsigned long)checkerboard;
+		tmp_img.user_ptr[0].size = img_w * img_h * 4;
+		break;
+	case G2D_IMGBUF_COLOR:
+	default:
+		ret = -EFAULT;
+		goto fail;
+	}
+
+	/* solid fill framebuffer with white color */
+	img.width = screen_width;
+	img.height = screen_height;
+	img.stride = screen_width * 4;
+	img.buf_type = G2D_IMGBUF_GEM;
+	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
+	img.color = 0xffffffff;
+
+	/* put checkerboard pattern in the center of the framebuffer */
+	cur_x = (screen_width - img_w) / 2;
+	cur_y = (screen_height - img_h) / 2;
+	tmp_img.width = img_w;
+	tmp_img.height = img_h;
+	tmp_img.stride = img_w * 4;
+	tmp_img.buf_type = type;
+	tmp_img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
+
+	ret = g2d_solid_fill(ctx, &img, 0, 0, screen_width, screen_height) ||
+		g2d_copy(ctx, &tmp_img, &img, 0, 0, cur_x, cur_y, img_w, img_h);
+
+	if (!ret)
+		ret = g2d_exec(ctx);
+	if (ret < 0)
+			goto fail;
+
+	printf("move test with %s.\n",
+			type == G2D_IMGBUF_GEM ? "gem" : "userptr");
+
+	srand(time(NULL));
+	for (count = 0; count < 256; ++count) {
+		const struct g2d_step *s;
+
+		/* select step and validate it */
+		while (1) {
+			s = &steps[random() % num_steps];
+
+			if (cur_x + s->x < 0 || cur_y + s->y < 0 ||
+				cur_x + img_w + s->x >= screen_width ||
+				cur_y + img_h + s->y >= screen_height)
+				continue;
+			else
+				break;
+		}
+
+		ret = g2d_move(ctx, &img, cur_x, cur_y, cur_x + s->x, cur_y + s->y,
+			img_w, img_h);
+		if (!ret)
+			ret = g2d_exec(ctx);
+
+		if (ret < 0)
+			goto fail;
+
+		cur_x += s->x;
+		cur_y += s->y;
+
+		usleep(100000);
+	}
+
+fail:
+	g2d_fini(ctx);
+
+	free(checkerboard);
+
+	return ret;
+}
+
 static int g2d_copy_with_scale_test(struct exynos_device *dev,
 					struct exynos_bo *src,
 					struct exynos_bo *dst,
@@ -708,6 +832,14 @@ int main(int argc, char **argv)
 
 	wait_for_user_input(0);
 
+	ret = g2d_move_test(dev, src, bo, G2D_IMGBUF_GEM);
+	if (ret < 0) {
+		fprintf(stderr, "failed to test move operation.\n");
+		goto err_free_src;
+	}
+
+	wait_for_user_input(0);
+
 	ret = g2d_copy_with_scale_test(dev, src, bo, G2D_IMGBUF_GEM);
 	if (ret < 0) {
 		fprintf(stderr, "failed to test copy and scale operation.\n");
-- 
2.0.5

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

[PATCH 11/13] exynos/fimg2d: add exynos_bo_unmap()

[Tobias Jakobi]

This unmaps a previously mapped (via exynos_bo_map())
buffer object.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos-symbol-check |  1 +
 exynos/exynos_drm.c        | 20 ++++++++++++++++++++
 exynos/exynos_drmif.h      |  1 +
 3 files changed, 22 insertions(+)

diff --git a/exynos/exynos-symbol-check b/exynos/exynos-symbol-check
index 2bb7718..2e4ba56 100755
--- a/exynos/exynos-symbol-check
+++ b/exynos/exynos-symbol-check
@@ -17,6 +17,7 @@ exynos_bo_get_info
 exynos_bo_get_name
 exynos_bo_handle
 exynos_bo_map
+exynos_bo_unmap
 exynos_device_create
 exynos_device_destroy
 exynos_prime_fd_to_handle
diff --git a/exynos/exynos_drm.c b/exynos/exynos_drm.c
index 7a400ad..e086a00 100644
--- a/exynos/exynos_drm.c
+++ b/exynos/exynos_drm.c
@@ -309,6 +309,26 @@ void *exynos_bo_map(struct exynos_bo *bo)
 	return bo->vaddr;
 }
 
+int exynos_bo_unmap(struct exynos_bo *bo)
+{
+	int ret = 0;
+
+	if (!bo->vaddr)
+		goto out;
+
+	ret = munmap(bo->vaddr, bo->size);
+	if (ret) {
+		fprintf(stderr, "failed to unmap buffer [%s].\n",
+			strerror(errno));
+		goto out;
+	}
+
+	bo->vaddr = NULL;
+
+out:
+	return ret;
+}
+
 /*
  * Export gem object to dmabuf as file descriptor.
  *
diff --git a/exynos/exynos_drmif.h b/exynos/exynos_drmif.h
index 626e399..3b26ebf 100644
--- a/exynos/exynos_drmif.h
+++ b/exynos/exynos_drmif.h
@@ -91,6 +91,7 @@ struct exynos_bo * exynos_bo_from_name(struct exynos_device *dev, uint32_t name)
 int exynos_bo_get_name(struct exynos_bo *bo, uint32_t *name);
 uint32_t exynos_bo_handle(struct exynos_bo *bo);
 void * exynos_bo_map(struct exynos_bo *bo);
+int exynos_bo_unmap(struct exynos_bo *bo);
 int exynos_prime_handle_to_fd(struct exynos_device *dev, uint32_t handle,
 					int *fd);
 int exynos_prime_fd_to_handle(struct exynos_device *dev, int fd,
-- 
2.0.5

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

[PATCH 12/13] exynos/fimg2d: add g2d_reset() to public API

[Tobias Jakobi]

After the rewrite of the command buffer submission
handling g2d_reset() is no longer called internally.

Still the user might want to reset the G2D
context so expose this call.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos-symbol-check |  1 +
 exynos/exynos_fimg2d.c     | 28 ++++++++++++++--------------
 exynos/exynos_fimg2d.h     |  1 +
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/exynos/exynos-symbol-check b/exynos/exynos-symbol-check
index 2e4ba56..57dd684 100755
--- a/exynos/exynos-symbol-check
+++ b/exynos/exynos-symbol-check
@@ -32,6 +32,7 @@ g2d_exec2
 g2d_config_event
 g2d_fini
 g2d_init
+g2d_reset
 g2d_scale_and_blend
 g2d_solid_fill
 EOF
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
index 8703629..cc5d9f4 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -255,20 +255,6 @@ static void g2d_set_direction(struct g2d_context *ctx,
 }
 
 /*
- * g2d_reset - reset fimg2d hardware.
- *
- * @ctx: a pointer to g2d_context structure.
- *
- */
-static void g2d_reset(struct g2d_context *ctx)
-{
-	ctx->cmd_nr = 0;
-	ctx->cmd_buf_nr = 0;
-
-	g2d_add_cmd(ctx, SOFT_RESET_REG, 0x01);
-}
-
-/*
  * g2d_flush - submit all commands and values in user side command buffer
  *		to command queue aware of fimg2d dma.
  *
@@ -356,6 +342,20 @@ void g2d_fini(struct g2d_context *ctx)
 	free(ctx);
 }
 
+/*
+ * g2d_reset - reset fimg2d hardware.
+ *
+ * @ctx: a pointer to g2d_context structure.
+ *
+ */
+void g2d_reset(struct g2d_context *ctx)
+{
+	ctx->cmd_nr = 0;
+	ctx->cmd_buf_nr = 0;
+
+	g2d_add_cmd(ctx, SOFT_RESET_REG, 0x01);
+}
+
 /**
  * g2d_config_event - setup userdata configuration for a g2d event.
  *		The next invocation of a g2d call (e.g. g2d_solid_fill) is
diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
index 2700686..03391c1 100644
--- a/exynos/exynos_fimg2d.h
+++ b/exynos/exynos_fimg2d.h
@@ -333,6 +333,7 @@ struct g2d_context;
 
 struct g2d_context *g2d_init(int fd);
 void g2d_fini(struct g2d_context *ctx);
+void g2d_reset(struct g2d_context *ctx);
 void g2d_config_event(struct g2d_context *ctx, void *userdata);
 int g2d_exec(struct g2d_context *ctx);
 int g2d_exec2(struct g2d_context *ctx, unsigned int flags);
-- 
2.0.5

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

[PATCH 13/13] exynos: bump version number

[Tobias Jakobi]

The Exynos API was extended quite a bit, so reflect this in the
version number.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/libdrm_exynos.pc.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exynos/libdrm_exynos.pc.in b/exynos/libdrm_exynos.pc.in
index 5ce9118..ff1c432 100644
--- a/exynos/libdrm_exynos.pc.in
+++ b/exynos/libdrm_exynos.pc.in
@@ -5,7 +5,7 @@ includedir=@includedir@
 
 Name: libdrm_exynos
 Description: Userspace interface to exynos kernel DRM services
-Version: 0.6
+Version: 0.7
 Libs: -L${libdir} -ldrm_exynos
 Cflags: -I${includedir} -I${includedir}/libdrm -I${includedir}/exynos
 Requires.private: libdrm
-- 
2.0.5

Re: [PATCH 00/13] drm/exynos: async G2D and g2d_move()

[Tobias Jakobi]

Gentle ping! :-)

- Tobias


Tobias Jakobi wrote:
> Hello,
> 
> this series mostly touches G2D code. It introduces the following:
> 
> (1) drmHandleEvent2() is added to enable processing of vendor-specific
>     events. This will be used to expose asynchronous operation of the
>     G2D. The necessary kernel infrastructure is already there since
>     a lot of kernel versions. [This touches libdrm core code!]
> 
> (2) The necessary infrastructure to handle G2D events. This includes
>     adding g2d_config_event() and g2d_exec2() to the public API.
>     A test application is provided to ensure that everything works
>     as expected.
> 
> (3) A small performance test application which can be used to measure
>     the speed of solid color clear operations. Interesting for
>     benchmarking and plotting colorful graphs (e.g. through
>     Mathematica).
> 
> (4) g2d_move() which works similar to g2d_copy() but like the C
>     memmove() properly handles overlapping buffer copies.
>     Again a test application is present to check that this
>     indeed does what it should.
> 
> (5) Various small changes. A framebuffer colorformat fix for the
>     general G2D test application. Moving the currently unused
>     g2d_reset() to the public API. Adding a counterpart to
>     exynos_bo_map() to unmap buffers again.
> 
> (6) Last but not least a small bump of the Exynos version number.
> 
> Please review and let me know what I should change/improve.
> 
> 
> With best wishes,
> Tobias
> 
> P.S.: Most patches were submitted already some time ago but never
> made it upstream. So if something looks familiar, don't worry! ;)
> 
> Tobias Jakobi (13):
>   drm: Implement drmHandleEvent2()
>   exynos: Introduce exynos_handle_event()
>   tests/exynos: add fimg2d performance analysis
>   exynos/fimg2d: add g2d_config_event
>   exynos: fimg2d: add g2d_exec2
>   tests/exynos: add fimg2d event test
>   tests/exynos: use XRGB8888 for framebuffer
>   exynos: fimg2d: add g2d_set_direction
>   exynos/fimg2d: add g2d_move
>   tests/exynos: add test for g2d_move
>   exynos/fimg2d: add exynos_bo_unmap()
>   exynos/fimg2d: add g2d_reset() to public API
>   exynos: bump version number
> 
>  exynos/exynos-symbol-check         |   5 +
>  exynos/exynos_drm.c                |  48 ++++++
>  exynos/exynos_drm.h                |  12 ++
>  exynos/exynos_drmif.h              |  27 +++
>  exynos/exynos_fimg2d.c             | 164 +++++++++++++++++--
>  exynos/exynos_fimg2d.h             |  49 ++++++
>  exynos/libdrm_exynos.pc.in         |   2 +-
>  tests/exynos/Makefile.am           |  26 ++-
>  tests/exynos/exynos_fimg2d_event.c | 326 +++++++++++++++++++++++++++++++++++++
>  tests/exynos/exynos_fimg2d_perf.c  | 320 ++++++++++++++++++++++++++++++++++++
>  tests/exynos/exynos_fimg2d_test.c  | 134 ++++++++++++++-
>  xf86drm.h                          |  21 +++
>  xf86drmMode.c                      |  10 +-
>  13 files changed, 1128 insertions(+), 16 deletions(-)
>  create mode 100644 tests/exynos/exynos_fimg2d_event.c
>  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
> 

Re: [PATCH 00/13] drm/exynos: async G2D and g2d_move()

[Tobias Jakobi]

Another ping!

I need someone from Samsung to review this, or at least someone familiar
with the G2D.

With best wishes,
Tobias


Tobias Jakobi wrote:
> Gentle ping! :-)
> 
> - Tobias
> 
> 
> Tobias Jakobi wrote:
>> Hello,
>>
>> this series mostly touches G2D code. It introduces the following:
>>
>> (1) drmHandleEvent2() is added to enable processing of vendor-specific
>>     events. This will be used to expose asynchronous operation of the
>>     G2D. The necessary kernel infrastructure is already there since
>>     a lot of kernel versions. [This touches libdrm core code!]
>>
>> (2) The necessary infrastructure to handle G2D events. This includes
>>     adding g2d_config_event() and g2d_exec2() to the public API.
>>     A test application is provided to ensure that everything works
>>     as expected.
>>
>> (3) A small performance test application which can be used to measure
>>     the speed of solid color clear operations. Interesting for
>>     benchmarking and plotting colorful graphs (e.g. through
>>     Mathematica).
>>
>> (4) g2d_move() which works similar to g2d_copy() but like the C
>>     memmove() properly handles overlapping buffer copies.
>>     Again a test application is present to check that this
>>     indeed does what it should.
>>
>> (5) Various small changes. A framebuffer colorformat fix for the
>>     general G2D test application. Moving the currently unused
>>     g2d_reset() to the public API. Adding a counterpart to
>>     exynos_bo_map() to unmap buffers again.
>>
>> (6) Last but not least a small bump of the Exynos version number.
>>
>> Please review and let me know what I should change/improve.
>>
>>
>> With best wishes,
>> Tobias
>>
>> P.S.: Most patches were submitted already some time ago but never
>> made it upstream. So if something looks familiar, don't worry! ;)
>>
>> Tobias Jakobi (13):
>>   drm: Implement drmHandleEvent2()
>>   exynos: Introduce exynos_handle_event()
>>   tests/exynos: add fimg2d performance analysis
>>   exynos/fimg2d: add g2d_config_event
>>   exynos: fimg2d: add g2d_exec2
>>   tests/exynos: add fimg2d event test
>>   tests/exynos: use XRGB8888 for framebuffer
>>   exynos: fimg2d: add g2d_set_direction
>>   exynos/fimg2d: add g2d_move
>>   tests/exynos: add test for g2d_move
>>   exynos/fimg2d: add exynos_bo_unmap()
>>   exynos/fimg2d: add g2d_reset() to public API
>>   exynos: bump version number
>>
>>  exynos/exynos-symbol-check         |   5 +
>>  exynos/exynos_drm.c                |  48 ++++++
>>  exynos/exynos_drm.h                |  12 ++
>>  exynos/exynos_drmif.h              |  27 +++
>>  exynos/exynos_fimg2d.c             | 164 +++++++++++++++++--
>>  exynos/exynos_fimg2d.h             |  49 ++++++
>>  exynos/libdrm_exynos.pc.in         |   2 +-
>>  tests/exynos/Makefile.am           |  26 ++-
>>  tests/exynos/exynos_fimg2d_event.c | 326 +++++++++++++++++++++++++++++++++++++
>>  tests/exynos/exynos_fimg2d_perf.c  | 320 ++++++++++++++++++++++++++++++++++++
>>  tests/exynos/exynos_fimg2d_test.c  | 134 ++++++++++++++-
>>  xf86drm.h                          |  21 +++
>>  xf86drmMode.c                      |  10 +-
>>  13 files changed, 1128 insertions(+), 16 deletions(-)
>>  create mode 100644 tests/exynos/exynos_fimg2d_event.c
>>  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
>>
> 

Re: [PATCH 00/13] drm/exynos: async G2D and g2d_move()

[Tobias Jakobi]

Another ping!

With best wishes,
Tobias


Tobias Jakobi wrote:
> Hello,
> 
> this series mostly touches G2D code. It introduces the following:
> 
> (1) drmHandleEvent2() is added to enable processing of vendor-specific
>     events. This will be used to expose asynchronous operation of the
>     G2D. The necessary kernel infrastructure is already there since
>     a lot of kernel versions. [This touches libdrm core code!]
> 
> (2) The necessary infrastructure to handle G2D events. This includes
>     adding g2d_config_event() and g2d_exec2() to the public API.
>     A test application is provided to ensure that everything works
>     as expected.
> 
> (3) A small performance test application which can be used to measure
>     the speed of solid color clear operations. Interesting for
>     benchmarking and plotting colorful graphs (e.g. through
>     Mathematica).
> 
> (4) g2d_move() which works similar to g2d_copy() but like the C
>     memmove() properly handles overlapping buffer copies.
>     Again a test application is present to check that this
>     indeed does what it should.
> 
> (5) Various small changes. A framebuffer colorformat fix for the
>     general G2D test application. Moving the currently unused
>     g2d_reset() to the public API. Adding a counterpart to
>     exynos_bo_map() to unmap buffers again.
> 
> (6) Last but not least a small bump of the Exynos version number.
> 
> Please review and let me know what I should change/improve.
> 
> 
> With best wishes,
> Tobias
> 
> P.S.: Most patches were submitted already some time ago but never
> made it upstream. So if something looks familiar, don't worry! ;)
> 
> Tobias Jakobi (13):
>   drm: Implement drmHandleEvent2()
>   exynos: Introduce exynos_handle_event()
>   tests/exynos: add fimg2d performance analysis
>   exynos/fimg2d: add g2d_config_event
>   exynos: fimg2d: add g2d_exec2
>   tests/exynos: add fimg2d event test
>   tests/exynos: use XRGB8888 for framebuffer
>   exynos: fimg2d: add g2d_set_direction
>   exynos/fimg2d: add g2d_move
>   tests/exynos: add test for g2d_move
>   exynos/fimg2d: add exynos_bo_unmap()
>   exynos/fimg2d: add g2d_reset() to public API
>   exynos: bump version number
> 
>  exynos/exynos-symbol-check         |   5 +
>  exynos/exynos_drm.c                |  48 ++++++
>  exynos/exynos_drm.h                |  12 ++
>  exynos/exynos_drmif.h              |  27 +++
>  exynos/exynos_fimg2d.c             | 164 +++++++++++++++++--
>  exynos/exynos_fimg2d.h             |  49 ++++++
>  exynos/libdrm_exynos.pc.in         |   2 +-
>  tests/exynos/Makefile.am           |  26 ++-
>  tests/exynos/exynos_fimg2d_event.c | 326 +++++++++++++++++++++++++++++++++++++
>  tests/exynos/exynos_fimg2d_perf.c  | 320 ++++++++++++++++++++++++++++++++++++
>  tests/exynos/exynos_fimg2d_test.c  | 134 ++++++++++++++-
>  xf86drm.h                          |  21 +++
>  xf86drmMode.c                      |  10 +-
>  13 files changed, 1128 insertions(+), 16 deletions(-)
>  create mode 100644 tests/exynos/exynos_fimg2d_event.c
>  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
> 

Re: [PATCH 07/13] tests/exynos: use XRGB8888 for framebuffer

[Hyungwon Hwang]

On Tue, 22 Sep 2015 17:54:56 +0200
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> This matches the G2D color mode that is used in the entire code.
> The previous (incorrect) RGBA8888 would only work since the
> Exynos mixer did its configuration based on the bpp, and not
> based on the actual pixelformat.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  tests/exynos/exynos_fimg2d_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/exynos/exynos_fimg2d_test.c
> b/tests/exynos/exynos_fimg2d_test.c index 8794dac..dfb00a0 100644
> --- a/tests/exynos/exynos_fimg2d_test.c
> +++ b/tests/exynos/exynos_fimg2d_test.c
> @@ -675,7 +675,7 @@ int main(int argc, char **argv)
>  	offsets[0] = 0;
>  
>  	ret = drmModeAddFB2(dev->fd, screen_width, screen_height,
> -				DRM_FORMAT_RGBA8888, handles,
> +				DRM_FORMAT_XRGB8888, handles,
>  				pitches, offsets, &fb_id, 0);

Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>

Nice catch. It's right, if there was no previous setting for source
image color mode. But I think it could be the source image color mode
was set by another application before when this test runs. So I think
the code which sets the source image color mode must be added.

Best regards,
Hyungwon Hwang


>  	if (ret < 0)
>  		goto err_destroy_buffer;

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Hyungwon Hwang]

On Tue, 22 Sep 2015 17:54:55 +0200
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> This tests async processing of G2D jobs. A separate thread is spawned
> to monitor the DRM fd for events and check whether a G2D job was
> completed.
> 
> v2: Add GPLv2 header, argument handling and documentation.
>     Test is only installed when requested.
> v3: Allocate G2D jobs with calloc which fixes 'busy' being
>     potentially uninitialized. Also enable timeout for poll()
>     in the monitor thread. This fixes pthread_join() not working
>     because of poll() not returning.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  tests/exynos/Makefile.am           |  11 +-
>  tests/exynos/exynos_fimg2d_event.c | 326
> +++++++++++++++++++++++++++++++++++++ 2 files changed, 335
> insertions(+), 2 deletions(-) create mode 100644
> tests/exynos/exynos_fimg2d_event.c
> 
> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
> index e82d199..357d6b8 100644
> --- a/tests/exynos/Makefile.am
> +++ b/tests/exynos/Makefile.am
> @@ -20,16 +20,23 @@ endif
>  
>  if HAVE_INSTALL_TESTS
>  bin_PROGRAMS += \
> -	exynos_fimg2d_perf
> +	exynos_fimg2d_perf \
> +	exynos_fimg2d_event
>  else
>  noinst_PROGRAMS += \
> -	exynos_fimg2d_perf
> +	exynos_fimg2d_perf \
> +	exynos_fimg2d_event
>  endif
>  
>  exynos_fimg2d_perf_LDADD = \
>  	$(top_builddir)/libdrm.la \
>  	$(top_builddir)/exynos/libdrm_exynos.la
>  
> +exynos_fimg2d_event_LDADD = \
> +	$(top_builddir)/libdrm.la \
> +	$(top_builddir)/exynos/libdrm_exynos.la \
> +	-lpthread
> +
>  exynos_fimg2d_test_LDADD = \
>  	$(top_builddir)/libdrm.la \
>  	$(top_builddir)/libkms/libkms.la \
> diff --git a/tests/exynos/exynos_fimg2d_event.c
> b/tests/exynos/exynos_fimg2d_event.c new file mode 100644
> index 0000000..c03dcff
> --- /dev/null
> +++ b/tests/exynos/exynos_fimg2d_event.c
> @@ -0,0 +1,326 @@
> +/*
> + * Copyright (C) 2015 - Tobias Jakobi
> + *
> + * This is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + *
> + * It is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + * You should have received a copy of the GNU General Public License
> + * along with it. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <unistd.h>
> +#include <poll.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <time.h>
> +#include <getopt.h>
> +
> +#include <pthread.h>
> +
> +#include <xf86drm.h>
> +
> +#include "exynos_drm.h"
> +#include "exynos_drmif.h"
> +#include "exynos_fimg2d.h"
> +
> +struct g2d_job {
> +	unsigned int id;
> +	unsigned int busy;
> +};
> +
> +struct exynos_evhandler {
> +	struct pollfd fds;
> +	struct exynos_event_context evctx;
> +};
> +
> +struct threaddata {
> +	unsigned int stop;
> +	struct exynos_device *dev;
> +	struct exynos_evhandler evhandler;
> +};
> +
> +static void g2d_event_handler(int fd, unsigned int cmdlist_no,
> unsigned int tv_sec,
> +							unsigned int
> tv_usec, void *user_data) +{
> +	struct g2d_job *job = user_data;
> +
> +	fprintf(stderr, "info: g2d job (id = %u, cmdlist number =
> %u) finished!\n",
> +			job->id, cmdlist_no);
> +
> +	job->busy = 0;
> +}
> +
> +static void setup_g2d_event_handler(struct exynos_evhandler
> *evhandler, int fd) +{
> +	evhandler->fds.fd = fd;
> +	evhandler->fds.events = POLLIN;
> +	evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
> +	evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;

The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
versions are bumped, the event will contains wrong version info.

Also, I think the type of event must be set here.

Best regards,
Hyungwon Hwang

> +	evhandler->evctx.g2d_event_handler = g2d_event_handler;
> +}
> +
> +static void* threadfunc(void *arg) {
> +	const int timeout = 0;
> +	struct threaddata *data;
> +
> +	data = arg;
> +
> +	while (1) {
> +		if (data->stop) break;
> +
> +		usleep(500);
> +
> +		data->evhandler.fds.revents = 0;
> +
> +		if (poll(&data->evhandler.fds, 1, timeout) < 0)
> +			continue;
> +
> +		if (data->evhandler.fds.revents & (POLLHUP |
> POLLERR))
> +			continue;
> +
> +		if (data->evhandler.fds.revents & POLLIN)
> +			exynos_handle_event(data->dev,
> &data->evhandler.evctx);
> +	}
> +
> +	pthread_exit(0);
> +}
> +
> +/*
> + * We need to wait until all G2D jobs are finished, otherwise we
> + * potentially remove a BO which the engine still operates on.
> + * This results in the following kernel message:
> + * [drm:exynos_drm_gem_put_dma_addr] *ERROR* failed to lookup gem
> object.
> + * Also any subsequent BO allocations fail then with:
> + * [drm:exynos_drm_alloc_buf] *ERROR* failed to allocate buffer.
> + */
> +static void wait_all_jobs(struct g2d_job* jobs, unsigned num_jobs)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < num_jobs; ++i) {
> +		while (jobs[i].busy)
> +			usleep(500);
> +	}
> +
> +}
> +
> +static struct g2d_job* free_job(struct g2d_job* jobs, unsigned
> num_jobs) +{
> +	unsigned i;
> +
> +	for (i = 0; i < num_jobs; ++i) {
> +		if (jobs[i].busy == 0)
> +			return &jobs[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static int g2d_work(struct g2d_context *ctx, struct g2d_image *img,
> +					unsigned num_jobs, unsigned
> iterations) +{
> +	struct g2d_job *jobs = calloc(num_jobs, sizeof(struct
> g2d_job));
> +	int ret;
> +	unsigned i;
> +
> +	/* setup job ids */
> +	for (i = 0; i < num_jobs; ++i)
> +		jobs[i].id = i;
> +
> +	for (i = 0; i < iterations; ++i) {
> +		unsigned x, y, w, h;
> +
> +		struct g2d_job *j = NULL;
> +
> +		while (1) {
> +			j = free_job(jobs, num_jobs);
> +
> +			if (j)
> +				break;
> +			else
> +				usleep(500);
> +		}
> +
> +		x = rand() % img->width;
> +		y = rand() % img->height;
> +
> +		if (x == (img->width - 1))
> +			x -= 1;
> +		if (y == (img->height - 1))
> +			y -= 1;
> +
> +		w = rand() % (img->width - x);
> +		h = rand() % (img->height - y);
> +
> +		if (w == 0) w = 1;
> +		if (h == 0) h = 1;
> +
> +		img->color = rand();
> +
> +		j->busy = 1;
> +		g2d_config_event(ctx, j);
> +
> +		ret = g2d_solid_fill(ctx, img, x, y, w, h);
> +
> +		if (ret == 0)
> +			g2d_exec2(ctx, G2D_EXEC_FLAG_ASYNC);
> +
> +		if (ret != 0) {
> +			fprintf(stderr, "error: iteration %u (x =
> %u, x = %u, x = %u, x = %u) failed\n",
> +					i, x, y, w, h);
> +			break;
> +		}
> +	}
> +
> +	wait_all_jobs(jobs, num_jobs);
> +	free(jobs);
> +
> +	return 0;
> +}
> +
> +static void usage(const char *name)
> +{
> +	fprintf(stderr, "usage: %s [-ijwh]\n\n", name);
> +
> +	fprintf(stderr, "\t-i <number of iterations>\n");
> +	fprintf(stderr, "\t-j <number of G2D jobs> (default =
> 4)\n\n"); +
> +	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
> +	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n");
> +
> +	exit(0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int fd, ret, c, parsefail;
> +
> +	pthread_t event_thread;
> +	struct threaddata event_data = {0};
> +
> +	struct exynos_device *dev;
> +	struct g2d_context *ctx;
> +	struct exynos_bo *bo;
> +
> +	struct g2d_image img = {0};
> +
> +	unsigned int iters = 0, njobs = 4;
> +	unsigned int bufw = 4096, bufh = 4096;
> +
> +	ret = 0;
> +	parsefail = 0;
> +
> +	while ((c = getopt(argc, argv, "i:j:w:h:")) != -1) {
> +		switch (c) {
> +		case 'i':
> +			if (sscanf(optarg, "%u", &iters) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'j':
> +			if (sscanf(optarg, "%u", &njobs) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'w':
> +			if (sscanf(optarg, "%u", &bufw) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'h':
> +			if (sscanf(optarg, "%u", &bufh) != 1)
> +				parsefail = 1;
> +			break;
> +		default:
> +			parsefail = 1;
> +			break;
> +		}
> +	}
> +
> +	if (parsefail || (argc == 1) || (iters == 0))
> +		usage(argv[0]);
> +
> +	if (bufw > 4096 || bufh > 4096) {
> +		fprintf(stderr, "error: buffer width/height should
> be less than 4096.\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	if (bufw == 0 || bufh == 0) {
> +		fprintf(stderr, "error: buffer width/height should
> be non-zero.\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	fd = drmOpen("exynos", NULL);
> +	if (fd < 0) {
> +		fprintf(stderr, "error: failed to open drm\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	dev = exynos_device_create(fd);
> +	if (dev == NULL) {
> +		fprintf(stderr, "error: failed to create device\n");
> +		ret = -2;
> +
> +		goto fail;
> +	}
> +
> +	ctx = g2d_init(fd);
> +	if (ctx == NULL) {
> +		fprintf(stderr, "error: failed to init G2D\n");
> +		ret = -3;
> +
> +		goto g2d_fail;
> +	}
> +
> +	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
> +	if (bo == NULL) {
> +		fprintf(stderr, "error: failed to create bo\n");
> +		ret = -4;
> +
> +		goto bo_fail;
> +	}
> +
> +	/* setup g2d image object */
> +	img.width = bufw;
> +	img.height = bufh;
> +	img.stride = bufw * 4;
> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
> +	img.buf_type = G2D_IMGBUF_GEM;
> +	img.bo[0] = bo->handle;
> +
> +	event_data.dev = dev;
> +	setup_g2d_event_handler(&event_data.evhandler, fd);
> +
> +	pthread_create(&event_thread, NULL, threadfunc, &event_data);
> +
> +	ret = g2d_work(ctx, &img, njobs, iters);
> +	if (ret != 0)
> +		fprintf(stderr, "error: g2d_work failed\n");
> +
> +	event_data.stop = 1;
> +	pthread_join(event_thread, NULL);
> +
> +	exynos_bo_destroy(bo);
> +
> +bo_fail:
> +	g2d_fini(ctx);
> +
> +g2d_fail:
> +	exynos_device_destroy(dev);
> +
> +fail:
> +	drmClose(fd);
> +
> +out:
> +	return ret;
> +}

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

Re: [PATCH 03/13] tests/exynos: add fimg2d performance analysis

[Hyungwon Hwang]

On Tue, 22 Sep 2015 17:54:52 +0200
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> Currently only fast solid color clear performance is measured.
> A large buffer is allocated and solid color clear operations
> are executed on it with randomly chosen properties (position
> and size of the region, clear color). Execution time is
> measured and output together with the amount of pixels
> processed.
> 
> The 'simple' variant only executes one G2D command buffer at
> a time, while the 'multi' variant executes multiple ones. This
> can be used to measure setup/exec overhead.
> 
> The test also serves a stability check. If clocks/voltages are
> too high or low respectively, the test quickly reveals this.
> 
> v2: Add GPLv2 header, argument handling and documentation.
>     Tool is only installed when requested.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  tests/exynos/Makefile.am          |  19 ++-
>  tests/exynos/exynos_fimg2d_perf.c | 320
> ++++++++++++++++++++++++++++++++++++++ 2 files changed, 337
> insertions(+), 2 deletions(-) create mode 100644
> tests/exynos/exynos_fimg2d_perf.c
> 
> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
> index b21d016..e82d199 100644
> --- a/tests/exynos/Makefile.am
> +++ b/tests/exynos/Makefile.am
> @@ -5,16 +5,31 @@ AM_CFLAGS = \
>  	-I $(top_srcdir)/exynos \
>  	-I $(top_srcdir)
>  
> +bin_PROGRAMS =
> +noinst_PROGRAMS =
> +
>  if HAVE_LIBKMS
>  if HAVE_INSTALL_TESTS
> -bin_PROGRAMS = \
> +bin_PROGRAMS += \
>  	exynos_fimg2d_test
>  else
> -noinst_PROGRAMS = \
> +noinst_PROGRAMS += \
>  	exynos_fimg2d_test
>  endif
>  endif
>  
> +if HAVE_INSTALL_TESTS
> +bin_PROGRAMS += \
> +	exynos_fimg2d_perf
> +else
> +noinst_PROGRAMS += \
> +	exynos_fimg2d_perf
> +endif
> +
> +exynos_fimg2d_perf_LDADD = \
> +	$(top_builddir)/libdrm.la \
> +	$(top_builddir)/exynos/libdrm_exynos.la
> +
>  exynos_fimg2d_test_LDADD = \
>  	$(top_builddir)/libdrm.la \
>  	$(top_builddir)/libkms/libkms.la \
> diff --git a/tests/exynos/exynos_fimg2d_perf.c
> b/tests/exynos/exynos_fimg2d_perf.c new file mode 100644
> index 0000000..73d28ea
> --- /dev/null
> +++ b/tests/exynos/exynos_fimg2d_perf.c
> @@ -0,0 +1,320 @@
> +/*
> + * Copyright (C) 2015 - Tobias Jakobi
> + *
> + * This is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation, either version 2 of the License,
> + * or (at your option) any later version.
> + *
> + * It is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + * You should have received a copy of the GNU General Public License
> + * along with it. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <time.h>
> +#include <getopt.h>
> +
> +#include <xf86drm.h>
> +
> +#include "exynos_drm.h"
> +#include "exynos_drmif.h"
> +#include "exynos_fimg2d.h"
> +
> +static int output_mathematica = 0;
> +
> +static int fimg2d_perf_simple(struct exynos_bo *bo, struct
> g2d_context *ctx,
> +			unsigned buf_width, unsigned buf_height,
> unsigned iterations) +{
> +	struct timespec tspec = { 0 };
> +	struct g2d_image img = { 0 };
> +
> +	unsigned long long g2d_time;
> +	unsigned i;
> +	int ret = 0;
> +
> +	img.width = buf_width;
> +	img.height = buf_height;
> +	img.stride = buf_width * 4;
> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
> +	img.buf_type = G2D_IMGBUF_GEM;
> +	img.bo[0] = bo->handle;
> +
> +	srand(time(NULL));
> +
> +	printf("starting simple G2D performance test\n");
> +	printf("buffer width = %u, buffer height = %u, iterations =
> %u\n",
> +		buf_width, buf_height, iterations);
> +
> +	if (output_mathematica)
> +		putchar('{');
> +
> +	for (i = 0; i < iterations; ++i) {
> +		unsigned x, y, w, h;
> +
> +		x = rand() % buf_width;
> +		y = rand() % buf_height;
> +
> +		if (x == (buf_width - 1))
> +			x -= 1;
> +		if (y == (buf_height - 1))
> +			y -= 1;
> +
> +		w = rand() % (buf_width - x);
> +		h = rand() % (buf_height - y);
> +
> +		if (w == 0) w = 1;
> +		if (h == 0) h = 1;
> +
> +		img.color = rand();
> +
> +		ret = g2d_solid_fill(ctx, &img, x, y, w, h);
> +
> +		clock_gettime(CLOCK_MONOTONIC, &tspec);
> +
> +		if (ret == 0)
> +			ret = g2d_exec(ctx);
> +
> +		if (ret != 0) {
> +			fprintf(stderr, "error: iteration %u failed
> (x = %u, y = %u, w = %u, h = %u)\n",
> +				i, x, y, w, h);
> +			break;
> +		} else {
> +			struct timespec end = { 0 };
> +			clock_gettime(CLOCK_MONOTONIC, &end);
> +
> +			g2d_time = (end.tv_sec - tspec.tv_sec) *
> 1000000000ULL;
> +			g2d_time += (end.tv_nsec - tspec.tv_nsec);
> +
> +			if (output_mathematica) {
> +				if (i != 0) putchar(',');
> +				printf("{%u,%llu}", w * h, g2d_time);
> +			} else {
> +				printf("num_pixels = %u, usecs =
> %llu\n", w * h, g2d_time);
> +			}
> +		}
> +	}
> +
> +	if (output_mathematica)
> +		printf("}\n");
> +
> +	return ret;
> +}
> +
> +static int fimg2d_perf_multi(struct exynos_bo *bo, struct
> g2d_context *ctx,
> +			unsigned buf_width, unsigned buf_height,
> unsigned iterations, unsigned batch) +{
> +	struct timespec tspec = { 0 };
> +	struct g2d_image *images;
> +
> +	unsigned long long g2d_time;
> +	unsigned i, j;
> +	int ret = 0;
> +
> +	images = calloc(batch, sizeof(struct g2d_image));

I think that this should be freed at the end of this function.

Best regards,
Hyungwon Hwang

> +	for (i = 0; i < batch; ++i) {
> +		images[i].width = buf_width;
> +		images[i].height = buf_height;
> +		images[i].stride = buf_width * 4;
> +		images[i].color_mode = G2D_COLOR_FMT_ARGB8888 |
> G2D_ORDER_AXRGB;
> +		images[i].buf_type = G2D_IMGBUF_GEM;
> +		images[i].bo[0] = bo->handle;
> +	}
> +
> +	srand(time(NULL));
> +
> +	printf("starting multi G2D performance test (batch size =
> %u)\n", batch);
> +	printf("buffer width = %u, buffer height = %u, iterations =
> %u\n",
> +		buf_width, buf_height, iterations);
> +
> +	if (output_mathematica)
> +		putchar('{');
> +
> +	for (i = 0; i < iterations; ++i) {
> +		unsigned num_pixels = 0;
> +
> +		for (j = 0; j < batch; ++j) {
> +			unsigned x, y, w, h;
> +
> +			x = rand() % buf_width;
> +			y = rand() % buf_height;
> +
> +			if (x == (buf_width - 1))
> +				x -= 1;
> +			if (y == (buf_height - 1))
> +				y -= 1;
> +
> +			w = rand() % (buf_width - x);
> +			h = rand() % (buf_height - y);
> +
> +			if (w == 0) w = 1;
> +			if (h == 0) h = 1;
> +
> +			images[j].color = rand();
> +
> +			num_pixels += w * h;
> +
> +			ret = g2d_solid_fill(ctx, &images[j], x, y,
> w, h);
> +			if (ret != 0)
> +				break;
> +		}
> +
> +		clock_gettime(CLOCK_MONOTONIC, &tspec);
> +
> +		if (ret == 0)
> +			ret = g2d_exec(ctx);
> +
> +		if (ret != 0) {
> +			fprintf(stderr, "error: iteration %u failed
> (num_pixels = %u)\n", i, num_pixels);
> +			break;
> +			break;
> +		} else {
> +			struct timespec end = { 0 };
> +			clock_gettime(CLOCK_MONOTONIC, &end);
> +
> +			g2d_time = (end.tv_sec - tspec.tv_sec) *
> 1000000000ULL;
> +			g2d_time += (end.tv_nsec - tspec.tv_nsec);
> +
> +			if (output_mathematica) {
> +				if (i != 0) putchar(',');
> +				printf("{%u,%llu}", num_pixels,
> g2d_time);
> +			} else {
> +				printf("num_pixels = %u, usecs =
> %llu\n", num_pixels, g2d_time);
> +			}
> +		}
> +	}
> +
> +	if (output_mathematica)
> +		printf("}\n");
> +
> +	return ret;
> +}
> +
> +static void usage(const char *name)
> +{
> +	fprintf(stderr, "usage: %s [-ibwh]\n\n", name);
> +
> +	fprintf(stderr, "\t-i <number of iterations>\n");
> +	fprintf(stderr, "\t-b <size of a batch> (default = 3)\n\n");
> +
> +	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
> +	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n\n");
> +
> +	fprintf(stderr, "\t-M <enable Mathematica styled output>\n");
> +
> +	exit(0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int fd, ret, c, parsefail;
> +
> +	struct exynos_device *dev;
> +	struct g2d_context *ctx;
> +	struct exynos_bo *bo;
> +
> +	unsigned int iters = 0, batch = 3;
> +	unsigned int bufw = 4096, bufh = 4096;
> +
> +	ret = 0;
> +	parsefail = 0;
> +
> +	while ((c = getopt(argc, argv, "i:b:w:h:M")) != -1) {
> +		switch (c) {
> +		case 'i':
> +			if (sscanf(optarg, "%u", &iters) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'b':
> +			if (sscanf(optarg, "%u", &batch) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'w':
> +			if (sscanf(optarg, "%u", &bufw) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'h':
> +			if (sscanf(optarg, "%u", &bufh) != 1)
> +				parsefail = 1;
> +			break;
> +		case 'M':
> +			output_mathematica = 1;
> +			break;
> +		default:
> +			parsefail = 1;
> +			break;
> +		}
> +	}
> +
> +	if (parsefail || (argc == 1) || (iters == 0))
> +		usage(argv[0]);
> +
> +	if (bufw < 2 || bufw > 4096 || bufh < 2 || bufh > 4096) {
> +		fprintf(stderr, "error: buffer width/height should
> be in the range 2 to 4096.\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	if (bufw == 0 || bufh == 0) {
> +		fprintf(stderr, "error: buffer width/height should
> be non-zero.\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	fd = drmOpen("exynos", NULL);
> +	if (fd < 0) {
> +		fprintf(stderr, "error: failed to open drm\n");
> +		ret = -1;
> +
> +		goto out;
> +	}
> +
> +	dev = exynos_device_create(fd);
> +	if (dev == NULL) {
> +		fprintf(stderr, "error: failed to create device\n");
> +		ret = -2;
> +
> +		goto fail;
> +	}
> +
> +	ctx = g2d_init(fd);
> +	if (ctx == NULL) {
> +		fprintf(stderr, "error: failed to init G2D\n");
> +		ret = -3;
> +
> +		goto g2d_fail;
> +	}
> +
> +	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
> +	if (bo == NULL) {
> +		fprintf(stderr, "error: failed to create bo\n");
> +		ret = -4;
> +
> +		goto bo_fail;
> +	}
> +
> +	ret = fimg2d_perf_simple(bo, ctx, bufw, bufh, iters);
> +
> +	if (ret == 0)
> +		ret = fimg2d_perf_multi(bo, ctx, bufw, bufh, iters,
> batch); +
> +	exynos_bo_destroy(bo);
> +
> +bo_fail:
> +	g2d_fini(ctx);
> +
> +g2d_fail:
> +	exynos_device_destroy(dev);
> +
> +fail:
> +	drmClose(fd);
> +
> +out:
> +	return ret;
> +}

Re: [PATCH 08/13] exynos: fimg2d: add g2d_set_direction

[Hyungwon Hwang]

On Tue, 22 Sep 2015 17:54:57 +0200
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> This allows setting the two direction registers, which specify how
> the engine blits pixels. This can be used for overlapping blits,
> which happen e.g. when 'moving' a rectangular region inside a
> fixed buffer.

Code itself looks good. But as I know, direction registers are related
with flip, not moving. Isn't it? I am not that much familiar with G2D.
Please let me know if I am wrong.

Best regards,
Hyungwon Hwang

> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  exynos/exynos_fimg2d.c | 13 +++++++++++++
>  exynos/exynos_fimg2d.h | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
> index e997d4b..4d5419c 100644
> --- a/exynos/exynos_fimg2d.c
> +++ b/exynos/exynos_fimg2d.c
> @@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct g2d_context
> *ctx, struct g2d_image *img, }
>  
>  /*
> + * g2d_set_direction - setup direction register (useful for
> overlapping blits).
> + *
> + * @ctx: a pointer to g2d_context structure.
> + * @dir: a pointer to the g2d_direction_val structure.
> + */
> +static void g2d_set_direction(struct g2d_context *ctx,
> +			const union g2d_direction_val *dir)
> +{
> +	g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
> +	g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
> +}
> +
> +/*
>   * g2d_reset - reset fimg2d hardware.
>   *
>   * @ctx: a pointer to g2d_context structure.
> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
> index c6b58ac..9eee7c0 100644
> --- a/exynos/exynos_fimg2d.h
> +++ b/exynos/exynos_fimg2d.h
> @@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
>  	G2D_EXEC_FLAG_ASYNC = (1 << 0)
>  };
>  
> +enum e_g2d_dir_mode {
> +	G2D_DIR_MODE_POSITIVE = 0,
> +	G2D_DIR_MODE_NEGATIVE = 1
> +};
> +
>  union g2d_point_val {
>  	unsigned int val;
>  	struct {
> @@ -269,6 +274,39 @@ union g2d_blend_func_val {
>  	} data;
>  };
>  
> +union g2d_direction_val {
> +	unsigned int val[2];
> +	struct {
> +		/* SRC_MSK_DIRECT_REG [0:1] (source) */
> +		enum e_g2d_dir_mode		src_x_direction:1;
> +		enum e_g2d_dir_mode		src_y_direction:1;
> +
> +		/* SRC_MSK_DIRECT_REG [2:3] */
> +		unsigned int			reversed1:2;
> +
> +		/* SRC_MSK_DIRECT_REG [4:5] (mask) */
> +		enum e_g2d_dir_mode
> mask_x_direction:1;
> +		enum e_g2d_dir_mode
> mask_y_direction:1; +
> +		/* SRC_MSK_DIRECT_REG [6:31] */
> +		unsigned int			padding1:26;
> +
> +		/* DST_PAT_DIRECT_REG [0:1] (destination) */
> +		enum e_g2d_dir_mode		dst_x_direction:1;
> +		enum e_g2d_dir_mode		dst_y_direction:1;
> +
> +		/* DST_PAT_DIRECT_REG [2:3] */
> +		unsigned int			reversed2:2;
> +
> +		/* DST_PAT_DIRECT_REG [4:5] (pattern) */
> +		enum e_g2d_dir_mode		pat_x_direction:1;
> +		enum e_g2d_dir_mode		pat_y_direction:1;
> +
> +		/* DST_PAT_DIRECT_REG [6:31] */
> +		unsigned int			padding2:26;
> +	} data;
> +};
> +
>  struct g2d_image {
>  	enum e_g2d_select_mode		select_mode;
>  	enum e_g2d_color_mode		color_mode;

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

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

[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; +

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,

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Tobias Jakobi]

Hello Hyungwon,

first of all thanks for reviewing the series!



Hyungwon Hwang wrote:
> On Tue, 22 Sep 2015 17:54:55 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> This tests async processing of G2D jobs. A separate thread is spawned
>> to monitor the DRM fd for events and check whether a G2D job was
>> completed.
>>
>> v2: Add GPLv2 header, argument handling and documentation.
>>     Test is only installed when requested.
>> v3: Allocate G2D jobs with calloc which fixes 'busy' being
>>     potentially uninitialized. Also enable timeout for poll()
>>     in the monitor thread. This fixes pthread_join() not working
>>     because of poll() not returning.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  tests/exynos/Makefile.am           |  11 +-
>>  tests/exynos/exynos_fimg2d_event.c | 326
>> +++++++++++++++++++++++++++++++++++++ 2 files changed, 335
>> insertions(+), 2 deletions(-) create mode 100644
>> tests/exynos/exynos_fimg2d_event.c
>>
>> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
>> index e82d199..357d6b8 100644
>> --- a/tests/exynos/Makefile.am
>> +++ b/tests/exynos/Makefile.am
>> @@ -20,16 +20,23 @@ endif
>>  
>>  if HAVE_INSTALL_TESTS
>>  bin_PROGRAMS += \
>> -	exynos_fimg2d_perf
>> +	exynos_fimg2d_perf \
>> +	exynos_fimg2d_event
>>  else
>>  noinst_PROGRAMS += \
>> -	exynos_fimg2d_perf
>> +	exynos_fimg2d_perf \
>> +	exynos_fimg2d_event
>>  endif
>>  
>>  exynos_fimg2d_perf_LDADD = \
>>  	$(top_builddir)/libdrm.la \
>>  	$(top_builddir)/exynos/libdrm_exynos.la
>>  
>> +exynos_fimg2d_event_LDADD = \
>> +	$(top_builddir)/libdrm.la \
>> +	$(top_builddir)/exynos/libdrm_exynos.la \
>> +	-lpthread
>> +
>>  exynos_fimg2d_test_LDADD = \
>>  	$(top_builddir)/libdrm.la \
>>  	$(top_builddir)/libkms/libkms.la \
>> diff --git a/tests/exynos/exynos_fimg2d_event.c
>> b/tests/exynos/exynos_fimg2d_event.c new file mode 100644
>> index 0000000..c03dcff
>> --- /dev/null
>> +++ b/tests/exynos/exynos_fimg2d_event.c
>> @@ -0,0 +1,326 @@
>> +/*
>> + * Copyright (C) 2015 - Tobias Jakobi
>> + *
>> + * This is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published
>> + * by the Free Software Foundation, either version 2 of the License,
>> + * or (at your option) any later version.
>> + *
>> + * It is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + * You should have received a copy of the GNU General Public License
>> + * along with it. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <unistd.h>
>> +#include <poll.h>
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <time.h>
>> +#include <getopt.h>
>> +
>> +#include <pthread.h>
>> +
>> +#include <xf86drm.h>
>> +
>> +#include "exynos_drm.h"
>> +#include "exynos_drmif.h"
>> +#include "exynos_fimg2d.h"
>> +
>> +struct g2d_job {
>> +	unsigned int id;
>> +	unsigned int busy;
>> +};
>> +
>> +struct exynos_evhandler {
>> +	struct pollfd fds;
>> +	struct exynos_event_context evctx;
>> +};
>> +
>> +struct threaddata {
>> +	unsigned int stop;
>> +	struct exynos_device *dev;
>> +	struct exynos_evhandler evhandler;
>> +};
>> +
>> +static void g2d_event_handler(int fd, unsigned int cmdlist_no,
>> unsigned int tv_sec,
>> +							unsigned int
>> tv_usec, void *user_data) +{
>> +	struct g2d_job *job = user_data;
>> +
>> +	fprintf(stderr, "info: g2d job (id = %u, cmdlist number =
>> %u) finished!\n",
>> +			job->id, cmdlist_no);
>> +
>> +	job->busy = 0;
>> +}
>> +
>> +static void setup_g2d_event_handler(struct exynos_evhandler
>> *evhandler, int fd) +{
>> +	evhandler->fds.fd = fd;
>> +	evhandler->fds.events = POLLIN;
>> +	evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>> +	evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
> 
> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
> versions are bumped, the event will contains wrong version info.
Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
version in the public header is bumped, then it's also bumped here. So I
don't see the issue.


> Also, I think the type of event must be set here.
What do you mean by 'type of event' here? I don't see a type field in
the event context structures.


With best wishes,
Tobias


> 
> Best regards,
> Hyungwon Hwang
> 
>> +	evhandler->evctx.g2d_event_handler = g2d_event_handler;
>> +}
>> +
>> +static void* threadfunc(void *arg) {
>> +	const int timeout = 0;
>> +	struct threaddata *data;
>> +
>> +	data = arg;
>> +
>> +	while (1) {
>> +		if (data->stop) break;
>> +
>> +		usleep(500);
>> +
>> +		data->evhandler.fds.revents = 0;
>> +
>> +		if (poll(&data->evhandler.fds, 1, timeout) < 0)
>> +			continue;
>> +
>> +		if (data->evhandler.fds.revents & (POLLHUP |
>> POLLERR))
>> +			continue;
>> +
>> +		if (data->evhandler.fds.revents & POLLIN)
>> +			exynos_handle_event(data->dev,
>> &data->evhandler.evctx);
>> +	}
>> +
>> +	pthread_exit(0);
>> +}
>> +
>> +/*
>> + * We need to wait until all G2D jobs are finished, otherwise we
>> + * potentially remove a BO which the engine still operates on.
>> + * This results in the following kernel message:
>> + * [drm:exynos_drm_gem_put_dma_addr] *ERROR* failed to lookup gem
>> object.
>> + * Also any subsequent BO allocations fail then with:
>> + * [drm:exynos_drm_alloc_buf] *ERROR* failed to allocate buffer.
>> + */
>> +static void wait_all_jobs(struct g2d_job* jobs, unsigned num_jobs)
>> +{
>> +	unsigned i;
>> +
>> +	for (i = 0; i < num_jobs; ++i) {
>> +		while (jobs[i].busy)
>> +			usleep(500);
>> +	}
>> +
>> +}
>> +
>> +static struct g2d_job* free_job(struct g2d_job* jobs, unsigned
>> num_jobs) +{
>> +	unsigned i;
>> +
>> +	for (i = 0; i < num_jobs; ++i) {
>> +		if (jobs[i].busy == 0)
>> +			return &jobs[i];
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int g2d_work(struct g2d_context *ctx, struct g2d_image *img,
>> +					unsigned num_jobs, unsigned
>> iterations) +{
>> +	struct g2d_job *jobs = calloc(num_jobs, sizeof(struct
>> g2d_job));
>> +	int ret;
>> +	unsigned i;
>> +
>> +	/* setup job ids */
>> +	for (i = 0; i < num_jobs; ++i)
>> +		jobs[i].id = i;
>> +
>> +	for (i = 0; i < iterations; ++i) {
>> +		unsigned x, y, w, h;
>> +
>> +		struct g2d_job *j = NULL;
>> +
>> +		while (1) {
>> +			j = free_job(jobs, num_jobs);
>> +
>> +			if (j)
>> +				break;
>> +			else
>> +				usleep(500);
>> +		}
>> +
>> +		x = rand() % img->width;
>> +		y = rand() % img->height;
>> +
>> +		if (x == (img->width - 1))
>> +			x -= 1;
>> +		if (y == (img->height - 1))
>> +			y -= 1;
>> +
>> +		w = rand() % (img->width - x);
>> +		h = rand() % (img->height - y);
>> +
>> +		if (w == 0) w = 1;
>> +		if (h == 0) h = 1;
>> +
>> +		img->color = rand();
>> +
>> +		j->busy = 1;
>> +		g2d_config_event(ctx, j);
>> +
>> +		ret = g2d_solid_fill(ctx, img, x, y, w, h);
>> +
>> +		if (ret == 0)
>> +			g2d_exec2(ctx, G2D_EXEC_FLAG_ASYNC);
>> +
>> +		if (ret != 0) {
>> +			fprintf(stderr, "error: iteration %u (x =
>> %u, x = %u, x = %u, x = %u) failed\n",
>> +					i, x, y, w, h);
>> +			break;
>> +		}
>> +	}
>> +
>> +	wait_all_jobs(jobs, num_jobs);
>> +	free(jobs);
>> +
>> +	return 0;
>> +}
>> +
>> +static void usage(const char *name)
>> +{
>> +	fprintf(stderr, "usage: %s [-ijwh]\n\n", name);
>> +
>> +	fprintf(stderr, "\t-i <number of iterations>\n");
>> +	fprintf(stderr, "\t-j <number of G2D jobs> (default =
>> 4)\n\n"); +
>> +	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
>> +	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n");
>> +
>> +	exit(0);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	int fd, ret, c, parsefail;
>> +
>> +	pthread_t event_thread;
>> +	struct threaddata event_data = {0};
>> +
>> +	struct exynos_device *dev;
>> +	struct g2d_context *ctx;
>> +	struct exynos_bo *bo;
>> +
>> +	struct g2d_image img = {0};
>> +
>> +	unsigned int iters = 0, njobs = 4;
>> +	unsigned int bufw = 4096, bufh = 4096;
>> +
>> +	ret = 0;
>> +	parsefail = 0;
>> +
>> +	while ((c = getopt(argc, argv, "i:j:w:h:")) != -1) {
>> +		switch (c) {
>> +		case 'i':
>> +			if (sscanf(optarg, "%u", &iters) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'j':
>> +			if (sscanf(optarg, "%u", &njobs) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'w':
>> +			if (sscanf(optarg, "%u", &bufw) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'h':
>> +			if (sscanf(optarg, "%u", &bufh) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		default:
>> +			parsefail = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (parsefail || (argc == 1) || (iters == 0))
>> +		usage(argv[0]);
>> +
>> +	if (bufw > 4096 || bufh > 4096) {
>> +		fprintf(stderr, "error: buffer width/height should
>> be less than 4096.\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	if (bufw == 0 || bufh == 0) {
>> +		fprintf(stderr, "error: buffer width/height should
>> be non-zero.\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	fd = drmOpen("exynos", NULL);
>> +	if (fd < 0) {
>> +		fprintf(stderr, "error: failed to open drm\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	dev = exynos_device_create(fd);
>> +	if (dev == NULL) {
>> +		fprintf(stderr, "error: failed to create device\n");
>> +		ret = -2;
>> +
>> +		goto fail;
>> +	}
>> +
>> +	ctx = g2d_init(fd);
>> +	if (ctx == NULL) {
>> +		fprintf(stderr, "error: failed to init G2D\n");
>> +		ret = -3;
>> +
>> +		goto g2d_fail;
>> +	}
>> +
>> +	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
>> +	if (bo == NULL) {
>> +		fprintf(stderr, "error: failed to create bo\n");
>> +		ret = -4;
>> +
>> +		goto bo_fail;
>> +	}
>> +
>> +	/* setup g2d image object */
>> +	img.width = bufw;
>> +	img.height = bufh;
>> +	img.stride = bufw * 4;
>> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
>> +	img.buf_type = G2D_IMGBUF_GEM;
>> +	img.bo[0] = bo->handle;
>> +
>> +	event_data.dev = dev;
>> +	setup_g2d_event_handler(&event_data.evhandler, fd);
>> +
>> +	pthread_create(&event_thread, NULL, threadfunc, &event_data);
>> +
>> +	ret = g2d_work(ctx, &img, njobs, iters);
>> +	if (ret != 0)
>> +		fprintf(stderr, "error: g2d_work failed\n");
>> +
>> +	event_data.stop = 1;
>> +	pthread_join(event_thread, NULL);
>> +
>> +	exynos_bo_destroy(bo);
>> +
>> +bo_fail:
>> +	g2d_fini(ctx);
>> +
>> +g2d_fail:
>> +	exynos_device_destroy(dev);
>> +
>> +fail:
>> +	drmClose(fd);
>> +
>> +out:
>> +	return ret;
>> +}
> 

Re: [PATCH 07/13] tests/exynos: use XRGB8888 for framebuffer

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> On Tue, 22 Sep 2015 17:54:56 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> This matches the G2D color mode that is used in the entire code.
>> The previous (incorrect) RGBA8888 would only work since the
>> Exynos mixer did its configuration based on the bpp, and not
>> based on the actual pixelformat.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  tests/exynos/exynos_fimg2d_test.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/exynos/exynos_fimg2d_test.c
>> b/tests/exynos/exynos_fimg2d_test.c index 8794dac..dfb00a0 100644
>> --- a/tests/exynos/exynos_fimg2d_test.c
>> +++ b/tests/exynos/exynos_fimg2d_test.c
>> @@ -675,7 +675,7 @@ int main(int argc, char **argv)
>>  	offsets[0] = 0;
>>  
>>  	ret = drmModeAddFB2(dev->fd, screen_width, screen_height,
>> -				DRM_FORMAT_RGBA8888, handles,
>> +				DRM_FORMAT_XRGB8888, handles,
>>  				pitches, offsets, &fb_id, 0);
> 
> Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>
> 
> Nice catch. It's right, if there was no previous setting for source
> image color mode. But I think it could be the source image color mode
> was set by another application before when this test runs. So I think
> the code which sets the source image color mode must be added.
I think you misunderstand something here. First of all settings from
anither application using DRM don't carry over.

The point for this change is that all used G2D image structures have the
color_mode field set to G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB. So the
G2D is operating on ARGB8888 pixel data.

However the framebuffer is set to DRM_FORMAT_RGBA8888, which obviously
is wrong since this is not the type of data we put into the framebuffer.


With best wishes,
Tobias


> 
> Best regards,
> Hyungwon Hwang
> 
> 
>>  	if (ret < 0)
>>  		goto err_destroy_buffer;
> 

Re: [PATCH 03/13] tests/exynos: add fimg2d performance analysis

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> On Tue, 22 Sep 2015 17:54:52 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> Currently only fast solid color clear performance is measured.
>> A large buffer is allocated and solid color clear operations
>> are executed on it with randomly chosen properties (position
>> and size of the region, clear color). Execution time is
>> measured and output together with the amount of pixels
>> processed.
>>
>> The 'simple' variant only executes one G2D command buffer at
>> a time, while the 'multi' variant executes multiple ones. This
>> can be used to measure setup/exec overhead.
>>
>> The test also serves a stability check. If clocks/voltages are
>> too high or low respectively, the test quickly reveals this.
>>
>> v2: Add GPLv2 header, argument handling and documentation.
>>     Tool is only installed when requested.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  tests/exynos/Makefile.am          |  19 ++-
>>  tests/exynos/exynos_fimg2d_perf.c | 320
>> ++++++++++++++++++++++++++++++++++++++ 2 files changed, 337
>> insertions(+), 2 deletions(-) create mode 100644
>> tests/exynos/exynos_fimg2d_perf.c
>>
>> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
>> index b21d016..e82d199 100644
>> --- a/tests/exynos/Makefile.am
>> +++ b/tests/exynos/Makefile.am
>> @@ -5,16 +5,31 @@ AM_CFLAGS = \
>>  	-I $(top_srcdir)/exynos \
>>  	-I $(top_srcdir)
>>  
>> +bin_PROGRAMS =
>> +noinst_PROGRAMS =
>> +
>>  if HAVE_LIBKMS
>>  if HAVE_INSTALL_TESTS
>> -bin_PROGRAMS = \
>> +bin_PROGRAMS += \
>>  	exynos_fimg2d_test
>>  else
>> -noinst_PROGRAMS = \
>> +noinst_PROGRAMS += \
>>  	exynos_fimg2d_test
>>  endif
>>  endif
>>  
>> +if HAVE_INSTALL_TESTS
>> +bin_PROGRAMS += \
>> +	exynos_fimg2d_perf
>> +else
>> +noinst_PROGRAMS += \
>> +	exynos_fimg2d_perf
>> +endif
>> +
>> +exynos_fimg2d_perf_LDADD = \
>> +	$(top_builddir)/libdrm.la \
>> +	$(top_builddir)/exynos/libdrm_exynos.la
>> +
>>  exynos_fimg2d_test_LDADD = \
>>  	$(top_builddir)/libdrm.la \
>>  	$(top_builddir)/libkms/libkms.la \
>> diff --git a/tests/exynos/exynos_fimg2d_perf.c
>> b/tests/exynos/exynos_fimg2d_perf.c new file mode 100644
>> index 0000000..73d28ea
>> --- /dev/null
>> +++ b/tests/exynos/exynos_fimg2d_perf.c
>> @@ -0,0 +1,320 @@
>> +/*
>> + * Copyright (C) 2015 - Tobias Jakobi
>> + *
>> + * This is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published
>> + * by the Free Software Foundation, either version 2 of the License,
>> + * or (at your option) any later version.
>> + *
>> + * It is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + * You should have received a copy of the GNU General Public License
>> + * along with it. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <time.h>
>> +#include <getopt.h>
>> +
>> +#include <xf86drm.h>
>> +
>> +#include "exynos_drm.h"
>> +#include "exynos_drmif.h"
>> +#include "exynos_fimg2d.h"
>> +
>> +static int output_mathematica = 0;
>> +
>> +static int fimg2d_perf_simple(struct exynos_bo *bo, struct
>> g2d_context *ctx,
>> +			unsigned buf_width, unsigned buf_height,
>> unsigned iterations) +{
>> +	struct timespec tspec = { 0 };
>> +	struct g2d_image img = { 0 };
>> +
>> +	unsigned long long g2d_time;
>> +	unsigned i;
>> +	int ret = 0;
>> +
>> +	img.width = buf_width;
>> +	img.height = buf_height;
>> +	img.stride = buf_width * 4;
>> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
>> +	img.buf_type = G2D_IMGBUF_GEM;
>> +	img.bo[0] = bo->handle;
>> +
>> +	srand(time(NULL));
>> +
>> +	printf("starting simple G2D performance test\n");
>> +	printf("buffer width = %u, buffer height = %u, iterations =
>> %u\n",
>> +		buf_width, buf_height, iterations);
>> +
>> +	if (output_mathematica)
>> +		putchar('{');
>> +
>> +	for (i = 0; i < iterations; ++i) {
>> +		unsigned x, y, w, h;
>> +
>> +		x = rand() % buf_width;
>> +		y = rand() % buf_height;
>> +
>> +		if (x == (buf_width - 1))
>> +			x -= 1;
>> +		if (y == (buf_height - 1))
>> +			y -= 1;
>> +
>> +		w = rand() % (buf_width - x);
>> +		h = rand() % (buf_height - y);
>> +
>> +		if (w == 0) w = 1;
>> +		if (h == 0) h = 1;
>> +
>> +		img.color = rand();
>> +
>> +		ret = g2d_solid_fill(ctx, &img, x, y, w, h);
>> +
>> +		clock_gettime(CLOCK_MONOTONIC, &tspec);
>> +
>> +		if (ret == 0)
>> +			ret = g2d_exec(ctx);
>> +
>> +		if (ret != 0) {
>> +			fprintf(stderr, "error: iteration %u failed
>> (x = %u, y = %u, w = %u, h = %u)\n",
>> +				i, x, y, w, h);
>> +			break;
>> +		} else {
>> +			struct timespec end = { 0 };
>> +			clock_gettime(CLOCK_MONOTONIC, &end);
>> +
>> +			g2d_time = (end.tv_sec - tspec.tv_sec) *
>> 1000000000ULL;
>> +			g2d_time += (end.tv_nsec - tspec.tv_nsec);
>> +
>> +			if (output_mathematica) {
>> +				if (i != 0) putchar(',');
>> +				printf("{%u,%llu}", w * h, g2d_time);
>> +			} else {
>> +				printf("num_pixels = %u, usecs =
>> %llu\n", w * h, g2d_time);
>> +			}
>> +		}
>> +	}
>> +
>> +	if (output_mathematica)
>> +		printf("}\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int fimg2d_perf_multi(struct exynos_bo *bo, struct
>> g2d_context *ctx,
>> +			unsigned buf_width, unsigned buf_height,
>> unsigned iterations, unsigned batch) +{
>> +	struct timespec tspec = { 0 };
>> +	struct g2d_image *images;
>> +
>> +	unsigned long long g2d_time;
>> +	unsigned i, j;
>> +	int ret = 0;
>> +
>> +	images = calloc(batch, sizeof(struct g2d_image));
> 
> I think that this should be freed at the end of this function.
Thanks, going to fix that!


With best wishes,
Tobias



> Best regards,
> Hyungwon Hwang
> 
>> +	for (i = 0; i < batch; ++i) {
>> +		images[i].width = buf_width;
>> +		images[i].height = buf_height;
>> +		images[i].stride = buf_width * 4;
>> +		images[i].color_mode = G2D_COLOR_FMT_ARGB8888 |
>> G2D_ORDER_AXRGB;
>> +		images[i].buf_type = G2D_IMGBUF_GEM;
>> +		images[i].bo[0] = bo->handle;
>> +	}
>> +
>> +	srand(time(NULL));
>> +
>> +	printf("starting multi G2D performance test (batch size =
>> %u)\n", batch);
>> +	printf("buffer width = %u, buffer height = %u, iterations =
>> %u\n",
>> +		buf_width, buf_height, iterations);
>> +
>> +	if (output_mathematica)
>> +		putchar('{');
>> +
>> +	for (i = 0; i < iterations; ++i) {
>> +		unsigned num_pixels = 0;
>> +
>> +		for (j = 0; j < batch; ++j) {
>> +			unsigned x, y, w, h;
>> +
>> +			x = rand() % buf_width;
>> +			y = rand() % buf_height;
>> +
>> +			if (x == (buf_width - 1))
>> +				x -= 1;
>> +			if (y == (buf_height - 1))
>> +				y -= 1;
>> +
>> +			w = rand() % (buf_width - x);
>> +			h = rand() % (buf_height - y);
>> +
>> +			if (w == 0) w = 1;
>> +			if (h == 0) h = 1;
>> +
>> +			images[j].color = rand();
>> +
>> +			num_pixels += w * h;
>> +
>> +			ret = g2d_solid_fill(ctx, &images[j], x, y,
>> w, h);
>> +			if (ret != 0)
>> +				break;
>> +		}
>> +
>> +		clock_gettime(CLOCK_MONOTONIC, &tspec);
>> +
>> +		if (ret == 0)
>> +			ret = g2d_exec(ctx);
>> +
>> +		if (ret != 0) {
>> +			fprintf(stderr, "error: iteration %u failed
>> (num_pixels = %u)\n", i, num_pixels);
>> +			break;
>> +			break;
>> +		} else {
>> +			struct timespec end = { 0 };
>> +			clock_gettime(CLOCK_MONOTONIC, &end);
>> +
>> +			g2d_time = (end.tv_sec - tspec.tv_sec) *
>> 1000000000ULL;
>> +			g2d_time += (end.tv_nsec - tspec.tv_nsec);
>> +
>> +			if (output_mathematica) {
>> +				if (i != 0) putchar(',');
>> +				printf("{%u,%llu}", num_pixels,
>> g2d_time);
>> +			} else {
>> +				printf("num_pixels = %u, usecs =
>> %llu\n", num_pixels, g2d_time);
>> +			}
>> +		}
>> +	}
>> +
>> +	if (output_mathematica)
>> +		printf("}\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static void usage(const char *name)
>> +{
>> +	fprintf(stderr, "usage: %s [-ibwh]\n\n", name);
>> +
>> +	fprintf(stderr, "\t-i <number of iterations>\n");
>> +	fprintf(stderr, "\t-b <size of a batch> (default = 3)\n\n");
>> +
>> +	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
>> +	fprintf(stderr, "\t-h <buffer height> (default = 4096)\n\n");
>> +
>> +	fprintf(stderr, "\t-M <enable Mathematica styled output>\n");
>> +
>> +	exit(0);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	int fd, ret, c, parsefail;
>> +
>> +	struct exynos_device *dev;
>> +	struct g2d_context *ctx;
>> +	struct exynos_bo *bo;
>> +
>> +	unsigned int iters = 0, batch = 3;
>> +	unsigned int bufw = 4096, bufh = 4096;
>> +
>> +	ret = 0;
>> +	parsefail = 0;
>> +
>> +	while ((c = getopt(argc, argv, "i:b:w:h:M")) != -1) {
>> +		switch (c) {
>> +		case 'i':
>> +			if (sscanf(optarg, "%u", &iters) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'b':
>> +			if (sscanf(optarg, "%u", &batch) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'w':
>> +			if (sscanf(optarg, "%u", &bufw) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'h':
>> +			if (sscanf(optarg, "%u", &bufh) != 1)
>> +				parsefail = 1;
>> +			break;
>> +		case 'M':
>> +			output_mathematica = 1;
>> +			break;
>> +		default:
>> +			parsefail = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (parsefail || (argc == 1) || (iters == 0))
>> +		usage(argv[0]);
>> +
>> +	if (bufw < 2 || bufw > 4096 || bufh < 2 || bufh > 4096) {
>> +		fprintf(stderr, "error: buffer width/height should
>> be in the range 2 to 4096.\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	if (bufw == 0 || bufh == 0) {
>> +		fprintf(stderr, "error: buffer width/height should
>> be non-zero.\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	fd = drmOpen("exynos", NULL);
>> +	if (fd < 0) {
>> +		fprintf(stderr, "error: failed to open drm\n");
>> +		ret = -1;
>> +
>> +		goto out;
>> +	}
>> +
>> +	dev = exynos_device_create(fd);
>> +	if (dev == NULL) {
>> +		fprintf(stderr, "error: failed to create device\n");
>> +		ret = -2;
>> +
>> +		goto fail;
>> +	}
>> +
>> +	ctx = g2d_init(fd);
>> +	if (ctx == NULL) {
>> +		fprintf(stderr, "error: failed to init G2D\n");
>> +		ret = -3;
>> +
>> +		goto g2d_fail;
>> +	}
>> +
>> +	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
>> +	if (bo == NULL) {
>> +		fprintf(stderr, "error: failed to create bo\n");
>> +		ret = -4;
>> +
>> +		goto bo_fail;
>> +	}
>> +
>> +	ret = fimg2d_perf_simple(bo, ctx, bufw, bufh, iters);
>> +
>> +	if (ret == 0)
>> +		ret = fimg2d_perf_multi(bo, ctx, bufw, bufh, iters,
>> batch); +
>> +	exynos_bo_destroy(bo);
>> +
>> +bo_fail:
>> +	g2d_fini(ctx);
>> +
>> +g2d_fail:
>> +	exynos_device_destroy(dev);
>> +
>> +fail:
>> +	drmClose(fd);
>> +
>> +out:
>> +	return ret;
>> +}
> 

Re: [PATCH 08/13] exynos: fimg2d: add g2d_set_direction

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> On Tue, 22 Sep 2015 17:54:57 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> This allows setting the two direction registers, which specify how
>> the engine blits pixels. This can be used for overlapping blits,
>> which happen e.g. when 'moving' a rectangular region inside a
>> fixed buffer.
> 
> Code itself looks good. But as I know, direction registers are related
> with flip, not moving. Isn't it? I am not that much familiar with G2D.
> Please let me know if I am wrong.
that's correct, you can use the direction registers to implement
flipping operations.

However what they really do is to change the operation direction of the
G2D engine. So you can manage how the engine traverses through pixel
rows and column (either in forward or in backward direction).

Normally this doesn't matter, like it doesn't matter if you memcpy()
from one buffer to another when there is no intersection of the buffers.
However if the two buffers overlap then you have to be careful with the
direction in which you traverse the buffer.

Now if you set the x-direction of the source G2D image to forward, but
the x-direction of the destination to backward, you get your mentioned
x-flipping.

The main purpose for g2d_move() is to be used e.g. in acceleration code
for the armsoc Xorg DDX. A common operation there is not move pixel
regions around in the same buffer.


With best wishes,
Tobias


P.S.: I think the Exynos user manual mentions flipping as one example on
how to use these registers. But like I said above, it's just one way to
use it.




> 
> Best regards,
> Hyungwon Hwang
> 
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  exynos/exynos_fimg2d.c | 13 +++++++++++++
>>  exynos/exynos_fimg2d.h | 38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>> index e997d4b..4d5419c 100644
>> --- a/exynos/exynos_fimg2d.c
>> +++ b/exynos/exynos_fimg2d.c
>> @@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct g2d_context
>> *ctx, struct g2d_image *img, }
>>  
>>  /*
>> + * g2d_set_direction - setup direction register (useful for
>> overlapping blits).
>> + *
>> + * @ctx: a pointer to g2d_context structure.
>> + * @dir: a pointer to the g2d_direction_val structure.
>> + */
>> +static void g2d_set_direction(struct g2d_context *ctx,
>> +			const union g2d_direction_val *dir)
>> +{
>> +	g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
>> +	g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
>> +}
>> +
>> +/*
>>   * g2d_reset - reset fimg2d hardware.
>>   *
>>   * @ctx: a pointer to g2d_context structure.
>> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
>> index c6b58ac..9eee7c0 100644
>> --- a/exynos/exynos_fimg2d.h
>> +++ b/exynos/exynos_fimg2d.h
>> @@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
>>  	G2D_EXEC_FLAG_ASYNC = (1 << 0)
>>  };
>>  
>> +enum e_g2d_dir_mode {
>> +	G2D_DIR_MODE_POSITIVE = 0,
>> +	G2D_DIR_MODE_NEGATIVE = 1
>> +};
>> +
>>  union g2d_point_val {
>>  	unsigned int val;
>>  	struct {
>> @@ -269,6 +274,39 @@ union g2d_blend_func_val {
>>  	} data;
>>  };
>>  
>> +union g2d_direction_val {
>> +	unsigned int val[2];
>> +	struct {
>> +		/* SRC_MSK_DIRECT_REG [0:1] (source) */
>> +		enum e_g2d_dir_mode		src_x_direction:1;
>> +		enum e_g2d_dir_mode		src_y_direction:1;
>> +
>> +		/* SRC_MSK_DIRECT_REG [2:3] */
>> +		unsigned int			reversed1:2;
>> +
>> +		/* SRC_MSK_DIRECT_REG [4:5] (mask) */
>> +		enum e_g2d_dir_mode
>> mask_x_direction:1;
>> +		enum e_g2d_dir_mode
>> mask_y_direction:1; +
>> +		/* SRC_MSK_DIRECT_REG [6:31] */
>> +		unsigned int			padding1:26;
>> +
>> +		/* DST_PAT_DIRECT_REG [0:1] (destination) */
>> +		enum e_g2d_dir_mode		dst_x_direction:1;
>> +		enum e_g2d_dir_mode		dst_y_direction:1;
>> +
>> +		/* DST_PAT_DIRECT_REG [2:3] */
>> +		unsigned int			reversed2:2;
>> +
>> +		/* DST_PAT_DIRECT_REG [4:5] (pattern) */
>> +		enum e_g2d_dir_mode		pat_x_direction:1;
>> +		enum e_g2d_dir_mode		pat_y_direction:1;
>> +
>> +		/* DST_PAT_DIRECT_REG [6:31] */
>> +		unsigned int			padding2:26;
>> +	} data;
>> +};
>> +
>>  struct g2d_image {
>>  	enum e_g2d_select_mode		select_mode;
>>  	enum e_g2d_color_mode		color_mode;
> 

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

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

[Tobias Jakobi]

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,
> 

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 11:16, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
> Hello Hyungwon,
>
> first of all thanks for reviewing the series!
>
>
>
> Hyungwon Hwang wrote:
>> On Tue, 22 Sep 2015 17:54:55 +0200
>> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>

>>> +    evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>> +    evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>
>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>> versions are bumped, the event will contains wrong version info.
> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
> version in the public header is bumped, then it's also bumped here. So I
> don't see the issue.
>
The issue is that the public header defines the interface available,
while you set the version that you implement. Currently those match,
but if/when we expand the API (bumping the version in the header) and
rebuild your program we will crash.

Before you ask - yes current libdrm users are not doing the right
thing and should be updated one of these days.

-Emil
P.S. Having some deja-vu here... I thought I mentioned this a while back.

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Tobias Jakobi]

Hello Emil,


Emil Velikov wrote:
> On 30 October 2015 at 11:16, Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de> wrote:
>> Hello Hyungwon,
>>
>> first of all thanks for reviewing the series!
>>
>>
>>
>> Hyungwon Hwang wrote:
>>> On Tue, 22 Sep 2015 17:54:55 +0200
>>> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>>
> 
>>>> +    evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>>> +    evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>>
>>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>>> versions are bumped, the event will contains wrong version info.
>> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
>> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
>> version in the public header is bumped, then it's also bumped here. So I
>> don't see the issue.
>>
> The issue is that the public header defines the interface available,
> while you set the version that you implement. Currently those match,
> but if/when we expand the API (bumping the version in the header) and
> rebuild your program we will crash.
Hmm, I'm still not sure I understand this. Do you mean rebuilding the
tests out-of-tree and then running them against a newer/older libdrm
version?

Because from my understanding the tests are always build together with
libdrm anyway. Or am I misunderstanding here something?


> Before you ask - yes current libdrm users are not doing the right
> thing and should be updated one of these days.
Maybe a dumb question, but what would be the right thing to do in may case.

Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?

With best wishes,
Tobias



> -Emil
> P.S. Having some deja-vu here... I thought I mentioned this a while back.
> 

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

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 11:28, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
> Hello Emil,
>
>
> Emil Velikov wrote:
>> On 30 October 2015 at 11:16, Tobias Jakobi
>> <tjakobi@math.uni-bielefeld.de> wrote:
>>> Hello Hyungwon,
>>>
>>> first of all thanks for reviewing the series!
>>>
>>>
>>>
>>> Hyungwon Hwang wrote:
>>>> On Tue, 22 Sep 2015 17:54:55 +0200
>>>> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>>>
>>
>>>>> +    evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>>>> +    evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>>>
>>>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>>>> versions are bumped, the event will contains wrong version info.
>>> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
>>> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
>>> version in the public header is bumped, then it's also bumped here. So I
>>> don't see the issue.
>>>
>> The issue is that the public header defines the interface available,
>> while you set the version that you implement. Currently those match,
>> but if/when we expand the API (bumping the version in the header) and
>> rebuild your program we will crash.
> Hmm, I'm still not sure I understand this. Do you mean rebuilding the
> tests out-of-tree and then running them against a newer/older libdrm
> version?
>
> Because from my understanding the tests are always build together with
> libdrm anyway. Or am I misunderstanding here something?
>
We're not talking about building out of tree or anything like that
here. The following example should illustrate what I have in mind.
Greatly appreciated if you can explain it in your own words.


Currently

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 2
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
}

* user
struct foo {
   .version = ...VERSION // 2
   ...
}


API bump

* xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 3
struct drmEventContext {
   int version;
   ...
   void (*page_flip_handler)(...);
   void (*page_flip_handler2)(...);
}

* user (hasn't been updated, only rebuilt)
struct foo {
   .version = ...VERSION // 3
   ...
   .page_flip_handler2 = 0 // ... or garbage.
}


* xf86drmMode.c
int drmHandleEvent(...)
{
...
if (foo.version >= 3)
   foo.page_flip_handler2(); // boom !
else
   foo.page_flip_handler();
...
}


Also worth mentioning is that the issue is rather wide spread rather
than limited to libdrm :'(

>
>> Before you ask - yes current libdrm users are not doing the right
>> thing and should be updated one of these days.
> Maybe a dumb question, but what would be the right thing to do in may case.
>
> Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?
>
No need for extra defines imho. Just set the versions to 2 and 1 for
evctx.base.version and evctx.version respectively.


Glad to see some of the Samsung/Exynos people looking this way :-)

Regards,
Emil

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Tobias Jakobi]

Hey Emil,


Emil Velikov wrote:
> On 30 October 2015 at 11:28, Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de> wrote:
>> Hello Emil,
>>
>>
>> Emil Velikov wrote:
>>> On 30 October 2015 at 11:16, Tobias Jakobi
>>> <tjakobi@math.uni-bielefeld.de> wrote:
>>>> Hello Hyungwon,
>>>>
>>>> first of all thanks for reviewing the series!
>>>>
>>>>
>>>>
>>>> Hyungwon Hwang wrote:
>>>>> On Tue, 22 Sep 2015 17:54:55 +0200
>>>>> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>>>>
>>>
>>>>>> +    evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
>>>>>> +    evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
>>>>>
>>>>> The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the
>>>>> versions are bumped, the event will contains wrong version info.
>>>> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
>>>> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the
>>>> version in the public header is bumped, then it's also bumped here. So I
>>>> don't see the issue.
>>>>
>>> The issue is that the public header defines the interface available,
>>> while you set the version that you implement. Currently those match,
>>> but if/when we expand the API (bumping the version in the header) and
>>> rebuild your program we will crash.
>> Hmm, I'm still not sure I understand this. Do you mean rebuilding the
>> tests out-of-tree and then running them against a newer/older libdrm
>> version?
>>
>> Because from my understanding the tests are always build together with
>> libdrm anyway. Or am I misunderstanding here something?
>>
> We're not talking about building out of tree or anything like that
> here. The following example should illustrate what I have in mind.
> Greatly appreciated if you can explain it in your own words.
> 
> 
> Currently
> 
> * xf86drm.h
> #define DRM_EVENT_CONTEXT_VERSION 2
> struct drmEventContext {
>    int version;
>    ...
>    void (*page_flip_handler)(...);
> }
> 
> * user
> struct foo {
>    .version = ...VERSION // 2
>    ...
> }
> 
> 
> API bump
> 
> * xf86drm.h
> #define DRM_EVENT_CONTEXT_VERSION 3
> struct drmEventContext {
>    int version;
>    ...
>    void (*page_flip_handler)(...);
>    void (*page_flip_handler2)(...);
> }
> 
> * user (hasn't been updated, only rebuilt)
> struct foo {
>    .version = ...VERSION // 3
>    ...
>    .page_flip_handler2 = 0 // ... or garbage.
> }
> 
> 
> * xf86drmMode.c
> int drmHandleEvent(...)
> {
> ...
> if (foo.version >= 3)
>    foo.page_flip_handler2(); // boom !
> else
>    foo.page_flip_handler();
> ...
> }
> 
> 
> Also worth mentioning is that the issue is rather wide spread rather
> than limited to libdrm :'(
OK, I see what you mean. However this shouldn't happen as long as the
user properly zero initializes the event context structures, right?



>>> Before you ask - yes current libdrm users are not doing the right
>>> thing and should be updated one of these days.
>> Maybe a dumb question, but what would be the right thing to do in may case.
>>
>> Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?
>>
> No need for extra defines imho. Just set the versions to 2 and 1 for
> evctx.base.version and evctx.version respectively.
OK, going to do this then. In any case, can the user assume that the
event structures only "grow", in the sense that new fields are added to it?


With best wishes,
Tobias


> Glad to see some of the Samsung/Exynos people looking this way :-)
> 
> Regards,
> Emil
> 

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

Re: [PATCH 08/13] exynos: fimg2d: add g2d_set_direction

[Tobias Jakobi]

Tobias Jakobi wrote:
> Hello Hyungwon,
> 
> 
> Hyungwon Hwang wrote:
>> On Tue, 22 Sep 2015 17:54:57 +0200
>> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>
>>> This allows setting the two direction registers, which specify how
>>> the engine blits pixels. This can be used for overlapping blits,
>>> which happen e.g. when 'moving' a rectangular region inside a
>>> fixed buffer.
>>
>> Code itself looks good. But as I know, direction registers are related
>> with flip, not moving. Isn't it? I am not that much familiar with G2D.
>> Please let me know if I am wrong.
> that's correct, you can use the direction registers to implement
> flipping operations.
> 
> However what they really do is to change the operation direction of the
> G2D engine. So you can manage how the engine traverses through pixel
> rows and column (either in forward or in backward direction).
> 
> Normally this doesn't matter, like it doesn't matter if you memcpy()
> from one buffer to another when there is no intersection of the buffers.
> However if the two buffers overlap then you have to be careful with the
> direction in which you traverse the buffer.
> 
> Now if you set the x-direction of the source G2D image to forward, but
> the x-direction of the destination to backward, you get your mentioned
> x-flipping.
> 
> The main purpose for g2d_move() is to be used e.g. in acceleration code
> for the armsoc Xorg DDX. A common operation there is not move pixel
> regions around in the same buffer.
Sorry, this should read "...there is to move pixel regions...".


- Tobias

> 
> 
> With best wishes,
> Tobias
> 
> 
> P.S.: I think the Exynos user manual mentions flipping as one example on
> how to use these registers. But like I said above, it's just one way to
> use it.
> 
> 
> 
> 
>>
>> Best regards,
>> Hyungwon Hwang
>>
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  exynos/exynos_fimg2d.c | 13 +++++++++++++
>>>  exynos/exynos_fimg2d.h | 38 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 51 insertions(+)
>>>
>>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>>> index e997d4b..4d5419c 100644
>>> --- a/exynos/exynos_fimg2d.c
>>> +++ b/exynos/exynos_fimg2d.c
>>> @@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct g2d_context
>>> *ctx, struct g2d_image *img, }
>>>  
>>>  /*
>>> + * g2d_set_direction - setup direction register (useful for
>>> overlapping blits).
>>> + *
>>> + * @ctx: a pointer to g2d_context structure.
>>> + * @dir: a pointer to the g2d_direction_val structure.
>>> + */
>>> +static void g2d_set_direction(struct g2d_context *ctx,
>>> +			const union g2d_direction_val *dir)
>>> +{
>>> +	g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
>>> +	g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
>>> +}
>>> +
>>> +/*
>>>   * g2d_reset - reset fimg2d hardware.
>>>   *
>>>   * @ctx: a pointer to g2d_context structure.
>>> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
>>> index c6b58ac..9eee7c0 100644
>>> --- a/exynos/exynos_fimg2d.h
>>> +++ b/exynos/exynos_fimg2d.h
>>> @@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
>>>  	G2D_EXEC_FLAG_ASYNC = (1 << 0)
>>>  };
>>>  
>>> +enum e_g2d_dir_mode {
>>> +	G2D_DIR_MODE_POSITIVE = 0,
>>> +	G2D_DIR_MODE_NEGATIVE = 1
>>> +};
>>> +
>>>  union g2d_point_val {
>>>  	unsigned int val;
>>>  	struct {
>>> @@ -269,6 +274,39 @@ union g2d_blend_func_val {
>>>  	} data;
>>>  };
>>>  
>>> +union g2d_direction_val {
>>> +	unsigned int val[2];
>>> +	struct {
>>> +		/* SRC_MSK_DIRECT_REG [0:1] (source) */
>>> +		enum e_g2d_dir_mode		src_x_direction:1;
>>> +		enum e_g2d_dir_mode		src_y_direction:1;
>>> +
>>> +		/* SRC_MSK_DIRECT_REG [2:3] */
>>> +		unsigned int			reversed1:2;
>>> +
>>> +		/* SRC_MSK_DIRECT_REG [4:5] (mask) */
>>> +		enum e_g2d_dir_mode
>>> mask_x_direction:1;
>>> +		enum e_g2d_dir_mode
>>> mask_y_direction:1; +
>>> +		/* SRC_MSK_DIRECT_REG [6:31] */
>>> +		unsigned int			padding1:26;
>>> +
>>> +		/* DST_PAT_DIRECT_REG [0:1] (destination) */
>>> +		enum e_g2d_dir_mode		dst_x_direction:1;
>>> +		enum e_g2d_dir_mode		dst_y_direction:1;
>>> +
>>> +		/* DST_PAT_DIRECT_REG [2:3] */
>>> +		unsigned int			reversed2:2;
>>> +
>>> +		/* DST_PAT_DIRECT_REG [4:5] (pattern) */
>>> +		enum e_g2d_dir_mode		pat_x_direction:1;
>>> +		enum e_g2d_dir_mode		pat_y_direction:1;
>>> +
>>> +		/* DST_PAT_DIRECT_REG [6:31] */
>>> +		unsigned int			padding2:26;
>>> +	} data;
>>> +};
>>> +
>>>  struct g2d_image {
>>>  	enum e_g2d_select_mode		select_mode;
>>>  	enum e_g2d_color_mode		color_mode;
>>
> 
> --
> 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
> 

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Emil Velikov]

On 30 October 2015 at 14:28, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:

> OK, I see what you mean. However this shouldn't happen as long as the
> user properly zero initializes the event context structures, right?
>
It pains me to say it but in this day and age, not everyone zero
initializes their structs (be that via C99 initailizers, memset or
just {}).

> OK, going to do this then. In any case, can the user assume that the
> event structures only "grow", in the sense that new fields are added to it?
>
That is correct. Note that this approach is also used elsewhere - the
DRI driver <> loader model, also the drm ioctls structs are extended
in a similar way (minus the actual version field).

With the current knowledge in hand, perhaps we could have done things
differently... But that's another story :-)

Regards,
Emil

Re: [PATCH 06/13] tests/exynos: add fimg2d event test

[Hyungwon Hwang]

On Fri, 30 Oct 2015 12:16:47 +0100
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> Hello Hyungwon,
> 
> first of all thanks for reviewing the series!
> 
> 
> 
> Hyungwon Hwang wrote:
> > On Tue, 22 Sep 2015 17:54:55 +0200
> > Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> > 
> >> This tests async processing of G2D jobs. A separate thread is
> >> spawned to monitor the DRM fd for events and check whether a G2D
> >> job was completed.
> >>
> >> v2: Add GPLv2 header, argument handling and documentation.
> >>     Test is only installed when requested.
> >> v3: Allocate G2D jobs with calloc which fixes 'busy' being
> >>     potentially uninitialized. Also enable timeout for poll()
> >>     in the monitor thread. This fixes pthread_join() not working
> >>     because of poll() not returning.
> >>
> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> >> ---
> >>  tests/exynos/Makefile.am           |  11 +-
> >>  tests/exynos/exynos_fimg2d_event.c | 326
> >> +++++++++++++++++++++++++++++++++++++ 2 files changed, 335
> >> insertions(+), 2 deletions(-) create mode 100644
> >> tests/exynos/exynos_fimg2d_event.c
> >>
> >> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
> >> index e82d199..357d6b8 100644
> >> --- a/tests/exynos/Makefile.am
> >> +++ b/tests/exynos/Makefile.am
> >> @@ -20,16 +20,23 @@ endif
> >>  
> >>  if HAVE_INSTALL_TESTS
> >>  bin_PROGRAMS += \
> >> -	exynos_fimg2d_perf
> >> +	exynos_fimg2d_perf \
> >> +	exynos_fimg2d_event
> >>  else
> >>  noinst_PROGRAMS += \
> >> -	exynos_fimg2d_perf
> >> +	exynos_fimg2d_perf \
> >> +	exynos_fimg2d_event
> >>  endif
> >>  
> >>  exynos_fimg2d_perf_LDADD = \
> >>  	$(top_builddir)/libdrm.la \
> >>  	$(top_builddir)/exynos/libdrm_exynos.la
> >>  
> >> +exynos_fimg2d_event_LDADD = \
> >> +	$(top_builddir)/libdrm.la \
> >> +	$(top_builddir)/exynos/libdrm_exynos.la \
> >> +	-lpthread
> >> +
> >>  exynos_fimg2d_test_LDADD = \
> >>  	$(top_builddir)/libdrm.la \
> >>  	$(top_builddir)/libkms/libkms.la \
> >> diff --git a/tests/exynos/exynos_fimg2d_event.c
> >> b/tests/exynos/exynos_fimg2d_event.c new file mode 100644
> >> index 0000000..c03dcff
> >> --- /dev/null
> >> +++ b/tests/exynos/exynos_fimg2d_event.c
> >> @@ -0,0 +1,326 @@
> >> +/*
> >> + * Copyright (C) 2015 - Tobias Jakobi
> >> + *
> >> + * This is free software: you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as
> >> published
> >> + * by the Free Software Foundation, either version 2 of the
> >> License,
> >> + * or (at your option) any later version.
> >> + *
> >> + * It is distributed in the hope that it will be useful, but
> >> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + * You should have received a copy of the GNU General Public
> >> License
> >> + * along with it. If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include <unistd.h>
> >> +#include <poll.h>
> >> +
> >> +#include <stdlib.h>
> >> +#include <stdio.h>
> >> +#include <time.h>
> >> +#include <getopt.h>
> >> +
> >> +#include <pthread.h>
> >> +
> >> +#include <xf86drm.h>
> >> +
> >> +#include "exynos_drm.h"
> >> +#include "exynos_drmif.h"
> >> +#include "exynos_fimg2d.h"
> >> +
> >> +struct g2d_job {
> >> +	unsigned int id;
> >> +	unsigned int busy;
> >> +};
> >> +
> >> +struct exynos_evhandler {
> >> +	struct pollfd fds;
> >> +	struct exynos_event_context evctx;
> >> +};
> >> +
> >> +struct threaddata {
> >> +	unsigned int stop;
> >> +	struct exynos_device *dev;
> >> +	struct exynos_evhandler evhandler;
> >> +};
> >> +
> >> +static void g2d_event_handler(int fd, unsigned int cmdlist_no,
> >> unsigned int tv_sec,
> >> +							unsigned
> >> int tv_usec, void *user_data) +{
> >> +	struct g2d_job *job = user_data;
> >> +
> >> +	fprintf(stderr, "info: g2d job (id = %u, cmdlist number =
> >> %u) finished!\n",
> >> +			job->id, cmdlist_no);
> >> +
> >> +	job->busy = 0;
> >> +}
> >> +
> >> +static void setup_g2d_event_handler(struct exynos_evhandler
> >> *evhandler, int fd) +{
> >> +	evhandler->fds.fd = fd;
> >> +	evhandler->fds.events = POLLIN;
> >> +	evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
> >> +	evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
> > 
> > The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After
> > the versions are bumped, the event will contains wrong version info.
> Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and
> EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If
> the version in the public header is bumped, then it's also bumped
> here. So I don't see the issue.
> 
> 
> > Also, I think the type of event must be set here.
> What do you mean by 'type of event' here? I don't see a type field in
> the event context structures.

Forget about it. I was confused about that. I think that it would be
just OK if the issue related with version is fixed.

Best regards,
Hyungwon Hwang

> 
> 
> With best wishes,
> Tobias
> 
> 
> > 
> > Best regards,
> > Hyungwon Hwang
> > 
> >> +	evhandler->evctx.g2d_event_handler = g2d_event_handler;
> >> +}
> >> +
> >> +static void* threadfunc(void *arg) {
> >> +	const int timeout = 0;
> >> +	struct threaddata *data;
> >> +
> >> +	data = arg;
> >> +
> >> +	while (1) {
> >> +		if (data->stop) break;
> >> +
> >> +		usleep(500);
> >> +
> >> +		data->evhandler.fds.revents = 0;
> >> +
> >> +		if (poll(&data->evhandler.fds, 1, timeout) < 0)
> >> +			continue;
> >> +
> >> +		if (data->evhandler.fds.revents & (POLLHUP |
> >> POLLERR))
> >> +			continue;
> >> +
> >> +		if (data->evhandler.fds.revents & POLLIN)
> >> +			exynos_handle_event(data->dev,
> >> &data->evhandler.evctx);
> >> +	}
> >> +
> >> +	pthread_exit(0);
> >> +}
> >> +
> >> +/*
> >> + * We need to wait until all G2D jobs are finished, otherwise we
> >> + * potentially remove a BO which the engine still operates on.
> >> + * This results in the following kernel message:
> >> + * [drm:exynos_drm_gem_put_dma_addr] *ERROR* failed to lookup gem
> >> object.
> >> + * Also any subsequent BO allocations fail then with:
> >> + * [drm:exynos_drm_alloc_buf] *ERROR* failed to allocate buffer.
> >> + */
> >> +static void wait_all_jobs(struct g2d_job* jobs, unsigned num_jobs)
> >> +{
> >> +	unsigned i;
> >> +
> >> +	for (i = 0; i < num_jobs; ++i) {
> >> +		while (jobs[i].busy)
> >> +			usleep(500);
> >> +	}
> >> +
> >> +}
> >> +
> >> +static struct g2d_job* free_job(struct g2d_job* jobs, unsigned
> >> num_jobs) +{
> >> +	unsigned i;
> >> +
> >> +	for (i = 0; i < num_jobs; ++i) {
> >> +		if (jobs[i].busy == 0)
> >> +			return &jobs[i];
> >> +	}
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >> +static int g2d_work(struct g2d_context *ctx, struct g2d_image
> >> *img,
> >> +					unsigned num_jobs,
> >> unsigned iterations) +{
> >> +	struct g2d_job *jobs = calloc(num_jobs, sizeof(struct
> >> g2d_job));
> >> +	int ret;
> >> +	unsigned i;
> >> +
> >> +	/* setup job ids */
> >> +	for (i = 0; i < num_jobs; ++i)
> >> +		jobs[i].id = i;
> >> +
> >> +	for (i = 0; i < iterations; ++i) {
> >> +		unsigned x, y, w, h;
> >> +
> >> +		struct g2d_job *j = NULL;
> >> +
> >> +		while (1) {
> >> +			j = free_job(jobs, num_jobs);
> >> +
> >> +			if (j)
> >> +				break;
> >> +			else
> >> +				usleep(500);
> >> +		}
> >> +
> >> +		x = rand() % img->width;
> >> +		y = rand() % img->height;
> >> +
> >> +		if (x == (img->width - 1))
> >> +			x -= 1;
> >> +		if (y == (img->height - 1))
> >> +			y -= 1;
> >> +
> >> +		w = rand() % (img->width - x);
> >> +		h = rand() % (img->height - y);
> >> +
> >> +		if (w == 0) w = 1;
> >> +		if (h == 0) h = 1;
> >> +
> >> +		img->color = rand();
> >> +
> >> +		j->busy = 1;
> >> +		g2d_config_event(ctx, j);
> >> +
> >> +		ret = g2d_solid_fill(ctx, img, x, y, w, h);
> >> +
> >> +		if (ret == 0)
> >> +			g2d_exec2(ctx, G2D_EXEC_FLAG_ASYNC);
> >> +
> >> +		if (ret != 0) {
> >> +			fprintf(stderr, "error: iteration %u (x =
> >> %u, x = %u, x = %u, x = %u) failed\n",
> >> +					i, x, y, w, h);
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	wait_all_jobs(jobs, num_jobs);
> >> +	free(jobs);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void usage(const char *name)
> >> +{
> >> +	fprintf(stderr, "usage: %s [-ijwh]\n\n", name);
> >> +
> >> +	fprintf(stderr, "\t-i <number of iterations>\n");
> >> +	fprintf(stderr, "\t-j <number of G2D jobs> (default =
> >> 4)\n\n"); +
> >> +	fprintf(stderr, "\t-w <buffer width> (default = 4096)\n");
> >> +	fprintf(stderr, "\t-h <buffer height> (default =
> >> 4096)\n"); +
> >> +	exit(0);
> >> +}
> >> +
> >> +int main(int argc, char **argv)
> >> +{
> >> +	int fd, ret, c, parsefail;
> >> +
> >> +	pthread_t event_thread;
> >> +	struct threaddata event_data = {0};
> >> +
> >> +	struct exynos_device *dev;
> >> +	struct g2d_context *ctx;
> >> +	struct exynos_bo *bo;
> >> +
> >> +	struct g2d_image img = {0};
> >> +
> >> +	unsigned int iters = 0, njobs = 4;
> >> +	unsigned int bufw = 4096, bufh = 4096;
> >> +
> >> +	ret = 0;
> >> +	parsefail = 0;
> >> +
> >> +	while ((c = getopt(argc, argv, "i:j:w:h:")) != -1) {
> >> +		switch (c) {
> >> +		case 'i':
> >> +			if (sscanf(optarg, "%u", &iters) != 1)
> >> +				parsefail = 1;
> >> +			break;
> >> +		case 'j':
> >> +			if (sscanf(optarg, "%u", &njobs) != 1)
> >> +				parsefail = 1;
> >> +			break;
> >> +		case 'w':
> >> +			if (sscanf(optarg, "%u", &bufw) != 1)
> >> +				parsefail = 1;
> >> +			break;
> >> +		case 'h':
> >> +			if (sscanf(optarg, "%u", &bufh) != 1)
> >> +				parsefail = 1;
> >> +			break;
> >> +		default:
> >> +			parsefail = 1;
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	if (parsefail || (argc == 1) || (iters == 0))
> >> +		usage(argv[0]);
> >> +
> >> +	if (bufw > 4096 || bufh > 4096) {
> >> +		fprintf(stderr, "error: buffer width/height should
> >> be less than 4096.\n");
> >> +		ret = -1;
> >> +
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (bufw == 0 || bufh == 0) {
> >> +		fprintf(stderr, "error: buffer width/height should
> >> be non-zero.\n");
> >> +		ret = -1;
> >> +
> >> +		goto out;
> >> +	}
> >> +
> >> +	fd = drmOpen("exynos", NULL);
> >> +	if (fd < 0) {
> >> +		fprintf(stderr, "error: failed to open drm\n");
> >> +		ret = -1;
> >> +
> >> +		goto out;
> >> +	}
> >> +
> >> +	dev = exynos_device_create(fd);
> >> +	if (dev == NULL) {
> >> +		fprintf(stderr, "error: failed to create
> >> device\n");
> >> +		ret = -2;
> >> +
> >> +		goto fail;
> >> +	}
> >> +
> >> +	ctx = g2d_init(fd);
> >> +	if (ctx == NULL) {
> >> +		fprintf(stderr, "error: failed to init G2D\n");
> >> +		ret = -3;
> >> +
> >> +		goto g2d_fail;
> >> +	}
> >> +
> >> +	bo = exynos_bo_create(dev, bufw * bufh * 4, 0);
> >> +	if (bo == NULL) {
> >> +		fprintf(stderr, "error: failed to create bo\n");
> >> +		ret = -4;
> >> +
> >> +		goto bo_fail;
> >> +	}
> >> +
> >> +	/* setup g2d image object */
> >> +	img.width = bufw;
> >> +	img.height = bufh;
> >> +	img.stride = bufw * 4;
> >> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
> >> +	img.buf_type = G2D_IMGBUF_GEM;
> >> +	img.bo[0] = bo->handle;
> >> +
> >> +	event_data.dev = dev;
> >> +	setup_g2d_event_handler(&event_data.evhandler, fd);
> >> +
> >> +	pthread_create(&event_thread, NULL, threadfunc,
> >> &event_data); +
> >> +	ret = g2d_work(ctx, &img, njobs, iters);
> >> +	if (ret != 0)
> >> +		fprintf(stderr, "error: g2d_work failed\n");
> >> +
> >> +	event_data.stop = 1;
> >> +	pthread_join(event_thread, NULL);
> >> +
> >> +	exynos_bo_destroy(bo);
> >> +
> >> +bo_fail:
> >> +	g2d_fini(ctx);
> >> +
> >> +g2d_fail:
> >> +	exynos_device_destroy(dev);
> >> +
> >> +fail:
> >> +	drmClose(fd);
> >> +
> >> +out:
> >> +	return ret;
> >> +}
> > 
> 

Re: [PATCH 07/13] tests/exynos: use XRGB8888 for framebuffer

[Hyungwon Hwang]

On Fri, 30 Oct 2015 12:17:02 +0100
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> Hello Hyungwon,
> 
> 
> Hyungwon Hwang wrote:
> > On Tue, 22 Sep 2015 17:54:56 +0200
> > Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> > 
> >> This matches the G2D color mode that is used in the entire code.
> >> The previous (incorrect) RGBA8888 would only work since the
> >> Exynos mixer did its configuration based on the bpp, and not
> >> based on the actual pixelformat.
> >>
> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> >> ---
> >>  tests/exynos/exynos_fimg2d_test.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tests/exynos/exynos_fimg2d_test.c
> >> b/tests/exynos/exynos_fimg2d_test.c index 8794dac..dfb00a0 100644
> >> --- a/tests/exynos/exynos_fimg2d_test.c
> >> +++ b/tests/exynos/exynos_fimg2d_test.c
> >> @@ -675,7 +675,7 @@ int main(int argc, char **argv)
> >>  	offsets[0] = 0;
> >>  
> >>  	ret = drmModeAddFB2(dev->fd, screen_width, screen_height,
> >> -				DRM_FORMAT_RGBA8888, handles,
> >> +				DRM_FORMAT_XRGB8888, handles,
> >>  				pitches, offsets, &fb_id, 0);
> > 
> > Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>
> > 
> > Nice catch. It's right, if there was no previous setting for source
> > image color mode. But I think it could be the source image color
> > mode was set by another application before when this test runs. So
> > I think the code which sets the source image color mode must be
> > added.
> I think you misunderstand something here. First of all settings from
> anither application using DRM don't carry over.
> 
> The point for this change is that all used G2D image structures have
> the color_mode field set to G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB.
> So the G2D is operating on ARGB8888 pixel data.
> 
> However the framebuffer is set to DRM_FORMAT_RGBA8888, which obviously
> is wrong since this is not the type of data we put into the
> framebuffer.

Oh. That's right. I misunderstanded the code. This patch is absolutely
right.

Best regards,
Hyungwon Hwang

> 
> 
> With best wishes,
> Tobias
> 
> 
> > 
> > Best regards,
> > Hyungwon Hwang
> > 
> > 
> >>  	if (ret < 0)
> >>  		goto err_destroy_buffer;
> > 
> 

Re: [PATCH 08/13] exynos: fimg2d: add g2d_set_direction

[Hyungwon Hwang]

On Fri, 30 Oct 2015 18:14:23 +0100
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> Tobias Jakobi wrote:
> > Hello Hyungwon,
> > 
> > 
> > Hyungwon Hwang wrote:
> >> On Tue, 22 Sep 2015 17:54:57 +0200
> >> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> >>
> >>> This allows setting the two direction registers, which specify how
> >>> the engine blits pixels. This can be used for overlapping blits,
> >>> which happen e.g. when 'moving' a rectangular region inside a
> >>> fixed buffer.
> >>
> >> Code itself looks good. But as I know, direction registers are
> >> related with flip, not moving. Isn't it? I am not that much
> >> familiar with G2D. Please let me know if I am wrong.
> > that's correct, you can use the direction registers to implement
> > flipping operations.
> > 
> > However what they really do is to change the operation direction of
> > the G2D engine. So you can manage how the engine traverses through
> > pixel rows and column (either in forward or in backward direction).
> > 
> > Normally this doesn't matter, like it doesn't matter if you memcpy()
> > from one buffer to another when there is no intersection of the
> > buffers. However if the two buffers overlap then you have to be
> > careful with the direction in which you traverse the buffer.
> > 
> > Now if you set the x-direction of the source G2D image to forward,
> > but the x-direction of the destination to backward, you get your
> > mentioned x-flipping.
> > 
> > The main purpose for g2d_move() is to be used e.g. in acceleration
> > code for the armsoc Xorg DDX. A common operation there is not move
> > pixel regions around in the same buffer.
> Sorry, this should read "...there is to move pixel regions...".

OK. I understood what you meant.

Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>


Best regards,
Hyungwon Hwang

> 
> 
> - Tobias
> 
> > 
> > 
> > With best wishes,
> > Tobias
> > 
> > 
> > P.S.: I think the Exynos user manual mentions flipping as one
> > example on how to use these registers. But like I said above, it's
> > just one way to use it.
> > 
> > 
> > 
> > 
> >>
> >> Best regards,
> >> Hyungwon Hwang
> >>
> >>>
> >>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> >>> ---
> >>>  exynos/exynos_fimg2d.c | 13 +++++++++++++
> >>>  exynos/exynos_fimg2d.h | 38
> >>> ++++++++++++++++++++++++++++++++++++++ 2 files changed, 51
> >>> insertions(+)
> >>>
> >>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
> >>> index e997d4b..4d5419c 100644
> >>> --- a/exynos/exynos_fimg2d.c
> >>> +++ b/exynos/exynos_fimg2d.c
> >>> @@ -242,6 +242,19 @@ static void g2d_add_base_addr(struct
> >>> g2d_context *ctx, struct g2d_image *img, }
> >>>  
> >>>  /*
> >>> + * g2d_set_direction - setup direction register (useful for
> >>> overlapping blits).
> >>> + *
> >>> + * @ctx: a pointer to g2d_context structure.
> >>> + * @dir: a pointer to the g2d_direction_val structure.
> >>> + */
> >>> +static void g2d_set_direction(struct g2d_context *ctx,
> >>> +			const union g2d_direction_val *dir)
> >>> +{
> >>> +	g2d_add_cmd(ctx, SRC_MASK_DIRECT_REG, dir->val[0]);
> >>> +	g2d_add_cmd(ctx, DST_PAT_DIRECT_REG, dir->val[1]);
> >>> +}
> >>> +
> >>> +/*
> >>>   * g2d_reset - reset fimg2d hardware.
> >>>   *
> >>>   * @ctx: a pointer to g2d_context structure.
> >>> diff --git a/exynos/exynos_fimg2d.h b/exynos/exynos_fimg2d.h
> >>> index c6b58ac..9eee7c0 100644
> >>> --- a/exynos/exynos_fimg2d.h
> >>> +++ b/exynos/exynos_fimg2d.h
> >>> @@ -189,6 +189,11 @@ enum e_g2d_exec_flag {
> >>>  	G2D_EXEC_FLAG_ASYNC = (1 << 0)
> >>>  };
> >>>  
> >>> +enum e_g2d_dir_mode {
> >>> +	G2D_DIR_MODE_POSITIVE = 0,
> >>> +	G2D_DIR_MODE_NEGATIVE = 1
> >>> +};
> >>> +
> >>>  union g2d_point_val {
> >>>  	unsigned int val;
> >>>  	struct {
> >>> @@ -269,6 +274,39 @@ union g2d_blend_func_val {
> >>>  	} data;
> >>>  };
> >>>  
> >>> +union g2d_direction_val {
> >>> +	unsigned int val[2];
> >>> +	struct {
> >>> +		/* SRC_MSK_DIRECT_REG [0:1] (source) */
> >>> +		enum e_g2d_dir_mode
> >>> src_x_direction:1;
> >>> +		enum e_g2d_dir_mode
> >>> src_y_direction:1; +
> >>> +		/* SRC_MSK_DIRECT_REG [2:3] */
> >>> +		unsigned int			reversed1:2;
> >>> +
> >>> +		/* SRC_MSK_DIRECT_REG [4:5] (mask) */
> >>> +		enum e_g2d_dir_mode
> >>> mask_x_direction:1;
> >>> +		enum e_g2d_dir_mode
> >>> mask_y_direction:1; +
> >>> +		/* SRC_MSK_DIRECT_REG [6:31] */
> >>> +		unsigned int			padding1:26;
> >>> +
> >>> +		/* DST_PAT_DIRECT_REG [0:1] (destination) */
> >>> +		enum e_g2d_dir_mode
> >>> dst_x_direction:1;
> >>> +		enum e_g2d_dir_mode
> >>> dst_y_direction:1; +
> >>> +		/* DST_PAT_DIRECT_REG [2:3] */
> >>> +		unsigned int			reversed2:2;
> >>> +
> >>> +		/* DST_PAT_DIRECT_REG [4:5] (pattern) */
> >>> +		enum e_g2d_dir_mode
> >>> pat_x_direction:1;
> >>> +		enum e_g2d_dir_mode
> >>> pat_y_direction:1; +
> >>> +		/* DST_PAT_DIRECT_REG [6:31] */
> >>> +		unsigned int			padding2:26;
> >>> +	} data;
> >>> +};
> >>> +
> >>>  struct g2d_image {
> >>>  	enum e_g2d_select_mode		select_mode;
> >>>  	enum e_g2d_color_mode		color_mode;
> >>
> > 
> > --
> > 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
> > 
> 

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

[Hyungwon Hwang]

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,

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

Re: [PATCH 10/13] tests/exynos: add test for g2d_move

[Hyungwon Hwang]

Hello,

I think this patch should update .gitignore, not for adding the built
binary to untracked file list.

But without it, it looks good to me, and I tested it on my Odroid U3
board.

Tested-by: Hyungwon Hwang <human.hwang@samsung.com>
Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>

Best regards,
Hyungwon Hwang


On Tue, 22 Sep 2015 17:54:59 +0200
Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:

> 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.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  tests/exynos/exynos_fimg2d_test.c | 132
> ++++++++++++++++++++++++++++++++++++++ 1 file changed, 132
> insertions(+)
> 
> diff --git a/tests/exynos/exynos_fimg2d_test.c
> b/tests/exynos/exynos_fimg2d_test.c index dfb00a0..797fb6e 100644
> --- a/tests/exynos/exynos_fimg2d_test.c
> +++ b/tests/exynos/exynos_fimg2d_test.c
> @@ -313,6 +313,130 @@ fail:
>  	return ret;
>  }
>  
> +static int g2d_move_test(struct exynos_device *dev,
> +				struct exynos_bo *tmp,
> +				struct exynos_bo *buf,
> +				enum e_g2d_buf_type type)
> +{
> +	struct g2d_context *ctx;
> +	struct g2d_image img = {0}, tmp_img = {0};
> +	unsigned int img_w, img_h, count;
> +	int cur_x, cur_y;
> +	void *checkerboard;
> +	int ret;
> +
> +	static const struct g2d_step {
> +		int x, y;
> +	} steps[] = {
> +		{ 1,  0}, { 0,  1},
> +		{-1,  0}, { 0, -1},
> +		{ 1,  1}, {-1, -1},
> +		{ 1, -1}, {-1,  1},
> +		{ 2,  1}, { 1,  2},
> +		{-2, -1}, {-1, -2},
> +		{ 2, -1}, { 1, -2},
> +		{-2,  1}, {-1,  2}
> +	};
> +	static const unsigned int num_steps =
> +		sizeof(steps) / sizeof(struct g2d_step);
> +
> +	ctx = g2d_init(dev->fd);
> +	if (!ctx)
> +		return -EFAULT;
> +
> +	img.bo[0] = buf->handle;
> +
> +	/* create pattern of half the screen size */
> +	checkerboard = create_checkerboard_pattern(screen_width /
> 64, screen_height / 64, 32);
> +	if (!checkerboard) {
> +		ret = -EFAULT;
> +		goto fail;
> +	}
> +
> +	img_w = (screen_width / 64) * 32;
> +	img_h = (screen_height / 64) * 32;
> +
> +	switch (type) {
> +	case G2D_IMGBUF_GEM:
> +		memcpy(tmp->vaddr, checkerboard, img_w * img_h * 4);
> +		tmp_img.bo[0] = tmp->handle;
> +		break;
> +	case G2D_IMGBUF_USERPTR:
> +		tmp_img.user_ptr[0].userptr = (unsigned
> long)checkerboard;
> +		tmp_img.user_ptr[0].size = img_w * img_h * 4;
> +		break;
> +	case G2D_IMGBUF_COLOR:
> +	default:
> +		ret = -EFAULT;
> +		goto fail;
> +	}
> +
> +	/* solid fill framebuffer with white color */
> +	img.width = screen_width;
> +	img.height = screen_height;
> +	img.stride = screen_width * 4;
> +	img.buf_type = G2D_IMGBUF_GEM;
> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
> +	img.color = 0xffffffff;
> +
> +	/* put checkerboard pattern in the center of the framebuffer
> */
> +	cur_x = (screen_width - img_w) / 2;
> +	cur_y = (screen_height - img_h) / 2;
> +	tmp_img.width = img_w;
> +	tmp_img.height = img_h;
> +	tmp_img.stride = img_w * 4;
> +	tmp_img.buf_type = type;
> +	tmp_img.color_mode = G2D_COLOR_FMT_ARGB8888 |
> G2D_ORDER_AXRGB; +
> +	ret = g2d_solid_fill(ctx, &img, 0, 0, screen_width,
> screen_height) ||
> +		g2d_copy(ctx, &tmp_img, &img, 0, 0, cur_x, cur_y,
> img_w, img_h); +
> +	if (!ret)
> +		ret = g2d_exec(ctx);
> +	if (ret < 0)
> +			goto fail;
> +
> +	printf("move test with %s.\n",
> +			type == G2D_IMGBUF_GEM ? "gem" : "userptr");
> +
> +	srand(time(NULL));
> +	for (count = 0; count < 256; ++count) {
> +		const struct g2d_step *s;
> +
> +		/* select step and validate it */
> +		while (1) {
> +			s = &steps[random() % num_steps];
> +
> +			if (cur_x + s->x < 0 || cur_y + s->y < 0 ||
> +				cur_x + img_w + s->x >= screen_width
> ||
> +				cur_y + img_h + s->y >=
> screen_height)
> +				continue;
> +			else
> +				break;
> +		}
> +
> +		ret = g2d_move(ctx, &img, cur_x, cur_y, cur_x +
> s->x, cur_y + s->y,
> +			img_w, img_h);
> +		if (!ret)
> +			ret = g2d_exec(ctx);
> +
> +		if (ret < 0)
> +			goto fail;
> +
> +		cur_x += s->x;
> +		cur_y += s->y;
> +
> +		usleep(100000);
> +	}
> +
> +fail:
> +	g2d_fini(ctx);
> +
> +	free(checkerboard);
> +
> +	return ret;
> +}
> +
>  static int g2d_copy_with_scale_test(struct exynos_device *dev,
>  					struct exynos_bo *src,
>  					struct exynos_bo *dst,
> @@ -708,6 +832,14 @@ int main(int argc, char **argv)
>  
>  	wait_for_user_input(0);
>  
> +	ret = g2d_move_test(dev, src, bo, G2D_IMGBUF_GEM);
> +	if (ret < 0) {
> +		fprintf(stderr, "failed to test move operation.\n");
> +		goto err_free_src;
> +	}
> +
> +	wait_for_user_input(0);
> +
>  	ret = g2d_copy_with_scale_test(dev, src, bo, G2D_IMGBUF_GEM);
>  	if (ret < 0) {
>  		fprintf(stderr, "failed to test copy and scale
> operation.\n");

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

[Tobias Jakobi]

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

Re: [PATCH 10/13] tests/exynos: add test for g2d_move

[Tobias Jakobi]

Hello Hyungwon,


Hyungwon Hwang wrote:
> Hello,
> 
> I think this patch should update .gitignore, not for adding the built
> binary to untracked file list.
good point. I should do this for the event test as well I guess.

Going to respin the series.


With best wishes,
Tobias


> But without it, it looks good to me, and I tested it on my Odroid U3
> board.
> 
> Tested-by: Hyungwon Hwang <human.hwang@samsung.com>
> Reviewed-by: Hyungwon Hwang <human.hwang@samsung.com>
> 
> Best regards,
> Hyungwon Hwang
> 
> 
> On Tue, 22 Sep 2015 17:54:59 +0200
> Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 
>> 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.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  tests/exynos/exynos_fimg2d_test.c | 132
>> ++++++++++++++++++++++++++++++++++++++ 1 file changed, 132
>> insertions(+)
>>
>> diff --git a/tests/exynos/exynos_fimg2d_test.c
>> b/tests/exynos/exynos_fimg2d_test.c index dfb00a0..797fb6e 100644
>> --- a/tests/exynos/exynos_fimg2d_test.c
>> +++ b/tests/exynos/exynos_fimg2d_test.c
>> @@ -313,6 +313,130 @@ fail:
>>  	return ret;
>>  }
>>  
>> +static int g2d_move_test(struct exynos_device *dev,
>> +				struct exynos_bo *tmp,
>> +				struct exynos_bo *buf,
>> +				enum e_g2d_buf_type type)
>> +{
>> +	struct g2d_context *ctx;
>> +	struct g2d_image img = {0}, tmp_img = {0};
>> +	unsigned int img_w, img_h, count;
>> +	int cur_x, cur_y;
>> +	void *checkerboard;
>> +	int ret;
>> +
>> +	static const struct g2d_step {
>> +		int x, y;
>> +	} steps[] = {
>> +		{ 1,  0}, { 0,  1},
>> +		{-1,  0}, { 0, -1},
>> +		{ 1,  1}, {-1, -1},
>> +		{ 1, -1}, {-1,  1},
>> +		{ 2,  1}, { 1,  2},
>> +		{-2, -1}, {-1, -2},
>> +		{ 2, -1}, { 1, -2},
>> +		{-2,  1}, {-1,  2}
>> +	};
>> +	static const unsigned int num_steps =
>> +		sizeof(steps) / sizeof(struct g2d_step);
>> +
>> +	ctx = g2d_init(dev->fd);
>> +	if (!ctx)
>> +		return -EFAULT;
>> +
>> +	img.bo[0] = buf->handle;
>> +
>> +	/* create pattern of half the screen size */
>> +	checkerboard = create_checkerboard_pattern(screen_width /
>> 64, screen_height / 64, 32);
>> +	if (!checkerboard) {
>> +		ret = -EFAULT;
>> +		goto fail;
>> +	}
>> +
>> +	img_w = (screen_width / 64) * 32;
>> +	img_h = (screen_height / 64) * 32;
>> +
>> +	switch (type) {
>> +	case G2D_IMGBUF_GEM:
>> +		memcpy(tmp->vaddr, checkerboard, img_w * img_h * 4);
>> +		tmp_img.bo[0] = tmp->handle;
>> +		break;
>> +	case G2D_IMGBUF_USERPTR:
>> +		tmp_img.user_ptr[0].userptr = (unsigned
>> long)checkerboard;
>> +		tmp_img.user_ptr[0].size = img_w * img_h * 4;
>> +		break;
>> +	case G2D_IMGBUF_COLOR:
>> +	default:
>> +		ret = -EFAULT;
>> +		goto fail;
>> +	}
>> +
>> +	/* solid fill framebuffer with white color */
>> +	img.width = screen_width;
>> +	img.height = screen_height;
>> +	img.stride = screen_width * 4;
>> +	img.buf_type = G2D_IMGBUF_GEM;
>> +	img.color_mode = G2D_COLOR_FMT_ARGB8888 | G2D_ORDER_AXRGB;
>> +	img.color = 0xffffffff;
>> +
>> +	/* put checkerboard pattern in the center of the framebuffer
>> */
>> +	cur_x = (screen_width - img_w) / 2;
>> +	cur_y = (screen_height - img_h) / 2;
>> +	tmp_img.width = img_w;
>> +	tmp_img.height = img_h;
>> +	tmp_img.stride = img_w * 4;
>> +	tmp_img.buf_type = type;
>> +	tmp_img.color_mode = G2D_COLOR_FMT_ARGB8888 |
>> G2D_ORDER_AXRGB; +
>> +	ret = g2d_solid_fill(ctx, &img, 0, 0, screen_width,
>> screen_height) ||
>> +		g2d_copy(ctx, &tmp_img, &img, 0, 0, cur_x, cur_y,
>> img_w, img_h); +
>> +	if (!ret)
>> +		ret = g2d_exec(ctx);
>> +	if (ret < 0)
>> +			goto fail;
>> +
>> +	printf("move test with %s.\n",
>> +			type == G2D_IMGBUF_GEM ? "gem" : "userptr");
>> +
>> +	srand(time(NULL));
>> +	for (count = 0; count < 256; ++count) {
>> +		const struct g2d_step *s;
>> +
>> +		/* select step and validate it */
>> +		while (1) {
>> +			s = &steps[random() % num_steps];
>> +
>> +			if (cur_x + s->x < 0 || cur_y + s->y < 0 ||
>> +				cur_x + img_w + s->x >= screen_width
>> ||
>> +				cur_y + img_h + s->y >=
>> screen_height)
>> +				continue;
>> +			else
>> +				break;
>> +		}
>> +
>> +		ret = g2d_move(ctx, &img, cur_x, cur_y, cur_x +
>> s->x, cur_y + s->y,
>> +			img_w, img_h);
>> +		if (!ret)
>> +			ret = g2d_exec(ctx);
>> +
>> +		if (ret < 0)
>> +			goto fail;
>> +
>> +		cur_x += s->x;
>> +		cur_y += s->y;
>> +
>> +		usleep(100000);
>> +	}
>> +
>> +fail:
>> +	g2d_fini(ctx);
>> +
>> +	free(checkerboard);
>> +
>> +	return ret;
>> +}
>> +
>>  static int g2d_copy_with_scale_test(struct exynos_device *dev,
>>  					struct exynos_bo *src,
>>  					struct exynos_bo *dst,
>> @@ -708,6 +832,14 @@ int main(int argc, char **argv)
>>  
>>  	wait_for_user_input(0);
>>  
>> +	ret = g2d_move_test(dev, src, bo, G2D_IMGBUF_GEM);
>> +	if (ret < 0) {
>> +		fprintf(stderr, "failed to test move operation.\n");
>> +		goto err_free_src;
>> +	}
>> +
>> +	wait_for_user_input(0);
>> +
>>  	ret = g2d_copy_with_scale_test(dev, src, bo, G2D_IMGBUF_GEM);
>>  	if (ret < 0) {
>>  		fprintf(stderr, "failed to test copy and scale
>> operation.\n");
> 

Re: [PATCH 10/13] tests/exynos: add test for g2d_move

[Emil Velikov]

On 9 November 2015 at 09:47, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
> Hello Hyungwon,
>
> Hyungwon Hwang wrote:
>> Hello,
>>
>> I think this patch should update .gitignore, not for adding the built
>> binary to untracked file list.
> good point. I should do this for the event test as well I guess.
>
> Going to respin the series.
>
Imho you can do that as a single patch on top. There is no problem if
git status prints out a binary name for a couple of commits.

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

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

[Hyungwon Hwang]

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

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

[Tobias Jakobi]

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
> 

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

[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