LinuxLists.cc - Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

2017-03-02 16:43:07

Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

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 <[email protected]>
> ---
> 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: [email protected] XMPP: [email protected]

2017-03-02 23:51:23

by Steve Longerbeam

[permalink] [raw]

Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

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 <[email protected]>
>> ---
>> 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

2017-03-03 00:58:25

by Steve Longerbeam

[permalink] [raw]

Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

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 <[email protected]>
>>> ---
>>> 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.
>

2017-03-03 02:22:20

by Steve Longerbeam

[permalink] [raw]

Em Fri, 10 Mar 2017 15:57:09 +0000
Russell King - ARM Linux <[email protected]> escreveu:

> On Fri, Mar 10, 2017 at 12:26:34PM -0300, Mauro Carvalho Chehab wrote:
> > Hi Russell,
> >
> > Em Fri, 10 Mar 2017 13:07:33 +0000
> > Russell King - ARM Linux <[email protected]> escreveu:
> >
> > > 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
> >
> > That's not true. There is an special flag, used only by libv4l2
> > emulated formats, that indicates when a video format is handled
> > via v4lconvert:
>
> I'm afraid that my statement comes from trying to use gstreamer with
> libv4l2 and _not_ being able to use the 8-bit bayer formats there at
> all - they are simply not passed across to the application through
> libv4l2/v4lconvert.
>
> Instead, the formats that are passed across are the emulated formats.
> As I said above, that forces applications to use only the v4lconvert
> formats, the raw formats are not available.
>
> So, the presence or absence of the V4L2_FMT_FLAG_EMULATED is quite
> meaningless if you can't even enumerate the non-converted formats.
>
> The problem comes from the "always needs conversion" stuff in
> v4lconvert coupled with the way this subdev stuff works - since it
> requires manual configuration of all the pads within the kernel
> media pipeline, the kernel ends up only advertising _one_ format
> to userspace - in my case, that's RGGB8.
>
> When v4lconvert_create_with_dev_ops() enumerates the formats from
> the kernel, it gets only RGGB8. That causes always_needs_conversion
> in there to remain true, so the special v4l control which enables/
> disables conversion gets created with a default value of "true".
> The RGGB8 bit is also set in data->supported_src_formats.
>
> This causes v4lconvert_supported_dst_fmt_only() to return true.
>
> What this all means is that v4lconvert_enum_fmt() will _not_ return
> any of the kernel formats, only the faked formats.
>
> Ergo, the RGGB8 format from the kernel is completely hidden from the
> application, and only the emulated format is made available. As I
> said above, this forces v4lconvert's debayering on the application,
> whether you want it or not.
>
> In the gstreamer case, it knows nothing about this special control,
> which means that trying to use this gstreamer pipeline:
>
> $ gst-launch-1.0 v4l2src device=/dev/video6 ! bayer2rgbneon ! xvimagesink
>
> is completely impossible without first rebuilding gstreamer _without_
> libv4l support. Build gstreamer without libv4l support, and the above
> works.
>
> Enabling debug output in gstreamer's v4l2src plugin confirms that
> the kernel's bayer format are totally hidden from gstreamer when
> linked with libv4l2, but are present when it isn't linked with
> libv4l2.

Argh! that is indeed a bug at libv4l (and maybe at gstreamer).

I guess that the always_needs_conversion logic was meant to be used to
really odd proprietary formats, e. g:

/* Vendor-specific formats */
#define V4L2_PIX_FMT_CPIA1 v4l2_fourcc('C', 'P', 'I', 'A') /* cpia1 YUV */
#define V4L2_PIX_FMT_WNVA v4l2_fourcc('W', 'N', 'V', 'A') /* Winnov hw compress */
#define V4L2_PIX_FMT_SN9C10X v4l2_fourcc('S', '9', '1', '0') /* SN9C10x compression */
...

I suspect that nobody uses libv4l2 with MC-based V4L2 devices. That's
likely why nobody reported this bug before (that I know of).

