[PATCH 1/3] exynos: fix g2d_copy

[PATCH 1/3] exynos: fix g2d_copy

[Tobias Jakobi]

---
 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 0b14618..a565910 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -382,7 +382,7 @@ int g2d_copy(struct g2d_context *ctx, struct g2d_image *src,
 	g2d_add_cmd(ctx, DST_LEFT_TOP_REG, pt.val);
 	pt.val = 0;
 	pt.data.x = dst_x + w;
-	pt.data.y = dst_x + h;
+	pt.data.y = dst_y + h;
 	g2d_add_cmd(ctx, DST_RIGHT_BOTTOM_REG, pt.val);
 
 	rop4.val = 0;
-- 
1.8.5.5

[PATCH 2/3] exynos: fix G2D_DOUBLE_TO_FIXED

[Tobias Jakobi]

---
 exynos/fimg2d.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h
index 1aac378..bc45ab5 100644
--- a/exynos/fimg2d.h
+++ b/exynos/fimg2d.h
@@ -25,7 +25,7 @@
 #define G2D_MAX_CMD_LIST_NR	64
 #define G2D_PLANE_MAX_NR	2
 
-#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d) * 65536.0)
+#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d * 65536.0))
 
 enum e_g2d_color_mode {
 	/* COLOR FORMAT */
-- 
1.8.5.5

Re: [PATCH 2/3] exynos: fix G2D_DOUBLE_TO_FIXED

[Tomasz Figa]

Hi Tobias,

First of all, thanks for spotting those issues and sending patches.

However looking at libdrm repo history, your patches don't seem to
follow formatting guidelines used there: they lack commit messages
(which should say what is changed and why) and your signed-off-by tags.

Also it is usually a good idea to include respective maintainers on Cc
list, although  unfortunately I'm not sure who would that be in case of
libdrm.

One more comment inline.

On 29.05.2014 23:58, Tobias Jakobi wrote:
> ---
>  exynos/fimg2d.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h
> index 1aac378..bc45ab5 100644
> --- a/exynos/fimg2d.h
> +++ b/exynos/fimg2d.h
> @@ -25,7 +25,7 @@
>  #define G2D_MAX_CMD_LIST_NR	64
>  #define G2D_PLANE_MAX_NR	2
>  
> -#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d) * 65536.0)
> +#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d * 65536.0))

You should also keep the parentheses around d, so that the macro
evaluates correctly even if an expression is passed as the argument.

Best regards,
Tomasz

Re: [PATCH 2/3] exynos: fix G2D_DOUBLE_TO_FIXED

[Tobias Jakobi]

Hello Tomasz!


Tomasz Figa wrote:
> However looking at libdrm repo history, your patches don't seem to
> follow formatting guidelines used there: they lack commit messages
> (which should say what is changed and why) and your signed-off-by tags.
Originally I sent these to Rob Clark (with Inki Dae in CC), but he
wanted me to use git-send-email to send the patches to dri-devel for
review. Which I then did.

I don't see how the patches lack commit messages? I don't think any
additional explanation is needed to what these changes do. They are
one-liners and the issues they address are obvious copy&paste errors
(which the fimg2d tests don't discover though). Or am I supposed to name
them something like "exynos: fix typo in xyz"?

I can resend them with my signed-off if that' fine? I assume adding
'--signoff' to git-send-email should do the trick?

> Also it is usually a good idea to include respective maintainers on Cc
> list, although  unfortunately I'm not sure who would that be in case of
> libdrm.
>
> One more comment inline.
>
> On 29.05.2014 23:58, Tobias Jakobi wrote:
>> ---
>>  exynos/fimg2d.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h
>> index 1aac378..bc45ab5 100644
>> --- a/exynos/fimg2d.h
>> +++ b/exynos/fimg2d.h
>> @@ -25,7 +25,7 @@
>>  #define G2D_MAX_CMD_LIST_NR	64
>>  #define G2D_PLANE_MAX_NR	2
>>  
>> -#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d) * 65536.0)
>> +#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d * 65536.0))
> You should also keep the parentheses around d, so that the macro
> evaluates correctly even if an expression is passed as the argument.
I don't think that affects the code using the macro, but you're right of
course. I'm going to fix this!

> Best regards,
> Tomasz

With best wishes,
Tobias

Re: [PATCH 2/3] exynos: fix G2D_DOUBLE_TO_FIXED

[Tomasz Figa]

On 01.06.2014 03:14, Tobias Jakobi wrote:
> Hello Tomasz!
> 
> 
> Tomasz Figa wrote:
>> However looking at libdrm repo history, your patches don't seem to
>> follow formatting guidelines used there: they lack commit messages
>> (which should say what is changed and why) and your signed-off-by tags.
> Originally I sent these to Rob Clark (with Inki Dae in CC), but he
> wanted me to use git-send-email to send the patches to dri-devel for
> review. Which I then did.

You went from one extreme to another. ;) Although, I can understand
this, because I don't see any clear guidelines for libdrm written
anywhere. Linux kernel SubmittingPatches[1] might be a good read to
learn certain rules that apply to most open source (or at least mailing
list) driven projects.

tl;dr: The recommended practice is to send the patches to appropriate
mailing lists but also add any potentially interested people on CC.

[1] https://www.kernel.org/doc/Documentation/SubmittingPatches

> 
> I don't see how the patches lack commit messages? I don't think any
> additional explanation is needed to what these changes do. They are
> one-liners and the issues they address are obvious copy&paste errors
> (which the fimg2d tests don't discover though). Or am I supposed to name
> them something like "exynos: fix typo in xyz"?

The subjects are fine, but there should be at least one or two sentences
explaining what and why the patch does without looking at the code. For
example:

Currently the G2D_DOUBLE_TO_FIXED() is defined incorrectly, because ...
. This patch fixes the definition by changing ... to ..., so that ... .

It is important from maintenance point of view, because it shows the
people reading the patch that you know what you do and makes them know
too. Also keep in mind repository history, which would be hard to look
through using just git log (without -p) if patches weren't described
properly.

> 
> I can resend them with my signed-off if that' fine? I assume adding
> '--signoff' to git-send-email should do the trick?

Should do, assuming that there is such option - I don't see it in git
help send-email of my git. I recommend reading section 12 (Sign your
work) of [1] to learn everything needed about this tag.

> 
>> Also it is usually a good idea to include respective maintainers on Cc
>> list, although  unfortunately I'm not sure who would that be in case of
>> libdrm.
>>
>> One more comment inline.
>>
>> On 29.05.2014 23:58, Tobias Jakobi wrote:
>>> ---
>>>  exynos/fimg2d.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/exynos/fimg2d.h b/exynos/fimg2d.h
>>> index 1aac378..bc45ab5 100644
>>> --- a/exynos/fimg2d.h
>>> +++ b/exynos/fimg2d.h
>>> @@ -25,7 +25,7 @@
>>>  #define G2D_MAX_CMD_LIST_NR	64
>>>  #define G2D_PLANE_MAX_NR	2
>>>  
>>> -#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d) * 65536.0)
>>> +#define G2D_DOUBLE_TO_FIXED(d)		((unsigned int)(d * 65536.0))
>> You should also keep the parentheses around d, so that the macro
>> evaluates correctly even if an expression is passed as the argument.
> I don't think that affects the code using the macro, but you're right of
> course. I'm going to fix this!

Thanks.

Best regards,
Tomasz

[PATCH 3/3] exynos: fix g2d_copy_with_scale

[Tobias Jakobi]

---
 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 a565910..fc281b6 100644
--- a/exynos/exynos_fimg2d.c
+++ b/exynos/exynos_fimg2d.c
@@ -451,7 +451,7 @@ int g2d_copy_with_scale(struct g2d_context *ctx, struct g2d_image *src,
 	else {
 		scale = 1;
 		scale_x = (double)src_w / (double)dst_w;
-		scale_y = (double)src_w / (double)dst_h;
+		scale_y = (double)src_h / (double)dst_h;
 	}
 
 	if (src_x + src_w > src->width)
-- 
1.8.5.5