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

[PATCH] 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>
---

I have a control that tells the user when there has been a external trigger
overrun. (Trigger while processing old image). This is a volatile control.

The user writes 0 to the control, to ack the error condition, and clear the
hardware flag.

Unfortunately, it only works one time, because the next time the user writes
a zero to the control cluster_changed returns false.

I think on volatile controls it is safer to run s_ctrl twice than missing a
valid s_ctrl.

I know I am abusing a bit the API for this :P, but I also believe that the
semantic here is a bit confusing.

 drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 45c5b47..3d0c7f4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1605,7 +1605,7 @@ static int cluster_changed(struct v4l2_ctrl *master)
 
 	for (i = 0; i < master->ncontrols; i++) {
 		struct v4l2_ctrl *ctrl = master->cluster[i];
-		bool ctrl_changed = false;
+		bool ctrl_changed = ctrl->flags & V4L2_CTRL_FLAG_VOLATILE;
 
 		if (ctrl == NULL)
 			continue;
-- 
2.1.4

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

[Hans Verkuil]

Hi Ricardo,

On 02/17/15 12:02, 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.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> 
> I have a control that tells the user when there has been a external trigger
> overrun. (Trigger while processing old image). This is a volatile control.

Does the application just read the control to check whether the trigger happened?
Or is the control perhaps changed by an interrupt handler?

> The user writes 0 to the control, to ack the error condition, and clear the
> hardware flag.

Would it be an idea to automatically ack the error condition when reading the
control?

Or, alternatively, have a separate button control to clear the condition.

> 
> Unfortunately, it only works one time, because the next time the user writes
> a zero to the control cluster_changed returns false.
> 
> I think on volatile controls it is safer to run s_ctrl twice than missing a
> valid s_ctrl.
> 
> I know I am abusing a bit the API for this :P, but I also believe that the
> semantic here is a bit confusing.

The reason for that is that I have yet to see a convincing argument for
allowing s_ctrl for a volatile control.

Regards,

	Hans

> 
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 45c5b47..3d0c7f4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1605,7 +1605,7 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  	for (i = 0; i < master->ncontrols; i++) {
>  		struct v4l2_ctrl *ctrl = master->cluster[i];
> -		bool ctrl_changed = false;
> +		bool ctrl_changed = ctrl->flags & V4L2_CTRL_FLAG_VOLATILE;
>  
>  		if (ctrl == NULL)
>  			continue;
> 

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

[Ricardo Ribalda Delgado]

Hello Hans

I need to figure out how can you  reply that fast. Thanks a lot!

On Tue, Feb 17, 2015 at 12:17 PM, Hans Verkuil <hansverk@cisco.com> wrote:
>> I have a control that tells the user when there has been a external trigger
>> overrun. (Trigger while processing old image). This is a volatile control.
>
> Does the application just read the control to check whether the trigger happened?
> Or is the control perhaps changed by an interrupt handler?

The control exposes a bit on the trigger system. The application polls
it at its own rate.
I could convince the hardware engineer to make an inq on that event,
but right now the
hw does not support it.

>
>> The user writes 0 to the control, to ack the error condition, and clear the
>> hardware flag.
>
> Would it be an idea to automatically ack the error condition when reading the
> control?

There might be two applications running at the same time.

ie: APP1 calibrates the camera, while APP2 gets images.
APP1 will ack the error and APP2 will never notice, when is APP2 the
one that cares abot the error.


>
> Or, alternatively, have a separate button control to clear the condition.
>

Of course this is an option, but I think this is not very clean.

>> I know I am abusing a bit the API for this :P, but I also believe that the
>> semantic here is a bit confusing.
>
> The reason for that is that I have yet to see a convincing argument for
> allowing s_ctrl for a volatile control.

This kind of error flags could be a nice candidate for this control.

Right now we can create a volatile control with s_ctrl, the api allows
it, so I think it is either
not allowing that or adding this patch.

 Both are perfectly fine :), but allowing s_ctrl and volatile and then
now running s_ctrl always
seems a bit weird to me.

Thanks!



-- 
Ricardo Ribalda

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

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
...
>> Unfortunately, it only works one time, because the next time the user writes
>> a zero to the control cluster_changed returns false.
>>
>> I think on volatile controls it is safer to run s_ctrl twice than missing a
>> valid s_ctrl.
>>
>> I know I am abusing a bit the API for this :P, but I also believe that the
>> semantic here is a bit confusing.
> 
> The reason for that is that I have yet to see a convincing argument for
> allowing s_ctrl for a volatile control.

Well, one example are LED flash class devices which implement V4L2 flash
API through a wrapper. The user may use the LED flash class API to
change the values of the controls, and V4L2 framework has no clue about
this. The V4L2 controls are volatile, and the real values of the
settings are stored in the LED flash class.

This is the current implementation (not merged yet); an alternative, a
more correct one, would be to use callbacks to tell about the changes in
control values. I haven't pushed for that, primarily because the
patchset is already quite complex and I've seen this as something that
can be always implemented later if it bothers someone.

Cc Jacek.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Jacek Anaszewski]

