[RFC,v3,04/19] media/usb/uvc: Implement vivioc_g_def_ext_ctrls

Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
use the controller framework.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
  

Laurent,

Can you review/ack this since it touches on uvc?

Thanks!

	Hans

On 06/12/2015 06:46 PM, Ricardo Ribalda Delgado wrote:
> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> use the controller framework.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 2764f43607c1..e2698a77138a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  	return uvc_ctrl_rollback(handle);
>  }
>  
> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> +				     struct v4l2_ext_controls *ctrls)
> +{
> +	struct uvc_fh *handle = fh;
> +	struct uvc_video_chain *chain = handle->chain;
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret;
> +	struct v4l2_queryctrl qc;
> +
> +	ret = uvc_ctrl_begin(chain);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		qc.id = ctrl->id;
> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> +		if (ret < 0) {
> +			ctrls->error_idx = i;
> +			return ret;
> +		}
> +		ctrl->value = qc.default_value;
> +	}
> +
> +	ctrls->error_idx = 0;
> +
> +	return 0;
> +}
> +
>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
>  				     bool commit)
> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>  	.vidioc_querymenu = uvc_ioctl_querymenu,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Ricardo,

Thank you for the patch.

On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> use the controller framework.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> void *fh, return uvc_ctrl_rollback(handle);
>  }
> 
> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> +				     struct v4l2_ext_controls *ctrls)
> +{
> +	struct uvc_fh *handle = fh;
> +	struct uvc_video_chain *chain = handle->chain;
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret;
> +	struct v4l2_queryctrl qc;
> +
> +	ret = uvc_ctrl_begin(chain);

There's no need to call uvc_ctrl_begin() here (and if there was, you'd have to 
call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).

> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		qc.id = ctrl->id;
> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> +		if (ret < 0) {
> +			ctrls->error_idx = i;
> +			return ret;
> +		}
> +		ctrl->value = qc.default_value;
> +	}
> +
> +	ctrls->error_idx = 0;
> +
> +	return 0;
> +}

Instead of open-coding this in multiple drivers, how about adding a helper 
function to the core ? Something like (totally untested)

int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
                               struct v4l2_ext_controls *ctrls)
{
	struct video_device *vdev = video_devdata(file);
	unsigned int i;
	int ret;

	for (i = 0; i < ctrls->count; ++i) {
		struct v4l2_queryctrl qc;

		qc.id = ctrl->id;
		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
		if (ret < 0) {
			ctrls->error_idx = i;
			return ret;
		}

		ctrls->controls[i].value = qc.default_value;
	}

	ctrls->error_idx = 0;

	return 0;
}

The function could be called by v4l_g_def_ext_ctrls() when ops-
>vidioc_g_def_ext_ctrls is NULL.

>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
>  				     bool commit)
> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>  	.vidioc_querymenu = uvc_ioctl_querymenu,
  

On 07/15/15 23:05, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>> use the controller framework.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, return uvc_ctrl_rollback(handle);
>>  }
>>
>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>> +				     struct v4l2_ext_controls *ctrls)
>> +{
>> +	struct uvc_fh *handle = fh;
>> +	struct uvc_video_chain *chain = handle->chain;
>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>> +	unsigned int i;
>> +	int ret;
>> +	struct v4l2_queryctrl qc;
>> +
>> +	ret = uvc_ctrl_begin(chain);
> 
> There's no need to call uvc_ctrl_begin() here (and if there was, you'd have to 
> call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>> +		qc.id = ctrl->id;
>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>> +		if (ret < 0) {
>> +			ctrls->error_idx = i;
>> +			return ret;
>> +		}
>> +		ctrl->value = qc.default_value;
>> +	}
>> +
>> +	ctrls->error_idx = 0;
>> +
>> +	return 0;
>> +}
> 
> Instead of open-coding this in multiple drivers, how about adding a helper 
> function to the core ? Something like (totally untested)

It's only open-coded in drivers that do not use the control framework. For
drivers that use the control framework it is completely transparent.

Regards,

	Hans

