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
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; +}
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
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
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
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
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; }
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
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/in...
The event stuff has been there since 2012: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/...
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
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/in...
The event stuff has been there since 2012: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/...
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
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
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
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
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
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
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,
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
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Rob Clark -
Tobias Jakobi -
Tobias Jakobi