Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

Hi Laurent,

Thanks for looking at this!

On Mon, Jun 19, 2017 at 6:17 PM, Laurent Pinchart
 wrote:
> Hi Tomasz,
>
> On Friday 16 Jun 2017 17:35:52 Tomasz Figa wrote:
>> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus wrote:
>> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
>> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa wrote:
>> >>> On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil wrote:
>>  On 06/06/17 09:25, Sakari Ailus wrote:
>> > On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
>> >> On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa wrote:
>> >>> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
>>  Add the IPU3 specific processing parameter format
>>  V4L2_META_FMT_IPU3_PARAMS and metadata formats
>>  for 3A and other statistics:
>> >>>
>> >>> Please see my comments inline.
>> >>>
>>    V4L2_META_FMT_IPU3_PARAMS
>>    V4L2_META_FMT_IPU3_STAT_3A
>>    V4L2_META_FMT_IPU3_STAT_DVS
>>    V4L2_META_FMT_IPU3_STAT_LACE
>> 
>>  Signed-off-by: Yong Zhi 
>>  ---
>> 
>>   drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>>   include/uapi/linux/videodev2.h   | 6 ++
>>   2 files changed, 10 insertions(+)
>> >>>
>> >>> [snip]
>> >>>
>>  +/* Vendor specific - used for IPU3 camera sub-system */
>>  +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3',
>>  'p') /* IPU3 params */
>>  +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3',
>>  's') /* IPU3 3A statistics */
>>  +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3',
>>  'd') /* IPU3 DVS statistics */
>>  +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3',
>>  'l') /* IPU3 LACE statistics */
>
> This series is missing a documentation patch with a clear and detailed
> description of the buffer contents for each of these formats. I'm not very
> concerned about the three statistics formats (although that might change after
> reading the documentation), but the "IPU3 params" format makes me feel nervous
> already.

I guess this is a note addressed to patch authors. :)

>
>> >>> We had some discussion about this with Laurent and if I remember
>> >>> correctly, the conclusion was that it might make sense to define
>> >>> one FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for
>> >>> example, and then have a V4L2-specific enum within the
>> >>> v4l2_pix_format(_mplane) struct that specifies the exact vendor data
>> >>> type.
>
> If I recall correctly, I mentioned that v4l2_format now has a struct
> v4l2_meta_format field that can be used to pass metadata-related parameters
> the same way that v4l2_pix_format passes image-related parameters. The only
> two metadata parameters currently defined are the data format (fourcc) and
> buffer size, and more can be added if needed. However, I don't think the
> v4l2_meta_format structure should be extended with vendor-specific fields.

Ah, then I got that wrong, sorry. But in general I believe we reached
exactly the same conclusion with Hans and Sakari after that.

Best regards,
Tomasz

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Laurent Pinchart]

Hi Tomasz,

On Friday 16 Jun 2017 17:35:52 Tomasz Figa wrote:
> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus wrote:
> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa wrote:
> >>> On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil wrote:
>  On 06/06/17 09:25, Sakari Ailus wrote:
> > On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
> >> On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa wrote:
> >>> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi wrote:
>  Add the IPU3 specific processing parameter format
>  V4L2_META_FMT_IPU3_PARAMS and metadata formats
>  for 3A and other statistics:
> >>>
> >>> Please see my comments inline.
> >>> 
>    V4L2_META_FMT_IPU3_PARAMS
>    V4L2_META_FMT_IPU3_STAT_3A
>    V4L2_META_FMT_IPU3_STAT_DVS
>    V4L2_META_FMT_IPU3_STAT_LACE
>  
>  Signed-off-by: Yong Zhi 
>  ---
>  
>   drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>   include/uapi/linux/videodev2.h   | 6 ++
>   2 files changed, 10 insertions(+)
> >>> 
> >>> [snip]
> >>> 
>  +/* Vendor specific - used for IPU3 camera sub-system */
>  +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3',
>  'p') /* IPU3 params */
>  +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3',
>  's') /* IPU3 3A statistics */
>  +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3',
>  'd') /* IPU3 DVS statistics */
>  +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3',
>  'l') /* IPU3 LACE statistics */

This series is missing a documentation patch with a clear and detailed 
description of the buffer contents for each of these formats. I'm not very 
concerned about the three statistics formats (although that might change after 
reading the documentation), but the "IPU3 params" format makes me feel nervous 
already.

> >>> We had some discussion about this with Laurent and if I remember
> >>> correctly, the conclusion was that it might make sense to define
> >>> one FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for
> >>> example, and then have a V4L2-specific enum within the
> >>> v4l2_pix_format(_mplane) struct that specifies the exact vendor data
> >>> type.

If I recall correctly, I mentioned that v4l2_format now has a struct 
v4l2_meta_format field that can be used to pass metadata-related parameters 
the same way that v4l2_pix_format passes image-related parameters. The only 
two metadata parameters currently defined are the data format (fourcc) and 
buffer size, and more can be added if needed. However, I don't think the 
v4l2_meta_format structure should be extended with vendor-specific fields.

