* [PATCH 0/3] uvcvideo: a cosmetic fix and 2 new features
@ 2016-06-24 11:28 Guennadi Liakhovetski
2016-06-24 11:28 ` [PATCH 1/3] uvcvideo: initialise the entity function field Guennadi Liakhovetski
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-06-24 11:28 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart
Hi Laurent,
The first patch in the series fixes a warning, introduced by a recent
framework change. Maybe you already have a similar one in your queue, drop
this one then, please.
Patch 2/3 implements support for asynchronous controls, using V4L2 events.
Patch 3/3 adds a metadata device node to enable streaming of private
payoad header to the user, based on your recent patch series. As everybody
was agreeing, I didn't use patch 2 from the series, adding a new metadata
video device type, using the GRABBER type instead. The additional video
node is so added unconditionally in this patch, which I find superfluous
and confusing for the user, because most cameras will not have any private
data in the payload header, so, those nodes will be useless. I'm open to
suggestions to make this node conditional on something - on a quirk flag?
Unfortunately you can only find out, whether a camera has additional data
in the header, when you receive the first frame.
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] uvcvideo: initialise the entity function field
2016-06-24 11:28 [PATCH 0/3] uvcvideo: a cosmetic fix and 2 new features Guennadi Liakhovetski
@ 2016-06-24 11:28 ` Guennadi Liakhovetski
2016-06-24 13:26 ` Laurent Pinchart
2016-06-24 11:29 ` [PATCH 2/3] uvcvideo: send a control event when a Control Change interrupt arrives Guennadi Liakhovetski
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
2 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-06-24 11:28 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart
Since a recent commit:
[media] media-device: move media entity register/unregister functions
drivers have to set entity function before registering an entity. Fix
the uvcvideo driver to comply with this.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/media/usb/uvc/uvc_entity.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index ac386bb..d93f413 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -88,6 +88,11 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
if (ret < 0)
return ret;
+ if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA)
+ entity->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ else
+ entity->subdev.entity.function = MEDIA_ENT_F_IO_V4L;
+
ret = v4l2_device_register_subdev(&chain->dev->vdev,
&entity->subdev);
} else if (entity->vdev != NULL) {
--
1.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] uvcvideo: send a control event when a Control Change interrupt arrives
2016-06-24 11:28 [PATCH 0/3] uvcvideo: a cosmetic fix and 2 new features Guennadi Liakhovetski
2016-06-24 11:28 ` [PATCH 1/3] uvcvideo: initialise the entity function field Guennadi Liakhovetski
@ 2016-06-24 11:29 ` Guennadi Liakhovetski
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
2 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-06-24 11:29 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart
UVC defines a method of handling asynchronous controls, which sends a
USB packet over the interrupt pipe. This patch implements support for
such packets by sending a control event to the user. Since this can
involve USB traffic and, therefore, scheduling, this has to be done
in a work queue.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
drivers/media/usb/uvc/uvc_ctrl.c | 121 ++++++++++++++++++++++++++++++++-----
drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++++++++++----
drivers/media/usb/uvc/uvc_v4l2.c | 4 +-
drivers/media/usb/uvc/uvcvideo.h | 14 ++++-
include/uapi/linux/uvcvideo.h | 2 +
5 files changed, 225 insertions(+), 28 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c2ee6e3..649a5b4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -20,6 +20,7 @@
#include <linux/videodev2.h>
#include <linux/vmalloc.h>
#include <linux/wait.h>
+#include <linux/workqueue.h>
#include <linux/atomic.h>
#include <media/v4l2-ctrls.h>
@@ -1236,16 +1237,93 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
}
}
-static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
- struct uvc_control *master, u32 slave_id,
- const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
+ struct uvc_control *master, u32 slave_id)
{
struct uvc_control_mapping *mapping = NULL;
struct uvc_control *ctrl = NULL;
u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
- unsigned int i;
s32 val = 0;
+ __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
+ if (ctrl == NULL)
+ return;
+
+ if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
+ changes |= V4L2_EVENT_CTRL_CH_VALUE;
+
+ uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+}
+
+static void uvc_ctrl_status_event_work(struct work_struct *work)
+{
+ struct uvc_device *dev = container_of(work, struct uvc_device,
+ async_ctrl.work);
+ struct uvc_ctrl_work *w = &dev->async_ctrl;
+ struct uvc_control_mapping *mapping;
+ struct uvc_control *ctrl;
+ struct uvc_fh *handle;
+ __u8 *data;
+ unsigned int i;
+
+ spin_lock_irq(&w->lock);
+ data = w->data;
+ w->data = NULL;
+ ctrl = w->ctrl;
+ handle = ctrl->handle;
+ ctrl->handle = NULL;
+ spin_unlock_irq(&w->lock);
+
+ list_for_each_entry(mapping, &ctrl->info.mappings, list) {
+ s32 value = mapping->get(mapping, UVC_GET_CUR, data);
+
+ for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
+ if (!mapping->slave_ids[i])
+ break;
+
+ __uvc_ctrl_send_slave_event(handle, ctrl,
+ mapping->slave_ids[i]);
+ }
+
+ if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
+ struct uvc_menu_info *menu = mapping->menu_info;
+ unsigned int i;
+
+ for (i = 0; i < mapping->menu_count; ++i, ++menu)
+ if (menu->value == value) {
+ value = i;
+ break;
+ }
+ }
+
+ uvc_ctrl_send_event(handle, ctrl, mapping, value,
+ V4L2_EVENT_CTRL_CH_VALUE);
+ }
+
+ kfree(data);
+}
+
+void uvc_ctrl_status_event(struct uvc_device *dev, struct uvc_control *ctrl,
+ __u8 *data, size_t len)
+{
+ struct uvc_ctrl_work *w = &dev->async_ctrl;
+
+ spin_lock(&w->lock);
+ w->data = kmalloc(len, GFP_ATOMIC);
+ if (w->data) {
+ memcpy(w->data, data, len);
+ w->ctrl = ctrl;
+ schedule_work(&w->work);
+ }
+ spin_unlock(&w->lock);
+}
+
+static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
+ struct uvc_control *master, u32 slave_id,
+ const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+{
+ unsigned int i;
+
/*
* We can skip sending an event for the slave if the slave
* is being modified in the same transaction.
@@ -1255,14 +1333,7 @@ static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
return;
}
- __uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
- if (ctrl == NULL)
- return;
-
- if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
- changes |= V4L2_EVENT_CTRL_CH_VALUE;
-
- uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+ __uvc_ctrl_send_slave_event(handle, master, slave_id);
}
static void uvc_ctrl_send_events(struct uvc_fh *handle,
@@ -1277,6 +1348,10 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
for (i = 0; i < xctrls_count; ++i) {
ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
+ if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+ /* Notification will be sent from an Interrupt event */
+ continue;
+
for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
if (!mapping->slave_ids[j])
break;
@@ -1472,9 +1547,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
}
-int uvc_ctrl_set(struct uvc_video_chain *chain,
+int uvc_ctrl_set(struct uvc_fh *handle,
struct v4l2_ext_control *xctrl)
{
+ struct uvc_video_chain *chain = handle->chain;
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
s32 value;
@@ -1488,6 +1564,11 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
return -EINVAL;
if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
return -EACCES;
+ if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) {
+ if (ctrl->handle)
+ return -EBUSY;
+ ctrl->handle = handle;
+ }
/* Clamp out of range values. */
switch (mapping->v4l2_type) {
@@ -1676,7 +1757,9 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev,
| (data[0] & UVC_CONTROL_CAP_SET ?
UVC_CTRL_FLAG_SET_CUR : 0)
| (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
- UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+ UVC_CTRL_FLAG_AUTO_UPDATE : 0)
+ | (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
+ UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
uvc_ctrl_fixup_xu_info(dev, ctrl, info);
@@ -2124,6 +2207,13 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
if (!ctrl->initialized)
return;
+ /* Temporarily abuse DATA_CURRENT buffer to avoid 1 byte allocation */
+ if (!uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id,
+ dev->intfnum, info->selector,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), 1) &&
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)[0] & 0x10)
+ ctrl->info.flags |= UVC_CTRL_FLAG_ASYNCHRONOUS;
+
for (; mapping < mend; ++mapping) {
if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
ctrl->info.selector == mapping->selector)
@@ -2139,6 +2229,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
struct uvc_entity *entity;
unsigned int i;
+ spin_lock_init(&dev->async_ctrl.lock);
+ INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
+
/* Walk the entities list and instantiate controls */
list_for_each_entry(entity, &dev->entities, list) {
struct uvc_control *ctrl;
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index f552ab9..1d8f776 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
/* --------------------------------------------------------------------------
* Status interrupt endpoint
*/
-static void uvc_event_streaming(struct uvc_device *dev, __u8 *data, int len)
+struct uvc_streaming_status {
+ __u8 bStatusType;
+ __u8 bOriginator;
+ __u8 bEvent;
+ __u8 bValue[];
+} __packed;
+
+struct uvc_control_status {
+ __u8 bStatusType;
+ __u8 bOriginator;
+ __u8 bEvent;
+ __u8 bSelector;
+ __u8 bAttribute;
+ __u8 bValue[];
+} __packed;
+
+static void uvc_event_streaming(struct uvc_device *dev,
+ struct uvc_streaming_status *status, int len)
{
if (len < 3) {
uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
@@ -86,30 +103,101 @@ static void uvc_event_streaming(struct uvc_device *dev, __u8 *data, int len)
return;
}
- if (data[2] == 0) {
+ if (status->bEvent == 0) {
if (len < 4)
return;
uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
- data[1], data[3] ? "pressed" : "released", len);
- uvc_input_report_key(dev, KEY_CAMERA, data[3]);
+ status->bOriginator,
+ status->bValue[0] ? "pressed" : "released", len);
+ uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
} else {
uvc_trace(UVC_TRACE_STATUS, "Stream %u error event %02x %02x "
- "len %d.\n", data[1], data[2], data[3], len);
+ "len %d.\n", status->bOriginator, status->bEvent,
+ status->bValue[0], len);
}
}
-static void uvc_event_control(struct uvc_device *dev, __u8 *data, int len)
+#define UVC_CTRL_VALUE_CHANGE 0
+#define UVC_CTRL_INFO_CHANGE 1
+#define UVC_CTRL_FAILURE_CHANGE 2
+#define UVC_CTRL_MIN_CHANGE 3
+#define UVC_CTRL_MAX_CHANGE 4
+
+static struct uvc_control *uvc_event_entity_ctrl(struct uvc_entity *entity,
+ __u8 selector)
+{
+ struct uvc_control *ctrl;
+ unsigned int i;
+
+ for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
+ if (ctrl->info.selector == selector)
+ return ctrl;
+
+ return NULL;
+}
+
+static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
+ struct uvc_control_status *status)
+{
+ struct uvc_video_chain *chain;
+
+ list_for_each_entry(chain, &dev->chains, list) {
+ struct uvc_entity *entity;
+ struct uvc_control *ctrl;
+
+ list_for_each_entry(entity, &chain->entities, chain) {
+ if (entity->id == status->bOriginator) {
+ ctrl = uvc_event_entity_ctrl(entity,
+ status->bSelector);
+ /*
+ * Some buggy cameras send asynchronous Control
+ * Change events for control, other than the
+ * ones, that had been changed, even though the
+ * AutoUpdate flag isn't set for the control.
+ */
+ if (ctrl && (!ctrl->handle ||
+ ctrl->handle->chain == chain))
+ return ctrl;
+ }
+ }
+ }
+
+ return NULL;
+}
+
+static void uvc_event_control(struct uvc_device *dev,
+ struct uvc_control_status *status, int len)
{
- char *attrs[3] = { "value", "info", "failure" };
+ struct uvc_control *ctrl;
+ char *attrs[] = { "value", "info", "failure", "min", "max" };
- if (len < 6 || data[2] != 0 || data[4] > 2) {
+ if (len < 6 || status->bEvent != 0 ||
+ status->bAttribute >= ARRAY_SIZE(attrs)) {
uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
"received.\n");
return;
}
uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
- data[1], data[3], attrs[data[4]], len);
+ status->bOriginator, status->bSelector,
+ attrs[status->bAttribute], len);
+
+ /* Find the control. */
+ ctrl = uvc_event_find_ctrl(dev, status);
+ if (!ctrl)
+ return;
+
+ switch (status->bAttribute) {
+ case UVC_CTRL_VALUE_CHANGE:
+ case UVC_CTRL_INFO_CHANGE:
+ uvc_ctrl_status_event(dev, ctrl, status->bValue, len -
+ offsetof(struct uvc_control_status, bValue));
+ break;
+ case UVC_CTRL_FAILURE_CHANGE:
+ case UVC_CTRL_MIN_CHANGE:
+ case UVC_CTRL_MAX_CHANGE:
+ break;
+ }
}
static void uvc_status_complete(struct urb *urb)
@@ -138,11 +226,13 @@ static void uvc_status_complete(struct urb *urb)
if (len > 0) {
switch (dev->status[0] & 0x0f) {
case UVC_STATUS_TYPE_CONTROL:
- uvc_event_control(dev, dev->status, len);
+ uvc_event_control(dev,
+ (struct uvc_control_status *)dev->status, len);
break;
case UVC_STATUS_TYPE_STREAMING:
- uvc_event_streaming(dev, dev->status, len);
+ uvc_event_streaming(dev,
+ (struct uvc_streaming_status *)dev->status, len);
break;
default:
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index cded5ef..ee295f3 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -975,7 +975,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
if (ret < 0)
return ret;
- ret = uvc_ctrl_set(chain, &xctrl);
+ ret = uvc_ctrl_set(handle, &xctrl);
if (ret < 0) {
uvc_ctrl_rollback(handle);
return ret;
@@ -1050,7 +1050,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
return ret;
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- ret = uvc_ctrl_set(chain, ctrl);
+ ret = uvc_ctrl_set(handle, ctrl);
if (ret < 0) {
uvc_ctrl_rollback(handle);
ctrls->error_idx = commit ? ctrls->count : i;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7e4d3ee..a61c55b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -11,6 +11,7 @@
#include <linux/usb/video.h>
#include <linux/uvcvideo.h>
#include <linux/videodev2.h>
+#include <linux/workqueue.h>
#include <media/media-device.h>
#include <media/v4l2-device.h>
#include <media/v4l2-event.h>
@@ -229,6 +230,8 @@ struct uvc_control {
initialized:1;
__u8 *uvc_data;
+
+ struct uvc_fh *handle; /* Used for asynchronous event delivery */
};
struct uvc_format_desc {
@@ -562,6 +565,13 @@ struct uvc_device {
__u8 *status;
struct input_dev *input;
char input_phys[64];
+
+ struct uvc_ctrl_work {
+ struct work_struct work;
+ struct uvc_control *ctrl;
+ spinlock_t lock;
+ void *data;
+ } async_ctrl;
};
enum uvc_handle_state {
@@ -708,6 +718,8 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
extern int uvc_ctrl_init_device(struct uvc_device *dev);
extern void uvc_ctrl_cleanup_device(struct uvc_device *dev);
extern int uvc_ctrl_restore_values(struct uvc_device *dev);
+extern void uvc_ctrl_status_event(struct uvc_device *dev,
+ struct uvc_control *ctrl, __u8 *data, size_t len);
extern int uvc_ctrl_begin(struct uvc_video_chain *chain);
extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
@@ -726,7 +738,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
extern int uvc_ctrl_get(struct uvc_video_chain *chain,
struct v4l2_ext_control *xctrl);
-extern int uvc_ctrl_set(struct uvc_video_chain *chain,
+extern int uvc_ctrl_set(struct uvc_fh *handle,
struct v4l2_ext_control *xctrl);
extern int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 3b08186..420883a 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -27,6 +27,8 @@
#define UVC_CTRL_FLAG_RESTORE (1 << 6)
/* Control can be updated by the camera. */
#define UVC_CTRL_FLAG_AUTO_UPDATE (1 << 7)
+/* Control supports asynchronous reporting */
+#define UVC_CTRL_FLAG_ASYNCHRONOUS (1 << 8)
#define UVC_CTRL_FLAG_GET_RANGE \
(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
--
1.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] uvcvideo: add a metadata device node
2016-06-24 11:28 [PATCH 0/3] uvcvideo: a cosmetic fix and 2 new features Guennadi Liakhovetski
2016-06-24 11:28 ` [PATCH 1/3] uvcvideo: initialise the entity function field Guennadi Liakhovetski
2016-06-24 11:29 ` [PATCH 2/3] uvcvideo: send a control event when a Control Change interrupt arrives Guennadi Liakhovetski
@ 2016-06-24 11:29 ` Guennadi Liakhovetski
2016-06-24 12:46 ` kbuild test robot
` (2 more replies)
2 siblings, 3 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-06-24 11:29 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart
Some UVC video cameras contain metadata in their payload headers. This
patch extracts that data, skipping the standard part of the header, on
both bulk and isochronous endpoints and makes it available to the user
space on a separate video node, using the V4L2_CAP_META_CAPTURE
capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
though different cameras will have different metadata formats, we use
the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
parse data, based on the specific camera model information.
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
drivers/media/usb/uvc/Makefile | 2 +-
drivers/media/usb/uvc/uvc_driver.c | 10 ++
drivers/media/usb/uvc/uvc_isight.c | 2 +-
drivers/media/usb/uvc/uvc_metadata.c | 234 +++++++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
drivers/media/usb/uvc/uvc_video.c | 59 +++++++--
drivers/media/usb/uvc/uvcvideo.h | 12 +-
7 files changed, 309 insertions(+), 12 deletions(-)
create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index c26d12f..06c7cd3 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,5 +1,5 @@
uvcvideo-objs := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
- uvc_status.o uvc_isight.o uvc_debugfs.o
+ uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
uvcvideo-objs += uvc_entity.o
endif
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 302e284..1a75ff0 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1740,6 +1740,9 @@ static void uvc_unregister_video(struct uvc_device *dev)
video_unregister_device(&stream->vdev);
+ if (video_is_registered(&stream->meta.vdev))
+ video_unregister_device(&stream->meta.vdev);
+
uvc_debugfs_cleanup_stream(stream);
}
@@ -1800,6 +1803,13 @@ static int uvc_register_video(struct uvc_device *dev,
return ret;
}
+ /*
+ * Register a metadata node. TODO: shall this only be enabled for some
+ * cameras?
+ */
+ if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
+ uvc_meta_register(stream);
+
if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
else
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c
index 8510e725..fb940cf 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -100,7 +100,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf,
}
void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
int ret, i;
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
new file mode 100644
index 0000000..54f326c
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -0,0 +1,234 @@
+/*
+ * uvc_metadata.c -- USB Video Class driver - Metadata handling
+ *
+ * Copyright (C) 2016
+ * Guennadi Liakhovetski (guennadi.liakhovetski@intel.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/usb.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-v4l2.h>
+#include <media/videobuf2-vmalloc.h>
+
+#include "uvcvideo.h"
+
+static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer *vbuf)
+{
+ return container_of(vbuf, struct uvc_buffer, buf);
+}
+
+/* -----------------------------------------------------------------------------
+ * videobuf2 Queue Operations
+ */
+
+/*
+ * Actually 253 bytes, but 256 is just a nicer number. We keep the buffer size
+ * constant and just set .usedbytes accordingly
+ */
+#define UVC_PAYLOAD_HEADER_MAX_SIZE 256
+
+static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+ unsigned int *nplanes, unsigned int sizes[],
+ void *alloc_ctxs[])
+{
+ if (*nplanes) {
+ if (*nplanes != 1)
+ return -EINVAL;
+
+ if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE)
+ return -EINVAL;
+
+ return 0;
+ }
+
+ *nplanes = 1;
+ sizes[0] = UVC_PAYLOAD_HEADER_MAX_SIZE;
+
+ return 0;
+}
+
+static int meta_buffer_prepare(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct uvc_buffer *buf = to_uvc_buffer(vbuf);
+
+ if (vb->num_planes != 1)
+ return -EINVAL;
+
+ if (vb2_plane_size(vb, 0) < UVC_PAYLOAD_HEADER_MAX_SIZE)
+ return -EINVAL;
+
+ buf->state = UVC_BUF_STATE_QUEUED;
+ buf->error = 0;
+ buf->mem = vb2_plane_vaddr(vb, 0);
+ buf->length = vb2_plane_size(vb, 0);
+ buf->bytesused = 0;
+
+ return 0;
+}
+
+static void meta_buffer_queue(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
+ struct uvc_buffer *buf = to_uvc_buffer(vbuf);
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+ list_add_tail(&buf->queue, &queue->irqqueue);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+}
+
+static int meta_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+ return 0;
+}
+
+static void meta_stop_streaming(struct vb2_queue *vq)
+{
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+ struct uvc_buffer *buffer;
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+
+ /* Remove all buffers from the IRQ queue. */
+ list_for_each_entry(buffer, &queue->irqqueue, queue)
+ vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
+ INIT_LIST_HEAD(&queue->irqqueue);
+
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+}
+
+static struct vb2_ops uvc_meta_queue_ops = {
+ .queue_setup = meta_queue_setup,
+ .buf_prepare = meta_buffer_prepare,
+ .buf_queue = meta_buffer_queue,
+ .wait_prepare = vb2_ops_wait_prepare,
+ .wait_finish = vb2_ops_wait_finish,
+ .start_streaming = meta_start_streaming,
+ .stop_streaming = meta_stop_streaming,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 ioctls
+ */
+
+static int meta_v4l2_querycap(struct file *file, void *fh,
+ struct v4l2_capability *cap)
+{
+ struct v4l2_fh *vfh = file->private_data;
+ struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
+
+ cap->device_caps = V4L2_CAP_META_CAPTURE
+ | V4L2_CAP_STREAMING;
+ cap->capabilities = V4L2_CAP_DEVICE_CAPS | cap->device_caps
+ | stream->chain->caps;
+
+ strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
+ strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
+ usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
+
+ return 0;
+}
+
+static int meta_v4l2_get_format(struct file *file, void *fh,
+ struct v4l2_format *format)
+{
+ struct v4l2_fh *vfh = file->private_data;
+ struct v4l2_meta_format *fmt = &format->fmt.meta;
+
+ if (format->type != vfh->vdev->queue->type)
+ return -EINVAL;
+
+ memset(fmt, 0, sizeof(*fmt));
+
+ fmt->dataformat = V4L2_META_FMT_UVC;
+ fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
+
+ return 0;
+}
+
+static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = {
+ .vidioc_querycap = meta_v4l2_querycap,
+ .vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_reqbufs = vb2_ioctl_reqbufs,
+ .vidioc_querybuf = vb2_ioctl_querybuf,
+ .vidioc_qbuf = vb2_ioctl_qbuf,
+ .vidioc_dqbuf = vb2_ioctl_dqbuf,
+ .vidioc_create_bufs = vb2_ioctl_create_bufs,
+ .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 File Operations
+ */
+
+static struct v4l2_file_operations uvc_meta_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = video_ioctl2,
+ .open = v4l2_fh_open,
+ .release = vb2_fop_release,
+ .poll = vb2_fop_poll,
+ .mmap = vb2_fop_mmap,
+};
+
+int uvc_meta_register(struct uvc_streaming *stream)
+{
+ struct uvc_device *dev = stream->dev;
+ struct uvc_meta_dev *meta = &stream->meta;
+ struct video_device *vdev = &meta->vdev;
+ struct uvc_video_queue *quvc = &meta->queue;
+ struct vb2_queue *queue = &quvc->queue;
+ int ret;
+
+ vdev->v4l2_dev = &dev->vdev;
+ vdev->fops = &uvc_meta_fops;
+ vdev->ioctl_ops = &uvc_meta_ioctl_ops;
+ vdev->release = video_device_release_empty;
+ vdev->prio = &stream->chain->prio;
+ vdev->vfl_dir = VFL_DIR_RX;
+ strlcpy(vdev->name, dev->name, sizeof(vdev->name));
+
+ video_set_drvdata(vdev, stream);
+
+ /* Initialize the video buffer queue. */
+ queue->type = V4L2_BUF_TYPE_META_CAPTURE;
+ queue->io_modes = VB2_MMAP | VB2_USERPTR;
+ queue->drv_priv = quvc;
+ queue->buf_struct_size = sizeof(struct uvc_buffer);
+ queue->ops = &uvc_meta_queue_ops;
+ queue->mem_ops = &vb2_vmalloc_memops;
+ queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
+ | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
+ queue->lock = &quvc->mutex;
+ ret = vb2_queue_init(queue);
+ if (ret < 0)
+ return ret;
+
+ mutex_init(&quvc->mutex);
+ spin_lock_init(&quvc->irqlock);
+ INIT_LIST_HEAD(&quvc->irqqueue);
+
+ vdev->queue = queue;
+
+ ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
+ if (ret < 0)
+ uvc_printk(KERN_ERR, "Failed to register metadata device (%d).\n", ret);
+
+ return ret;
+}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index ee295f3..9e6e2fb 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -572,7 +572,7 @@ static int uvc_ioctl_querycap(struct file *file, void *fh,
strlcpy(cap->card, vdev->name, sizeof(cap->card));
usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
- | chain->caps;
+ | V4L2_CAP_META_CAPTURE | chain->caps;
if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
else
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 79827dd..52e2d45 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1202,8 +1202,42 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
/*
* Completion handler for video URBs.
*/
+static void uvc_video_decode_meta(struct uvc_streaming *stream,
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
+ u8 *mem, int length)
+{
+ size_t header_len = 2;
+
+ if (!meta_buf)
+ return;
+
+ if (mem[1] & UVC_STREAM_PTS)
+ /* dwPresentationTime is included */
+ header_len += 4;
+ if (mem[1] & UVC_STREAM_SCR)
+ /* scrSourceClock is included */
+ header_len += 6;
+
+ if (length > header_len) {
+ size_t nbytes = min_t(unsigned int,
+ length - header_len, meta_buf->length);
+
+ meta_buf->buf.sequence = buf->buf.sequence;
+ meta_buf->buf.field = buf->buf.field;
+ meta_buf->buf.vb2_buf.timestamp =
+ buf->buf.vb2_buf.timestamp;
+
+ memcpy(meta_buf->mem, mem + header_len, nbytes);
+ meta_buf->bytesused = nbytes;
+ meta_buf->state = UVC_BUF_STATE_READY;
+
+ uvc_queue_next_buffer(&stream->meta.queue,
+ meta_buf);
+ }
+}
+
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem;
int ret, i;
@@ -1233,6 +1267,8 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
if (ret < 0)
continue;
+ uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
+
/* Decode the payload data. */
uvc_video_decode_data(stream, buf, mem + ret,
urb->iso_frame_desc[i].actual_length - ret);
@@ -1249,7 +1285,7 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
}
static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem;
int len, ret;
@@ -1283,6 +1319,8 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
memcpy(stream->bulk.header, mem, ret);
stream->bulk.header_size = ret;
+ uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
+
mem += ret;
len -= ret;
}
@@ -1306,8 +1344,7 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
uvc_video_decode_end(stream, buf, stream->bulk.header,
stream->bulk.payload_size);
if (buf->state == UVC_BUF_STATE_READY)
- buf = uvc_queue_next_buffer(&stream->queue,
- buf);
+ uvc_queue_next_buffer(&stream->queue, buf);
}
stream->bulk.header_size = 0;
@@ -1317,7 +1354,7 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
}
static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem = urb->transfer_buffer;
int len = stream->urb_size, ret;
@@ -1363,7 +1400,8 @@ static void uvc_video_complete(struct urb *urb)
{
struct uvc_streaming *stream = urb->context;
struct uvc_video_queue *queue = &stream->queue;
- struct uvc_buffer *buf = NULL;
+ struct uvc_video_queue *qmeta = &stream->meta.queue;
+ struct uvc_buffer *buf = NULL, *buf_meta = NULL;
unsigned long flags;
int ret;
@@ -1382,6 +1420,7 @@ static void uvc_video_complete(struct urb *urb)
case -ECONNRESET: /* usb_unlink_urb() called. */
case -ESHUTDOWN: /* The endpoint is being disabled. */
uvc_queue_cancel(queue, urb->status == -ESHUTDOWN);
+ uvc_queue_cancel(qmeta, urb->status == -ESHUTDOWN);
return;
}
@@ -1391,7 +1430,13 @@ static void uvc_video_complete(struct urb *urb)
queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- stream->decode(urb, stream, buf);
+ spin_lock_irqsave(&qmeta->irqlock, flags);
+ if (!list_empty(&qmeta->irqqueue))
+ buf_meta = list_first_entry(&qmeta->irqqueue, struct uvc_buffer,
+ queue);
+ spin_unlock_irqrestore(&qmeta->irqlock, flags);
+
+ stream->decode(urb, stream, buf, buf_meta);
if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a61c55b..7e2e220 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -455,6 +455,11 @@ struct uvc_stats_stream {
unsigned int max_sof; /* Maximum STC.SOF value */
};
+struct uvc_meta_dev {
+ struct video_device vdev;
+ struct uvc_video_queue queue;
+};
+
struct uvc_streaming {
struct list_head list;
struct uvc_device *dev;
@@ -486,7 +491,9 @@ struct uvc_streaming {
unsigned int frozen : 1;
struct uvc_video_queue queue;
void (*decode) (struct urb *urb, struct uvc_streaming *video,
- struct uvc_buffer *buf);
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
+
+ struct uvc_meta_dev meta;
/* Context data used by the bulk completion handler. */
struct {
@@ -698,6 +705,7 @@ extern int uvc_query_ctrl(struct uvc_device *dev, __u8 query, __u8 unit,
void uvc_video_clock_update(struct uvc_streaming *stream,
struct vb2_v4l2_buffer *vbuf,
struct uvc_buffer *buf);
+int uvc_meta_register(struct uvc_streaming *stream);
/* Status */
extern int uvc_status_init(struct uvc_device *dev);
@@ -754,7 +762,7 @@ extern struct usb_host_endpoint *uvc_find_endpoint(
/* Quirks support */
void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf);
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
/* debugfs and statistics */
int uvc_debugfs_init(void);
--
1.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] uvcvideo: add a metadata device node
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
@ 2016-06-24 12:46 ` kbuild test robot
2016-06-24 13:12 ` kbuild test robot
2016-12-02 10:53 ` [PATCH v2 " Guennadi Liakhovetski
2 siblings, 0 replies; 18+ messages in thread
From: kbuild test robot @ 2016-06-24 12:46 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: kbuild-all, Linux Media Mailing List, Laurent Pinchart
[-- Attachment #1: Type: text/plain, Size: 4620 bytes --]
Hi,
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.7-rc4 next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Guennadi-Liakhovetski/uvcvideo-a-cosmetic-fix-and-2-new-features/20160624-193142
base: git://linuxtv.org/media_tree.git master
config: x86_64-rhel (attached as .config)
compiler: gcc-4.9 (Debian 4.9.3-14) 4.9.3
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/media/usb/uvc/uvc_v4l2.c: In function 'uvc_ioctl_querycap':
>> drivers/media/usb/uvc/uvc_v4l2.c:575:8: error: 'V4L2_CAP_META_CAPTURE' undeclared (first use in this function)
| V4L2_CAP_META_CAPTURE | chain->caps;
^
drivers/media/usb/uvc/uvc_v4l2.c:575:8: note: each undeclared identifier is reported only once for each function it appears in
--
drivers/media/usb/uvc/uvc_metadata.c: In function 'meta_v4l2_querycap':
>> drivers/media/usb/uvc/uvc_metadata.c:133:21: error: 'V4L2_CAP_META_CAPTURE' undeclared (first use in this function)
cap->device_caps = V4L2_CAP_META_CAPTURE
^
drivers/media/usb/uvc/uvc_metadata.c:133:21: note: each undeclared identifier is reported only once for each function it appears in
drivers/media/usb/uvc/uvc_metadata.c: In function 'meta_v4l2_get_format':
>> drivers/media/usb/uvc/uvc_metadata.c:149:45: error: 'union <anonymous>' has no member named 'meta'
struct v4l2_meta_format *fmt = &format->fmt.meta;
^
>> drivers/media/usb/uvc/uvc_metadata.c:154:24: error: dereferencing pointer to incomplete type
memset(fmt, 0, sizeof(*fmt));
^
drivers/media/usb/uvc/uvc_metadata.c:156:5: error: dereferencing pointer to incomplete type
fmt->dataformat = V4L2_META_FMT_UVC;
^
>> drivers/media/usb/uvc/uvc_metadata.c:156:20: error: 'V4L2_META_FMT_UVC' undeclared (first use in this function)
fmt->dataformat = V4L2_META_FMT_UVC;
^
drivers/media/usb/uvc/uvc_metadata.c:157:5: error: dereferencing pointer to incomplete type
fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
^
drivers/media/usb/uvc/uvc_metadata.c: At top level:
>> drivers/media/usb/uvc/uvc_metadata.c:164:2: error: unknown field 'vidioc_g_fmt_meta_cap' specified in initializer
.vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
^
>> drivers/media/usb/uvc/uvc_metadata.c:164:2: warning: initialization from incompatible pointer type
drivers/media/usb/uvc/uvc_metadata.c:164:2: warning: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_cap')
>> drivers/media/usb/uvc/uvc_metadata.c:165:2: error: unknown field 'vidioc_s_fmt_meta_cap' specified in initializer
.vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:165:2: warning: initialization from incompatible pointer type
drivers/media/usb/uvc/uvc_metadata.c:165:2: warning: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_overlay')
>> drivers/media/usb/uvc/uvc_metadata.c:166:2: error: unknown field 'vidioc_try_fmt_meta_cap' specified in initializer
.vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:166:2: warning: initialization from incompatible pointer type
drivers/media/usb/uvc/uvc_metadata.c:166:2: warning: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_out')
drivers/media/usb/uvc/uvc_metadata.c: In function 'uvc_meta_register':
>> drivers/media/usb/uvc/uvc_metadata.c:210:16: error: 'V4L2_BUF_TYPE_META_CAPTURE' undeclared (first use in this function)
queue->type = V4L2_BUF_TYPE_META_CAPTURE;
^
vim +/V4L2_CAP_META_CAPTURE +575 drivers/media/usb/uvc/uvc_v4l2.c
569 struct uvc_streaming *stream = handle->stream;
570
571 strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
572 strlcpy(cap->card, vdev->name, sizeof(cap->card));
573 usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
574 cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
> 575 | V4L2_CAP_META_CAPTURE | chain->caps;
576 if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
577 cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
578 else
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 37222 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] uvcvideo: add a metadata device node
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
2016-06-24 12:46 ` kbuild test robot
@ 2016-06-24 13:12 ` kbuild test robot
2016-12-02 10:53 ` [PATCH v2 " Guennadi Liakhovetski
2 siblings, 0 replies; 18+ messages in thread
From: kbuild test robot @ 2016-06-24 13:12 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: kbuild-all, Linux Media Mailing List, Laurent Pinchart
[-- Attachment #1: Type: text/plain, Size: 5298 bytes --]
Hi,
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.7-rc4 next-20160624]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Guennadi-Liakhovetski/uvcvideo-a-cosmetic-fix-and-2-new-features/20160624-193142
base: git://linuxtv.org/media_tree.git master
config: arm-exynos_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/media/usb/uvc/uvc_metadata.c: In function 'meta_v4l2_querycap':
drivers/media/usb/uvc/uvc_metadata.c:133:21: error: 'V4L2_CAP_META_CAPTURE' undeclared (first use in this function)
cap->device_caps = V4L2_CAP_META_CAPTURE
^
drivers/media/usb/uvc/uvc_metadata.c:133:21: note: each undeclared identifier is reported only once for each function it appears in
drivers/media/usb/uvc/uvc_metadata.c: In function 'meta_v4l2_get_format':
drivers/media/usb/uvc/uvc_metadata.c:149:45: error: 'union <anonymous>' has no member named 'meta'
struct v4l2_meta_format *fmt = &format->fmt.meta;
^
In file included from include/linux/string.h:18:0,
from include/linux/dynamic_debug.h:111,
from include/linux/printk.h:289,
from include/linux/kernel.h:13,
from drivers/media/usb/uvc/uvc_metadata.c:13:
>> drivers/media/usb/uvc/uvc_metadata.c:154:24: error: dereferencing pointer to incomplete type 'struct v4l2_meta_format'
memset(fmt, 0, sizeof(*fmt));
^
arch/arm/include/asm/string.h:31:34: note: in definition of macro 'memset'
void *__p = (p); size_t __n = n; \
^
drivers/media/usb/uvc/uvc_metadata.c:156:20: error: 'V4L2_META_FMT_UVC' undeclared (first use in this function)
fmt->dataformat = V4L2_META_FMT_UVC;
^
drivers/media/usb/uvc/uvc_metadata.c: At top level:
drivers/media/usb/uvc/uvc_metadata.c:164:2: error: unknown field 'vidioc_g_fmt_meta_cap' specified in initializer
.vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
^
>> drivers/media/usb/uvc/uvc_metadata.c:164:28: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:164:28: note: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_cap')
drivers/media/usb/uvc/uvc_metadata.c:165:2: error: unknown field 'vidioc_s_fmt_meta_cap' specified in initializer
.vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:165:28: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:165:28: note: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_overlay')
drivers/media/usb/uvc/uvc_metadata.c:166:2: error: unknown field 'vidioc_try_fmt_meta_cap' specified in initializer
.vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:166:29: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
^
drivers/media/usb/uvc/uvc_metadata.c:166:29: note: (near initialization for 'uvc_meta_ioctl_ops.vidioc_enum_fmt_vid_out')
drivers/media/usb/uvc/uvc_metadata.c: In function 'uvc_meta_register':
drivers/media/usb/uvc/uvc_metadata.c:210:16: error: 'V4L2_BUF_TYPE_META_CAPTURE' undeclared (first use in this function)
queue->type = V4L2_BUF_TYPE_META_CAPTURE;
^
cc1: some warnings being treated as errors
vim +154 drivers/media/usb/uvc/uvc_metadata.c
143 }
144
145 static int meta_v4l2_get_format(struct file *file, void *fh,
146 struct v4l2_format *format)
147 {
148 struct v4l2_fh *vfh = file->private_data;
> 149 struct v4l2_meta_format *fmt = &format->fmt.meta;
150
151 if (format->type != vfh->vdev->queue->type)
152 return -EINVAL;
153
> 154 memset(fmt, 0, sizeof(*fmt));
155
> 156 fmt->dataformat = V4L2_META_FMT_UVC;
157 fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
158
159 return 0;
160 }
161
162 static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = {
163 .vidioc_querycap = meta_v4l2_querycap,
> 164 .vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
165 .vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
166 .vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
167 .vidioc_reqbufs = vb2_ioctl_reqbufs,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24938 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] uvcvideo: initialise the entity function field
2016-06-24 11:28 ` [PATCH 1/3] uvcvideo: initialise the entity function field Guennadi Liakhovetski
@ 2016-06-24 13:26 ` Laurent Pinchart
0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2016-06-24 13:26 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List
Hi Guennadi,
Thank you for the patch.
On Friday 24 Jun 2016 13:28:55 Guennadi Liakhovetski wrote:
> Since a recent commit:
>
> [media] media-device: move media entity register/unregister functions
>
> drivers have to set entity function before registering an entity. Fix
> the uvcvideo driver to comply with this.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> drivers/media/usb/uvc/uvc_entity.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_entity.c
> b/drivers/media/usb/uvc/uvc_entity.c index ac386bb..d93f413 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -88,6 +88,11 @@ static int uvc_mc_init_entity(struct uvc_video_chain
> *chain, if (ret < 0)
> return ret;
>
> + if (UVC_ENTITY_TYPE(entity) == UVC_ITT_CAMERA)
> + entity->subdev.entity.function =
MEDIA_ENT_F_CAM_SENSOR;
> + else
> + entity->subdev.entity.function = MEDIA_ENT_F_IO_V4L;
> +
I've discussed this some time ago with Hans (over IRC if I recall correctly).
We need to define new functions, as not all UVC entities map to the existing
ones. MEDIA_ENT_F_CAM_SENSOR should be fine for UVC_ITT_CAMERA, but
MEDIA_ENT_F_IO_V4L isn't right as a default.
> ret = v4l2_device_register_subdev(&chain->dev->vdev,
> &entity->subdev);
> } else if (entity->vdev != NULL) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
2016-06-24 12:46 ` kbuild test robot
2016-06-24 13:12 ` kbuild test robot
@ 2016-12-02 10:53 ` Guennadi Liakhovetski
2016-12-05 10:53 ` Laurent Pinchart
2 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-02 10:53 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Laurent Pinchart, Hans Verkuil
Some UVC video cameras contain metadata in their payload headers. This
patch extracts that data, skipping the standard part of the header, on
both bulk and isochronous endpoints and makes it available to the user
space on a separate video node, using the V4L2_CAP_META_CAPTURE
capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
though different cameras will have different metadata formats, we use
the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
parse data, based on the specific camera model information.
Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
v2:
- updated to the current media/master
- removed superfluous META capability from capture nodes
- now the complete UVC payload header is copied to buffers, including
standard fields
Still open for discussion: is this really OK to always create an
additional metadata node for each UVC camera or UVC video interface.
IIUC, Laurent's metadata node patch
https://patchwork.linuxtv.org/patch/36810/ has been acked by Hans and the
only thing, that's preventing it from being merged it the lack of
documentation. While waiting for documentation, I'd appreciate some
discussion of this patch to beat it into shape soon enough and have it
ready for merge soon after Laurent's patches are pulled in.
Thanks
Guennadi
drivers/media/usb/uvc/Makefile | 2 +-
drivers/media/usb/uvc/uvc_driver.c | 10 ++
drivers/media/usb/uvc/uvc_isight.c | 2 +-
drivers/media/usb/uvc/uvc_metadata.c | 228 +++++++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvc_video.c | 47 ++++++--
drivers/media/usb/uvc/uvcvideo.h | 12 +-
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/uvcvideo.h | 10 ++
include/uapi/linux/videodev2.h | 3 +
9 files changed, 304 insertions(+), 11 deletions(-)
create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
diff --git a/drivers/media/usb/uvc/Makefile b/drivers/media/usb/uvc/Makefile
index c26d12f..06c7cd3 100644
--- a/drivers/media/usb/uvc/Makefile
+++ b/drivers/media/usb/uvc/Makefile
@@ -1,5 +1,5 @@
uvcvideo-objs := uvc_driver.o uvc_queue.o uvc_v4l2.o uvc_video.o uvc_ctrl.o \
- uvc_status.o uvc_isight.o uvc_debugfs.o
+ uvc_status.o uvc_isight.o uvc_debugfs.o uvc_metadata.o
ifeq ($(CONFIG_MEDIA_CONTROLLER),y)
uvcvideo-objs += uvc_entity.o
endif
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 04bf350..edb67ac 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1866,6 +1866,9 @@ static void uvc_unregister_video(struct uvc_device *dev)
video_unregister_device(&stream->vdev);
+ if (video_is_registered(&stream->meta.vdev))
+ video_unregister_device(&stream->meta.vdev);
+
uvc_debugfs_cleanup_stream(stream);
}
@@ -1926,6 +1929,13 @@ static int uvc_register_video(struct uvc_device *dev,
return ret;
}
+ /*
+ * Register a metadata node. TODO: shall this only be enabled for some
+ * cameras?
+ */
+ if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
+ uvc_meta_register(stream);
+
if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
else
diff --git a/drivers/media/usb/uvc/uvc_isight.c b/drivers/media/usb/uvc/uvc_isight.c
index 8510e725..fb940cf 100644
--- a/drivers/media/usb/uvc/uvc_isight.c
+++ b/drivers/media/usb/uvc/uvc_isight.c
@@ -100,7 +100,7 @@ static int isight_decode(struct uvc_video_queue *queue, struct uvc_buffer *buf,
}
void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
int ret, i;
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
new file mode 100644
index 0000000..ddf77d9
--- /dev/null
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -0,0 +1,228 @@
+/*
+ * uvc_metadata.c -- USB Video Class driver - Metadata handling
+ *
+ * Copyright (C) 2016
+ * Guennadi Liakhovetski (guennadi.liakhovetski@intel.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/usb.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-v4l2.h>
+#include <media/videobuf2-vmalloc.h>
+
+#include "uvcvideo.h"
+
+static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer *vbuf)
+{
+ return container_of(vbuf, struct uvc_buffer, buf);
+}
+
+/* -----------------------------------------------------------------------------
+ * videobuf2 Queue Operations
+ */
+
+static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+ unsigned int *nplanes, unsigned int sizes[],
+ struct device *alloc_devs[])
+{
+ if (*nplanes) {
+ if (*nplanes != 1)
+ return -EINVAL;
+
+ if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE)
+ return -EINVAL;
+
+ return 0;
+ }
+
+ *nplanes = 1;
+ sizes[0] = UVC_PAYLOAD_HEADER_MAX_SIZE;
+
+ return 0;
+}
+
+static int meta_buffer_prepare(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct uvc_buffer *buf = to_uvc_buffer(vbuf);
+
+ if (vb->num_planes != 1)
+ return -EINVAL;
+
+ if (vb2_plane_size(vb, 0) < UVC_PAYLOAD_HEADER_MAX_SIZE)
+ return -EINVAL;
+
+ buf->state = UVC_BUF_STATE_QUEUED;
+ buf->error = 0;
+ buf->mem = vb2_plane_vaddr(vb, 0);
+ buf->length = vb2_plane_size(vb, 0);
+ buf->bytesused = 0;
+
+ return 0;
+}
+
+static void meta_buffer_queue(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
+ struct uvc_buffer *buf = to_uvc_buffer(vbuf);
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+ list_add_tail(&buf->queue, &queue->irqqueue);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+}
+
+static int meta_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+ return 0;
+}
+
+static void meta_stop_streaming(struct vb2_queue *vq)
+{
+ struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+ struct uvc_buffer *buffer;
+ unsigned long flags;
+
+ spin_lock_irqsave(&queue->irqlock, flags);
+
+ /* Remove all buffers from the IRQ queue. */
+ list_for_each_entry(buffer, &queue->irqqueue, queue)
+ vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
+ INIT_LIST_HEAD(&queue->irqqueue);
+
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+}
+
+static struct vb2_ops uvc_meta_queue_ops = {
+ .queue_setup = meta_queue_setup,
+ .buf_prepare = meta_buffer_prepare,
+ .buf_queue = meta_buffer_queue,
+ .wait_prepare = vb2_ops_wait_prepare,
+ .wait_finish = vb2_ops_wait_finish,
+ .start_streaming = meta_start_streaming,
+ .stop_streaming = meta_stop_streaming,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 ioctls
+ */
+
+static int meta_v4l2_querycap(struct file *file, void *fh,
+ struct v4l2_capability *cap)
+{
+ struct v4l2_fh *vfh = file->private_data;
+ struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
+
+ cap->device_caps = V4L2_CAP_META_CAPTURE
+ | V4L2_CAP_STREAMING;
+ cap->capabilities = V4L2_CAP_DEVICE_CAPS | cap->device_caps
+ | stream->chain->caps;
+
+ strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
+ strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
+ usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
+
+ return 0;
+}
+
+static int meta_v4l2_get_format(struct file *file, void *fh,
+ struct v4l2_format *format)
+{
+ struct v4l2_fh *vfh = file->private_data;
+ struct v4l2_meta_format *fmt = &format->fmt.meta;
+
+ if (format->type != vfh->vdev->queue->type)
+ return -EINVAL;
+
+ memset(fmt, 0, sizeof(*fmt));
+
+ fmt->dataformat = V4L2_META_FMT_UVC;
+ fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
+
+ return 0;
+}
+
+static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = {
+ .vidioc_querycap = meta_v4l2_querycap,
+ .vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
+ .vidioc_reqbufs = vb2_ioctl_reqbufs,
+ .vidioc_querybuf = vb2_ioctl_querybuf,
+ .vidioc_qbuf = vb2_ioctl_qbuf,
+ .vidioc_dqbuf = vb2_ioctl_dqbuf,
+ .vidioc_create_bufs = vb2_ioctl_create_bufs,
+ .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 File Operations
+ */
+
+static struct v4l2_file_operations uvc_meta_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = video_ioctl2,
+ .open = v4l2_fh_open,
+ .release = vb2_fop_release,
+ .poll = vb2_fop_poll,
+ .mmap = vb2_fop_mmap,
+};
+
+int uvc_meta_register(struct uvc_streaming *stream)
+{
+ struct uvc_device *dev = stream->dev;
+ struct uvc_meta_dev *meta = &stream->meta;
+ struct video_device *vdev = &meta->vdev;
+ struct uvc_video_queue *quvc = &meta->queue;
+ struct vb2_queue *queue = &quvc->queue;
+ int ret;
+
+ vdev->v4l2_dev = &dev->vdev;
+ vdev->fops = &uvc_meta_fops;
+ vdev->ioctl_ops = &uvc_meta_ioctl_ops;
+ vdev->release = video_device_release_empty;
+ vdev->prio = &stream->chain->prio;
+ vdev->vfl_dir = VFL_DIR_RX;
+ strlcpy(vdev->name, dev->name, sizeof(vdev->name));
+
+ video_set_drvdata(vdev, stream);
+
+ /* Initialize the video buffer queue. */
+ queue->type = V4L2_BUF_TYPE_META_CAPTURE;
+ queue->io_modes = VB2_MMAP | VB2_USERPTR;
+ queue->drv_priv = quvc;
+ queue->buf_struct_size = sizeof(struct uvc_buffer);
+ queue->ops = &uvc_meta_queue_ops;
+ queue->mem_ops = &vb2_vmalloc_memops;
+ queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
+ | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
+ queue->lock = &quvc->mutex;
+ ret = vb2_queue_init(queue);
+ if (ret < 0)
+ return ret;
+
+ mutex_init(&quvc->mutex);
+ spin_lock_init(&quvc->irqlock);
+ INIT_LIST_HEAD(&quvc->irqqueue);
+
+ vdev->queue = queue;
+
+ ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
+ if (ret < 0)
+ uvc_printk(KERN_ERR, "Failed to register metadata device (%d).\n", ret);
+
+ return ret;
+}
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index b5589d5..1bda8e1 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1158,8 +1158,30 @@ static void uvc_video_validate_buffer(const struct uvc_streaming *stream,
/*
* Completion handler for video URBs.
*/
+static void uvc_video_decode_meta(struct uvc_streaming *stream,
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
+ u8 *mem, int length)
+{
+ size_t nbytes;
+
+ if (!meta_buf || !length)
+ return;
+
+ nbytes = min_t(unsigned int, length, meta_buf->length);
+
+ meta_buf->buf.sequence = buf->buf.sequence;
+ meta_buf->buf.field = buf->buf.field;
+ meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
+
+ memcpy(meta_buf->mem, mem, nbytes);
+ meta_buf->bytesused = nbytes;
+ meta_buf->state = UVC_BUF_STATE_READY;
+
+ uvc_queue_next_buffer(&stream->meta.queue, meta_buf);
+}
+
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem;
int ret, i;
@@ -1189,6 +1211,8 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
if (ret < 0)
continue;
+ uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
+
/* Decode the payload data. */
uvc_video_decode_data(stream, buf, mem + ret,
urb->iso_frame_desc[i].actual_length - ret);
@@ -1205,7 +1229,7 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
}
static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem;
int len, ret;
@@ -1239,6 +1263,8 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
memcpy(stream->bulk.header, mem, ret);
stream->bulk.header_size = ret;
+ uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
+
mem += ret;
len -= ret;
}
@@ -1262,8 +1288,7 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
uvc_video_decode_end(stream, buf, stream->bulk.header,
stream->bulk.payload_size);
if (buf->state == UVC_BUF_STATE_READY)
- buf = uvc_queue_next_buffer(&stream->queue,
- buf);
+ uvc_queue_next_buffer(&stream->queue, buf);
}
stream->bulk.header_size = 0;
@@ -1273,7 +1298,7 @@ static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming *stream,
}
static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf)
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
{
u8 *mem = urb->transfer_buffer;
int len = stream->urb_size, ret;
@@ -1319,7 +1344,8 @@ static void uvc_video_complete(struct urb *urb)
{
struct uvc_streaming *stream = urb->context;
struct uvc_video_queue *queue = &stream->queue;
- struct uvc_buffer *buf = NULL;
+ struct uvc_video_queue *qmeta = &stream->meta.queue;
+ struct uvc_buffer *buf = NULL, *buf_meta = NULL;
unsigned long flags;
int ret;
@@ -1338,6 +1364,7 @@ static void uvc_video_complete(struct urb *urb)
case -ECONNRESET: /* usb_unlink_urb() called. */
case -ESHUTDOWN: /* The endpoint is being disabled. */
uvc_queue_cancel(queue, urb->status == -ESHUTDOWN);
+ uvc_queue_cancel(qmeta, urb->status == -ESHUTDOWN);
return;
}
@@ -1347,7 +1374,13 @@ static void uvc_video_complete(struct urb *urb)
queue);
spin_unlock_irqrestore(&queue->irqlock, flags);
- stream->decode(urb, stream, buf);
+ spin_lock_irqsave(&qmeta->irqlock, flags);
+ if (!list_empty(&qmeta->irqqueue))
+ buf_meta = list_first_entry(&qmeta->irqqueue, struct uvc_buffer,
+ queue);
+ spin_unlock_irqrestore(&qmeta->irqlock, flags);
+
+ stream->decode(urb, stream, buf, buf_meta);
if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 3d6cc62..ebff4b6 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -464,6 +464,11 @@ struct uvc_stats_stream {
unsigned int max_sof; /* Maximum STC.SOF value */
};
+struct uvc_meta_dev {
+ struct video_device vdev;
+ struct uvc_video_queue queue;
+};
+
struct uvc_streaming {
struct list_head list;
struct uvc_device *dev;
@@ -495,7 +500,9 @@ struct uvc_streaming {
unsigned int frozen : 1;
struct uvc_video_queue queue;
void (*decode) (struct urb *urb, struct uvc_streaming *video,
- struct uvc_buffer *buf);
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
+
+ struct uvc_meta_dev meta;
/* Context data used by the bulk completion handler. */
struct {
@@ -700,6 +707,7 @@ extern int uvc_query_ctrl(struct uvc_device *dev, __u8 query, __u8 unit,
void uvc_video_clock_update(struct uvc_streaming *stream,
struct vb2_v4l2_buffer *vbuf,
struct uvc_buffer *buf);
+int uvc_meta_register(struct uvc_streaming *stream);
/* Status */
extern int uvc_status_init(struct uvc_device *dev);
@@ -754,7 +762,7 @@ extern struct usb_host_endpoint *uvc_find_endpoint(
/* Quirks support */
void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
- struct uvc_buffer *buf);
+ struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
/* debugfs and statistics */
int uvc_debugfs_init(void);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 44a29af..1618be4 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1232,6 +1232,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_TCH_FMT_DELTA_TD08: descr = "8-bit signed deltas"; break;
case V4L2_TCH_FMT_TU16: descr = "16-bit unsigned touch data"; break;
case V4L2_TCH_FMT_TU08: descr = "8-bit unsigned touch data"; break;
+ case V4L2_META_FMT_UVC: descr = "UVC payload header metadata"; break;
default:
/* Compressed formats */
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 3b08186..e98de14 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -67,4 +67,14 @@ struct uvc_xu_control_query {
#define UVCIOC_CTRL_MAP _IOWR('u', 0x20, struct uvc_xu_control_mapping)
#define UVCIOC_CTRL_QUERY _IOWR('u', 0x21, struct uvc_xu_control_query)
+/*
+ * Metadata node
+ */
+
+/*
+ * Actually 255 bytes, but 256 is just a nicer number. We keep the buffer size
+ * constant and just set .usedbytes accordingly
+ */
+#define UVC_PAYLOAD_HEADER_MAX_SIZE 256
+
#endif
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1b894db..6b74191 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -678,6 +678,9 @@ struct v4l2_pix_format {
#define V4L2_TCH_FMT_TU16 v4l2_fourcc('T', 'U', '1', '6') /* 16-bit unsigned touch data */
#define V4L2_TCH_FMT_TU08 v4l2_fourcc('T', 'U', '0', '8') /* 8-bit unsigned touch data */
+/* Meta-data formats */
+#define V4L2_META_FMT_UVC v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
+
/* priv field value to indicates that subsequent fields are valid. */
#define V4L2_PIX_FMT_PRIV_MAGIC 0xfeedcafe
--
1.9.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-02 10:53 ` [PATCH v2 " Guennadi Liakhovetski
@ 2016-12-05 10:53 ` Laurent Pinchart
2016-12-05 15:35 ` Guennadi Liakhovetski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2016-12-05 10:53 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Guennadi,
Thank you for the patch.
On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> Some UVC video cameras contain metadata in their payload headers. This
> patch extracts that data, skipping the standard part of the header, on
> both bulk and isochronous endpoints and makes it available to the user
> space on a separate video node, using the V4L2_CAP_META_CAPTURE
> capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
> though different cameras will have different metadata formats, we use
> the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
> parse data, based on the specific camera model information.
>
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>
> v2:
> - updated to the current media/master
> - removed superfluous META capability from capture nodes
> - now the complete UVC payload header is copied to buffers, including
> standard fields
>
> Still open for discussion: is this really OK to always create an
> additional metadata node for each UVC camera or UVC video interface.
>
> IIUC, Laurent's metadata node patch
> https://patchwork.linuxtv.org/patch/36810/ has been acked by Hans and the
> only thing, that's preventing it from being merged it the lack of
> documentation. While waiting for documentation, I'd appreciate some
> discussion of this patch to beat it into shape soon enough and have it
> ready for merge soon after Laurent's patches are pulled in.
>
> Thanks
> Guennadi
>
> drivers/media/usb/uvc/Makefile | 2 +-
> drivers/media/usb/uvc/uvc_driver.c | 10 ++
> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> drivers/media/usb/uvc/uvc_metadata.c | 228 +++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvc_video.c | 47 ++++++--
> drivers/media/usb/uvc/uvcvideo.h | 12 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/uvcvideo.h | 10 ++
> include/uapi/linux/videodev2.h | 3 +
> 9 files changed, 304 insertions(+), 11 deletions(-)
> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 04bf350..edb67ac 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1866,6 +1866,9 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
>
> video_unregister_device(&stream->vdev);
>
> + if (video_is_registered(&stream->meta.vdev))
> + video_unregister_device(&stream->meta.vdev);
video_unregister_device() contains a video_is_registered(), no need to
duplicate it.
> +
> uvc_debugfs_cleanup_stream(stream);
> }
>
> @@ -1926,6 +1929,13 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
> }
>
> + /*
> + * Register a metadata node. TODO: shall this only be enabled for some
> + * cameras?
> + */
> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> + uvc_meta_register(stream);
> +
I think so, only for the cameras that can produce metadata.
> if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> else
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c
> b/drivers/media/usb/uvc/uvc_metadata.c new file mode 100644
> index 0000000..ddf77d9
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,228 @@
> +/*
> + * uvc_metadata.c -- USB Video Class driver - Metadata handling
> + *
> + * Copyright (C) 2016
> + * Guennadi Liakhovetski (guennadi.liakhovetski@intel.com)
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/videobuf2-vmalloc.h>
> +
> +#include "uvcvideo.h"
> +
> +static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer
> *vbuf)
> +{
> + return container_of(vbuf, struct uvc_buffer, buf);
> +}
You can move this function to uvcvideo.h and use it in uvc_queue.c (a separate
patch would be best).
> +/* ------------------------------------------------------------------------
> + * videobuf2 Queue Operations
> + */
> +
> +static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + if (*nplanes) {
> + if (*nplanes != 1)
> + return -EINVAL;
> +
> + if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE)
> + return -EINVAL;
> +
> + return 0;
> + }
> +
> + *nplanes = 1;
> + sizes[0] = UVC_PAYLOAD_HEADER_MAX_SIZE;
> +
> + return 0;
> +}
> +
> +static int meta_buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct uvc_buffer *buf = to_uvc_buffer(vbuf);
> +
> + if (vb->num_planes != 1)
> + return -EINVAL;
> +
> + if (vb2_plane_size(vb, 0) < UVC_PAYLOAD_HEADER_MAX_SIZE)
> + return -EINVAL;
> +
> + buf->state = UVC_BUF_STATE_QUEUED;
> + buf->error = 0;
> + buf->mem = vb2_plane_vaddr(vb, 0);
> + buf->length = vb2_plane_size(vb, 0);
> + buf->bytesused = 0;
> +
> + return 0;
> +}
> +
> +static void meta_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> + struct uvc_buffer *buf = to_uvc_buffer(vbuf);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&queue->irqlock, flags);
Shouldn't you handle the UVC_QUEUE_DISCONNECTED flag here ?
> + list_add_tail(&buf->queue, &queue->irqqueue);
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +}
> +
> +static int meta_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + return 0;
> +}
> +
> +static void meta_stop_streaming(struct vb2_queue *vq)
> +{
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> + struct uvc_buffer *buffer;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&queue->irqlock, flags);
> +
> + /* Remove all buffers from the IRQ queue. */
> + list_for_each_entry(buffer, &queue->irqqueue, queue)
> + vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
> + INIT_LIST_HEAD(&queue->irqqueue);
> +
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +}
> +
> +static struct vb2_ops uvc_meta_queue_ops = {
> + .queue_setup = meta_queue_setup,
> + .buf_prepare = meta_buffer_prepare,
> + .buf_queue = meta_buffer_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .start_streaming = meta_start_streaming,
> + .stop_streaming = meta_stop_streaming,
> +};
How about reusing the uvc_queue.c implementation, with a few new checks for
metadata buffers where needed, instead of duplicating code ? At first sight
the changes don't seem to be too intrusive (but I might have overlooked
something).
> +/* ------------------------------------------------------------------------
> + * V4L2 ioctls
> + */
> +
> +static int meta_v4l2_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
> +
> + cap->device_caps = V4L2_CAP_META_CAPTURE
> + | V4L2_CAP_STREAMING;
> + cap->capabilities = V4L2_CAP_DEVICE_CAPS | cap->device_caps
> + | stream->chain->caps;
If you set the device_caps field of struct video_device you won't need the
above four lines.
> + strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> + strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
> + usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap-
>bus_info));
> +
> + return 0;
> +}
> +
> +static int meta_v4l2_get_format(struct file *file, void *fh,
> + struct v4l2_format *format)
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct v4l2_meta_format *fmt = &format->fmt.meta;
> +
> + if (format->type != vfh->vdev->queue->type)
> + return -EINVAL;
> +
> + memset(fmt, 0, sizeof(*fmt));
> +
> + fmt->dataformat = V4L2_META_FMT_UVC;
> + fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = {
> + .vidioc_querycap = meta_v4l2_querycap,
> + .vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +};
> +
> +/* ------------------------------------------------------------------------
> + * V4L2 File Operations
> + */
> +
> +static struct v4l2_file_operations uvc_meta_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = video_ioctl2,
> + .open = v4l2_fh_open,
> + .release = vb2_fop_release,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +int uvc_meta_register(struct uvc_streaming *stream)
> +{
> + struct uvc_device *dev = stream->dev;
> + struct uvc_meta_dev *meta = &stream->meta;
> + struct video_device *vdev = &meta->vdev;
> + struct uvc_video_queue *quvc = &meta->queue;
> + struct vb2_queue *queue = &quvc->queue;
> + int ret;
> +
> + vdev->v4l2_dev = &dev->vdev;
> + vdev->fops = &uvc_meta_fops;
> + vdev->ioctl_ops = &uvc_meta_ioctl_ops;
> + vdev->release = video_device_release_empty;
> + vdev->prio = &stream->chain->prio;
> + vdev->vfl_dir = VFL_DIR_RX;
> + strlcpy(vdev->name, dev->name, sizeof(vdev->name));
> +
> + video_set_drvdata(vdev, stream);
> +
> + /* Initialize the video buffer queue. */
> + queue->type = V4L2_BUF_TYPE_META_CAPTURE;
> + queue->io_modes = VB2_MMAP | VB2_USERPTR;
> + queue->drv_priv = quvc;
> + queue->buf_struct_size = sizeof(struct uvc_buffer);
> + queue->ops = &uvc_meta_queue_ops;
> + queue->mem_ops = &vb2_vmalloc_memops;
> + queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> + | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> + queue->lock = &quvc->mutex;
> + ret = vb2_queue_init(queue);
> + if (ret < 0)
> + return ret;
> +
> + mutex_init(&quvc->mutex);
> + spin_lock_init(&quvc->irqlock);
> + INIT_LIST_HEAD(&quvc->irqqueue);
> +
> + vdev->queue = queue;
You can set vdev->queue above with the rest of the vdev initialization.
> + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> + if (ret < 0)
> + uvc_printk(KERN_ERR, "Failed to register metadata device (%d).
\n", ret);
> +
> + return ret;
> +}
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index b5589d5..1bda8e1 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1158,8 +1158,30 @@ static void uvc_video_validate_buffer(const struct
> uvc_streaming *stream, /*
> * Completion handler for video URBs.
> */
> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
> + u8 *mem, int length)
I don't think length can be negative, you can make it an unsigned int and
replace min_t with min below.
> +{
> + size_t nbytes;
> +
> + if (!meta_buf || !length)
> + return;
> +
> + nbytes = min_t(unsigned int, length, meta_buf->length);
> +
> + meta_buf->buf.sequence = buf->buf.sequence;
> + meta_buf->buf.field = buf->buf.field;
> + meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
> +
> + memcpy(meta_buf->mem, mem, nbytes);
Do you need the whole header ? Shouldn't you strip the part already handled by
the driver ?
> + meta_buf->bytesused = nbytes;
> + meta_buf->state = UVC_BUF_STATE_READY;
> +
> + uvc_queue_next_buffer(&stream->meta.queue, meta_buf);
This essentially means that you'll need one buffer per isochronous packet.
Given the number of isochronous packets that make a complete image the cost
seem prohibitive to me. You should instead gather metadata from all headers
into a single buffer.
> +}
> +
> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem;
> int ret, i;
> @@ -1189,6 +1211,8 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, if (ret < 0)
> continue;
>
> + uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
> +
> /* Decode the payload data. */
> uvc_video_decode_data(stream, buf, mem + ret,
> urb->iso_frame_desc[i].actual_length - ret);
> @@ -1205,7 +1229,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, }
>
> static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem;
> int len, ret;
> @@ -1239,6 +1263,8 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, memcpy(stream->bulk.header, mem, ret);
> stream->bulk.header_size = ret;
>
> + uvc_video_decode_meta(stream, buf, meta_buf, mem,
ret);
> +
> mem += ret;
> len -= ret;
> }
> @@ -1262,8 +1288,7 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, uvc_video_decode_end(stream, buf,
> stream->bulk.header,
> stream->bulk.payload_size);
> if (buf->state == UVC_BUF_STATE_READY)
> - buf = uvc_queue_next_buffer(&stream->queue,
> - buf);
> + uvc_queue_next_buffer(&stream->queue, buf);
This could be split to a separate patch.
> }
>
> stream->bulk.header_size = 0;
> @@ -1273,7 +1298,7 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, }
>
> static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem = urb->transfer_buffer;
> int len = stream->urb_size, ret;
> @@ -1319,7 +1344,8 @@ static void uvc_video_complete(struct urb *urb)
> {
> struct uvc_streaming *stream = urb->context;
> struct uvc_video_queue *queue = &stream->queue;
> - struct uvc_buffer *buf = NULL;
> + struct uvc_video_queue *qmeta = &stream->meta.queue;
> + struct uvc_buffer *buf = NULL, *buf_meta = NULL;
One variable per line please.
> unsigned long flags;
> int ret;
>
> @@ -1338,6 +1364,7 @@ static void uvc_video_complete(struct urb *urb)
> case -ECONNRESET: /* usb_unlink_urb() called. */
> case -ESHUTDOWN: /* The endpoint is being disabled. */
> uvc_queue_cancel(queue, urb->status == -ESHUTDOWN);
> + uvc_queue_cancel(qmeta, urb->status == -ESHUTDOWN);
> return;
> }
>
> @@ -1347,7 +1374,13 @@ static void uvc_video_complete(struct urb *urb)
> queue);
> spin_unlock_irqrestore(&queue->irqlock, flags);
>
> - stream->decode(urb, stream, buf);
> + spin_lock_irqsave(&qmeta->irqlock, flags);
> + if (!list_empty(&qmeta->irqqueue))
> + buf_meta = list_first_entry(&qmeta->irqqueue, struct
uvc_buffer,
> + queue);
> + spin_unlock_irqrestore(&qmeta->irqlock, flags);
> +
> + stream->decode(urb, stream, buf, buf_meta);
>
> if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 3d6cc62..ebff4b6 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -464,6 +464,11 @@ struct uvc_stats_stream {
> unsigned int max_sof; /* Maximum STC.SOF value */
> };
>
> +struct uvc_meta_dev {
All other structures use _device, not _dev. Let's be consistent.
> + struct video_device vdev;
> + struct uvc_video_queue queue;
> +};
> +
> struct uvc_streaming {
> struct list_head list;
> struct uvc_device *dev;
> @@ -495,7 +500,9 @@ struct uvc_streaming {
> unsigned int frozen : 1;
> struct uvc_video_queue queue;
> void (*decode) (struct urb *urb, struct uvc_streaming *video,
> - struct uvc_buffer *buf);
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
> +
> + struct uvc_meta_dev meta;
>
> /* Context data used by the bulk completion handler. */
> struct {
> @@ -700,6 +707,7 @@ extern int uvc_query_ctrl(struct uvc_device *dev, __u8
> query, __u8 unit, void uvc_video_clock_update(struct uvc_streaming *stream,
> struct vb2_v4l2_buffer *vbuf,
> struct uvc_buffer *buf);
> +int uvc_meta_register(struct uvc_streaming *stream);
>
> /* Status */
> extern int uvc_status_init(struct uvc_device *dev);
> @@ -754,7 +762,7 @@ extern struct usb_host_endpoint *uvc_find_endpoint(
>
> /* Quirks support */
> void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
> - struct uvc_buffer *buf);
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
>
> /* debugfs and statistics */
> int uvc_debugfs_init(void);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index 44a29af..1618be4 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1232,6 +1232,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> case V4L2_TCH_FMT_DELTA_TD08: descr = "8-bit signed deltas"; break; case
> V4L2_TCH_FMT_TU16: descr = "16-bit unsigned touch data"; break;
case
> V4L2_TCH_FMT_TU08: descr = "8-bit unsigned touch data"; break;
> + case
> V4L2_META_FMT_UVC: descr = "UVC payload header metadata"; break;
>
> default:
> /* Compressed formats */
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 3b08186..e98de14 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -67,4 +67,14 @@ struct uvc_xu_control_query {
> #define UVCIOC_CTRL_MAP _IOWR('u', 0x20, struct
uvc_xu_control_mapping)
> #define UVCIOC_CTRL_QUERY _IOWR('u', 0x21, struct uvc_xu_control_query)
>
> +/*
> + * Metadata node
> + */
> +
> +/*
> + * Actually 255 bytes, but 256 is just a nicer number. We keep the buffer
> size
> + * constant and just set .usedbytes accordingly
> + */
> +#define UVC_PAYLOAD_HEADER_MAX_SIZE 256
Why is lying about the size better ?
> #endif
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-05 10:53 ` Laurent Pinchart
@ 2016-12-05 15:35 ` Guennadi Liakhovetski
2016-12-05 22:06 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-05 15:35 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Laurent,
Thanks for the review! I'll work to address your comments. A couple of
clarifications:
On Mon, 5 Dec 2016, Laurent Pinchart wrote:
> Hi Guennadi,
>
> Thank you for the patch.
>
> On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> > Some UVC video cameras contain metadata in their payload headers. This
> > patch extracts that data, skipping the standard part of the header, on
> > both bulk and isochronous endpoints and makes it available to the user
> > space on a separate video node, using the V4L2_CAP_META_CAPTURE
> > capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
> > though different cameras will have different metadata formats, we use
> > the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
> > parse data, based on the specific camera model information.
> >
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> >
> > v2:
> > - updated to the current media/master
> > - removed superfluous META capability from capture nodes
> > - now the complete UVC payload header is copied to buffers, including
> > standard fields
> >
> > Still open for discussion: is this really OK to always create an
> > additional metadata node for each UVC camera or UVC video interface.
[snip]
> > + /*
> > + * Register a metadata node. TODO: shall this only be enabled for some
> > + * cameras?
> > + */
> > + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > + uvc_meta_register(stream);
> > +
>
> I think so, only for the cameras that can produce metadata.
Every UVC camera produces metadata, but most cameras only have standard
fields there. Whether we should stream standard header fields from the
metadata node will be discussed later. If we do decide to stream standard
header fields, then every USB camera gets metadata nodes. If we decide not
to include standard fields, how do we know whether the camera has any
private fields in headers without streaming from it? Do you want a quirk
for such cameras?
[snip]
> > +static struct vb2_ops uvc_meta_queue_ops = {
> > + .queue_setup = meta_queue_setup,
> > + .buf_prepare = meta_buffer_prepare,
> > + .buf_queue = meta_buffer_queue,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .start_streaming = meta_start_streaming,
> > + .stop_streaming = meta_stop_streaming,
> > +};
>
> How about reusing the uvc_queue.c implementation, with a few new checks for
> metadata buffers where needed, instead of duplicating code ? At first sight
> the changes don't seem to be too intrusive (but I might have overlooked
> something).
I thought about that in the beginning and even started implementing it
that way, but at some point it became too inconvenient, so, I switched
over to a separate implementation. I'll think about it more and either
explain, why I didn't want to do that, or unite them.
[snip]
> > +{
> > + size_t nbytes;
> > +
> > + if (!meta_buf || !length)
> > + return;
> > +
> > + nbytes = min_t(unsigned int, length, meta_buf->length);
> > +
> > + meta_buf->buf.sequence = buf->buf.sequence;
> > + meta_buf->buf.field = buf->buf.field;
> > + meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
> > +
> > + memcpy(meta_buf->mem, mem, nbytes);
>
> Do you need the whole header ? Shouldn't you strip the part already handled by
> the driver ?
My original version did that, but we also need the timestamp from the
header. The driver does parse it, but the implementation there has
multiple times been reported as buggy and noone has been able to fix it so
far :) So, I ended up pulling the complete header to the user-space.
Unless time-stamp processing is fixed in the kernel, I don't think we can
frop this.
> > + meta_buf->bytesused = nbytes;
> > + meta_buf->state = UVC_BUF_STATE_READY;
> > +
> > + uvc_queue_next_buffer(&stream->meta.queue, meta_buf);
>
> This essentially means that you'll need one buffer per isochronous packet.
> Given the number of isochronous packets that make a complete image the cost
> seem prohibitive to me. You should instead gather metadata from all headers
> into a single buffer.
Hm, I only worked with cameras, using bulk transfers, so, didn't consider
ISOC implications. Will have to think about this.
[snip]
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-05 15:35 ` Guennadi Liakhovetski
@ 2016-12-05 22:06 ` Laurent Pinchart
2016-12-05 22:13 ` Guennadi Liakhovetski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2016-12-05 22:06 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Guennadi,
On Monday 05 Dec 2016 16:35:39 Guennadi Liakhovetski wrote:
> On Mon, 5 Dec 2016, Laurent Pinchart wrote:
> > On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> >> Some UVC video cameras contain metadata in their payload headers. This
> >> patch extracts that data, skipping the standard part of the header, on
> >> both bulk and isochronous endpoints and makes it available to the user
> >> space on a separate video node, using the V4L2_CAP_META_CAPTURE
> >> capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
> >> though different cameras will have different metadata formats, we use
> >> the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
> >> parse data, based on the specific camera model information.
> >>
> >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> >> ---
> >>
> >> v2:
> >> - updated to the current media/master
> >> - removed superfluous META capability from capture nodes
> >> - now the complete UVC payload header is copied to buffers, including
> >> standard fields
> >>
> >> Still open for discussion: is this really OK to always create an
> >> additional metadata node for each UVC camera or UVC video interface.
>
> [snip]
>
> >> + /*
> >> + * Register a metadata node. TODO: shall this only be enabled for some
> >> + * cameras?
> >> + */
> >> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >> + uvc_meta_register(stream);
> >> +
> >
> > I think so, only for the cameras that can produce metadata.
>
> Every UVC camera produces metadata, but most cameras only have standard
> fields there. Whether we should stream standard header fields from the
> metadata node will be discussed later. If we do decide to stream standard
> header fields, then every USB camera gets metadata nodes. If we decide not
> to include standard fields, how do we know whether the camera has any
> private fields in headers without streaming from it? Do you want a quirk
> for such cameras?
Unless they can be detected in a standard way that's probably the best
solution. Please remember that the UVC specification doesn't allow vendors to
extend headers in a vendor-specific way. This is an abuse of the
specification, so a quirk is probably the best option.
> [snip]
>
> > > +static struct vb2_ops uvc_meta_queue_ops = {
> > > + .queue_setup = meta_queue_setup,
> > > + .buf_prepare = meta_buffer_prepare,
> > > + .buf_queue = meta_buffer_queue,
> > > + .wait_prepare = vb2_ops_wait_prepare,
> > > + .wait_finish = vb2_ops_wait_finish,
> > > + .start_streaming = meta_start_streaming,
> > > + .stop_streaming = meta_stop_streaming,
> > > +};
> >
> > How about reusing the uvc_queue.c implementation, with a few new checks
> > for metadata buffers where needed, instead of duplicating code ? At first
> > sight the changes don't seem to be too intrusive (but I might have
> > overlooked something).
>
> I thought about that in the beginning and even started implementing it
> that way, but at some point it became too inconvenient, so, I switched
> over to a separate implementation. I'll think about it more and either
> explain, why I didn't want to do that, or unite them.
>
> [snip]
>
> > > +{
> > > + size_t nbytes;
> > > +
> > > + if (!meta_buf || !length)
> > > + return;
> > > +
> > > + nbytes = min_t(unsigned int, length, meta_buf->length);
> > > +
> > > + meta_buf->buf.sequence = buf->buf.sequence;
> > > + meta_buf->buf.field = buf->buf.field;
> > > + meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
> > > +
> > > + memcpy(meta_buf->mem, mem, nbytes);
> >
> > Do you need the whole header ? Shouldn't you strip the part already
> > handled by the driver ?
>
> My original version did that, but we also need the timestamp from the
> header. The driver does parse it, but the implementation there has
> multiple times been reported as buggy and noone has been able to fix it so
> far :)
-ENOTIME I'm afraid, but feel free to give it a go :-) On the other hand
supplying the PTS and SCR values to userspace would be useful to implement a
high-accuracy clock translation algorithm that could make use of floating
point operations.
> So, I ended up pulling the complete header to the user-space. Unless time-
> stamp processing is fixed in the kernel, I don't think we can frop this.
SCR and PTS should be provided to userspace in a standard way.
> >> + meta_buf->bytesused = nbytes;
> >> + meta_buf->state = UVC_BUF_STATE_READY;
> >> +
> >> + uvc_queue_next_buffer(&stream->meta.queue, meta_buf);
> >
> > This essentially means that you'll need one buffer per isochronous packet.
> > Given the number of isochronous packets that make a complete image the
> > cost seem prohibitive to me. You should instead gather metadata from all
> > headers into a single buffer.
>
> Hm, I only worked with cameras, using bulk transfers, so, didn't consider
> ISOC implications. Will have to think about this.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-05 22:06 ` Laurent Pinchart
@ 2016-12-05 22:13 ` Guennadi Liakhovetski
2016-12-05 22:25 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-05 22:13 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Hans Verkuil
Just one question:
On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >> + /*
> > >> + * Register a metadata node. TODO: shall this only be enabled for some
> > >> + * cameras?
> > >> + */
> > >> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >> + uvc_meta_register(stream);
> > >> +
> > >
> > > I think so, only for the cameras that can produce metadata.
> >
> > Every UVC camera produces metadata, but most cameras only have standard
> > fields there. Whether we should stream standard header fields from the
> > metadata node will be discussed later. If we do decide to stream standard
> > header fields, then every USB camera gets metadata nodes. If we decide not
> > to include standard fields, how do we know whether the camera has any
> > private fields in headers without streaming from it? Do you want a quirk
> > for such cameras?
>
> Unless they can be detected in a standard way that's probably the best
> solution. Please remember that the UVC specification doesn't allow vendors to
> extend headers in a vendor-specific way.
Does it not? Where is that specified? I didn't find that anywhere.
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-05 22:13 ` Guennadi Liakhovetski
@ 2016-12-05 22:25 ` Laurent Pinchart
2016-12-06 10:39 ` Guennadi Liakhovetski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2016-12-05 22:25 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Guennadi,
On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> Just one question:
>
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >>>> + /*
> >>>> + * Register a metadata node. TODO: shall this only be enabled
> >>>> for some
> >>>> + * cameras?
> >>>> + */
> >>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >>>> + uvc_meta_register(stream);
> >>>> +
> >>>
> >>> I think so, only for the cameras that can produce metadata.
> >>
> >> Every UVC camera produces metadata, but most cameras only have standard
> >> fields there. Whether we should stream standard header fields from the
> >> metadata node will be discussed later. If we do decide to stream
> >> standard header fields, then every USB camera gets metadata nodes. If we
> >> decide not to include standard fields, how do we know whether the camera
> >> has any private fields in headers without streaming from it? Do you want
> >> a quirk for such cameras?
> >
> > Unless they can be detected in a standard way that's probably the best
> > solution. Please remember that the UVC specification doesn't allow vendors
> > to extend headers in a vendor-specific way.
>
> Does it not? Where is that specified? I didn't find that anywhere.
It's the other way around, it document a standard way to extend the payload
header. Any option you pick risks being incompatible with future revisions of
the specification. For instance the payload header's bmHeaderInfo field can be
extended through the use of the EOF bit, but a future version of the
specification could also extend it, in a way that would make a vendor-specific
extension be mistaken for the standard extension.
Now, you could add data after the standard payload without touching the
bmHeaderInfo field, but I'm still worried it could be broken by future
versions of the specification.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-05 22:25 ` Laurent Pinchart
@ 2016-12-06 10:39 ` Guennadi Liakhovetski
2016-12-06 15:56 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-06 10:39 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Laurent,
On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> > Just one question:
> >
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >>>> + /*
> > >>>> + * Register a metadata node. TODO: shall this only be enabled
> > >>>> for some
> > >>>> + * cameras?
> > >>>> + */
> > >>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >>>> + uvc_meta_register(stream);
> > >>>> +
> > >>>
> > >>> I think so, only for the cameras that can produce metadata.
> > >>
> > >> Every UVC camera produces metadata, but most cameras only have standard
> > >> fields there. Whether we should stream standard header fields from the
> > >> metadata node will be discussed later. If we do decide to stream
> > >> standard header fields, then every USB camera gets metadata nodes. If we
> > >> decide not to include standard fields, how do we know whether the camera
> > >> has any private fields in headers without streaming from it? Do you want
> > >> a quirk for such cameras?
> > >
> > > Unless they can be detected in a standard way that's probably the best
> > > solution. Please remember that the UVC specification doesn't allow vendors
> > > to extend headers in a vendor-specific way.
> >
> > Does it not? Where is that specified? I didn't find that anywhere.
>
> It's the other way around, it document a standard way to extend the payload
> header. Any option you pick risks being incompatible with future revisions of
> the specification. For instance the payload header's bmHeaderInfo field can be
> extended through the use of the EOF bit, but a future version of the
> specification could also extend it, in a way that would make a vendor-specific
> extension be mistaken for the standard extension.
>
> Now, you could add data after the standard payload without touching the
> bmHeaderInfo field, but I'm still worried it could be broken by future
> versions of the specification.
Exactly - using "free" space in payload headers is not a part of the spec,
but is also not prohibited by it. As for future versions - cameras specify
which version of the spec they implement, and existing versions cannot
change. If a camera decides to upgrade and in future UVC versions there
won't be enough space left for the private data, only then the camera
manufacturer will have a problem, that they will have to solve. The
user-space software will have to know, that UVC 5.1 metadata has a
different format, than UVC 1.5, that's true.
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-06 10:39 ` Guennadi Liakhovetski
@ 2016-12-06 15:56 ` Laurent Pinchart
2016-12-08 13:34 ` Guennadi Liakhovetski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2016-12-06 15:56 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Guennadi,
On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >>>>>> + /*
> >>>>>> + * Register a metadata node. TODO: shall this only be enabled
> >>>>>> for some
> >>>>>> + * cameras?
> >>>>>> + */
> >>>>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >>>>>> + uvc_meta_register(stream);
> >>>>>> +
> >>>>>
> >>>>> I think so, only for the cameras that can produce metadata.
> >>>>
> >>>> Every UVC camera produces metadata, but most cameras only have standard
> >>>> fields there. Whether we should stream standard header fields from the
> >>>> metadata node will be discussed later. If we do decide to stream
> >>>> standard header fields, then every USB camera gets metadata nodes. If
> >>>> we decide not to include standard fields, how do we know whether the
> >>>> camera has any private fields in headers without streaming from it? Do
> >>>> you want a quirk for such cameras?
> >>>
> >>> Unless they can be detected in a standard way that's probably the best
> >>> solution. Please remember that the UVC specification doesn't allow
> >>> vendors to extend headers in a vendor-specific way.
> >>
> >> Does it not? Where is that specified? I didn't find that anywhere.
> >
> > It's the other way around, it document a standard way to extend the
> > payload header. Any option you pick risks being incompatible with future
> > revisions of the specification. For instance the payload header's
> > bmHeaderInfo field can be extended through the use of the EOF bit, but a
> > future version of the specification could also extend it, in a way that
> > would make a vendor-specific extension be mistaken for the standard
> > extension.
> >
> > Now, you could add data after the standard payload without touching the
> > bmHeaderInfo field, but I'm still worried it could be broken by future
> > versions of the specification.
>
> Exactly - using "free" space in payload headers is not a part of the spec,
> but is also not prohibited by it. As for future versions - cameras specify
> which version of the spec they implement, and existing versions cannot
> change. If a camera decides to upgrade and in future UVC versions there
> won't be enough space left for the private data, only then the camera
> manufacturer will have a problem, that they will have to solve. The
> user-space software will have to know, that UVC 5.1 metadata has a
> different format, than UVC 1.5, that's true.
I agree that the specification doesn't explicitly forbid it, but it's a very
grey area. An extension mechanism should be standardized by the USB-IF UVC
working group. I'd propose it myself if they hadn't decided to kick me out
years ago :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-06 15:56 ` Laurent Pinchart
@ 2016-12-08 13:34 ` Guennadi Liakhovetski
2016-12-08 13:39 ` Laurent Pinchart
0 siblings, 1 reply; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-08 13:34 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Laurent,
One more question:
On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > > On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> > >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >>>>>> + /*
> > >>>>>> + * Register a metadata node. TODO: shall this only be enabled
> > >>>>>> for some
> > >>>>>> + * cameras?
> > >>>>>> + */
> > >>>>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >>>>>> + uvc_meta_register(stream);
> > >>>>>> +
> > >>>>>
> > >>>>> I think so, only for the cameras that can produce metadata.
> > >>>>
> > >>>> Every UVC camera produces metadata, but most cameras only have standard
> > >>>> fields there. Whether we should stream standard header fields from the
> > >>>> metadata node will be discussed later. If we do decide to stream
> > >>>> standard header fields, then every USB camera gets metadata nodes. If
> > >>>> we decide not to include standard fields, how do we know whether the
> > >>>> camera has any private fields in headers without streaming from it? Do
> > >>>> you want a quirk for such cameras?
> > >>>
> > >>> Unless they can be detected in a standard way that's probably the best
> > >>> solution.
How about a module parameter with a list of VID:PID pairs? The problem
with the quirk is, that as vendors produce multiple cameras with different
PIDs they will have to push patches for each such camera.
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-08 13:34 ` Guennadi Liakhovetski
@ 2016-12-08 13:39 ` Laurent Pinchart
2016-12-08 15:18 ` Guennadi Liakhovetski
0 siblings, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2016-12-08 13:39 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Guennadi,
On Thursday 08 Dec 2016 14:34:46 Guennadi Liakhovetski wrote:
> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >>> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> >>>> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> >>>>>>>> + /*
> >>>>>>>> + * Register a metadata node. TODO: shall this only be enabled
> >>>>>>>> for some
> >>>>>>>> + * cameras?
> >>>>>>>> + */
> >>>>>>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> >>>>>>>> + uvc_meta_register(stream);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> I think so, only for the cameras that can produce metadata.
> >>>>>>
> >>>>>> Every UVC camera produces metadata, but most cameras only have
> >>>>>> standard fields there. Whether we should stream standard header
> >>>>>> fields from the metadata node will be discussed later. If we do
> >>>>>> decide to stream standard header fields, then every USB camera gets
> >>>>>> metadata nodes. If we decide not to include standard fields, how do
> >>>>>> we know whether the camera has any private fields in headers
> >>>>>> without streaming from it? Do you want a quirk for such cameras?
> >>>>>
> >>>>> Unless they can be detected in a standard way that's probably the
> >>>>> best solution.
>
> How about a module parameter with a list of VID:PID pairs?
I'd like something that works out of the box for end-users, at least in most
cases. There's already a way to set quirks through a module parameter, and I
think I'd accept a patch extending that it make it VID:PID dependent. That's
an acceptable solution for testing, but should not be considered as the way to
go for production.
> The problem with the quirk is, that as vendors produce multiple cameras with
> different PIDs they will have to push patches for each such camera.
How many such devices do you expect ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
2016-12-08 13:39 ` Laurent Pinchart
@ 2016-12-08 15:18 ` Guennadi Liakhovetski
0 siblings, 0 replies; 18+ messages in thread
From: Guennadi Liakhovetski @ 2016-12-08 15:18 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Linux Media Mailing List, Hans Verkuil
On Thu, 8 Dec 2016, Laurent Pinchart wrote:
> Hi Guennadi,
>
> On Thursday 08 Dec 2016 14:34:46 Guennadi Liakhovetski wrote:
> > On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > > On Tuesday 06 Dec 2016 11:39:22 Guennadi Liakhovetski wrote:
> > >> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >>> On Monday 05 Dec 2016 23:13:53 Guennadi Liakhovetski wrote:
> > >>>> On Tue, 6 Dec 2016, Laurent Pinchart wrote:
> > >>>>>>>> + /*
> > >>>>>>>> + * Register a metadata node. TODO: shall this only be enabled
> > >>>>>>>> for some
> > >>>>>>>> + * cameras?
> > >>>>>>>> + */
> > >>>>>>>> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> > >>>>>>>> + uvc_meta_register(stream);
> > >>>>>>>> +
> > >>>>>>>
> > >>>>>>> I think so, only for the cameras that can produce metadata.
> > >>>>>>
> > >>>>>> Every UVC camera produces metadata, but most cameras only have
> > >>>>>> standard fields there. Whether we should stream standard header
> > >>>>>> fields from the metadata node will be discussed later. If we do
> > >>>>>> decide to stream standard header fields, then every USB camera gets
> > >>>>>> metadata nodes. If we decide not to include standard fields, how do
> > >>>>>> we know whether the camera has any private fields in headers
> > >>>>>> without streaming from it? Do you want a quirk for such cameras?
> > >>>>>
> > >>>>> Unless they can be detected in a standard way that's probably the
> > >>>>> best solution.
> >
> > How about a module parameter with a list of VID:PID pairs?
>
> I'd like something that works out of the box for end-users, at least in most
> cases. There's already a way to set quirks through a module parameter, and I
> think I'd accept a patch extending that it make it VID:PID dependent.
Ok, that helps already, sure.
> That's
> an acceptable solution for testing, but should not be considered as the way to
> go for production.
>
> > The problem with the quirk is, that as vendors produce multiple cameras with
> > different PIDs they will have to push patches for each such camera.
>
> How many such devices do you expect ?
No idea, significantly more than 2, let's say :) But well, you already can
count a few RealSense USB / UVC cameras on the market.
Concerning metadata for isochronous endpoints. I actually don't know what
to do with it. I don't have any such cameras with non-standard metadata.
For the standard metadata it would probably be enough to get either the
first or the last or the middle payload. Collecting all of them seems
redundant to me. Maybe I could for now only enable metadata nodes for bulk
endpoints. Would that be acceptable?
Thanks
Guennadi
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-12-08 15:18 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-24 11:28 [PATCH 0/3] uvcvideo: a cosmetic fix and 2 new features Guennadi Liakhovetski
2016-06-24 11:28 ` [PATCH 1/3] uvcvideo: initialise the entity function field Guennadi Liakhovetski
2016-06-24 13:26 ` Laurent Pinchart
2016-06-24 11:29 ` [PATCH 2/3] uvcvideo: send a control event when a Control Change interrupt arrives Guennadi Liakhovetski
2016-06-24 11:29 ` [PATCH 3/3] uvcvideo: add a metadata device node Guennadi Liakhovetski
2016-06-24 12:46 ` kbuild test robot
2016-06-24 13:12 ` kbuild test robot
2016-12-02 10:53 ` [PATCH v2 " Guennadi Liakhovetski
2016-12-05 10:53 ` Laurent Pinchart
2016-12-05 15:35 ` Guennadi Liakhovetski
2016-12-05 22:06 ` Laurent Pinchart
2016-12-05 22:13 ` Guennadi Liakhovetski
2016-12-05 22:25 ` Laurent Pinchart
2016-12-06 10:39 ` Guennadi Liakhovetski
2016-12-06 15:56 ` Laurent Pinchart
2016-12-08 13:34 ` Guennadi Liakhovetski
2016-12-08 13:39 ` Laurent Pinchart
2016-12-08 15:18 ` Guennadi Liakhovetski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.