Re: [PATCH v1 10/19] uvcvideo: Support UVC 1.5 runtime control property.

2013-11-10 Thread Laurent Pinchart
Hi Pawel,

Thank you for the patch.

On Friday 30 August 2013 11:17:09 Pawel Osciak wrote:
 UVC 1.5 introduces the concept of runtime controls, which can be set during
 streaming. Non-runtime controls can only be changed while device is idle.
 
 Signed-off-by: Pawel Osciak posc...@chromium.org
 ---
  drivers/media/usb/uvc/uvc_ctrl.c | 45 ++---
  drivers/media/usb/uvc/uvc_v4l2.c | 18 ++--
  drivers/media/usb/uvc/uvcvideo.h | 12 +++
  3 files changed, 57 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
 b/drivers/media/usb/uvc/uvc_ctrl.c index d735c88..b0a19b9 100644
 --- a/drivers/media/usb/uvc/uvc_ctrl.c
 +++ b/drivers/media/usb/uvc/uvc_ctrl.c
 @@ -1076,8 +1076,19 @@ void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
 mutex_unlock(chain-pipeline-ctrl_mutex);
  }
 
 +static int uvc_check_ctrl_runtime(struct uvc_control *ctrl, bool streaming)
 +{
 + if (streaming  !ctrl-in_runtime) {
 + uvc_trace(UVC_TRACE_CONTROL,
 + Control disabled while streaming\n);
 + return -EBUSY;
 + }
 +
 + return 0;
 +}
 +
  int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 - struct v4l2_queryctrl *v4l2_ctrl)
 + struct v4l2_queryctrl *v4l2_ctrl, bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 @@ -1093,6 +1104,10 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain
 *chain, goto done;
   }
 
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + goto done;
 +