In any case, for non-proprietary formats, the default should be to
always offer both the emulated format and the original one.

I suspect that the enclosed patch should fix the issue with bayer formats.

>

Thanks,
Mauro

[PATCH RFC] libv4lconvert: by default, offer the original format to the client

Applications should have the right to decide between using a
libv4lconvert emulated format or to implement the decoding themselves,
as this may have significative performance impact.

So, change the default to always show both formats.

Change also the default for Bayer encoded formats, as userspace
likely will want to handle it directly.

Signed-off-by: Mauro Carvalho Chehab <[email protected]>

diff --git a/lib/libv4lconvert/libv4lconvert.c b/lib/libv4lconvert/libv4lconvert.c
index da718918b030..2e5458fa420d 100644
--- a/lib/libv4lconvert/libv4lconvert.c
+++ b/lib/libv4lconvert/libv4lconvert.c
@@ -118,10 +118,10 @@ static const struct v4lconvert_pixfmt supported_src_pixfmts[] = {
{ V4L2_PIX_FMT_OV511, 0, 7, 7, 1 },
{ V4L2_PIX_FMT_OV518, 0, 7, 7, 1 },
/* uncompressed bayer */
- { V4L2_PIX_FMT_SBGGR8, 8, 8, 8, 1 },
- { V4L2_PIX_FMT_SGBRG8, 8, 8, 8, 1 },
- { V4L2_PIX_FMT_SGRBG8, 8, 8, 8, 1 },
- { V4L2_PIX_FMT_SRGGB8, 8, 8, 8, 1 },
+ { V4L2_PIX_FMT_SBGGR8, 8, 8, 8, 0 },
+ { V4L2_PIX_FMT_SGBRG8, 8, 8, 8, 0 },
+ { V4L2_PIX_FMT_SGRBG8, 8, 8, 8, 0 },
+ { V4L2_PIX_FMT_SRGGB8, 8, 8, 8, 0 },
{ V4L2_PIX_FMT_STV0680, 8, 8, 8, 1 },
/* compressed bayer */
{ V4L2_PIX_FMT_SPCA561, 0, 9, 9, 1 },
@@ -178,7 +178,7 @@ struct v4lconvert_data *v4lconvert_create_with_dev_ops(int fd, void *dev_ops_pri
/* This keeps tracks of devices which have only formats for which apps
most likely will need conversion and we can thus safely add software
processing controls without a performance impact. */
- int always_needs_conversion = 1;
+ int always_needs_conversion = 0;

if (!data) {
fprintf(stderr, "libv4lconvert: error: out of memory!\n");
@@ -208,8 +208,8 @@ struct v4lconvert_data *v4lconvert_create_with_dev_ops(int fd, void *dev_ops_pri
if (j < ARRAY_SIZE(supported_src_pixfmts)) {
data->supported_src_formats |= 1ULL << j;
v4lconvert_get_framesizes(data, fmt.pixelformat, j);
- if (!supported_src_pixfmts[j].needs_conversion)
- always_needs_conversion = 0;
+ if (supported_src_pixfmts[j].needs_conversion)
+ always_needs_conversion = 1;
} else
always_needs_conversion = 0;
}

2017-03-10 21:55:34

On Sat, Mar 11, 2017 at 05:32:29PM +0200, Sakari Ailus wrote:
> My understanding of the i.MX6 case is the hardware is configurable enough
> to warrant the use of the Media controller API. Some patches indicate
> there are choices to be made in data routing.

The iMX6 does have configurable data routing, but in some scenarios
(eg, when receiving bayer data) there's only one possible routing.

> Steve: could you enlighten us on the topic, by e.g. doing media-ctl
> --print-dot and sending the results to the list? What kind of different IP
> blocks are there and what do they do? A pointer to hardware documentation
> wouldn't hurt either (if it's available).

Attached for the imx219 camera. Note that although the CSI2 block has
four outputs, each output is dedicated to a CSI virtual channel, so
they can not be arbitarily assigned without configuring the sensor.

Since the imx219 only produces bayer, the graph is also showing the
_only_ possible routing for the imx219 configured for CSI virtual
channel 0.

The iMX6 manuals are available on the 'net.

https://community.nxp.com/docs/DOC-101840

There are several chapters that cover the capture side:

* MIPI CSI2
* IPU CSI2 gasket
* IPU

The IPU not only performs capture, but also display as well.

--
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.

Attachments:

(No filename) (1.40 kB)
imx219.dot (4.45 kB)
Download all attachments

2017-03-11 18:08:38

Hi!

> > > Ok, perhaps supporting both subdev API and V4L2 API at the same
> > > time doesn't make much sense. We could disable one in favor of the
> > > other, either at compilation time or at runtime.
> >
> > Right. If the subdev API is disabled, then you have to inherit the subdev
> > controls in the bridge driver (how else would you be able to access them?).
> > And that's the usual case.
> >
> > If you do have the subdev API enabled, AND you use the MC, then the
> > intention clearly is to give userspace full control and inheriting controls
> > no longer makes any sense (and is highly confusing IMHO).
>
> I tend to agree with that.

Well, having different userspace interface according to config options
is strange. I believe the right solution is to make complex drivers
depend on CONFIG_VIDEO_V4L2_SUBDEV_API...

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (984.00 B)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-11 21:53:37

On Sun 2017-03-12 07:37:45, Russell King - ARM Linux wrote:
> On Sat, Mar 11, 2017 at 07:31:18PM -0800, Steve Longerbeam wrote:
> >
> >
> > On 03/11/2017 10:59 AM, Russell King - ARM Linux wrote:
> > >On Sat, Mar 11, 2017 at 10:54:55AM -0800, Steve Longerbeam wrote:
> > >>
> > >>
> > >>On 03/11/2017 10:45 AM, Russell King - ARM Linux wrote:
> > >>>I really don't think expecting the user to understand and configure
> > >>>the pipeline is a sane way forward. Think about it - should the
> > >>>user need to know that, because they have a bayer-only CSI data
> > >>>source, that there is only one path possible, and if they try to
> > >>>configure a different path, then things will just error out?
> > >>>
> > >>>For the case of imx219 connected to iMX6, it really is as simple as
> > >>>"there is only one possible path" and all the complexity of the media
> > >>>interfaces/subdevs is completely unnecessary. Every other block in
> > >>>the graph is just noise.
> > >>>
> > >>>The fact is that these dot graphs show a complex picture, but reality
> > >>>is somewhat different - there's only relatively few paths available
> > >>>depending on the connected source and the rest of the paths are
> > >>>completely useless.
> > >>>
> > >>
> > >>I totally disagree there. Raw bayer requires passthrough yes, but for
> > >>all other media bus formats on a mipi csi-2 bus, and all other media
> > >>bus formats on 8-bit parallel buses, the conersion pipelines can be
> > >>used for scaling, CSC, rotation, and motion-compensated de-interlacing.
> > >
> > >... which only makes sense _if_ your source can produce those formats.
> > >We don't actually disagree on that.
> >
> > ...and there are lots of those sources! You should try getting out of
> > your imx219 shell some time, and have a look around! :)
>
> If you think that, you are insulting me. I've been thinking about this
> from the "big picture" point of view. If you think I'm only thinking
> about this from only the bayer point of view, you're wrong.

Can you stop that insults nonsense?

> Given what Mauro has said, I'm convinced that the media controller stuff
> is a complete failure for usability, and adding further drivers using it
> is a mistake.

Hmm. But you did not present any alternative. Seems some hardware is
simply complex. So either we don't add complex drivers (_that_ would
be a mistake), or some userspace solution will need to be
done. Shell-script running media-ctl does not seem that hard.

