Re: [PATCH/RFC v2 0/4] Meta-data video device type

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Friday 13 May 2016 11:26:22 Hans Verkuil wrote:
> On 05/12/2016 02:17 AM, Laurent Pinchart wrote:
> > Hello,
> > 
> > This RFC patch series is a second attempt at adding support for passing
> > statistics data to userspace using a standard API.
> > 
> > The core requirements haven't changed. Statistics data capture requires
> > zero-copy and decoupling statistics buffers from images buffers, in order
> > to make statistics data available to userspace as soon as they're
> > captured. For those reasons the early consensus we have reached is to use
> > a video device node with a buffer queue to pass statistics buffers using
> > the V4L2 API, and this new RFC version doesn't challenge that.
> > 
> > The major change compared to the previous version is how the first patch
> > has been split in two. Patch 1/4 now adds a new metadata buffer type and
> > format (including their support in videobuf2-v4l2), usable with regular
> > V4L2 video device nodes, while patch 2/4 adds the new metadata video
> > device type. Metadata buffer queues are thus usable on both the regular
> > V4L2 device nodes and the new metadata device nodes.
> > 
> > This change was driven by the fact that an important category of use cases
> > doesn't differentiate between metadata and image data in hardware at the
> > DMA engine level. With such hardware (CSI-2 receivers in particular, but
> > other bus types could also fall into this category) a stream containing
> > both metadata and image data virtual streams is transmitted over a single
> > physical link. The receiver demultiplexes, filters and routes the virtual
> > streams to further hardware blocks, and in many cases, directly to DMA
> > engines that are part of the receiver. Those DMA engines can capture a
> > single virtual stream to memory, with as many DMA engines physically
> > present in the device as the number of virtual streams that can be
> > captured concurrently. All those DMA engines are usually identical and
> > don't care about the type of data they receive and capture. For that
> > reason limiting the metadata buffer type to metadata device nodes would
> > require creating two device nodes for each DMA engine (and possibly more
> > later if we need to capture other types of data). Not only would this
> > make the API more complex to use for applications, it wouldn't bring any
> > added value as the video and metadata device nodes associated with a DMA
> > engine couldn't be used concurrently anyway, as they both correspond to
> > the same hardware resource.
> > 
> > For this reason the ability to capture metadata on a video device node is
> > useful and desired, and is implemented patch 1/4 using a dedicated video
> > buffers queue. In the CSI-2 case a driver will create two buffer queues
> > internally for the same DMA engine, and can select which one to use based
> > on the buffer type passed for instance to the REQBUFS ioctl (details
> > still need to be discussed here).
> 
> Not quite. It still has only one vb2_queue, you just change the type
> depending on what mode it is in (video or meta data). Similar to raw vs
> sliced VBI.
> 
> In the latter case it is the VIDIOC_S_FMT call that changes the vb2_queue
> type depending on whether raw or sliced VBI is requested. That's probably
> where I would do this for video vs meta as well.

That sounds good to me. I didn't know we had support for changing the type of 
a vb2 queue at runtime, that's good news :-)

> There is one big thing missing here: how does userspace know in this case
> whether it will get metadata or video? Who decides which CSI virtual stream
> is routed to which video node?

I've replied to Sakari's e-mail about this.

> > A device that contains DMA engines dedicated to
> > metadata would create a single buffer queue and implement metadata capture
> > only.
> > 
> > Patch 2/4 then adds a dedicated metadata device node type that is limited
> > to metadata capture. Support for metadata on video device nodes isn't
> > removed though, both device node types support metadata capture. I have
> > included this patch as the code existed in the previous version of the
> > series (and was explicitly requested during review of an earlier
> > version), but I don't really see what value this would bring compared to
> > just using video device nodes.
> 
> I'm inclined to agree with you.

Great :-) Should I just drop patch 2/4 then ? Sakari, Mauro, would that be 
fine with you ?

