drm/exynos: add async G2D execution to libdrm

drm/exynos: add async G2D execution to libdrm

[Tobias Jakobi]

Hello,

this series exposes async execution of G2D command buffers to userspace. Also includes is a small performance analysis test, which can also be used to 
stress test the engine. The async operation is of course also tested.

Please review and let me know what I can improve.

With best wishes,
Tobias

[PATCH 1/5] 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.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 tests/exynos/Makefile.am          |   8 +-
 tests/exynos/exynos_fimg2d_perf.c | 245 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 252 insertions(+), 1 deletion(-)
 create mode 100644 tests/exynos/exynos_fimg2d_perf.c

diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
index b21d016..243f957 100644
--- a/tests/exynos/Makefile.am
+++ b/tests/exynos/Makefile.am
@@ -5,9 +5,11 @@ AM_CFLAGS = \
 	-I $(top_srcdir)/exynos \
 	-I $(top_srcdir)
 
+bin_PROGRAMS = exynos_fimg2d_perf
+
 if HAVE_LIBKMS
 if HAVE_INSTALL_TESTS
-bin_PROGRAMS = \
+bin_PROGRAMS += \
 	exynos_fimg2d_test
 else
 noinst_PROGRAMS = \
@@ -15,6 +17,10 @@ noinst_PROGRAMS = \
 endif
 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..f45cacc
--- /dev/null
+++ b/tests/exynos/exynos_fimg2d_perf.c
@@ -0,0 +1,245 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <time.h>
+
+#include <xf86drm.h>
+
+#include "exynos_drm.h"
+#include "exynos_drmif.h"
+#include "exynos_fimg2d.h"
+
+/* Enable to format the output so that it can be fed into Mathematica. */
+#define 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 == 1)
+	putchar('{');
+#endif
+
+	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 == 1)
+			if (i != 0) putchar(',');
+			printf("{%u,%llu}", w * h, g2d_time);
+#else
+			printf("num_pixels = %u, usecs = %llu\n", w * h, g2d_time);
+#endif
+		}
+	}
+
+#if (OUTPUT_MATHEMATICA == 1)
+	printf("}\n");
+#endif
+
+	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 == 1)
+	putchar('{');
+#endif
+
+	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 == 1)
+			if (i != 0) putchar(',');
+			printf("{%u,%llu}", num_pixels, g2d_time);
+#else
+			printf("num_pixels = %u, usecs = %llu\n", num_pixels, g2d_time);
+#endif
+		}
+	}
+
+#if (OUTPUT_MATHEMATICA == 1)
+	printf("}\n");
+#endif
+
+	return ret;
+}
+
+int main(int argc, char **argv)
+{
+	int fd, ret;
+
+	struct exynos_device *dev;
+	struct g2d_context *ctx;
+	struct exynos_bo *bo;
+
+	ret = 0;
+
+	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, 4096 * 4096 * 4, 0);
+	if (bo == NULL) {
+		fprintf(stderr, "error: failed to create bo\n");
+		ret = -4;
+
+		goto bo_fail;
+	}
+
+	ret = fimg2d_perf_simple(bo, ctx, 4096, 4096, 128);
+
+	if (ret == 0)
+		ret = fimg2d_perf_multi(bo, ctx, 4096, 4096, 128, 3);
+
+	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 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Tobias Jakobi]

This event is specific to Exynos G2D DRM driver. Only
process it when Exynos support is enabled.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 exynos/exynos_drm.h | 12 ++++++++++++
 xf86drm.h           |  7 ++++++-
 xf86drmMode.c       | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

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/xf86drm.h b/xf86drm.h
index 40c55c9..6b1c9b4 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -719,7 +719,7 @@ extern void drmMsg(const char *format, ...) DRM_PRINTFLIKE(1, 2);
 extern int drmSetMaster(int fd);
 extern int drmDropMaster(int fd);
 
-#define DRM_EVENT_CONTEXT_VERSION 2
+#define DRM_EVENT_CONTEXT_VERSION 3
 
 typedef struct _drmEventContext {
 
@@ -739,6 +739,11 @@ typedef struct _drmEventContext {
 				  unsigned int tv_usec,
 				  void *user_data);
 
+	void (*g2d_event_handler)(int fd,
+				  unsigned int cmdlist_no,
+				  unsigned int tv_sec,
+				  unsigned int tv_usec,
+				  void *user_data);
 } drmEventContext, *drmEventContextPtr;
 
 extern int drmHandleEvent(int fd, drmEventContextPtr evctx);
