[libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Fix V4L2 buffer cache entry availabity check

[Naushir Patuck]

Hi,

Would anybody - hopefully Niklas :) - be able to do a quick review of
the below patch please?  It does seem we need it (or similar
alternative) for good performance on our platform.

Regards,
Naush

On Thu, 30 Jan 2020 at 15:43, <naush at raspberrypi.com> wrote:
>
> From: Naushir Patuck <naush at raspberrypi.com>
>
> The logic for keeping track of the cache holding dmabuf file descriptors
> did not distinguish between slots that are occuiped but free and slots
> that are not occupied.  In devices using imported dmabufs this would
> result in ping-ponging between the top two cache entries - and that in
> turn would result in unnecessary cache missies.
>
> Fix this by tracking if an entry is occupied separately from if an entry
> is free.
>
> Fixes: 9e71540ebbde ("libcamera: v4l2_videodevice: Add V4L2BufferCache to deal with index mapping")
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/include/v4l2_videodevice.h | 1 +
>  src/libcamera/v4l2_videodevice.cpp       | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index e4d35ab..a77b62e 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -125,6 +125,7 @@ private:
>                 bool operator==(const FrameBuffer &buffer);
>
>                 bool free;
> +               bool occupied;
>
>         private:
>                 struct Plane {
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 37b8d33..b887a38 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>                 if (!entry.free)
>                         continue;
>
> -               if (use < 0)
> +               if (use < 0 && !entry.occupied)
>                         use = index;
>
>                 /* Try to find a cache hit by comparing the planes. */
> @@ -245,12 +245,12 @@ void V4L2BufferCache::put(unsigned int index)
>  }
>
>  V4L2BufferCache::Entry::Entry()
> -       : free(true)
> +       : free(true), occupied(false)
>  {
>  }
>
>  V4L2BufferCache::Entry::Entry(bool free, const FrameBuffer &buffer)
> -       : free(free)
> +       : free(free), occupied(true)
>  {
>         for (const FrameBuffer::Plane &plane : buffer.planes())
>                 planes_.emplace_back(plane);
> --
> 2.17.1
>