Hello,
during the discussion about the last patchset touching the fimg2d code, it became apparent that the error handling for the command submission is currently unsatisfactory.
This series rewrites the handling. All functions that submit command buffers now first check if enough space is available and only then proceed to build the command buffers.
In particular the command buffer is no longer left in a half-finished state, since parameters passed to the functions are now validated before command submission. For this some validation functions are introduced.
This should also increase performance if the bottleneck is the submission part, since adding commands to the buffer is now more lightweight.
Last but not least some prefix was added to messages printed by fprintf and printf, and the G2D context struct was moved out of the public header.
Please review and let me know if I can improve anything!
With best wishes, Tobias
Tobias Jakobi (14): exynos/fimg2d: fix empty buffer handling in g2d_flush() exynos/fimg2d: simplify base address submission in g2d_scale_and_blend() exynos/fimg2d: add g2d_check_space() exynos/fimg2d: check buffer space in g2d_solid_fill() exynos/fimg2d: check buffer space in g2d_copy() exynos/fimg2d: check buffer space in g2d_copy_with_scale() exynos/fimg2d: add g2d_validate_xyz() functions exynos/fimg2d: check buffer space in g2d_blend() exynos/fimg2d: check buffer space in g2d_scale_and_blend() exynos/fimg2d: remove default case from g2d_get_blend_op() exynos/fimg2d: remove superfluous initialization of g2d_point_val exynos/fimg2d: make g2d_add_cmd() less heavy exynos/fimg2d: add message prefix exynos/fimg2d: remove g2d_context from public header
exynos/exynos_fimg2d.c | 367 +++++++++++++++++++++++++++++-------------------- exynos/exynos_fimg2d.h | 14 +- 2 files changed, 222 insertions(+), 159 deletions(-)
Empty command buffers are no error, we just don't have anything to do for flushing then.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 24a06d0..4a88e0c 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -191,7 +191,7 @@ static int g2d_flush(struct g2d_context *ctx) struct drm_exynos_g2d_set_cmdlist cmdlist = {0};
if (ctx->cmd_nr == 0 && ctx->cmd_buf_nr == 0) - return -1; + return 0;
if (ctx->cmdlist_nr >= G2D_MAX_CMD_LIST_NR) { fprintf(stderr, "Overflow cmdlist.\n");
Use g2d_add_base_addr() for source and destination base address just like all other calls.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4a88e0c..85b2317 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -672,12 +672,7 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_NORMAL);
g2d_add_cmd(ctx, DST_COLOR_MODE_REG, dst->color_mode); - if (dst->buf_type == G2D_IMGBUF_USERPTR) - g2d_add_cmd(ctx, DST_BASE_ADDR_REG | G2D_BUF_USERPTR, - (unsigned long)&dst->user_ptr[0]); - else - g2d_add_cmd(ctx, DST_BASE_ADDR_REG, dst->bo[0]); - + g2d_add_base_addr(ctx, dst, g2d_dst); g2d_add_cmd(ctx, DST_STRIDE_REG, dst->stride);
g2d_add_cmd(ctx, SRC_SELECT_REG, src->select_mode); @@ -685,12 +680,7 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src,
switch (src->select_mode) { case G2D_SELECT_MODE_NORMAL: - if (src->buf_type == G2D_IMGBUF_USERPTR) - g2d_add_cmd(ctx, SRC_BASE_ADDR_REG | G2D_BUF_USERPTR, - (unsigned long)&src->user_ptr[0]); - else - g2d_add_cmd(ctx, SRC_BASE_ADDR_REG, src->bo[0]); - + g2d_add_base_addr(ctx, src, g2d_src); g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride); break; case G2D_SELECT_MODE_FGCOLOR:
This is going to be used to check if the command buffers have enough space left prior to actual submission of the commands.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 85b2317..1ae8adf 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -102,6 +102,23 @@ static unsigned int g2d_get_blend_op(enum e_g2d_op op) }
/* + * g2d_check_space - check if command buffers have enough space left. + * + * @ctx: a pointer to g2d_context structure. + * @num_cmds: number of (regular) commands. + * @num_gem_cmds: number of GEM commands. + */ +static unsigned int g2d_check_space(const struct g2d_context *ctx, + unsigned int num_cmds, unsigned int num_gem_cmds) +{ + if (ctx->cmd_nr + num_cmds >= G2D_MAX_CMD_NR || + ctx->cmd_buf_nr + num_gem_cmds >= G2D_MAX_GEM_CMD_NR) + return 1; + else + return 0; +} + +/* * g2d_add_cmd - set given command and value to user side command buffer. * * @ctx: a pointer to g2d_context structure.
The amount of commands (regular and GEM) doesn't depend on the input here.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 1ae8adf..9b7bcce 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -319,6 +319,9 @@ g2d_solid_fill(struct g2d_context *ctx, struct g2d_image *img, union g2d_bitblt_cmd_val bitblt; union g2d_point_val pt;
+ if (g2d_check_space(ctx, 7, 1)) + return -ENOSPC; + g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_NORMAL); g2d_add_cmd(ctx, DST_COLOR_MODE_REG, img->color_mode); g2d_add_base_addr(ctx, img, g2d_dst);
Hello!
Inki Dae wrote:
Hmm, so which 3 (respectively 4) patches should be squashed?
I tried to separate stuff to make review easier. If squashing here is the only issue, do I need to resend the series or can e.g. Emil just do the squash when merging?
With best wishes, Tobias
On 31 August 2015 at 20:27, Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
I believe he meant "squash the introduction of the function and its uses into a single patch".
Not sure how much value that brings, so I'll let you guys decide on the bike shed colour :-)
-Emil
Move the parameter validation before buffer space checking so that we can exit early if it fails. Also don't reset the G2D context anymore in this situation (since the buffers are not partially submitted).
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 9b7bcce..185aa80 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -375,17 +375,7 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, { union g2d_rop4_val rop4; union g2d_point_val pt; - unsigned int src_w = 0, src_h = 0, dst_w = 0, dst_h = 0; - - g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR); - g2d_add_cmd(ctx, DST_COLOR_MODE_REG, dst->color_mode); - g2d_add_base_addr(ctx, dst, g2d_dst); - g2d_add_cmd(ctx, DST_STRIDE_REG, dst->stride); - - g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL); - g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, src->color_mode); - g2d_add_base_addr(ctx, src, g2d_src); - g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride); + unsigned int src_w, src_h, dst_w, dst_h;
src_w = w; src_h = h; @@ -406,10 +396,22 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src,
if (w <= 0 || h <= 0) { fprintf(stderr, "invalid width or height.\n"); - g2d_reset(ctx); return -EINVAL; }
+ if (g2d_check_space(ctx, 11, 2)) + return -ENOSPC; + + g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR); + g2d_add_cmd(ctx, DST_COLOR_MODE_REG, dst->color_mode); + g2d_add_base_addr(ctx, dst, g2d_dst); + g2d_add_cmd(ctx, DST_STRIDE_REG, dst->stride); + + g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL); + g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, src->color_mode); + g2d_add_base_addr(ctx, src, g2d_src); + g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride); + pt.val = 0; pt.data.x = src_x; pt.data.y = src_y;
Parameter validation goes to the top. Repeat mode is checked first to make computation of space easier.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 53 ++++++++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 185aa80..2e04f4a 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -467,23 +467,12 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, { union g2d_rop4_val rop4; union g2d_point_val pt; - unsigned int scale; + unsigned int scale, repeat_pad; unsigned int scale_x, scale_y;
- g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR); - g2d_add_cmd(ctx, DST_COLOR_MODE_REG, dst->color_mode); - g2d_add_base_addr(ctx, dst, g2d_dst); - g2d_add_cmd(ctx, DST_STRIDE_REG, dst->stride); - - g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL); - g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, src->color_mode); - - g2d_add_cmd(ctx, SRC_REPEAT_MODE_REG, src->repeat_mode); - if (src->repeat_mode == G2D_REPEAT_MODE_PAD) - g2d_add_cmd(ctx, SRC_PAD_VALUE_REG, dst->color); - - g2d_add_base_addr(ctx, src, g2d_src); - g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride); + /* Sanitize this parameter to facilitate space computation below. */ + if (negative) + negative = 1;
if (src_w == dst_w && src_h == dst_h) scale = 0; @@ -493,6 +482,8 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, scale_y = g2d_get_scaling(src_h, dst_h); }
+ repeat_pad = src->repeat_mode == G2D_REPEAT_MODE_PAD ? 1 : 0; + if (src_x + src_w > src->width) src_w = src->width - src_x; if (src_y + src_h > src->height) @@ -505,21 +496,37 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src,
if (src_w <= 0 || src_h <= 0 || dst_w <= 0 || dst_h <= 0) { fprintf(stderr, "invalid width or height.\n"); - g2d_reset(ctx); return -EINVAL; }
+ if (g2d_check_space(ctx, 12 + scale * 3 + negative + repeat_pad, 2)) + return -ENOSPC; + + g2d_add_cmd(ctx, DST_SELECT_REG, G2D_SELECT_MODE_BGCOLOR); + g2d_add_cmd(ctx, DST_COLOR_MODE_REG, dst->color_mode); + g2d_add_base_addr(ctx, dst, g2d_dst); + g2d_add_cmd(ctx, DST_STRIDE_REG, dst->stride); + + g2d_add_cmd(ctx, SRC_SELECT_REG, G2D_SELECT_MODE_NORMAL); + g2d_add_cmd(ctx, SRC_COLOR_MODE_REG, src->color_mode); + + g2d_add_cmd(ctx, SRC_REPEAT_MODE_REG, src->repeat_mode); + if (repeat_pad) + g2d_add_cmd(ctx, SRC_PAD_VALUE_REG, dst->color); + + g2d_add_base_addr(ctx, src, g2d_src); + g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride); + + rop4.val = 0; + rop4.data.unmasked_rop3 = G2D_ROP3_SRC; + if (negative) { g2d_add_cmd(ctx, BG_COLOR_REG, 0x00FFFFFF); - rop4.val = 0; - rop4.data.unmasked_rop3 = G2D_ROP3_SRC^G2D_ROP3_DST; - g2d_add_cmd(ctx, ROP4_REG, rop4.val); - } else { - rop4.val = 0; - rop4.data.unmasked_rop3 = G2D_ROP3_SRC; - g2d_add_cmd(ctx, ROP4_REG, rop4.val); + rop4.data.unmasked_rop3 ^= G2D_ROP3_DST; }
+ g2d_add_cmd(ctx, ROP4_REG, rop4.val); + if (scale) { g2d_add_cmd(ctx, SRC_SCALE_CTRL_REG, G2D_SCALE_MODE_BILINEAR); g2d_add_cmd(ctx, SRC_XSCALE_REG, scale_x);
The G2D headers define a number of modes through enums (like e.g. color, select, repeat, etc.).
This introduces g2d_validate_select_mode() and g2d_validate_blending_op() which validate a select mode or blending operation respectively.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 2e04f4a..781aff5 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx, }
/* + * g2d_validate_select_mode - validate select mode. + * + * @mode: the mode to validate + */ +static unsigned int g2d_validate_select_mode( + enum e_g2d_select_mode mode) +{ + switch (mode) { + case G2D_SELECT_MODE_NORMAL: + case G2D_SELECT_MODE_FGCOLOR: + case G2D_SELECT_MODE_BGCOLOR: + return 0; + } + + return 1; +} + +/* + * g2d_validate_blending_op - validate blending operation. + * + * @operation: the operation to validate + */ +static unsigned int g2d_validate_blending_op( + enum e_g2d_op operation) +{ + switch (operation) { + case G2D_OP_CLEAR: + case G2D_OP_SRC: + case G2D_OP_DST: + case G2D_OP_OVER: + case G2D_OP_INTERPOLATE: + case G2D_OP_DISJOINT_CLEAR: + case G2D_OP_DISJOINT_SRC: + case G2D_OP_DISJOINT_DST: + case G2D_OP_CONJOINT_CLEAR: + case G2D_OP_CONJOINT_SRC: + case G2D_OP_CONJOINT_DST: + return 0; + } + + return 1; +} + +/* * g2d_add_cmd - set given command and value to user side command buffer. * * @ctx: a pointer to g2d_context structure.
On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
It's strange use a bit. Just check the range like below,
First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and
if (G2D_SELECT_MODE_MAX < mode) { fprintf(strerr, "invalid command(0x%x)\n", mode); return -EINVAL; }
And I think it'd be better to change return type of this function to int,
so return 0
Ditto, You could modify it like above.
Thanks, Inki Dae
On 31 August 2015 at 14:18, Inki Dae inki.dae@samsung.com wrote:
Listing every value might seem like an overkill, indeed. Yet I think it's the more robust approach.
By adding _MAX to the API we effectively lock it down. That can be avoided, plus the compiler will warn us when new values are added to the enum. If someone starts getting smart (due to the _MAX) and adds G2D_SELECT_MODE_FOO = -1, the above check will fail :(
And I think it'd be better to change return type of this function to int,
Good idea.
Cheers, Emil
Hello,
Emil Velikov wrote:
that's my thinking as well. The overhead shouldn't be too high and the compiler probably optimizes this anyway.
What would be the benefit of this? We're just returning only 0 and 1 anyway. My first reaction was to use a bool here.
Cheers, Emil
With best wishes, Tobias
Move parameter validation to the top and also validate the select mode of the source image and the requested blending operation before starting command submission.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 66 +++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 781aff5..5acccf8 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -623,7 +623,45 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src, union g2d_point_val pt; union g2d_bitblt_cmd_val bitblt; union g2d_blend_func_val blend; - unsigned int src_w = 0, src_h = 0, dst_w = 0, dst_h = 0; + unsigned int gem_space; + unsigned int src_w, src_h, dst_w, dst_h; + + src_w = w; + src_h = h; + if (src_x + w > src->width) + src_w = src->width - src_x; + if (src_y + h > src->height) + src_h = src->height - src_y; + + dst_w = w; + dst_h = h; + if (dst_x + w > dst->width) + dst_w = dst->width - dst_x; + if (dst_y + h > dst->height) + dst_h = dst->height - dst_y; + + w = MIN(src_w, dst_w); + h = MIN(src_h, dst_h); + + if (w <= 0 || h <= 0) { + fprintf(stderr, "invalid width or height.\n"); + return -EINVAL; + } + + if (g2d_validate_select_mode(src->select_mode)) { + fprintf(stderr , "invalid select mode for source.\n"); + return -EINVAL; + } + + if (g2d_validate_blending_op(op)) { + fprintf(stderr , "unsupported blending operation.\n"); + return -EINVAL; + } + + gem_space = src->select_mode == G2D_SELECT_MODE_NORMAL ? 2 : 1; + + if (g2d_check_space(ctx, 12, gem_space)) + return -ENOSPC;
bitblt.val = 0; blend.val = 0; @@ -651,32 +689,6 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src, case G2D_SELECT_MODE_BGCOLOR: g2d_add_cmd(ctx, BG_COLOR_REG, src->color); break; - default: - fprintf(stderr , "failed to set src.\n"); - return -EINVAL; - } - - src_w = w; - src_h = h; - if (src_x + w > src->width) - src_w = src->width - src_x; - if (src_y + h > src->height) - src_h = src->height - src_y; - - dst_w = w; - dst_h = h; - if (dst_x + w > dst->width) - dst_w = dst->width - dst_x; - if (dst_y + h > dst->height) - dst_h = dst->height - dst_y; - - w = MIN(src_w, dst_w); - h = MIN(src_h, dst_h); - - if (w <= 0 || h <= 0) { - fprintf(stderr, "invalid width or height.\n"); - g2d_reset(ctx); - return -EINVAL; }
bitblt.data.alpha_blend_mode = G2D_ALPHA_BLEND_MODE_ENABLE;
Apply the same transformation as in g2d_blend().
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 67 +++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 5acccf8..4274a94 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -745,9 +745,47 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, union g2d_point_val pt; union g2d_bitblt_cmd_val bitblt; union g2d_blend_func_val blend; - unsigned int scale; + unsigned int scale, gem_space; unsigned int scale_x, scale_y;
+ if (src_w == dst_w && src_h == dst_h) + scale = 0; + else { + scale = 1; + scale_x = g2d_get_scaling(src_w, dst_w); + scale_y = g2d_get_scaling(src_h, dst_h); + } + + if (src_x + src_w > src->width) + src_w = src->width - src_x; + if (src_y + src_h > src->height) + src_h = src->height - src_y; + + if (dst_x + dst_w > dst->width) + dst_w = dst->width - dst_x; + if (dst_y + dst_h > dst->height) + dst_h = dst->height - dst_y; + + if (src_w <= 0 || src_h <= 0 || dst_w <= 0 || dst_h <= 0) { + fprintf(stderr, "invalid width or height.\n"); + return -EINVAL; + } + + if (g2d_validate_select_mode(src->select_mode)) { + fprintf(stderr , "invalid select mode for source.\n"); + return -EINVAL; + } + + if (g2d_validate_blending_op(op)) { + fprintf(stderr , "unsupported blending operation.\n"); + return -EINVAL; + } + + gem_space = src->select_mode == G2D_SELECT_MODE_NORMAL ? 2 : 1; + + if (g2d_check_space(ctx, 12 + scale * 3, gem_space)) + return -ENOSPC; + bitblt.val = 0; blend.val = 0;
@@ -774,33 +812,6 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, case G2D_SELECT_MODE_BGCOLOR: g2d_add_cmd(ctx, BG_COLOR_REG, src->color); break; - default: - fprintf(stderr , "failed to set src.\n"); - return -EINVAL; - } - - if (src_w == dst_w && src_h == dst_h) - scale = 0; - else { - scale = 1; - scale_x = g2d_get_scaling(src_w, dst_w); - scale_y = g2d_get_scaling(src_h, dst_h); - } - - if (src_x + src_w > src->width) - src_w = src->width - src_x; - if (src_y + src_h > src->height) - src_h = src->height - src_y; - - if (dst_x + dst_w > dst->width) - dst_w = dst->width - dst_x; - if (dst_y + dst_h > dst->height) - dst_h = dst->height - dst_y; - - if (src_w <= 0 || src_h <= 0 || dst_w <= 0 || dst_h <= 0) { - fprintf(stderr, "invalid width or height.\n"); - g2d_reset(ctx); - return -EINVAL; }
if (scale) {
We now validate the blending mode via g2d_validate_mode() prior to feeding it to g2d_get_blend_op().
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index 4274a94..d708e91 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -91,11 +91,6 @@ static unsigned int g2d_get_blend_op(enum e_g2d_op op) SET_BF(val, G2D_COEFF_MODE_SRC_ALPHA, 0, 0, 0, G2D_COEFF_MODE_SRC_ALPHA, 1, 0, 0); break; - default: - fprintf(stderr, "Not support operation(%d).\n", op); - SET_BF(val, G2D_COEFF_MODE_ONE, 0, 0, 0, G2D_COEFF_MODE_ZERO, - 0, 0, 0); - break; }
return val.val;
On 31 August 2015 at 14:25, Inki Dae inki.dae@samsung.com wrote:
Out of curiosity: why is if/else statement preferred - won't it make things longer/harder to read (there are 11 cases here) ?
Cheers, Emil
Hello Inki,
Inki Dae wrote:
I would like to keep the switch statement here. As Emil has pointed out the big advantage of switch is that the compiler can warn us if we're not dealing with a value of an enum. I think that's definitely a feature which we want here.
I would suggest instead this: I add a short comment to the switch making clear why we don't have a default case here and that a user of g2d_get_blend_op() should first validate any data passed to it.
With best wishes, Tobias
The g2d_point_val union consists of two coordinates of 16 bits. Whenever this union is used though, both coordinates are explicitly set. Hence prior initialization is unnecessary.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 19 ------------------- 1 file changed, 19 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index d708e91..acde645 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -371,15 +371,12 @@ g2d_solid_fill(struct g2d_context *ctx, struct g2d_image *img, if (y + h > img->height) h = img->height - y;
- pt.val = 0; pt.data.x = x; pt.data.y = y; g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val);
- pt.val = 0; pt.data.x = x + w; pt.data.y = y + h; - g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
g2d_add_cmd(ctx, SF_COLOR_REG, img->color); @@ -451,20 +448,16 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, g2d_add_base_addr(ctx, src, g2d_src); g2d_add_cmd(ctx, SRC_STRIDE_REG, src->stride);
- pt.val = 0; pt.data.x = src_x; pt.data.y = src_y; g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = src_x + w; pt.data.y = src_y + h; g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
- pt.val = 0; pt.data.x = dst_x; pt.data.y = dst_y; g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = dst_x + w; pt.data.y = dst_y + h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val); @@ -572,20 +565,16 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, g2d_add_cmd(ctx, SRC_YSCALE_REG, scale_y); }
- pt.val = 0; pt.data.x = src_x; pt.data.y = src_y; g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = src_x + src_w; pt.data.y = src_y + src_h; g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
- pt.val = 0; pt.data.x = dst_x; pt.data.y = dst_y; g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = dst_x + dst_w; pt.data.y = dst_y + dst_h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val); @@ -691,20 +680,16 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src, g2d_add_cmd(ctx, BITBLT_COMMAND_REG, bitblt.val); g2d_add_cmd(ctx, BLEND_FUNCTION_REG, blend.val);
- pt.val = 0; pt.data.x = src_x; pt.data.y = src_y; g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = src_x + w; pt.data.y = src_y + h; g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
- pt.val = 0; pt.data.x = dst_x; pt.data.y = dst_y; g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = dst_x + w; pt.data.y = dst_y + h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val); @@ -820,20 +805,16 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, g2d_add_cmd(ctx, BITBLT_COMMAND_REG, bitblt.val); g2d_add_cmd(ctx, BLEND_FUNCTION_REG, blend.val);
- pt.val = 0; pt.data.x = src_x; pt.data.y = src_y; g2d_add_cmd(ctx, SRC_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = src_x + src_w; pt.data.y = src_y + src_h; g2d_add_cmd(ctx, SRC_RIGHT_BOTTOM_REG, pt.val);
- pt.val = 0; pt.data.x = dst_x; pt.data.y = dst_y; g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val); - pt.val = 0; pt.data.x = dst_x + dst_w; pt.data.y = dst_y + dst_h; g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
The function currently checks for each added command if an overflow of the corresponding command buffers occurs, but none of the callers ever checks the return value.
Since all callers are now converted to use g2d_check_space() simplify the function.
(1) The overflow checks become asserts, so they're only active for debug builds. This is fine since g2d_add_cmd() is not part of the public API.
(2) Switch the return value to void.
(3) Explicitly state that the caller has to check buffer space before calling g2d_add_cmd().
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index acde645..ad44998 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -18,6 +18,7 @@ #include <stdio.h> #include <string.h> #include <errno.h> +#include <assert.h>
#include <sys/mman.h> #include <linux/stddef.h> @@ -163,8 +164,11 @@ static unsigned int g2d_validate_blending_op( * @ctx: a pointer to g2d_context structure. * @cmd: command data. * @value: value data. + * + * The caller has to make sure that the commands buffers have enough space + * left to hold the command. Use g2d_check_space() to ensure this. */ -static int g2d_add_cmd(struct g2d_context *ctx, unsigned long cmd, +static void g2d_add_cmd(struct g2d_context *ctx, unsigned long cmd, unsigned long value) { switch (cmd & ~(G2D_BUF_USERPTR)) { @@ -174,28 +178,20 @@ static int g2d_add_cmd(struct g2d_context *ctx, unsigned long cmd, case DST_PLANE2_BASE_ADDR_REG: case PAT_BASE_ADDR_REG: case MASK_BASE_ADDR_REG: - if (ctx->cmd_buf_nr >= G2D_MAX_GEM_CMD_NR) { - fprintf(stderr, "Overflow cmd_gem size.\n"); - return -EINVAL; - } + assert(ctx->cmd_buf_nr < G2D_MAX_GEM_CMD_NR);
ctx->cmd_buf[ctx->cmd_buf_nr].offset = cmd; ctx->cmd_buf[ctx->cmd_buf_nr].data = value; ctx->cmd_buf_nr++; break; default: - if (ctx->cmd_nr >= G2D_MAX_CMD_NR) { - fprintf(stderr, "Overflow cmd size.\n"); - return -EINVAL; - } + assert(ctx->cmd_nr < G2D_MAX_CMD_NR);
ctx->cmd[ctx->cmd_nr].offset = cmd; ctx->cmd[ctx->cmd_nr].data = value; ctx->cmd_nr++; break; } - - return 0; }
/*
Add a prefix to the messages printed to the console via printf() and fprintf() so that one can easily see where the message comes from.
Signed-off-by: Tobias Jakobi tjakobi@math.uni-bielefeld.de --- exynos/exynos_fimg2d.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c index ad44998..91df441 100644 --- a/exynos/exynos_fimg2d.c +++ b/exynos/exynos_fimg2d.c @@ -42,6 +42,8 @@
#define MIN(a, b) ((a) < (b) ? (a) : (b))
+#define MSG_PREFIX "exynos/fimg2d: " + enum g2d_base_addr_reg { g2d_dst = 0, g2d_src @@ -246,7 +248,7 @@ static int g2d_flush(struct g2d_context *ctx) return 0;
if (ctx->cmdlist_nr >= G2D_MAX_CMD_LIST_NR) { - fprintf(stderr, "Overflow cmdlist.\n"); + fprintf(stderr, MSG_PREFIX "command list overflow.\n"); return -EINVAL; }
@@ -262,7 +264,7 @@ static int g2d_flush(struct g2d_context *ctx)
ret = drmIoctl(ctx->fd, DRM_IOCTL_EXYNOS_G2D_SET_CMDLIST, &cmdlist); if (ret < 0) { - fprintf(stderr, "failed to set cmdlist.\n"); + fprintf(stderr, MSG_PREFIX "failed to set cmdlist.\n"); return ret; }
@@ -284,7 +286,7 @@ struct g2d_context *g2d_init(int fd)
ctx = calloc(1, sizeof(*ctx)); if (!ctx) { - fprintf(stderr, "failed to allocate context.\n"); + fprintf(stderr, MSG_PREFIX "failed to allocate context.\n"); return NULL; }
@@ -292,7 +294,7 @@ struct g2d_context *g2d_init(int fd)
ret = drmIoctl(fd, DRM_IOCTL_EXYNOS_G2D_GET_VER, &ver); if (ret < 0) { - fprintf(stderr, "failed to get version.\n"); + fprintf(stderr, MSG_PREFIX "failed to get version.\n"); free(ctx); return NULL; } @@ -300,7 +302,7 @@ struct g2d_context *g2d_init(int fd) ctx->major = ver.major; ctx->minor = ver.minor;
- printf("g2d version(%d.%d).\n", ctx->major, ctx->minor); + printf(MSG_PREFIX "G2D version (%d.%d).\n", ctx->major, ctx->minor); return ctx; }
@@ -326,7 +328,7 @@ int g2d_exec(struct g2d_context *ctx)
ret = drmIoctl(ctx->fd, DRM_IOCTL_EXYNOS_G2D_EXEC, &exec); if (ret < 0) { - fprintf(stderr, "failed to execute.\n"); + fprintf(stderr, MSG_PREFIX "failed to execute.\n"); return ret; }
@@ -427,7 +429,7 @@ g2d_copy(struct g2d_context *ctx, struct g2d_image *src, h = MIN(src_h, dst_h);
if (w <= 0 || h <= 0) { - fprintf(stderr, "invalid width or height.\n"); + fprintf(stderr, MSG_PREFIX "invalid width or height.\n"); return -EINVAL; }
@@ -523,7 +525,7 @@ g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src, dst_h = dst->height - dst_y;
if (src_w <= 0 || src_h <= 0 || dst_w <= 0 || dst_h <= 0) { - fprintf(stderr, "invalid width or height.\n"); + fprintf(stderr, MSG_PREFIX "invalid width or height.\n"); return -EINVAL; }
@@ -624,17 +626,17 @@ g2d_blend(struct g2d_context *ctx, struct g2d_image *src, h = MIN(src_h, dst_h);
if (w <= 0 || h <= 0) { - fprintf(stderr, "invalid width or height.\n"); + fprintf(stderr, MSG_PREFIX "invalid width or height.\n"); return -EINVAL; }
if (g2d_validate_select_mode(src->select_mode)) { - fprintf(stderr , "invalid select mode for source.\n"); + fprintf(stderr , MSG_PREFIX "invalid select mode for source.\n"); return -EINVAL; }
if (g2d_validate_blending_op(op)) { - fprintf(stderr , "unsupported blending operation.\n"); + fprintf(stderr , MSG_PREFIX "unsupported blending operation.\n"); return -EINVAL; }
@@ -743,17 +745,17 @@ g2d_scale_and_blend(struct g2d_context *ctx, struct g2d_image *src, dst_h = dst->height - dst_y;
if (src_w <= 0 || src_h <= 0 || dst_w <= 0 || dst_h <= 0) { - fprintf(stderr, "invalid width or height.\n"); + fprintf(stderr, MSG_PREFIX "invalid width or height.\n"); return -EINVAL; }
if (g2d_validate_select_mode(src->select_mode)) { - fprintf(stderr , "invalid select mode for source.\n"); + fprintf(stderr , MSG_PREFIX "invalid select mode for source.\n"); return -EINVAL; }
if (g2d_validate_blending_op(op)) { - fprintf(stderr , "unsupported blending operation.\n"); + fprintf(stderr , MSG_PREFIX "unsupported blending operation.\n"); return -EINVAL; }
Hi Tobias,
On 24 August 2015 at 15:13, Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Thanks for going with my earlier suggestion and untangling all this.
I've went through the lot and it looks great afaict. Fwiw for the series Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
As pretty much none of this is hardware specific and/or requires additional knowledge of the kernel module I'm inclined to pull this in even if we don't get too many reviewers. We better keep it around for a couple of weeks in case others are swamped with unrelated work atm, yet willing to take a look.
Cheers, Emil
On 2015년 08월 28일 05:46, Tobias Jakobi wrote:
I had a review and looks good to me. There are just cleanup issues but no problem to upstream them as-is. However, in my opinion, I think the patch 7 could be more cleaned up.
Signed-off-by: Inki Dae inki.dae@samsung.com
Thanks, Inki Dae
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Inki Dae -
Tobias Jakobi