diff --git a/xf86drmMode.c b/xf86drmMode.c
index 61d5e01..d9f2898 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -53,6 +53,10 @@
 #include <unistd.h>
 #include <errno.h>
 
+#ifdef HAVE_EXYNOS
+#include <exynos/exynos_drm.h>
+#endif
+
 #ifdef HAVE_VALGRIND
 #include <valgrind.h>
 #include <memcheck.h>
@@ -846,6 +850,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 	int len, i;
 	struct drm_event *e;
 	struct drm_event_vblank *vblank;
+	struct drm_exynos_g2d_event *g2d;
 	
 	/* The DRM read semantics guarantees that we always get only
 	 * complete events. */
@@ -882,6 +887,19 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 						 vblank->tv_usec,
 						 U642VOID (vblank->user_data));
 			break;
+#ifdef HAVE_EXYNOS
+		case DRM_EXYNOS_G2D_EVENT:
+			if (evctx->version < 3 ||
+			    evctx->g2d_event_handler == NULL)
+				break;
+			g2d = (struct drm_exynos_g2d_event *) e;
+			evctx->g2d_event_handler(fd,
+						 g2d->cmdlist_no,
+						 g2d->tv_sec,
+						 g2d->tv_usec,
+						 U642VOID (g2d->user_data));
+			break;
+#endif
 		default:
 			break;
 		}
-- 
2.0.5

[PATCH 3/5] 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).

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

diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
index fc605ed..e90ffed 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -202,8 +202,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;
@@ -259,6 +266,22 @@ drm_public 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
+ */
+drm_public 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 9db0c88..421249d 100644
--- a/exynos/exynos_fimg2d.h
+++ b/exynos/exynos_fimg2d.h
@@ -298,10 +298,12 @@ struct g2d_context {
 	unsigned int			cmd_nr;
 	unsigned int			cmd_buf_nr;
 	unsigned int			cmdlist_nr;
+	void				*event_userdata;
 };
 
 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

Re: drm/exynos: add async G2D execution to libdrm

[Tobias Jakobi]

Tobias Jakobi wrote:
> Hello,
> 
> this series exposes async execution of G2D command buffers to userspace. Also includes is a small performance analysis test, which can also be used to 
> stress test the engine. The async operation is of course also tested.
> 
> Please review and let me know what I can improve.
> 
> With best wishes,
> Tobias
Almost forgot this. In case someone is interested in the solid fill
performance, I've uploaded a plot here:
http://www.math.uni-bielefeld.de/~tjakobi/exynos/g2d_clear_perf.pdf

And yes, that should be _nano_seconds on the y-axis. What I find
interesting that despite the performance being linear (well, affine
linear, since we have a setup time) in the number of pixels to process,
you can still see some kind of 'clustering'.
I wonder where this comes from, maybe some small alignment penalty, or
something?

With best wishes,
Tobias

Re: [PATCH 1/5] tests/exynos: add fimg2d performance analysis

[Emil Velikov]

On 20/03/15 22:25, Tobias Jakobi 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.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Hi Tobias,

As most of this series is quite Exynos specific I will cover the misc
bits, and leave the core part to someone familiar with the hardware.

> ---
>  tests/exynos/Makefile.am          |   8 +-
>  tests/exynos/exynos_fimg2d_perf.c | 245 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 252 insertions(+), 1 deletion(-)
>  create mode 100644 tests/exynos/exynos_fimg2d_perf.c
> 
> diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
> index b21d016..243f957 100644
> --- a/tests/exynos/Makefile.am
> +++ b/tests/exynos/Makefile.am
> @@ -5,9 +5,11 @@ AM_CFLAGS = \
>  	-I $(top_srcdir)/exynos \
>  	-I $(top_srcdir)
>  
> +bin_PROGRAMS = exynos_fimg2d_perf
> +
Might I suggest that we treat this (and your follow up utility) as a
test ? I.e. use