Hi Hans, Sakari,

On 02/17/2015 12:32 PM, Sakari Ailus wrote:
> Hi Hans,
>
> Hans Verkuil wrote:
> ...
>>> Unfortunately, it only works one time, because the next time the user writes
>>> a zero to the control cluster_changed returns false.
>>>
>>> I think on volatile controls it is safer to run s_ctrl twice than missing a
>>> valid s_ctrl.
>>>
>>> I know I am abusing a bit the API for this :P, but I also believe that the
>>> semantic here is a bit confusing.
>>
>> The reason for that is that I have yet to see a convincing argument for
>> allowing s_ctrl for a volatile control.
>
> Well, one example are LED flash class devices which implement V4L2 flash
> API through a wrapper. The user may use the LED flash class API to
> change the values of the controls, and V4L2 framework has no clue about
> this. The V4L2 controls are volatile, and the real values of the
> settings are stored in the LED flash class.
>
> This is the current implementation (not merged yet); an alternative, a
> more correct one, would be to use callbacks to tell about the changes in
> control values. I haven't pushed for that, primarily because the
> patchset is already quite complex and I've seen this as something that
> can be always implemented later if it bothers someone.
>
> Cc Jacek.
>

Actually this will be not an issue for v4l2-flash sub-device anymore.
In the next version of the patch set the v4l2-flash sub-device
will be synchronizing the flash device registers with the
state of the controls on open.

-- 
Best Regards,
Jacek Anaszewski

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

[Sakari Ailus]

Hi Jacek,

Jacek Anaszewski wrote:
> Hi Hans, Sakari,
> 
> On 02/17/2015 12:32 PM, Sakari Ailus wrote:
>> Hi Hans,
>>
>> Hans Verkuil wrote:
>> ...
>>>> Unfortunately, it only works one time, because the next time the
>>>> user writes
>>>> a zero to the control cluster_changed returns false.
>>>>
>>>> I think on volatile controls it is safer to run s_ctrl twice than
>>>> missing a
>>>> valid s_ctrl.
>>>>
>>>> I know I am abusing a bit the API for this :P, but I also believe
>>>> that the
>>>> semantic here is a bit confusing.
>>>
>>> The reason for that is that I have yet to see a convincing argument for
>>> allowing s_ctrl for a volatile control.
>>
>> Well, one example are LED flash class devices which implement V4L2 flash
>> API through a wrapper. The user may use the LED flash class API to
>> change the values of the controls, and V4L2 framework has no clue about
>> this. The V4L2 controls are volatile, and the real values of the
>> settings are stored in the LED flash class.
>>
>> This is the current implementation (not merged yet); an alternative, a
>> more correct one, would be to use callbacks to tell about the changes in
>> control values. I haven't pushed for that, primarily because the
>> patchset is already quite complex and I've seen this as something that
>> can be always implemented later if it bothers someone.
>>
>> Cc Jacek.
>>
> 
> Actually this will be not an issue for v4l2-flash sub-device anymore.
> In the next version of the patch set the v4l2-flash sub-device
> will be synchronizing the flash device registers with the
> state of the controls on open.

