[PATCH 1/1] v4l: Ignore ctrl_class in the control framework

[PATCH 1/1] v4l: Ignore ctrl_class in the control framework

[Sakari Ailus]

Back in the old days there was probably a reason to require that controls
that are being used to access using VIDIOC_{TRY,G,S}_EXT_CTRLS belonged to
the same class. These days such reason does not exist, or at least cannot be
remembered, and concrete examples of the opposite can be seen: a single
(sub)device may well offer controls that belong to different classes and
there is no reason to deny changing them atomically.

This patch removes the check for v4l2_ext_controls.ctrl_class in the control
framework. The control framework issues the s_ctrl() op to the drivers
separately so changing the behaviour does not really change how this works
from the drivers' perspective.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/v4l2-ctrls.c |   18 +++++-------------
 1 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index da1f4c2..fff3bb3 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1855,9 +1855,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 
 		cs->error_idx = i;
 
-		if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
-			return -EINVAL;
-
 		/* Old-style private controls are not allowed for
 		   extended controls */
 		if (id >= V4L2_CID_PRIVATE_BASE)
@@ -1918,13 +1915,10 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 }
 
 /* Handles the corner case where cs->count == 0. It checks whether the
-   specified control class exists. If that class ID is 0, then it checks
-   whether there are any controls at all. */
-static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
+   there are any controls at all. */
+static int handler_check(struct v4l2_ctrl_handler *hdl)
 {
-	if (ctrl_class == 0)
-		return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
-	return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
+	return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
 }
 
 
@@ -1938,13 +1932,12 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 	int i, j;
 
 	cs->error_idx = cs->count;
-	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
 
 	if (hdl == NULL)
 		return -EINVAL;
 
 	if (cs->count == 0)