if HAVE_INSTALL_TESTS
bin_PROGRAMS = \
	exynos_fimg2d_perf
else
noinst_PROGRAMS = \
	exynos_fimg2d_perf
endif

and amend the block below appropriately ?

>  if HAVE_LIBKMS
>  if HAVE_INSTALL_TESTS
> -bin_PROGRAMS = \
> +bin_PROGRAMS += \
>  	exynos_fimg2d_test
>  else
>  noinst_PROGRAMS = \
> @@ -15,6 +17,10 @@ noinst_PROGRAMS = \
>  endif
>  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..f45cacc
> --- /dev/null
> +++ b/tests/exynos/exynos_fimg2d_perf.c
> @@ -0,0 +1,245 @@
Can you add a licence to this file. Would be nice if it's covered by
X/MIT so that *BSD folk and others can use your tool.

> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <time.h>
> +
> +#include <xf86drm.h>
> +
> +#include "exynos_drm.h"
> +#include "exynos_drmif.h"
> +#include "exynos_fimg2d.h"
> +
> +/* Enable to format the output so that it can be fed into Mathematica. */
> +#define 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 == 1)
> +	putchar('{');
> +#endif
> +
I'm suspecting that having this as a runtime option will be better.
Something like ./exynos_fimg2d_perf --output mathematica ?

As a general note I would recommend keeping statements on separate lines
(none of if (foo) far()) as it makes debugging easier.

Although all of the above are my take on things, so checks with the
Exynos crew if they feel otherwise :-)

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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

On 20/03/15 22:25, Tobias Jakobi wrote:
> This event is specific to Exynos G2D DRM driver. Only
> process it when Exynos support is enabled.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  exynos/exynos_drm.h | 12 ++++++++++++
>  xf86drm.h           |  7 ++++++-
>  xf86drmMode.c       | 18 ++++++++++++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
I fear we are not (yet) allowed to do either of these changes.

The exynos/exynos_drm.h header is (supposed to) be in sync/come from the
kernel. And changes here are to be reflected only when the corresponding
patch lands in drm-next (as per RELEASING).

Wrt extending the current drmEventContext, I'm not sure that adding
device specific changes to it are allowed.

Wish I would give you some better news but... I cannot sorry :-\

-Emil

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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Tobias Jakobi]

Hello Emil,


Emil Velikov wrote:
> I fear we are not (yet) allowed to do either of these changes.
> 
> The exynos/exynos_drm.h header is (supposed to) be in sync/come from the
> kernel. And changes here are to be reflected only when the corresponding
> patch lands in drm-next (as per RELEASING).
the point here is, that the current header in libdrm in _not_ in sync
with the one in the kernel. It's hopelessly outdated, but mainly because
exynos/libdrm doesn't use any new functionality provided by some update.

Here's the current kernel header:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/uapi/drm/exynos_drm.h

The event stuff has been there since 2012:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/drm/exynos_drm.h?id=d7f1642c90ab5eb2d7c48af0581c993094f97e1a

The only reason why I haven't added the IPP things, is because I don't
intend to work on this for the moment.



> Wrt extending the current drmEventContext, I'm not sure that adding
> device specific changes to it are allowed.
Why shouldn't it? Isn't drmHandleEvent supposed to handle all kinds of
DRM events that the kernel produces?


With best wishes,
Tobias

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

Re: [PATCH 1/5] tests/exynos: add fimg2d performance analysis

[Tobias Jakobi]

Hello Emil,


Emil Velikov wrote:
> Might I suggest that we treat this (and your follow up utility) as a
> test ? I.e. use
> 
> if HAVE_INSTALL_TESTS
> bin_PROGRAMS = \
> 	exynos_fimg2d_perf
> else
> noinst_PROGRAMS = \
> 	exynos_fimg2d_perf
> endif
> 
> and amend the block below appropriately ?
sure, honestly I don't even remember why I didn't add these as tests?
*confused*


> Can you add a licence to this file. Would be nice if it's covered by
> X/MIT so that *BSD folk and others can use your tool.
Will do! Even though I probably won't go with a MIT license.