> 
> int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>                                struct v4l2_ext_controls *ctrls)
> {
> 	struct video_device *vdev = video_devdata(file);
> 	unsigned int i;
> 	int ret;
> 
> 	for (i = 0; i < ctrls->count; ++i) {
> 		struct v4l2_queryctrl qc;
> 
> 		qc.id = ctrl->id;
> 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
> 		if (ret < 0) {
> 			ctrls->error_idx = i;
> 			return ret;
> 		}
> 
> 		ctrls->controls[i].value = qc.default_value;
> 	}
> 
> 	ctrls->error_idx = 0;
> 
> 	return 0;
> }
> 
> The function could be called by v4l_g_def_ext_ctrls() when ops-
>> vidioc_g_def_ext_ctrls is NULL.
> 
>>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>>  				     struct v4l2_ext_controls *ctrls,
>>  				     bool commit)
>> @@ -1500,6 +1529,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>>  	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
>>  	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
>> +	.vidioc_g_def_ext_ctrls = uvc_ioctl_g_def_ext_ctrls,
>>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>>  	.vidioc_querymenu = uvc_ioctl_querymenu,
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Hans,

On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
> On 07/15/15 23:05, Laurent Pinchart wrote:
> > On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> >> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> >> use the controller framework.
> >> 
> >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >> ---
> >> 
> >>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
> >>  1 file changed, 30 insertions(+)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> >> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
> >> 100644
> >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
> >> *file, void *fh,
> >>  	return uvc_ctrl_rollback(handle);
> >>  }
> >> 
> >> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >> +				     struct v4l2_ext_controls *ctrls)
> >> +{
> >> +	struct uvc_fh *handle = fh;
> >> +	struct uvc_video_chain *chain = handle->chain;
> >> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> >> +	unsigned int i;
> >> +	int ret;
> >> +	struct v4l2_queryctrl qc;
> >> +
> >> +	ret = uvc_ctrl_begin(chain);
> > 
> > There's no need to call uvc_ctrl_begin() here (and if there was, you'd
> > have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> > 
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >> +		qc.id = ctrl->id;
> >> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> >> +		if (ret < 0) {
> >> +			ctrls->error_idx = i;
> >> +			return ret;
> >> +		}
> >> +		ctrl->value = qc.default_value;
> >> +	}
> >> +
> >> +	ctrls->error_idx = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Instead of open-coding this in multiple drivers, how about adding a helper
> > function to the core ? Something like (totally untested)
> 
> It's only open-coded in drivers that do not use the control framework. For
> drivers that use the control framework it is completely transparent.

Sure, but still, the same function is implemented several times while a single 
implementation could do. I'd prefer having it in the core until all (or all 
but one) drivers are converted to the control framework.

> > int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >                                struct v4l2_ext_controls *ctrls)
> > {
> > 	struct video_device *vdev = video_devdata(file);
> > 	unsigned int i;
> > 	int ret;
> > 	
> > 	for (i = 0; i < ctrls->count; ++i) {
> > 		struct v4l2_queryctrl qc;
> > 		
> > 		qc.id = ctrl->id;
> > 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
> > 		if (ret < 0) {
> > 			ctrls->error_idx = i;
> > 			return ret;
> > 		}
> > 		ctrls->controls[i].value = qc.default_value;
> > 	}
> > 	ctrls->error_idx = 0;
> > 	
> > 	return 0;
> > }
> > 
> > The function could be called by v4l_g_def_ext_ctrls() when ops->
> > vidioc_g_def_ext_ctrls is NULL.
  

On 07/16/15 10:11, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
>> On 07/15/15 23:05, Laurent Pinchart wrote:
>>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>>>> use the controller framework.
>>>>
>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>> ---
>>>>
>>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>>>  1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
>>>> 100644
>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
>>>> *file, void *fh,
>>>>  	return uvc_ctrl_rollback(handle);
>>>>  }
>>>>
>>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>> +				     struct v4l2_ext_controls *ctrls)
>>>> +{
>>>> +	struct uvc_fh *handle = fh;
>>>> +	struct uvc_video_chain *chain = handle->chain;
>>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>>>> +	unsigned int i;
>>>> +	int ret;
>>>> +	struct v4l2_queryctrl qc;
>>>> +
>>>> +	ret = uvc_ctrl_begin(chain);
>>>
>>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
>>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
>>>
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>>>> +		qc.id = ctrl->id;
>>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>>>> +		if (ret < 0) {
>>>> +			ctrls->error_idx = i;
>>>> +			return ret;
>>>> +		}
>>>> +		ctrl->value = qc.default_value;
>>>> +	}
>>>> +
>>>> +	ctrls->error_idx = 0;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Instead of open-coding this in multiple drivers, how about adding a helper
>>> function to the core ? Something like (totally untested)
>>
>> It's only open-coded in drivers that do not use the control framework. For
>> drivers that use the control framework it is completely transparent.
> 
> Sure, but still, the same function is implemented several times while a single 
> implementation could do. I'd prefer having it in the core until all (or all 
> but one) drivers are converted to the control framework.