> > As before patch 3/4 defines a first metadata format for the R-Car VSP1 1-D
> > statistics format as an example, and the new patch 4/4 adds support for
> > the histogram engine to the VSP1 driver. The implementation uses a
> > metadata device node, and switching to a video device node wouldn't
> > require more than applying the following one-liner patch.
> > 
> > -       histo->queue.type = V4L2_BUF_TYPE_META_CAPTURE;
> > +       histo->queue.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> 
> You probably mean replacing this:
> 
> 	histo->video.vfl_type = VFL_TYPE_META;
> 
> by this:
> 
> 	histo->video.vfl_type = VFL_TYPE_GRABBER;

Yes, of course, my bad.

> > Beside whether patch 2/4 should be included or not (I would prefer
> > dropping it) and how to select the active queue on a multi-type video
> > device node (through the REQBUFS ioctl or through a diffent mean), one
> > point that remains to be discussed is what information to include in the
> > metadata format. Patch 1/1 defines the new metadata format as
> > 
> > struct v4l2_meta_format {
> > 	__u32				dataformat;
> > 	__u32				buffersize;
> > 	__u8				reserved[24];
> > } __attribute__ ((packed));
> > 
> > but at least in the CSI-2 case metadata is, as image data, transmitted in
> > lines and the receiver needs to be programmed with the line length and the
> > number of lines for proper operation. We started discussing this on IRC
> > but haven't reached a conclusion yet.

This is the last problem that needs to be solved. We can possibly postpone it 
as I don't need width/height for now.

> > Laurent Pinchart (4):
> >   v4l: Add metadata buffer type and format
> >   v4l: Add metadata video device type
> >   v4l: Define a pixel format for the R-Car VSP1 1-D histogram engine
> >   v4l: vsp1: Add HGO support
> >  
> >  Documentation/DocBook/media/v4l/dev-meta.xml       |  97 ++++
> >  .../DocBook/media/v4l/pixfmt-meta-vsp1-hgo.xml     | 307 +++++++++++++
> >  Documentation/DocBook/media/v4l/pixfmt.xml         |   9 +
> >  Documentation/DocBook/media/v4l/v4l2.xml           |   1 +
> >  drivers/media/platform/Kconfig                     |   1 +
> >  drivers/media/platform/vsp1/Makefile               |   2 +
> >  drivers/media/platform/vsp1/vsp1.h                 |   3 +
> >  drivers/media/platform/vsp1/vsp1_drm.c             |   2 +-
> >  drivers/media/platform/vsp1/vsp1_drv.c             |  37 +-
> >  drivers/media/platform/vsp1/vsp1_entity.c          | 131 +++++-
> >  drivers/media/platform/vsp1/vsp1_entity.h          |   7 +-
> >  drivers/media/platform/vsp1/vsp1_hgo.c             | 496 ++++++++++++++++
> >  drivers/media/platform/vsp1/vsp1_hgo.h             |  50 +++
> >  drivers/media/platform/vsp1/vsp1_histo.c           | 307 +++++++++++++
> >  drivers/media/platform/vsp1/vsp1_histo.h           |  68 +++
> >  drivers/media/platform/vsp1/vsp1_pipe.c            |  30 +-
> >  drivers/media/platform/vsp1/vsp1_pipe.h            |   2 +
> >  drivers/media/platform/vsp1/vsp1_regs.h            |  24 +-
> >  drivers/media/platform/vsp1/vsp1_video.c           |  22 +-
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  19 +
> >  drivers/media/v4l2-core/v4l2-dev.c                 |  37 +-
> >  drivers/media/v4l2-core/v4l2-ioctl.c               |  40 ++
> >  drivers/media/v4l2-core/videobuf2-v4l2.c           |   3 +
> >  include/media/v4l2-dev.h                           |   3 +-
> >  include/media/v4l2-ioctl.h                         |   8 +
> >  include/uapi/linux/media.h                         |   2 +
> >  include/uapi/linux/videodev2.h                     |  17 +
> >  27 files changed, 1678 insertions(+), 47 deletions(-)
> >  create mode 100644 Documentation/DocBook/media/v4l/dev-meta.xml
> >  create mode 100644
> >  Documentation/DocBook/media/v4l/pixfmt-meta-vsp1-hgo.xml
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgo.c
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgo.h
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_histo.c
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_histo.h

-- 
Regards,

Laurent Pinchart