> I'm suspecting that having this as a runtime option will be better.
> Something like ./exynos_fimg2d_perf --output mathematica ?
Well, I was thinking about removing the Mathematica specific code for
the submission, but I then left it in. I use Mathematica for parts of my
thesis, so it's usually my preferred tool to visualize data. I guess a
more 'open-source' friendly solution here would be to provide GnuPlot
output, but I guess I leave that to another use :)


> As a general note I would recommend keeping statements on separate lines
> (none of if (foo) far()) as it makes debugging easier.
OK, changing this.


With best wishes,
Tobias

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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

Hi Tobias,
On 22/03/15 16:29, Tobias Jakobi wrote:
> Hello Emil,
> Emil Velikov wrote:
>> I fear we are not (yet) allowed to do either of these changes.
>>
>> The exynos/exynos_drm.h header is (supposed to) be in sync/come from the
>> kernel. And changes here are to be reflected only when the corresponding
>> patch lands in drm-next (as per RELEASING).
> the point here is, that the current header in libdrm in _not_ in sync
> with the one in the kernel. It's hopelessly outdated, but mainly because
> exynos/libdrm doesn't use any new functionality provided by some update.
> 
> Here's the current kernel header:
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/include/uapi/drm/exynos_drm.h
> 
> The event stuff has been there since 2012:
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/drm/exynos_drm.h?id=d7f1642c90ab5eb2d7c48af0581c993094f97e1a
> 
> The only reason why I haven't added the IPP things, is because I don't
> intend to work on this for the moment.
> 
Hmmm good point. Seems like the changes went in before I started
following exynos development. If it's in an upstream kernel, then it's
save to land in libdrm. Objection withdrawn.

I have an idea how we can get things back into shape (wrt headers being
out if sync) - I will propose/post a solution shortly.

>> Wrt extending the current drmEventContext, I'm not sure that adding
>> device specific changes to it are allowed.
> Why shouldn't it? Isn't drmHandleEvent supposed to handle all kinds of
> DRM events that the kernel produces?
> 
Bth I'm not familiar with the code in question, although a quick grep
indicates that libdrm does not contain any driver specific information.
That is aside from the include/drm headers, although they are not part
of the library or its interface.

Maybe some of the more experienced devs can share some light ?

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

Re: [PATCH 1/5] tests/exynos: add fimg2d performance analysis

[Tobias Jakobi]

Hello,

the new version should fix most of the mentioned issues.

Tobias Jakobi wrote:
>> As a general note I would recommend keeping statements on separate lines
>> (none of if (foo) far()) as it makes debugging easier.
> OK, changing this.
Except for this, I didn't change it since I don't see the point. In fact
I think that splitting the few occurrences makes the code less readable.

I haven't worked with getopt before, so I gave it a try and made all
these hardcoded parameters configurable.

With best wishes,
Tobias

Re: [PATCH 1/5] tests/exynos: add fimg2d performance analysis

[Emil Velikov]

On 25/03/15 18:27, Tobias Jakobi wrote:
> Hello,
> 
> the new version should fix most of the mentioned issues.
> 
> Tobias Jakobi wrote:
>>> As a general note I would recommend keeping statements on separate lines
>>> (none of if (foo) far()) as it makes debugging easier.
>> OK, changing this.
> Except for this, I didn't change it since I don't see the point. In fact
> I think that splitting the few occurrences makes the code less readable.
> 
Beauty is in the eye of the beholder.

> I haven't worked with getopt before, so I gave it a try and made all
> these hardcoded parameters configurable.
> 
Nice :-)

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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Rob Clark]

so, iirc, vmwgfx also has some custom events..  not really sure if
they have their own hand-rolled drmHandleEvent() or if they have
another way of catching those.

Maybe we need some more flexible way to register handlers for driver
custom events?  But everyone adding their own thing to
drmHandleEvent() doesn't really seem so nice..  that said, I'm not
sure how much to care.  If it is just exynos and vmwgfx, then telling
them to use there own version of of drmHandleEvent() might be ok.  But
if driver custom events somehow become more popular, maybe we want a
better solution..

BR,
-R

