[PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Ricardo Ribalda Delgado]

Volatile controls can change their value outside the v4l-ctrl framework.
We should ignore the cached written value of the ctrl when evaluating if
we should run s_ctrl.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
v4: Hans Verkuil:

explicity set has_changed to false. and add comment

 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 45c5b47..f34a689 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1609,6 +1609,17 @@ static int cluster_changed(struct v4l2_ctrl *master)
 
 		if (ctrl == NULL)
 			continue;
+
+		if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
+			/*
+			 * Set has_changed to false to avoid generating
+			 * the event V4L2_EVENT_CTRL_CH_VALUE
+			 */
+			ctrl->has_changed = false;
+			changed = true;
+			continue;
+		}
+
 		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
 			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
 				ctrl->p_cur, ctrl->p_new);
-- 
2.1.4

Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Hans Verkuil]

Hi Ricardo,

On 02/17/2015 04:08 PM, Ricardo Ribalda Delgado wrote:
> Volatile controls can change their value outside the v4l-ctrl framework.
> We should ignore the cached written value of the ctrl when evaluating if
> we should run s_ctrl.

I've been thinking some more about this (also due to some comments Laurent
made on irc), and I think this should be done differently.

What you want to do here is to signal that setting this control will execute
some action that needs to happen even if the same value is set twice.

That's not really covered by VOLATILE. Interestingly, the WRITE_ONLY flag is
to be used for just that purpose, but this happens to be a R/W control, so
that can't be used either.

What is needed is the following:

1) Add a new flag: V4L2_CTRL_FLAG_ACTION.
2) Any control that sets FLAG_WRITE_ONLY should OR it with FLAG_ACTION (to
   keep the current meaning of WRITE_ONLY).
3) Any control with FLAG_ACTION set should return changed == true in
   cluster_changed.
4) Any control with FLAG_VOLATILE set should set ctrl->has_changed to false
   to prevent generating the CH_VALUE control (that's a real bug).

Your control will now set FLAG_ACTION and FLAG_VOLATILE and it will do the
right thing.

Basically what was missing was a flag to explicitly signal this 'writing
executes an action' behavior. Trying to shoehorn that into the volatile
flag or the write_only flag is just not right. It's a flag in its own right.

Regards,

	Hans

> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v4: Hans Verkuil:
> 
> explicity set has_changed to false. and add comment
> 
>  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 45c5b47..f34a689 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1609,6 +1609,17 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  		if (ctrl == NULL)
>  			continue;
> +
> +		if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
> +			/*
> +			 * Set has_changed to false to avoid generating
> +			 * the event V4L2_EVENT_CTRL_CH_VALUE
> +			 */
> +			ctrl->has_changed = false;
> +			changed = true;
> +			continue;
> +		}
> +
>  		for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
>  			ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
>  				ctrl->p_cur, ctrl->p_new);
> 

Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Laurent Pinchart]

Hi Hans,

On Monday 23 February 2015 10:06:10 Hans Verkuil wrote:
> On 02/17/2015 04:08 PM, Ricardo Ribalda Delgado wrote:
> > Volatile controls can change their value outside the v4l-ctrl framework.
> > We should ignore the cached written value of the ctrl when evaluating if
> > we should run s_ctrl.
> 
> I've been thinking some more about this (also due to some comments Laurent
> made on irc), and I think this should be done differently.
> 
> What you want to do here is to signal that setting this control will execute
> some action that needs to happen even if the same value is set twice.
> 
> That's not really covered by VOLATILE. Interestingly, the WRITE_ONLY flag is
> to be used for just that purpose, but this happens to be a R/W control, so
> that can't be used either.
> 
> What is needed is the following:
> 
> 1) Add a new flag: V4L2_CTRL_FLAG_ACTION.
> 2) Any control that sets FLAG_WRITE_ONLY should OR it with FLAG_ACTION (to
>    keep the current meaning of WRITE_ONLY).
> 3) Any control with FLAG_ACTION set should return changed == true in
>    cluster_changed.
> 4) Any control with FLAG_VOLATILE set should set ctrl->has_changed to false
>    to prevent generating the CH_VALUE control (that's a real bug).
> 
> Your control will now set FLAG_ACTION and FLAG_VOLATILE and it will do the
> right thing.

I'm not sure about Ricardo's use case, is it the one we've discussed on #v4l ? 
If so, and if I recall correctly, the idea was to perform an action with a 
parameter, and didn't require volatility.

> Basically what was missing was a flag to explicitly signal this 'writing
> executes an action' behavior. Trying to shoehorn that into the volatile
> flag or the write_only flag is just not right. It's a flag in its own right.

Just for the sake of exploring all options, what did you think about the idea 
of making button controls accept a value ?

Your proposal is interesting as well, but I'm not sure about the 
V4L2_CTRL_FLAG_ACTION name. Aren't all controls supposed to have an action of 
some sort ? That's nitpicking of course.

Also, should the action flag be automatically set for button controls ? Button 
controls would in a way become type-less controls with the action flag set, 
that's interesting. I suppose type-less controls without the action flag don't 
make sense.

> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> > v4: Hans Verkuil:
> > 
> > explicity set has_changed to false. and add comment
> > 
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Ricardo Ribalda Delgado]

Hello Hans and Laurent


I understand volatile as a control that can change its value by the device. So in that sense I think that my control is volatile and writeable (ack by the user).

The value written by the user is meaning-less in my usercase, but in another s it could be useful.

I am outside the office and with no computer for the next two weeks. If you can wait until then I can implement Hans idea or another one and try it out with my hw. 

