[libcamera-devel] [PATCH v3 05/33] v4l2: camera: Handle memory mapping of buffers directly

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:37:40PM +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>
> ---
>  src/v4l2/v4l2_camera.cpp         | 12 ++++++------
>  src/v4l2/v4l2_camera.h           |  4 ++--
>  src/v4l2/v4l2_camera_proxy.cpp   | 20 +++++++++++++++-----
>  src/v4l2/v4l2_camera_proxy.h     |  2 +-
>  src/v4l2/v4l2_compat_manager.cpp |  2 +-
>  5 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 2a507b9bb8318025..ba3783876101b284 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();
>  }
>  
> +int V4L2Camera::getBufferFd(unsigned int index)

How about returning a FileDescriptor instance, especially given that
further in the series you will get a FileDescriptor from FrameBuffer.

> +{
> +	Stream *stream = *camera_->streams().begin();
> +	return 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..afa20a232235e778 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -53,14 +53,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();
> +	int 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..3c8eba6b29be4811 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);
> +	int fd = vcam_->invokeMethod(&V4L2Camera::getBufferFd,
> +				     ConnectionTypeBlocking, index);
> +	if (fd < 0) {
> +		errno = -fd;
> +		return MAP_FAILED;
> +	}
> +
> +	void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> +							       flags, 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)

We also need to patch munmap() to call fops().munmap(). This should be
fairly trivial as V4L2CompatManager::munmap() doesn't allow partial
unmapping, so we don't need to care about that. You just need to call
fops().munmap() in V4L2CameraProxy::munmap() without real error handling
(but logging an Error message would be useful).

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> 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