Ah, right --- you're preventing the use of the LED flash class whilst
the V4L2 sub-device is opened? I'm not fully certain whether that'd be
really useful, as the V4L2 sub-device can also be opened by multiple
users at the same time. However the applications that would access the
LED class API are mostly different ones and for different purposes; I
don't really have a strong opinion either way here.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Jacek Anaszewski]

On 02/17/2015 03:06 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> Jacek Anaszewski wrote:
>> Hi Hans, Sakari,
>>
>> On 02/17/2015 12:32 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Hans Verkuil wrote:
>>> ...
>>>>> Unfortunately, it only works one time, because the next time the
>>>>> user writes
>>>>> a zero to the control cluster_changed returns false.
>>>>>
>>>>> I think on volatile controls it is safer to run s_ctrl twice than
>>>>> missing a
>>>>> valid s_ctrl.
>>>>>
>>>>> I know I am abusing a bit the API for this :P, but I also believe
>>>>> that the
>>>>> semantic here is a bit confusing.
>>>>
>>>> The reason for that is that I have yet to see a convincing argument for
>>>> allowing s_ctrl for a volatile control.
>>>
>>> Well, one example are LED flash class devices which implement V4L2 flash
>>> API through a wrapper. The user may use the LED flash class API to
>>> change the values of the controls, and V4L2 framework has no clue about
>>> this. The V4L2 controls are volatile, and the real values of the
>>> settings are stored in the LED flash class.
>>>
>>> This is the current implementation (not merged yet); an alternative, a
>>> more correct one, would be to use callbacks to tell about the changes in
>>> control values. I haven't pushed for that, primarily because the
>>> patchset is already quite complex and I've seen this as something that
>>> can be always implemented later if it bothers someone.
>>>
>>> Cc Jacek.
>>>
>>
>> Actually this will be not an issue for v4l2-flash sub-device anymore.
>> In the next version of the patch set the v4l2-flash sub-device
>> will be synchronizing the flash device registers with the
>> state of the controls on open.
>
> Ah, right --- you're preventing the use of the LED flash class whilst
> the V4L2 sub-device is opened?

Yes.

> I'm not fully certain whether that'd be
> really useful, as the V4L2 sub-device can also be opened by multiple
> users at the same time.

We also prevent from this using v4l2_fh_is_singular on open.

> However the applications that would access the
> LED class API are mostly different ones and for different purposes; I
> don't really have a strong opinion either way here.


-- 
Best Regards,
Jacek Anaszewski

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

[Sakari Ailus]

Jacek Anaszewski wrote:
> On 02/17/2015 03:06 PM, Sakari Ailus wrote:
>> Hi Jacek,
>>
>> Jacek Anaszewski wrote:
>>> Hi Hans, Sakari,
>>>
>>> On 02/17/2015 12:32 PM, Sakari Ailus wrote:
>>>> Hi Hans,
>>>>
>>>> Hans Verkuil wrote:
>>>> ...
>>>>>> Unfortunately, it only works one time, because the next time the
>>>>>> user writes
>>>>>> a zero to the control cluster_changed returns false.
>>>>>>
>>>>>> I think on volatile controls it is safer to run s_ctrl twice than
>>>>>> missing a
>>>>>> valid s_ctrl.
>>>>>>
>>>>>> I know I am abusing a bit the API for this :P, but I also believe
>>>>>> that the
>>>>>> semantic here is a bit confusing.
>>>>>
>>>>> The reason for that is that I have yet to see a convincing argument
>>>>> for
>>>>> allowing s_ctrl for a volatile control.
>>>>
>>>> Well, one example are LED flash class devices which implement V4L2
>>>> flash
>>>> API through a wrapper. The user may use the LED flash class API to
>>>> change the values of the controls, and V4L2 framework has no clue about
>>>> this. The V4L2 controls are volatile, and the real values of the
>>>> settings are stored in the LED flash class.
>>>>
>>>> This is the current implementation (not merged yet); an alternative, a
>>>> more correct one, would be to use callbacks to tell about the
>>>> changes in
>>>> control values. I haven't pushed for that, primarily because the
>>>> patchset is already quite complex and I've seen this as something that
>>>> can be always implemented later if it bothers someone.
>>>>
>>>> Cc Jacek.
>>>>
>>>
>>> Actually this will be not an issue for v4l2-flash sub-device anymore.
>>> In the next version of the patch set the v4l2-flash sub-device
>>> will be synchronizing the flash device registers with the
>>> state of the controls on open.
>>
>> Ah, right --- you're preventing the use of the LED flash class whilst
>> the V4L2 sub-device is opened?
> 
> Yes.
> 
>> I'm not fully certain whether that'd be
>> really useful, as the V4L2 sub-device can also be opened by multiple
>> users at the same time.
> 
> We also prevent from this using v4l2_fh_is_singular on open.

