Hi Niklas,
Thank you for the patch.
On Sun, Jan 12, 2020 at 02:01:45AM +0100, Niklas Söderlund wrote:
> In the upcoming FrameBuffer API the memory mapping of buffers will be
> left to the user of the FrameBuffer objects. Prepare the V4L2
> compatibility layer to this upcoming change to ease conversion to the
> new API.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> * Changes since v3
> - Return FileDescriptor from getBufferFd()
> - Call fops().munmap() in V4L2CameraProxy::munmap()
> ---
> src/v4l2/v4l2_camera.cpp | 12 ++++++------
> src/v4l2/v4l2_camera.h | 5 +++--
> src/v4l2/v4l2_camera_proxy.cpp | 24 +++++++++++++++++++-----
> src/v4l2/v4l2_camera_proxy.h | 2 +-
> src/v4l2/v4l2_compat_manager.cpp | 2 +-
> 5 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 2a507b9bb8318025..07f05a31261c1b25 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -121,12 +121,6 @@ int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
> return 0;
> }
>
> -void *V4L2Camera::mmap(unsigned int index)
> -{
> - Stream *stream = *camera_->streams().begin();
> - return stream->buffers()[index].planes()[0].mem();
> -}
> -
> int V4L2Camera::allocBuffers(unsigned int count)
> {
> int ret = camera_->allocateBuffers();
> @@ -138,6 +132,12 @@ void V4L2Camera::freeBuffers()
> camera_->freeBuffers();
> }
>
> +FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> +{
> + Stream *stream = *camera_->streams().begin();
> + return FileDescriptor(stream->buffers()[index].planes()[0].dmabuf());
> +}
> +
> int V4L2Camera::streamOn()
> {
> if (isRunning_)
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 81f7908e5e8a6beb..f760316c854fba2f 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -14,6 +14,7 @@
>
> #include <libcamera/buffer.h>
> #include <libcamera/camera.h>
> +#include <libcamera/file_descriptor.h>
>
> #include "semaphore.h"
>
> @@ -53,14 +54,14 @@ public:
> void getStreamConfig(StreamConfiguration *streamConfig);
> std::vector<V4L2FrameMetadata> completedBuffers();
>
> - void *mmap(unsigned int index);
> -
> int configure(StreamConfiguration *streamConfigOut,
> const Size &size, PixelFormat pixelformat,
> unsigned int bufferCount);
>
> int allocBuffers(unsigned int count);
> void freeBuffers();
> + FileDescriptor getBufferFd(unsigned int index);
> +
> int streamOn();
> int streamOff();
>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 52f8468cdaa06a7a..b4a9e2107c0f9f28 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -75,7 +75,8 @@ void V4L2CameraProxy::close()
> vcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking);
> }
>
> -void *V4L2CameraProxy::mmap(size_t length, int prot, int flags, off_t offset)
> +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> + off_t offset)
> {
> LOG(V4L2Compat, Debug) << "Servicing mmap";
>
> @@ -91,13 +92,22 @@ void *V4L2CameraProxy::mmap(size_t length, int prot, int flags, off_t offset)
> return MAP_FAILED;
> }
>
> - void *val = vcam_->invokeMethod(&V4L2Camera::mmap,
> - ConnectionTypeBlocking, index);
> + FileDescriptor fd = vcam_->invokeMethod(&V4L2Camera::getBufferFd,
> + ConnectionTypeBlocking, index);
> + if (!fd.isValid()) {
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> +
> + void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> + flags, fd.fd(), 0);
> + if (map == MAP_FAILED)
> + return map;
>
> buffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;
> - mmaps_[val] = index;
> + mmaps_[map] = index;
>
> - return val;
> + return map;
> }
>
> int V4L2CameraProxy::munmap(void *addr, size_t length)
> @@ -110,6 +120,10 @@ int V4L2CameraProxy::munmap(void *addr, size_t length)
> return -1;
> }
>
> + if (V4L2CompatManager::instance()->fops().munmap(addr, length))
> + LOG(V4L2Compat, Error) << "Unmapping " << addr
> + << " with length " << length;
How about explicitly stating that this is a failure ?
LOG(V4L2Compat, Error) << "Failed to unmap " << addr
<< " with length " << length;
Apart from that, you can keep my R-b.
> +
> buffers_[iter->second].flags &= ~V4L2_BUF_FLAG_MAPPED;
> mmaps_.erase(iter);
>
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index b59d19d707590d88..c8e61adf80f1b93b 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -28,7 +28,7 @@ public:
> int open(bool nonBlocking);
> void dup();
> void close();
> - void *mmap(size_t length, int prot, int flags, off_t offset);
> + void *mmap(void *addr, size_t length, int prot, int flags, off_t offset);
> int munmap(void *addr, size_t length);
>
> int ioctl(unsigned long request, void *arg);
> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp
> index fe453445a9fb2cdd..f5a7b2ac4229d5d5 100644
> --- a/src/v4l2/v4l2_compat_manager.cpp
> +++ b/src/v4l2/v4l2_compat_manager.cpp
> @@ -223,7 +223,7 @@ void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,
> if (!proxy)
> return fops_.mmap(addr, length, prot, flags, fd, offset);
>
> - void *map = proxy->mmap(length, prot, flags, offset);
> + void *map = proxy->mmap(addr, length, prot, flags, offset);
> if (map == MAP_FAILED)
> return map;
>
--
Regards,
Laurent Pinchart