Do we really need to disallow querying controls during streaming ? Shouldn't 
we cache the runtime controls at init time instead ?

   ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
  done:
   __uvc_ctrl_unlock(chain);
 @@ -1109,7 +1124,7 @@ done:
   * manually.
   */
  int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
 - struct v4l2_querymenu *query_menu)
 + struct v4l2_querymenu *query_menu, bool streaming)
  {
   struct uvc_menu_info *menu_info;
   struct uvc_control_mapping *mapping;
 @@ -1132,6 +1147,10 @@ int uvc_query_v4l2_menu(struct uvc_video_chain
 *chain, goto done;
   }
 
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + goto done;
 +

Same here.

   if (query_menu-index = mapping-menu_count) {
   ret = -EINVAL;
   goto done;
 @@ -1436,21 +1455,26 @@ done:
   return ret;
  }
 
 -int uvc_ctrl_get(struct uvc_video_chain *chain,
 - struct v4l2_ext_control *xctrl)
 +int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
 *xctrl,
 +  bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 + int ret;
 
   ctrl = uvc_find_control(chain, xctrl-id, mapping);
   if (ctrl == NULL)
   return -EINVAL;
 
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + return ret;
 +

Even for the get operation, would it be possible to use the cached value ? Can 
a runtime control be also an auto-update or asynchronous control ?

   return __uvc_ctrl_get(chain, ctrl, mapping, xctrl-value);
  }
 
 -int uvc_ctrl_set(struct uvc_video_chain *chain,
 - struct v4l2_ext_control *xctrl)
 +int uvc_ctrl_set(struct uvc_video_chain *chain, struct v4l2_ext_control
 *xctrl,
 +  bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 @@ -1466,6 +1490,10 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
   if (!(ctrl-info.flags  UVC_CTRL_FLAG_SET_CUR))
   return -EACCES;
 
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + return ret;
 +
   /* Clamp out of range values. */
   switch (mapping-v4l2_type) {
   case V4L2_CTRL_TYPE_INTEGER:
 @@ -1885,8 +1913,9 @@ static int uvc_ctrl_add_info(struct uvc_device *dev,
 struct uvc_control *ctrl, ctrl-initialized = 1;
 
   uvc_trace(UVC_TRACE_CONTROL, Added control %pUl/%u to device %s 
 - entity %u\n, ctrl-info.entity, ctrl-info.selector,
 - dev-udev-devpath, ctrl-entity-id);
 + entity %u, init/runtime %d/%d\n, ctrl-info.entity,

%u/%u ?

 + ctrl-info.selector, dev-udev-devpath, ctrl-entity-id,
 + ctrl-on_init, ctrl-in_runtime);
 
  done:
   if (ret  0)
 diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
 b/drivers/media/usb/uvc/uvc_v4l2.c index a899159..decd65f 100644
 --- a/drivers/media/usb/uvc/uvc_v4l2.c
 +++ b/drivers/media/usb/uvc/uvc_v4l2.c
 @@ -597,7 +597,8 @@ static long uvc_v4l2_do_ioctl(struct file *file,
 unsigned int cmd, void *arg)
 
   /* Get, Set  Query control */
   case VIDIOC_QUERYCTRL:
 - return uvc_query_v4l2_ctrl(chain, arg);
 + return uvc_query_v4l2_ctrl(chain, arg,
 + 

Re: [PATCH v1 10/19] uvcvideo: Support UVC 1.5 runtime control property.

2013-08-30 Thread Hans Verkuil
On 08/30/2013 04:17 AM, Pawel Osciak wrote:
 UVC 1.5 introduces the concept of runtime controls, which can be set during
 streaming. Non-runtime controls can only be changed while device is idle.

I don't know if you know this, but the V4L2_CTRL_FLAG_GRABBED flag can be set 
for
controls that cannot be changed while streaming. Typically drivers will set that
flag for the relevant controls when streaming starts and clear it when streaming
stops.

Toggling that flag also means that a control event has to be generated reporting
a change of the control flags.

Regards,

Hans

 
 Signed-off-by: Pawel Osciak posc...@chromium.org
 ---
  drivers/media/usb/uvc/uvc_ctrl.c | 45 
 +---
  drivers/media/usb/uvc/uvc_v4l2.c | 18 ++--
  drivers/media/usb/uvc/uvcvideo.h | 12 +++
  3 files changed, 57 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/media/usb/uvc/uvc_ctrl.c 
 b/drivers/media/usb/uvc/uvc_ctrl.c
 index d735c88..b0a19b9 100644
 --- a/drivers/media/usb/uvc/uvc_ctrl.c
 +++ b/drivers/media/usb/uvc/uvc_ctrl.c
 @@ -1076,8 +1076,19 @@ void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
   mutex_unlock(chain-pipeline-ctrl_mutex);
  }
  
 +static int uvc_check_ctrl_runtime(struct uvc_control *ctrl, bool streaming)
 +{
 + if (streaming  !ctrl-in_runtime) {
 + uvc_trace(UVC_TRACE_CONTROL,
 + Control disabled while streaming\n);
 + return -EBUSY;
 + }
 +
 + return 0;
 +}
 +
  int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 - struct v4l2_queryctrl *v4l2_ctrl)
 + struct v4l2_queryctrl *v4l2_ctrl, bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 @@ -1093,6 +1104,10 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
   goto done;
   }
  
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + goto done;
 +
   ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
  done:
   __uvc_ctrl_unlock(chain);
 @@ -1109,7 +1124,7 @@ done:
   * manually.
   */
  int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
 - struct v4l2_querymenu *query_menu)
 + struct v4l2_querymenu *query_menu, bool streaming)
  {
   struct uvc_menu_info *menu_info;
   struct uvc_control_mapping *mapping;
 @@ -1132,6 +1147,10 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
   goto done;
   }
  
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + goto done;
 +
   if (query_menu-index = mapping-menu_count) {
   ret = -EINVAL;
   goto done;
 @@ -1436,21 +1455,26 @@ done:
   return ret;
  }
  
 -int uvc_ctrl_get(struct uvc_video_chain *chain,
 - struct v4l2_ext_control *xctrl)
 +int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control 
 *xctrl,
 +  bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 + int ret;
  
   ctrl = uvc_find_control(chain, xctrl-id, mapping);
   if (ctrl == NULL)
   return -EINVAL;
  
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + return ret;
 +
   return __uvc_ctrl_get(chain, ctrl, mapping, xctrl-value);
  }
  
 -int uvc_ctrl_set(struct uvc_video_chain *chain,
 - struct v4l2_ext_control *xctrl)
 +int uvc_ctrl_set(struct uvc_video_chain *chain, struct v4l2_ext_control 
 *xctrl,
 +  bool streaming)
  {
   struct uvc_control *ctrl;
   struct uvc_control_mapping *mapping;
 @@ -1466,6 +1490,10 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
   if (!(ctrl-info.flags  UVC_CTRL_FLAG_SET_CUR))
   return -EACCES;
  
 + ret = uvc_check_ctrl_runtime(ctrl, streaming);
 + if (ret  0)
 + return ret;
 +
   /* Clamp out of range values. */
   switch (mapping-v4l2_type) {
   case V4L2_CTRL_TYPE_INTEGER:
 @@ -1885,8 +1913,9 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, 
 struct uvc_control *ctrl,
   ctrl-initialized = 1;
  
   uvc_trace(UVC_TRACE_CONTROL, Added control %pUl/%u to device %s 
 - entity %u\n, ctrl-info.entity, ctrl-info.selector,
 - dev-udev-devpath, ctrl-entity-id);
 + entity %u, init/runtime %d/%d\n, ctrl-info.entity,
 + ctrl-info.selector, dev-udev-devpath, ctrl-entity-id,
 + ctrl-on_init, ctrl-in_runtime);
  
  done:
   if (ret  0)
 diff --git a/drivers/media/usb/uvc/uvc_v4l2.c 
 b/drivers/media/usb/uvc/uvc_v4l2.c
 index a899159..decd65f 100644
 --- a/drivers/media/usb/uvc/uvc_v4l2.c
 +++ b/drivers/media/usb/uvc/uvc_v4l2.c
 @@ -597,7 +597,8 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned 
 int cmd, void *arg)
  
   /* Get, Set  Query control */
   case VIDIOC_QUERYCTRL:
 - return uvc_query_v4l2_ctrl(chain, arg);
 + 