> >>> It seems saner than assigning a new FourCC whenever a new
> >>> hardware revision comes out, especially given that FourCCs tend to
> >>> be used outside of the V4L2 world as well and being kind of (de
> >>> facto) standardized (with existing exceptions, unfortunately).
>  
>  I can't remember that discussion
> >>> 
> >>> I think that was just a casual chat between Lauren, me and few more
> >>> guys.
> >>> 
>  although I've had other discussions with Laurent related to this on how
>  to handle formats that have many variations on a theme.
>  
>  But speaking for this specific case I see no reason to do something
>  special. There are only four new formats, which seems perfectly
>  reasonable to me.
>  
>  I don't see the advantage of adding another layer of pixel formats.
>  You still need to define something for this, one way or the other.
>  And this way doesn't require API changes.
>  
> > If we have four video nodes with different vendor specific formats,
> > how does the user tell the formats apart? I presume the user space
> > could use the entity names for instance, but that would essentially
> > make them device specific.
>  
>  Well, they are. There really is no way to avoid that.
>  
> > I'm not sure if there would be any harm from that in practice though:
> > the user will need to find the device nodes somehow and that will be
> > very likely based on e.g. entity names.
> > 
> > How should the documentation be arranged? The documentation is
> > arranged by fourccs currently; we'd probably need a separate section
> > for vendor specific formats. I think the device name should be listed
> > there as well.
>  
>  There already is a separate section for metadata formats:
>  
>  https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
>  
>  But perhaps that page should be organized by device. And with some
>  more detailed information on how to find the video node (i.e. entity
>  

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

On Fri, Jun 16, 2017 at 6:19 PM, Sakari Ailus  wrote:
> On Fri, Jun 16, 2017 at 06:03:13PM +0900, Tomasz Figa wrote:
>> On Fri, Jun 16, 2017 at 5:49 PM, Sakari Ailus  wrote:
>> > Hi Tomasz,
>> >
>> > On Fri, Jun 16, 2017 at 05:35:52PM +0900, Tomasz Figa wrote:
>> >> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus  wrote:
>> >> > Hi Tomasz,
>> >> >
>> >> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
>> >> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
>> >> >> Actually, there is one more thing, which would become possible with
>> >> >> switching to different queue types. If we have a device with queues
>> >> >> like this:
>> >> >> - video input,
>> >> >> - video output,
>> >> >> - parameters,
>> >> >> - statistics,
>> >> >> they could all be contained within one video node simply exposing 4
>> >> >> different queues. It would actually even allow an easy implementation
>> >> >
>> >> > The problem comes when you have multiple queues with the same type. I
>> >> > actually once proposed that (albeit for a slightly different purposes:
>> >> > streams) but the idea was rejected. It was decided to use separate video
>> >> > nodes instead.
>> >> >
>> >> >> of mem2mem, given that for mem2mem devices opening a video node means
>> >> >> creating a mem2mem context (while multiple video nodes would require
>> >> >> some special synchronization to map contexts together, which doesn't
>> >> >> exist as of today).
>> >> >
>> >> > V4L2 is very stream oriented and the mem2mem interface somewhat gets 
>> >> > around
>> >> > that. There are cases where at least partially changing per-frame
>> >> > configuration is needed in streaming cases as well. The request API is
>> >> > supposed to resolve these issues but it has become evident that the
>> >> > implementation is far from trivial.
>> >> >
>> >> > I'd rather like to have a more generic solution than a number of
>> >> > framework-lets that have their own semantics of the generic V4L2 IOCTLs 
>> >> > that
>> >> > only work with a particular kind of a device. Once there are new kind of
>> >> > devices, we'd need to implement another framework-let to support them.
>> >> >
>> >> > Add a CSI-2 receiver to the ImgU device and we'll need again something 
>> >> > very
>> >> > different...
>> >>
>> >> I need to think if Request API alone is really capable of solving this
>> >> problem, but if so, it would make sense indeed.
>> >
>> > What comes to this driver --- the request API could be beneficial, but the
>> > driver does not strictly need it. If there were controls that would need to
>> > be changed during streaming or if the device contained a CSI-2 receiver,
>> > then it'd be more important to have the request API.
>>
>> There is one use case, though, which can't be achieved easily with
>> current model - processing images for two cameras at the same time.
>> One could theoretically do all the S_FMT/S_WHATNOT magic every frame,
>> to process the cameras in a round robin fashion, but I'm not sure if
>> this would work really well in practice.
>
> That's true --- having to wait for all the buffers before configuring the
> formats would introduce some systematic delay which would decrease the total
> throughput. I'm not sure how much that would be though. The number of IOCTLs
> on each frame is big but then again IOCTLs are fast. The buffer memory isn't
> affected in any case. Process scheduling will be required though.

If we don't need to set any controls, we might be able to actually do
this without waiting, because all the processing is based on vb2
queues and all parameters could be latched into buffers at queuing
time.

Still, I believe it would actually require doing everything from one
process or some explicit userspace-based synchronization between
processes, because queuing one frame means actually queuing to
multiple queues, which is not atomic.

Best regards,
Tomasz

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Sakari Ailus]

On Fri, Jun 16, 2017 at 06:03:13PM +0900, Tomasz Figa wrote:
> On Fri, Jun 16, 2017 at 5:49 PM, Sakari Ailus  wrote:
> > Hi Tomasz,
> >
> > On Fri, Jun 16, 2017 at 05:35:52PM +0900, Tomasz Figa wrote:
> >> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus  wrote:
> >> > Hi Tomasz,
> >> >
> >> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
> >> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
> >> >> Actually, there is one more thing, which would become possible with
> >> >> switching to different queue types. If we have a device with queues
> >> >> like this:
> >> >> - video input,
> >> >> - video output,
> >> >> - parameters,
> >> >> - statistics,
> >> >> they could all be contained within one video node simply exposing 4
> >> >> different queues. It would actually even allow an easy implementation
> >> >
> >> > The problem comes when you have multiple queues with the same type. I
> >> > actually once proposed that (albeit for a slightly different purposes:
> >> > streams) but the idea was rejected. It was decided to use separate video
> >> > nodes instead.
> >> >
> >> >> of mem2mem, given that for mem2mem devices opening a video node means
> >> >> creating a mem2mem context (while multiple video nodes would require
> >> >> some special synchronization to map contexts together, which doesn't
> >> >> exist as of today).
> >> >
> >> > V4L2 is very stream oriented and the mem2mem interface somewhat gets 
> >> > around
> >> > that. There are cases where at least partially changing per-frame
> >> > configuration is needed in streaming cases as well. The request API is
> >> > supposed to resolve these issues but it has become evident that the
> >> > implementation is far from trivial.
> >> >
> >> > I'd rather like to have a more generic solution than a number of
> >> > framework-lets that have their own semantics of the generic V4L2 IOCTLs 
> >> > that
> >> > only work with a particular kind of a device. Once there are new kind of
> >> > devices, we'd need to implement another framework-let to support them.
> >> >
> >> > Add a CSI-2 receiver to the ImgU device and we'll need again something 
> >> > very
> >> > different...
> >>
> >> I need to think if Request API alone is really capable of solving this
> >> problem, but if so, it would make sense indeed.
> >
> > What comes to this driver --- the request API could be beneficial, but the
> > driver does not strictly need it. If there were controls that would need to
> > be changed during streaming or if the device contained a CSI-2 receiver,
> > then it'd be more important to have the request API.
> 
> There is one use case, though, which can't be achieved easily with
> current model - processing images for two cameras at the same time.
> One could theoretically do all the S_FMT/S_WHATNOT magic every frame,
> to process the cameras in a round robin fashion, but I'm not sure if
> this would work really well in practice.