I'm not fully certain if I'd do that --- no other flash chip driver
does. It might be good to think about how does one acquire the ownership
of media devices or parts of media devices, or whether it's something
that's needed at all.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Jacek Anaszewski]

On 02/17/2015 03:35 PM, Sakari Ailus wrote:
> Jacek Anaszewski wrote:
>> On 02/17/2015 03:06 PM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> Jacek Anaszewski wrote:
>>>> Hi Hans, Sakari,
>>>>
>>>> On 02/17/2015 12:32 PM, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Hans Verkuil wrote:
>>>>> ...
>>>>>>> Unfortunately, it only works one time, because the next time the
>>>>>>> user writes
>>>>>>> a zero to the control cluster_changed returns false.
>>>>>>>
>>>>>>> I think on volatile controls it is safer to run s_ctrl twice than
>>>>>>> missing a
>>>>>>> valid s_ctrl.
>>>>>>>
>>>>>>> I know I am abusing a bit the API for this :P, but I also believe
>>>>>>> that the
>>>>>>> semantic here is a bit confusing.
>>>>>>
>>>>>> The reason for that is that I have yet to see a convincing argument
>>>>>> for
>>>>>> allowing s_ctrl for a volatile control.
>>>>>
>>>>> Well, one example are LED flash class devices which implement V4L2
>>>>> flash
>>>>> API through a wrapper. The user may use the LED flash class API to
>>>>> change the values of the controls, and V4L2 framework has no clue about
>>>>> this. The V4L2 controls are volatile, and the real values of the
>>>>> settings are stored in the LED flash class.
>>>>>
>>>>> This is the current implementation (not merged yet); an alternative, a
>>>>> more correct one, would be to use callbacks to tell about the
>>>>> changes in
>>>>> control values. I haven't pushed for that, primarily because the
>>>>> patchset is already quite complex and I've seen this as something that
>>>>> can be always implemented later if it bothers someone.
>>>>>
>>>>> Cc Jacek.
>>>>>
>>>>
>>>> Actually this will be not an issue for v4l2-flash sub-device anymore.
>>>> In the next version of the patch set the v4l2-flash sub-device
>>>> will be synchronizing the flash device registers with the
>>>> state of the controls on open.
>>>
>>> Ah, right --- you're preventing the use of the LED flash class whilst
>>> the V4L2 sub-device is opened?
>>
>> Yes.
>>
>>> I'm not fully certain whether that'd be
>>> really useful, as the V4L2 sub-device can also be opened by multiple
>>> users at the same time.
>>
>> We also prevent from this using v4l2_fh_is_singular on open.
>
> I'm not fully certain if I'd do that --- no other flash chip driver
> does. It might be good to think about how does one acquire the ownership
> of media devices or parts of media devices, or whether it's something
> that's needed at all.
>

Well, it was your remark from the review to add this :)