There are only three drivers that need to implement this manually: uvc, saa7164
and pvrusb2. That's not enough to warrant moving this into the core.

One of these days I should sit down and convert saa7164 to the control framework.
That shouldn't be too difficult.

Regards,

	Hans

> 
>>> int v4l2_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>                                struct v4l2_ext_controls *ctrls)
>>> {
>>> 	struct video_device *vdev = video_devdata(file);
>>> 	unsigned int i;
>>> 	int ret;
>>> 	
>>> 	for (i = 0; i < ctrls->count; ++i) {
>>> 		struct v4l2_queryctrl qc;
>>> 		
>>> 		qc.id = ctrl->id;
>>> 		ret = vdev->ioctl_ops->vidioc_queryctrl(file, fh, &qc);
>>> 		if (ret < 0) {
>>> 			ctrls->error_idx = i;
>>> 			return ret;
>>> 		}
>>> 		ctrls->controls[i].value = qc.default_value;
>>> 	}
>>> 	ctrls->error_idx = 0;
>>> 	
>>> 	return 0;
>>> }
>>>
>>> The function could be called by v4l_g_def_ext_ctrls() when ops->
>>> vidioc_g_def_ext_ctrls is NULL.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thursday 16 July 2015 10:23:03 Hans Verkuil wrote:
> On 07/16/15 10:11, Laurent Pinchart wrote:
> > On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
> >> On 07/15/15 23:05, Laurent Pinchart wrote:
> >>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
> >>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
> >>>> use the controller framework.
> >>>> 
> >>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> >>>> ---
> >>>> 
> >>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
> >>>>  1 file changed, 30 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
> >>>> 100644
> >>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
> >>>> *file, void *fh,
> >>>>  	return uvc_ctrl_rollback(handle);
> >>>>  }
> >>>> 
> >>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
> >>>> +				     struct v4l2_ext_controls *ctrls)
> >>>> +{
> >>>> +	struct uvc_fh *handle = fh;
> >>>> +	struct uvc_video_chain *chain = handle->chain;
> >>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> >>>> +	unsigned int i;
> >>>> +	int ret;
> >>>> +	struct v4l2_queryctrl qc;
> >>>> +
> >>>> +	ret = uvc_ctrl_begin(chain);
> >>> 
> >>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
> >>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
> >>> 
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >>>> +		qc.id = ctrl->id;
> >>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
> >>>> +		if (ret < 0) {
> >>>> +			ctrls->error_idx = i;
> >>>> +			return ret;
> >>>> +		}
> >>>> +		ctrl->value = qc.default_value;
> >>>> +	}
> >>>> +
> >>>> +	ctrls->error_idx = 0;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>> 
> >>> Instead of open-coding this in multiple drivers, how about adding a
> >>> helper function to the core ? Something like (totally untested)
> >> 
> >> It's only open-coded in drivers that do not use the control framework.
> >> For drivers that use the control framework it is completely transparent.
> > 
> > Sure, but still, the same function is implemented several times while a
> > single implementation could do. I'd prefer having it in the core until
> > all (or all but one) drivers are converted to the control framework.
> 
> There are only three drivers that need to implement this manually: uvc,
> saa7164 and pvrusb2. That's not enough to warrant moving this into the
> core.

I'd argue that even just two drivers would be enough :-) Especially given that 
the proposed implementation for uvcvideo is wrong.

> One of these days I should sit down and convert saa7164 to the control
> framework. That shouldn't be too difficult.

How about pvrusb2, is there a good reason not to use the control framework 
there ?
  

