Hello,
I've split off the Exynos specific bits, so this is just some cleanups and small fixes. Everything can be reviewed without knowledge about the Exynos platform. My hope is that I can get at least some of the patches from my last series upstream.
With best wishes, Tobias
Changes in v2: - Made it clear that the error handling is currently 'unsatisfactory', but that this is going to be adressed in a later patch. - Point out that removed code also wasn't used anywhere in the past (by inspection of git history).
Tobias Jakobi (9): exynos: fimg2d: fix return codes tests/exynos: replace return by break exynos/fimg2d: simplify g2d_fini() tests/exynos: clean struct connector tests/exynos: remove unused define tests/exynos: remove struct fimg2d_test_case tests/exynos: simplify drm_set_crtc tests/exynos: remove connector_find_plane tests/exynos: handle G2D_IMGBUF_COLOR in switch statements
exynos/exynos_fimg2d.c | 23 ++------ tests/exynos/exynos_fimg2d_test.c | 112 +++++++------------------------------- 2 files changed, 26 insertions(+), 109 deletions(-)
Even if flushing the command buffer doesn't succeed, the G2D calls would still return zero. Fix this by just passing the flush return code.
In fact error handling currently ignores the fact that g2d_add_cmd() can fail. This is going to be handled in a later patch.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 86ae898..5ea42e6 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -330,9 +330,7 @@ g2d_solid_fill(struct g2d_context *ctx, struct g2d_image *img, bitblt.data.fast_solid_color_fill_en = 1; g2d_add_cmd(ctx, BITBLT_COMMAND_REG, bitblt.val);
- g2d_flush(ctx); - - return 0; + return g2d_flush(ctx); }
/** @@ -415,9 +413,7 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, rop4.data.unmasked_rop3 = G2D_ROP3_SRC; g2d_add_cmd(ctx, ROP4_REG, rop4.val);
- g2d_flush(ctx); - - return 0; + return g2d_flush(ctx); }
/** @@ -527,9 +523,7 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, pt.data.y = dst_y + dst_h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
- g2d_flush(ctx); - - return 0; + return g2d_flush(ctx); }
/** @@ -636,9 +630,7 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src, pt.data.y = dst_y + h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
- g2d_flush(ctx); - - return 0; + return g2d_flush(ctx); }
/** @@ -766,7 +758,5 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, pt.data.y = dst_y + dst_h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
- g2d_flush(ctx); - - return 0; + return g2d_flush(ctx); }
The 'usage' function already does exit(0), so that this 'return -EINVAL' is never called. Just put a break there to avoid confusion.
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 e64bb32..64dacfa 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -689,7 +689,7 @@ int main(int argc, char **argv) break; default: usage(argv[0]); - return -EINVAL; + break; } }
free()ing a nullptr is a noop, so remove the check.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 5ea42e6..24a06d0 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -254,8 +254,7 @@ struct g2d_context *g2d_init(int fd)
void g2d_fini(struct g2d_context *ctx) { - if (ctx) - free(ctx); + free(ctx); }
/**
Remove all unused struct members. An inspection of the git history shows that these members were also never used in the past.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index 64dacfa..6af9277 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -65,17 +65,9 @@ struct fimg2d_test_case { struct connector { uint32_t id; char mode_str[64]; - char format_str[5]; - unsigned int fourcc; drmModeModeInfo *mode; drmModeEncoder *encoder; int crtc; - int pipe; - int plane_zpos; - unsigned int fb_id[2], current_fb_id; - struct timeval start; - - int swap_count; };
static void connector_find_mode(int fd, struct connector *c, @@ -750,8 +742,6 @@ int main(int argc, char **argv) if (ret < 0) goto err_destroy_buffer;
- con.plane_zpos = -1; - memset(bo->vaddr, 0xff, screen_width * screen_height * 4);
ret = drm_set_crtc(dev, &con, fb_id);
It doesn't make sense to limit the number of test cases anyway.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index 6af9277..080d178 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -34,7 +34,6 @@ #include "exynos_fimg2d.h"
#define DRM_MODULE_NAME "exynos" -#define MAX_TEST_CASE 8
static unsigned int screen_width, screen_height;
It doesn't make sense to keep this structure, since we can just call all tests directly. An inspection of the git history shows that no code ever used this abstraction in the past.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 42 +++++---------------------------------- 1 file changed, 5 insertions(+), 37 deletions(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index 080d178..de6a2b7 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -37,30 +37,6 @@
static unsigned int screen_width, screen_height;
-/* - * A structure to test fimg2d hw. - * - * @solid_fill: fill given color data to source buffer. - * @copy: copy source to destination buffer. - * @copy_with_scale: copy source to destination buffer scaling up or - * down properly. - * @blend: blend source to destination buffer. - */ -struct fimg2d_test_case { - int (*solid_fill)(struct exynos_device *dev, struct exynos_bo *dst); - int (*copy)(struct exynos_device *dev, struct exynos_bo *src, - struct exynos_bo *dst, enum e_g2d_buf_type); - int (*copy_with_scale)(struct exynos_device *dev, - struct exynos_bo *src, struct exynos_bo *dst, - enum e_g2d_buf_type); - int (*blend)(struct exynos_device *dev, - struct exynos_bo *src, struct exynos_bo *dst, - enum e_g2d_buf_type); - int (*checkerboard)(struct exynos_device *dev, - struct exynos_bo *src, struct exynos_bo *dst, - enum e_g2d_buf_type); -}; - struct connector { uint32_t id; char mode_str[64]; @@ -630,14 +606,6 @@ fail: return ret; }
-static struct fimg2d_test_case test_case = { - .solid_fill = &g2d_solid_fill_test, - .copy = &g2d_copy_test, - .copy_with_scale = &g2d_copy_with_scale_test, - .blend = &g2d_blend_test, - .checkerboard = &g2d_checkerboard_test, -}; - static void usage(char *name) { fprintf(stderr, "usage: %s [-s]\n", name); @@ -747,7 +715,7 @@ int main(int argc, char **argv) if (ret < 0) goto err_rm_fb;
- ret = test_case.solid_fill(dev, bo); + ret = g2d_solid_fill_test(dev, bo); if (ret < 0) { fprintf(stderr, "failed to solid fill operation.\n"); goto err_rm_fb; @@ -761,7 +729,7 @@ int main(int argc, char **argv) goto err_rm_fb; }
- ret = test_case.copy(dev, src, bo, G2D_IMGBUF_GEM); + ret = g2d_copy_test(dev, src, bo, G2D_IMGBUF_GEM); if (ret < 0) { fprintf(stderr, "failed to test copy operation.\n"); goto err_free_src; @@ -769,7 +737,7 @@ int main(int argc, char **argv)
wait_for_user_input(0);
- ret = test_case.copy_with_scale(dev, src, bo, G2D_IMGBUF_GEM); + 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"); goto err_free_src; @@ -777,7 +745,7 @@ int main(int argc, char **argv)
wait_for_user_input(0);
- ret = test_case.checkerboard(dev, src, bo, G2D_IMGBUF_GEM); + ret = g2d_checkerboard_test(dev, src, bo, G2D_IMGBUF_GEM); if (ret < 0) { fprintf(stderr, "failed to issue checkerboard test.\n"); goto err_free_src; @@ -794,7 +762,7 @@ int main(int argc, char **argv) * Disable the test for now, until the kernel code has been sanitized. */ #if 0 - ret = test_case.blend(dev, src, bo, G2D_IMGBUF_USERPTR); + ret = g2d_blend_test(dev, src, bo, G2D_IMGBUF_USERPTR); if (ret < 0) fprintf(stderr, "failed to test blend operation.\n");
We can just return 'ret' here, the goto serves no purpose.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index de6a2b7..1ec7340 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -150,14 +150,9 @@ static int drm_set_crtc(struct exynos_device *dev, struct connector *c,
ret = drmModeSetCrtc(dev->fd, c->crtc, fb_id, 0, 0, &c->id, 1, c->mode); - if (ret) { + if (ret) drmMsg("failed to set mode: %s\n", strerror(errno)); - goto err; - } - - return 0;
-err: return ret; }
No test uses DRM planes at the moment so this function is never called. Inspection of the git history shows that DRM planes were also never used in these tests in the past.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 31 ------------------------------- 1 file changed, 31 deletions(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index 1ec7340..59de4ba 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -112,37 +112,6 @@ static void connector_find_mode(int fd, struct connector *c, c->crtc = c->encoder->crtc_id; }
-static int connector_find_plane(int fd, unsigned int *plane_id) -{ - drmModePlaneRes *plane_resources; - drmModePlane *ovr; - int i; - - plane_resources = drmModeGetPlaneResources(fd); - if (!plane_resources) { - fprintf(stderr, "drmModeGetPlaneResources failed: %s\n", - strerror(errno)); - return -1; - } - - for (i = 0; i < plane_resources->count_planes; i++) { - plane_id[i] = 0; - - ovr = drmModeGetPlane(fd, plane_resources->planes[i]); - if (!ovr) { - fprintf(stderr, "drmModeGetPlane failed: %s\n", - strerror(errno)); - continue; - } - - if (ovr->possible_crtcs & (1 << 0)) - plane_id[i] = ovr->plane_id; - drmModeFreePlane(ovr); - } - - return 0; -} - static int drm_set_crtc(struct exynos_device *dev, struct connector *c, unsigned int fb_id) {
This fixes a compiler warning about missing handling of enum values in the switch statements.
Also remove the silent mapping to G2D_IMGBUF_GEM when an unknown buffer type is encountered. We have full control about the type here, and if it's unknown then we obviously have a bug in the code.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- tests/exynos/exynos_fimg2d_test.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c index 59de4ba..8794dac 100644 --- a/tests/exynos/exynos_fimg2d_test.c +++ b/tests/exynos/exynos_fimg2d_test.c @@ -270,9 +270,10 @@ static int g2d_copy_test(struct exynos_device *dev, struct exynos_bo *src, src_img.user_ptr[0].userptr = userptr; src_img.user_ptr[0].size = size; break; + case G2D_IMGBUF_COLOR: default: - type = G2D_IMGBUF_GEM; - break; + ret = -EFAULT; + goto fail; }
printf("copy test with %s.\n", @@ -306,6 +307,7 @@ err_free_userptr: if (userptr) free((void *)userptr);
+fail: g2d_fini(ctx);
return ret; @@ -349,9 +351,10 @@ static int g2d_copy_with_scale_test(struct exynos_device *dev, src_img.user_ptr[0].userptr = userptr; src_img.user_ptr[0].size = size; break; + case G2D_IMGBUF_COLOR: default: - type = G2D_IMGBUF_GEM; - break; + ret = -EFAULT; + goto fail; }
printf("copy and scale test with %s.\n", @@ -390,6 +393,7 @@ err_free_userptr: if (userptr) free((void *)userptr);
+fail: g2d_fini(ctx);
return 0; @@ -435,9 +439,10 @@ static int g2d_blend_test(struct exynos_device *dev, src_img.user_ptr[0].userptr = userptr; src_img.user_ptr[0].size = size; break; + case G2D_IMGBUF_COLOR: default: - type = G2D_IMGBUF_GEM; - break; + ret = -EFAULT; + goto fail; }
printf("blend test with %s.\n", @@ -487,6 +492,7 @@ err_free_userptr: if (userptr) free((void *)userptr);
+fail: g2d_fini(ctx);
return 0; @@ -532,6 +538,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev, src_img.user_ptr[0].userptr = (unsigned long)checkerboard; src_img.user_ptr[0].size = img_w * img_h * 4; break; + case G2D_IMGBUF_COLOR: default: ret = -EFAULT; goto fail;
On 12 June 2015 at 18:15, Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello,
I've split off the Exynos specific bits, so this is just some cleanups and small fixes. Everything can be reviewed without knowledge about the Exynos platform. My hope is that I can get at least some of the patches from my last series upstream.
With best wishes, Tobias
Changes in v2:
- Made it clear that the error handling is currently 'unsatisfactory', but that this is going to be adressed in a later patch.
- Point out that removed code also wasn't used anywhere in the past (by inspection of git history).
Big thanks for the update. It will make things clear a week/month down the road - when we've forgotten pretty much everything in the area :-) Considering how trivial these are and the silence from the Samsung/Exynos folk, I'll be pushing these later on today.
If some of the dead code ends up required, we can always bring it back.
Cheers, Emil
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Tobias Jakobi