> So, tell me how the user can possibly use iMX6 video capture without
> resorting to opening up a terminal and using media-ctl to manually
> configure the pipeline. How is the user going to control the source
> device without using media-ctl to find the subdev node, and then using
> v4l2-ctl on it. How is the user supposed to know which /dev/video*
> node they should be opening with their capture application?

Complex hardware sometimes requires userspace configuration. Running a
shell script on startup does not seem that hard.

And maybe we could do some kind of default setup in kernel, but that
does not really solve the problem.

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (3.20 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-12 21:29:16

Em Tue, 14 Mar 2017 08:55:36 +0100
Hans Verkuil <[email protected]> escreveu:

> On 03/14/2017 04:45 AM, Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> >
> > I started preparing a long argument about it, but gave up in favor of a
> > simpler one.
> >
> > Em Mon, 13 Mar 2017 14:46:22 +0200
> > Sakari Ailus <[email protected]> escreveu:
> >
> >> Drivers are written to support hardware, not particular use case.
> >
> > No, it is just the reverse: drivers and hardware are developed to
> > support use cases.
> >
> > Btw, you should remember that the hardware is the full board, not just the
> > SoC. In practice, the board do limit the use cases: several provide a
> > single physical CSI connector, allowing just one sensor.
> >
> >>> 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.
> >>
> >> I'd argue I know better what happened with that attempt than you do. I had a
> >> prototype of a generic pipeline configuration library but due to various
> >> reasons I haven't been able to continue working on that since around 2012.
> >
> > ...
> >
> >>> 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.
> >>
> >> I believe Jacek ceased to work on that plugin in his day job; other than
> >> that, there are some matters left to be addressed in his latest patchset.
> >
> > The two above basically summaries the issue: the task of doing a generic
> > plugin on userspace, even for a single driver is complex enough to
> > not cover within a reasonable timeline.
> >
> > From 2009 to 2012, you were working on it, but didn't finish it.
> >
> > Apparently, nobody worked on it between 2013-2014 (but I may be wrong, as
> > I didn't check when the generic plugin interface was added to libv4l).
> >
> > In the case of Jacek's work, the first patch I was able to find was
> > written in Oct, 2014:
> > https://patchwork.kernel.org/patch/5098111/
> > (not sure what happened with the version 1).
> >
> > The last e-mail about this subject was issued in Dec, 2016.
> >
> > In summary, you had this on your task for 3 years for an OMAP3
> > plugin (where you have a good expertise), and Jacek for 2 years,
> > for Exynos 4, where he should also have a good knowledge.
> >
> > Yet, with all that efforts, no concrete results were achieved, as none
> > of the plugins got merged.
> >
> > Even if they were merged, if we keep the same mean time to develop a
> > libv4l plugin, that would mean that a plugin for i.MX6 could take 2-3
> > years to be developed.
> >
> > There's a clear message on it:
> > - we shouldn't keep pushing for a solution via libv4l.
>
> Or:
>
> - userspace plugin development had a very a low priority and
> never got the attention it needed.

The end result is the same: we can't count on it.

>
> I know that's *my* reason. I rarely if ever looked at it. I always assumed
> Sakari and/or Laurent would look at it. If this reason is also valid for
> Sakari and Laurent, then it is no wonder nothing has happened in all that
> time.
>
> We're all very driver-development-driven, and userspace gets very little
> attention in general. So before just throwing in the towel we should take
> a good look at the reasons why there has been little or no development: is
> it because of fundamental design defects, or because nobody paid attention
> to it?

No. We should look it the other way: basically, there are patches
for i.MX6 driver that sends control from videonode to subdevs.

If we nack apply it, who will write the userspace plugin? When
such change will be merged upstream?

If we don't have answers to any of the above questions, we should not
nack it.

That's said, that doesn't prevent merging a libv4l plugin if/when
someone can find time/interest to develop it.

> I strongly suspect it is the latter.
>
> In addition, I suspect end-users of these complex devices don't really care
> about a plugin: they want full control and won't typically use generic
> applications. If they would need support for that, we'd have seen much more
> interest. The main reason for having a plugin is to simplify testing and
> if this is going to be used on cheap hobbyist devkits.

What are the needs for a cheap hobbyist devkit owner? Do we currently
satisfy those needs? I'd say that having a functional driver when
compiled without the subdev API, that implements the ioctl's/controls
that a generic application like camorama/google talk/skype/zbar...
would work should be enough to make them happy, even if they need to
add some udev rule and/or run some "prep" application that would setup
the pipelines via MC and eventually rename the device with a working
pipeline to /dev/video0.

>
> An additional complication is simply that it is hard to find fully supported
> MC hardware. omap3 boards are hard to find these days, renesas boards are not
> easy to get, freescale isn't the most popular either. Allwinner, mediatek,
> amlogic, broadcom and qualcomm all have closed source implementations or no
> implementation at all.

I'd say that we should not care anymore on providing a solution for
generic applications to run on boards like OMAP3[1]. For hardware that
are currently available that have Kernel driver and boards developed
to be used as "cheap hobbyist devkit", I'd say we should implement
a Kernel solution that would allow them to be used without subdev
API, e. g. having all ioctls needed by generic applications to work
functional, after some external application sets the pipeline.

[1] Yet, I might eventually do that for fun, an OMAP3 board with tvp5150
just arrived here last week. It would be nice to have xawtv3 running on it :-)
So, if I have a lot of spare time (with is very unlikely), I might eventually
do something for it to work.

> I know it took me a very long time before I had a working omap3.

My first OMAP3 board with working V4L2 source just arrived last week :-)

