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
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) {
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_ */
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; +}
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 shouldbe in the range 2 to 4096.\n");
ret = -1;goto out;- }
- if (bufw == 0 || bufh == 0) {
fprintf(stderr, "error: buffer width/height shouldbe 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;
+}
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 shouldbe in the range 2 to 4096.\n");
ret = -1;goto out;- }
- if (bufw == 0 || bufh == 0) {
fprintf(stderr, "error: buffer width/height shouldbe 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;
+}
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,
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);
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; +}
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 inttv_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, unsignediterations) +{
- 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;elseusleep(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 shouldbe less than 4096.\n");
ret = -1;goto out;- }
- if (bufw == 0 || bufh == 0) {
fprintf(stderr, "error: buffer width/height shouldbe 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;
+}
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 inttv_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, unsignediterations) +{
- 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;elseusleep(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 shouldbe less than 4096.\n");
ret = -1;goto out;- }
- if (bufw == 0 || bufh == 0) {
fprintf(stderr, "error: buffer width/height shouldbe 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;
+}
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.
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.
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
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
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
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,
unsignedint 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;elseusleep(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 shouldbe less than 4096.\n");
ret = -1;goto out;- }
- if (bufw == 0 || bufh == 0) {
fprintf(stderr, "error: buffer width/height shouldbe 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 createdevice\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;
+}
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;
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;
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;
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;
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;
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_modemask_x_direction:1;
enum e_g2d_dir_modemask_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;
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_modemask_x_direction:1;
enum e_g2d_dir_modemask_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;
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_modemask_x_direction:1;
enum e_g2d_dir_modemask_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
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_modesrc_x_direction:1;
enum e_g2d_dir_modesrc_y_direction:1; +
/* SRC_MSK_DIRECT_REG [2:3] */unsigned int reversed1:2;/* SRC_MSK_DIRECT_REG [4:5] (mask) */enum e_g2d_dir_modemask_x_direction:1;
enum e_g2d_dir_modemask_y_direction:1; +
/* SRC_MSK_DIRECT_REG [6:31] */unsigned int padding1:26;/* DST_PAT_DIRECT_REG [0:1] (destination) */enum e_g2d_dir_modedst_x_direction:1;
enum e_g2d_dir_modedst_y_direction:1; +
/* DST_PAT_DIRECT_REG [2:3] */unsigned int reversed2:2;/* DST_PAT_DIRECT_REG [4:5] (pattern) */enum e_g2d_dir_modepat_x_direction:1;
enum e_g2d_dir_modepat_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
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,
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 orheight.\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 intdst_x,
unsigned dst_y, unsigned int w, unsigned int h);int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, struct g2d_image *dst, unsigned int src_x, unsigned int src_y, unsigned int src_w,
Hello Hyungwon,
Hyungwon Hwang wrote:
On Tue, 22 Sep 2015 17:54:58 +0200 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
We already have g2d_copy() which implements G2D copy operations from one buffer to another. However we can't do a overlapping copy operation in one buffer.
Add g2d_move() which acts like the standard memmove() and properly handles overlapping copies.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
exynos/exynos_fimg2d.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ exynos/exynos_fimg2d.h | 3 ++ 2 files changed, 97 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4d5419c..8703629 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, }
/**
- g2d_move - move content inside single buffer.
- Similar to 'memmove' this moves a rectangular region
- of the provided buffer to another location (the source
- and destination region potentially overlapping).
- @ctx: a pointer to g2d_context structure.
- @img: a pointer to g2d_image structure providing
- buffer information.
- @src_x: x position of source rectangle.
- @src_y: y position of source rectangle.
- @dst_x: x position of destination rectangle.
- @dst_y: y position of destination rectangle.
- @w: width of rectangle to move.
- @h: height of rectangle to move.
- */
+int +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
unsigned int src_x, unsigned int src_y,unsigned int dst_x, unsigned dst_y, unsigned int w,unsigned int h)+{
- union g2d_rop4_val rop4;
- union g2d_point_val pt;
- union g2d_direction_val dir;
- unsigned int src_w, src_h, dst_w, dst_h;
- src_w = w;
- src_h = h;
- if (src_x + img->width > w)
src_w = img->width - src_x;- if (src_y + img->height > h)
src_h = img->height - src_y;- dst_w = w;
- dst_h = w;
- if (dst_x + img->width > w)
dst_w = img->width - dst_x;- if (dst_y + img->height > h)
dst_h = img->height - dst_y;- w = MIN(src_w, dst_w);
- h = MIN(src_h, dst_h);
- if (w == 0 || h == 0) {
fprintf(stderr, MSG_PREFIX "invalid width orheight.\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 intdst_x,
unsigned dst_y, unsigned int w, unsigned int h);int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, struct g2d_image *dst, unsigned int src_x, unsigned int src_y, unsigned int src_w,
Hello Tobias,
I was in vacation last week, so I could run your code today. I found that what g2d_move() does is actually copying not moving, because the operation does not clear the previous area. Would it be possible to generalize g2d_copy() works better, so it could works well in case of the src buffer and dst buffer being same. If it is possible, I think it would be better way to do that. If it is not, at least chaning the function name is needed. I tested it on my Odroid U3 board.
Best regards, Hyungwon Hwang
On Tue, 22 Sep 2015 17:54:58 +0200 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
We already have g2d_copy() which implements G2D copy operations from one buffer to another. However we can't do a overlapping copy operation in one buffer.
Add g2d_move() which acts like the standard memmove() and properly handles overlapping copies.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
exynos/exynos_fimg2d.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ exynos/exynos_fimg2d.h | 3 ++ 2 files changed, 97 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4d5419c..8703629 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, }
/**
- g2d_move - move content inside single buffer.
- Similar to 'memmove' this moves a rectangular region
- of the provided buffer to another location (the source
- and destination region potentially overlapping).
- @ctx: a pointer to g2d_context structure.
- @img: a pointer to g2d_image structure providing
- buffer information.
- @src_x: x position of source rectangle.
- @src_y: y position of source rectangle.
- @dst_x: x position of destination rectangle.
- @dst_y: y position of destination rectangle.
- @w: width of rectangle to move.
- @h: height of rectangle to move.
- */
+int +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
unsigned int src_x, unsigned int src_y,unsigned int dst_x, unsigned dst_y, unsigned int w,unsigned int h)+{
- union g2d_rop4_val rop4;
- union g2d_point_val pt;
- union g2d_direction_val dir;
- unsigned int src_w, src_h, dst_w, dst_h;
- src_w = w;
- src_h = h;
- if (src_x + img->width > w)
src_w = img->width - src_x;- if (src_y + img->height > h)
src_h = img->height - src_y;- dst_w = w;
- dst_h = w;
- if (dst_x + img->width > w)
dst_w = img->width - dst_x;- if (dst_y + img->height > h)
dst_h = img->height - dst_y;- w = MIN(src_w, dst_w);
- h = MIN(src_h, dst_h);
- if (w == 0 || h == 0) {
fprintf(stderr, MSG_PREFIX "invalid width orheight.\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 intdst_x,
unsigned dst_y, unsigned int w, unsigned int h);int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, struct g2d_image *dst, unsigned int src_x, unsigned int src_y, unsigned int src_w,
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
I was in vacation last week, so I could run your code today. I found that what g2d_move() does is actually copying not moving, because the operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc. Like in libc 'moving' memory doesn't actually move it, like you would move a real object, since it's undefined what 'empty' memory should be.
The same applies here. It's not defined what 'empty' areas of the buffer should be.
Would it be possible to generalize g2d_copy() works better, so it could works well in case of the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.
I also want the user to explicitly request this. The user should make sure what requirements he has for the buffers in question. Are the buffers disjoint or not?
If it is possible, I think it would be better way to do that. If it is not, at least chaning the function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?
I still think the naming is fine though, since it mirrors libc's naming. And the user is supposed to read the documentation anyway.
With best wishes, Tobias
Best regards, Hyungwon Hwang
On Tue, 22 Sep 2015 17:54:58 +0200 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
We already have g2d_copy() which implements G2D copy operations from one buffer to another. However we can't do a overlapping copy operation in one buffer.
Add g2d_move() which acts like the standard memmove() and properly handles overlapping copies.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
exynos/exynos_fimg2d.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ exynos/exynos_fimg2d.h | 3 ++ 2 files changed, 97 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4d5419c..8703629 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, }
/**
- g2d_move - move content inside single buffer.
- Similar to 'memmove' this moves a rectangular region
- of the provided buffer to another location (the source
- and destination region potentially overlapping).
- @ctx: a pointer to g2d_context structure.
- @img: a pointer to g2d_image structure providing
- buffer information.
- @src_x: x position of source rectangle.
- @src_y: y position of source rectangle.
- @dst_x: x position of destination rectangle.
- @dst_y: y position of destination rectangle.
- @w: width of rectangle to move.
- @h: height of rectangle to move.
- */
+int +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
unsigned int src_x, unsigned int src_y,unsigned int dst_x, unsigned dst_y, unsigned int w,unsigned int h)+{
- union g2d_rop4_val rop4;
- union g2d_point_val pt;
- union g2d_direction_val dir;
- unsigned int src_w, src_h, dst_w, dst_h;
- src_w = w;
- src_h = h;
- if (src_x + img->width > w)
src_w = img->width - src_x;- if (src_y + img->height > h)
src_h = img->height - src_y;- dst_w = w;
- dst_h = w;
- if (dst_x + img->width > w)
dst_w = img->width - dst_x;- if (dst_y + img->height > h)
dst_h = img->height - dst_y;- w = MIN(src_w, dst_w);
- h = MIN(src_h, dst_h);
- if (w == 0 || h == 0) {
fprintf(stderr, MSG_PREFIX "invalid width orheight.\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 intdst_x,
unsigned dst_y, unsigned int w, unsigned int h);int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, struct g2d_image *dst, unsigned int src_x, unsigned int src_y, unsigned int src_w,
Hello Tobias,
On Mon, 09 Nov 2015 10:47:02 +0100 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
I was in vacation last week, so I could run your code today. I found that what g2d_move() does is actually copying not moving, because the operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc. Like in libc 'moving' memory doesn't actually move it, like you would move a real object, since it's undefined what 'empty' memory should be.
The same applies here. It's not defined what 'empty' areas of the buffer should be.
Would it be possible to generalize g2d_copy() works better, so it could works well in case of the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.
I also want the user to explicitly request this. The user should make sure what requirements he has for the buffers in question. Are the buffers disjoint or not?
If it is possible, I think it would be better way to do that. If it is not, at least chaning the function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?
I still think the naming is fine though, since it mirrors libc's naming. And the user is supposed to read the documentation anyway.
With best wishes, Tobias
In that manner following glibc, I agree that the naming is reasonable. I commented like that previously, because at the first time when I run the test, I think that the result seems like a bug. The test program said that it was a move test, but the operation seemed copying. It would be just OK if it is well documented or printed when runs the test that the test does not do anything about the previous area intentionally.
BRs, Hyungwon Hwang
Best regards, Hyungwon Hwang
On Tue, 22 Sep 2015 17:54:58 +0200 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
We already have g2d_copy() which implements G2D copy operations from one buffer to another. However we can't do a overlapping copy operation in one buffer.
Add g2d_move() which acts like the standard memmove() and properly handles overlapping copies.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de
exynos/exynos_fimg2d.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ exynos/exynos_fimg2d.h | 3 ++ 2 files changed, 97 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4d5419c..8703629 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -540,6 +540,100 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, }
/**
- g2d_move - move content inside single buffer.
- Similar to 'memmove' this moves a rectangular region
- of the provided buffer to another location (the source
- and destination region potentially overlapping).
- @ctx: a pointer to g2d_context structure.
- @img: a pointer to g2d_image structure providing
- buffer information.
- @src_x: x position of source rectangle.
- @src_y: y position of source rectangle.
- @dst_x: x position of destination rectangle.
- @dst_y: y position of destination rectangle.
- @w: width of rectangle to move.
- @h: height of rectangle to move.
- */
+int +g2d_move(struct g2d_context *ctx, struct g2d_image *img,
unsigned int src_x, unsigned int src_y,unsigned int dst_x, unsigned dst_y, unsigned intw,
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 orheight.\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, unsignedint dst_x,
unsigned dst_y, unsigned int w, unsigned int h);int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, struct g2d_image *dst, unsigned int src_x, unsigned int src_y, unsigned int src_w,
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
On Mon, 09 Nov 2015 10:47:02 +0100 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
I was in vacation last week, so I could run your code today. I found that what g2d_move() does is actually copying not moving, because the operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc. Like in libc 'moving' memory doesn't actually move it, like you would move a real object, since it's undefined what 'empty' memory should be.
The same applies here. It's not defined what 'empty' areas of the buffer should be.
Would it be possible to generalize g2d_copy() works better, so it could works well in case of the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.
I also want the user to explicitly request this. The user should make sure what requirements he has for the buffers in question. Are the buffers disjoint or not?
If it is possible, I think it would be better way to do that. If it is not, at least chaning the function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?
I still think the naming is fine though, since it mirrors libc's naming. And the user is supposed to read the documentation anyway.
With best wishes, Tobias
In that manner following glibc, I agree that the naming is reasonable.
well, that was just my way of thinking. But I guess most people have experience using the libc, so the naming should look at least 'familiar'.
I commented like that previously, because at the first time when I run the test, I think that the result seems like a bug. The test program said that it was a move test, but the operation seemed copying.
Ok, so just that I understand this correctly. Your issue is with the commit the description of the test or with the commit description of the patch that introduces g2d_move()?
Because I don't see what you point out in the test commit description:
" tests/exynos: add test for g2d_move
To check if g2d_move() works properly we create a small checkerboard pattern in the center of the screen and then shift this pattern around with g2d_move(). The pattern should be properly preserved by the operation. "
I intentionally avoid to write "...move this pattern around...", so instead I choose "shift".
I'm not a native speaker, so I'm clueless how to formulate this in a more clear way.
It would be just OK if it is well documented or printed when runs the test that the test does not do anything about the previous area intentionally.
I could add solid fills of the 'empty' areas after each move() operation. Would that be more in line what you think the test should do?
With best wishes, Tobias
BRs, Hyungwon Hwang
Hello Tobias,
On Tue, 10 Nov 2015 14:24:11 +0100 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
On Mon, 09 Nov 2015 10:47:02 +0100 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Hyungwon,
Hyungwon Hwang wrote:
Hello Tobias,
I was in vacation last week, so I could run your code today. I found that what g2d_move() does is actually copying not moving, because the operation does not clear the previous area.
I choose g2d_move() because we also have memcpy() and memmove() in libc. Like in libc 'moving' memory doesn't actually move it, like you would move a real object, since it's undefined what 'empty' memory should be.
The same applies here. It's not defined what 'empty' areas of the buffer should be.
Would it be possible to generalize g2d_copy() works better, so it could works well in case of the src buffer and dst buffer being same.
I think this would break g2d_copy() backward compatibility.
I also want the user to explicitly request this. The user should make sure what requirements he has for the buffers in question. Are the buffers disjoint or not?
If it is possible, I think it would be better way to do that. If it is not, at least chaning the function name is needed. I tested it on my Odroid U3 board.
I don't have a strong opinion on the naming. Any recommendations?
I still think the naming is fine though, since it mirrors libc's naming. And the user is supposed to read the documentation anyway.
With best wishes, Tobias
In that manner following glibc, I agree that the naming is reasonable.
well, that was just my way of thinking. But I guess most people have experience using the libc, so the naming should look at least 'familiar'.
I commented like that previously, because at the first time when I run the test, I think that the result seems like a bug. The test program said that it was a move test, but the operation seemed copying.
Ok, so just that I understand this correctly. Your issue is with the commit the description of the test or with the commit description of the patch that introduces g2d_move()?
Because I don't see what you point out in the test commit description:
" tests/exynos: add test for g2d_move
To check if g2d_move() works properly we create a small checkerboard pattern in the center of the screen and then shift this pattern around with g2d_move(). The pattern should be properly preserved by the operation. "
I intentionally avoid to write "...move this pattern around...", so instead I choose "shift".
I'm not a native speaker, so I'm clueless how to formulate this in a more clear way.
I'm also not a native speaker, so maybe I could not figure out the subtle difference between move and shift. I just thought that 'shift' was just the synonym of 'move'.
It would be just OK if it is well documented or printed when runs the test that the test does not do anything about the previous area intentionally.
I could add solid fills of the 'empty' areas after each move() operation. Would that be more in line what you think the test should do?
No. Because g2d_move() is to g2d_copy() what memmove() is to memcpy(), the current implementation seems enough.
What about change 'move' to 'copy' in the function description?
* g2d_move - copy content inside single buffer. * Similar to 'memmove' this copies a rectangular region * of the provided buffer to another location (the source * and destination region potentially overlapping)
Best regards, Hyungwon Hwang
With best wishes, Tobias
BRs, Hyungwon Hwang
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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 = (unsignedlong)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;elsebreak;}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");
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 = (unsignedlong)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;elsebreak;}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");
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
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,
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);
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
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
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
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
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Hyungwon Hwang -
Tobias Jakobi