All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: "Hans Verkuil" <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org,
	"Sakari Ailus" <sakari.ailus@maxwell.research.nokia.com>,
	"Cohen David Abraham" <david.cohen@nokia.com>,
	"Koskipää Antti Jussi Petteri" <antti.koskipaa@nokia.com>,
	"Zutshi Vimarsh (Nokia-D-MSW/Helsinki)"
	<vimarsh.zutshi@nokia.com>,
	stefan.kost@nokia.com
Subject: Re: [RFC] Global video buffers pool
Date: Fri, 18 Sep 2009 10:39:17 +0200	[thread overview]
Message-ID: <200909181039.18012.laurent.pinchart@ideasonboard.com> (raw)
In-Reply-To: <20090917194542.5df9c65b@pedra.chehab.org>

Hi Mauro,

thanks for the review. A few comments.

On Friday 18 September 2009 00:45:42 Mauro Carvalho Chehab wrote:
> Em Thu, 17 Sep 2009 23:19:24 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:

[snip]

> > > 4) As you've mentioned, a global set of buffers seem to be the better
> > > alternative. This means that V4L2 core will take care of controlling
> > > the pool, instead of leaving this task to the drivers. This makes
> > > easier to have a boot-time parameter specifying the size of the memory
> > > pool and will optimize memory usage. We may even have a Kconfig var
> > > specifying the default size of the memory pool (although this is not
> > > really needed, since new kernels allow specifying default line command
> > > parameters).
> >
> > Different devices may have quite different buffer requirements (size,
> > number of buffers). Would it be safe to have them all allocated from a
> > global pool? I do not feel confident myself that I understand all the
> > implications of a global pool or whether you actually always want that.
> 
> This is a problem with the pool concept. Even having the same driver,
>  you'll still be needing different resolutions, frame rates, formats and
>  bits per pixel on each /dev/video interface.

That's right (the frame rate doesn't matter though), but not different memory 
type (low-mem, non-cacheable, contiguous, ...) requirements. The only thing 
that matters in the end is the number of buffers and their size. The pool 
doesn't care about the formats and resolutions separately.

>  I'm not sure how to deal.

My idea was to have several groups of video buffers. You could allocate on 
"large" group of low-resolution buffers for video preview, and a "small" group 
of high-resolution buffers for still image capture. Video devices could then 
pick buffers from one of those groups depending on their needs.

>  Maybe we'll need to allocate the buffers considering the worse case that
>  can be passed to the driver. For example, in the case of a kernel
>  parameter, it could be something like:
> 	videobuf=buffers=32,size=256K
> To allocate 32 buffers with 256K each. This way, even if application asks
>  for a smaller buffer, it will keep reserving 256K for each buffer. If bad
>  specified, memory will be wasted, but the memory will be there.
> Eventually, after allocating that memory, some API could be provided for
> example to rearrange the allocated space into 64 x 128K.

We still need separate groups, otherwise we will waste too much memory. 5MP 
sensors are common today, and the size will probably grow in the years to 
come. We can't allocate 32 5MP buffers on an embedded system.

> > > 5) The step to have a a global-wide video buffers pool allocation, as
> > > you mentioned at the RFC, is to make sure that all drivers will use
> > > v4l2 framework to allocate memory. So, this means porting a few drivers
> > > (ivtv, uvcvideo, cx18 and gspca) to use videobuf. As videobuf already
> > > supports all sorts of different memory types and configs (contig and
> > > Scatter/Gather DMA, vmalloced buffers, mmap, userptr, read, overlay
> > > modes), it should fits well on the needs.
> >
> > Why would I want to change ivtv for this? In fact, I see no reason to
> > modify any of the existing drivers. A mc-wide or global memory pool is
> > only of interest for very complex devices where you want to pass buffers
> > around between various sub-devices (and possibly to other media devices
> > or DSPs). And yes, they probably will have to use the framework in order
> > to be able to coordinate these pools properly.
> 
> The issue here is not necessarely related to device complexity. It can be
> motivated by other factors, for example:
> 
> 	- arch's with non-coherent cache;
> 	- devices that aren't capable of doing DMA scatter/gather;
> 	- high memory fragmentation.
> 
> Just as an example, I used an old laptop with "only" 256 Mb of ram, running
>  a new distro, when I started developing the tm6000 drivers. On that
>  hardware, I was needing buffers of about 600 KB each. It were very common
>  to not be able to allocate such buffers there, due to high memory
>  fragmentation, since the USB driver were trying to allocate a continuous
>  buffer on that hardware.
> 
> So, the same argument we used with the EMBEEDED Kconfig option also applies
>  here: it is not everything black or white. For example, surveillance
>  systems need to be very reliable. So, the possibility of allocating memory
>  during boot will help them.
> 
> Just to take a random real usecase, David Liontooth mentioned recently at
>  the ML his intention of maybe using ivtv hardware to capture TV signals at
>  remote locations, having the hardware minimally assisted. He mention the
>  needs of capturing data continuously for 15 hours. That means that the
>  machine will likely close devices and reopen once a day, during years. In
>  such application, a video buffer pool will for sure reduce the risk of
>  memory fragmentation on such systems, giving more reliability to the
>  system, especially if the hardware it will use requires continuous
>  buffers.
> 
> So, while I agree that it is not a mandatory requirement to port the
>  existing drivers to benefit with the memory pool, by not doing it, those
>  drivers will be less reliable than the other drivers on professional
>  usage.