That's true --- having to wait for all the buffers before configuring the
formats would introduce some systematic delay which would decrease the total
throughput. I'm not sure how much that would be though. The number of IOCTLs
on each frame is big but then again IOCTLs are fast. The buffer memory isn't
affected in any case. Process scheduling will be required though.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

On Fri, Jun 16, 2017 at 5:49 PM, Sakari Ailus  wrote:
> Hi Tomasz,
>
> On Fri, Jun 16, 2017 at 05:35:52PM +0900, Tomasz Figa wrote:
>> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus  wrote:
>> > Hi Tomasz,
>> >
>> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
>> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
>> >> Actually, there is one more thing, which would become possible with
>> >> switching to different queue types. If we have a device with queues
>> >> like this:
>> >> - video input,
>> >> - video output,
>> >> - parameters,
>> >> - statistics,
>> >> they could all be contained within one video node simply exposing 4
>> >> different queues. It would actually even allow an easy implementation
>> >
>> > The problem comes when you have multiple queues with the same type. I
>> > actually once proposed that (albeit for a slightly different purposes:
>> > streams) but the idea was rejected. It was decided to use separate video
>> > nodes instead.
>> >
>> >> of mem2mem, given that for mem2mem devices opening a video node means
>> >> creating a mem2mem context (while multiple video nodes would require
>> >> some special synchronization to map contexts together, which doesn't
>> >> exist as of today).
>> >
>> > V4L2 is very stream oriented and the mem2mem interface somewhat gets around
>> > that. There are cases where at least partially changing per-frame
>> > configuration is needed in streaming cases as well. The request API is
>> > supposed to resolve these issues but it has become evident that the
>> > implementation is far from trivial.
>> >
>> > I'd rather like to have a more generic solution than a number of
>> > framework-lets that have their own semantics of the generic V4L2 IOCTLs 
>> > that
>> > only work with a particular kind of a device. Once there are new kind of
>> > devices, we'd need to implement another framework-let to support them.
>> >
>> > Add a CSI-2 receiver to the ImgU device and we'll need again something very
>> > different...
>>
>> I need to think if Request API alone is really capable of solving this
>> problem, but if so, it would make sense indeed.
>
> What comes to this driver --- the request API could be beneficial, but the
> driver does not strictly need it. If there were controls that would need to
> be changed during streaming or if the device contained a CSI-2 receiver,
> then it'd be more important to have the request API.

There is one use case, though, which can't be achieved easily with
current model - processing images for two cameras at the same time.
One could theoretically do all the S_FMT/S_WHATNOT magic every frame,
to process the cameras in a round robin fashion, but I'm not sure if
this would work really well in practice.

Best regards,
Tomasz

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Sakari Ailus]

Hi Tomasz,

On Fri, Jun 16, 2017 at 05:35:52PM +0900, Tomasz Figa wrote:
> On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus  wrote:
> > Hi Tomasz,
> >
> > On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
> >> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
> >> > On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil  wrote:
> >> >> On 06/06/17 09:25, Sakari Ailus wrote:
> >> >>> Hi Tomasz,
> >> >>>
> >> >>> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
> >>  Uhm, +Laurent. Sorry for the noise.
> >> 
> >>  On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  
> >>  wrote:
> >> > Hi Yong,
> >> >
> >> > On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> >> >> Add the IPU3 specific processing parameter format
> >> >> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> >> >> for 3A and other statistics:
> >> >
> >> > Please see my comments inline.
> >> >
> >> >>
> >> >>   V4L2_META_FMT_IPU3_PARAMS
> >> >>   V4L2_META_FMT_IPU3_STAT_3A
> >> >>   V4L2_META_FMT_IPU3_STAT_DVS
> >> >>   V4L2_META_FMT_IPU3_STAT_LACE
> >> >>
> >> >> Signed-off-by: Yong Zhi 
> >> >> ---
> >> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
> >> >>  include/uapi/linux/videodev2.h   | 6 ++
> >> >>  2 files changed, 10 insertions(+)
> >> > [snip]
> >> >> +/* Vendor specific - used for IPU3 camera sub-system */
> >> >> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 
> >> >> 'p') /* IPU3 params */
> >> >> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 
> >> >> 's') /* IPU3 3A statistics */
> >> >> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 
> >> >> 'd') /* IPU3 DVS statistics */
> >> >> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 
> >> >> 'l') /* IPU3 LACE statistics */
> >> >
> >> > We had some discussion about this with Laurent and if I remember
> >> > correctly, the conclusion was that it might make sense to define one
> >> > FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for 
> >> > example,
> >> > and then have a V4L2-specific enum within the 
> >> > v4l2_pix_format(_mplane)
> >> > struct that specifies the exact vendor data type. It seems saner than
> >> > assigning a new FourCC whenever a new hardware revision comes out,
> >> > especially given that FourCCs tend to be used outside of the V4L2
> >> > world as well and being kind of (de facto) standardized (with 
> >> > existing
> >> > exceptions, unfortunately).
> >> >>
> >> >> I can't remember that discussion
> >> >
> >> > I think that was just a casual chat between Lauren, me and few more guys.
> >> >
> >> >> although I've had other discussions with
> >> >> Laurent related to this on how to handle formats that have many 
> >> >> variations
> >> >> on a theme.
> >> >>
> >> >> But speaking for this specific case I see no reason to do something 
> >> >> special.
> >> >> There are only four new formats, which seems perfectly reasonable to me.
> >> >>
> >> >> I don't see the advantage of adding another layer of pixel formats. You 
> >> >> still
> >> >> need to define something for this, one way or the other. And this way 
> >> >> doesn't
> >> >> require API changes.
> >> >>
> >> >>> If we have four video nodes with different vendor specific formats, 
> >> >>> how does
> >> >>> the user tell the formats apart? I presume the user space could use the
> >> >>> entity names for instance, but that would essentially make them device
> >> >>> specific.
> >> >>
> >> >> Well, they are. There really is no way to avoid that.
> >> >>
> >> >>> I'm not sure if there would be any harm from that in practice though: 
> >> >>> the
> >> >>> user will need to find the device nodes somehow and that will be very 
> >> >>> likely
> >> >>> based on e.g. entity names.
> >> >>>
> >> >>> How should the documentation be arranged? The documentation is 
> >> >>> arranged by
> >> >>> fourccs currently; we'd probably need a separate section for vendor 
> >> >>> specific
> >> >>> formats. I think the device name should be listed there as well.
> >> >>
> >> >> There already is a separate section for metadata formats:
> >> >>
> >> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
> >> >>
> >> >> But perhaps that page should be organized by device. And with some more
> >> >> detailed information on how to find the video node (i.e. entity names).
> >> >>
> >> >>> I'd like to have perhaps Hans's comment on that as well.
> >> >>>
> >> >>> I don't really see a drawback in the current way of doing this either; 
> >> >>> we
> >> >>> may get a few new fourcc codes occasionally of which I'm not really 
> >> >>> worried
> >> >>> about. --- I'd rather ask why should there be an exception on how 
> >> 

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

