[{"id":765,"web_url":"https://patchwork.libcamera.org/comment/765/","msgid":"<20190207222829.kfu5z34h5rvrrlu3@uno.localdomain>","date":"2019-02-07T22:28:29","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote:\n> exportBuffers() can only operate on an existing BufferPool allocation. The\n\n\"an interanlly allocated BufferPool\" ?\n\n> pool identifies its size through its .count() method.\n>\n> Passing a count in to the exportBuffers() call is redundant and can be\n> incorrect if the value is not the same as the BufferPool size.\n>\n> Simplify the function and remove the unnecessary argument, correcting all uses\n> throughout the code base.\n>\n> While we're here, remove the createBuffers() helper from the V4L2DeviceTest\n> which only served to obfuscate which pool the buffers were being allocated for.\n>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/libcamera/include/v4l2_device.h  |  2 +-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 +--\n>  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-\n>  src/libcamera/pipeline/vimc.cpp      |  2 +-\n>  src/libcamera/v4l2_device.cpp        | 15 ++++++---------\n>  test/v4l2_device/buffer_sharing.cpp  |  2 +-\n>  test/v4l2_device/capture_async.cpp   |  4 ++--\n>  test/v4l2_device/request_buffers.cpp |  4 ++--\n>  test/v4l2_device/stream_on_off.cpp   |  4 ++--\n>  test/v4l2_device/v4l2_device_test.h  |  2 --\n>  10 files changed, 17 insertions(+), 23 deletions(-)\n>\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 3acb5e466d64..98fa2110efb5 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -97,7 +97,7 @@ public:\n>  \tint getFormat(V4L2DeviceFormat *format);\n>  \tint setFormat(V4L2DeviceFormat *format);\n>\n> -\tint exportBuffers(unsigned int count, BufferPool *pool);\n> +\tint exportBuffers(BufferPool *pool);\n>  \tint importBuffers(BufferPool *pool);\n>  \tint releaseBuffers();\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 34b03995ae31..677e127dd738 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  \tif (!cfg.bufferCount)\n>  \t\treturn -EINVAL;\n>\n> -\tint ret = data->cio2_->exportBuffers(cfg.bufferCount,\n> -\t\t\t\t\t     &stream->bufferPool());\n> +\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>  \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index ed8228bb2fc6..1a0dcb582670 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n>\n>  \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>\n> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> +\treturn video_->exportBuffers(&stream->bufferPool());\n>  }\n>\n>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 0e9ad7b59ee5..06627b1cb847 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n\n>\n>  \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>\n> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> +\treturn video_->exportBuffers(&stream->bufferPool());\n>  }\n>\n>  int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c654e6dd7b8d..c2e4d0ea8db2 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count)\n>  }\n>\n>  /**\n> - * \\brief Request \\a count buffers to be allocated from the device and stored in\n> - * the buffer pool provided.\n> - * \\param[in] count Number of buffers to allocate\n> + * \\brief Request buffers to be allocated from the device and stored in the\n> + *  buffer pool provided.\n>   * \\param[out] pool BufferPool to populate with buffers\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n> +int V4L2Device::exportBuffers(BufferPool *pool)\n>  {\n>  \tunsigned int allocatedBuffers;\n>  \tunsigned int i;\n> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>\n>  \tmemoryType_ = V4L2_MEMORY_MMAP;\n>\n> -\tret = requestBuffers(count);\n> +\tret = requestBuffers(pool->count());\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>\n>  \tallocatedBuffers = ret;\n> -\tif (allocatedBuffers < count) {\n> +\tif (allocatedBuffers < pool->count()) {\n\nI have a feeling we discussed this, but could you drop\nallocatedBuffers now?\n\nThe rest looks fine!\nThanks\n   j\n\n>  \t\tLOG(V4L2, Error) << \"Not enough buffers provided by V4L2Device\";\n>  \t\trequestBuffers(0);\n>  \t\treturn -ENOMEM;\n>  \t}\n>\n> -\tcount = ret;\n> -\n>  \t/* Map the buffers. */\n> -\tfor (i = 0; i < count; ++i) {\n> +\tfor (i = 0; i < pool->count(); ++i) {\n>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n>  \t\tstruct v4l2_buffer buf = {};\n>  \t\tstruct Buffer &buffer = pool->buffers()[i];\n> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp\n> index 0e96f7b894bd..ca086f2728f1 100644\n> --- a/test/v4l2_device/buffer_sharing.cpp\n> +++ b/test/v4l2_device/buffer_sharing.cpp\n> @@ -80,7 +80,7 @@ protected:\n>\n>  \t\tpool_.createBuffers(bufferCount);\n>\n> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>\n> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp\n> index 7a0735f65535..d3e20ed86b98 100644\n> --- a/test/v4l2_device/capture_async.cpp\n> +++ b/test/v4l2_device/capture_async.cpp\n> @@ -38,9 +38,9 @@ protected:\n>  \t\tTimer timeout;\n>  \t\tint ret;\n>\n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>\n> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>\n> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp\n> index bc6ff2c18a57..26d7d9528982 100644\n> --- a/test/v4l2_device/request_buffers.cpp\n> +++ b/test/v4l2_device/request_buffers.cpp\n> @@ -19,9 +19,9 @@ protected:\n>  \t\t */\n>  \t\tconst unsigned int bufferCount = 8;\n>\n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>\n> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tint ret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>\n> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp\n> index b564d2a2ab67..861d8d695813 100644\n> --- a/test/v4l2_device/stream_on_off.cpp\n> +++ b/test/v4l2_device/stream_on_off.cpp\n> @@ -14,9 +14,9 @@ protected:\n>  \t{\n>  \t\tconst unsigned int bufferCount = 8;\n>\n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>\n> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tint ret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>\n> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n> index f22f0bb555d8..43539655f85b 100644\n> --- a/test/v4l2_device/v4l2_device_test.h\n> +++ b/test/v4l2_device/v4l2_device_test.h\n> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test\n>  public:\n>  \tV4L2DeviceTest() : dev_(nullptr){};\n>\n> -\tvoid createBuffers(unsigned int qty) { pool_.createBuffers(qty); }\n> -\n>  protected:\n>  \tint init();\n>  \tvoid cleanup();\n> --\n> 2.19.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8EB1610B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  7 Feb 2019 23:28:09 +0100 (CET)","from uno.localdomain (d51A4137F.access.telenet.be [81.164.19.127])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 299EBFF805;\n\tThu,  7 Feb 2019 22:28:08 +0000 (UTC)"],"X-Originating-IP":"81.164.19.127","Date":"Thu, 7 Feb 2019 23:28:29 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190207222829.kfu5z34h5rvrrlu3@uno.localdomain>","References":"<20190207212119.30299-1-kieran.bingham@ideasonboard.com>\n\t<20190207212119.30299-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"7mpdh4dzc5vspnvp\"","Content-Disposition":"inline","In-Reply-To":"<20190207212119.30299-5-kieran.bingham@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 07 Feb 2019 22:28:09 -0000"}},{"id":772,"web_url":"https://patchwork.libcamera.org/comment/772/","msgid":"<20190208173223.GI4562@pendragon.ideasonboard.com>","date":"2019-02-08T17:32:23","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote:\n> exportBuffers() can only operate on an existing BufferPool allocation. The\n> pool identifies its size through its .count() method.\n> \n> Passing a count in to the exportBuffers() call is redundant and can be\n> incorrect if the value is not the same as the BufferPool size.\n> \n> Simplify the function and remove the unnecessary argument, correcting all uses\n> throughout the code base.\n> \n> While we're here, remove the createBuffers() helper from the V4L2DeviceTest\n> which only served to obfuscate which pool the buffers were being allocated for.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nThis looks good to me. I wonder why we added a count parameter in the\nfirst place :-)\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/libcamera/include/v4l2_device.h  |  2 +-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 +--\n>  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-\n>  src/libcamera/pipeline/vimc.cpp      |  2 +-\n>  src/libcamera/v4l2_device.cpp        | 15 ++++++---------\n>  test/v4l2_device/buffer_sharing.cpp  |  2 +-\n>  test/v4l2_device/capture_async.cpp   |  4 ++--\n>  test/v4l2_device/request_buffers.cpp |  4 ++--\n>  test/v4l2_device/stream_on_off.cpp   |  4 ++--\n>  test/v4l2_device/v4l2_device_test.h  |  2 --\n>  10 files changed, 17 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 3acb5e466d64..98fa2110efb5 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -97,7 +97,7 @@ public:\n>  \tint getFormat(V4L2DeviceFormat *format);\n>  \tint setFormat(V4L2DeviceFormat *format);\n>  \n> -\tint exportBuffers(unsigned int count, BufferPool *pool);\n> +\tint exportBuffers(BufferPool *pool);\n>  \tint importBuffers(BufferPool *pool);\n>  \tint releaseBuffers();\n>  \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 34b03995ae31..677e127dd738 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  \tif (!cfg.bufferCount)\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = data->cio2_->exportBuffers(cfg.bufferCount,\n> -\t\t\t\t\t     &stream->bufferPool());\n> +\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>  \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index ed8228bb2fc6..1a0dcb582670 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n>  \n>  \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>  \n> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> +\treturn video_->exportBuffers(&stream->bufferPool());\n>  }\n>  \n>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 0e9ad7b59ee5..06627b1cb847 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n>  \n>  \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>  \n> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> +\treturn video_->exportBuffers(&stream->bufferPool());\n>  }\n>  \n>  int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c654e6dd7b8d..c2e4d0ea8db2 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count)\n>  }\n>  \n>  /**\n> - * \\brief Request \\a count buffers to be allocated from the device and stored in\n> - * the buffer pool provided.\n> - * \\param[in] count Number of buffers to allocate\n> + * \\brief Request buffers to be allocated from the device and stored in the\n> + *  buffer pool provided.\n>   * \\param[out] pool BufferPool to populate with buffers\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n> +int V4L2Device::exportBuffers(BufferPool *pool)\n>  {\n>  \tunsigned int allocatedBuffers;\n>  \tunsigned int i;\n> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>  \n>  \tmemoryType_ = V4L2_MEMORY_MMAP;\n>  \n> -\tret = requestBuffers(count);\n> +\tret = requestBuffers(pool->count());\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n>  \tallocatedBuffers = ret;\n> -\tif (allocatedBuffers < count) {\n> +\tif (allocatedBuffers < pool->count()) {\n>  \t\tLOG(V4L2, Error) << \"Not enough buffers provided by V4L2Device\";\n>  \t\trequestBuffers(0);\n>  \t\treturn -ENOMEM;\n>  \t}\n>  \n> -\tcount = ret;\n> -\n>  \t/* Map the buffers. */\n> -\tfor (i = 0; i < count; ++i) {\n> +\tfor (i = 0; i < pool->count(); ++i) {\n>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n>  \t\tstruct v4l2_buffer buf = {};\n>  \t\tstruct Buffer &buffer = pool->buffers()[i];\n> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp\n> index 0e96f7b894bd..ca086f2728f1 100644\n> --- a/test/v4l2_device/buffer_sharing.cpp\n> +++ b/test/v4l2_device/buffer_sharing.cpp\n> @@ -80,7 +80,7 @@ protected:\n>  \n>  \t\tpool_.createBuffers(bufferCount);\n>  \n> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>  \n> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp\n> index 7a0735f65535..d3e20ed86b98 100644\n> --- a/test/v4l2_device/capture_async.cpp\n> +++ b/test/v4l2_device/capture_async.cpp\n> @@ -38,9 +38,9 @@ protected:\n>  \t\tTimer timeout;\n>  \t\tint ret;\n>  \n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>  \n> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>  \n> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp\n> index bc6ff2c18a57..26d7d9528982 100644\n> --- a/test/v4l2_device/request_buffers.cpp\n> +++ b/test/v4l2_device/request_buffers.cpp\n> @@ -19,9 +19,9 @@ protected:\n>  \t\t */\n>  \t\tconst unsigned int bufferCount = 8;\n>  \n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>  \n> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tint ret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>  \n> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp\n> index b564d2a2ab67..861d8d695813 100644\n> --- a/test/v4l2_device/stream_on_off.cpp\n> +++ b/test/v4l2_device/stream_on_off.cpp\n> @@ -14,9 +14,9 @@ protected:\n>  \t{\n>  \t\tconst unsigned int bufferCount = 8;\n>  \n> -\t\tcreateBuffers(bufferCount);\n> +\t\tpool_.createBuffers(bufferCount);\n>  \n> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> +\t\tint ret = dev_->exportBuffers(&pool_);\n>  \t\tif (ret)\n>  \t\t\treturn TestFail;\n>  \n> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n> index f22f0bb555d8..43539655f85b 100644\n> --- a/test/v4l2_device/v4l2_device_test.h\n> +++ b/test/v4l2_device/v4l2_device_test.h\n> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test\n>  public:\n>  \tV4L2DeviceTest() : dev_(nullptr){};\n>  \n> -\tvoid createBuffers(unsigned int qty) { pool_.createBuffers(qty); }\n> -\n>  protected:\n>  \tint init();\n>  \tvoid cleanup();","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","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 3E7906101F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  8 Feb 2019 18:32:25 +0100 (CET)","from pendragon.ideasonboard.com (d51A4137F.access.telenet.be\n\t[81.164.19.127])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADD5FF9;\n\tFri,  8 Feb 2019 18:32:24 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1549647144;\n\tbh=bPy9J1aX3jryIXJ2KNL+hf8T3eT+/c6CW94M+z7jPIQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Js6iCfFwCcuSfOvFsk2WBQB8IBgY6FcbL1fXVDaWisthnal3F+NVgh6Pa40wrI6oP\n\tz5W24prGkfPyrawQ16Y3W6SBqUYkX8QKCa6xDwfNTi4ukdJCtgFnxxzyOFPq+DwHGx\n\t15xBGmr6iKShVZYr8ZQuD/m1Rlwcb5ofSE8tp+K0=","Date":"Fri, 8 Feb 2019 19:32:23 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190208173223.GI4562@pendragon.ideasonboard.com>","References":"<20190207212119.30299-1-kieran.bingham@ideasonboard.com>\n\t<20190207212119.30299-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190207212119.30299-5-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 08 Feb 2019 17:32:25 -0000"}},{"id":780,"web_url":"https://patchwork.libcamera.org/comment/780/","msgid":"<6c08f64a-f1a8-9bb7-6115-928966cd19dc@ideasonboard.com>","date":"2019-02-11T11:57:01","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 07/02/2019 22:28, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote:\n>> exportBuffers() can only operate on an existing BufferPool allocation. The\n> \n> \"an interanlly allocated BufferPool\" ?\n\nPossibly but I'm not sure how to word this to be accurate.\n\nexportBuffers requires a BufferPool to exist and have non-populated buffers.\n\n*exportBuffers()* is doing the allocation, so it's not right to say it\nrequries an internally allocated BufferPool - because it requires it to\nbe not yet allocated.\n\n\n>> pool identifies its size through its .count() method.\n>>\n>> Passing a count in to the exportBuffers() call is redundant and can be\n>> incorrect if the value is not the same as the BufferPool size.\n>>\n>> Simplify the function and remove the unnecessary argument, correcting all uses\n>> throughout the code base.\n>>\n>> While we're here, remove the createBuffers() helper from the V4L2DeviceTest\n>> which only served to obfuscate which pool the buffers were being allocated for.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  src/libcamera/include/v4l2_device.h  |  2 +-\n>>  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 +--\n>>  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-\n>>  src/libcamera/pipeline/vimc.cpp      |  2 +-\n>>  src/libcamera/v4l2_device.cpp        | 15 ++++++---------\n>>  test/v4l2_device/buffer_sharing.cpp  |  2 +-\n>>  test/v4l2_device/capture_async.cpp   |  4 ++--\n>>  test/v4l2_device/request_buffers.cpp |  4 ++--\n>>  test/v4l2_device/stream_on_off.cpp   |  4 ++--\n>>  test/v4l2_device/v4l2_device_test.h  |  2 --\n>>  10 files changed, 17 insertions(+), 23 deletions(-)\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> index 3acb5e466d64..98fa2110efb5 100644\n>> --- a/src/libcamera/include/v4l2_device.h\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -97,7 +97,7 @@ public:\n>>  \tint getFormat(V4L2DeviceFormat *format);\n>>  \tint setFormat(V4L2DeviceFormat *format);\n>>\n>> -\tint exportBuffers(unsigned int count, BufferPool *pool);\n>> +\tint exportBuffers(BufferPool *pool);\n>>  \tint importBuffers(BufferPool *pool);\n>>  \tint releaseBuffers();\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 34b03995ae31..677e127dd738 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>>  \tif (!cfg.bufferCount)\n>>  \t\treturn -EINVAL;\n>>\n>> -\tint ret = data->cio2_->exportBuffers(cfg.bufferCount,\n>> -\t\t\t\t\t     &stream->bufferPool());\n>> +\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n>>  \tif (ret) {\n>>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>>  \t\treturn ret;\n>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n>> index ed8228bb2fc6..1a0dcb582670 100644\n>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n>> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n>>\n>>  \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>>\n>> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n>> +\treturn video_->exportBuffers(&stream->bufferPool());\n>>  }\n>>\n>>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n>> index 0e9ad7b59ee5..06627b1cb847 100644\n>> --- a/src/libcamera/pipeline/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc.cpp\n>> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n> \n>>\n>>  \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>>\n>> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n>> +\treturn video_->exportBuffers(&stream->bufferPool());\n>>  }\n>>\n>>  int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index c654e6dd7b8d..c2e4d0ea8db2 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count)\n>>  }\n>>\n>>  /**\n>> - * \\brief Request \\a count buffers to be allocated from the device and stored in\n>> - * the buffer pool provided.\n>> - * \\param[in] count Number of buffers to allocate\n>> + * \\brief Request buffers to be allocated from the device and stored in the\n>> + *  buffer pool provided.\n>>   * \\param[out] pool BufferPool to populate with buffers\n>>   * \\return 0 on success or a negative error code otherwise\n>>   */\n>> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>> +int V4L2Device::exportBuffers(BufferPool *pool)\n>>  {\n>>  \tunsigned int allocatedBuffers;\n>>  \tunsigned int i;\n>> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>>\n>>  \tmemoryType_ = V4L2_MEMORY_MMAP;\n>>\n>> -\tret = requestBuffers(count);\n>> +\tret = requestBuffers(pool->count());\n>>  \tif (ret < 0)\n>>  \t\treturn ret;\n>>\n>>  \tallocatedBuffers = ret;\n>> -\tif (allocatedBuffers < count) {\n>> +\tif (allocatedBuffers < pool->count()) {\n> \n> I have a feeling we discussed this, but could you drop\n> allocatedBuffers now?\n> \n\nrequestBuffers returns a signed int. pool->count() is an unsigned int.\n\nthe allocatedBuffers is essentially a cleaner (I hope) way to write the\nexpression such that it doesn't require casting of the signed/unsigned\ntypes comparision.\n\nIt's safe because the first test \"if (ret < 0)\" ensures that the value\ncan only be positive. Then the allocatedBuffers (unsigned) is set as the\n(guaranteed postive) ret.\n\n\n\n> The rest looks fine!\n> Thanks\n>    j\n> \n>>  \t\tLOG(V4L2, Error) << \"Not enough buffers provided by V4L2Device\";\n>>  \t\trequestBuffers(0);\n>>  \t\treturn -ENOMEM;\n>>  \t}\n>>\n>> -\tcount = ret;\n>> -\n>>  \t/* Map the buffers. */\n>> -\tfor (i = 0; i < count; ++i) {\n>> +\tfor (i = 0; i < pool->count(); ++i) {\n>>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n>>  \t\tstruct v4l2_buffer buf = {};\n>>  \t\tstruct Buffer &buffer = pool->buffers()[i];\n>> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp\n>> index 0e96f7b894bd..ca086f2728f1 100644\n>> --- a/test/v4l2_device/buffer_sharing.cpp\n>> +++ b/test/v4l2_device/buffer_sharing.cpp\n>> @@ -80,7 +80,7 @@ protected:\n>>\n>>  \t\tpool_.createBuffers(bufferCount);\n>>\n>> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>\n>> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp\n>> index 7a0735f65535..d3e20ed86b98 100644\n>> --- a/test/v4l2_device/capture_async.cpp\n>> +++ b/test/v4l2_device/capture_async.cpp\n>> @@ -38,9 +38,9 @@ protected:\n>>  \t\tTimer timeout;\n>>  \t\tint ret;\n>>\n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>\n>> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>\n>> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp\n>> index bc6ff2c18a57..26d7d9528982 100644\n>> --- a/test/v4l2_device/request_buffers.cpp\n>> +++ b/test/v4l2_device/request_buffers.cpp\n>> @@ -19,9 +19,9 @@ protected:\n>>  \t\t */\n>>  \t\tconst unsigned int bufferCount = 8;\n>>\n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>\n>> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tint ret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>\n>> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp\n>> index b564d2a2ab67..861d8d695813 100644\n>> --- a/test/v4l2_device/stream_on_off.cpp\n>> +++ b/test/v4l2_device/stream_on_off.cpp\n>> @@ -14,9 +14,9 @@ protected:\n>>  \t{\n>>  \t\tconst unsigned int bufferCount = 8;\n>>\n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>\n>> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tint ret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>\n>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n>> index f22f0bb555d8..43539655f85b 100644\n>> --- a/test/v4l2_device/v4l2_device_test.h\n>> +++ b/test/v4l2_device/v4l2_device_test.h\n>> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test\n>>  public:\n>>  \tV4L2DeviceTest() : dev_(nullptr){};\n>>\n>> -\tvoid createBuffers(unsigned int qty) { pool_.createBuffers(qty); }\n>> -\n>>  protected:\n>>  \tint init();\n>>  \tvoid cleanup();\n>> --\n>> 2.19.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F10DE610B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Feb 2019 12:57:04 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E5C82E4;\n\tMon, 11 Feb 2019 12:57:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1549886224;\n\tbh=jSn2pEY2CLobyzkkjiPfrySoVAwCajg9SJRvAy5xxeg=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JwWoNIE9lkl5APAU6FxDo7VJLiAvcLVwrmIrZ9xVYziKBztJkkMAnyXjHCrJK/k8h\n\tF34/q13ZkKcxrrZKEVBfYLCirkor4uv3PxII3dSzx27+2ffPserpY8MfmLy1U0azto\n\tt8rm7QjzvhbHSDMllrZuMwxDIJESEmQ31k/L9uIE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190207212119.30299-1-kieran.bingham@ideasonboard.com>\n\t<20190207212119.30299-5-kieran.bingham@ideasonboard.com>\n\t<20190207222829.kfu5z34h5rvrrlu3@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<6c08f64a-f1a8-9bb7-6115-928966cd19dc@ideasonboard.com>","Date":"Mon, 11 Feb 2019 11:57:01 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190207222829.kfu5z34h5rvrrlu3@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 11 Feb 2019 11:57:05 -0000"}},{"id":781,"web_url":"https://patchwork.libcamera.org/comment/781/","msgid":"<a64dc358-cb26-1b04-bb32-3e1a3eaa3f49@ideasonboard.com>","date":"2019-02-11T11:59:05","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 08/02/2019 17:32, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote:\n>> exportBuffers() can only operate on an existing BufferPool allocation. The\n>> pool identifies its size through its .count() method.\n>>\n>> Passing a count in to the exportBuffers() call is redundant and can be\n>> incorrect if the value is not the same as the BufferPool size.\n>>\n>> Simplify the function and remove the unnecessary argument, correcting all uses\n>> throughout the code base.\n>>\n>> While we're here, remove the createBuffers() helper from the V4L2DeviceTest\n>> which only served to obfuscate which pool the buffers were being allocated for.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> This looks good to me. I wonder why we added a count parameter in the\n> first place :-)\n\nThe original implementation used the count parameter to determine how\nmany buffers to add to an empty pool.\n\nWe modified this to provide an existing sized (but unpopulated) pool.\n\nI guess it was easy to miss because the count still looked reasonable :).\n\nIt was almost techincally still correct but it does hide subtle issues\nin case the allocatedBuffers != pool.count() so it's best to remove it.\n\n\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nThanks.\n\n\n> \n>> ---\n>>  src/libcamera/include/v4l2_device.h  |  2 +-\n>>  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 +--\n>>  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-\n>>  src/libcamera/pipeline/vimc.cpp      |  2 +-\n>>  src/libcamera/v4l2_device.cpp        | 15 ++++++---------\n>>  test/v4l2_device/buffer_sharing.cpp  |  2 +-\n>>  test/v4l2_device/capture_async.cpp   |  4 ++--\n>>  test/v4l2_device/request_buffers.cpp |  4 ++--\n>>  test/v4l2_device/stream_on_off.cpp   |  4 ++--\n>>  test/v4l2_device/v4l2_device_test.h  |  2 --\n>>  10 files changed, 17 insertions(+), 23 deletions(-)\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> index 3acb5e466d64..98fa2110efb5 100644\n>> --- a/src/libcamera/include/v4l2_device.h\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -97,7 +97,7 @@ public:\n>>  \tint getFormat(V4L2DeviceFormat *format);\n>>  \tint setFormat(V4L2DeviceFormat *format);\n>>  \n>> -\tint exportBuffers(unsigned int count, BufferPool *pool);\n>> +\tint exportBuffers(BufferPool *pool);\n>>  \tint importBuffers(BufferPool *pool);\n>>  \tint releaseBuffers();\n>>  \n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 34b03995ae31..677e127dd738 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>>  \tif (!cfg.bufferCount)\n>>  \t\treturn -EINVAL;\n>>  \n>> -\tint ret = data->cio2_->exportBuffers(cfg.bufferCount,\n>> -\t\t\t\t\t     &stream->bufferPool());\n>> +\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n>>  \tif (ret) {\n>>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>>  \t\treturn ret;\n>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n>> index ed8228bb2fc6..1a0dcb582670 100644\n>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n>> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n>>  \n>>  \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>>  \n>> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n>> +\treturn video_->exportBuffers(&stream->bufferPool());\n>>  }\n>>  \n>>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n>> index 0e9ad7b59ee5..06627b1cb847 100644\n>> --- a/src/libcamera/pipeline/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc.cpp\n>> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n>>  \n>>  \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n>>  \n>> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n>> +\treturn video_->exportBuffers(&stream->bufferPool());\n>>  }\n>>  \n>>  int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index c654e6dd7b8d..c2e4d0ea8db2 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count)\n>>  }\n>>  \n>>  /**\n>> - * \\brief Request \\a count buffers to be allocated from the device and stored in\n>> - * the buffer pool provided.\n>> - * \\param[in] count Number of buffers to allocate\n>> + * \\brief Request buffers to be allocated from the device and stored in the\n>> + *  buffer pool provided.\n>>   * \\param[out] pool BufferPool to populate with buffers\n>>   * \\return 0 on success or a negative error code otherwise\n>>   */\n>> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>> +int V4L2Device::exportBuffers(BufferPool *pool)\n>>  {\n>>  \tunsigned int allocatedBuffers;\n>>  \tunsigned int i;\n>> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n>>  \n>>  \tmemoryType_ = V4L2_MEMORY_MMAP;\n>>  \n>> -\tret = requestBuffers(count);\n>> +\tret = requestBuffers(pool->count());\n>>  \tif (ret < 0)\n>>  \t\treturn ret;\n>>  \n>>  \tallocatedBuffers = ret;\n>> -\tif (allocatedBuffers < count) {\n>> +\tif (allocatedBuffers < pool->count()) {\n>>  \t\tLOG(V4L2, Error) << \"Not enough buffers provided by V4L2Device\";\n>>  \t\trequestBuffers(0);\n>>  \t\treturn -ENOMEM;\n>>  \t}\n>>  \n>> -\tcount = ret;\n>> -\n>>  \t/* Map the buffers. */\n>> -\tfor (i = 0; i < count; ++i) {\n>> +\tfor (i = 0; i < pool->count(); ++i) {\n>>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n>>  \t\tstruct v4l2_buffer buf = {};\n>>  \t\tstruct Buffer &buffer = pool->buffers()[i];\n>> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp\n>> index 0e96f7b894bd..ca086f2728f1 100644\n>> --- a/test/v4l2_device/buffer_sharing.cpp\n>> +++ b/test/v4l2_device/buffer_sharing.cpp\n>> @@ -80,7 +80,7 @@ protected:\n>>  \n>>  \t\tpool_.createBuffers(bufferCount);\n>>  \n>> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>  \n>> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp\n>> index 7a0735f65535..d3e20ed86b98 100644\n>> --- a/test/v4l2_device/capture_async.cpp\n>> +++ b/test/v4l2_device/capture_async.cpp\n>> @@ -38,9 +38,9 @@ protected:\n>>  \t\tTimer timeout;\n>>  \t\tint ret;\n>>  \n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>  \n>> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>  \n>> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp\n>> index bc6ff2c18a57..26d7d9528982 100644\n>> --- a/test/v4l2_device/request_buffers.cpp\n>> +++ b/test/v4l2_device/request_buffers.cpp\n>> @@ -19,9 +19,9 @@ protected:\n>>  \t\t */\n>>  \t\tconst unsigned int bufferCount = 8;\n>>  \n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>  \n>> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tint ret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>  \n>> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp\n>> index b564d2a2ab67..861d8d695813 100644\n>> --- a/test/v4l2_device/stream_on_off.cpp\n>> +++ b/test/v4l2_device/stream_on_off.cpp\n>> @@ -14,9 +14,9 @@ protected:\n>>  \t{\n>>  \t\tconst unsigned int bufferCount = 8;\n>>  \n>> -\t\tcreateBuffers(bufferCount);\n>> +\t\tpool_.createBuffers(bufferCount);\n>>  \n>> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n>> +\t\tint ret = dev_->exportBuffers(&pool_);\n>>  \t\tif (ret)\n>>  \t\t\treturn TestFail;\n>>  \n>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n>> index f22f0bb555d8..43539655f85b 100644\n>> --- a/test/v4l2_device/v4l2_device_test.h\n>> +++ b/test/v4l2_device/v4l2_device_test.h\n>> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test\n>>  public:\n>>  \tV4L2DeviceTest() : dev_(nullptr){};\n>>  \n>> -\tvoid createBuffers(unsigned int qty) { pool_.createBuffers(qty); }\n>> -\n>>  protected:\n>>  \tint init();\n>>  \tvoid cleanup();\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0587B610B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Feb 2019 12:59:09 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 566C62E4;\n\tMon, 11 Feb 2019 12:59:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1549886348;\n\tbh=kqeejLSVpn3v4mfPx4g91woGS4Cbs/hOV0UgOMg4YpM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NHj/stJg17yFmb2/Oj7Unl2h7r0dT09GCFwj2ocpIxj4RdCVT6576sLbHdWuixwFM\n\t/rrpexYeU1Z7R/NKGPcH/RONmdAkiaOS/hGyutT6m57j6ofBERspd5bBfLXtx92fSa\n\t1CQ/SelzsroePhJYrNlmjl55m6PUWaGcN13WbKq0=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190207212119.30299-1-kieran.bingham@ideasonboard.com>\n\t<20190207212119.30299-5-kieran.bingham@ideasonboard.com>\n\t<20190208173223.GI4562@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<a64dc358-cb26-1b04-bb32-3e1a3eaa3f49@ideasonboard.com>","Date":"Mon, 11 Feb 2019 11:59:05 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190208173223.GI4562@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 11 Feb 2019 11:59:09 -0000"}},{"id":783,"web_url":"https://patchwork.libcamera.org/comment/783/","msgid":"<20190212085250.mrhaoxmj5uf7ilgb@uno.localdomain>","date":"2019-02-12T08:52:50","subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Feb 11, 2019 at 11:57:01AM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 07/02/2019 22:28, Jacopo Mondi wrote:\n> > Hi Kieran,\n> >\n> > On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote:\n> >> exportBuffers() can only operate on an existing BufferPool allocation. The\n> >\n> > \"an interanlly allocated BufferPool\" ?\n>\n> Possibly but I'm not sure how to word this to be accurate.\n>\n> exportBuffers requires a BufferPool to exist and have non-populated buffers.\n>\n> *exportBuffers()* is doing the allocation, so it's not right to say it\n> requries an internally allocated BufferPool - because it requires it to\n> be not yet allocated.\n\nCorrect, my bad for the confusion. whatever is fine, is a commit\nmessage...\n\n>\n>\n> >> pool identifies its size through its .count() method.\n> >>\n> >> Passing a count in to the exportBuffers() call is redundant and can be\n> >> incorrect if the value is not the same as the BufferPool size.\n> >>\n> >> Simplify the function and remove the unnecessary argument, correcting all uses\n> >> throughout the code base.\n> >>\n> >> While we're here, remove the createBuffers() helper from the V4L2DeviceTest\n> >> which only served to obfuscate which pool the buffers were being allocated for.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  src/libcamera/include/v4l2_device.h  |  2 +-\n> >>  src/libcamera/pipeline/ipu3/ipu3.cpp |  3 +--\n> >>  src/libcamera/pipeline/uvcvideo.cpp  |  2 +-\n> >>  src/libcamera/pipeline/vimc.cpp      |  2 +-\n> >>  src/libcamera/v4l2_device.cpp        | 15 ++++++---------\n> >>  test/v4l2_device/buffer_sharing.cpp  |  2 +-\n> >>  test/v4l2_device/capture_async.cpp   |  4 ++--\n> >>  test/v4l2_device/request_buffers.cpp |  4 ++--\n> >>  test/v4l2_device/stream_on_off.cpp   |  4 ++--\n> >>  test/v4l2_device/v4l2_device_test.h  |  2 --\n> >>  10 files changed, 17 insertions(+), 23 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> index 3acb5e466d64..98fa2110efb5 100644\n> >> --- a/src/libcamera/include/v4l2_device.h\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -97,7 +97,7 @@ public:\n> >>  \tint getFormat(V4L2DeviceFormat *format);\n> >>  \tint setFormat(V4L2DeviceFormat *format);\n> >>\n> >> -\tint exportBuffers(unsigned int count, BufferPool *pool);\n> >> +\tint exportBuffers(BufferPool *pool);\n> >>  \tint importBuffers(BufferPool *pool);\n> >>  \tint releaseBuffers();\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 34b03995ae31..677e127dd738 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n> >>  \tif (!cfg.bufferCount)\n> >>  \t\treturn -EINVAL;\n> >>\n> >> -\tint ret = data->cio2_->exportBuffers(cfg.bufferCount,\n> >> -\t\t\t\t\t     &stream->bufferPool());\n> >> +\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n> >>  \tif (ret) {\n> >>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n> >>  \t\treturn ret;\n> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> >> index ed8228bb2fc6..1a0dcb582670 100644\n> >> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> >> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> >> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n> >>\n> >>  \tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> >>\n> >> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> >> +\treturn video_->exportBuffers(&stream->bufferPool());\n> >>  }\n> >>\n> >>  int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)\n> >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> >> index 0e9ad7b59ee5..06627b1cb847 100644\n> >> --- a/src/libcamera/pipeline/vimc.cpp\n> >> +++ b/src/libcamera/pipeline/vimc.cpp\n> >> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)\n> >\n> >>\n> >>  \tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> >>\n> >> -\treturn video_->exportBuffers(cfg.bufferCount, &stream->bufferPool());\n> >> +\treturn video_->exportBuffers(&stream->bufferPool());\n> >>  }\n> >>\n> >>  int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream)\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index c654e6dd7b8d..c2e4d0ea8db2 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count)\n> >>  }\n> >>\n> >>  /**\n> >> - * \\brief Request \\a count buffers to be allocated from the device and stored in\n> >> - * the buffer pool provided.\n> >> - * \\param[in] count Number of buffers to allocate\n> >> + * \\brief Request buffers to be allocated from the device and stored in the\n> >> + *  buffer pool provided.\n> >>   * \\param[out] pool BufferPool to populate with buffers\n> >>   * \\return 0 on success or a negative error code otherwise\n> >>   */\n> >> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n> >> +int V4L2Device::exportBuffers(BufferPool *pool)\n> >>  {\n> >>  \tunsigned int allocatedBuffers;\n> >>  \tunsigned int i;\n> >> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool)\n> >>\n> >>  \tmemoryType_ = V4L2_MEMORY_MMAP;\n> >>\n> >> -\tret = requestBuffers(count);\n> >> +\tret = requestBuffers(pool->count());\n> >>  \tif (ret < 0)\n> >>  \t\treturn ret;\n> >>\n> >>  \tallocatedBuffers = ret;\n> >> -\tif (allocatedBuffers < count) {\n> >> +\tif (allocatedBuffers < pool->count()) {\n> >\n> > I have a feeling we discussed this, but could you drop\n> > allocatedBuffers now?\n> >\n>\n> requestBuffers returns a signed int. pool->count() is an unsigned int.\n>\n> the allocatedBuffers is essentially a cleaner (I hope) way to write the\n> expression such that it doesn't require casting of the signed/unsigned\n> types comparision.\n>\n> It's safe because the first test \"if (ret < 0)\" ensures that the value\n> can only be positive. Then the allocatedBuffers (unsigned) is set as the\n> (guaranteed postive) ret.\n\nGreat, please have my\n\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nAs I suspect a v2 will come, I'll review other patches in the next\niteration.\n\nThanks\n  j\n>\n>\n>\n> > The rest looks fine!\n> > Thanks\n> >    j\n> >\n> >>  \t\tLOG(V4L2, Error) << \"Not enough buffers provided by V4L2Device\";\n> >>  \t\trequestBuffers(0);\n> >>  \t\treturn -ENOMEM;\n> >>  \t}\n> >>\n> >> -\tcount = ret;\n> >> -\n> >>  \t/* Map the buffers. */\n> >> -\tfor (i = 0; i < count; ++i) {\n> >> +\tfor (i = 0; i < pool->count(); ++i) {\n> >>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> >>  \t\tstruct v4l2_buffer buf = {};\n> >>  \t\tstruct Buffer &buffer = pool->buffers()[i];\n> >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp\n> >> index 0e96f7b894bd..ca086f2728f1 100644\n> >> --- a/test/v4l2_device/buffer_sharing.cpp\n> >> +++ b/test/v4l2_device/buffer_sharing.cpp\n> >> @@ -80,7 +80,7 @@ protected:\n> >>\n> >>  \t\tpool_.createBuffers(bufferCount);\n> >>\n> >> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> >> +\t\tret = dev_->exportBuffers(&pool_);\n> >>  \t\tif (ret)\n> >>  \t\t\treturn TestFail;\n> >>\n> >> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp\n> >> index 7a0735f65535..d3e20ed86b98 100644\n> >> --- a/test/v4l2_device/capture_async.cpp\n> >> +++ b/test/v4l2_device/capture_async.cpp\n> >> @@ -38,9 +38,9 @@ protected:\n> >>  \t\tTimer timeout;\n> >>  \t\tint ret;\n> >>\n> >> -\t\tcreateBuffers(bufferCount);\n> >> +\t\tpool_.createBuffers(bufferCount);\n> >>\n> >> -\t\tret = dev_->exportBuffers(bufferCount, &pool_);\n> >> +\t\tret = dev_->exportBuffers(&pool_);\n> >>  \t\tif (ret)\n> >>  \t\t\treturn TestFail;\n> >>\n> >> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp\n> >> index bc6ff2c18a57..26d7d9528982 100644\n> >> --- a/test/v4l2_device/request_buffers.cpp\n> >> +++ b/test/v4l2_device/request_buffers.cpp\n> >> @@ -19,9 +19,9 @@ protected:\n> >>  \t\t */\n> >>  \t\tconst unsigned int bufferCount = 8;\n> >>\n> >> -\t\tcreateBuffers(bufferCount);\n> >> +\t\tpool_.createBuffers(bufferCount);\n> >>\n> >> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> >> +\t\tint ret = dev_->exportBuffers(&pool_);\n> >>  \t\tif (ret)\n> >>  \t\t\treturn TestFail;\n> >>\n> >> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp\n> >> index b564d2a2ab67..861d8d695813 100644\n> >> --- a/test/v4l2_device/stream_on_off.cpp\n> >> +++ b/test/v4l2_device/stream_on_off.cpp\n> >> @@ -14,9 +14,9 @@ protected:\n> >>  \t{\n> >>  \t\tconst unsigned int bufferCount = 8;\n> >>\n> >> -\t\tcreateBuffers(bufferCount);\n> >> +\t\tpool_.createBuffers(bufferCount);\n> >>\n> >> -\t\tint ret = dev_->exportBuffers(bufferCount, &pool_);\n> >> +\t\tint ret = dev_->exportBuffers(&pool_);\n> >>  \t\tif (ret)\n> >>  \t\t\treturn TestFail;\n> >>\n> >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n> >> index f22f0bb555d8..43539655f85b 100644\n> >> --- a/test/v4l2_device/v4l2_device_test.h\n> >> +++ b/test/v4l2_device/v4l2_device_test.h\n> >> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test\n> >>  public:\n> >>  \tV4L2DeviceTest() : dev_(nullptr){};\n> >>\n> >> -\tvoid createBuffers(unsigned int qty) { pool_.createBuffers(qty); }\n> >> -\n> >>  protected:\n> >>  \tint init();\n> >>  \tvoid cleanup();\n> >> --\n> >> 2.19.1\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E198260DBB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 12 Feb 2019 09:52:32 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 458F8200009;\n\tTue, 12 Feb 2019 08:52:32 +0000 (UTC)"],"Date":"Tue, 12 Feb 2019 09:52:50 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190212085250.mrhaoxmj5uf7ilgb@uno.localdomain>","References":"<20190207212119.30299-1-kieran.bingham@ideasonboard.com>\n\t<20190207212119.30299-5-kieran.bingham@ideasonboard.com>\n\t<20190207222829.kfu5z34h5rvrrlu3@uno.localdomain>\n\t<6c08f64a-f1a8-9bb7-6115-928966cd19dc@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wvmsdpsf42ig6stg\"","Content-Disposition":"inline","In-Reply-To":"<6c08f64a-f1a8-9bb7-6115-928966cd19dc@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify\n\texportBuffers()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 12 Feb 2019 08:52:33 -0000"}}]