Good point. No need to be too clever though. I think that the memory pool 
concept can be restricted to use cases where the user knows in advance what's 
going to happen with the hardware. A video monitoring system is one of them, a 
digital camera is another one. In those cases the system designer knows what 
resolutions will be streamed at, and how many buffers will be needed. This 
information can come from userspace or the kernel command line, and the memory 
pool won't need to become a complete memory management system. An application 
that wants to use buffers from the pool will the explicitly which set of 
buffers it wants to use.

> > > 6) As videobuf uses a common method of allocating memory, and all
> > > memory requests passes via videobuf-core (videobuf_alloc function), the
> > > implementation of a global-wide set of videobuffer means to touch on
> > > just one function there, at the abstraction layer, and to double check
> > > at the videobuf-dma-sg/videobuf-vmalloc/videobuf-contig if they don't
> > > call directly their own allocation methods. If they do, a simple change
> > > would be needed.
> > >
> > > 7) IMO, the better interface for it is to add some sysfs attributes to
> > > media class, providing there the means to control the video buffer
> > > pools. If the size of a video buffer pool is set to zero, it will use
> > > normal memory allocation. Otherwise, it will work at the "pool mode".
> >
> > Or you use the existing API to request either MEMORY_MMAP or
> > MEMORY_POOL_MMAP. So much cleaner than creating some random sysfs
> > attribute.
> 
> The existing V4L2 API applies over a /dev/video device, not at videobuf or
>  V4L2 core level. As we want this at a core level, we need to apply it on a
>  different place.

VIDIOC_REQBUFS/VIDIOC_QBUF/VIDIOC_DQBUF on the /dev/video device will need to 
tell the driver to use buffers from the pool, so a new memory type is needed.

> > > 8) By using videobuf, we can also export usage statistics via debugfs,
> > > providing runtime statistics about how many memory is being used by
> > > what drivers and /dev devices.
> >
> > Wouldn't procfs be more appropriate? I don't think debugfs is very
> > common.
> 
> perf counters, strace, and other advanced monitoring tools use debugfs.

-- 
Laurent Pinchart

  reply	other threads:[~2009-09-18  8:38 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-16 15:46 [RFC] Global video buffers pool Laurent Pinchart
2009-09-17 18:06 ` Hans de Goede
2009-09-17 18:49 ` Mauro Carvalho Chehab
2009-09-17 21:19   ` Hans Verkuil
2009-09-17 22:45     ` Mauro Carvalho Chehab
2009-09-18  8:39       ` Laurent Pinchart [this message]
2009-09-18 12:47         ` Mauro Carvalho Chehab
2009-09-18  8:29     ` Laurent Pinchart
2009-09-17 19:38 ` Karicheri, Muralidharan
2009-09-18  7:22 ` Hiremath, Vaibhav
2009-09-21 15:07 ` Marek Szyprowski
2009-09-28 14:04 ` Stefan.Kost
2009-09-28 21:54   ` Laurent Pinchart
2009-10-02 22:57     ` Robert Tivy
2009-10-27  7:49     ` Guennadi Liakhovetski
2009-10-28 15:14       ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200909181039.18012.laurent.pinchart@ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=antti.koskipaa@nokia.com \
    --cc=david.cohen@nokia.com \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@infradead.org \
    --cc=sakari.ailus@maxwell.research.nokia.com \
    --cc=stefan.kost@nokia.com \
    --cc=vimarsh.zutshi@nokia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.