On Fri, Mar 20, 2015 at 6:25 PM, Tobias Jakobi
<tjakobi@math.uni-bielefeld.de> wrote:
> This event is specific to Exynos G2D DRM driver. Only
> process it when Exynos support is enabled.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  exynos/exynos_drm.h | 12 ++++++++++++
>  xf86drm.h           |  7 ++++++-
>  xf86drmMode.c       | 18 ++++++++++++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
>
> 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/xf86drm.h b/xf86drm.h
> index 40c55c9..6b1c9b4 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -719,7 +719,7 @@ extern void drmMsg(const char *format, ...) DRM_PRINTFLIKE(1, 2);
>  extern int drmSetMaster(int fd);
>  extern int drmDropMaster(int fd);
>
> -#define DRM_EVENT_CONTEXT_VERSION 2
> +#define DRM_EVENT_CONTEXT_VERSION 3
>
>  typedef struct _drmEventContext {
>
> @@ -739,6 +739,11 @@ typedef struct _drmEventContext {
>                                   unsigned int tv_usec,
>                                   void *user_data);
>
> +       void (*g2d_event_handler)(int fd,
> +                                 unsigned int cmdlist_no,
> +                                 unsigned int tv_sec,
> +                                 unsigned int tv_usec,
> +                                 void *user_data);
>  } drmEventContext, *drmEventContextPtr;
>
>  extern int drmHandleEvent(int fd, drmEventContextPtr evctx);
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index 61d5e01..d9f2898 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -53,6 +53,10 @@
>  #include <unistd.h>
>  #include <errno.h>
>
> +#ifdef HAVE_EXYNOS
> +#include <exynos/exynos_drm.h>
> +#endif
> +
>  #ifdef HAVE_VALGRIND
>  #include <valgrind.h>
>  #include <memcheck.h>
> @@ -846,6 +850,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
>         int len, i;
>         struct drm_event *e;
>         struct drm_event_vblank *vblank;
> +       struct drm_exynos_g2d_event *g2d;
>
>         /* The DRM read semantics guarantees that we always get only
>          * complete events. */
> @@ -882,6 +887,19 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
>                                                  vblank->tv_usec,
>                                                  U642VOID (vblank->user_data));
>                         break;
> +#ifdef HAVE_EXYNOS
> +               case DRM_EXYNOS_G2D_EVENT:
> +                       if (evctx->version < 3 ||
> +                           evctx->g2d_event_handler == NULL)
> +                               break;
> +                       g2d = (struct drm_exynos_g2d_event *) e;
> +                       evctx->g2d_event_handler(fd,
> +                                                g2d->cmdlist_no,
> +                                                g2d->tv_sec,
> +                                                g2d->tv_usec,
> +                                                U642VOID (g2d->user_data));
> +                       break;
> +#endif
>                 default:
>                         break;
>                 }
> --
> 2.0.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Tobias Jakobi]

Hello,


On 2015-03-30 02:02, Rob Clark wrote:
> Maybe we need some more flexible way to register handlers for driver
> custom events?  But everyone adding their own thing to
> drmHandleEvent() doesn't really seem so nice..  that said, I'm not
> sure how much to care.  If it is just exynos and vmwgfx, then telling
> them to use there own version of of drmHandleEvent() might be ok.  But
> if driver custom events somehow become more popular, maybe we want a
> better solution..
I don't like the idea of just copying the current drmHandleEvent() code
and putting it into the exynos code together with the additional switch
cases. It just screams "HACK!" to me.

I'm going to try to come up with a solution where we can at least share
some of the code.

With best wishes,
Tobias