On Fri, Jun 16, 2017 at 5:25 PM, Sakari Ailus  wrote:
> Hi Tomasz,
>
> On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
>> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
>> > On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil  wrote:
>> >> On 06/06/17 09:25, Sakari Ailus wrote:
>> >>> Hi Tomasz,
>> >>>
>> >>> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
>>  Uhm, +Laurent. Sorry for the noise.
>> 
>>  On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
>> > Hi Yong,
>> >
>> > On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
>> >> Add the IPU3 specific processing parameter format
>> >> V4L2_META_FMT_IPU3_PARAMS and metadata formats
>> >> for 3A and other statistics:
>> >
>> > Please see my comments inline.
>> >
>> >>
>> >>   V4L2_META_FMT_IPU3_PARAMS
>> >>   V4L2_META_FMT_IPU3_STAT_3A
>> >>   V4L2_META_FMT_IPU3_STAT_DVS
>> >>   V4L2_META_FMT_IPU3_STAT_LACE
>> >>
>> >> Signed-off-by: Yong Zhi 
>> >> ---
>> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>> >>  include/uapi/linux/videodev2.h   | 6 ++
>> >>  2 files changed, 10 insertions(+)
>> > [snip]
>> >> +/* Vendor specific - used for IPU3 camera sub-system */
>> >> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 
>> >> 'p') /* IPU3 params */
>> >> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 
>> >> 's') /* IPU3 3A statistics */
>> >> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 
>> >> 'd') /* IPU3 DVS statistics */
>> >> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 
>> >> 'l') /* IPU3 LACE statistics */
>> >
>> > We had some discussion about this with Laurent and if I remember
>> > correctly, the conclusion was that it might make sense to define one
>> > FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
>> > and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
>> > struct that specifies the exact vendor data type. It seems saner than
>> > assigning a new FourCC whenever a new hardware revision comes out,
>> > especially given that FourCCs tend to be used outside of the V4L2
>> > world as well and being kind of (de facto) standardized (with existing
>> > exceptions, unfortunately).
>> >>
>> >> I can't remember that discussion
>> >
>> > I think that was just a casual chat between Lauren, me and few more guys.
>> >
>> >> although I've had other discussions with
>> >> Laurent related to this on how to handle formats that have many variations
>> >> on a theme.
>> >>
>> >> But speaking for this specific case I see no reason to do something 
>> >> special.
>> >> There are only four new formats, which seems perfectly reasonable to me.
>> >>
>> >> I don't see the advantage of adding another layer of pixel formats. You 
>> >> still
>> >> need to define something for this, one way or the other. And this way 
>> >> doesn't
>> >> require API changes.
>> >>
>> >>> If we have four video nodes with different vendor specific formats, how 
>> >>> does
>> >>> the user tell the formats apart? I presume the user space could use the
>> >>> entity names for instance, but that would essentially make them device
>> >>> specific.
>> >>
>> >> Well, they are. There really is no way to avoid that.
>> >>
>> >>> I'm not sure if there would be any harm from that in practice though: the
>> >>> user will need to find the device nodes somehow and that will be very 
>> >>> likely
>> >>> based on e.g. entity names.
>> >>>
>> >>> How should the documentation be arranged? The documentation is arranged 
>> >>> by
>> >>> fourccs currently; we'd probably need a separate section for vendor 
>> >>> specific
>> >>> formats. I think the device name should be listed there as well.
>> >>
>> >> There already is a separate section for metadata formats:
>> >>
>> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
>> >>
>> >> But perhaps that page should be organized by device. And with some more
>> >> detailed information on how to find the video node (i.e. entity names).
>> >>
>> >>> I'd like to have perhaps Hans's comment on that as well.
>> >>>
>> >>> I don't really see a drawback in the current way of doing this either; we
>> >>> may get a few new fourcc codes occasionally of which I'm not really 
>> >>> worried
>> >>> about. --- I'd rather ask why should there be an exception on how vendor
>> >>> specific formats are defined. And if we do make an exception, then how do
>> >>> you decide which one is and isn't vendor specific? There are raw bayer
>> >>> format variants that are just raw bayer data but the pixels are arranged
>> >>> differently (e.g. CIO2 driver).
>> >>>
>> >>
>> >> For these unique formats I am happy with the way it is 

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Sakari Ailus]

Hi Tomasz,