Regarding locking the LED subsystem sysfs interface - it is required for
preventing reconfiguration of the device from the sysfs level. Without
this the V4L2 flash device couldn't be certain about the flash LED
device state.

The patch adding the locking mechanism to the LED subsystem has been
merged few months ago.

-- 
Best Regards,
Jacek Anaszewski

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

[Hans Verkuil]

Hi Ricardo,

I've thought about this some more and I agree that this should be allowed.

But I have some comments, see below.

On 02/17/15 12:02, 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.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> 
> I have a control that tells the user when there has been a external trigger
> overrun. (Trigger while processing old image). This is a volatile control.
> 
> The user writes 0 to the control, to ack the error condition, and clear the
> hardware flag.
> 
> Unfortunately, it only works one time, because the next time the user writes
> a zero to the control cluster_changed returns false.
> 
> I think on volatile controls it is safer to run s_ctrl twice than missing a
> valid s_ctrl.
> 
> I know I am abusing a bit the API for this :P, but I also believe that the
> semantic here is a bit confusing.
> 
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 45c5b47..3d0c7f4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1605,7 +1605,7 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  	for (i = 0; i < master->ncontrols; i++) {
>  		struct v4l2_ctrl *ctrl = master->cluster[i];
> -		bool ctrl_changed = false;
> +		bool ctrl_changed = ctrl->flags & V4L2_CTRL_FLAG_VOLATILE;

Should be done after the 'ctrl == NULL' check.

>  
>  		if (ctrl == NULL)
>  			continue;
> 

There is one more change that has to be made: setting a volatile control
should never generate a V4L2_EVENT_CTRL_CH_VALUE event since that makes
no sense. The way to prevent that is to ensure that ctrl->has_changed is
always false for volatile controls. The new_to_cur function looks at that
field to decide whether to send an event.

The documentation should also be updated: that of V4L2_CTRL_FLAG_VOLATILE
(in VIDIOC_QUERYCTRL), and of V4L2_EVENT_CTRL_CH_VALUE.

Regards,

	Hans

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

[Ricardo Ribalda Delgado]

Hello Hans

On Tue, Feb 17, 2015 at 1:03 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Should be done after the 'ctrl == NULL' check.

Good catch. Fixed on v2

>
>>
>>               if (ctrl == NULL)
>>                       continue;
>>
>
> There is one more change that has to be made: setting a volatile control
> should never generate a V4L2_EVENT_CTRL_CH_VALUE event since that makes
> no sense. The way to prevent that is to ensure that ctrl->has_changed is
> always false for volatile controls. The new_to_cur function looks at that
> field to decide whether to send an event.
>
> The documentation should also be updated: that of V4L2_CTRL_FLAG_VOLATILE
> (in VIDIOC_QUERYCTRL), and of V4L2_EVENT_CTRL_CH_VALUE.

I can do this also if you want. It has been a while without
contributing to media :)

Regards!

>
> Regards,
>
>         Hans



-- 
Ricardo Ribalda

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

[Hans Verkuil]

On 02/17/15 13:21, Ricardo Ribalda Delgado wrote:
> Hello Hans
> 
> On Tue, Feb 17, 2015 at 1:03 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Should be done after the 'ctrl == NULL' check.
> 
> Good catch. Fixed on v2
> 
>>
>>>
>>>               if (ctrl == NULL)
>>>                       continue;
>>>
>>
>> There is one more change that has to be made: setting a volatile control
>> should never generate a V4L2_EVENT_CTRL_CH_VALUE event since that makes
>> no sense. The way to prevent that is to ensure that ctrl->has_changed is
>> always false for volatile controls. The new_to_cur function looks at that
>> field to decide whether to send an event.
>>
>> The documentation should also be updated: that of V4L2_CTRL_FLAG_VOLATILE
>> (in VIDIOC_QUERYCTRL), and of V4L2_EVENT_CTRL_CH_VALUE.
> 
> I can do this also if you want. It has been a while without
> contributing to media :)

Yes, please. I can't accept the patch without these other changes anyway :-)

Regards,

	Hans