> 
> BR,
> -R
> 
> On Fri, Mar 20, 2015 at 6:25 PM, Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de> wrote:
>> This event is specific to Exynos G2D DRM driver. Only
>> process it when Exynos support is enabled.
>> 
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  exynos/exynos_drm.h | 12 ++++++++++++
>>  xf86drm.h           |  7 ++++++-
>>  xf86drmMode.c       | 18 ++++++++++++++++++
>>  3 files changed, 36 insertions(+), 1 deletion(-)
>> 
>> 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/xf86drm.h b/xf86drm.h
>> index 40c55c9..6b1c9b4 100644
>> --- a/xf86drm.h
>> +++ b/xf86drm.h
>> @@ -719,7 +719,7 @@ extern void drmMsg(const char *format, ...) 
>> DRM_PRINTFLIKE(1, 2);
>>  extern int drmSetMaster(int fd);
>>  extern int drmDropMaster(int fd);
>> 
>> -#define DRM_EVENT_CONTEXT_VERSION 2
>> +#define DRM_EVENT_CONTEXT_VERSION 3
>> 
>>  typedef struct _drmEventContext {
>> 
>> @@ -739,6 +739,11 @@ typedef struct _drmEventContext {
>>                                   unsigned int tv_usec,
>>                                   void *user_data);
>> 
>> +       void (*g2d_event_handler)(int fd,
>> +                                 unsigned int cmdlist_no,
>> +                                 unsigned int tv_sec,
>> +                                 unsigned int tv_usec,
>> +                                 void *user_data);
>>  } drmEventContext, *drmEventContextPtr;
>> 
>>  extern int drmHandleEvent(int fd, drmEventContextPtr evctx);
>> diff --git a/xf86drmMode.c b/xf86drmMode.c
>> index 61d5e01..d9f2898 100644
>> --- a/xf86drmMode.c
>> +++ b/xf86drmMode.c
>> @@ -53,6 +53,10 @@
>>  #include <unistd.h>
>>  #include <errno.h>
>> 
>> +#ifdef HAVE_EXYNOS
>> +#include <exynos/exynos_drm.h>
>> +#endif
>> +
>>  #ifdef HAVE_VALGRIND
>>  #include <valgrind.h>
>>  #include <memcheck.h>
>> @@ -846,6 +850,7 @@ int drmHandleEvent(int fd, drmEventContextPtr 
>> evctx)
>>         int len, i;
>>         struct drm_event *e;
>>         struct drm_event_vblank *vblank;
>> +       struct drm_exynos_g2d_event *g2d;
>> 
>>         /* The DRM read semantics guarantees that we always get only
>>          * complete events. */
>> @@ -882,6 +887,19 @@ int drmHandleEvent(int fd, drmEventContextPtr 
>> evctx)
>>                                                  vblank->tv_usec,
>>                                                  U642VOID 
>> (vblank->user_data));
>>                         break;
>> +#ifdef HAVE_EXYNOS
>> +               case DRM_EXYNOS_G2D_EVENT:
>> +                       if (evctx->version < 3 ||
>> +                           evctx->g2d_event_handler == NULL)
>> +                               break;
>> +                       g2d = (struct drm_exynos_g2d_event *) e;
>> +                       evctx->g2d_event_handler(fd,
>> +                                                g2d->cmdlist_no,
>> +                                                g2d->tv_sec,
>> +                                                g2d->tv_usec,
>> +                                                U642VOID 
>> (g2d->user_data));
>> +                       break;
>> +#endif
>>                 default:
>>                         break;
>>                 }
>> --
>> 2.0.5
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Tobias Jakobi]

Hello,

On 2015-03-30 02:02, Rob Clark wrote:
> so, iirc, vmwgfx also has some custom events..  not really sure if
> they have their own hand-rolled drmHandleEvent() or if they have
> another way of catching those.
> 
> Maybe we need some more flexible way to register handlers for driver
> custom events?  But everyone adding their own thing to
> drmHandleEvent() doesn't really seem so nice..  that said, I'm not
> sure how much to care.  If it is just exynos and vmwgfx, then telling
> them to use there own version of of drmHandleEvent() might be ok.  But
> if driver custom events somehow become more popular, maybe we want a
> better solution..

would something like this work for you guys:
https://www.math.uni-bielefeld.de/~tjakobi/archive/0001-custom-events.patch

(this is not compile tested or anything, just a draft)

Basically this introduces drmHandleEvent2, which is drmHandleEvent with 
two additional arguments. The first one being a function pointer through 
which the 'remaining' events (which are not handled by the common code) 
are handled, and some (opaque) pointer to data that the handler might 
need.

In the vendor specific code I then introcuded exynos_handle_event which 
calls dramHandleEvent2 with a properly setup handler. vmwgfx could do 
the same here I guess.

With best wishes,
Tobias

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Emil Velikov]

Hi Tobias,

