Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1267120 > unrolled thread
| Started by | Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> |
|---|---|
| First post | 2015-11-11 13:00 +0100 |
| Last post | 2015-11-11 13:50 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] v4l2-core/v4l2-ctrls: Filter NOOP CH_RANGE events Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2015-11-11 13:00 +0100
Re: [PATCH] v4l2-core/v4l2-ctrls: Filter NOOP CH_RANGE events Hans Verkuil <hverkuil@xs4all.nl> - 2015-11-11 13:20 +0100
Re: [PATCH] v4l2-core/v4l2-ctrls: Filter NOOP CH_RANGE events Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2015-11-11 13:30 +0100
Re: [PATCH] v4l2-core/v4l2-ctrls: Filter NOOP CH_RANGE events Hans Verkuil <hansverk@cisco.com> - 2015-11-11 13:50 +0100
| From | Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> |
|---|---|
| Date | 2015-11-11 13:00 +0100 |
| Subject | [PATCH] v4l2-core/v4l2-ctrls: Filter NOOP CH_RANGE events |
| Message-ID | <qtCef-2hA-19@gated-at.bofh.it> |
If modify_range is called but no range is changed, do not send the
CH_RANGE event.
Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 4a1d9fdd14bb..f9c0e8150bd1 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3300,7 +3300,8 @@ EXPORT_SYMBOL(v4l2_ctrl_notify);
int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
s64 min, s64 max, u64 step, s64 def)
{
- bool changed;
+ bool value_changed;
+ bool range_changed = false;
int ret;
lockdep_assert_held(ctrl->handler->lock);
@@ -3324,10 +3325,14 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
default:
return -EINVAL;
}
- ctrl->minimum = min;
- ctrl->maximum = max;
- ctrl->step = step;
- ctrl->default_value = def;
+ if ((ctrl->minimum != min) || (ctrl->maximum != max) ||
+ (ctrl->step != step) || ctrl->default_value != def) {
+ range_changed = true;
+ ctrl->minimum = min;
+ ctrl->maximum = max;
+ ctrl->step = step;
+ ctrl->default_value = def;
+ }
cur_to_new(ctrl);
if (validate_new(ctrl, ctrl->p_new)) {
if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
@@ -3337,12 +3342,12 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
}
if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
- changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
+ value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
else
- changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
- if (changed)
+ value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
+ if (value_changed)
ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
- else
+ else if (range_changed)
send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
return ret;
}
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Hans Verkuil <hverkuil@xs4all.nl> |
|---|---|
| Date | 2015-11-11 13:20 +0100 |
| Message-ID | <qtCxB-2EB-19@gated-at.bofh.it> |
| In reply to | #1267120 |
On 11/11/15 12:58, Ricardo Ribalda Delgado wrote:
> If modify_range is called but no range is changed, do not send the
> CH_RANGE event.
While not opposed to this patch, I do wonder what triggered this patch?
Is it just a matter of efficiency? And since it is a driver that calls
this, shouldn't the driver only call this function when something
actually changes?
In other words, can you give some background information?
Regards,
Hans
PS: still haven't processed your V4L2_CTRL_WHICH_DEF_VAL patch series. Hope
to do this next week at the latest.
>
> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 4a1d9fdd14bb..f9c0e8150bd1 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3300,7 +3300,8 @@ EXPORT_SYMBOL(v4l2_ctrl_notify);
> int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
> s64 min, s64 max, u64 step, s64 def)
> {
> - bool changed;
> + bool value_changed;
> + bool range_changed = false;
> int ret;
>
> lockdep_assert_held(ctrl->handler->lock);
> @@ -3324,10 +3325,14 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
> default:
> return -EINVAL;
> }
> - ctrl->minimum = min;
> - ctrl->maximum = max;
> - ctrl->step = step;
> - ctrl->default_value = def;
> + if ((ctrl->minimum != min) || (ctrl->maximum != max) ||
> + (ctrl->step != step) || ctrl->default_value != def) {
> + range_changed = true;
> + ctrl->minimum = min;
> + ctrl->maximum = max;
> + ctrl->step = step;
> + ctrl->default_value = def;
> + }
> cur_to_new(ctrl);
> if (validate_new(ctrl, ctrl->p_new)) {
> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> @@ -3337,12 +3342,12 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
> }
>
> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> - changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
> + value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
> else
> - changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
> - if (changed)
> + value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
> + if (value_changed)
> ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
> - else
> + else if (range_changed)
> send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
> return ret;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> |
|---|---|
| Date | 2015-11-11 13:30 +0100 |
| Message-ID | <qtCHg-2HY-9@gated-at.bofh.it> |
| In reply to | #1267149 |
Hello Hans
On my cameras, the framerate can be controlled in very fine steps (so
you can be in sync with the conveyor belt). The exposure time is
dependent on the framerate. I was calling modify_range every time
there was a change on the framerate on all the different heads.
Leading to invalid/noop events.
I have already fixed it on my sensor code, but there is a lot of copy
paste.... I thought that check belong to the framework. It also will
give some consistency, because If my memory is good, set_ctrl does not
send and event if the value is unchanged.
Thanks :)
And take as much time as you need for V4L2_CTRL_WHICH_DEF_VAL :-)
On Wed, Nov 11, 2015 at 1:11 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/11/15 12:58, Ricardo Ribalda Delgado wrote:
>> If modify_range is called but no range is changed, do not send the
>> CH_RANGE event.
>
> While not opposed to this patch, I do wonder what triggered this patch?
> Is it just a matter of efficiency? And since it is a driver that calls
> this, shouldn't the driver only call this function when something
> actually changes?
>
> In other words, can you give some background information?
>
> Regards,
>
> Hans
>
> PS: still haven't processed your V4L2_CTRL_WHICH_DEF_VAL patch series. Hope
> to do this next week at the latest.
>
>>
>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>> drivers/media/v4l2-core/v4l2-ctrls.c | 23 ++++++++++++++---------
>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 4a1d9fdd14bb..f9c0e8150bd1 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -3300,7 +3300,8 @@ EXPORT_SYMBOL(v4l2_ctrl_notify);
>> int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>> s64 min, s64 max, u64 step, s64 def)
>> {
>> - bool changed;
>> + bool value_changed;
>> + bool range_changed = false;
>> int ret;
>>
>> lockdep_assert_held(ctrl->handler->lock);
>> @@ -3324,10 +3325,14 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>> default:
>> return -EINVAL;
>> }
>> - ctrl->minimum = min;
>> - ctrl->maximum = max;
>> - ctrl->step = step;
>> - ctrl->default_value = def;
>> + if ((ctrl->minimum != min) || (ctrl->maximum != max) ||
>> + (ctrl->step != step) || ctrl->default_value != def) {
>> + range_changed = true;
>> + ctrl->minimum = min;
>> + ctrl->maximum = max;
>> + ctrl->step = step;
>> + ctrl->default_value = def;
>> + }
>> cur_to_new(ctrl);
>> if (validate_new(ctrl, ctrl->p_new)) {
>> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
>> @@ -3337,12 +3342,12 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>> }
>>
>> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
>> - changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
>> + value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
>> else
>> - changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
>> - if (changed)
>> + value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
>> + if (value_changed)
>> ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
>> - else
>> + else if (range_changed)
>> send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
>> return ret;
>> }
>>
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Hans Verkuil <hansverk@cisco.com> |
|---|---|
| Date | 2015-11-11 13:50 +0100 |
| Message-ID | <qtD0B-2OK-1@gated-at.bofh.it> |
| In reply to | #1267152 |
On 11/11/15 13:20, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> On my cameras, the framerate can be controlled in very fine steps (so
> you can be in sync with the conveyor belt). The exposure time is
> dependent on the framerate. I was calling modify_range every time
> there was a change on the framerate on all the different heads.
> Leading to invalid/noop events.
>
> I have already fixed it on my sensor code, but there is a lot of copy
> paste.... I thought that check belong to the framework. It also will
> give some consistency, because If my memory is good, set_ctrl does not
> send and event if the value is unchanged.
Ah, OK. That makes sense. Thanks for the explanation.
Hans
>
>
> Thanks :)
>
> And take as much time as you need for V4L2_CTRL_WHICH_DEF_VAL :-)
>
>
>
> On Wed, Nov 11, 2015 at 1:11 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 11/11/15 12:58, Ricardo Ribalda Delgado wrote:
>>> If modify_range is called but no range is changed, do not send the
>>> CH_RANGE event.
>>
>> While not opposed to this patch, I do wonder what triggered this patch?
>> Is it just a matter of efficiency? And since it is a driver that calls
>> this, shouldn't the driver only call this function when something
>> actually changes?
>>
>> In other words, can you give some background information?
>>
>> Regards,
>>
>> Hans
>>
>> PS: still haven't processed your V4L2_CTRL_WHICH_DEF_VAL patch series. Hope
>> to do this next week at the latest.
>>
>>>
>>> Reported-by: Dimitrios Katsaros <patcherwork@gmail.com>
>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>> ---
>>> drivers/media/v4l2-core/v4l2-ctrls.c | 23 ++++++++++++++---------
>>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index 4a1d9fdd14bb..f9c0e8150bd1 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -3300,7 +3300,8 @@ EXPORT_SYMBOL(v4l2_ctrl_notify);
>>> int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>>> s64 min, s64 max, u64 step, s64 def)
>>> {
>>> - bool changed;
>>> + bool value_changed;
>>> + bool range_changed = false;
>>> int ret;
>>>
>>> lockdep_assert_held(ctrl->handler->lock);
>>> @@ -3324,10 +3325,14 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>>> default:
>>> return -EINVAL;
>>> }
>>> - ctrl->minimum = min;
>>> - ctrl->maximum = max;
>>> - ctrl->step = step;
>>> - ctrl->default_value = def;
>>> + if ((ctrl->minimum != min) || (ctrl->maximum != max) ||
>>> + (ctrl->step != step) || ctrl->default_value != def) {
>>> + range_changed = true;
>>> + ctrl->minimum = min;
>>> + ctrl->maximum = max;
>>> + ctrl->step = step;
>>> + ctrl->default_value = def;
>>> + }
>>> cur_to_new(ctrl);
>>> if (validate_new(ctrl, ctrl->p_new)) {
>>> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
>>> @@ -3337,12 +3342,12 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
>>> }
>>>
>>> if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
>>> - changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
>>> + value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64;
>>> else
>>> - changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
>>> - if (changed)
>>> + value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32;
>>> + if (value_changed)
>>> ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
>>> - else
>>> + else if (range_changed)
>>> send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
>>> return ret;
>>> }
>>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web