[PATCH 0/9] drm/exynos: cleanups and small fixes for libdrm

[PATCH 0/9] drm/exynos: cleanups and small fixes for libdrm

[Tobias Jakobi]

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

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(-)

-- 
2.0.5

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

[PATCH 1/9] exynos: fimg2d: fix return codes

[Tobias Jakobi]

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.

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);
 }
-- 
2.0.5

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

[PATCH 2/9] tests/exynos: replace return by break

[Tobias Jakobi]

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;
 		}
 	}
 
-- 
2.0.5

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

[PATCH 3/9] exynos/fimg2d: simplify g2d_fini()

[Tobias Jakobi]

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);
 }
 
 /**
-- 
2.0.5

[PATCH 4/9] tests/exynos: clean struct connector

[Tobias Jakobi]

Remove all unused struct members.

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);
-- 
2.0.5

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

[PATCH 5/9] tests/exynos: remove unused define

[Tobias Jakobi]

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

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

[PATCH 6/9] tests/exynos: remove struct fimg2d_test_case

[Tobias Jakobi]

It doesn't make sense to keep this structure, since we
can just call all tests directly.

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");
 
-- 
2.0.5

[PATCH 7/9] tests/exynos: simplify drm_set_crtc

[Tobias Jakobi]

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;
 }
 
-- 
2.0.5

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

[PATCH 8/9] tests/exynos: remove connector_find_plane

[Tobias Jakobi]

No test uses DRM planes at the moment so this function
is never called.

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)
 {
-- 
2.0.5

[PATCH 9/9] tests/exynos: handle G2D_IMGBUF_COLOR in switch statements

[Tobias Jakobi]

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

Re: [PATCH 1/9] exynos: fimg2d: fix return codes

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 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.
>
> 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);
>
Strictly speaking g2d_add_cmd() can fail, and all the functions that
build upon it. In the latter case most ignore the return value which
is a bit bad. That plus the fact that these are part of the public API
makes things easier to misuse.

One way to avoid all this is to implement an internal function that
does the size checks ahead of time, and drop them from g2d_add_cmd(),
apart from this patch.

The nouveau mesa drivers do a similar thing - see PUSH_SPACE().

-Emil

Re: [PATCH 4/9] tests/exynos: clean struct connector

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> Remove all unused struct members.
>
Mentioning if they were used at some point in the past will be great.

Thanks
Emil

> 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);
> --
> 2.0.5
>

Re: [PATCH 8/9] tests/exynos: remove connector_find_plane

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> No test uses DRM planes at the moment so this function
> is never called.
>
Similar to the other patch - mention if this been used previously. I'm
assuming that the exynos guys might have some work stashed which
depends on this ?

-Emil

Re: [PATCH 6/9] tests/exynos: remove struct fimg2d_test_case

[Emil Velikov]

On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> It doesn't make sense to keep this structure, since we
> can just call all tests directly.
>
Seems like it was designed in mind of having another test case.
Perhaps the exynos guys cannot really upstream it (licensing or other
issues) ?

-Emil

Re: [PATCH 1/9] exynos: fimg2d: fix return codes

[Tobias Jakobi]

Hello Emil,


Emil Velikov wrote:
> On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>> 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.
>>
>> 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);
>>
> Strictly speaking g2d_add_cmd() can fail, and all the functions that
> build upon it. In the latter case most ignore the return value which
> is a bit bad. That plus the fact that these are part of the public API
> makes things easier to misuse.
I'm totally aware of that. In fact I've already rewritten the error
checking logic. But that would be for a later series.

I prefer to do this in small steps, in particular because I see the
tendency that nobody from Samsung reviews the larger patches anyway. Or
any patches at all. And yes, I'm voicing my frustration here...



> One way to avoid all this is to implement an internal function that
> does the size checks ahead of time, and drop them from g2d_add_cmd(),
> apart from this patch.
I'm doing something different, which however results in more or less the
same thing.


> 
> The nouveau mesa drivers do a similar thing - see PUSH_SPACE().
> 
> -Emil
> 


With best wishes,
Tobias

Re: [PATCH 4/9] tests/exynos: clean struct connector

[Tobias Jakobi]

Hello Emil,

Emil Velikov wrote:
> On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>> Remove all unused struct members.
>>
> Mentioning if they were used at some point in the past will be great.
OK, I'll take a look at git history but AFAIK it is not.


With best wishes,
Tobias


> 
> Thanks
> Emil
> 
>> 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);
>> --
>> 2.0.5
>>

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

Re: [PATCH 6/9] tests/exynos: remove struct fimg2d_test_case

[Tobias Jakobi]

Hello Emil,

Emil Velikov wrote:
> On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>> It doesn't make sense to keep this structure, since we
>> can just call all tests directly.
>>
> Seems like it was designed in mind of having another test case.
> Perhaps the exynos guys cannot really upstream it (licensing or other
> issues) ?
might be. But why should we care about that anyway?

My personal guess: Big giant code bomb which noone bothered to check. ;)


With best wishes,
Tobias



> 
> -Emil
> 

Re: [PATCH 6/9] tests/exynos: remove struct fimg2d_test_case

[Emil Velikov]

On 12 June 2015 at 17:25, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> Hello Emil,
>
> Emil Velikov wrote:
>> On 10 June 2015 at 14:42, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>> It doesn't make sense to keep this structure, since we
>>> can just call all tests directly.
>>>
>> Seems like it was designed in mind of having another test case.
>> Perhaps the exynos guys cannot really upstream it (licensing or other
>> issues) ?
> might be. But why should we care about that anyway?
>
Would be nice to avoid giving the guys the finger considering they
don't have much time to dedicate to libdrm_exynos. For all we know all
of the issues (if any) are resolved and they're dusting off the
patches, as we speak.

> My personal guess: Big giant code bomb which noone bothered to check. ;)
>
Perhaps somewhere around rev 4 (of 20) it was used, and after that
people just forgot about it :-P

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