Thanks for consideing my usercase :)


Regards

(Sorry for duplicate, still trying to convince my phone to use plain text)

On 24 February 2015 06:07:49 GMT+07:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>Hi Hans,
>
>On Monday 23 February 2015 10:06:10 Hans Verkuil wrote:
>> On 02/17/2015 04:08 PM, Ricardo Ribalda Delgado wrote:
>> > Volatile controls can change their value outside the v4l-ctrl
>framework.
>> > We should ignore the cached written value of the ctrl when
>evaluating if
>> > we should run s_ctrl.
>> 
>> I've been thinking some more about this (also due to some comments
>Laurent
>> made on irc), and I think this should be done differently.
>> 
>> What you want to do here is to signal that setting this control will
>execute
>> some action that needs to happen even if the same value is set twice.
>> 
>> That's not really covered by VOLATILE. Interestingly, the WRITE_ONLY
>flag is
>> to be used for just that purpose, but this happens to be a R/W
>control, so
>> that can't be used either.
>> 
>> What is needed is the following:
>> 
>> 1) Add a new flag: V4L2_CTRL_FLAG_ACTION.
>> 2) Any control that sets FLAG_WRITE_ONLY should OR it with
>FLAG_ACTION (to
>>    keep the current meaning of WRITE_ONLY).
>> 3) Any control with FLAG_ACTION set should return changed == true in
>>    cluster_changed.
>> 4) Any control with FLAG_VOLATILE set should set ctrl->has_changed to
>false
>>    to prevent generating the CH_VALUE control (that's a real bug).
>> 
>> Your control will now set FLAG_ACTION and FLAG_VOLATILE and it will
>do the
>> right thing.
>
>I'm not sure about Ricardo's use case, is it the one we've discussed on
>#v4l ? 
>If so, and if I recall correctly, the idea was to perform an action
>with a 
>parameter, and didn't require volatility.
>
>> Basically what was missing was a flag to explicitly signal this
>'writing
>> executes an action' behavior. Trying to shoehorn that into the
>volatile
>> flag or the write_only flag is just not right. It's a flag in its own
>right.
>
>Just for the sake of exploring all options, what did you think about
>the idea 
>of making button controls accept a value ?
>
>Your proposal is interesting as well, but I'm not sure about the 
>V4L2_CTRL_FLAG_ACTION name. Aren't all controls supposed to have an
>action of 
>some sort ? That's nitpicking of course.
>
>Also, should the action flag be automatically set for button controls ?
>Button 
>controls would in a way become type-less controls with the action flag
>set, 
>that's interesting. I suppose type-less controls without the action
>flag don't 
>make sense.
>
>> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> > ---
>> > v4: Hans Verkuil:
>> > 
>> > explicity set has_changed to false. and add comment
>> > 
>> >  drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)

-- 
Ricardo Ribalda
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Ricardo Ribalda Delgado]

Hello

Back from holidays and back to this issue. Sorry for the delay.

> I'm not sure about Ricardo's use case, is it the one we've discussed on #v4l ?
> If so, and if I recall correctly, the idea was to perform an action with a
> parameter, and didn't require volatility.

In my case, there is a trigger overflow bit. The user acks the trigger
overflow by writting any value to the control.

There is no parameter. In that sense it looks like a volatile read + a
button on a controll.

> Your proposal is interesting as well, but I'm not sure about the
> V4L2_CTRL_FLAG_ACTION name. Aren't all controls supposed to have an action of
> some sort ? That's nitpicking of course.

What about the name STATELESS_WRITE ? or perhaps VOLATILE_WRITE? I
dont care about the name :), but better have it solved before I write
the doc :P

>
> Also, should the action flag be automatically set for button controls ? Button
> controls would in a way become type-less controls with the action flag set,
> that's interesting. I suppose type-less controls without the action flag don't
> make sense.

I agree!


Best regards and thanks!

-- 
Ricardo Ribalda

Re: [PATCH v4 1/2] media/v4l2-ctrls: Always run s_ctrl on volatile ctrls

[Hans Verkuil]

On 03/09/2015 05:45 PM, Ricardo Ribalda Delgado wrote:
> Hello
> 
> Back from holidays and back to this issue. Sorry for the delay.
> 
>> I'm not sure about Ricardo's use case, is it the one we've discussed on #v4l ?
>> If so, and if I recall correctly, the idea was to perform an action with a
>> parameter, and didn't require volatility.
> 
> In my case, there is a trigger overflow bit. The user acks the trigger
> overflow by writting any value to the control.
> 
> There is no parameter. In that sense it looks like a volatile read + a
> button on a controll.
> 
>> Your proposal is interesting as well, but I'm not sure about the
>> V4L2_CTRL_FLAG_ACTION name. Aren't all controls supposed to have an action of
>> some sort ? That's nitpicking of course.

Actually, usually controls just set some parameter and if the new value is
the same as the old value, then nothing is done at all.

A button control always executes some action immediately.

> 
> What about the name STATELESS_WRITE ? or perhaps VOLATILE_WRITE? I
> dont care about the name :), but better have it solved before I write
> the doc :P

How about: ACT_ON_WRITE or EXECUTE_ON_WRITE?

> 
>>
>> Also, should the action flag be automatically set for button controls ? Button
>> controls would in a way become type-less controls with the action flag set,
>> that's interesting. I suppose type-less controls without the action flag don't
>> make sense.

Yes, this flag will be set for button controls. Those are already WRITE_ONLY and
all WRITE_ONLY controls will set the new flag as well.

Regards,

	Hans