> So I am not at all surprised that little progress has been made.

I'm not surprised, but I'm disappointed, as I tried to push toward a
solution for this problem since when we had our initial meetings about
it.

Thanks,
Mauro

2017-03-14 18:26:46

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

Hi!

> > Mid-layer is difficult... there are _hundreds_ of possible
> > pipeline setups. If it should live in kernel or in userspace is a
> > question... but I don't think having it in kernel helps in any way.
>
> Mid-layer is difficult, because we either need to feed some
> library with knowledge for all kernel drivers or we need to improve
> the MC API to provide more details.
>
> For example, several drivers used to expose entities via the
> generic MEDIA_ENT_T_DEVNODE to represent entities of different
> types. See, for example, entities 1, 5 and 7 (and others) at:
>
> > https://mchehab.fedorapeople.org/mc-next-gen/igepv2_omap3isp.png

Well... we provide enough information, so that device-specific code
does not have to be in kernel.

There are few types of ENT_T_DEVNODE there. "ISP CCP2" does not really
provide any functionality to the user. It just has to be there,
because the pipeline needs to be connected. "ISP Preview" provides
format conversion. "ISP Resizer" provides rescaling.

I'm not sure if it ever makes sense to use "ISP Preview
output". Normally you take data for display from "ISP Resizer
output". (Would there be some power advantage from that?)