On Fri, Jun 16, 2017 at 02:52:07PM +0900, Tomasz Figa wrote:
> On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
> > On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil  wrote:
> >> On 06/06/17 09:25, Sakari Ailus wrote:
> >>> Hi Tomasz,
> >>>
> >>> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
>  Uhm, +Laurent. Sorry for the noise.
> 
>  On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
> > Hi Yong,
> >
> > On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> >> Add the IPU3 specific processing parameter format
> >> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> >> for 3A and other statistics:
> >
> > Please see my comments inline.
> >
> >>
> >>   V4L2_META_FMT_IPU3_PARAMS
> >>   V4L2_META_FMT_IPU3_STAT_3A
> >>   V4L2_META_FMT_IPU3_STAT_DVS
> >>   V4L2_META_FMT_IPU3_STAT_LACE
> >>
> >> Signed-off-by: Yong Zhi 
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
> >>  include/uapi/linux/videodev2.h   | 6 ++
> >>  2 files changed, 10 insertions(+)
> > [snip]
> >> +/* Vendor specific - used for IPU3 camera sub-system */
> >> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 
> >> 'p') /* IPU3 params */
> >> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 
> >> 's') /* IPU3 3A statistics */
> >> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 
> >> 'd') /* IPU3 DVS statistics */
> >> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 
> >> 'l') /* IPU3 LACE statistics */
> >
> > We had some discussion about this with Laurent and if I remember
> > correctly, the conclusion was that it might make sense to define one
> > FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
> > and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
> > struct that specifies the exact vendor data type. It seems saner than
> > assigning a new FourCC whenever a new hardware revision comes out,
> > especially given that FourCCs tend to be used outside of the V4L2
> > world as well and being kind of (de facto) standardized (with existing
> > exceptions, unfortunately).
> >>
> >> I can't remember that discussion
> >
> > I think that was just a casual chat between Lauren, me and few more guys.
> >
> >> although I've had other discussions with
> >> Laurent related to this on how to handle formats that have many variations
> >> on a theme.
> >>
> >> But speaking for this specific case I see no reason to do something 
> >> special.
> >> There are only four new formats, which seems perfectly reasonable to me.
> >>
> >> I don't see the advantage of adding another layer of pixel formats. You 
> >> still
> >> need to define something for this, one way or the other. And this way 
> >> doesn't
> >> require API changes.
> >>
> >>> If we have four video nodes with different vendor specific formats, how 
> >>> does
> >>> the user tell the formats apart? I presume the user space could use the
> >>> entity names for instance, but that would essentially make them device
> >>> specific.
> >>
> >> Well, they are. There really is no way to avoid that.
> >>
> >>> I'm not sure if there would be any harm from that in practice though: the
> >>> user will need to find the device nodes somehow and that will be very 
> >>> likely
> >>> based on e.g. entity names.
> >>>
> >>> How should the documentation be arranged? The documentation is arranged by
> >>> fourccs currently; we'd probably need a separate section for vendor 
> >>> specific
> >>> formats. I think the device name should be listed there as well.
> >>
> >> There already is a separate section for metadata formats:
> >>
> >> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
> >>
> >> But perhaps that page should be organized by device. And with some more
> >> detailed information on how to find the video node (i.e. entity names).
> >>
> >>> I'd like to have perhaps Hans's comment on that as well.
> >>>
> >>> I don't really see a drawback in the current way of doing this either; we
> >>> may get a few new fourcc codes occasionally of which I'm not really 
> >>> worried
> >>> about. --- I'd rather ask why should there be an exception on how vendor
> >>> specific formats are defined. And if we do make an exception, then how do
> >>> you decide which one is and isn't vendor specific? There are raw bayer
> >>> format variants that are just raw bayer data but the pixels are arranged
> >>> differently (e.g. CIO2 driver).
> >>>
> >>
> >> For these unique formats I am happy with the way it is today. The problem
> >> is more with 'parameterized' formats. A simple example would be the 4:2:2
> >> interleaved YUV formats where you have four different ways of ordering the
> >> Y, U and V 

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

On Tue, Jun 6, 2017 at 7:09 PM, Tomasz Figa  wrote:
> On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil  wrote:
>> On 06/06/17 09:25, Sakari Ailus wrote:
>>> Hi Tomasz,
>>>
>>> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
 Uhm, +Laurent. Sorry for the noise.

 On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
> Hi Yong,
>
> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
>> Add the IPU3 specific processing parameter format
>> V4L2_META_FMT_IPU3_PARAMS and metadata formats
>> for 3A and other statistics:
>
> Please see my comments inline.
>
>>
>>   V4L2_META_FMT_IPU3_PARAMS
>>   V4L2_META_FMT_IPU3_STAT_3A
>>   V4L2_META_FMT_IPU3_STAT_DVS
>>   V4L2_META_FMT_IPU3_STAT_LACE
>>
>> Signed-off-by: Yong Zhi 
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>>  include/uapi/linux/videodev2.h   | 6 ++
>>  2 files changed, 10 insertions(+)
> [snip]
>> +/* Vendor specific - used for IPU3 camera sub-system */
>> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') 
>> /* IPU3 params */
>> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') 
>> /* IPU3 3A statistics */
>> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') 
>> /* IPU3 DVS statistics */
>> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') 
>> /* IPU3 LACE statistics */
>
> We had some discussion about this with Laurent and if I remember
> correctly, the conclusion was that it might make sense to define one
> FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
> and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
> struct that specifies the exact vendor data type. It seems saner than
> assigning a new FourCC whenever a new hardware revision comes out,
> especially given that FourCCs tend to be used outside of the V4L2
> world as well and being kind of (de facto) standardized (with existing
> exceptions, unfortunately).
>>
>> I can't remember that discussion
>
> I think that was just a casual chat between Lauren, me and few more guys.
>
>> although I've had other discussions with
>> Laurent related to this on how to handle formats that have many variations
>> on a theme.
>>
>> But speaking for this specific case I see no reason to do something special.
>> There are only four new formats, which seems perfectly reasonable to me.
>>
>> I don't see the advantage of adding another layer of pixel formats. You still
>> need to define something for this, one way or the other. And this way doesn't
>> require API changes.
>>
>>> If we have four video nodes with different vendor specific formats, how does
>>> the user tell the formats apart? I presume the user space could use the
>>> entity names for instance, but that would essentially make them device
>>> specific.
>>
>> Well, they are. There really is no way to avoid that.
>>
>>> I'm not sure if there would be any harm from that in practice though: the
>>> user will need to find the device nodes somehow and that will be very likely
>>> based on e.g. entity names.
>>>
>>> How should the documentation be arranged? The documentation is arranged by
>>> fourccs currently; we'd probably need a separate section for vendor specific
>>> formats. I think the device name should be listed there as well.
>>
>> There already is a separate section for metadata formats:
>>
>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
>>
>> But perhaps that page should be organized by device. And with some more
>> detailed information on how to find the video node (i.e. entity names).
>>
>>> I'd like to have perhaps Hans's comment on that as well.
>>>
>>> I don't really see a drawback in the current way of doing this either; we
>>> may get a few new fourcc codes occasionally of which I'm not really worried
>>> about. --- I'd rather ask why should there be an exception on how vendor
>>> specific formats are defined. And if we do make an exception, then how do
>>> you decide which one is and isn't vendor specific? There are raw bayer
>>> format variants that are just raw bayer data but the pixels are arranged
>>> differently (e.g. CIO2 driver).
>>>
>>
>> For these unique formats I am happy with the way it is today. The problem
>> is more with 'parameterized' formats. A simple example would be the 4:2:2
>> interleaved YUV formats where you have four different ways of ordering the
>> Y, U and V components. Right now we have four defines for that, but things
>> get out of hand quickly when you have multiple parameters like that.
>>
>> Laurent and myself discussed that with NVidia some time ago, without
>> reaching a clear conclusion. Mostly because we couldn't come up with an
>> API that is simple enough.
>
> 

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Sakari Ailus]