-		return class_check(hdl, cs->ctrl_class);
+		return handler_check(hdl);
 
 	if (cs->count > ARRAY_SIZE(helper)) {
 		helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
@@ -2160,13 +2153,12 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 	int ret;
 
 	cs->error_idx = cs->count;
-	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
 
 	if (hdl == NULL)
 		return -EINVAL;
 
 	if (cs->count == 0)
-		return class_check(hdl, cs->ctrl_class);
+		return handler_check(hdl);
 
 	if (cs->count > ARRAY_SIZE(helper)) {
 		helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
-- 
1.7.2.5

Re: [PATCH 1/1] v4l: Ignore ctrl_class in the control framework

[Hans Verkuil]

Hi Sakari,

On Tuesday, January 10, 2012 20:14:22 Sakari Ailus wrote:
> Back in the old days there was probably a reason to require that controls
> that are being used to access using VIDIOC_{TRY,G,S}_EXT_CTRLS belonged to
> the same class. These days such reason does not exist, or at least cannot be
> remembered, and concrete examples of the opposite can be seen: a single
> (sub)device may well offer controls that belong to different classes and
> there is no reason to deny changing them atomically.
> 
> This patch removes the check for v4l2_ext_controls.ctrl_class in the control
> framework. The control framework issues the s_ctrl() op to the drivers
> separately so changing the behaviour does not really change how this works
> from the drivers' perspective.

What is the rationale of this patch? It does change the behavior of the API.
There are still some drivers that use the extended control API without the
control framework (pvrusb2, and some other cx2341x-based drivers), and that
do test the ctrl_class argument.

I don't see any substantial gain by changing the current behavior of the
control framework.

Apps can just set ctrl_class to 0 and then the control framework will no
longer check the control class. And yes, this still has to be properly
documented in the spec.

The reason for the ctrl_class check is that without the control framework it
was next to impossible to allow atomic setting of controls of different
classes, since control of different classes would typically also be handled
by different drivers. By limiting the controls to one class it made it much
easier for drivers to implement this API.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/v4l2-ctrls.c |   18 +++++-------------
>  1 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index da1f4c2..fff3bb3 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -1855,9 +1855,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  
>  		cs->error_idx = i;
>  
> -		if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
> -			return -EINVAL;
> -
>  		/* Old-style private controls are not allowed for
>  		   extended controls */
>  		if (id >= V4L2_CID_PRIVATE_BASE)
> @@ -1918,13 +1915,10 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  }
>  
>  /* Handles the corner case where cs->count == 0. It checks whether the
> -   specified control class exists. If that class ID is 0, then it checks
> -   whether there are any controls at all. */
> -static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
> +   there are any controls at all. */
> +static int handler_check(struct v4l2_ctrl_handler *hdl)
>  {
> -	if (ctrl_class == 0)
> -		return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
> -	return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
> +	return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
>  }
>  
>  
> @@ -1938,13 +1932,12 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>  	int i, j;
>  
>  	cs->error_idx = cs->count;
> -	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>  
>  	if (hdl == NULL)
>  		return -EINVAL;
>  
>  	if (cs->count == 0)
> -		return class_check(hdl, cs->ctrl_class);
> +		return handler_check(hdl);
>  
>  	if (cs->count > ARRAY_SIZE(helper)) {
>  		helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
> @@ -2160,13 +2153,12 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  	int ret;
>  
>  	cs->error_idx = cs->count;
> -	cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>  
>  	if (hdl == NULL)
>  		return -EINVAL;
>  
>  	if (cs->count == 0)
> -		return class_check(hdl, cs->ctrl_class);
> +		return handler_check(hdl);
>  
>  	if (cs->count > ARRAY_SIZE(helper)) {
>  		helpers = kmalloc(sizeof(helper[0]) * cs->count, GFP_KERNEL);
> 

Re: [PATCH 1/1] v4l: Ignore ctrl_class in the control framework

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> On Tuesday, January 10, 2012 20:14:22 Sakari Ailus wrote:
>> Back in the old days there was probably a reason to require that controls
>> that are being used to access using VIDIOC_{TRY,G,S}_EXT_CTRLS belonged to
>> the same class. These days such reason does not exist, or at least cannot be
>> remembered, and concrete examples of the opposite can be seen: a single
>> (sub)device may well offer controls that belong to different classes and
>> there is no reason to deny changing them atomically.
>>
>> This patch removes the check for v4l2_ext_controls.ctrl_class in the control
>> framework. The control framework issues the s_ctrl() op to the drivers
>> separately so changing the behaviour does not really change how this works
>> from the drivers' perspective.
>
> What is the rationale of this patch? It does change the behavior of the API.
> There are still some drivers that use the extended control API without the
> control framework (pvrusb2, and some other cx2341x-based drivers), and that
> do test the ctrl_class argument.

These drivers still don't use the control framework. I don't see benefit 
in checking the class for those drivers that don't really care about it.

Also, to be able to set controls without artificial limitations 
applications have to set the ctrl_class field on some devices and on 
some they must not.

> I don't see any substantial gain by changing the current behavior of the
> control framework.
>
> Apps can just set ctrl_class to 0 and then the control framework will no
> longer check the control class. And yes, this still has to be properly
> documented in the spec.

That's a good point, indeed. Should the spec then say "on some drivers 
you must set it while on some you must not"? The difficulty, albeit not 
sure if it's a practical one, is that I don't think there's anything 
that would hint applications into which of the two classes a driver 
belongs to.

> The reason for the ctrl_class check is that without the control framework it
> was next to impossible to allow atomic setting of controls of different
> classes, since control of different classes would typically also be handled
> by different drivers. By limiting the controls to one class it made it much
> easier for drivers to implement this API.

Ok. But I don't think this patch would have any effect on those drivers.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH 1/1] v4l: Ignore ctrl_class in the control framework

[Hans Verkuil]

On Wednesday 11 January 2012 00:40:43 Sakari Ailus wrote:
> Hi Hans,
> 
> Hans Verkuil wrote:
> > On Tuesday, January 10, 2012 20:14:22 Sakari Ailus wrote:
> >> Back in the old days there was probably a reason to require that
> >> controls that are being used to access using VIDIOC_{TRY,G,S}_EXT_CTRLS
> >> belonged to the same class. These days such reason does not exist, or
> >> at least cannot be remembered, and concrete examples of the opposite
> >> can be seen: a single (sub)device may well offer controls that belong
> >> to different classes and there is no reason to deny changing them
> >> atomically.
> >> 
> >> This patch removes the check for v4l2_ext_controls.ctrl_class in the
> >> control framework. The control framework issues the s_ctrl() op to the
> >> drivers separately so changing the behaviour does not really change how
> >> this works from the drivers' perspective.
> > 
> > What is the rationale of this patch? It does change the behavior of the
> > API. There are still some drivers that use the extended control API
> > without the control framework (pvrusb2, and some other cx2341x-based
> > drivers), and that do test the ctrl_class argument.
> 
> These drivers still don't use the control framework. I don't see benefit
> in checking the class for those drivers that don't really care about it.

It's in the spec. It's as simple as that.

If I would start all over again, then I would remove the ctrl_class field.
But certainly for as long as there are drivers that support extended controls 
but not the control framework I want to keep this check.

> Also, to be able to set controls without artificial limitations
> applications have to set the ctrl_class field on some devices and on
> some they must not.

The fix for this is to convert the remaining drivers to the control framework.
But it is so hard to find the time to do this type of work :-(

> 
> > I don't see any substantial gain by changing the current behavior of the
> > control framework.
> > 
> > Apps can just set ctrl_class to 0 and then the control framework will no
> > longer check the control class. And yes, this still has to be properly
> > documented in the spec.
> 
> That's a good point, indeed. Should the spec then say "on some drivers
> you must set it while on some you must not"? The difficulty, albeit not
> sure if it's a practical one, is that I don't think there's anything
> that would hint applications into which of the two classes a driver
> belongs to.

There is: set the ctrl_class to 0, then call TRY_EXT_CTRLS with no controls
(i.e. count == 0). Those drivers that do not support setting ctrl_class to 0
will return -EINVAL.

> 
> > The reason for the ctrl_class check is that without the control framework
> > it was next to impossible to allow atomic setting of controls of
> > different classes, since control of different classes would typically
> > also be handled by different drivers. By limiting the controls to one
> > class it made it much easier for drivers to implement this API.
> 
> Ok. But I don't think this patch would have any effect on those drivers.

It doesn't. It's the drivers that use the control framework that are affected.
They no longer comply to the spec.

Regards,

	Hans