[PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[Tobias Jakobi]

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

-- 
2.0.5

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

[PATCH 01/14] exynos/fimg2d: fix empty buffer handling in g2d_flush()

[Tobias Jakobi]

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

[PATCH 02/14] exynos/fimg2d: simplify base address submission in g2d_scale_and_blend()

[Tobias Jakobi]

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

[PATCH 03/14] exynos/fimg2d: add g2d_check_space()

[Tobias Jakobi]

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

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

[PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Tobias Jakobi]

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

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

Re: [PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Inki Dae]

On 2015년 08월 24일 23:13, Tobias Jakobi wrote:
> 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;

You can make 3 and 4 patches to one. These should be same patch.

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

Re: [PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Tobias Jakobi]

Hello!


Inki Dae wrote:
> On 2015년 08월 24일 23:13, Tobias Jakobi wrote:
>> 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;
> 
> You can make 3 and 4 patches to one. These should be same patch.
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


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

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

Re: [PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Emil Velikov]

On 31 August 2015 at 20:27, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> Hello!
>
> Inki Dae wrote:
>> On 2015년 08월 24일 23:13, Tobias Jakobi wrote:

>>> +    if (g2d_check_space(ctx, 7, 1))
>>> +            return -ENOSPC;
>>
>> You can make 3 and 4 patches to one. These should be same patch.
> Hmm, so which 3 (respectively 4) patches should be squashed?
>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/14] exynos/fimg2d: check buffer space in g2d_solid_fill()

[Inki Dae]

On 2015년 09월 01일 04:57, Emil Velikov wrote:
> On 31 August 2015 at 20:27, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>> Hello!
>>
>> Inki Dae wrote:
>>> On 2015년 08월 24일 23:13, Tobias Jakobi wrote:
> 
>>>> +    if (g2d_check_space(ctx, 7, 1))
>>>> +            return -ENOSPC;
>>>
>>> You can make 3 and 4 patches to one. These should be same patch.
>> Hmm, so which 3 (respectively 4) patches should be squashed?
>>
> 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 :-)

That - including relevant my comments - is just my opinion so no problem
to upstream it as is. This is really trivial.

Thanks,
Inki Dae

> 
> -Emil
> --
> 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
> 

[PATCH 05/14] exynos/fimg2d: check buffer space in g2d_copy()

[Tobias Jakobi]

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

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

Re: [PATCH 05/14] exynos/fimg2d: check buffer space in g2d_copy()

[Inki Dae]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> 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;

Above lines could be integrated with 3 and 4 patches as one patch. And
you can make other codes to other one.

Thanks,
Inki Dae

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

[PATCH 06/14] exynos/fimg2d: check buffer space in g2d_copy_with_scale()

[Tobias Jakobi]

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

[PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Tobias Jakobi]

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

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

Re: [PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Inki Dae]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> 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;
> +	}

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,

> +
> +	return 1;

so return 0

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

Ditto, You could modify it like above.

Thanks,
Inki Dae