Hi Alan,

On Mon, Jun 05, 2017 at 09:43:27PM +0100, Alan Cox wrote:
> On Mon,  5 Jun 2017 15:39:06 -0500
> Yong Zhi  wrote:
> 
> > Add the IPU3 specific processing parameter format
> > V4L2_META_FMT_IPU3_PARAMS and metadata formats
> > for 3A and other statistics:
> > 
> >   V4L2_META_FMT_IPU3_PARAMS
> >   V4L2_META_FMT_IPU3_STAT_3A
> >   V4L2_META_FMT_IPU3_STAT_DVS
> >   V4L2_META_FMT_IPU3_STAT_LACE
> 
> Are these specific to IPU v3 or do they match other IPU versions ?

The parameters (V4L2_META_FMT_IPU3_PARAMS) are bound to be different (for
previous versions had a private IOCTL to pass them). There could be
similarities between different versions of the IPUs but they're still not
exactly the same. The hardware tends to change quite a bit between the
generations.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

On Tue, Jun 6, 2017 at 5:04 PM, Hans Verkuil  wrote:
> On 06/06/17 09:25, Sakari Ailus wrote:
>> Hi Tomasz,
>>
>> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
>>> Uhm, +Laurent. Sorry for the noise.
>>>
>>> On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
 Hi Yong,

 On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> Add the IPU3 specific processing parameter format
> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> for 3A and other statistics:

 Please see my comments inline.

>
>   V4L2_META_FMT_IPU3_PARAMS
>   V4L2_META_FMT_IPU3_STAT_3A
>   V4L2_META_FMT_IPU3_STAT_DVS
>   V4L2_META_FMT_IPU3_STAT_LACE
>
> Signed-off-by: Yong Zhi 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>  include/uapi/linux/videodev2.h   | 6 ++
>  2 files changed, 10 insertions(+)
 [snip]
> +/* Vendor specific - used for IPU3 camera sub-system */
> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') 
> /* IPU3 params */
> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') 
> /* IPU3 3A statistics */
> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') 
> /* IPU3 DVS statistics */
> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') 
> /* IPU3 LACE statistics */

 We had some discussion about this with Laurent and if I remember
 correctly, the conclusion was that it might make sense to define one
 FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
 and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
 struct that specifies the exact vendor data type. It seems saner than
 assigning a new FourCC whenever a new hardware revision comes out,
 especially given that FourCCs tend to be used outside of the V4L2
 world as well and being kind of (de facto) standardized (with existing
 exceptions, unfortunately).
>
> I can't remember that discussion

I think that was just a casual chat between Lauren, me and few more guys.

> although I've had other discussions with
> Laurent related to this on how to handle formats that have many variations
> on a theme.
>
> But speaking for this specific case I see no reason to do something special.
> There are only four new formats, which seems perfectly reasonable to me.
>
> I don't see the advantage of adding another layer of pixel formats. You still
> need to define something for this, one way or the other. And this way doesn't
> require API changes.
>
>> If we have four video nodes with different vendor specific formats, how does
>> the user tell the formats apart? I presume the user space could use the
>> entity names for instance, but that would essentially make them device
>> specific.
>
> Well, they are. There really is no way to avoid that.
>
>> I'm not sure if there would be any harm from that in practice though: the
>> user will need to find the device nodes somehow and that will be very likely
>> based on e.g. entity names.
>>
>> How should the documentation be arranged? The documentation is arranged by
>> fourccs currently; we'd probably need a separate section for vendor specific
>> formats. I think the device name should be listed there as well.
>
> There already is a separate section for metadata formats:
>
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html
>
> But perhaps that page should be organized by device. And with some more
> detailed information on how to find the video node (i.e. entity names).
>
>> I'd like to have perhaps Hans's comment on that as well.
>>
>> I don't really see a drawback in the current way of doing this either; we
>> may get a few new fourcc codes occasionally of which I'm not really worried
>> about. --- I'd rather ask why should there be an exception on how vendor
>> specific formats are defined. And if we do make an exception, then how do
>> you decide which one is and isn't vendor specific? There are raw bayer
>> format variants that are just raw bayer data but the pixels are arranged
>> differently (e.g. CIO2 driver).
>>
>
> For these unique formats I am happy with the way it is today. The problem
> is more with 'parameterized' formats. A simple example would be the 4:2:2
> interleaved YUV formats where you have four different ways of ordering the
> Y, U and V components. Right now we have four defines for that, but things
> get out of hand quickly when you have multiple parameters like that.
>
> Laurent and myself discussed that with NVidia some time ago, without
> reaching a clear conclusion. Mostly because we couldn't come up with an
> API that is simple enough.

Actually I back off a bit. Still, it looks like we have a metadata
interface already, but it's limited to CAPTURE:

https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-meta.html#metadata

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Hans Verkuil]