On 07/16/15 10:27, Laurent Pinchart wrote:
> On Thursday 16 July 2015 10:23:03 Hans Verkuil wrote:
>> On 07/16/15 10:11, Laurent Pinchart wrote:
>>> On Thursday 16 July 2015 09:38:11 Hans Verkuil wrote:
>>>> On 07/15/15 23:05, Laurent Pinchart wrote:
>>>>> On Friday 12 June 2015 18:46:23 Ricardo Ribalda Delgado wrote:
>>>>>> Callback needed by ioctl VIDIOC_G_DEF_EXT_CTRLS as this driver does not
>>>>>> use the controller framework.
>>>>>>
>>>>>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> b/drivers/media/usb/uvc/uvc_v4l2.c index 2764f43607c1..e2698a77138a
>>>>>> 100644
>>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>> @@ -1001,6 +1001,35 @@ static int uvc_ioctl_g_ext_ctrls(struct file
>>>>>> *file, void *fh,
>>>>>>  	return uvc_ctrl_rollback(handle);
>>>>>>  }
>>>>>>
>>>>>> +static int uvc_ioctl_g_def_ext_ctrls(struct file *file, void *fh,
>>>>>> +				     struct v4l2_ext_controls *ctrls)
>>>>>> +{
>>>>>> +	struct uvc_fh *handle = fh;
>>>>>> +	struct uvc_video_chain *chain = handle->chain;
>>>>>> +	struct v4l2_ext_control *ctrl = ctrls->controls;
>>>>>> +	unsigned int i;
>>>>>> +	int ret;
>>>>>> +	struct v4l2_queryctrl qc;
>>>>>> +
>>>>>> +	ret = uvc_ctrl_begin(chain);
>>>>>
>>>>> There's no need to call uvc_ctrl_begin() here (and if there was, you'd
>>>>> have to call uvc_ctrl_rollback() or uvc_ctrl_commit() before returning).
>>>>>
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>>>>>> +		qc.id = ctrl->id;
>>>>>> +		ret = uvc_query_v4l2_ctrl(chain, &qc);
>>>>>> +		if (ret < 0) {
>>>>>> +			ctrls->error_idx = i;
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +		ctrl->value = qc.default_value;
>>>>>> +	}
>>>>>> +
>>>>>> +	ctrls->error_idx = 0;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>
>>>>> Instead of open-coding this in multiple drivers, how about adding a
>>>>> helper function to the core ? Something like (totally untested)
>>>>
>>>> It's only open-coded in drivers that do not use the control framework.
>>>> For drivers that use the control framework it is completely transparent.
>>>
>>> Sure, but still, the same function is implemented several times while a
>>> single implementation could do. I'd prefer having it in the core until
>>> all (or all but one) drivers are converted to the control framework.
>>
>> There are only three drivers that need to implement this manually: uvc,
>> saa7164 and pvrusb2. That's not enough to warrant moving this into the
>> core.
> 
> I'd argue that even just two drivers would be enough :-) Especially given that 
> the proposed implementation for uvcvideo is wrong.

I don't want to add code to the core to cater to exceptions.

>> One of these days I should sit down and convert saa7164 to the control
>> framework. That shouldn't be too difficult.
> 
> How about pvrusb2, is there a good reason not to use the control framework 
> there ?

Brrr. pvrusb2 exports controls (and a bunch of other things) to /sys allowing
scripts to get/set controls using cat and echo. I have no idea how to switch pvrusb2
to the control framework while still keeping this non-standard API.

I guess one of these days I will need to look at it, but I've been postponing it
for years now :-)

There are still a few other drivers that do not use the control framework:

saa7164, zoran, fsl-viu, omap_vout (although I hope that one can be removed),
usbvision and bcm2048 (that's a staging driver, so I can probably ignore that one).

pvrusb2 is actually fairly important: v4l2-subdev still has legacy ops for handling
controls and as long as pvrusb2 is not converted I cannot remove those ops.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hello Laurent

On Thu, Jul 16, 2015 at 10:27 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> I'd argue that even just two drivers would be enough :-) Especially given that
> the proposed implementation for uvcvideo is wrong.

This is why we have the review process :P. I do my best, but you are
the expert on your driver.

A core implementation cannot be completely correct, as it will not
support array controls.

I will resend this patch ASAP.


Thanks for your comments!