On 30/03/15 13:04, Tobias Jakobi wrote:
> Hello,
> 
> On 2015-03-30 02:02, Rob Clark wrote:
>> so, iirc, vmwgfx also has some custom events..  not really sure if
>> they have their own hand-rolled drmHandleEvent() or if they have
>> another way of catching those.
>>
>> Maybe we need some more flexible way to register handlers for driver
>> custom events?  But everyone adding their own thing to
>> drmHandleEvent() doesn't really seem so nice..  that said, I'm not
>> sure how much to care.  If it is just exynos and vmwgfx, then telling
>> them to use there own version of of drmHandleEvent() might be ok.  But
>> if driver custom events somehow become more popular, maybe we want a
>> better solution..
> 
> would something like this work for you guys:
> https://www.math.uni-bielefeld.de/~tjakobi/archive/0001-custom-events.patch
> 
> (this is not compile tested or anything, just a draft)
> 
> Basically this introduces drmHandleEvent2, which is drmHandleEvent with
> two additional arguments. The first one being a function pointer through
> which the 'remaining' events (which are not handled by the common code)
> are handled, and some (opaque) pointer to data that the handler might need.
> 
> In the vendor specific code I then introcuded exynos_handle_event which
> calls dramHandleEvent2 with a properly setup handler. vmwgfx could do
> the same here I guess.
> 
I'm assuming that one of the concerns here is about adding API for a
single (and not so common) user to the core library.

>From a quick look at the mesa and xf86-video-vmware I cannot see the
vmwgfx driver using events. It has some definitions in vmwgfx_drm.h but
that's about it.

That aside - the drmHandleEvent2 approach looks like a massive
improvement over the original patch. Personally I do not see any
problems with it and think that it's a good way forward.

Perhaps you can come over to #dri-devel and ping the devs to get some
more feedback. As the topic is not a priority for most of them your
suggestions has mostly gone unnoticed.

Cheers,
Emil

Re: [PATCH 2/5] exynos: add EXYNOS_G2D_EVENT to drmHandleEvent

[Tobias Jakobi]

Hello Emil,

On 2015-04-21 20:14, Emil Velikov wrote:
> Hi Tobias,
> 
> On 30/03/15 13:04, Tobias Jakobi wrote:
>> Hello,
>> 
>> On 2015-03-30 02:02, Rob Clark wrote:
>>> so, iirc, vmwgfx also has some custom events..  not really sure if
>>> they have their own hand-rolled drmHandleEvent() or if they have
>>> another way of catching those.
>>> 
>>> Maybe we need some more flexible way to register handlers for driver
>>> custom events?  But everyone adding their own thing to
>>> drmHandleEvent() doesn't really seem so nice..  that said, I'm not
>>> sure how much to care.  If it is just exynos and vmwgfx, then telling
>>> them to use there own version of of drmHandleEvent() might be ok.  
>>> But
>>> if driver custom events somehow become more popular, maybe we want a
>>> better solution..
>> 
>> would something like this work for you guys:
>> https://www.math.uni-bielefeld.de/~tjakobi/archive/0001-custom-events.patch
>> 
>> (this is not compile tested or anything, just a draft)
>> 
>> Basically this introduces drmHandleEvent2, which is drmHandleEvent 
>> with
>> two additional arguments. The first one being a function pointer 
>> through
>> which the 'remaining' events (which are not handled by the common 
>> code)
>> are handled, and some (opaque) pointer to data that the handler might 
>> need.
>> 
>> In the vendor specific code I then introcuded exynos_handle_event 
>> which
>> calls dramHandleEvent2 with a properly setup handler. vmwgfx could do
>> the same here I guess.
>> 
> I'm assuming that one of the concerns here is about adding API for a
> single (and not so common) user to the core library.
> 
> From a quick look at the mesa and xf86-video-vmware I cannot see the
> vmwgfx driver using events. It has some definitions in vmwgfx_drm.h but
> that's about it.
> 
> That aside - the drmHandleEvent2 approach looks like a massive
> improvement over the original patch. Personally I do not see any
> problems with it and think that it's a good way forward.
> 
> Perhaps you can come over to #dri-devel and ping the devs to get some
> more feedback. As the topic is not a priority for most of them your
> suggestions has mostly gone unnoticed.
I'm going to flesh out the non-exynos patches some more before sending 
them to the ML for discussion.


With best wishes,
Tobias