* [media-s3c-camif] question about arguments position
@ 2017-05-04 19:05 Gustavo A. R. Silva
2017-05-04 19:34 ` Sylwester Nawrocki
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-04 19:05 UTC (permalink / raw)
To: Sylwester Nawrocki, Mauro Carvalho Chehab
Cc: linux-media, linux-samsung-soc, linux-kernel
Hello everybody,
While looking into Coverity ID 1248800 I ran into the following piece
of code at drivers/media/platform/s3c-camif/camif-capture.c:67:
/* Locking: called with camif->slock spinlock held */
static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
{
const struct s3c_camif_variant *variant = camif->variant;
if (camif->sensor.sd == NULL || vp->out_fmt == NULL)
return -EINVAL;
if (variant->ip_revision == S3C244X_CAMIF_IP_REV)
camif_hw_clear_fifo_overflow(vp);
camif_hw_set_camera_bus(camif);
camif_hw_set_source_format(camif);
camif_hw_set_camera_crop(camif);
camif_hw_set_test_pattern(camif, camif->test_pattern);
if (variant->has_img_effect)
camif_hw_set_effect(camif, camif->colorfx,
camif->colorfx_cb, camif->colorfx_cr);
if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
camif_hw_set_input_path(vp);
camif_cfg_video_path(vp);
vp->state &= ~ST_VP_CONFIG;
return 0;
}
The issue here is that the position of arguments in the call to
camif_hw_set_effect() function do not match the order of the parameters:
camif->colorfx_cb is passed to cr
camif->colorfx_cr is passed to cb
This is the function prototype:
void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
unsigned int cr, unsigned int cb)
My question here is if this is intentional?
In case it is not, I will send a patch to fix it. But first it would
be great to hear any comment about it.
By the way... the same is happening at
drivers/media/platform/s3c-camif/camif-capture.c:366
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [media-s3c-camif] question about arguments position
2017-05-04 19:05 [media-s3c-camif] question about arguments position Gustavo A. R. Silva
@ 2017-05-04 19:34 ` Sylwester Nawrocki
2017-05-04 19:50 ` Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2017-05-04 19:34 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Mauro Carvalho Chehab, linux-media, linux-samsung-soc, linux-kernel
Hi Gustavo,
On 05/04/2017 09:05 PM, Gustavo A. R. Silva wrote:
> The issue here is that the position of arguments in the call to
> camif_hw_set_effect() function do not match the order of the parameters:
>
> camif->colorfx_cb is passed to cr
> camif->colorfx_cr is passed to cb
>
> This is the function prototype:
>
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> unsigned int cr, unsigned int cb)
>
> My question here is if this is intentional?
>
> In case it is not, I will send a patch to fix it. But first it would be
> great to hear any comment about it.
You are right, it seems you have found a real bug. Feel free to send a patch.
The best thing to do now might be to change the function prototype to:
void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
unsigned int cb, unsigned int cr)
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [media-s3c-camif] question about arguments position
2017-05-04 19:34 ` Sylwester Nawrocki
@ 2017-05-04 19:50 ` Gustavo A. R. Silva
2017-05-04 21:42 ` [PATCH] media: platform: s3c-camif: fix function prototype Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-04 19:50 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Mauro Carvalho Chehab, linux-media, linux-samsung-soc, linux-kernel
Hello Sylwester,
Quoting Sylwester Nawrocki <sylvester.nawrocki@gmail.com>:
> Hi Gustavo,
>
> On 05/04/2017 09:05 PM, Gustavo A. R. Silva wrote:
>> The issue here is that the position of arguments in the call to
>> camif_hw_set_effect() function do not match the order of the parameters:
>>
>> camif->colorfx_cb is passed to cr
>> camif->colorfx_cr is passed to cb
>>
>> This is the function prototype:
>>
>> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>> unsigned int cr, unsigned int cb)
>>
>> My question here is if this is intentional?
>>
>> In case it is not, I will send a patch to fix it. But first it would be
>> great to hear any comment about it.
>
> You are right, it seems you have found a real bug. Feel free to send a patch.
> The best thing to do now might be to change the function prototype to:
>
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> unsigned int cb, unsigned int cr)
>
OK, I'll send a patch for this shortly.
Thanks for clarifying.
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] media: platform: s3c-camif: fix function prototype
2017-05-04 19:50 ` Gustavo A. R. Silva
@ 2017-05-04 21:42 ` Gustavo A. R. Silva
2017-05-22 9:02 ` Hans Verkuil
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-04 21:42 UTC (permalink / raw)
To: Sylwester Nawrocki, Mauro Carvalho Chehab
Cc: linux-media, linux-samsung-soc, linux-kernel, Gustavo A. R. Silva
Fix function prototype so the position of arguments camif->colorfx_cb and
camif->colorfx_cr match the order of the parameters when calling
camif_hw_set_effect() function.
Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141
Cc: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/media/platform/s3c-camif/camif-regs.c | 2 +-
drivers/media/platform/s3c-camif/camif-regs.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
index 812fb3a..d70ffef 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.c
+++ b/drivers/media/platform/s3c-camif/camif-regs.c
@@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
}
void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
- unsigned int cr, unsigned int cb)
+ unsigned int cb, unsigned int cr)
{
static const struct v4l2_control colorfx[] = {
{ V4L2_COLORFX_NONE, CIIMGEFF_FIN_BYPASS },
diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
index 5ad36c1..dfb49a5 100644
--- a/drivers/media/platform/s3c-camif/camif-regs.h
+++ b/drivers/media/platform/s3c-camif/camif-regs.h
@@ -255,7 +255,7 @@ void camif_hw_set_output_dma(struct camif_vp *vp);
void camif_hw_set_target_format(struct camif_vp *vp);
void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
- unsigned int cr, unsigned int cb);
+ unsigned int cb, unsigned int cr);
void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
int index);
void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: platform: s3c-camif: fix function prototype
2017-05-04 21:42 ` [PATCH] media: platform: s3c-camif: fix function prototype Gustavo A. R. Silva
@ 2017-05-22 9:02 ` Hans Verkuil
2017-06-01 21:37 ` Sylwester Nawrocki
0 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2017-05-22 9:02 UTC (permalink / raw)
To: Gustavo A. R. Silva, Sylwester Nawrocki, Mauro Carvalho Chehab
Cc: linux-media, linux-samsung-soc, linux-kernel, Sylwester Nawrocki
On 05/04/2017 11:42 PM, Gustavo A. R. Silva wrote:
> Fix function prototype so the position of arguments camif->colorfx_cb and
> camif->colorfx_cr match the order of the parameters when calling
> camif_hw_set_effect() function.
>
> Addresses-Coverity-ID: 1248800
> Addresses-Coverity-ID: 1269141
> Cc: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/media/platform/s3c-camif/camif-regs.c | 2 +-
> drivers/media/platform/s3c-camif/camif-regs.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/s3c-camif/camif-regs.c b/drivers/media/platform/s3c-camif/camif-regs.c
> index 812fb3a..d70ffef 100644
> --- a/drivers/media/platform/s3c-camif/camif-regs.c
> +++ b/drivers/media/platform/s3c-camif/camif-regs.c
> @@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
> }
>
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> - unsigned int cr, unsigned int cb)
> + unsigned int cb, unsigned int cr)
> {
> static const struct v4l2_control colorfx[] = {
> { V4L2_COLORFX_NONE, CIIMGEFF_FIN_BYPASS },
This will also affect this line:
cfg |= cr | (cb << 13);
cr and cb are now swapped so this will result in a different color.
Sylwester, who is wrong here: the prototype or how this function is called?
I suspect that Gustavo is right and that the prototype is wrong. But in that
case this patch should also change the cfg assignment.
Regards,
Hans
> diff --git a/drivers/media/platform/s3c-camif/camif-regs.h b/drivers/media/platform/s3c-camif/camif-regs.h
> index 5ad36c1..dfb49a5 100644
> --- a/drivers/media/platform/s3c-camif/camif-regs.h
> +++ b/drivers/media/platform/s3c-camif/camif-regs.h
> @@ -255,7 +255,7 @@ void camif_hw_set_output_dma(struct camif_vp *vp);
> void camif_hw_set_target_format(struct camif_vp *vp);
> void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern);
> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
> - unsigned int cr, unsigned int cb);
> + unsigned int cb, unsigned int cr);
> void camif_hw_set_output_addr(struct camif_vp *vp, struct camif_addr *paddr,
> int index);
> void camif_hw_dump_regs(struct camif_dev *camif, const char *label);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: platform: s3c-camif: fix function prototype
2017-05-22 9:02 ` Hans Verkuil
@ 2017-06-01 21:37 ` Sylwester Nawrocki
2017-06-02 3:43 ` [PATCH] media: platform: s3c-camif: fix arguments position in function call Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2017-06-01 21:37 UTC (permalink / raw)
To: Hans Verkuil, Gustavo A. R. Silva
Cc: Mauro Carvalho Chehab, linux-media, linux-samsung-soc,
linux-kernel, Sylwester Nawrocki
On 05/22/2017 11:02 AM, Hans Verkuil wrote:
>> --- a/drivers/media/platform/s3c-camif/camif-regs.c
>> +++ b/drivers/media/platform/s3c-camif/camif-regs.c
>> @@ -58,7 +58,7 @@ void camif_hw_set_test_pattern(struct camif_dev *camif, unsigned int pattern)
>> }
>>
>> void camif_hw_set_effect(struct camif_dev *camif, unsigned int effect,
>> - unsigned int cr, unsigned int cb)
>> + unsigned int cb, unsigned int cr)
>> {
>> static const struct v4l2_control colorfx[] = {
>> { V4L2_COLORFX_NONE, CIIMGEFF_FIN_BYPASS },
>
> This will also affect this line:
>
> cfg |= cr | (cb << 13);
>
> cr and cb are now swapped so this will result in a different color.
>
> Sylwester, who is wrong here: the prototype or how this function is called?
>
> I suspect that Gustavo is right and that the prototype is wrong. But in that
> case this patch should also change the cfg assignment.
The function is currently called in a wrong way, it's clear from looking
at the prototype. CR should end up in bits 0:7 and CR in bits 20:13 of
the register. So yes, colour will change after applying the patch - to the
expected one, matching the user API documentation.
Unfortunately I can't test it because I have only the s3c2440 SoC based
evaluation board where the image effect is not supported.
Probably a more straightforward fix would be to amend the callers (apologies
Gustavo for misleading suggestions). But I'm inclined to apply the $subject
patch as is to just close this bug report case.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] media: platform: s3c-camif: fix arguments position in function call
2017-06-01 21:37 ` Sylwester Nawrocki
@ 2017-06-02 3:43 ` Gustavo A. R. Silva
2017-06-05 10:07 ` Sylwester Nawrocki
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2017-06-02 3:43 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Hans Verkuil, Mauro Carvalho Chehab, linux-media,
linux-samsung-soc, linux-kernel, Gustavo A. R. Silva
Hi Sylwester,
Here is another patch in case you decide that it is
better to apply this one.
Thanks
--
Gustavo A. R. Silva
==========
Fix the position of the arguments in function call.
Addresses-Coverity-ID: 1248800
Addresses-Coverity-ID: 1269141
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/media/platform/s3c-camif/camif-capture.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
index 1b30be72..25c7a7d 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -80,7 +80,7 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
camif_hw_set_test_pattern(camif, camif->test_pattern);
if (variant->has_img_effect)
camif_hw_set_effect(camif, camif->colorfx,
- camif->colorfx_cb, camif->colorfx_cr);
+ camif->colorfx_cr, camif->colorfx_cb);
if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
camif_hw_set_input_path(vp);
camif_cfg_video_path(vp);
@@ -364,7 +364,7 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
camif_hw_set_test_pattern(camif, camif->test_pattern);
if (camif->variant->has_img_effect)
camif_hw_set_effect(camif, camif->colorfx,
- camif->colorfx_cb, camif->colorfx_cr);
+ camif->colorfx_cr, camif->colorfx_cb);
vp->state &= ~ST_VP_CONFIG;
}
unlock:
--
2.5.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: platform: s3c-camif: fix arguments position in function call
2017-06-02 3:43 ` [PATCH] media: platform: s3c-camif: fix arguments position in function call Gustavo A. R. Silva
@ 2017-06-05 10:07 ` Sylwester Nawrocki
2017-06-05 15:52 ` Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2017-06-05 10:07 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Hans Verkuil, Mauro Carvalho Chehab, linux-media,
linux-samsung-soc, linux-kernel
On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
> Hi Sylwester,
>
> Here is another patch in case you decide that it is
> better to apply this one.
Thanks, I applied this patch. In future please put any comments only after
the scissors ("---") line, the comments can be then discarded automatically
and there will be no need for manually editing the patch before applying.
--
Regards,
Sylwester
> Fix the position of the arguments in function call.
>
> Addresses-Coverity-ID: 1248800
> Addresses-Coverity-ID: 1269141
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
^^^^^
> drivers/media/platform/s3c-camif/camif-capture.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
> index 1b30be72..25c7a7d 100644
> --- a/drivers/media/platform/s3c-camif/camif-capture.c
> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> @@ -80,7 +80,7 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp)
> camif_hw_set_test_pattern(camif, camif->test_pattern);
> if (variant->has_img_effect)
> camif_hw_set_effect(camif, camif->colorfx,
> - camif->colorfx_cb, camif->colorfx_cr);
> + camif->colorfx_cr, camif->colorfx_cb);
> if (variant->ip_revision == S3C6410_CAMIF_IP_REV)
> camif_hw_set_input_path(vp);
> camif_cfg_video_path(vp);
> @@ -364,7 +364,7 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv)
> camif_hw_set_test_pattern(camif, camif->test_pattern);
> if (camif->variant->has_img_effect)
> camif_hw_set_effect(camif, camif->colorfx,
> - camif->colorfx_cb, camif->colorfx_cr);
> + camif->colorfx_cr, camif->colorfx_cb);
> vp->state &= ~ST_VP_CONFIG;
> }
> unlock:
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: platform: s3c-camif: fix arguments position in function call
2017-06-05 10:07 ` Sylwester Nawrocki
@ 2017-06-05 15:52 ` Gustavo A. R. Silva
0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2017-06-05 15:52 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Hans Verkuil, Mauro Carvalho Chehab, linux-media,
linux-samsung-soc, linux-kernel
Hi Sylwester,
Quoting Sylwester Nawrocki <s.nawrocki@samsung.com>:
> On 06/02/2017 05:43 AM, Gustavo A. R. Silva wrote:
>> Hi Sylwester,
>>
>> Here is another patch in case you decide that it is
>> better to apply this one.
>
> Thanks, I applied this patch. In future please put any comments only after
> the scissors ("---") line, the comments can be then discarded automatically
> and there will be no need for manually editing the patch before applying.
>
OK, I will do that.
> --
> Regards,
> Sylwester
>
>> Fix the position of the arguments in function call.
>>
>> Addresses-Coverity-ID: 1248800
>> Addresses-Coverity-ID: 1269141
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
> ^^^^^
>
I got it.
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-05 15:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 19:05 [media-s3c-camif] question about arguments position Gustavo A. R. Silva
2017-05-04 19:34 ` Sylwester Nawrocki
2017-05-04 19:50 ` Gustavo A. R. Silva
2017-05-04 21:42 ` [PATCH] media: platform: s3c-camif: fix function prototype Gustavo A. R. Silva
2017-05-22 9:02 ` Hans Verkuil
2017-06-01 21:37 ` Sylwester Nawrocki
2017-06-02 3:43 ` [PATCH] media: platform: s3c-camif: fix arguments position in function call Gustavo A. R. Silva
2017-06-05 10:07 ` Sylwester Nawrocki
2017-06-05 15:52 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).