[{"id":3426,"web_url":"https://patchwork.libcamera.org/comment/3426/","msgid":"<20200112134121.GB25899@pendragon.ideasonboard.com>","date":"2020-01-12T13:41:21","subject":"Re: [libcamera-devel] [PATCH v4 05/32] v4l2: camera: Handle memory\n\tmapping of buffers directly","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Jan 12, 2020 at 02:01:45AM +0100, Niklas Söderlund wrote:\n> In the upcoming FrameBuffer API the memory mapping of buffers will be\n> left to the user of the FrameBuffer objects. Prepare the V4L2\n> compatibility layer to this upcoming change to ease conversion to the\n> new API.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Laurent Pinchart \n> ---\n> * Changes since v3\n> - Return FileDescriptor from getBufferFd()\n> - Call fops().munmap() in V4L2CameraProxy::munmap()\n> ---\n> src/v4l2/v4l2_camera.cpp | 12 ++++++------\n> src/v4l2/v4l2_camera.h | 5 +++--\n> src/v4l2/v4l2_camera_proxy.cpp | 24 +++++++++++++++++++-----\n> src/v4l2/v4l2_camera_proxy.h | 2 +-\n> src/v4l2/v4l2_compat_manager.cpp | 2 +-\n> 5 files changed, 30 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 2a507b9bb8318025..07f05a31261c1b25 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -121,12 +121,6 @@ int V4L2Camera::configure(StreamConfiguration *streamConfigOut,\n> \treturn 0;\n> }\n> \n> -void *V4L2Camera::mmap(unsigned int index)\n> -{\n> -\tStream *stream = *camera_->streams().begin();\n> -\treturn stream->buffers()[index].planes()[0].mem();\n> -}\n> -\n> int V4L2Camera::allocBuffers(unsigned int count)\n> {\n> \tint ret = camera_->allocateBuffers();\n> @@ -138,6 +132,12 @@ void V4L2Camera::freeBuffers()\n> \tcamera_->freeBuffers();\n> }\n> \n> +FileDescriptor V4L2Camera::getBufferFd(unsigned int index)\n> +{\n> +\tStream *stream = *camera_->streams().begin();\n> +\treturn FileDescriptor(stream->buffers()[index].planes()[0].dmabuf());\n> +}\n> +\n> int V4L2Camera::streamOn()\n> {\n> \tif (isRunning_)\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index 81f7908e5e8a6beb..f760316c854fba2f 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> #include \n> +#include \n> \n> #include \"semaphore.h\"\n> \n> @@ -53,14 +54,14 @@ public:\n> \tvoid getStreamConfig(StreamConfiguration *streamConfig);\n> \tstd::vector completedBuffers();\n> \n> -\tvoid *mmap(unsigned int index);\n> -\n> \tint configure(StreamConfiguration *streamConfigOut,\n> \t\t const Size &size, PixelFormat pixelformat,\n> \t\t unsigned int bufferCount);\n> \n> \tint allocBuffers(unsigned int count);\n> \tvoid freeBuffers();\n> +\tFileDescriptor getBufferFd(unsigned int index);\n> +\n> \tint streamOn();\n> \tint streamOff();\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 52f8468cdaa06a7a..b4a9e2107c0f9f28 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -75,7 +75,8 @@ void V4L2CameraProxy::close()\n> \tvcam_->invokeMethod(&V4L2Camera::close, ConnectionTypeBlocking);\n> }\n> \n> -void *V4L2CameraProxy::mmap(size_t length, int prot, int flags, off_t offset)\n> +void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> +\t\t\t off_t offset)\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing mmap\";\n> \n> @@ -91,13 +92,22 @@ void *V4L2CameraProxy::mmap(size_t length, int prot, int flags, off_t offset)\n> \t\treturn MAP_FAILED;\n> \t}\n> \n> -\tvoid *val = vcam_->invokeMethod(&V4L2Camera::mmap,\n> -\t\t\t\t\tConnectionTypeBlocking, index);\n> +\tFileDescriptor fd = vcam_->invokeMethod(&V4L2Camera::getBufferFd,\n> +\t\t\t\t\t\tConnectionTypeBlocking, index);\n> +\tif (!fd.isValid()) {\n> +\t\terrno = EINVAL;\n> +\t\treturn MAP_FAILED;\n> +\t}\n> +\n> +\tvoid *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,\n> +\t\t\t\t\t\t\t flags, fd.fd(), 0);\n> +\tif (map == MAP_FAILED)\n> +\t\treturn map;\n> \n> \tbuffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;\n> -\tmmaps_[val] = index;\n> +\tmmaps_[map] = index;\n> \n> -\treturn val;\n> +\treturn map;\n> }\n> \n> int V4L2CameraProxy::munmap(void *addr, size_t length)\n> @@ -110,6 +120,10 @@ int V4L2CameraProxy::munmap(void *addr, size_t length)\n> \t\treturn -1;\n> \t}\n> \n> +\tif (V4L2CompatManager::instance()->fops().munmap(addr, length))\n> +\t\tLOG(V4L2Compat, Error) << \"Unmapping \" << addr\n> +\t\t\t\t << \" with length \" << length;\n\nHow about explicitly stating that this is a failure ?\n\n\t\tLOG(V4L2Compat, Error) << \"Failed to unmap \" << addr\n\t\t\t\t << \" with length \" << length;\n\nApart from that, you can keep my R-b.\n\n> +\n> \tbuffers_[iter->second].flags &= ~V4L2_BUF_FLAG_MAPPED;\n> \tmmaps_.erase(iter);\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index b59d19d707590d88..c8e61adf80f1b93b 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -28,7 +28,7 @@ public:\n> \tint open(bool nonBlocking);\n> \tvoid dup();\n> \tvoid close();\n> -\tvoid *mmap(size_t length, int prot, int flags, off_t offset);\n> +\tvoid *mmap(void *addr, size_t length, int prot, int flags, off_t offset);\n> \tint munmap(void *addr, size_t length);\n> \n> \tint ioctl(unsigned long request, void *arg);\n> diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp\n> index fe453445a9fb2cdd..f5a7b2ac4229d5d5 100644\n> --- a/src/v4l2/v4l2_compat_manager.cpp\n> +++ b/src/v4l2/v4l2_compat_manager.cpp\n> @@ -223,7 +223,7 @@ void *V4L2CompatManager::mmap(void *addr, size_t length, int prot, int flags,\n> \tif (!proxy)\n> \t\treturn fops_.mmap(addr, length, prot, flags, fd, offset);\n> \n> -\tvoid *map = proxy->mmap(length, prot, flags, offset);\n> +\tvoid *map = proxy->mmap(addr, length, prot, flags, offset);\n> \tif (map == MAP_FAILED)\n> \t\treturn map;\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 454006045E\n\tfor ;\n\tSun, 12 Jan 2020 14:41:41 +0100 (CET)","from pendragon.ideasonboard.com (85-76-9-232-nat.elisa-mobile.fi\n\t[85.76.9.232])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 00C1830F;\n\tSun, 12 Jan 2020 14:41:39 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578836501;\n\tbh=/WvRVP9c8blvfTE09v/S7yc02DVBImz60MhBObYQ3Hc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KiyunfkMA5cSQntZdj+MAyt9pOWoltJoxmmiu+OeaY/gQBoEh+gFiVMZDK7jnajgj\n\tdzj68zmszytN8Vc3R2zkFyGQX0at4lkx50esae8TzTJZaozsTV+0nnMAFehTm83+4+\n\ten/e3uUp0BItLhAov3zRwExTF93Flpf92ArMLEbc=","Date":"Sun, 12 Jan 2020 15:41:21 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200112134121.GB25899@pendragon.ideasonboard.com>","References":"<20200112010212.2609025-1-niklas.soderlund@ragnatech.se>\n\t<20200112010212.2609025-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200112010212.2609025-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 05/32] v4l2: camera: Handle memory\n\tmapping of buffers directly","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 12 Jan 2020 13:41:41 -0000"}}]