On 06/06/17 09:25, Sakari Ailus wrote:
> Hi Tomasz,
> 
> On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
>> Uhm, +Laurent. Sorry for the noise.
>>
>> On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
>>> Hi Yong,
>>>
>>> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
 Add the IPU3 specific processing parameter format
 V4L2_META_FMT_IPU3_PARAMS and metadata formats
 for 3A and other statistics:
>>>
>>> Please see my comments inline.
>>>

   V4L2_META_FMT_IPU3_PARAMS
   V4L2_META_FMT_IPU3_STAT_3A
   V4L2_META_FMT_IPU3_STAT_DVS
   V4L2_META_FMT_IPU3_STAT_LACE

 Signed-off-by: Yong Zhi 
 ---
  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
  include/uapi/linux/videodev2.h   | 6 ++
  2 files changed, 10 insertions(+)
>>> [snip]
 +/* Vendor specific - used for IPU3 camera sub-system */
 +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') /* 
 IPU3 params */
 +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* 
 IPU3 3A statistics */
 +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') /* 
 IPU3 DVS statistics */
 +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') /* 
 IPU3 LACE statistics */
>>>
>>> We had some discussion about this with Laurent and if I remember
>>> correctly, the conclusion was that it might make sense to define one
>>> FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
>>> and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
>>> struct that specifies the exact vendor data type. It seems saner than
>>> assigning a new FourCC whenever a new hardware revision comes out,
>>> especially given that FourCCs tend to be used outside of the V4L2
>>> world as well and being kind of (de facto) standardized (with existing
>>> exceptions, unfortunately).

I can't remember that discussion, although I've had other discussions with
Laurent related to this on how to handle formats that have many variations
on a theme.

But speaking for this specific case I see no reason to do something special.
There are only four new formats, which seems perfectly reasonable to me.

I don't see the advantage of adding another layer of pixel formats. You still
need to define something for this, one way or the other. And this way doesn't
require API changes.

> If we have four video nodes with different vendor specific formats, how does
> the user tell the formats apart? I presume the user space could use the
> entity names for instance, but that would essentially make them device
> specific.

Well, they are. There really is no way to avoid that.

> I'm not sure if there would be any harm from that in practice though: the
> user will need to find the device nodes somehow and that will be very likely
> based on e.g. entity names.
> 
> How should the documentation be arranged? The documentation is arranged by
> fourccs currently; we'd probably need a separate section for vendor specific
> formats. I think the device name should be listed there as well.

There already is a separate section for metadata formats:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/meta-formats.html

But perhaps that page should be organized by device. And with some more
detailed information on how to find the video node (i.e. entity names).

> I'd like to have perhaps Hans's comment on that as well.
> 
> I don't really see a drawback in the current way of doing this either; we
> may get a few new fourcc codes occasionally of which I'm not really worried
> about. --- I'd rather ask why should there be an exception on how vendor
> specific formats are defined. And if we do make an exception, then how do
> you decide which one is and isn't vendor specific? There are raw bayer
> format variants that are just raw bayer data but the pixels are arranged
> differently (e.g. CIO2 driver).
> 

For these unique formats I am happy with the way it is today. The problem
is more with 'parameterized' formats. A simple example would be the 4:2:2
interleaved YUV formats where you have four different ways of ordering the
Y, U and V components. Right now we have four defines for that, but things
get out of hand quickly when you have multiple parameters like that.

Laurent and myself discussed that with NVidia some time ago, without
reaching a clear conclusion. Mostly because we couldn't come up with an
API that is simple enough.

Regards,

Hans

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Sakari Ailus]

Hi Tomasz,

On Tue, Jun 06, 2017 at 01:30:41PM +0900, Tomasz Figa wrote:
> Uhm, +Laurent. Sorry for the noise.
> 
> On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
> > Hi Yong,
> >
> > On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> >> Add the IPU3 specific processing parameter format
> >> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> >> for 3A and other statistics:
> >
> > Please see my comments inline.
> >
> >>
> >>   V4L2_META_FMT_IPU3_PARAMS
> >>   V4L2_META_FMT_IPU3_STAT_3A
> >>   V4L2_META_FMT_IPU3_STAT_DVS
> >>   V4L2_META_FMT_IPU3_STAT_LACE
> >>
> >> Signed-off-by: Yong Zhi 
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
> >>  include/uapi/linux/videodev2.h   | 6 ++
> >>  2 files changed, 10 insertions(+)
> > [snip]
> >> +/* Vendor specific - used for IPU3 camera sub-system */
> >> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') /* 
> >> IPU3 params */
> >> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* 
> >> IPU3 3A statistics */
> >> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') /* 
> >> IPU3 DVS statistics */
> >> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') /* 
> >> IPU3 LACE statistics */
> >
> > We had some discussion about this with Laurent and if I remember
> > correctly, the conclusion was that it might make sense to define one
> > FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
> > and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
> > struct that specifies the exact vendor data type. It seems saner than
> > assigning a new FourCC whenever a new hardware revision comes out,
> > especially given that FourCCs tend to be used outside of the V4L2
> > world as well and being kind of (de facto) standardized (with existing
> > exceptions, unfortunately).

If we have four video nodes with different vendor specific formats, how does
the user tell the formats apart? I presume the user space could use the
entity names for instance, but that would essentially make them device
specific.

I'm not sure if there would be any harm from that in practice though: the
user will need to find the device nodes somehow and that will be very likely
based on e.g. entity names.

How should the documentation be arranged? The documentation is arranged by
fourccs currently; we'd probably need a separate section for vendor specific
formats. I think the device name should be listed there as well.

I'd like to have perhaps Hans's comment on that as well.

I don't really see a drawback in the current way of doing this either; we
may get a few new fourcc codes occasionally of which I'm not really worried
about. --- I'd rather ask why should there be an exception on how vendor
specific formats are defined. And if we do make an exception, then how do
you decide which one is and isn't vendor specific? There are raw bayer
format variants that are just raw bayer data but the pixels are arranged
differently (e.g. CIO2 driver).

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

Uhm, +Laurent. Sorry for the noise.