> +	}
> +
> +	return 1;
> +}
> +
> +/*
>   * g2d_add_cmd - set given command and value to user side command buffer.
>   *
>   * @ctx: a pointer to g2d_context structure.
> 

Re: [PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Emil Velikov]

On 31 August 2015 at 14:18, Inki Dae <inki.dae@samsung.com> wrote:
> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>> 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;
>> +     }
>
> 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;
> }
>
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

Re: [PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Tobias Jakobi]

Hello,


Emil Velikov wrote:
> On 31 August 2015 at 14:18, Inki Dae <inki.dae@samsung.com> wrote:
>> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>>> 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;
>>> +     }
>>
>> 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;
>> }
>>
> Listing every value might seem like an overkill, indeed. Yet I think
> it's the more robust approach.
that's my thinking as well. The overhead shouldn't be too high and the
compiler probably optimizes this anyway.



> 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.
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

[PATCH 08/14] exynos/fimg2d: check buffer space in g2d_blend()

[Tobias Jakobi]

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

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

Re: [PATCH 08/14] exynos/fimg2d: check buffer space in g2d_blend()

[Inki Dae]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> 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;

As I mentioned before, above two lines could be integrated with other
patches.

Thanks,
Inki Dae

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

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

[PATCH 09/14] exynos/fimg2d: check buffer space in g2d_scale_and_blend()

[Tobias Jakobi]

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

Re: [PATCH 09/14] exynos/fimg2d: check buffer space in g2d_scale_and_blend()

[Inki Dae]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> 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;

Ditto.

Thanks,
Inki Dae

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

[PATCH 10/14] exynos/fimg2d: remove default case from g2d_get_blend_op()

[Tobias Jakobi]

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

Re: [PATCH 10/14] exynos/fimg2d: remove default case from g2d_get_blend_op()

[Inki Dae]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> 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;

With this, how about changing above switch and case statement to if
statement?

Thanks,
Inki Dae


>  	}
>  
>  	return val.val;
> 

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

Re: [PATCH 10/14] exynos/fimg2d: remove default case from g2d_get_blend_op()

[Emil Velikov]

On 31 August 2015 at 14:25, Inki Dae <inki.dae@samsung.com> wrote:
> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>> 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;
>
> With this, how about changing above switch and case statement to if
> statement?
>
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

Re: [PATCH 10/14] exynos/fimg2d: remove default case from g2d_get_blend_op()

[Inki Dae]

On 2015년 09월 01일 03:53, Emil Velikov wrote:
> On 31 August 2015 at 14:25, Inki Dae <inki.dae@samsung.com> wrote:
>> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>>> 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;
>>
>> With this, how about changing above switch and case statement to if
>> statement?
>>
> Out of curiosity: why is if/else statement preferred - won't it make
> things longer/harder to read (there are 11 cases here) ?

Just looks strange to me switch and case statement has no default
statement. This is just my opinion and trivial.

Thanks,
Inki Dae

> 
> Cheers,
> Emil
> --
> 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
> 

Re: [PATCH 10/14] exynos/fimg2d: remove default case from g2d_get_blend_op()

[Tobias Jakobi]

Hello Inki,


Inki Dae wrote:
> On 2015년 09월 01일 03:53, Emil Velikov wrote:
>> On 31 August 2015 at 14:25, Inki Dae <inki.dae@samsung.com> wrote:
>>> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>>>> 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;
>>>
>>> With this, how about changing above switch and case statement to if
>>> statement?
>>>
>> Out of curiosity: why is if/else statement preferred - won't it make
>> things longer/harder to read (there are 11 cases here) ?
> 
> Just looks strange to me switch and case statement has no default
> statement. This is just my opinion and trivial.
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


> 
> Thanks,
> Inki Dae
> 
>>
>> Cheers,
>> Emil
>> --
>> 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
>>
> 

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

[PATCH 11/14] exynos/fimg2d: remove superfluous initialization of g2d_point_val

[Tobias Jakobi]

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

[PATCH 12/14] exynos/fimg2d: make g2d_add_cmd() less heavy

[Tobias Jakobi]

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

[PATCH 13/14] exynos/fimg2d: add message prefix

[Tobias Jakobi]

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

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

Re: [PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[Emil Velikov]

Hi Tobias,

On 24 August 2015 at 15:13, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
> 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.
>
>
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

Re: [PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[Tobias Jakobi]

Hey Emil,


Emil Velikov wrote:
> Hi Tobias,
> 
> On 24 August 2015 at 15:13, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>> 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.
>>
>>
> 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>
thanks for the review and the help on IRC!


> 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.
Sure, I'm going to wait and do some pings from time to time :)


With best wishes,
Tobias

> 
> Cheers,
> Emil
> 

Re: [PATCH 00/14] drm/exynos: rewrite fimg2d error handling

[Inki Dae]

On 2015년 08월 28일 05:46, Tobias Jakobi wrote:
> Hey Emil,
> 
> 
> Emil Velikov wrote:
>> Hi Tobias,
>>
>> On 24 August 2015 at 15:13, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote:
>>> 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.
>>>
>>>
>> 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>
> thanks for the review and the help on IRC!
> 
> 
>> 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.
> Sure, I'm going to wait and do some pings from time to time :)
> 

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


> 
> With best wishes,
> Tobias
> 
>>
>> Cheers,
>> Emil
>>
> 
> --
> 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
> 

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