> A device-specific code could either be hardcoding the entity number
> or checking for the entity strings to add some logic to setup
> controls on those "unknown" entities, a generic app won't be able
> to do anything with them, as it doesn't know what function(s) such
> entity provide.

Generic app should know if it wants RGGB10 data, then it can use "ISP
CCDC output", or if it wants "cooked" data suitable for display, then
it wants to use "ISP Resizer output". Once application knows what
output it wants, there's just one path through the system. So being
able to tell the ENT_T_DEVNODEs apart does not seem to be critical.

OTOH, for useful camera application, different paths are needed at
different phases.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.00 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-14 22:33:04

Hi!

> > Well, I believe first question is: what applications would we want to
> > run on complex devices? Will sending control from video to subdevs
> > actually help?
>
> I would say: camorama, xawtv3, zbar, google talk, skype. If it runs
> with those, it will likely run with any other application.

I'll take a look when I'm at better internet access.

> > mplayer is useful for testing... but that one already works (after you
> > setup the pipeline, and configure exposure/gain).
> >
> > But thats useful for testing, not really for production. Image will be
> > out of focus and with wrong white balance.
> >
> > What I would really like is an application to get still photos. For
> > taking pictures with manual settings we need
> >
> > a) units for controls: user wants to focus on 1m, and take picture
> > with ISO200, 1/125 sec. We should also tell him that lens is f/5.6 and
> > focal length is 20mm with 5mm chip.
> >
> > But... autofocus/autogain would really be good to have. Thus we need:
> >
> > b) for each frame, we need exposure settings and focus position at
> > time frame was taken. Otherwise autofocus/autogain will be too
> > slow. At least focus position is going to be tricky -- either kernel
> > would have to compute focus position for us (not trivial) or we'd need
> > enough information to compute it in userspace.
> >
> > There are more problems: hardware-accelerated preview is not trivial
> > to set up (and I'm unsure if it can be done in generic way). Still
> > photos application needs to switch resolutions between preview and
> > photo capture. Probably hardware-accelerated histograms are needed for
> > white balance, auto gain and auto focus, ....
> >
> > It seems like there's a _lot_ of stuff to be done before we have
> > useful support for complex cameras...
>
> Taking still pictures using a hardware-accelerated preview is
> a sophisticated use case. I don't know any userspace application
> that does that. Ok, several allow taking snapshots, by simply
> storing the image of the current frame.

Well, there are applications that take still pictures. Android has
one. Maemo has another. Then there's fcam-dev. Its open source; with
modified kernel it is fully usable. I have version that runs on recent
nearly-mainline on N900.

So yes, I'd like solution for problems a) and b).

> > (And I'm not sure... when application such as skype is running, is
> > there some way to run autogain/autofocus/autowhitebalance? Is that
> > something we want to support?)
>
> Autofocus no. Autogain/Autowhite can be done via libv4l, provided that
> it can access the device's controls via /dev/video devnode. Other
> applications may be using some other similar algorithms.
>
> Ok, they don't use histograms provided by the SoC. So, they do it in
> software, with is slower. Still, it works fine when the light
> conditions don't change too fast.

I guess it is going to work well enough with higher CPU
usage. Question is if camera without autofocus is usable. I'd say "not
really".

> > I believe other question is: will not having same control on main
> > video device and subdevs be confusing? Does it actually help userspace
> > in any way? Yes, we can make controls accessible to old application,
> > but does it make them more useful?
>
> Yes. As I said, libv4l (and some apps) have logic inside to adjust
> the image via bright, contrast and white balance controls, using the
> video devnode. They don't talk subdev API. So, if those controls
> aren't exported, they won't be able to provide a good quality image.

Next question is if the libv4l will do the right thing if we just put
all controls to one node. For example on N900 you have exposure/gain
and brightness. But the brightness is applied at preview phase, so it
is "basically useless". You really need to adjust the image using the
exposure/gain.

> > > > In addition, I suspect end-users of these complex devices don't really care
> > > > about a plugin: they want full control and won't typically use generic
> > > > applications. If they would need support for that, we'd have seen much more
> > > > interest. The main reason for having a plugin is to simplify testing and
> > > > if this is going to be used on cheap hobbyist devkits.
> > >
> > > What are the needs for a cheap hobbyist devkit owner? Do we currently
> > > satisfy those needs? I'd say that having a functional driver when
> > > compiled without the subdev API, that implements the ioctl's/controls
> >
> > Having different interface based on config options... is just
> > weird. What about poor people (like me) trying to develop complex
> > applications?
>
> Well, that could be done using other mechanisms, like a modprobe
> parameter or by switching the behaviour if a subdev interface is
> opened. I don't see much trouble on allowing accessing a control via
> both interfaces.

If we really want to go that way (is not modifying library to access
the right files quite easy?), I believe non-confusing option would be
to have '/dev/video0 -- omap3 camera for legacy applications' which
would include all the controls.

> > You can get Nokia N900 on aliexpress. If not, they are still available
> > between people :-)
>
> I have one. Unfortunately, I never had a chance to use it, as the display
> stopped working one week after I get it.