On Tue, Jun 6, 2017 at 1:30 PM, Tomasz Figa  wrote:
> Hi Yong,
>
> On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
>> Add the IPU3 specific processing parameter format
>> V4L2_META_FMT_IPU3_PARAMS and metadata formats
>> for 3A and other statistics:
>
> Please see my comments inline.
>
>>
>>   V4L2_META_FMT_IPU3_PARAMS
>>   V4L2_META_FMT_IPU3_STAT_3A
>>   V4L2_META_FMT_IPU3_STAT_DVS
>>   V4L2_META_FMT_IPU3_STAT_LACE
>>
>> Signed-off-by: Yong Zhi 
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>>  include/uapi/linux/videodev2.h   | 6 ++
>>  2 files changed, 10 insertions(+)
> [snip]
>> +/* Vendor specific - used for IPU3 camera sub-system */
>> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') /* 
>> IPU3 params */
>> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* 
>> IPU3 3A statistics */
>> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') /* 
>> IPU3 DVS statistics */
>> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') /* 
>> IPU3 LACE statistics */
>
> We had some discussion about this with Laurent and if I remember
> correctly, the conclusion was that it might make sense to define one
> FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
> and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
> struct that specifies the exact vendor data type. It seems saner than
> assigning a new FourCC whenever a new hardware revision comes out,
> especially given that FourCCs tend to be used outside of the V4L2
> world as well and being kind of (de facto) standardized (with existing
> exceptions, unfortunately).
>
> Best regards,
> Tomasz

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Tomasz Figa]

Hi Yong,

On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> Add the IPU3 specific processing parameter format
> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> for 3A and other statistics:

Please see my comments inline.

>
>   V4L2_META_FMT_IPU3_PARAMS
>   V4L2_META_FMT_IPU3_STAT_3A
>   V4L2_META_FMT_IPU3_STAT_DVS
>   V4L2_META_FMT_IPU3_STAT_LACE
>
> Signed-off-by: Yong Zhi 
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 4 
>  include/uapi/linux/videodev2.h   | 6 ++
>  2 files changed, 10 insertions(+)
[snip]
> +/* Vendor specific - used for IPU3 camera sub-system */
> +#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') /* 
> IPU3 params */
> +#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* 
> IPU3 3A statistics */
> +#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') /* 
> IPU3 DVS statistics */
> +#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') /* 
> IPU3 LACE statistics */

We had some discussion about this with Laurent and if I remember
correctly, the conclusion was that it might make sense to define one
FourCC for a vendor specific format, ('v', 'n', 'd', 'r') for example,
and then have a V4L2-specific enum within the v4l2_pix_format(_mplane)
struct that specifies the exact vendor data type. It seems saner than
assigning a new FourCC whenever a new hardware revision comes out,
especially given that FourCCs tend to be used outside of the V4L2
world as well and being kind of (de facto) standardized (with existing
exceptions, unfortunately).

Best regards,
Tomasz

Re: [PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Alan Cox]

On Mon,  5 Jun 2017 15:39:06 -0500
Yong Zhi  wrote:

> Add the IPU3 specific processing parameter format
> V4L2_META_FMT_IPU3_PARAMS and metadata formats
> for 3A and other statistics:
> 
>   V4L2_META_FMT_IPU3_PARAMS
>   V4L2_META_FMT_IPU3_STAT_3A
>   V4L2_META_FMT_IPU3_STAT_DVS
>   V4L2_META_FMT_IPU3_STAT_LACE

Are these specific to IPU v3 or do they match other IPU versions ?

Alan

[PATCH 01/12] videodev2.h, v4l2-ioctl: add IPU3 meta buffer format

[Yong Zhi]

Add the IPU3 specific processing parameter format
V4L2_META_FMT_IPU3_PARAMS and metadata formats
for 3A and other statistics:

  V4L2_META_FMT_IPU3_PARAMS
  V4L2_META_FMT_IPU3_STAT_3A
  V4L2_META_FMT_IPU3_STAT_DVS
  V4L2_META_FMT_IPU3_STAT_LACE

Signed-off-by: Yong Zhi 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 4 
 include/uapi/linux/videodev2.h   | 6 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index fb1387f..919aa24 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1239,6 +1239,10 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_TCH_FMT_TU08: descr = "8-bit unsigned touch data"; 
break;
case V4L2_META_FMT_VSP1_HGO:descr = "R-Car VSP1 1-D Histogram"; 
break;
case V4L2_META_FMT_VSP1_HGT:descr = "R-Car VSP1 2-D Histogram"; 
break;
+   case V4L2_META_FMT_IPU3_PARAMS: descr = "IPU3 processing parameters"; 
break;
+   case V4L2_META_FMT_IPU3_STAT_3A:descr = "IPU3 3A statistics"; 
break;
+   case V4L2_META_FMT_IPU3_STAT_DVS:   descr = "IPU3 DVS statistics"; 
break;
+   case V4L2_META_FMT_IPU3_STAT_LACE:  descr = "IPU3 LACE statistics"; 
break;
 
default:
/* Compressed formats */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 7bfa6ad..fab8822 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -685,6 +685,12 @@ struct v4l2_pix_format {
 #define V4L2_META_FMT_VSP1_HGOv4l2_fourcc('V', 'S', 'P', 'H') /* R-Car 
VSP1 1-D Histogram */
 #define V4L2_META_FMT_VSP1_HGTv4l2_fourcc('V', 'S', 'P', 'T') /* R-Car 
VSP1 2-D Histogram */
 
+/* Vendor specific - used for IPU3 camera sub-system */
+#define V4L2_META_FMT_IPU3_PARAMS  v4l2_fourcc('i', 'p', '3', 'p') /* IPU3 
params */
+#define V4L2_META_FMT_IPU3_STAT_3A v4l2_fourcc('i', 'p', '3', 's') /* IPU3 
3A statistics */
+#define V4L2_META_FMT_IPU3_STAT_DVSv4l2_fourcc('i', 'p', '3', 'd') /* IPU3 
DVS statistics */
+#define V4L2_META_FMT_IPU3_STAT_LACE   v4l2_fourcc('i', 'p', '3', 'l') /* IPU3 
LACE statistics */
+
 /* priv field value to indicates that subsequent fields are valid. */
 #define V4L2_PIX_FMT_PRIV_MAGIC0xfeedcafe
 
-- 
2.7.4