[PATCH v1 10/19] uvcvideo: Support UVC 1.5 runtime control property.

2013-08-29 Thread Pawel Osciak
UVC 1.5 introduces the concept of runtime controls, which can be set during
streaming. Non-runtime controls can only be changed while device is idle.

Signed-off-by: Pawel Osciak posc...@chromium.org
---
 drivers/media/usb/uvc/uvc_ctrl.c | 45 +---
 drivers/media/usb/uvc/uvc_v4l2.c | 18 ++--
 drivers/media/usb/uvc/uvcvideo.h | 12 +++
 3 files changed, 57 insertions(+), 18 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index d735c88..b0a19b9 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1076,8 +1076,19 @@ void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
mutex_unlock(chain-pipeline-ctrl_mutex);
 }
 
+static int uvc_check_ctrl_runtime(struct uvc_control *ctrl, bool streaming)
+{
+   if (streaming  !ctrl-in_runtime) {
+   uvc_trace(UVC_TRACE_CONTROL,
+   Control disabled while streaming\n);
+   return -EBUSY;
+   }
+
+   return 0;
+}
+
 int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
-   struct v4l2_queryctrl *v4l2_ctrl)
+   struct v4l2_queryctrl *v4l2_ctrl, bool streaming)
 {
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
@@ -1093,6 +1104,10 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
goto done;
}
 
+   ret = uvc_check_ctrl_runtime(ctrl, streaming);
+   if (ret  0)
+   goto done;
+
ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
 done:
__uvc_ctrl_unlock(chain);
@@ -1109,7 +1124,7 @@ done:
  * manually.
  */
 int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
-   struct v4l2_querymenu *query_menu)
+   struct v4l2_querymenu *query_menu, bool streaming)
 {
struct uvc_menu_info *menu_info;
struct uvc_control_mapping *mapping;
@@ -1132,6 +1147,10 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
goto done;
}
 
+   ret = uvc_check_ctrl_runtime(ctrl, streaming);
+   if (ret  0)
+   goto done;
+
if (query_menu-index = mapping-menu_count) {
ret = -EINVAL;
goto done;
@@ -1436,21 +1455,26 @@ done:
return ret;
 }
 
-int uvc_ctrl_get(struct uvc_video_chain *chain,
-   struct v4l2_ext_control *xctrl)
+int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl,
+bool streaming)
 {
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
+   int ret;
 
ctrl = uvc_find_control(chain, xctrl-id, mapping);
if (ctrl == NULL)
return -EINVAL;
 
+   ret = uvc_check_ctrl_runtime(ctrl, streaming);
+   if (ret  0)
+   return ret;
+
return __uvc_ctrl_get(chain, ctrl, mapping, xctrl-value);
 }
 
-int uvc_ctrl_set(struct uvc_video_chain *chain,
-   struct v4l2_ext_control *xctrl)
+int uvc_ctrl_set(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl,
+bool streaming)
 {
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
@@ -1466,6 +1490,10 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
if (!(ctrl-info.flags  UVC_CTRL_FLAG_SET_CUR))
return -EACCES;
 
+   ret = uvc_check_ctrl_runtime(ctrl, streaming);
+   if (ret  0)
+   return ret;
+
/* Clamp out of range values. */
switch (mapping-v4l2_type) {
case V4L2_CTRL_TYPE_INTEGER:
@@ -1885,8 +1913,9 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, 
struct uvc_control *ctrl,
ctrl-initialized = 1;
 
uvc_trace(UVC_TRACE_CONTROL, Added control %pUl/%u to device %s 
-   entity %u\n, ctrl-info.entity, ctrl-info.selector,
-   dev-udev-devpath, ctrl-entity-id);
+   entity %u, init/runtime %d/%d\n, ctrl-info.entity,
+   ctrl-info.selector, dev-udev-devpath, ctrl-entity-id,
+   ctrl-on_init, ctrl-in_runtime);
 
 done:
if (ret  0)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a899159..decd65f 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -597,7 +597,8 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned 
int cmd, void *arg)
 
/* Get, Set  Query control */
case VIDIOC_QUERYCTRL:
-   return uvc_query_v4l2_ctrl(chain, arg);
+   return uvc_query_v4l2_ctrl(chain, arg,
+   uvc_is_stream_streaming(stream));
 
case VIDIOC_G_CTRL:
{
@@ -611,7 +612,8 @@ static long uvc_v4l2_do_ioctl(struct file *file, unsigned 
int cmd, void *arg)
if (ret  0)
return ret;
 
-   ret = uvc_ctrl_get(chain, xctrl);
+   ret = uvc_ctrl_get(chain, xctrl,
+