Well, I guess the easiest option is to just get another one :-).

But otoh -- N900 is quite usable without the screen. 0xffff tool can
be used to boot the kernel, then you can use nfsroot and usb
networking. It also has serial port (over strange
connector). Connected over ssh over usb network is actually how I do
most of the v4l work.

Best regards,

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (5.71 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-15 18:56:23

Hi!

> > Making use of the full potential of the hardware requires using a more
> > expressive interface.
>
> That's the core of the problem: most users don't need "full potential
> of the hardware". It is actually worse than that: several boards
> don't allow "full potential" of the SoC capabilities.

Well, in kernel we usually try to support "full hardware" potential.

And we are pretty sure users would like to take still photos, even if
common v4l2 applications can not do it.

> > That's what the kernel interface must provide. If
> > we decide to limit ourselves to a small sub-set of that potential on the
> > level of the kernel interface, we have made a wrong decision. It's as
> > simple as that. This is why the functionality (and which requires taking
> > a lot of policy decisions) belongs to the user space. We cannot have
> > multiple drivers providing multiple kernel interfaces for the same hardware.
>
> I strongly disagree. Looking only at the hardware capabilities without
> having a solution to provide what the user wants is *wrong*.

Hardware manufacturers already did this kind of research for us. They
don't usually include features noone wants...

> Another case: the cx25821 hardware supports 12 video streams,
> consuming almost all available bandwidth of an ePCI bus. Each video
> stream connector can either be configured to be capture or output, in
> runtime. The hardware vendor chose to hardcode the driver to provide
> 8 inputs and 4 outputs. Their decision was based in the fact that
> the driver is already very complex, and it satisfies their customer's
> needs. The cost/efforts of make the driver to be reconfigured in
> runtime were too high for almost no benefit.

Well, it is okay to provide 'limited' driver -- there's possibility to
fix the driver. But IMO it is not okay to provide 'limited' kernel
interface -- because if you try to fix it, you'll suddenly have
regressions.

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.03 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-26 09:13:03

by Pavel Machek

[permalink] [raw]

Subject: script to setup pipeline was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

Hi!

> > I do agree with you that MC places a lot of burden on the user to
> > attain a lot of knowledge of the system's architecture.
>
> Setting up the pipeline is not the hard part. One could write a
> script to do that.

Can you try to write that script? I believe it would solve big part of
the problem.

> > And my other point is, I think most people who have a need to work with
> > the media framework on a particular platform will likely already be
> > quite familiar with that platform.
>
> I disagree. The most popular platform device currently is Raspberry PI.
>
> I doubt that almost all owners of RPi + camera module know anything
> about MC. They just use Raspberry's official driver with just provides
> the V4L2 interface.
>
> I have a strong opinion that, for hardware like RPi, just the V4L2
> API is enough for more than 90% of the cases.

Maybe V4L2 API is enough for 90% of the users. But I don't believe
that means that we should provide compatibility. V4L2 API is not good
enough for complex devices, and if we can make RPi people fix
userspace... that's a good thing.

> > The media graph for imx6 is fairly self-explanatory in my opinion.
> > Yes that graph has to be generated, but just with a simple 'media-ctl
> > --print-dot', I don't see how that is difficult for the user.
>
> Again, IMHO, the problem is not how to setup the pipeline, but, instead,
> the need to forward controls to the subdevices.
>
> To use a camera, the user needs to set up a set of controls for the
> image to make sense (bright, contrast, focus, etc). If the driver
> doesn't forward those controls to the subdevs, an application like
> "camorama" won't actually work for real, as the user won't be able
> to adjust those parameters via GUI.

I believe this can be fixed in libv4l2.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (1.90 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2017-03-26 16:43:45

by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline

Hi Hans,

On Tuesday 14 Mar 2017 08:55:36 Hans Verkuil wrote:
> On 03/14/2017 04:45 AM, Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> >
> > I started preparing a long argument about it, but gave up in favor of a
> > simpler one.
> >
> > Em Mon, 13 Mar 2017 14:46:22 +0200 Sakari Ailus escreveu:
> >> Drivers are written to support hardware, not particular use case.
> >
> > No, it is just the reverse: drivers and hardware are developed to
> > support use cases.
> >
> > Btw, you should remember that the hardware is the full board, not just the
> > SoC. In practice, the board do limit the use cases: several provide a
> > single physical CSI connector, allowing just one sensor.
> >
> >>> 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.
> >>
> >> I'd argue I know better what happened with that attempt than you do. I
> >> had a prototype of a generic pipeline configuration library but due to
> >> various reasons I haven't been able to continue working on that since
> >> around 2012.
> > ...
> >
> >>> 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.
> >>
> >> I believe Jacek ceased to work on that plugin in his day job; other than
> >> that, there are some matters left to be addressed in his latest patchset.
> >
> > The two above basically summaries the issue: the task of doing a generic
> > plugin on userspace, even for a single driver is complex enough to
> > not cover within a reasonable timeline.
> >
> > From 2009 to 2012, you were working on it, but didn't finish it.
> >
> > Apparently, nobody worked on it between 2013-2014 (but I may be wrong, as
> > I didn't check when the generic plugin interface was added to libv4l).
> >
> > In the case of Jacek's work, the first patch I was able to find was
> >
> > written in Oct, 2014:
> > https://patchwork.kernel.org/patch/5098111/
> > (not sure what happened with the version 1).
> >
> > The last e-mail about this subject was issued in Dec, 2016.
> >
> > In summary, you had this on your task for 3 years for an OMAP3
> > plugin (where you have a good expertise), and Jacek for 2 years,
> > for Exynos 4, where he should also have a good knowledge.
> >
> > Yet, with all that efforts, no concrete results were achieved, as none
> > of the plugins got merged.
> >
> > Even if they were merged, if we keep the same mean time to develop a
> > libv4l plugin, that would mean that a plugin for i.MX6 could take 2-3
> > years to be developed.
> >
> > There's a clear message on it:
> > - we shouldn't keep pushing for a solution via libv4l.
>
> Or:
> - userspace plugin development had a very a low priority and
> never got the attention it needed.
>
> I know that's *my* reason. I rarely if ever looked at it. I always assumed
> Sakari and/or Laurent would look at it. If this reason is also valid for
> Sakari and Laurent, then it is no wonder nothing has happened in all that
> time.

The reason is also valid for me. I'd really love to work on the userspace
side, but I just can't find time at the moment.

> We're all very driver-development-driven, and userspace gets very little
> attention in general. So before just throwing in the towel we should take
> a good look at the reasons why there has been little or no development: is
> it because of fundamental design defects, or because nobody paid attention
> to it?
>
> I strongly suspect it is the latter.
>
> In addition, I suspect end-users of these complex devices don't really care
> about a plugin: they want full control and won't typically use generic
> applications. If they would need support for that, we'd have seen much more
> interest. The main reason for having a plugin is to simplify testing and
> if this is going to be used on cheap hobbyist devkits.
>
> An additional complication is simply that it is hard to find fully supported
> MC hardware. omap3 boards are hard to find these days, renesas boards are
> not easy to get, freescale isn't the most popular either. Allwinner,
> mediatek, amlogic, broadcom and qualcomm all have closed source
> implementations or no implementation at all.
>
> I know it took me a very long time before I had a working omap3.
>
> So I am not at all surprised that little progress has been made.

--
Regards,

Laurent Pinchart