Re: [PATCH v1 03/19] uvcvideo: Add support for multiple chains with common roots.
Hi,
I'm repeating myself, but this seems the proper place to post this.
This patch causes a regression with the logitech c930e.
Please see comments below:
2013/11/10 Laurent Pinchart laurent.pinch...@ideasonboard.com:
Hi Pawel,
Thank you for the patch.
On Friday 30 August 2013 11:17:02 Pawel Osciak wrote:
This adds support for pipelines that fork into branches consisting of more
than one entity, creating a chain for each fork and putting common entities
on all chains that share them.
This requires us to share the ctrl_mutex across forked chains. Whenever we
discover a shared part of a chain, we assign the pointer to an existing
mutex to the sharing chain instead of creating a new one.
The forward scan is not needed anymore, as after scanning back from OTs,
we go over all entities which are not on a path from an OT and accept
single-XU branches, adding them to the existing chains.
Also extract control locking into __uvc_ctrl_{lock,unlock} functions.
This is one core piece of the UVC 1.5 rework, and I have mixed feelings about
it.
Adding entities to multiple overlapping chains somehow doesn't feel right.
What would you think about using a pipeline object instead, which would store
all entities in the pipeline ?
The driver currently iterates over all entities in a chain for several
purposes:
- registering streaming terminals as video nodes (uvc_driver.c)
- registering MC entities (uvc_entity.c)
- enumerating inputs (uvc_v4l2.c)
- finding controls and extension units (uvc_ctrl.c)
The first two uses could easily be replaced by iterations over the whole
pipeline. Input enumeration would probably require a bit of custom code
anyway, so we would be left with controls.
At first sight it would make sense to expose on a video node only the controls
that can be reached from that video node moving up the pipeline (relatively to
the video flow). However, this might break existing applications, as the
driver currently also includes controls reacheable by forward scans of side
branches. We thus need to carefully think about what controls to include, and
we need to take into account output video nodes corresponding to input
streaming terminals.
Signed-off-by: Pawel Osciak posc...@chromium.org
---
drivers/media/usb/uvc/uvc_ctrl.c | 70 -
drivers/media/usb/uvc/uvc_driver.c | 210 +++--
drivers/media/usb/uvc/uvc_entity.c | 15 ++-
drivers/media/usb/uvc/uvc_v4l2.c | 9 +-
drivers/media/usb/uvc/uvcvideo.h | 20 +++-
5 files changed, 199 insertions(+), 125 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
b/drivers/media/usb/uvc/uvc_ctrl.c index a2f4501..ba159a4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -841,6 +841,7 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain, {
struct uvc_control *ctrl = NULL;
struct uvc_entity *entity;
+ struct uvc_chain_entry *entry;
int next = v4l2_id V4L2_CTRL_FLAG_NEXT_CTRL;
*mapping = NULL;
@@ -849,7 +850,8 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain, v4l2_id = V4L2_CTRL_ID_MASK;
/* Find the control. */
- list_for_each_entry(entity, chain-entities, chain) {
+ list_for_each_entry(entry, chain-entities, chain_entry) {
+ entity = entry-entity;
__uvc_find_control(entity, v4l2_id, mapping, ctrl, next);
if (ctrl !next)
return ctrl;
@@ -1048,6 +1050,16 @@ static int __uvc_query_v4l2_ctrl(struct
uvc_video_chain *chain, return 0;
}
+int __uvc_ctrl_lock(struct uvc_video_chain *chain)
+{
+ return mutex_lock_interruptible(chain-pipeline-ctrl_mutex) ?
+ -ERESTARTSYS : 0;
+}
+void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
+{
+ mutex_unlock(chain-pipeline-ctrl_mutex);
+}
+
int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct v4l2_queryctrl *v4l2_ctrl)
{
@@ -1055,9 +1067,9 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct uvc_control_mapping *mapping;
int ret;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(chain, v4l2_ctrl-id, mapping);
if (ctrl == NULL) {
@@ -1067,7 +1079,7 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
done:
- mutex_unlock(chain-ctrl_mutex);
+ __uvc_ctrl_unlock(chain);
return ret;
}
@@ -1094,9 +1106,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
query_menu-id = id;
query_menu-index = index;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+
Re: [PATCH v1 03/19] uvcvideo: Add support for multiple chains with common roots.
Hi Pawel,
Thank you for the patch.
On Friday 30 August 2013 11:17:02 Pawel Osciak wrote:
This adds support for pipelines that fork into branches consisting of more
than one entity, creating a chain for each fork and putting common entities
on all chains that share them.
This requires us to share the ctrl_mutex across forked chains. Whenever we
discover a shared part of a chain, we assign the pointer to an existing
mutex to the sharing chain instead of creating a new one.
The forward scan is not needed anymore, as after scanning back from OTs,
we go over all entities which are not on a path from an OT and accept
single-XU branches, adding them to the existing chains.
Also extract control locking into __uvc_ctrl_{lock,unlock} functions.
This is one core piece of the UVC 1.5 rework, and I have mixed feelings about
it.
Adding entities to multiple overlapping chains somehow doesn't feel right.
What would you think about using a pipeline object instead, which would store
all entities in the pipeline ?
The driver currently iterates over all entities in a chain for several
purposes:
- registering streaming terminals as video nodes (uvc_driver.c)
- registering MC entities (uvc_entity.c)
- enumerating inputs (uvc_v4l2.c)
- finding controls and extension units (uvc_ctrl.c)
The first two uses could easily be replaced by iterations over the whole
pipeline. Input enumeration would probably require a bit of custom code
anyway, so we would be left with controls.
At first sight it would make sense to expose on a video node only the controls
that can be reached from that video node moving up the pipeline (relatively to
the video flow). However, this might break existing applications, as the
driver currently also includes controls reacheable by forward scans of side
branches. We thus need to carefully think about what controls to include, and
we need to take into account output video nodes corresponding to input
streaming terminals.
Signed-off-by: Pawel Osciak posc...@chromium.org
---
drivers/media/usb/uvc/uvc_ctrl.c | 70 -
drivers/media/usb/uvc/uvc_driver.c | 210 +++--
drivers/media/usb/uvc/uvc_entity.c | 15 ++-
drivers/media/usb/uvc/uvc_v4l2.c | 9 +-
drivers/media/usb/uvc/uvcvideo.h | 20 +++-
5 files changed, 199 insertions(+), 125 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
b/drivers/media/usb/uvc/uvc_ctrl.c index a2f4501..ba159a4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -841,6 +841,7 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain, {
struct uvc_control *ctrl = NULL;
struct uvc_entity *entity;
+ struct uvc_chain_entry *entry;
int next = v4l2_id V4L2_CTRL_FLAG_NEXT_CTRL;
*mapping = NULL;
@@ -849,7 +850,8 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain, v4l2_id = V4L2_CTRL_ID_MASK;
/* Find the control. */
- list_for_each_entry(entity, chain-entities, chain) {
+ list_for_each_entry(entry, chain-entities, chain_entry) {
+ entity = entry-entity;
__uvc_find_control(entity, v4l2_id, mapping, ctrl, next);
if (ctrl !next)
return ctrl;
@@ -1048,6 +1050,16 @@ static int __uvc_query_v4l2_ctrl(struct
uvc_video_chain *chain, return 0;
}
+int __uvc_ctrl_lock(struct uvc_video_chain *chain)
+{
+ return mutex_lock_interruptible(chain-pipeline-ctrl_mutex) ?
+ -ERESTARTSYS : 0;
+}
+void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
+{
+ mutex_unlock(chain-pipeline-ctrl_mutex);
+}
+
int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct v4l2_queryctrl *v4l2_ctrl)
{
@@ -1055,9 +1067,9 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct uvc_control_mapping *mapping;
int ret;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(chain, v4l2_ctrl-id, mapping);
if (ctrl == NULL) {
@@ -1067,7 +1079,7 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
done:
- mutex_unlock(chain-ctrl_mutex);
+ __uvc_ctrl_unlock(chain);
return ret;
}
@@ -1094,9 +1106,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
query_menu-id = id;
query_menu-index = index;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(chain, query_menu-id, mapping);
if (ctrl == NULL || mapping-v4l2_type != V4L2_CTRL_TYPE_MENU) {
@@ -1132,7 +1144,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain
[PATCH v1 03/19] uvcvideo: Add support for multiple chains with common roots.
This adds support for pipelines that fork into branches consisting of more
than one entity, creating a chain for each fork and putting common entities
on all chains that share them.
This requires us to share the ctrl_mutex across forked chains. Whenever we
discover a shared part of a chain, we assign the pointer to an existing
mutex to the sharing chain instead of creating a new one.
The forward scan is not needed anymore, as after scanning back from OTs,
we go over all entities which are not on a path from an OT and accept
single-XU branches, adding them to the existing chains.
Also extract control locking into __uvc_ctrl_{lock,unlock} functions.
Signed-off-by: Pawel Osciak posc...@chromium.org
---
drivers/media/usb/uvc/uvc_ctrl.c | 70 -
drivers/media/usb/uvc/uvc_driver.c | 210 +
drivers/media/usb/uvc/uvc_entity.c | 15 ++-
drivers/media/usb/uvc/uvc_v4l2.c | 9 +-
drivers/media/usb/uvc/uvcvideo.h | 20 +++-
5 files changed, 199 insertions(+), 125 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a2f4501..ba159a4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -841,6 +841,7 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain,
{
struct uvc_control *ctrl = NULL;
struct uvc_entity *entity;
+ struct uvc_chain_entry *entry;
int next = v4l2_id V4L2_CTRL_FLAG_NEXT_CTRL;
*mapping = NULL;
@@ -849,7 +850,8 @@ static struct uvc_control *uvc_find_control(struct
uvc_video_chain *chain,
v4l2_id = V4L2_CTRL_ID_MASK;
/* Find the control. */
- list_for_each_entry(entity, chain-entities, chain) {
+ list_for_each_entry(entry, chain-entities, chain_entry) {
+ entity = entry-entity;
__uvc_find_control(entity, v4l2_id, mapping, ctrl, next);
if (ctrl !next)
return ctrl;
@@ -1048,6 +1050,16 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain
*chain,
return 0;
}
+int __uvc_ctrl_lock(struct uvc_video_chain *chain)
+{
+ return mutex_lock_interruptible(chain-pipeline-ctrl_mutex) ?
+ -ERESTARTSYS : 0;
+}
+void __uvc_ctrl_unlock(struct uvc_video_chain *chain)
+{
+ mutex_unlock(chain-pipeline-ctrl_mutex);
+}
+
int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct v4l2_queryctrl *v4l2_ctrl)
{
@@ -1055,9 +1067,9 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
struct uvc_control_mapping *mapping;
int ret;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(chain, v4l2_ctrl-id, mapping);
if (ctrl == NULL) {
@@ -1067,7 +1079,7 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
done:
- mutex_unlock(chain-ctrl_mutex);
+ __uvc_ctrl_unlock(chain);
return ret;
}
@@ -1094,9 +1106,9 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
query_menu-id = id;
query_menu-index = index;
- ret = mutex_lock_interruptible(chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(chain, query_menu-id, mapping);
if (ctrl == NULL || mapping-v4l2_type != V4L2_CTRL_TYPE_MENU) {
@@ -1132,7 +1144,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
strlcpy(query_menu-name, menu_info-name, sizeof query_menu-name);
done:
- mutex_unlock(chain-ctrl_mutex);
+ __uvc_ctrl_unlock(chain);
return ret;
}
@@ -1257,9 +1269,9 @@ static int uvc_ctrl_add_event(struct
v4l2_subscribed_event *sev, unsigned elems)
struct uvc_control *ctrl;
int ret;
- ret = mutex_lock_interruptible(handle-chain-ctrl_mutex);
+ ret = __uvc_ctrl_lock(handle-chain);
if (ret 0)
- return -ERESTARTSYS;
+ return ret;
ctrl = uvc_find_control(handle-chain, sev-id, mapping);
if (ctrl == NULL) {
@@ -1285,7 +1297,7 @@ static int uvc_ctrl_add_event(struct
v4l2_subscribed_event *sev, unsigned elems)
}
done:
- mutex_unlock(handle-chain-ctrl_mutex);
+ __uvc_ctrl_unlock(handle-chain);
return ret;
}
@@ -1293,9 +1305,9 @@ static void uvc_ctrl_del_event(struct
v4l2_subscribed_event *sev)
{
struct uvc_fh *handle = container_of(sev-fh, struct uvc_fh, vfh);
- mutex_lock(handle-chain-ctrl_mutex);
+ __uvc_ctrl_lock(handle-chain);
list_del(sev-node);
- mutex_unlock(handle-chain-ctrl_mutex);
+ __uvc_ctrl_unlock(handle-chain);
}
const struct v4l2_subscribed_event_ops
