Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1591273 > unrolled thread
| Started by | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| First post | 2017-03-02 17:50 +0100 |
| Last post | 2017-03-11 22:40 +0100 |
| Articles | 20 on this page of 52 — 6 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-02 17:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-03 01:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-03 02:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-03 03:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-03 20:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-03 23:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-04 00:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-04 01:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-04 14:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-10 14:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 14:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-10 14:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 15:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-10 15:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-10 17:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-10 23:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-11 12:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-11 23:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-12 00:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-12 01:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-12 22:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-12 23:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-13 13:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-10 16:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 17:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 18:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-10 21:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-10 23:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-10 16:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-11 12:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-11 14:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-11 16:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-11 18:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-11 19:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-11 19:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-11 20:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-11 20:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-11 20:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-11 21:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-12 04:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-12 08:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-12 19:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-12 23:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-13 11:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-13 12:00 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-13 12:10 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Mauro Carvalho Chehab <mchehab@s-opensource.com> - 2017-03-13 12:50 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-13 13:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-12 19:20 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-11 21:30 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-11 21:40 +0100
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline Pavel Machek <pavel@ucw.cz> - 2017-03-11 22:40 +0100
Page 1 of 3 [1] 2 3 Next page →
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-03-02 17:50 +0100 |
| Subject | Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline |
| Message-ID | <tgC5s-53e-15@gated-at.bofh.it> |
Hi Steve,
On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote:
> v4l2_pipeline_inherit_controls() will add the v4l2 controls from
> all subdev entities in a pipeline to a given video device.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++
> include/media/v4l2-mc.h | 25 ++++++++++++++++++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 303980b..09d4d97 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -22,6 +22,7 @@
> #include <linux/usb.h>
> #include <media/media-device.h>
> #include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
> #include <media/v4l2-fh.h>
> #include <media/v4l2-mc.h>
> #include <media/v4l2-subdev.h>
> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q)
> }
> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
>
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity)
I have a few concerns / questions:
- What's the purpose of this patch? Why not to access the sub-device node
directly?
- This implementation is only workable as long as you do not modify the
pipeline. Once you disable a link along the pipeline, a device where the
control was inherited from may no longer be a part of the pipeline.
Depending on the hardware, it could be a part of another pipeline, in
which case it certainly must not be accessible through an unrelated video
node. As the function is added to the framework, I would expect it to
handle such a case correctly.
- I assume it is the responsibility of the caller of this function to ensure
the device in question will not be powered off whilst the video node is
used as another user space interface to such a sub-device. If the driver
uses the generic PM functions in the same file, this works, but it still
has to be documented.
> +{
> + struct media_device *mdev = start_entity->graph_obj.mdev;
> + struct media_entity *entity;
> + struct media_graph graph;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + ret = media_graph_walk_init(&graph, mdev);
> + if (ret)
> + return ret;
> +
> + media_graph_walk_start(&graph, start_entity);
> +
> + while ((entity = media_graph_walk_next(&graph))) {
> + if (!is_media_entity_v4l2_subdev(entity))
> + continue;
> +
> + sd = media_entity_to_v4l2_subdev(entity);
> +
> + ret = v4l2_ctrl_add_handler(vfd->ctrl_handler,
> + sd->ctrl_handler,
> + NULL);
> + if (ret)
> + break;
> + }
> +
> + media_graph_walk_cleanup(&graph);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(__v4l2_pipeline_inherit_controls);
> +
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity)
> +{
> + struct media_device *mdev = start_entity->graph_obj.mdev;
> + int ret;
> +
> + mutex_lock(&mdev->graph_mutex);
> + ret = __v4l2_pipeline_inherit_controls(vfd, start_entity);
> + mutex_unlock(&mdev->graph_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_inherit_controls);
> +
> /* -----------------------------------------------------------------------------
> * Pipeline power management
> *
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 2634d9d..9848e77 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -171,6 +171,17 @@ void v4l_disable_media_source(struct video_device *vdev);
> */
> int v4l_vb2q_enable_media_source(struct vb2_queue *q);
>
> +/**
> + * v4l2_pipeline_inherit_controls - Add the v4l2 controls from all
> + * subdev entities in a pipeline to
> + * the given video device.
> + * @vfd: the video device
> + * @start_entity: Starting entity
> + */
> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity);
> +int v4l2_pipeline_inherit_controls(struct video_device *vfd,
> + struct media_entity *start_entity);
>
> /**
> * v4l2_pipeline_pm_use - Update the use count of an entity
> @@ -231,6 +242,20 @@ static inline int v4l_vb2q_enable_media_source(struct vb2_queue *q)
> return 0;
> }
>
> +static inline int __v4l2_pipeline_inherit_controls(
> + struct video_device *vfd,
> + struct media_entity *start_entity)
> +{
> + return 0;
> +}
> +
> +static inline int v4l2_pipeline_inherit_controls(
> + struct video_device *vfd,
> + struct media_entity *start_entity)
> +{
> + return 0;
> +}
> +
> static inline int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> {
> return 0;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-03 01:00 +0100 |
| Message-ID | <tgINA-13L-25@gated-at.bofh.it> |
| In reply to | #1591273 |
On 03/02/2017 08:02 AM, Sakari Ailus wrote: > Hi Steve, > > On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >> all subdev entities in a pipeline to a given video device. >> >> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >> --- >> drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++ >> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >> 2 files changed, 73 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c >> index 303980b..09d4d97 100644 >> --- a/drivers/media/v4l2-core/v4l2-mc.c >> +++ b/drivers/media/v4l2-core/v4l2-mc.c >> @@ -22,6 +22,7 @@ >> #include <linux/usb.h> >> #include <media/media-device.h> >> #include <media/media-entity.h> >> +#include <media/v4l2-ctrls.h> >> #include <media/v4l2-fh.h> >> #include <media/v4l2-mc.h> >> #include <media/v4l2-subdev.h> >> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q) >> } >> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >> >> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >> + struct media_entity *start_entity) > > I have a few concerns / questions: > > - What's the purpose of this patch? Why not to access the sub-device node > directly? I don't really understand what you are trying to say. That's exactly what this function is doing, accessing every subdevice in a pipeline directly, in one convenient function call. > > - This implementation is only workable as long as you do not modify the > pipeline. Once you disable a link along the pipeline, a device where the > control was inherited from may no longer be a part of the pipeline. That's correct. It's up to the media driver to clear the video device's inherited controls whenever the pipeline is modified, and then call this function again if need be. In imx-media driver, the function is called in link_setup when the link from a source pad that is attached to a capture video node is enabled. This is the last link that must be made to define the pipeline, so it is at this time that a complete list of subdevice controls can be gathered by walking the pipeline. > Depending on the hardware, it could be a part of another pipeline, in > which case it certainly must not be accessible through an unrelated video > node. As the function is added to the framework, I would expect it to > handle such a case correctly. The function will not inherit controls from a device that is not reachable from the given starting subdevice, so I don't understand you're point here. > > - I assume it is the responsibility of the caller of this function to ensure > the device in question will not be powered off whilst the video node is > used as another user space interface to such a sub-device. If the driver > uses the generic PM functions in the same file, this works, but it still > has to be documented. I guess I'm missing something. Why are you bringing up the subject of power? What does this function have to do with whether a subdevice is powered or not? The function makes use of v4l2_ctrl_add_handler(), and the latter has no requirements about whether the device's owning the control handlers are powered or not. Steve
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-03 02:00 +0100 |
| Message-ID | <tgJJD-1Lt-1@gated-at.bofh.it> |
| In reply to | #1591566 |
On 03/02/2017 03:48 PM, Steve Longerbeam wrote: > > > On 03/02/2017 08:02 AM, Sakari Ailus wrote: >> Hi Steve, >> >> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >>> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >>> all subdev entities in a pipeline to a given video device. >>> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>> --- >>> drivers/media/v4l2-core/v4l2-mc.c | 48 >>> +++++++++++++++++++++++++++++++++++++++ >>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >>> 2 files changed, 73 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-mc.c >>> b/drivers/media/v4l2-core/v4l2-mc.c >>> index 303980b..09d4d97 100644 >>> --- a/drivers/media/v4l2-core/v4l2-mc.c >>> +++ b/drivers/media/v4l2-core/v4l2-mc.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/usb.h> >>> #include <media/media-device.h> >>> #include <media/media-entity.h> >>> +#include <media/v4l2-ctrls.h> >>> #include <media/v4l2-fh.h> >>> #include <media/v4l2-mc.h> >>> #include <media/v4l2-subdev.h> >>> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct >>> vb2_queue *q) >>> } >>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >>> >>> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >>> + struct media_entity *start_entity) >> >> I have a few concerns / questions: >> >> - What's the purpose of this patch? Why not to access the sub-device node >> directly? > > > I don't really understand what you are trying to say. That's exactly > what this function is doing, accessing every subdevice in a pipeline > directly, in one convenient function call. > > >> >> - This implementation is only workable as long as you do not modify the >> pipeline. Once you disable a link along the pipeline, a device where >> the >> control was inherited from may no longer be a part of the pipeline. > > That's correct. It's up to the media driver to clear the video device's > inherited controls whenever the pipeline is modified, and then call this > function again if need be. And here is where I need to eat my words :). I'm not actually clearing the inherited controls if an upstream link from the device node link is modified after the whole pipeline has been configured. If the user does that the controls can become invalid. Need to fix that by clearing device node controls in the link_notify callback. Steve > > In imx-media driver, the function is called in link_setup when the link > from a source pad that is attached to a capture video node is enabled. > This is the last link that must be made to define the pipeline, so it > is at this time that a complete list of subdevice controls can be > gathered by walking the pipeline. >
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-03 03:30 +0100 |
| Message-ID | <tgL8J-2LO-9@gated-at.bofh.it> |
| In reply to | #1591566 |
On 03/02/2017 03:48 PM, Steve Longerbeam wrote: > > > On 03/02/2017 08:02 AM, Sakari Ailus wrote: >> Hi Steve, >> >> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >>> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >>> all subdev entities in a pipeline to a given video device. >>> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>> --- >>> drivers/media/v4l2-core/v4l2-mc.c | 48 >>> +++++++++++++++++++++++++++++++++++++++ >>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >>> 2 files changed, 73 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-mc.c >>> b/drivers/media/v4l2-core/v4l2-mc.c >>> index 303980b..09d4d97 100644 >>> --- a/drivers/media/v4l2-core/v4l2-mc.c >>> +++ b/drivers/media/v4l2-core/v4l2-mc.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/usb.h> >>> #include <media/media-device.h> >>> #include <media/media-entity.h> >>> +#include <media/v4l2-ctrls.h> >>> #include <media/v4l2-fh.h> >>> #include <media/v4l2-mc.h> >>> #include <media/v4l2-subdev.h> >>> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct >>> vb2_queue *q) >>> } >>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >>> >>> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >>> + struct media_entity *start_entity) >> >> I have a few concerns / questions: >> >> - What's the purpose of this patch? Why not to access the sub-device node >> directly? > > > I don't really understand what you are trying to say.<snip> > Actually I think I understand what you mean now. Yes, the user can always access a subdev's control directly from its /dev/v4l-subdevXX. I'm only providing this feature as a convenience to the user, so that all controls in a pipeline can be accessed from one place, i.e. the main capture device node. Steve
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-03-03 20:20 +0100 |
| Message-ID | <th0Ua-5DA-21@gated-at.bofh.it> |
| In reply to | #1591632 |
Hi Steve, On Thu, Mar 02, 2017 at 06:12:43PM -0800, Steve Longerbeam wrote: > > > On 03/02/2017 03:48 PM, Steve Longerbeam wrote: > > > > > >On 03/02/2017 08:02 AM, Sakari Ailus wrote: > >>Hi Steve, > >> > >>On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: > >>>v4l2_pipeline_inherit_controls() will add the v4l2 controls from > >>>all subdev entities in a pipeline to a given video device. > >>> > >>>Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> > >>>--- > >>> drivers/media/v4l2-core/v4l2-mc.c | 48 > >>>+++++++++++++++++++++++++++++++++++++++ > >>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ > >>> 2 files changed, 73 insertions(+) > >>> > >>>diff --git a/drivers/media/v4l2-core/v4l2-mc.c > >>>b/drivers/media/v4l2-core/v4l2-mc.c > >>>index 303980b..09d4d97 100644 > >>>--- a/drivers/media/v4l2-core/v4l2-mc.c > >>>+++ b/drivers/media/v4l2-core/v4l2-mc.c > >>>@@ -22,6 +22,7 @@ > >>> #include <linux/usb.h> > >>> #include <media/media-device.h> > >>> #include <media/media-entity.h> > >>>+#include <media/v4l2-ctrls.h> > >>> #include <media/v4l2-fh.h> > >>> #include <media/v4l2-mc.h> > >>> #include <media/v4l2-subdev.h> > >>>@@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct > >>>vb2_queue *q) > >>> } > >>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); > >>> > >>>+int __v4l2_pipeline_inherit_controls(struct video_device *vfd, > >>>+ struct media_entity *start_entity) > >> > >>I have a few concerns / questions: > >> > >>- What's the purpose of this patch? Why not to access the sub-device node > >> directly? > > > > > >I don't really understand what you are trying to say.<snip> > > > > Actually I think I understand what you mean now. Yes, the user can > always access a subdev's control directly from its /dev/v4l-subdevXX. > I'm only providing this feature as a convenience to the user, so that > all controls in a pipeline can be accessed from one place, i.e. the > main capture device node. No other MC based V4L2 driver does this. You'd be creating device specific behaviour that differs from what the rest of the drivers do. The purpose of MC is to provide the user with knowledge of what devices are there, and the V4L2 sub-devices interface is used to access them in this case. It does matter where a control is implemented, too. If the pipeline contains multiple sub-devices that implement the same control, only one of them may be accessed. The driver calling the function (or even less the function) would not know which one of them should be ignored. If you need such functionality, it should be implemented in the user space instead. -- Regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-03 23:50 +0100 |
| Message-ID | <th4bn-7P9-17@gated-at.bofh.it> |
| In reply to | #1592232 |
On 03/03/2017 11:17 AM, Sakari Ailus wrote: > Hi Steve, > > On Thu, Mar 02, 2017 at 06:12:43PM -0800, Steve Longerbeam wrote: >> >> >> On 03/02/2017 03:48 PM, Steve Longerbeam wrote: >>> >>> >>> On 03/02/2017 08:02 AM, Sakari Ailus wrote: >>>> Hi Steve, >>>> >>>> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >>>>> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >>>>> all subdev entities in a pipeline to a given video device. >>>>> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-mc.c | 48 >>>>> +++++++++++++++++++++++++++++++++++++++ >>>>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >>>>> 2 files changed, 73 insertions(+) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-mc.c >>>>> b/drivers/media/v4l2-core/v4l2-mc.c >>>>> index 303980b..09d4d97 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-mc.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-mc.c >>>>> @@ -22,6 +22,7 @@ >>>>> #include <linux/usb.h> >>>>> #include <media/media-device.h> >>>>> #include <media/media-entity.h> >>>>> +#include <media/v4l2-ctrls.h> >>>>> #include <media/v4l2-fh.h> >>>>> #include <media/v4l2-mc.h> >>>>> #include <media/v4l2-subdev.h> >>>>> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct >>>>> vb2_queue *q) >>>>> } >>>>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >>>>> >>>>> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >>>>> + struct media_entity *start_entity) >>>> >>>> I have a few concerns / questions: >>>> >>>> - What's the purpose of this patch? Why not to access the sub-device node >>>> directly? >>> >>> >>> I don't really understand what you are trying to say.<snip> >>> >> >> Actually I think I understand what you mean now. Yes, the user can >> always access a subdev's control directly from its /dev/v4l-subdevXX. >> I'm only providing this feature as a convenience to the user, so that >> all controls in a pipeline can be accessed from one place, i.e. the >> main capture device node. > > No other MC based V4L2 driver does this. You'd be creating device specific > behaviour that differs from what the rest of the drivers do. The purpose of > MC is to provide the user with knowledge of what devices are there, and the > V4L2 sub-devices interface is used to access them in this case. Well, again, I don't mind removing this. As I said it is only a convenience (although quite a nice one in my opinion). I'd like to hear from others whether this is worth keeping though. > > It does matter where a control is implemented, too. If the pipeline contains > multiple sub-devices that implement the same control, only one of them may > be accessed. The driver calling the function (or even less the function) > would not know which one of them should be ignored. Yes the pipeline should not have any duplicate controls. On imx-media no pipelines that can be configured have duplicate controls. Steve > > If you need such functionality, it should be implemented in the user space > instead. >
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-03-04 00:20 +0100 |
| Message-ID | <th4Ep-8eD-3@gated-at.bofh.it> |
| In reply to | #1591273 |
On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote:
> Hi Steve,
>
> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote:
> > v4l2_pipeline_inherit_controls() will add the v4l2 controls from
> > all subdev entities in a pipeline to a given video device.
> >
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > ---
> > drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++
> > include/media/v4l2-mc.h | 25 ++++++++++++++++++++
> > 2 files changed, 73 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > index 303980b..09d4d97 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -22,6 +22,7 @@
> > #include <linux/usb.h>
> > #include <media/media-device.h>
> > #include <media/media-entity.h>
> > +#include <media/v4l2-ctrls.h>
> > #include <media/v4l2-fh.h>
> > #include <media/v4l2-mc.h>
> > #include <media/v4l2-subdev.h>
> > @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q)
> > }
> > EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source);
> >
> > +int __v4l2_pipeline_inherit_controls(struct video_device *vfd,
> > + struct media_entity *start_entity)
>
> I have a few concerns / questions:
>
> - What's the purpose of this patch? Why not to access the sub-device node
> directly?
What tools are in existance _today_ to provide access to these controls
via the sub-device nodes?
v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools
which is capable of accessing the subdevices is media-ctl, and that only
provides functionality for configuring the pipeline.
So, pointing people at vapourware userspace is really quite rediculous.
The established way to control video capture is through the main video
capture device, not through the sub-devices. Yes, the controls are
exposed through sub-devices too, but that does not mean that is the
correct way to access them.
The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst)
even disagrees with your statements. That talks about control
inheritence from sub-devices to the main video device, and the core
v4l2 code provides _automatic_ support for this - see
v4l2_device_register_subdev():
/* This just returns 0 if either of the two args is NULL */
err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
which merges the subdev's controls into the main device's control
handler.
So, (a) I don't think Steve needs to add this code, and (b) I think
your statements about not inheriting controls goes against the
documentation and API compatibility with _existing_ applications,
and ultimately hurts the user experience, since there's nothing
existing today to support what you're suggesting in userspace.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-04 01:40 +0100 |
| Message-ID | <th5TP-zw-3@gated-at.bofh.it> |
| In reply to | #1592336 |
On 03/03/2017 03:06 PM, Russell King - ARM Linux wrote: > On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote: >> Hi Steve, >> >> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >>> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >>> all subdev entities in a pipeline to a given video device. >>> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>> --- >>> drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++ >>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >>> 2 files changed, 73 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c >>> index 303980b..09d4d97 100644 >>> --- a/drivers/media/v4l2-core/v4l2-mc.c >>> +++ b/drivers/media/v4l2-core/v4l2-mc.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/usb.h> >>> #include <media/media-device.h> >>> #include <media/media-entity.h> >>> +#include <media/v4l2-ctrls.h> >>> #include <media/v4l2-fh.h> >>> #include <media/v4l2-mc.h> >>> #include <media/v4l2-subdev.h> >>> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q) >>> } >>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >>> >>> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >>> + struct media_entity *start_entity) >> >> I have a few concerns / questions: >> >> - What's the purpose of this patch? Why not to access the sub-device node >> directly? > > What tools are in existance _today_ to provide access to these controls > via the sub-device nodes? > > v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools > which is capable of accessing the subdevices is media-ctl, and that only > provides functionality for configuring the pipeline. > > So, pointing people at vapourware userspace is really quite rediculous. Hi Russell, Yes, that's a big reason why I added this capability. The v4l2-ctl tool won't accept subdev nodes, although Philipp Zabel has a quick hack to get around this (ignore return code from VIDIOC_QUERYCAP). > > The established way to control video capture is through the main video > capture device, not through the sub-devices. Yes, the controls are > exposed through sub-devices too, but that does not mean that is the > correct way to access them. > > The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst) > even disagrees with your statements. That talks about control > inheritence from sub-devices to the main video device, and the core > v4l2 code provides _automatic_ support for this - see > v4l2_device_register_subdev(): > > /* This just returns 0 if either of the two args is NULL */ > err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL); > > which merges the subdev's controls into the main device's control > handler. Actually v4l2_dev->ctrl_handler is not of much use to me. This will compose a list of controls from all registered subdevs, i.e. _all possible controls_. What v4l2_pipeline_inherit_controls() does is compose a list of controls that are reachable and available in the currently configured pipeline. Steve > > So, (a) I don't think Steve needs to add this code, and (b) I think > your statements about not inheriting controls goes against the > documentation and API compatibility with _existing_ applications, > and ultimately hurts the user experience, since there's nothing > existing today to support what you're suggesting in userspace. >
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-03-04 14:20 +0100 |
| Message-ID | <thhLk-18A-3@gated-at.bofh.it> |
| In reply to | #1592336 |
Hi Russell, On Fri, Mar 03, 2017 at 11:06:45PM +0000, Russell King - ARM Linux wrote: > On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote: > > Hi Steve, > > > > On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: > > > v4l2_pipeline_inherit_controls() will add the v4l2 controls from > > > all subdev entities in a pipeline to a given video device. > > > > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> > > > --- > > > drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++ > > > include/media/v4l2-mc.h | 25 ++++++++++++++++++++ > > > 2 files changed, 73 insertions(+) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c > > > index 303980b..09d4d97 100644 > > > --- a/drivers/media/v4l2-core/v4l2-mc.c > > > +++ b/drivers/media/v4l2-core/v4l2-mc.c > > > @@ -22,6 +22,7 @@ > > > #include <linux/usb.h> > > > #include <media/media-device.h> > > > #include <media/media-entity.h> > > > +#include <media/v4l2-ctrls.h> > > > #include <media/v4l2-fh.h> > > > #include <media/v4l2-mc.h> > > > #include <media/v4l2-subdev.h> > > > @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q) > > > } > > > EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); > > > > > > +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, > > > + struct media_entity *start_entity) > > > > I have a few concerns / questions: > > > > - What's the purpose of this patch? Why not to access the sub-device node > > directly? > > What tools are in existance _today_ to provide access to these controls > via the sub-device nodes? yavta, for instance: <URL:http://git.ideasonboard.org/yavta.git> VIDIOC_QUERYCAP isn't supported on sub-devices and v4l2-ctl appears to be checking for that. That check should be removed (with possible other implications taken into account). > > v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools > which is capable of accessing the subdevices is media-ctl, and that only > provides functionality for configuring the pipeline. > > So, pointing people at vapourware userspace is really quite rediculous. Do bear in mind that there are other programs that can make use of these interfaces. It's not just the test programs, or a test program you attempted to use. > > The established way to control video capture is through the main video > capture device, not through the sub-devices. Yes, the controls are > exposed through sub-devices too, but that does not mean that is the > correct way to access them. It is. That's the very purpose of the sub-devices: to provide access to the hardware independently of how the links are configured. > > The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst) > even disagrees with your statements. That talks about control > inheritence from sub-devices to the main video device, and the core > v4l2 code provides _automatic_ support for this - see > v4l2_device_register_subdev(): > > /* This just returns 0 if either of the two args is NULL */ > err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL); > > which merges the subdev's controls into the main device's control > handler. That's done on different kind of devices: those that provide plain V4L2 API to control the entire device. V4L2 sub-device interface is used *in kernel* as an interface to control sub-devices that do not need to be exposed to the user space. Devices that have complex pipeline that do essentially require using the Media controller interface to configure them are out of that scope. -- Regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Hans Verkuil <hverkuil@xs4all.nl> |
|---|---|
| Date | 2017-03-10 14:00 +0100 |
| Message-ID | <tjsjh-49K-49@gated-at.bofh.it> |
| In reply to | #1592494 |
On 04/03/17 14:13, Sakari Ailus wrote: > Hi Russell, > > On Fri, Mar 03, 2017 at 11:06:45PM +0000, Russell King - ARM Linux wrote: >> On Thu, Mar 02, 2017 at 06:02:57PM +0200, Sakari Ailus wrote: >>> Hi Steve, >>> >>> On Wed, Feb 15, 2017 at 06:19:16PM -0800, Steve Longerbeam wrote: >>>> v4l2_pipeline_inherit_controls() will add the v4l2 controls from >>>> all subdev entities in a pipeline to a given video device. >>>> >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-mc.c | 48 +++++++++++++++++++++++++++++++++++++++ >>>> include/media/v4l2-mc.h | 25 ++++++++++++++++++++ >>>> 2 files changed, 73 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c >>>> index 303980b..09d4d97 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-mc.c >>>> +++ b/drivers/media/v4l2-core/v4l2-mc.c >>>> @@ -22,6 +22,7 @@ >>>> #include <linux/usb.h> >>>> #include <media/media-device.h> >>>> #include <media/media-entity.h> >>>> +#include <media/v4l2-ctrls.h> >>>> #include <media/v4l2-fh.h> >>>> #include <media/v4l2-mc.h> >>>> #include <media/v4l2-subdev.h> >>>> @@ -238,6 +239,53 @@ int v4l_vb2q_enable_media_source(struct vb2_queue *q) >>>> } >>>> EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_source); >>>> >>>> +int __v4l2_pipeline_inherit_controls(struct video_device *vfd, >>>> + struct media_entity *start_entity) >>> >>> I have a few concerns / questions: >>> >>> - What's the purpose of this patch? Why not to access the sub-device node >>> directly? >> >> What tools are in existance _today_ to provide access to these controls >> via the sub-device nodes? > > yavta, for instance: > > <URL:http://git.ideasonboard.org/yavta.git> > > VIDIOC_QUERYCAP isn't supported on sub-devices and v4l2-ctl appears to be > checking for that. That check should be removed (with possible other > implications taken into account). No, the subdev API should get a similar QUERYCAP ioctl. There isn't a single ioctl that is guaranteed to be available for all subdev devices. I've made proposals for this in the past, and those have all been shot down. Add that, and I'll add support for subdevs in v4l2-ctl. > >> >> v4l-tools doesn't last time I looked - in fact, the only tool in v4l-tools >> which is capable of accessing the subdevices is media-ctl, and that only >> provides functionality for configuring the pipeline. >> >> So, pointing people at vapourware userspace is really quite rediculous. > > Do bear in mind that there are other programs that can make use of these > interfaces. It's not just the test programs, or a test program you attempted > to use. > >> >> The established way to control video capture is through the main video >> capture device, not through the sub-devices. Yes, the controls are >> exposed through sub-devices too, but that does not mean that is the >> correct way to access them. > > It is. That's the very purpose of the sub-devices: to provide access to the > hardware independently of how the links are configured. > >> >> The v4l2 documentation (Documentation/media/kapi/v4l2-controls.rst) >> even disagrees with your statements. That talks about control >> inheritence from sub-devices to the main video device, and the core >> v4l2 code provides _automatic_ support for this - see >> v4l2_device_register_subdev(): >> >> /* This just returns 0 if either of the two args is NULL */ >> err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL); >> >> which merges the subdev's controls into the main device's control >> handler. > > That's done on different kind of devices: those that provide plain V4L2 API > to control the entire device. V4L2 sub-device interface is used *in kernel* > as an interface to control sub-devices that do not need to be exposed to the > user space. > > Devices that have complex pipeline that do essentially require using the > Media controller interface to configure them are out of that scope. > Way too much of how the MC devices should be used is in the minds of developers. There is a major lack for good detailed documentation, utilities, compliance test (really needed!) and libv4l plugins. Russell's comments are spot on and it is a thorn in my side that this still hasn't been addressed. I want to see if I can get time from my boss to work on this this summer, but there is no guarantee. The main reason this hasn't been a much bigger problem is that most end-users make custom applications for this hardware. It makes sense, if you need full control over everything you make the application yourself, that's the whole point. But there was always meant to be a layer (libv4l plugin) that could be used to setup a 'default scenario' that existing applications could use, but that was never enforced, sadly. Anyway, regarding this specific patch and for this MC-aware driver: no, you shouldn't inherit controls from subdevs. It defeats the purpose. Regards, Hans
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-03-10 14:10 +0100 |
| Message-ID | <tjssW-4sG-19@gated-at.bofh.it> |
| In reply to | #1597464 |
On Fri, Mar 10, 2017 at 01:54:28PM +0100, Hans Verkuil wrote: > But there was always meant to be a layer (libv4l plugin) that could be > used to setup a 'default scenario' that existing applications could use, > but that was never enforced, sadly. However, there's other painful issues lurking in userspace, particularly to do with the v4l libraries. The idea that the v4l libraries should intercept the format negotiation between the application and kernel is a particularly painful one - the default gstreamer build detects the v4l libraries, and links against it. That much is fine. However, the problem comes when you're trying to use bayer formats. The v4l libraries "helpfully" (or rather unhelpfully) intercept the format negotiation, and decide that they'll invoke v4lconvert to convert the bayer to RGB for you, whether you want them to do that or not. v4lconvert may not be the most efficient way to convert, or even what is desired (eg, you may want to receive the raw bayer image.) However, since the v4l libraries/v4lconvert gives you no option but to have its conversion forced into the pipeline, other options (such as using the gstreamer neon accelerated de-bayer plugin) isn't an option without rebuilding gstreamer _without_ linking against the v4l libraries. At that point, saying "this should be done in a libv4l plugin" becomes a total nonsense, because if you need to avoid libv4l due to its stupidities, you don't get the benefit of subdevs, and it yet again _forces_ people down the route of custom applications. So, I really don't agree with pushing this into a userspace library plugin - at least not with the current state there. _At least_ the debayering in the v4l libraries needs to become optional. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
[toc] | [prev] | [next] | [standalone]
| From | Hans Verkuil <hverkuil@xs4all.nl> |
|---|---|
| Date | 2017-03-10 14:30 +0100 |
| Message-ID | <tjsMh-4Ar-3@gated-at.bofh.it> |
| In reply to | #1597489 |
On 10/03/17 14:07, Russell King - ARM Linux wrote: > On Fri, Mar 10, 2017 at 01:54:28PM +0100, Hans Verkuil wrote: >> But there was always meant to be a layer (libv4l plugin) that could be >> used to setup a 'default scenario' that existing applications could use, >> but that was never enforced, sadly. > > However, there's other painful issues lurking in userspace, particularly > to do with the v4l libraries. > > The idea that the v4l libraries should intercept the format negotiation > between the application and kernel is a particularly painful one - the > default gstreamer build detects the v4l libraries, and links against it. > That much is fine. > > However, the problem comes when you're trying to use bayer formats. The > v4l libraries "helpfully" (or rather unhelpfully) intercept the format > negotiation, and decide that they'll invoke v4lconvert to convert the > bayer to RGB for you, whether you want them to do that or not. > > v4lconvert may not be the most efficient way to convert, or even what > is desired (eg, you may want to receive the raw bayer image.) However, > since the v4l libraries/v4lconvert gives you no option but to have its > conversion forced into the pipeline, other options (such as using the > gstreamer neon accelerated de-bayer plugin) isn't an option without > rebuilding gstreamer _without_ linking against the v4l libraries. > > At that point, saying "this should be done in a libv4l plugin" becomes > a total nonsense, because if you need to avoid libv4l due to its > stupidities, you don't get the benefit of subdevs, and it yet again > _forces_ people down the route of custom applications. > > So, I really don't agree with pushing this into a userspace library > plugin - at least not with the current state there. > > _At least_ the debayering in the v4l libraries needs to become optional. > I *thought* that when a plugin is used the format conversion code was disabled. But I'm not sure. The whole problem is that we still don't have a decent plugin for an MC driver. There is one for the exynos4 floating around, but it's still not accepted. Companies write the driver, but the plugin isn't really needed since their customers won't use it anyway since they make their own embedded driver. And nobody of the media core developers has the time to work on the docs, utilities and libraries you need to make this all work cleanly and reliably. As mentioned, I will attempt to try and get some time to work on this later this year. Fingers crossed. We also have a virtual MC driver floating around. I've pinged the author if she can fix the last round of review comments and post a new version. Having a virtual driver makes life much easier when writing docs, utilities, etc. since you don't need real hardware which can be hard to obtain and run. Again, I agree completely with you. But we don't have many core developers who can do something like this, and it's even harder for them to find the time. Solutions on a postcard... BTW, Steve: this has nothing to do with your work, it's a problem in our subsystem. Regards, Hans
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-03-10 15:10 +0100 |
| Message-ID | <tjtp1-56o-49@gated-at.bofh.it> |
| In reply to | #1597564 |
On Fri, Mar 10, 2017 at 02:22:29PM +0100, Hans Verkuil wrote:
> And nobody of the media core developers has the time to work on the docs,
> utilities and libraries you need to make this all work cleanly and reliably.
Well, talking about docs, and in connection to control inheritence,
this is already documented in at least three separate places:
Documentation/media/uapi/v4l/dev-subdev.rst:
Controls
========
...
Depending on the driver, those controls might also be exposed through
one (or several) V4L2 device nodes.
Documentation/media/kapi/v4l2-subdev.rst:
``VIDIOC_QUERYCTRL``,
``VIDIOC_QUERYMENU``,
``VIDIOC_G_CTRL``,
``VIDIOC_S_CTRL``,
``VIDIOC_G_EXT_CTRLS``,
``VIDIOC_S_EXT_CTRLS`` and
``VIDIOC_TRY_EXT_CTRLS``:
The controls ioctls are identical to the ones defined in V4L2. They
behave identically, with the only exception that they deal only with
controls implemented in the sub-device. Depending on the driver, those
controls can be also be accessed through one (or several) V4L2 device
nodes.
Then there's Documentation/media/kapi/v4l2-controls.rst, which gives a
step by step approach to the main video device inheriting controls from
its subdevices, and it says:
Inheriting Controls
-------------------
When a sub-device is registered with a V4L2 driver by calling
v4l2_device_register_subdev() and the ctrl_handler fields of both v4l2_subdev
and v4l2_device are set, then the controls of the subdev will become
automatically available in the V4L2 driver as well. If the subdev driver
contains controls that already exist in the V4L2 driver, then those will be
skipped (so a V4L2 driver can always override a subdev control).
What happens here is that v4l2_device_register_subdev() calls
v4l2_ctrl_add_handler() adding the controls of the subdev to the controls
of v4l2_device.
So, either the docs are wrong, or the advice being mentioned in emails
about subdev control inheritence is misleading. Whatever, the two are
currently inconsistent.
As I've already mentioned, from talking about this with Mauro, it seems
Mauro is in agreement with permitting the control inheritence... I wish
Mauro would comment for himself, as I can't quote our private discussion
on the subject.
Right now, my view is that v4l2 is currently being screwed up by people
with different opinions - there is no unified concensus on how any of
this stuff is supposed to work, everyone is pulling in different
directions. That needs solving _really_ quickly, so I suggest that
v4l2 people urgently talk to each other and thrash out some of the
issues that Steve's patch set has brought up, and settle on a way
forward, rather than what is seemingly happening today - which is
everyone working in isolation of everyone else with their own bias on
how things should be done.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
[toc] | [prev] | [next] | [standalone]
| From | Hans Verkuil <hverkuil@xs4all.nl> |
|---|---|
| Date | 2017-03-10 15:30 +0100 |
| Message-ID | <tjtIn-5ge-47@gated-at.bofh.it> |
| In reply to | #1597747 |
On 10/03/17 15:01, Russell King - ARM Linux wrote: > On Fri, Mar 10, 2017 at 02:22:29PM +0100, Hans Verkuil wrote: >> And nobody of the media core developers has the time to work on the docs, >> utilities and libraries you need to make this all work cleanly and reliably. > > Well, talking about docs, and in connection to control inheritence, > this is already documented in at least three separate places: > > Documentation/media/uapi/v4l/dev-subdev.rst: > > Controls > ======== > ... > Depending on the driver, those controls might also be exposed through > one (or several) V4L2 device nodes. > > Documentation/media/kapi/v4l2-subdev.rst: > > ``VIDIOC_QUERYCTRL``, > ``VIDIOC_QUERYMENU``, > ``VIDIOC_G_CTRL``, > ``VIDIOC_S_CTRL``, > ``VIDIOC_G_EXT_CTRLS``, > ``VIDIOC_S_EXT_CTRLS`` and > ``VIDIOC_TRY_EXT_CTRLS``: > > The controls ioctls are identical to the ones defined in V4L2. They > behave identically, with the only exception that they deal only with > controls implemented in the sub-device. Depending on the driver, those > controls can be also be accessed through one (or several) V4L2 device > nodes. > > Then there's Documentation/media/kapi/v4l2-controls.rst, which gives a > step by step approach to the main video device inheriting controls from > its subdevices, and it says: > > Inheriting Controls > ------------------- > > When a sub-device is registered with a V4L2 driver by calling > v4l2_device_register_subdev() and the ctrl_handler fields of both v4l2_subdev > and v4l2_device are set, then the controls of the subdev will become > automatically available in the V4L2 driver as well. If the subdev driver > contains controls that already exist in the V4L2 driver, then those will be > skipped (so a V4L2 driver can always override a subdev control). > > What happens here is that v4l2_device_register_subdev() calls > v4l2_ctrl_add_handler() adding the controls of the subdev to the controls > of v4l2_device. > > So, either the docs are wrong, or the advice being mentioned in emails > about subdev control inheritence is misleading. Whatever, the two are > currently inconsistent. These docs were written for non-MC drivers, and for those the documentation is correct. Unfortunately, this was never updated for MC drivers. > As I've already mentioned, from talking about this with Mauro, it seems > Mauro is in agreement with permitting the control inheritence... I wish > Mauro would comment for himself, as I can't quote our private discussion > on the subject. I can't comment either, not having seen his mail and reasoning. > Right now, my view is that v4l2 is currently being screwed up by people > with different opinions - there is no unified concensus on how any of > this stuff is supposed to work, everyone is pulling in different > directions. That needs solving _really_ quickly, so I suggest that > v4l2 people urgently talk to each other and thrash out some of the > issues that Steve's patch set has brought up, and settle on a way > forward, rather than what is seemingly happening today - which is > everyone working in isolation of everyone else with their own bias on > how things should be done. The simple fact is that to my knowledge no other MC applications inherit controls from subdevs. Suddenly doing something different here seems very wrong to me and needs very good reasons. But yes, the current situation sucks. Yelling doesn't help though if nobody has time and there are several other high-prio projects that need our attention as well. If you know a good kernel developer who has a few months to spare, please point him/her in our direction! Regards, Hans
[toc] | [prev] | [next] | [standalone]
| From | Mauro Carvalho Chehab <mchehab@s-opensource.com> |
|---|---|
| Date | 2017-03-10 17:00 +0100 |
| Message-ID | <tjv7r-66F-13@gated-at.bofh.it> |
| In reply to | #1597832 |
Em Fri, 10 Mar 2017 15:20:48 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > As I've already mentioned, from talking about this with Mauro, it seems > > Mauro is in agreement with permitting the control inheritence... I wish > > Mauro would comment for himself, as I can't quote our private discussion > > on the subject. > > I can't comment either, not having seen his mail and reasoning. The rationale is that we should support the simplest use cases first. In the case of the first MC-based driver (and several subsequent ones), the simplest use case required MC, as it was meant to suport a custom-made sophisticated application that required fine control on each component of the pipeline and to allow their advanced proprietary AAA userspace-based algorithms to work. That's not true, for example, for the UVC driver. There, MC is optional, as it should be. > > Right now, my view is that v4l2 is currently being screwed up by people > > with different opinions - there is no unified concensus on how any of > > this stuff is supposed to work, everyone is pulling in different > > directions. That needs solving _really_ quickly, so I suggest that > > v4l2 people urgently talk to each other and thrash out some of the > > issues that Steve's patch set has brought up, and settle on a way > > forward, rather than what is seemingly happening today - which is > > everyone working in isolation of everyone else with their own bias on > > how things should be done. > > The simple fact is that to my knowledge no other MC applications inherit > controls from subdevs. Suddenly doing something different here seems very > wrong to me and needs very good reasons. That's because it was not needed before, as other subdev-based drivers are meant to be used only on complex scenarios with custom-made apps. Thanks, Mauro
[toc] | [prev] | [next] | [standalone]
| From | Sakari Ailus <sakari.ailus@iki.fi> |
|---|---|
| Date | 2017-03-10 23:40 +0100 |
| Message-ID | <tjBmy-1Vq-13@gated-at.bofh.it> |
| In reply to | #1597938 |
Hi Mauro (and others), On Fri, Mar 10, 2017 at 12:53:42PM -0300, Mauro Carvalho Chehab wrote: > Em Fri, 10 Mar 2017 15:20:48 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > > > > As I've already mentioned, from talking about this with Mauro, it seems > > > Mauro is in agreement with permitting the control inheritence... I wish > > > Mauro would comment for himself, as I can't quote our private discussion > > > on the subject. > > > > I can't comment either, not having seen his mail and reasoning. > > The rationale is that we should support the simplest use cases first. > > In the case of the first MC-based driver (and several subsequent > ones), the simplest use case required MC, as it was meant to suport > a custom-made sophisticated application that required fine control > on each component of the pipeline and to allow their advanced > proprietary AAA userspace-based algorithms to work. The first MC based driver (omap3isp) supports what the hardware can do, it does not support applications as such. Adding support to drivers for different "operation modes" --- this is essentially what is being asked for --- is not an approach which could serve either purpose (some functionality with simple interface vs. fully support what the hardware can do, with interfaces allowing that) adequately in the short or the long run. If we are missing pieces in the puzzle --- in this case the missing pieces in the puzzle are a generic pipeline configuration library and another library that, with the help of pipeline autoconfiguration would implement "best effort" service for regular V4L2 on top of the MC + V4L2 subdev + V4L2 --- then these pieces need to be impelemented. The solution is *not* to attempt to support different types of applications in each driver separately. That will make writing drivers painful, error prone and is unlikely ever deliver what either purpose requires. So let's continue to implement the functionality that the hardware supports. Making a different choice here is bound to create a lasting conflict between having to change kernel interface behaviour and the requirement of supporting new functionality that hasn't been previously thought of, pushing away SoC vendors from V4L2 ecosystem. This is what we all do want to avoid. As far as i.MX6 driver goes, it is always possible to implement i.MX6 plugin for libv4l to perform this. This should be much easier than getting the automatic pipe configuration library and the rest working, and as it is custom for i.MX6, the resulting plugin may make informed technical choices for better functionality. Jacek has been working on such a plugin for Samsung Exynos hardware, but I don't think he has quite finished it yey. The original plan was and continues to be sound, it's just that there have always been too few hands to implement it. :-( > > That's not true, for example, for the UVC driver. There, MC > is optional, as it should be. UVC is different. The device simply provides additional information through MC to the user but MC (or V4L2 sub-device interface) is not used for controlling the device. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
[toc] | [prev] | [next] | [standalone]
| From | Mauro Carvalho Chehab <mchehab@s-opensource.com> |
|---|---|
| Date | 2017-03-11 12:30 +0100 |
| Message-ID | <tjNnH-1ZO-5@gated-at.bofh.it> |
| In reply to | #1598150 |
Em Sat, 11 Mar 2017 00:37:14 +0200 Sakari Ailus <sakari.ailus@iki.fi> escreveu: > Hi Mauro (and others), > > On Fri, Mar 10, 2017 at 12:53:42PM -0300, Mauro Carvalho Chehab wrote: > > Em Fri, 10 Mar 2017 15:20:48 +0100 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > > > > > > > As I've already mentioned, from talking about this with Mauro, it seems > > > > Mauro is in agreement with permitting the control inheritence... I wish > > > > Mauro would comment for himself, as I can't quote our private discussion > > > > on the subject. > > > > > > I can't comment either, not having seen his mail and reasoning. > > > > The rationale is that we should support the simplest use cases first. > > > > In the case of the first MC-based driver (and several subsequent > > ones), the simplest use case required MC, as it was meant to suport > > a custom-made sophisticated application that required fine control > > on each component of the pipeline and to allow their advanced > > proprietary AAA userspace-based algorithms to work. > > The first MC based driver (omap3isp) supports what the hardware can do, it > does not support applications as such. All media drivers support a subset of what the hardware can do. The question is if such subset covers the use cases or not. The current MC-based drivers (except for uvc) took a patch to offer a more advanced API, to allow direct control to each IP module, as it was said, by the time we merged the OMAP3 driver, that, for the N9/N900 camera to work, it was mandatory to access the pipeline's individual components. Such approach require that some userspace software will have knowledge about some hardware details, in order to setup pipelines and send controls to the right components. That makes really hard to have a generic user friendly application to use such devices. Non-MC based drivers control the hardware via a portable interface with doesn't require any knowledge about the hardware specifics, as either the Kernel or some firmware at the device will set any needed pipelines. In the case of V4L2 controls, when there's no subdev API, the main driver (e. g. the driver that creates the /dev/video nodes) sends a multicast message to all bound I2C drivers. The driver(s) that need them handle it. When the same control may be implemented on different drivers, the main driver sends a unicast message to just one driver[1]. [1] There are several non-MC drivers that have multiple ways to control some things, like doing scaling or adjust volume levels at either the bridge driver or at a subdriver. There's nothing wrong with this approach: it works, it is simpler, it is generic. So, if it covers most use cases, why not allowing it for usecases where a finer control is not a requirement? > Adding support to drivers for different "operation modes" --- this is > essentially what is being asked for --- is not an approach which could serve > either purpose (some functionality with simple interface vs. fully support > what the hardware can do, with interfaces allowing that) adequately in the > short or the long run. Why not? > If we are missing pieces in the puzzle --- in this case the missing pieces > in the puzzle are a generic pipeline configuration library and another > library that, with the help of pipeline autoconfiguration would implement > "best effort" service for regular V4L2 on top of the MC + V4L2 subdev + V4L2 > --- then these pieces need to be impelemented. The solution is > *not* to attempt to support different types of applications in each driver > separately. That will make writing drivers painful, error prone and is > unlikely ever deliver what either purpose requires. > > So let's continue to implement the functionality that the hardware supports. > Making a different choice here is bound to create a lasting conflict between > having to change kernel interface behaviour and the requirement of > supporting new functionality that hasn't been previously thought of, pushing > away SoC vendors from V4L2 ecosystem. This is what we all do want to avoid. This situation is there since 2009. If I remember well, you tried to write such generic plugin in the past, but never finished it, apparently because it is too complex. Others tried too over the years. The last trial was done by Jacek, trying to cover just the exynos4 driver. Yet, even such limited scope plugin was not good enough, as it was never merged upstream. Currently, there's no such plugins upstream. If we can't even merge a plugin that solves it for just *one* driver, I have no hope that we'll be able to do it for the generic case. That's why I'm saying that I'm OK on merging any patch that would allow setting controls via the /dev/video interface on MC-based drivers when compiled without subdev API. I may also consider merging patches allowing to change the behavior on runtime, when compiled with subdev API. > As far as i.MX6 driver goes, it is always possible to implement i.MX6 plugin > for libv4l to perform this. This should be much easier than getting the > automatic pipe configuration library and the rest working, and as it is > custom for i.MX6, the resulting plugin may make informed technical choices > for better functionality. I wouldn't call "much easier" something that experienced media developers failed to do over the last 8 years. It is just the opposite: broadcasting a control via I2C is very easy: there are several examples about how to do that all over the media drivers. > Jacek has been working on such a plugin for > Samsung Exynos hardware, but I don't think he has quite finished it yey. As Jacek answered when questioned about the merge status: Hi Hans, On 11/03/2016 12:51 PM, Hans Verkuil wrote: > Hi all, > > Is there anything that blocks me from merging this? > > This plugin work has been ongoing for years and unless there are serious > objections I propose that this is merged. > > Jacek, is there anything missing that would prevent merging this? There were issues raised by Sakari during last review, related to the way how v4l2 control bindings are defined. That discussion wasn't finished, so I stayed by my approach. Other than that - I've tested it and it works fine both with GStreamer and my test app. After that, he sent a new version (v7.1), but never got reviews. > The original plan was and continues to be sound, it's just that there have > always been too few hands to implement it. :-( If there are no people to implement a plan, it doesn't matter how good the plan is, it won't work. > > That's not true, for example, for the UVC driver. There, MC > > is optional, as it should be. > > UVC is different. The device simply provides additional information through > MC to the user but MC (or V4L2 sub-device interface) is not used for > controlling the device. It is not different. If the Kernel is compiled without the V4L2 subdev interface, the i.MX6 driver (or whatever other driver) won't receive any control via the subdev interface. So, it has to handle the control logic control via the only interface that supports it, e. g. via the video devnode. Thanks, Mauro
[toc] | [prev] | [next] | [standalone]
| From | Pavel Machek <pavel@ucw.cz> |
|---|---|
| Date | 2017-03-11 23:00 +0100 |
| Message-ID | <tjXdn-jj-1@gated-at.bofh.it> |
| In reply to | #1598303 |
[Multipart message — attachments visible in raw view] — view raw
Hi! > > > The rationale is that we should support the simplest use cases first. > > > > > > In the case of the first MC-based driver (and several subsequent > > > ones), the simplest use case required MC, as it was meant to suport > > > a custom-made sophisticated application that required fine control > > > on each component of the pipeline and to allow their advanced > > > proprietary AAA userspace-based algorithms to work. > > > > The first MC based driver (omap3isp) supports what the hardware can do, it > > does not support applications as such. > > All media drivers support a subset of what the hardware can do. The > question is if such subset covers the use cases or not. > > The current MC-based drivers (except for uvc) took a patch to offer a > more advanced API, to allow direct control to each IP module, as it was > said, by the time we merged the OMAP3 driver, that, for the N9/N900 camera > to work, it was mandatory to access the pipeline's individual components. > > Such approach require that some userspace software will have knowledge > about some hardware details, in order to setup pipelines and send controls > to the right components. That makes really hard to have a generic user > friendly application to use such devices. Well. Even if you propagate controls to the right components, there's still a lot application needs to know about the camera subsystem. Focus lengths, for example. Speed of the focus coil. Whether or not aperture controls are available. If they are not, what is the fixed aperture. Dunno. Knowing what control to apply on what subdevice does not look like the hardest part of camera driver. Yes, it would be a tiny bit easier if I would have just one device to deal with, but.... fcam-dev has cca 20000 lines of C++ code. > In the case of V4L2 controls, when there's no subdev API, the main > driver (e. g. the driver that creates the /dev/video nodes) sends a > multicast message to all bound I2C drivers. The driver(s) that need > them handle it. When the same control may be implemented on different > drivers, the main driver sends a unicast message to just one > driver[1]. Dunno. There's quite common to have two flashes. In that case, will application control both at the same time? > There's nothing wrong with this approach: it works, it is simpler, > it is generic. So, if it covers most use cases, why not allowing it > for usecases where a finer control is not a requirement? Because the resulting interface is quite ugly? > That's why I'm saying that I'm OK on merging any patch that would allow > setting controls via the /dev/video interface on MC-based drivers when > compiled without subdev API. I may also consider merging patches allowing So.. userspace will now have to detect if subdev is available or not, and access hardware in different ways? > > The original plan was and continues to be sound, it's just that there have > > always been too few hands to implement it. :-( > > If there are no people to implement a plan, it doesn't matter how good > the plan is, it won't work. If the plan is good, someone will do it. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[toc] | [prev] | [next] | [standalone]
| From | Russell King - ARM Linux <linux@armlinux.org.uk> |
|---|---|
| Date | 2017-03-12 00:20 +0100 |
| Message-ID | <tjYsN-1oY-1@gated-at.bofh.it> |
| In reply to | #1598303 |
On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote: > This situation is there since 2009. If I remember well, you tried to write > such generic plugin in the past, but never finished it, apparently because > it is too complex. Others tried too over the years. > > The last trial was done by Jacek, trying to cover just the exynos4 driver. > Yet, even such limited scope plugin was not good enough, as it was never > merged upstream. Currently, there's no such plugins upstream. > > If we can't even merge a plugin that solves it for just *one* driver, > I have no hope that we'll be able to do it for the generic case. This is what really worries me right now about the current proposal for iMX6. What's being proposed is to make the driver exclusively MC-based. What that means is that existing applications are _not_ going to work until we have some answer for libv4l2, and from what you've said above, it seems that this has been attempted multiple times over the last _8_ years, and each time it's failed. When thinking about it, it's quite obvious why merely trying to push the problem into userspace fails: If we assert that the kernel does not have sufficient information to make decisions about how to route and control the hardware, then under what scenario does a userspace library have sufficient information to make those decisions? So, merely moving the problem into userspace doesn't solve anything. Loading the problem onto the user in the hope that the user knows enough to properly configure it also doesn't work - who is going to educate the user about the various quirks of the hardware they're dealing with? I don't think pushing it into platform specific libv4l2 plugins works either - as you say above, even just trying to develop a plugin for exynos4 seems to have failed, so what makes us think that developing a plugin for iMX6 is going to succeed? Actually, that's exactly where the problem lies. Is "iMX6 plugin" even right? That only deals with the complexity of one part of the system - what about the source device, which as we have already seen can be a tuner or a camera with its own multiple sub-devices. What if there's a video improvement chip in the chain as well - how is a "generic" iMX6 plugin supposed to know how to deal with that? It seems to me that what's required is not an "iMX6 plugin" but a separate plugin for each platform - or worse. Consider boards like the Raspberry Pi, where users can attach a variety of cameras. I don't think this approach scales. (This is relevant: the iMX6 board I have here has a RPi compatible connector for a MIPI CSI2 camera. In fact, the IMX219 module I'm using _is_ a RPi camera, it's the RPi NoIR Camera V2.) The iMX6 problem is way larger than just "which subdev do I need to configure for control X" - if you look at the dot graphs both Steve and myself have supplied, you'll notice that there are eight (yes, 8) video capture devices. Let's say that we can solve the subdev problem in libv4l2. There's another problem lurking here - libv4l2 is /dev/video* based. How does it know which /dev/video* device to open? We don't open by sensor, we open by /dev/video*. In my case, there is only one correct /dev/video* node for the attached sensor, the other seven are totally irrelevant. For other situations, there may be the choice of three functional /dev/video* nodes. Right now, for my case, there isn't the information exported from the kernel to know which is the correct one, since that requires knowing which virtual channel the data is going to be sent over the CSI2 interface. That information is not present in DT, or anywhere. It only comes from system knowledge - in my case, I know that the IMX219 is currently being configured to use virtual channel 0. SMIA cameras are also configurable. Then there's CSI2 cameras that can produce different formats via different virtual channels (eg, JPEG compressed image on one channel while streaming a RGB image via the other channel.) Whether you can use one or three in _this_ scenario depends on the source format - again, another bit of implementation specific information that userspace would need to know. Kernel space should know that, and it's discoverable by testing which paths accept the source format - but that doesn't tell you ahead of time which /dev/video* node to open. So, the problem space we have here is absolutely huge, and merely having a plugin that activates when you open a /dev/video* node really doesn't solve it. All in all, I really don't think "lets hope someone writes a v4l2 plugin to solve it" is ever going to be successful. I don't even see that there will ever be a userspace application that is anything more than a representation of the dot graphs that users can use to manually configure the capture system with system knowledge. I think everyone needs to take a step back and think long and hard about this from the system usability perspective - I seriously doubt that we will ever see any kind of solution to this if we continue to progress with "we'll sort it in userspace some day." -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
[toc] | [prev] | [next] | [standalone]
| From | Steve Longerbeam <slongerbeam@gmail.com> |
|---|---|
| Date | 2017-03-12 01:30 +0100 |
| Message-ID | <tjZyy-246-3@gated-at.bofh.it> |
| In reply to | #1598505 |
On 03/11/2017 03:14 PM, Russell King - ARM Linux wrote: > On Sat, Mar 11, 2017 at 08:25:49AM -0300, Mauro Carvalho Chehab wrote: >> This situation is there since 2009. If I remember well, you tried to write >> such generic plugin in the past, but never finished it, apparently because >> it is too complex. Others tried too over the years. >> >> The last trial was done by Jacek, trying to cover just the exynos4 driver. >> Yet, even such limited scope plugin was not good enough, as it was never >> merged upstream. Currently, there's no such plugins upstream. >> >> If we can't even merge a plugin that solves it for just *one* driver, >> I have no hope that we'll be able to do it for the generic case. > > This is what really worries me right now about the current proposal for > iMX6. What's being proposed is to make the driver exclusively MC-based. > I don't see anything wrong with that. > What that means is that existing applications are _not_ going to work > until we have some answer for libv4l2, and from what you've said above, > it seems that this has been attempted multiple times over the last _8_ > years, and each time it's failed. > > When thinking about it, it's quite obvious why merely trying to push > the problem into userspace fails: > > If we assert that the kernel does not have sufficient information to > make decisions about how to route and control the hardware, then under > what scenario does a userspace library have sufficient information to > make those decisions? > > So, merely moving the problem into userspace doesn't solve anything. > > Loading the problem onto the user in the hope that the user knows > enough to properly configure it also doesn't work - who is going to > educate the user about the various quirks of the hardware they're > dealing with? Documentation? > > I don't think pushing it into platform specific libv4l2 plugins works > either - as you say above, even just trying to develop a plugin for > exynos4 seems to have failed, so what makes us think that developing > a plugin for iMX6 is going to succeed? Actually, that's exactly where > the problem lies. > > Is "iMX6 plugin" even right? That only deals with the complexity of > one part of the system - what about the source device, which as we > have already seen can be a tuner or a camera with its own multiple > sub-devices. What if there's a video improvement chip in the chain > as well - how is a "generic" iMX6 plugin supposed to know how to deal > with that? > > It seems to me that what's required is not an "iMX6 plugin" but a > separate plugin for each platform - or worse. Consider boards like > the Raspberry Pi, where users can attach a variety of cameras. I > don't think this approach scales. (This is relevant: the iMX6 board > I have here has a RPi compatible connector for a MIPI CSI2 camera. > In fact, the IMX219 module I'm using _is_ a RPi camera, it's the RPi > NoIR Camera V2.) > > The iMX6 problem is way larger than just "which subdev do I need to > configure for control X" - if you look at the dot graphs both Steve > and myself have supplied, you'll notice that there are eight (yes, > 8) video capture devices. There are 4 video nodes (per IPU): - unconverted capture from CSI0 - unconverted capture from CSI1 - scaled, CSC, and/or rotated capture from PRP ENC - scaled, CSC, rotated, and/or de-interlaced capture from PRP VF Configuring the imx6 pipelines are not that difficult. I've put quite a bit of detail in the media doc, so it should become clear to any user with MC knowledge (even those with absolutely no knowledge of imx) to quickly start getting working pipelines. Let's say that we can solve the subdev > problem in libv4l2. There's another problem lurking here - libv4l2 > is /dev/video* based. How does it know which /dev/video* device to > open? > > We don't open by sensor, we open by /dev/video*. In my case, there > is only one correct /dev/video* node for the attached sensor, the > other seven are totally irrelevant. For other situations, there may > be the choice of three functional /dev/video* nodes. > > Right now, for my case, there isn't the information exported from the > kernel to know which is the correct one, since that requires knowing > which virtual channel the data is going to be sent over the CSI2 > interface. That information is not present in DT, or anywhere. It is described in the media doc: "This is the MIPI CSI-2 receiver entity. It has one sink pad to receive the MIPI CSI-2 stream (usually from a MIPI CSI-2 camera sensor). It has four source pads, corresponding to the four MIPI CSI-2 demuxed virtual channel outputs." > It only comes from system knowledge - in my case, I know that the IMX219 > is currently being configured to use virtual channel 0. SMIA cameras > are also configurable. Then there's CSI2 cameras that can produce > different formats via different virtual channels (eg, JPEG compressed > image on one channel while streaming a RGB image via the other channel.) > > Whether you can use one or three in _this_ scenario depends on the > source format - again, another bit of implementation specific > information that userspace would need to know. Kernel space should > know that, and it's discoverable by testing which paths accept the > source format - but that doesn't tell you ahead of time which > /dev/video* node to open. > > So, the problem space we have here is absolutely huge, and merely > having a plugin that activates when you open a /dev/video* node > really doesn't solve it. > > All in all, I really don't think "lets hope someone writes a v4l2 > plugin to solve it" is ever going to be successful. I don't even > see that there will ever be a userspace application that is anything > more than a representation of the dot graphs that users can use to > manually configure the capture system with system knowledge. > > I think everyone needs to take a step back and think long and hard > about this from the system usability perspective - I seriously > doubt that we will ever see any kind of solution to this if we > continue to progress with "we'll sort it in userspace some day." While I admit when I first came across the MC idea a couple years ago, my first impression was it was putting a lot of burden on the user to have a detailed knowledge of the system in question. But I don't think that is a problem with good documentation, and most people who have a need to use a specific MC driver will already have that knowledge. Steve
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web