[{"id":11414,"web_url":"https://patchwork.libcamera.org/comment/11414/","msgid":"<20200715232015.GI6144@pendragon.ideasonboard.com>","date":"2020-07-15T23:20:15","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Wed, Jul 15, 2020 at 10:39:11AM +0100, Naushir Patuck wrote:\n> Remove use of vcsm allocations and replace with dma heap allocations.\n> The pipeline handler now passes the fd of the allocation over to the IPA\n> instead of the raw pointer.\n> \n> Also use libcamera::FileDescriptor for fd lifetime management.\n> \n> This commit must be built alongside the accompanying BCM2835 ISP kernel\n> driver changes at https://github.com/raspberrypi/linux/pull/3715.\n> Otherwise a mismatch will cause undefined behavior.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/linux/bcm2835-isp.h | 4 +-\n> include/linux/vc_sm_cma_ioctl.h | 135 ----------------\n> src/ipa/raspberrypi/raspberrypi.cpp | 29 +++-\n> .../pipeline/raspberrypi/dma_heaps.cpp | 77 +++++++++\n> .../pipeline/raspberrypi/dma_heaps.h | 43 +++++\n> .../pipeline/raspberrypi/meson.build | 1 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++----\n> src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> 8 files changed, 160 insertions(+), 324 deletions(-)\n> delete mode 100644 include/linux/vc_sm_cma_ioctl.h\n> create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> delete mode 100644 src/libcamera/pipeline/raspberrypi/vcsm.h\n> \n> diff --git a/include/linux/bcm2835-isp.h b/include/linux/bcm2835-isp.h\n> index e7afc367..45abb681 100644\n> --- a/include/linux/bcm2835-isp.h\n> +++ b/include/linux/bcm2835-isp.h\n> @@ -108,7 +108,7 @@ enum bcm2835_isp_gain_format {\n> * @grid_stride:\tRow to row distance (in grid cells) between grid cells\n> *\t\t\tin the same horizontal location.\n> * @grid_height:\tHeight of lens shading tables in grid cells.\n> - * @mem_handle_table:\tMemory handle to the tables.\n> + * @dmabuf:\t\tdmabuf file handle containing the table.\n> * @ref_transform:\tReference transform - unsupported, please pass zero.\n> * @corner_sampled:\tWhether the gains are sampled at the corner points\n> *\t\t\tof the grid cells or in the cell centres.\n> @@ -120,7 +120,7 @@ struct bcm2835_isp_lens_shading {\n> \t__u32 grid_width;\n> \t__u32 grid_stride;\n> \t__u32 grid_height;\n> -\t__u32 mem_handle_table;\n> +\t__s32 dmabuf;\n> \t__u32 ref_transform;\n> \t__u32 corner_sampled;\n> \t__u32 gain_format;\n> diff --git a/include/linux/vc_sm_cma_ioctl.h b/include/linux/vc_sm_cma_ioctl.h\n> deleted file mode 100644\n> index 21b8758e..00000000\n> --- a/include/linux/vc_sm_cma_ioctl.h\n> +++ /dev/null\n> @@ -1,135 +0,0 @@\n> -/*\n> -Copyright (c) 2012, Broadcom Europe Ltd\n> -All rights reserved.\n> -\n> -Redistribution and use in source and binary forms, with or without\n> -modification, are permitted provided that the following conditions are met:\n> - * Redistributions of source code must retain the above copyright\n> - notice, this list of conditions and the following disclaimer.\n> - * Redistributions in binary form must reproduce the above copyright\n> - notice, this list of conditions and the following disclaimer in the\n> - documentation and/or other materials provided with the distribution.\n> - * Neither the name of the copyright holder nor the\n> - names of its contributors may be used to endorse or promote products\n> - derived from this software without specific prior written permission.\n> -\n> -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS \"AS IS\" AND\n> -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED\n> -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE\n> -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY\n> -DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES\n> -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;\n> -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND\n> -ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT\n> -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS\n> -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n> -*/\n> -\n> -/*\n> - * Copyright 2019 Raspberry Pi (Trading) Ltd. All rights reserved.\n> - *\n> - * Based on vmcs_sm_ioctl.h Copyright Broadcom Corporation.\n> - */\n> -\n> -#ifndef __VC_SM_CMA_IOCTL_H\n> -#define __VC_SM_CMA_IOCTL_H\n> -\n> -/* ---- Include Files ---------------------------------------------------- */\n> -\n> -#include \t/* Needed for standard types */\n> -\n> -#include \n> -\n> -/* ---- Constants and Types ---------------------------------------------- */\n> -\n> -#define VC_SM_CMA_RESOURCE_NAME 32\n> -#define VC_SM_CMA_RESOURCE_NAME_DEFAULT \"sm-host-resource\"\n> -\n> -/* Type define used to create unique IOCTL number */\n> -#define VC_SM_CMA_MAGIC_TYPE 'J'\n> -\n> -/* IOCTL commands on /dev/vc-sm-cma */\n> -enum vc_sm_cma_cmd_e {\n> -\tVC_SM_CMA_CMD_ALLOC = 0x5A,\t/* Start at 0x5A arbitrarily */\n> -\n> -\tVC_SM_CMA_CMD_IMPORT_DMABUF,\n> -\n> -\tVC_SM_CMA_CMD_CLEAN_INVALID2,\n> -\n> -\tVC_SM_CMA_CMD_LAST\t/* Do not delete */\n> -};\n> -\n> -/* Cache type supported, conveniently matches the user space definition in\n> - * user-vcsm.h.\n> - */\n> -enum vc_sm_cma_cache_e {\n> -\tVC_SM_CMA_CACHE_NONE,\n> -\tVC_SM_CMA_CACHE_HOST,\n> -\tVC_SM_CMA_CACHE_VC,\n> -\tVC_SM_CMA_CACHE_BOTH,\n> -};\n> -\n> -/* IOCTL Data structures */\n> -struct vc_sm_cma_ioctl_alloc {\n> -\t/* user -> kernel */\n> -\t__u32 size;\n> -\t__u32 num;\n> -\t__u32 cached;\t\t/* enum vc_sm_cma_cache_e */\n> -\t__u32 pad;\n> -\t__u8 name[VC_SM_CMA_RESOURCE_NAME];\n> -\n> -\t/* kernel -> user */\n> -\t__s32 handle;\n> -\t__u32 vc_handle;\n> -\t__u64 dma_addr;\n> -};\n> -\n> -struct vc_sm_cma_ioctl_import_dmabuf {\n> -\t/* user -> kernel */\n> -\t__s32 dmabuf_fd;\n> -\t__u32 cached;\t\t/* enum vc_sm_cma_cache_e */\n> -\t__u8 name[VC_SM_CMA_RESOURCE_NAME];\n> -\n> -\t/* kernel -> user */\n> -\t__s32 handle;\n> -\t__u32 vc_handle;\n> -\t__u32 size;\n> -\t__u32 pad;\n> -\t__u64 dma_addr;\n> -};\n> -\n> -/*\n> - * Cache functions to be set to struct vc_sm_cma_ioctl_clean_invalid2\n> - * invalidate_mode.\n> - */\n> -#define VC_SM_CACHE_OP_NOP 0x00\n> -#define VC_SM_CACHE_OP_INV 0x01\n> -#define VC_SM_CACHE_OP_CLEAN 0x02\n> -#define VC_SM_CACHE_OP_FLUSH 0x03\n> -\n> -struct vc_sm_cma_ioctl_clean_invalid2 {\n> -\t__u32 op_count;\n> -\t__u32 pad;\n> -\tstruct vc_sm_cma_ioctl_clean_invalid_block {\n> -\t\t__u32 invalidate_mode;\n> -\t\t__u32 block_count;\n> -\t\tvoid * /*__user */start_address;\n> -\t\t__u32 block_size;\n> -\t\t__u32 inter_block_stride;\n> -\t} s[0];\n> -};\n> -\n> -/* IOCTL numbers */\n> -#define VC_SM_CMA_IOCTL_MEM_ALLOC\\\n> -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_ALLOC,\\\n> -\t struct vc_sm_cma_ioctl_alloc)\n> -\n> -#define VC_SM_CMA_IOCTL_MEM_IMPORT_DMABUF\\\n> -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_IMPORT_DMABUF,\\\n> -\t struct vc_sm_cma_ioctl_import_dmabuf)\n> -\n> -#define VC_SM_CMA_IOCTL_MEM_CLEAN_INVALID2\\\n> -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_CLEAN_INVALID2,\\\n> -\t struct vc_sm_cma_ioctl_clean_invalid2)\n> -\n> -#endif /* __VC_SM_CMA_IOCTL_H */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index b1f27861..8efc9aa5 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -65,12 +66,14 @@ public:\n> \tIPARPi()\n> \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> \t\t frame_count_(0), check_count_(0), hide_count_(0),\n> -\t\t mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)\n> +\t\t mistrust_count_(0), lsTable_(nullptr)\n> \t{\n> \t}\n> \n> \t~IPARPi()\n> \t{\n> +\t\tif (lsTable_)\n> +\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> \t}\n> \n> \tint init(const IPASettings &settings) override;\n> @@ -137,7 +140,7 @@ private:\n> \t/* How many frames we should avoid running control algos on. */\n> \tunsigned int mistrust_count_;\n> \t/* LS table allocation passed in from the pipeline handler. */\n> -\tuint32_t lsTableHandle_;\n> +\tFileDescriptor lsTableHandle_;\n> \tvoid *lsTable_;\n> };\n> \n> @@ -357,8 +360,24 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t}\n> \n> \tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> -\t\tlsTableHandle_ = event.data[1];\n> +\t\t/* Remove any previous table, if there was one. */\n> +\t\tif (lsTable_)\n> +\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> +\n> +\t\tlsTable_ = nullptr;\n\nNot that it matters much, but I would have but the assignment inside the\nif block, it seems logical to me to group it with munmap.\n\n> +\n> +\t\t/* Map the LS table buffer into user space. */\n> +\t\tlsTableHandle_ = FileDescriptor(event.data[0]);\n> +\t\tif (lsTableHandle_.isValid()) {\n> +\t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> +\t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> +\n> +\t\t\tif (lsTable_ == MAP_FAILED) {\n> +\t\t\t\tLOG(IPARPI, Error) << \"dmaHeap mmap failure for LS table.\";\n> +\t\t\t\tlsTable_ = nullptr;\n> +\t\t\t}\n> +\t\t}\n> +\n> \t\tbreak;\n> \t}\n> \n> @@ -1026,7 +1045,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> \t\t.grid_width = w,\n> \t\t.grid_stride = w,\n> \t\t.grid_height = h,\n> -\t\t.mem_handle_table = lsTableHandle_,\n> +\t\t.dmabuf = lsTableHandle_.fd(),\n> \t\t.ref_transform = 0,\n> \t\t.corner_sampled = 1,\n> \t\t.gain_format = GAIN_FORMAT_U4P10\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> new file mode 100644\n> index 00000000..c29a1a25\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> @@ -0,0 +1,77 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * dma_heaps.h - Helper class for dma-heap allocations.\n> + */\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +#include \"dma_heaps.h\"\n\nThis should be the very first header to be included, to test that it can\nbe compiled self-contained. This helps avoiding future compilation\nbreakages when other headers are reworked.\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(RPI)\n> +\n> +namespace RPi {\n> +\n> +DmaHeap::DmaHeap()\n> +{\n> +\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n> +\tif (dmaHeapHandle_ == -1) {\n> +\t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n> +\t\tif (dmaHeapHandle_ == -1) {\n> +\t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> +\t\t}\n> +\t}\n> +}\n> +\n> +DmaHeap::~DmaHeap()\n> +{\n> +\tif (dmaHeapHandle_)\n> +\t\t::close(dmaHeapHandle_);\n> +}\n> +\n> +FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> +{\n> +\tunsigned int pageSize = getpagesize();\n\nThe getpagesize() man page states that \"Portable applications should\nemploy sysconf(_SC_PAGESIZE) instead of getpagesize()\". But do we need\nthis ? The dma-heap implementation rounds up the requested size to a\nmultiple of PAGE_SIZE internally. Of course none of the userspace API is\ndocumented, so whether this is an implementation detail or an API\nguarantee is not known :-@ How can we seriously merge an API these days\nwithout documentation ?\n\n> +\tint ret;\n> +\n> +\tif (!name)\n> +\t\treturn FileDescriptor();\n> +\n> +\t/* Ask for page aligned allocation. */\n> +\tsize = (size + pageSize - 1) & ~(pageSize - 1);\n> +\n> +\tstruct dma_heap_allocation_data alloc = {};\n> +\n> +\talloc.len = size;\n> +\talloc.fd_flags = O_CLOEXEC | O_RDWR;\n> +\n> +\tret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);\n> +\n> +\tif (ret < 0) {\n> +\t\tLOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> +\t\t\t\t<< name;\n> +\t\treturn FileDescriptor();\n> +\t}\n> +\n> +\tret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);\n> +\tif (ret < 0) {\n> +\t\tLOG(RPI, Error) << \"dmaHeap naming failure for \"\n> +\t\t\t\t<< name;\n> +\t\t::close(alloc.fd);\n> +\t\treturn FileDescriptor();\n> +\t}\n> +\n> +\treturn FileDescriptor(std::move(alloc.fd));\n> +}\n> +\n> +} /* namespace RPi */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> new file mode 100644\n> index 00000000..80892fd4\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> @@ -0,0 +1,43 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * dma_heaps.h - Helper class for dma-heap allocations.\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(RPI)\n> +\n> +namespace RPi {\n> +\n> +/*\n> + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma to\n> + * only have to worry about importing.\n> + *\n> + * Annoyingly, should the cma heap size be specified on the kernel command line\n> + * instead of DT, the heap gets named \"reserved\" instead.\n> + */\n> +#define DMA_HEAP_CMA_NAME \"/dev/dma_heap/linux,cma\"\n> +#define DMA_HEAP_CMA_ALT_NAME \"/dev/dma_heap/reserved\"\n\nI would move this to the .cpp file.\n\nReviewed-by: Laurent Pinchart \n\nPlease let me know if you would like me to make those small\nmodifications when applying the series, or if you would like to send a\nv3.\n\n> +\n> +class DmaHeap\n> +{\n> +public:\n> +\tDmaHeap();\n> +\t~DmaHeap();\n> +\tFileDescriptor alloc(const char *name, std::size_t size);\n> +\n> +private:\n> +\tint dmaHeapHandle_;\n> +};\n> +\n> +} /* namespace RPi */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__ */\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index dcfe07c5..ae0aed3b 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -1,6 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_sources += files([\n> + 'dma_heaps.cpp',\n> 'raspberrypi.cpp',\n> 'staggered_ctrl.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7450062f..431de44d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -13,6 +13,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -30,8 +31,8 @@\n> #include \"libcamera/internal/v4l2_controls.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> +#include \"dma_heaps.h\"\n> #include \"staggered_ctrl.h\"\n> -#include \"vcsm.h\"\n> \n> namespace libcamera {\n> \n> @@ -285,23 +286,13 @@ class RPiCameraData : public CameraData\n> {\n> public:\n> \tRPiCameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),\n> -\t\t state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)\n> +\t\t: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> +\t\t dropFrame_(false), ispOutputCount_(0)\n> \t{\n> \t}\n> \n> \t~RPiCameraData()\n> \t{\n> -\t\t/*\n> -\t\t * Free the LS table if we have allocated one. Another\n> -\t\t * allocation will occur in applyLS() with the appropriate\n> -\t\t * size.\n> -\t\t */\n> -\t\tif (lsTable_) {\n> -\t\t\tvcsm_.free(lsTable_);\n> -\t\t\tlsTable_ = nullptr;\n> -\t\t}\n> -\n> \t\t/* Stop the IPA proxy thread. */\n> \t\tif (ipa_)\n> \t\t\tipa_->stop();\n> @@ -330,9 +321,9 @@ public:\n> \t/* Buffers passed to the IPA. */\n> \tstd::vector ipaBuffers_;\n> \n> -\t/* VCSM allocation helper. */\n> -\t::RPi::Vcsm vcsm_;\n> -\tvoid *lsTable_;\n> +\t/* DMAHEAP allocation helper. */\n> +\tRPi::DmaHeap dmaHeap_;\n> +\tFileDescriptor lsTable_;\n> \n> \tRPi::StaggeredCtrl staggeredCtrl_;\n> \tuint32_t expectedSequence_;\n> @@ -1006,27 +997,16 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)\n> \tentityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());\n> \tentityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());\n> \n> -\t/* Allocate the lens shading table via vcsm and pass to the IPA. */\n> -\tif (!data->lsTable_) {\n> -\t\tdata->lsTable_ = data->vcsm_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> -\t\tuintptr_t ptr = reinterpret_cast(data->lsTable_);\n> -\n> -\t\tif (!data->lsTable_)\n> +\t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> +\tif (!data->lsTable_.isValid()) {\n> +\t\tdata->lsTable_ = data->dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> +\t\tif (!data->lsTable_.isValid())\n> \t\t\treturn -ENOMEM;\n> \n> -\t\t/*\n> -\t\t * The vcsm allocation will always be in the memory region\n> -\t\t * < 32-bits to allow Videocore to access the memory.\n> -\t\t *\n> -\t\t * \\todo Sending a pointer to the IPA is a workaround for\n> -\t\t * vc_sm_cma not yet supporting dmabuf. This will not work with\n> -\t\t * IPA module isolation and should be reworked when vc_sma_cma\n> -\t\t * will permit.\n> -\t\t */\n> +\t\t/* Allow the IPA to mmap the LS table via the file descriptor. */\n> \t\tIPAOperationData op;\n> \t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> -\t\top.data = { static_cast(ptr & 0xffffffff),\n> -\t\t\t data->vcsm_.getVCHandle(data->lsTable_) };\n> +\t\top.data = { static_cast(data->lsTable_.fd()) };\n> \t\tdata->ipa_->processEvent(op);\n> \t}\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/vcsm.h b/src/libcamera/pipeline/raspberrypi/vcsm.h\n> deleted file mode 100644\n> index bebe86a7..00000000\n> --- a/src/libcamera/pipeline/raspberrypi/vcsm.h\n> +++ /dev/null\n> @@ -1,149 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> - *\n> - * vcsm.h - Helper class for vcsm allocations.\n> - */\n> -#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> -#define __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> -\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> -namespace RPi {\n> -\n> -#define VCSM_CMA_DEVICE_NAME \"/dev/vcsm-cma\"\n> -\n> -class Vcsm\n> -{\n> -public:\n> -\tVcsm()\n> -\t{\n> -\t\tvcsmHandle_ = ::open(VCSM_CMA_DEVICE_NAME, O_RDWR, 0);\n> -\t\tif (vcsmHandle_ == -1) {\n> -\t\t\tstd::cerr << \"Could not open vcsm device: \"\n> -\t\t\t\t << VCSM_CMA_DEVICE_NAME;\n> -\t\t}\n> -\t}\n> -\n> -\t~Vcsm()\n> -\t{\n> -\t\t/* Free all existing allocations. */\n> -\t\tauto it = allocMap_.begin();\n> -\t\twhile (it != allocMap_.end())\n> -\t\t\tit = remove(it->first);\n> -\n> -\t\tif (vcsmHandle_)\n> -\t\t\t::close(vcsmHandle_);\n> -\t}\n> -\n> -\tvoid *alloc(const char *name, unsigned int size,\n> -\t\t vc_sm_cma_cache_e cache = VC_SM_CMA_CACHE_NONE)\n> -\t{\n> -\t\tunsigned int pageSize = getpagesize();\n> -\t\tvoid *user_ptr;\n> -\t\tint ret;\n> -\n> -\t\tif (!name)\n> -\t\t\treturn nullptr;\n> -\n> -\t\t/* Ask for page aligned allocation. */\n> -\t\tsize = (size + pageSize - 1) & ~(pageSize - 1);\n> -\n> -\t\tstruct vc_sm_cma_ioctl_alloc alloc;\n> -\t\tmemset(&alloc, 0, sizeof(alloc));\n> -\t\talloc.size = size;\n> -\t\talloc.num = 1;\n> -\t\talloc.cached = cache;\n> -\t\talloc.handle = 0;\n> -\t\tmemcpy(alloc.name, name, 32);\n> -\n> -\t\tret = ::ioctl(vcsmHandle_, VC_SM_CMA_IOCTL_MEM_ALLOC, &alloc);\n> -\n> -\t\tif (ret < 0 || alloc.handle < 0) {\n> -\t\t\tstd::cerr << \"vcsm allocation failure for \"\n> -\t\t\t\t << name << std::endl;\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\t/* Map the buffer into user space. */\n> -\t\tuser_ptr = ::mmap(0, alloc.size, PROT_READ | PROT_WRITE,\n> -\t\t\t\t MAP_SHARED, alloc.handle, 0);\n> -\n> -\t\tif (user_ptr == MAP_FAILED) {\n> -\t\t\tstd::cerr << \"vcsm mmap failure for \" << name << std::endl;\n> -\t\t\t::close(alloc.handle);\n> -\t\t\treturn nullptr;\n> -\t\t}\n> -\n> -\t\tstd::lock_guard lock(lock_);\n> -\t\tallocMap_.emplace(user_ptr, AllocInfo(alloc.handle,\n> -\t\t\t\t\t\t alloc.size, alloc.vc_handle));\n> -\n> -\t\treturn user_ptr;\n> -\t}\n> -\n> -\tvoid free(void *user_ptr)\n> -\t{\n> -\t\tstd::lock_guard lock(lock_);\n> -\t\tremove(user_ptr);\n> -\t}\n> -\n> -\tunsigned int getVCHandle(void *user_ptr)\n> -\t{\n> -\t\tstd::lock_guard lock(lock_);\n> -\t\tauto it = allocMap_.find(user_ptr);\n> -\t\tif (it != allocMap_.end())\n> -\t\t\treturn it->second.vcHandle;\n> -\n> -\t\treturn 0;\n> -\t}\n> -\n> -private:\n> -\tstruct AllocInfo {\n> -\t\tAllocInfo(int handle_, int size_, int vcHandle_)\n> -\t\t\t: handle(handle_), size(size_), vcHandle(vcHandle_)\n> -\t\t{\n> -\t\t}\n> -\n> -\t\tint handle;\n> -\t\tint size;\n> -\t\tuint32_t vcHandle;\n> -\t};\n> -\n> -\t/* Map of all allocations that have been requested. */\n> -\tusing AllocMap = std::map;\n> -\n> -\tAllocMap::iterator remove(void *user_ptr)\n> -\t{\n> -\t\tauto it = allocMap_.find(user_ptr);\n> -\t\tif (it != allocMap_.end()) {\n> -\t\t\tint handle = it->second.handle;\n> -\t\t\tint size = it->second.size;\n> -\t\t\t::munmap(user_ptr, size);\n> -\t\t\t::close(handle);\n> -\t\t\t/*\n> -\t\t\t * Remove the allocation from the map. This returns\n> -\t\t\t * an iterator to the next element.\n> -\t\t\t */\n> -\t\t\tit = allocMap_.erase(it);\n> -\t\t}\n> -\n> -\t\t/* Returns an iterator to the next element. */\n> -\t\treturn it;\n> -\t}\n> -\n> -\tAllocMap allocMap_;\n> -\tint vcsmHandle_;\n> -\tstd::mutex lock_;\n> -};\n> -\n> -} /* namespace RPi */\n> -\n> -#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1A62ABD792\n\tfor ;\n\tWed, 15 Jul 2020 23:20:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79514605AC;\n\tThu, 16 Jul 2020 01:20:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD1B560493\n\tfor ;\n\tThu, 16 Jul 2020 01:20:24 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B92C8564;\n\tThu, 16 Jul 2020 01:20:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dqhSPGKS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594855222;\n\tbh=J0rqvdWqxl5wIiNEJF7Q3MQLDSNkdOaTa1mZvUq+Ogs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dqhSPGKSkUSCEGhJYrWl2nyC7c9lDZgvnLD3xpf4s9B1y8AFUW8FHWxtI5MIMBVRj\n\tlSdv4i5TCzsYItYPXBqoH468t+8I4vNK/Zg5VfUAdPRA3ya5B0BTe+DaD2vuovuI/q\n\tCjH1dNH2zfgVyXjT5pTInwGAK/MuajolKIxn0Gg4=","Date":"Thu, 16 Jul 2020 02:20:15 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200715232015.GI6144@pendragon.ideasonboard.com>","References":"<20200715093911.554684-1-naush@raspberrypi.com>\n\t<20200715093911.554684-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200715093911.554684-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11415,"web_url":"https://patchwork.libcamera.org/comment/11415/","msgid":"<20200715232846.GK6144@pendragon.ideasonboard.com>","date":"2020-07-15T23:28:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Jul 16, 2020 at 02:20:15AM +0300, Laurent Pinchart wrote:\n> Hi Naush,\n> \n> Thank you for the patch.\n> \n> On Wed, Jul 15, 2020 at 10:39:11AM +0100, Naushir Patuck wrote:\n> > Remove use of vcsm allocations and replace with dma heap allocations.\n> > The pipeline handler now passes the fd of the allocation over to the IPA\n> > instead of the raw pointer.\n> > \n> > Also use libcamera::FileDescriptor for fd lifetime management.\n> > \n> > This commit must be built alongside the accompanying BCM2835 ISP kernel\n> > driver changes at https://github.com/raspberrypi/linux/pull/3715.\n> > Otherwise a mismatch will cause undefined behavior.\n> > \n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/linux/bcm2835-isp.h | 4 +-\n> > include/linux/vc_sm_cma_ioctl.h | 135 ----------------\n> > src/ipa/raspberrypi/raspberrypi.cpp | 29 +++-\n> > .../pipeline/raspberrypi/dma_heaps.cpp | 77 +++++++++\n> > .../pipeline/raspberrypi/dma_heaps.h | 43 +++++\n> > .../pipeline/raspberrypi/meson.build | 1 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++----\n> > src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> > 8 files changed, 160 insertions(+), 324 deletions(-)\n> > delete mode 100644 include/linux/vc_sm_cma_ioctl.h\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > delete mode 100644 src/libcamera/pipeline/raspberrypi/vcsm.h\n> > \n> > diff --git a/include/linux/bcm2835-isp.h b/include/linux/bcm2835-isp.h\n> > index e7afc367..45abb681 100644\n> > --- a/include/linux/bcm2835-isp.h\n> > +++ b/include/linux/bcm2835-isp.h\n> > @@ -108,7 +108,7 @@ enum bcm2835_isp_gain_format {\n> > * @grid_stride:\tRow to row distance (in grid cells) between grid cells\n> > *\t\t\tin the same horizontal location.\n> > * @grid_height:\tHeight of lens shading tables in grid cells.\n> > - * @mem_handle_table:\tMemory handle to the tables.\n> > + * @dmabuf:\t\tdmabuf file handle containing the table.\n> > * @ref_transform:\tReference transform - unsupported, please pass zero.\n> > * @corner_sampled:\tWhether the gains are sampled at the corner points\n> > *\t\t\tof the grid cells or in the cell centres.\n> > @@ -120,7 +120,7 @@ struct bcm2835_isp_lens_shading {\n> > \t__u32 grid_width;\n> > \t__u32 grid_stride;\n> > \t__u32 grid_height;\n> > -\t__u32 mem_handle_table;\n> > +\t__s32 dmabuf;\n> > \t__u32 ref_transform;\n> > \t__u32 corner_sampled;\n> > \t__u32 gain_format;\n> > diff --git a/include/linux/vc_sm_cma_ioctl.h b/include/linux/vc_sm_cma_ioctl.h\n> > deleted file mode 100644\n> > index 21b8758e..00000000\n> > --- a/include/linux/vc_sm_cma_ioctl.h\n> > +++ /dev/null\n> > @@ -1,135 +0,0 @@\n> > -/*\n> > -Copyright (c) 2012, Broadcom Europe Ltd\n> > -All rights reserved.\n> > -\n> > -Redistribution and use in source and binary forms, with or without\n> > -modification, are permitted provided that the following conditions are met:\n> > - * Redistributions of source code must retain the above copyright\n> > - notice, this list of conditions and the following disclaimer.\n> > - * Redistributions in binary form must reproduce the above copyright\n> > - notice, this list of conditions and the following disclaimer in the\n> > - documentation and/or other materials provided with the distribution.\n> > - * Neither the name of the copyright holder nor the\n> > - names of its contributors may be used to endorse or promote products\n> > - derived from this software without specific prior written permission.\n> > -\n> > -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS \"AS IS\" AND\n> > -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED\n> > -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE\n> > -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY\n> > -DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES\n> > -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;\n> > -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND\n> > -ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT\n> > -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS\n> > -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n> > -*/\n> > -\n> > -/*\n> > - * Copyright 2019 Raspberry Pi (Trading) Ltd. All rights reserved.\n> > - *\n> > - * Based on vmcs_sm_ioctl.h Copyright Broadcom Corporation.\n> > - */\n> > -\n> > -#ifndef __VC_SM_CMA_IOCTL_H\n> > -#define __VC_SM_CMA_IOCTL_H\n> > -\n> > -/* ---- Include Files ---------------------------------------------------- */\n> > -\n> > -#include \t/* Needed for standard types */\n> > -\n> > -#include \n> > -\n> > -/* ---- Constants and Types ---------------------------------------------- */\n> > -\n> > -#define VC_SM_CMA_RESOURCE_NAME 32\n> > -#define VC_SM_CMA_RESOURCE_NAME_DEFAULT \"sm-host-resource\"\n> > -\n> > -/* Type define used to create unique IOCTL number */\n> > -#define VC_SM_CMA_MAGIC_TYPE 'J'\n> > -\n> > -/* IOCTL commands on /dev/vc-sm-cma */\n> > -enum vc_sm_cma_cmd_e {\n> > -\tVC_SM_CMA_CMD_ALLOC = 0x5A,\t/* Start at 0x5A arbitrarily */\n> > -\n> > -\tVC_SM_CMA_CMD_IMPORT_DMABUF,\n> > -\n> > -\tVC_SM_CMA_CMD_CLEAN_INVALID2,\n> > -\n> > -\tVC_SM_CMA_CMD_LAST\t/* Do not delete */\n> > -};\n> > -\n> > -/* Cache type supported, conveniently matches the user space definition in\n> > - * user-vcsm.h.\n> > - */\n> > -enum vc_sm_cma_cache_e {\n> > -\tVC_SM_CMA_CACHE_NONE,\n> > -\tVC_SM_CMA_CACHE_HOST,\n> > -\tVC_SM_CMA_CACHE_VC,\n> > -\tVC_SM_CMA_CACHE_BOTH,\n> > -};\n> > -\n> > -/* IOCTL Data structures */\n> > -struct vc_sm_cma_ioctl_alloc {\n> > -\t/* user -> kernel */\n> > -\t__u32 size;\n> > -\t__u32 num;\n> > -\t__u32 cached;\t\t/* enum vc_sm_cma_cache_e */\n> > -\t__u32 pad;\n> > -\t__u8 name[VC_SM_CMA_RESOURCE_NAME];\n> > -\n> > -\t/* kernel -> user */\n> > -\t__s32 handle;\n> > -\t__u32 vc_handle;\n> > -\t__u64 dma_addr;\n> > -};\n> > -\n> > -struct vc_sm_cma_ioctl_import_dmabuf {\n> > -\t/* user -> kernel */\n> > -\t__s32 dmabuf_fd;\n> > -\t__u32 cached;\t\t/* enum vc_sm_cma_cache_e */\n> > -\t__u8 name[VC_SM_CMA_RESOURCE_NAME];\n> > -\n> > -\t/* kernel -> user */\n> > -\t__s32 handle;\n> > -\t__u32 vc_handle;\n> > -\t__u32 size;\n> > -\t__u32 pad;\n> > -\t__u64 dma_addr;\n> > -};\n> > -\n> > -/*\n> > - * Cache functions to be set to struct vc_sm_cma_ioctl_clean_invalid2\n> > - * invalidate_mode.\n> > - */\n> > -#define VC_SM_CACHE_OP_NOP 0x00\n> > -#define VC_SM_CACHE_OP_INV 0x01\n> > -#define VC_SM_CACHE_OP_CLEAN 0x02\n> > -#define VC_SM_CACHE_OP_FLUSH 0x03\n> > -\n> > -struct vc_sm_cma_ioctl_clean_invalid2 {\n> > -\t__u32 op_count;\n> > -\t__u32 pad;\n> > -\tstruct vc_sm_cma_ioctl_clean_invalid_block {\n> > -\t\t__u32 invalidate_mode;\n> > -\t\t__u32 block_count;\n> > -\t\tvoid * /*__user */start_address;\n> > -\t\t__u32 block_size;\n> > -\t\t__u32 inter_block_stride;\n> > -\t} s[0];\n> > -};\n> > -\n> > -/* IOCTL numbers */\n> > -#define VC_SM_CMA_IOCTL_MEM_ALLOC\\\n> > -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_ALLOC,\\\n> > -\t struct vc_sm_cma_ioctl_alloc)\n> > -\n> > -#define VC_SM_CMA_IOCTL_MEM_IMPORT_DMABUF\\\n> > -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_IMPORT_DMABUF,\\\n> > -\t struct vc_sm_cma_ioctl_import_dmabuf)\n> > -\n> > -#define VC_SM_CMA_IOCTL_MEM_CLEAN_INVALID2\\\n> > -\t_IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_CLEAN_INVALID2,\\\n> > -\t struct vc_sm_cma_ioctl_clean_invalid2)\n> > -\n> > -#endif /* __VC_SM_CMA_IOCTL_H */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index b1f27861..8efc9aa5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -65,12 +66,14 @@ public:\n> > \tIPARPi()\n> > \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> > \t\t frame_count_(0), check_count_(0), hide_count_(0),\n> > -\t\t mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)\n> > +\t\t mistrust_count_(0), lsTable_(nullptr)\n> > \t{\n> > \t}\n> > \n> > \t~IPARPi()\n> > \t{\n> > +\t\tif (lsTable_)\n> > +\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> > \t}\n> > \n> > \tint init(const IPASettings &settings) override;\n> > @@ -137,7 +140,7 @@ private:\n> > \t/* How many frames we should avoid running control algos on. */\n> > \tunsigned int mistrust_count_;\n> > \t/* LS table allocation passed in from the pipeline handler. */\n> > -\tuint32_t lsTableHandle_;\n> > +\tFileDescriptor lsTableHandle_;\n> > \tvoid *lsTable_;\n> > };\n> > \n> > @@ -357,8 +360,24 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \t}\n> > \n> > \tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> > -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> > -\t\tlsTableHandle_ = event.data[1];\n> > +\t\t/* Remove any previous table, if there was one. */\n> > +\t\tif (lsTable_)\n> > +\t\t\tmunmap(lsTable_, MAX_LS_GRID_SIZE);\n> > +\n> > +\t\tlsTable_ = nullptr;\n> \n> Not that it matters much, but I would have but the assignment inside the\n> if block, it seems logical to me to group it with munmap.\n> \n> > +\n> > +\t\t/* Map the LS table buffer into user space. */\n> > +\t\tlsTableHandle_ = FileDescriptor(event.data[0]);\n> > +\t\tif (lsTableHandle_.isValid()) {\n> > +\t\t\tlsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> > +\t\t\t\t\tMAP_SHARED, lsTableHandle_.fd(), 0);\n> > +\n> > +\t\t\tif (lsTable_ == MAP_FAILED) {\n> > +\t\t\t\tLOG(IPARPI, Error) << \"dmaHeap mmap failure for LS table.\";\n> > +\t\t\t\tlsTable_ = nullptr;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > \t\tbreak;\n> > \t}\n> > \n> > @@ -1026,7 +1045,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > \t\t.grid_width = w,\n> > \t\t.grid_stride = w,\n> > \t\t.grid_height = h,\n> > -\t\t.mem_handle_table = lsTableHandle_,\n> > +\t\t.dmabuf = lsTableHandle_.fd(),\n> > \t\t.ref_transform = 0,\n> > \t\t.corner_sampled = 1,\n> > \t\t.gain_format = GAIN_FORMAT_U4P10\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > new file mode 100644\n> > index 00000000..c29a1a25\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > @@ -0,0 +1,77 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * dma_heaps.h - Helper class for dma-heap allocations.\n> > + */\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +#include \"dma_heaps.h\"\n> \n> This should be the very first header to be included, to test that it can\n> be compiled self-contained. This helps avoiding future compilation\n> breakages when other headers are reworked.\n\nAnd that shows that the LOG_DECLARE_CATEGORY(RPI) statement in\ndma_heaps.h is not needed :-)\n\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(RPI)\n> > +\n> > +namespace RPi {\n> > +\n> > +DmaHeap::DmaHeap()\n> > +{\n> > +\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n> > +\tif (dmaHeapHandle_ == -1) {\n> > +\t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n> > +\t\tif (dmaHeapHandle_ == -1) {\n> > +\t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> > +\t\t}\n> > +\t}\n> > +}\n> > +\n> > +DmaHeap::~DmaHeap()\n> > +{\n> > +\tif (dmaHeapHandle_)\n> > +\t\t::close(dmaHeapHandle_);\n> > +}\n> > +\n> > +FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > +\tunsigned int pageSize = getpagesize();\n> \n> The getpagesize() man page states that \"Portable applications should\n> employ sysconf(_SC_PAGESIZE) instead of getpagesize()\". But do we need\n> this ? The dma-heap implementation rounds up the requested size to a\n> multiple of PAGE_SIZE internally. Of course none of the userspace API is\n> documented, so whether this is an implementation detail or an API\n> guarantee is not known :-@ How can we seriously merge an API these days\n> without documentation ?\n> \n> > +\tint ret;\n> > +\n> > +\tif (!name)\n> > +\t\treturn FileDescriptor();\n> > +\n> > +\t/* Ask for page aligned allocation. */\n> > +\tsize = (size + pageSize - 1) & ~(pageSize - 1);\n> > +\n> > +\tstruct dma_heap_allocation_data alloc = {};\n> > +\n> > +\talloc.len = size;\n> > +\talloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > +\n> > +\tret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > +\n> > +\tif (ret < 0) {\n> > +\t\tLOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > +\t\t\t\t<< name;\n> > +\t\treturn FileDescriptor();\n> > +\t}\n> > +\n> > +\tret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(RPI, Error) << \"dmaHeap naming failure for \"\n> > +\t\t\t\t<< name;\n> > +\t\t::close(alloc.fd);\n> > +\t\treturn FileDescriptor();\n> > +\t}\n> > +\n> > +\treturn FileDescriptor(std::move(alloc.fd));\n> > +}\n> > +\n> > +} /* namespace RPi */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > new file mode 100644\n> > index 00000000..80892fd4\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * dma_heaps.h - Helper class for dma-heap allocations.\n> > + */\n> > +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(RPI)\n> > +\n> > +namespace RPi {\n> > +\n> > +/*\n> > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma to\n> > + * only have to worry about importing.\n> > + *\n> > + * Annoyingly, should the cma heap size be specified on the kernel command line\n> > + * instead of DT, the heap gets named \"reserved\" instead.\n> > + */\n> > +#define DMA_HEAP_CMA_NAME \"/dev/dma_heap/linux,cma\"\n> > +#define DMA_HEAP_CMA_ALT_NAME \"/dev/dma_heap/reserved\"\n> \n> I would move this to the .cpp file.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> Please let me know if you would like me to make those small\n> modifications when applying the series, or if you would like to send a\n> v3.\n> \n> > +\n> > +class DmaHeap\n> > +{\n> > +public:\n> > +\tDmaHeap();\n> > +\t~DmaHeap();\n> > +\tFileDescriptor alloc(const char *name, std::size_t size);\n> > +\n> > +private:\n> > +\tint dmaHeapHandle_;\n> > +};\n> > +\n> > +} /* namespace RPi */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__ */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index dcfe07c5..ae0aed3b 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -1,6 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> > \n> > libcamera_sources += files([\n> > + 'dma_heaps.cpp',\n> > 'raspberrypi.cpp',\n> > 'staggered_ctrl.cpp',\n> > ])\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7450062f..431de44d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -13,6 +13,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -30,8 +31,8 @@\n> > #include \"libcamera/internal/v4l2_controls.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > \n> > +#include \"dma_heaps.h\"\n> > #include \"staggered_ctrl.h\"\n> > -#include \"vcsm.h\"\n> > \n> > namespace libcamera {\n> > \n> > @@ -285,23 +286,13 @@ class RPiCameraData : public CameraData\n> > {\n> > public:\n> > \tRPiCameraData(PipelineHandler *pipe)\n> > -\t\t: CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),\n> > -\t\t state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)\n> > +\t\t: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> > +\t\t dropFrame_(false), ispOutputCount_(0)\n> > \t{\n> > \t}\n> > \n> > \t~RPiCameraData()\n> > \t{\n> > -\t\t/*\n> > -\t\t * Free the LS table if we have allocated one. Another\n> > -\t\t * allocation will occur in applyLS() with the appropriate\n> > -\t\t * size.\n> > -\t\t */\n> > -\t\tif (lsTable_) {\n> > -\t\t\tvcsm_.free(lsTable_);\n> > -\t\t\tlsTable_ = nullptr;\n> > -\t\t}\n> > -\n> > \t\t/* Stop the IPA proxy thread. */\n> > \t\tif (ipa_)\n> > \t\t\tipa_->stop();\n> > @@ -330,9 +321,9 @@ public:\n> > \t/* Buffers passed to the IPA. */\n> > \tstd::vector ipaBuffers_;\n> > \n> > -\t/* VCSM allocation helper. */\n> > -\t::RPi::Vcsm vcsm_;\n> > -\tvoid *lsTable_;\n> > +\t/* DMAHEAP allocation helper. */\n> > +\tRPi::DmaHeap dmaHeap_;\n> > +\tFileDescriptor lsTable_;\n> > \n> > \tRPi::StaggeredCtrl staggeredCtrl_;\n> > \tuint32_t expectedSequence_;\n> > @@ -1006,27 +997,16 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)\n> > \tentityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());\n> > \tentityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());\n> > \n> > -\t/* Allocate the lens shading table via vcsm and pass to the IPA. */\n> > -\tif (!data->lsTable_) {\n> > -\t\tdata->lsTable_ = data->vcsm_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > -\t\tuintptr_t ptr = reinterpret_cast(data->lsTable_);\n> > -\n> > -\t\tif (!data->lsTable_)\n> > +\t/* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > +\tif (!data->lsTable_.isValid()) {\n> > +\t\tdata->lsTable_ = data->dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > +\t\tif (!data->lsTable_.isValid())\n> > \t\t\treturn -ENOMEM;\n> > \n> > -\t\t/*\n> > -\t\t * The vcsm allocation will always be in the memory region\n> > -\t\t * < 32-bits to allow Videocore to access the memory.\n> > -\t\t *\n> > -\t\t * \\todo Sending a pointer to the IPA is a workaround for\n> > -\t\t * vc_sm_cma not yet supporting dmabuf. This will not work with\n> > -\t\t * IPA module isolation and should be reworked when vc_sma_cma\n> > -\t\t * will permit.\n> > -\t\t */\n> > +\t\t/* Allow the IPA to mmap the LS table via the file descriptor. */\n> > \t\tIPAOperationData op;\n> > \t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> > -\t\top.data = { static_cast(ptr & 0xffffffff),\n> > -\t\t\t data->vcsm_.getVCHandle(data->lsTable_) };\n> > +\t\top.data = { static_cast(data->lsTable_.fd()) };\n> > \t\tdata->ipa_->processEvent(op);\n> > \t}\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/vcsm.h b/src/libcamera/pipeline/raspberrypi/vcsm.h\n> > deleted file mode 100644\n> > index bebe86a7..00000000\n> > --- a/src/libcamera/pipeline/raspberrypi/vcsm.h\n> > +++ /dev/null\n> > @@ -1,149 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> > - *\n> > - * vcsm.h - Helper class for vcsm allocations.\n> > - */\n> > -#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> > -#define __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> > -\n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -\n> > -namespace RPi {\n> > -\n> > -#define VCSM_CMA_DEVICE_NAME \"/dev/vcsm-cma\"\n> > -\n> > -class Vcsm\n> > -{\n> > -public:\n> > -\tVcsm()\n> > -\t{\n> > -\t\tvcsmHandle_ = ::open(VCSM_CMA_DEVICE_NAME, O_RDWR, 0);\n> > -\t\tif (vcsmHandle_ == -1) {\n> > -\t\t\tstd::cerr << \"Could not open vcsm device: \"\n> > -\t\t\t\t << VCSM_CMA_DEVICE_NAME;\n> > -\t\t}\n> > -\t}\n> > -\n> > -\t~Vcsm()\n> > -\t{\n> > -\t\t/* Free all existing allocations. */\n> > -\t\tauto it = allocMap_.begin();\n> > -\t\twhile (it != allocMap_.end())\n> > -\t\t\tit = remove(it->first);\n> > -\n> > -\t\tif (vcsmHandle_)\n> > -\t\t\t::close(vcsmHandle_);\n> > -\t}\n> > -\n> > -\tvoid *alloc(const char *name, unsigned int size,\n> > -\t\t vc_sm_cma_cache_e cache = VC_SM_CMA_CACHE_NONE)\n> > -\t{\n> > -\t\tunsigned int pageSize = getpagesize();\n> > -\t\tvoid *user_ptr;\n> > -\t\tint ret;\n> > -\n> > -\t\tif (!name)\n> > -\t\t\treturn nullptr;\n> > -\n> > -\t\t/* Ask for page aligned allocation. */\n> > -\t\tsize = (size + pageSize - 1) & ~(pageSize - 1);\n> > -\n> > -\t\tstruct vc_sm_cma_ioctl_alloc alloc;\n> > -\t\tmemset(&alloc, 0, sizeof(alloc));\n> > -\t\talloc.size = size;\n> > -\t\talloc.num = 1;\n> > -\t\talloc.cached = cache;\n> > -\t\talloc.handle = 0;\n> > -\t\tmemcpy(alloc.name, name, 32);\n> > -\n> > -\t\tret = ::ioctl(vcsmHandle_, VC_SM_CMA_IOCTL_MEM_ALLOC, &alloc);\n> > -\n> > -\t\tif (ret < 0 || alloc.handle < 0) {\n> > -\t\t\tstd::cerr << \"vcsm allocation failure for \"\n> > -\t\t\t\t << name << std::endl;\n> > -\t\t\treturn nullptr;\n> > -\t\t}\n> > -\n> > -\t\t/* Map the buffer into user space. */\n> > -\t\tuser_ptr = ::mmap(0, alloc.size, PROT_READ | PROT_WRITE,\n> > -\t\t\t\t MAP_SHARED, alloc.handle, 0);\n> > -\n> > -\t\tif (user_ptr == MAP_FAILED) {\n> > -\t\t\tstd::cerr << \"vcsm mmap failure for \" << name << std::endl;\n> > -\t\t\t::close(alloc.handle);\n> > -\t\t\treturn nullptr;\n> > -\t\t}\n> > -\n> > -\t\tstd::lock_guard lock(lock_);\n> > -\t\tallocMap_.emplace(user_ptr, AllocInfo(alloc.handle,\n> > -\t\t\t\t\t\t alloc.size, alloc.vc_handle));\n> > -\n> > -\t\treturn user_ptr;\n> > -\t}\n> > -\n> > -\tvoid free(void *user_ptr)\n> > -\t{\n> > -\t\tstd::lock_guard lock(lock_);\n> > -\t\tremove(user_ptr);\n> > -\t}\n> > -\n> > -\tunsigned int getVCHandle(void *user_ptr)\n> > -\t{\n> > -\t\tstd::lock_guard lock(lock_);\n> > -\t\tauto it = allocMap_.find(user_ptr);\n> > -\t\tif (it != allocMap_.end())\n> > -\t\t\treturn it->second.vcHandle;\n> > -\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > -private:\n> > -\tstruct AllocInfo {\n> > -\t\tAllocInfo(int handle_, int size_, int vcHandle_)\n> > -\t\t\t: handle(handle_), size(size_), vcHandle(vcHandle_)\n> > -\t\t{\n> > -\t\t}\n> > -\n> > -\t\tint handle;\n> > -\t\tint size;\n> > -\t\tuint32_t vcHandle;\n> > -\t};\n> > -\n> > -\t/* Map of all allocations that have been requested. */\n> > -\tusing AllocMap = std::map;\n> > -\n> > -\tAllocMap::iterator remove(void *user_ptr)\n> > -\t{\n> > -\t\tauto it = allocMap_.find(user_ptr);\n> > -\t\tif (it != allocMap_.end()) {\n> > -\t\t\tint handle = it->second.handle;\n> > -\t\t\tint size = it->second.size;\n> > -\t\t\t::munmap(user_ptr, size);\n> > -\t\t\t::close(handle);\n> > -\t\t\t/*\n> > -\t\t\t * Remove the allocation from the map. This returns\n> > -\t\t\t * an iterator to the next element.\n> > -\t\t\t */\n> > -\t\t\tit = allocMap_.erase(it);\n> > -\t\t}\n> > -\n> > -\t\t/* Returns an iterator to the next element. */\n> > -\t\treturn it;\n> > -\t}\n> > -\n> > -\tAllocMap allocMap_;\n> > -\tint vcsmHandle_;\n> > -\tstd::mutex lock_;\n> > -};\n> > -\n> > -} /* namespace RPi */\n> > -\n> > -#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3DDD3BD790\n\tfor ;\n\tWed, 15 Jul 2020 23:28:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFFC760E9D;\n\tThu, 16 Jul 2020 01:28:56 +0200 (CEST)","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 F1AD760493\n\tfor ;\n\tThu, 16 Jul 2020 01:28:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 460FB564;\n\tThu, 16 Jul 2020 01:28:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ovP2SZIG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594855734;\n\tbh=i+BmzF1fsHT0jeYDd7UNIBXdNf9+EjZbsyW+fc1OUgM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ovP2SZIGZwkn/VrgeZUkOlsq5cFrsQy/7l/I8AuNdkaJpWvkihn13VI3VhX+Wpfru\n\tDrosF2P0KlFSAu4oYZUD43Sdm3+691FF0dShuZgj3AVdSBFFnFAbfCCdhcf/ulMoNj\n\tGQkFhxUFOdj6YhwfeuQvlRKx95zpSaEhM6Boyw5o=","Date":"Thu, 16 Jul 2020 02:28:46 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200715232846.GK6144@pendragon.ideasonboard.com>","References":"<20200715093911.554684-1-naush@raspberrypi.com>\n\t<20200715093911.554684-2-naush@raspberrypi.com>\n\t<20200715232015.GI6144@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200715232015.GI6144@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11417,"web_url":"https://patchwork.libcamera.org/comment/11417/","msgid":"","date":"2020-07-16T07:23:36","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThanks for the comments.\n\nOn Thu, 16 Jul 2020 at 00:20, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Wed, Jul 15, 2020 at 10:39:11AM +0100, Naushir Patuck wrote:\n> > Remove use of vcsm allocations and replace with dma heap allocations.\n> > The pipeline handler now passes the fd of the allocation over to the IPA\n> > instead of the raw pointer.\n> >\n> > Also use libcamera::FileDescriptor for fd lifetime management.\n> >\n> > This commit must be built alongside the accompanying BCM2835 ISP kernel\n> > driver changes at https://github.com/raspberrypi/linux/pull/3715.\n> > Otherwise a mismatch will cause undefined behavior.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/linux/bcm2835-isp.h | 4 +-\n> > include/linux/vc_sm_cma_ioctl.h | 135 ----------------\n> > src/ipa/raspberrypi/raspberrypi.cpp | 29 +++-\n> > .../pipeline/raspberrypi/dma_heaps.cpp | 77 +++++++++\n> > .../pipeline/raspberrypi/dma_heaps.h | 43 +++++\n> > .../pipeline/raspberrypi/meson.build | 1 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++----\n> > src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> > 8 files changed, 160 insertions(+), 324 deletions(-)\n> > delete mode 100644 include/linux/vc_sm_cma_ioctl.h\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > create mode 100644 src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > delete mode 100644 src/libcamera/pipeline/raspberrypi/vcsm.h\n> >\n> > diff --git a/include/linux/bcm2835-isp.h b/include/linux/bcm2835-isp.h\n> > index e7afc367..45abb681 100644\n> > --- a/include/linux/bcm2835-isp.h\n> > +++ b/include/linux/bcm2835-isp.h\n> > @@ -108,7 +108,7 @@ enum bcm2835_isp_gain_format {\n> > * @grid_stride: Row to row distance (in grid cells) between grid cells\n> > * in the same horizontal location.\n> > * @grid_height: Height of lens shading tables in grid cells.\n> > - * @mem_handle_table: Memory handle to the tables.\n> > + * @dmabuf: dmabuf file handle containing the table.\n> > * @ref_transform: Reference transform - unsupported, please pass zero.\n> > * @corner_sampled: Whether the gains are sampled at the corner points\n> > * of the grid cells or in the cell centres.\n> > @@ -120,7 +120,7 @@ struct bcm2835_isp_lens_shading {\n> > __u32 grid_width;\n> > __u32 grid_stride;\n> > __u32 grid_height;\n> > - __u32 mem_handle_table;\n> > + __s32 dmabuf;\n> > __u32 ref_transform;\n> > __u32 corner_sampled;\n> > __u32 gain_format;\n> > diff --git a/include/linux/vc_sm_cma_ioctl.h b/include/linux/vc_sm_cma_ioctl.h\n> > deleted file mode 100644\n> > index 21b8758e..00000000\n> > --- a/include/linux/vc_sm_cma_ioctl.h\n> > +++ /dev/null\n> > @@ -1,135 +0,0 @@\n> > -/*\n> > -Copyright (c) 2012, Broadcom Europe Ltd\n> > -All rights reserved.\n> > -\n> > -Redistribution and use in source and binary forms, with or without\n> > -modification, are permitted provided that the following conditions are met:\n> > - * Redistributions of source code must retain the above copyright\n> > - notice, this list of conditions and the following disclaimer.\n> > - * Redistributions in binary form must reproduce the above copyright\n> > - notice, this list of conditions and the following disclaimer in the\n> > - documentation and/or other materials provided with the distribution.\n> > - * Neither the name of the copyright holder nor the\n> > - names of its contributors may be used to endorse or promote products\n> > - derived from this software without specific prior written permission.\n> > -\n> > -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS \"AS IS\" AND\n> > -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED\n> > -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE\n> > -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY\n> > -DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES\n> > -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;\n> > -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND\n> > -ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT\n> > -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS\n> > -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n> > -*/\n> > -\n> > -/*\n> > - * Copyright 2019 Raspberry Pi (Trading) Ltd. All rights reserved.\n> > - *\n> > - * Based on vmcs_sm_ioctl.h Copyright Broadcom Corporation.\n> > - */\n> > -\n> > -#ifndef __VC_SM_CMA_IOCTL_H\n> > -#define __VC_SM_CMA_IOCTL_H\n> > -\n> > -/* ---- Include Files ---------------------------------------------------- */\n> > -\n> > -#include /* Needed for standard types */\n> > -\n> > -#include \n> > -\n> > -/* ---- Constants and Types ---------------------------------------------- */\n> > -\n> > -#define VC_SM_CMA_RESOURCE_NAME 32\n> > -#define VC_SM_CMA_RESOURCE_NAME_DEFAULT \"sm-host-resource\"\n> > -\n> > -/* Type define used to create unique IOCTL number */\n> > -#define VC_SM_CMA_MAGIC_TYPE 'J'\n> > -\n> > -/* IOCTL commands on /dev/vc-sm-cma */\n> > -enum vc_sm_cma_cmd_e {\n> > - VC_SM_CMA_CMD_ALLOC = 0x5A, /* Start at 0x5A arbitrarily */\n> > -\n> > - VC_SM_CMA_CMD_IMPORT_DMABUF,\n> > -\n> > - VC_SM_CMA_CMD_CLEAN_INVALID2,\n> > -\n> > - VC_SM_CMA_CMD_LAST /* Do not delete */\n> > -};\n> > -\n> > -/* Cache type supported, conveniently matches the user space definition in\n> > - * user-vcsm.h.\n> > - */\n> > -enum vc_sm_cma_cache_e {\n> > - VC_SM_CMA_CACHE_NONE,\n> > - VC_SM_CMA_CACHE_HOST,\n> > - VC_SM_CMA_CACHE_VC,\n> > - VC_SM_CMA_CACHE_BOTH,\n> > -};\n> > -\n> > -/* IOCTL Data structures */\n> > -struct vc_sm_cma_ioctl_alloc {\n> > - /* user -> kernel */\n> > - __u32 size;\n> > - __u32 num;\n> > - __u32 cached; /* enum vc_sm_cma_cache_e */\n> > - __u32 pad;\n> > - __u8 name[VC_SM_CMA_RESOURCE_NAME];\n> > -\n> > - /* kernel -> user */\n> > - __s32 handle;\n> > - __u32 vc_handle;\n> > - __u64 dma_addr;\n> > -};\n> > -\n> > -struct vc_sm_cma_ioctl_import_dmabuf {\n> > - /* user -> kernel */\n> > - __s32 dmabuf_fd;\n> > - __u32 cached; /* enum vc_sm_cma_cache_e */\n> > - __u8 name[VC_SM_CMA_RESOURCE_NAME];\n> > -\n> > - /* kernel -> user */\n> > - __s32 handle;\n> > - __u32 vc_handle;\n> > - __u32 size;\n> > - __u32 pad;\n> > - __u64 dma_addr;\n> > -};\n> > -\n> > -/*\n> > - * Cache functions to be set to struct vc_sm_cma_ioctl_clean_invalid2\n> > - * invalidate_mode.\n> > - */\n> > -#define VC_SM_CACHE_OP_NOP 0x00\n> > -#define VC_SM_CACHE_OP_INV 0x01\n> > -#define VC_SM_CACHE_OP_CLEAN 0x02\n> > -#define VC_SM_CACHE_OP_FLUSH 0x03\n> > -\n> > -struct vc_sm_cma_ioctl_clean_invalid2 {\n> > - __u32 op_count;\n> > - __u32 pad;\n> > - struct vc_sm_cma_ioctl_clean_invalid_block {\n> > - __u32 invalidate_mode;\n> > - __u32 block_count;\n> > - void * /*__user */start_address;\n> > - __u32 block_size;\n> > - __u32 inter_block_stride;\n> > - } s[0];\n> > -};\n> > -\n> > -/* IOCTL numbers */\n> > -#define VC_SM_CMA_IOCTL_MEM_ALLOC\\\n> > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_ALLOC,\\\n> > - struct vc_sm_cma_ioctl_alloc)\n> > -\n> > -#define VC_SM_CMA_IOCTL_MEM_IMPORT_DMABUF\\\n> > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_IMPORT_DMABUF,\\\n> > - struct vc_sm_cma_ioctl_import_dmabuf)\n> > -\n> > -#define VC_SM_CMA_IOCTL_MEM_CLEAN_INVALID2\\\n> > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_CLEAN_INVALID2,\\\n> > - struct vc_sm_cma_ioctl_clean_invalid2)\n> > -\n> > -#endif /* __VC_SM_CMA_IOCTL_H */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index b1f27861..8efc9aa5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -65,12 +66,14 @@ public:\n> > IPARPi()\n> > : lastMode_({}), controller_(), controllerInit_(false),\n> > frame_count_(0), check_count_(0), hide_count_(0),\n> > - mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)\n> > + mistrust_count_(0), lsTable_(nullptr)\n> > {\n> > }\n> >\n> > ~IPARPi()\n> > {\n> > + if (lsTable_)\n> > + munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > }\n> >\n> > int init(const IPASettings &settings) override;\n> > @@ -137,7 +140,7 @@ private:\n> > /* How many frames we should avoid running control algos on. */\n> > unsigned int mistrust_count_;\n> > /* LS table allocation passed in from the pipeline handler. */\n> > - uint32_t lsTableHandle_;\n> > + FileDescriptor lsTableHandle_;\n> > void *lsTable_;\n> > };\n> >\n> > @@ -357,8 +360,24 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > }\n> >\n> > case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> > - lsTable_ = reinterpret_cast(event.data[0]);\n> > - lsTableHandle_ = event.data[1];\n> > + /* Remove any previous table, if there was one. */\n> > + if (lsTable_)\n> > + munmap(lsTable_, MAX_LS_GRID_SIZE);\n> > +\n> > + lsTable_ = nullptr;\n>\n> Not that it matters much, but I would have but the assignment inside the\n> if block, it seems logical to me to group it with munmap.\n>\n> > +\n> > + /* Map the LS table buffer into user space. */\n> > + lsTableHandle_ = FileDescriptor(event.data[0]);\n> > + if (lsTableHandle_.isValid()) {\n> > + lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,\n> > + MAP_SHARED, lsTableHandle_.fd(), 0);\n> > +\n> > + if (lsTable_ == MAP_FAILED) {\n> > + LOG(IPARPI, Error) << \"dmaHeap mmap failure for LS table.\";\n> > + lsTable_ = nullptr;\n> > + }\n> > + }\n> > +\n> > break;\n> > }\n> >\n> > @@ -1026,7 +1045,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)\n> > .grid_width = w,\n> > .grid_stride = w,\n> > .grid_height = h,\n> > - .mem_handle_table = lsTableHandle_,\n> > + .dmabuf = lsTableHandle_.fd(),\n> > .ref_transform = 0,\n> > .corner_sampled = 1,\n> > .gain_format = GAIN_FORMAT_U4P10\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > new file mode 100644\n> > index 00000000..c29a1a25\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > @@ -0,0 +1,77 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * dma_heaps.h - Helper class for dma-heap allocations.\n> > + */\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +#include \"dma_heaps.h\"\n>\n> This should be the very first header to be included, to test that it can\n> be compiled self-contained. This helps avoiding future compilation\n> breakages when other headers are reworked.\n>\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(RPI)\n> > +\n> > +namespace RPi {\n> > +\n> > +DmaHeap::DmaHeap()\n> > +{\n> > + dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n> > + if (dmaHeapHandle_ == -1) {\n> > + dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n> > + if (dmaHeapHandle_ == -1) {\n> > + LOG(RPI, Error) << \"Could not open dmaHeap device\";\n> > + }\n> > + }\n> > +}\n> > +\n> > +DmaHeap::~DmaHeap()\n> > +{\n> > + if (dmaHeapHandle_)\n> > + ::close(dmaHeapHandle_);\n> > +}\n> > +\n> > +FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> > +{\n> > + unsigned int pageSize = getpagesize();\n>\n> The getpagesize() man page states that \"Portable applications should\n> employ sysconf(_SC_PAGESIZE) instead of getpagesize()\". But do we need\n> this ? The dma-heap implementation rounds up the requested size to a\n> multiple of PAGE_SIZE internally. Of course none of the userspace API is\n> documented, so whether this is an implementation detail or an API\n> guarantee is not known :-@ How can we seriously merge an API these days\n> without documentation ?\n\nI think we can probably get rid of the getpagesize() usage and\nalignment in that case.\n\n>\n> > + int ret;\n> > +\n> > + if (!name)\n> > + return FileDescriptor();\n> > +\n> > + /* Ask for page aligned allocation. */\n> > + size = (size + pageSize - 1) & ~(pageSize - 1);\n> > +\n> > + struct dma_heap_allocation_data alloc = {};\n> > +\n> > + alloc.len = size;\n> > + alloc.fd_flags = O_CLOEXEC | O_RDWR;\n> > +\n> > + ret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);\n> > +\n> > + if (ret < 0) {\n> > + LOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> > + << name;\n> > + return FileDescriptor();\n> > + }\n> > +\n> > + ret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);\n> > + if (ret < 0) {\n> > + LOG(RPI, Error) << \"dmaHeap naming failure for \"\n> > + << name;\n> > + ::close(alloc.fd);\n> > + return FileDescriptor();\n> > + }\n> > +\n> > + return FileDescriptor(std::move(alloc.fd));\n> > +}\n> > +\n> > +} /* namespace RPi */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > new file mode 100644\n> > index 00000000..80892fd4\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > @@ -0,0 +1,43 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * dma_heaps.h - Helper class for dma-heap allocations.\n> > + */\n> > +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(RPI)\n> > +\n> > +namespace RPi {\n> > +\n> > +/*\n> > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma to\n> > + * only have to worry about importing.\n> > + *\n> > + * Annoyingly, should the cma heap size be specified on the kernel command line\n> > + * instead of DT, the heap gets named \"reserved\" instead.\n> > + */\n> > +#define DMA_HEAP_CMA_NAME \"/dev/dma_heap/linux,cma\"\n> > +#define DMA_HEAP_CMA_ALT_NAME \"/dev/dma_heap/reserved\"\n>\n> I would move this to the .cpp file.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> Please let me know if you would like me to make those small\n> modifications when applying the series, or if you would like to send a\n> v3.\n\nHappy for you to go ahead and make the modifications before\nsubmitting. Could you give me a quick email before merging, so I can\nget Phil to merge the kernel changes at around the same time please?\n\n\n>\n> > +\n> > +class DmaHeap\n> > +{\n> > +public:\n> > + DmaHeap();\n> > + ~DmaHeap();\n> > + FileDescriptor alloc(const char *name, std::size_t size);\n> > +\n> > +private:\n> > + int dmaHeapHandle_;\n> > +};\n> > +\n> > +} /* namespace RPi */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__ */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> > index dcfe07c5..ae0aed3b 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> > +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> > @@ -1,6 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > libcamera_sources += files([\n> > + 'dma_heaps.cpp',\n> > 'raspberrypi.cpp',\n> > 'staggered_ctrl.cpp',\n> > ])\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7450062f..431de44d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -13,6 +13,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -30,8 +31,8 @@\n> > #include \"libcamera/internal/v4l2_controls.h\"\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> >\n> > +#include \"dma_heaps.h\"\n> > #include \"staggered_ctrl.h\"\n> > -#include \"vcsm.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -285,23 +286,13 @@ class RPiCameraData : public CameraData\n> > {\n> > public:\n> > RPiCameraData(PipelineHandler *pipe)\n> > - : CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),\n> > - state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)\n> > + : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> > + dropFrame_(false), ispOutputCount_(0)\n> > {\n> > }\n> >\n> > ~RPiCameraData()\n> > {\n> > - /*\n> > - * Free the LS table if we have allocated one. Another\n> > - * allocation will occur in applyLS() with the appropriate\n> > - * size.\n> > - */\n> > - if (lsTable_) {\n> > - vcsm_.free(lsTable_);\n> > - lsTable_ = nullptr;\n> > - }\n> > -\n> > /* Stop the IPA proxy thread. */\n> > if (ipa_)\n> > ipa_->stop();\n> > @@ -330,9 +321,9 @@ public:\n> > /* Buffers passed to the IPA. */\n> > std::vector ipaBuffers_;\n> >\n> > - /* VCSM allocation helper. */\n> > - ::RPi::Vcsm vcsm_;\n> > - void *lsTable_;\n> > + /* DMAHEAP allocation helper. */\n> > + RPi::DmaHeap dmaHeap_;\n> > + FileDescriptor lsTable_;\n> >\n> > RPi::StaggeredCtrl staggeredCtrl_;\n> > uint32_t expectedSequence_;\n> > @@ -1006,27 +997,16 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)\n> > entityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());\n> > entityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());\n> >\n> > - /* Allocate the lens shading table via vcsm and pass to the IPA. */\n> > - if (!data->lsTable_) {\n> > - data->lsTable_ = data->vcsm_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > - uintptr_t ptr = reinterpret_cast(data->lsTable_);\n> > -\n> > - if (!data->lsTable_)\n> > + /* Allocate the lens shading table via dmaHeap and pass to the IPA. */\n> > + if (!data->lsTable_.isValid()) {\n> > + data->lsTable_ = data->dmaHeap_.alloc(\"ls_grid\", MAX_LS_GRID_SIZE);\n> > + if (!data->lsTable_.isValid())\n> > return -ENOMEM;\n> >\n> > - /*\n> > - * The vcsm allocation will always be in the memory region\n> > - * < 32-bits to allow Videocore to access the memory.\n> > - *\n> > - * \\todo Sending a pointer to the IPA is a workaround for\n> > - * vc_sm_cma not yet supporting dmabuf. This will not work with\n> > - * IPA module isolation and should be reworked when vc_sma_cma\n> > - * will permit.\n> > - */\n> > + /* Allow the IPA to mmap the LS table via the file descriptor. */\n> > IPAOperationData op;\n> > op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> > - op.data = { static_cast(ptr & 0xffffffff),\n> > - data->vcsm_.getVCHandle(data->lsTable_) };\n> > + op.data = { static_cast(data->lsTable_.fd()) };\n> > data->ipa_->processEvent(op);\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/vcsm.h b/src/libcamera/pipeline/raspberrypi/vcsm.h\n> > deleted file mode 100644\n> > index bebe86a7..00000000\n> > --- a/src/libcamera/pipeline/raspberrypi/vcsm.h\n> > +++ /dev/null\n> > @@ -1,149 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> > - *\n> > - * vcsm.h - Helper class for vcsm allocations.\n> > - */\n> > -#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> > -#define __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__\n> > -\n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -\n> > -namespace RPi {\n> > -\n> > -#define VCSM_CMA_DEVICE_NAME \"/dev/vcsm-cma\"\n> > -\n> > -class Vcsm\n> > -{\n> > -public:\n> > - Vcsm()\n> > - {\n> > - vcsmHandle_ = ::open(VCSM_CMA_DEVICE_NAME, O_RDWR, 0);\n> > - if (vcsmHandle_ == -1) {\n> > - std::cerr << \"Could not open vcsm device: \"\n> > - << VCSM_CMA_DEVICE_NAME;\n> > - }\n> > - }\n> > -\n> > - ~Vcsm()\n> > - {\n> > - /* Free all existing allocations. */\n> > - auto it = allocMap_.begin();\n> > - while (it != allocMap_.end())\n> > - it = remove(it->first);\n> > -\n> > - if (vcsmHandle_)\n> > - ::close(vcsmHandle_);\n> > - }\n> > -\n> > - void *alloc(const char *name, unsigned int size,\n> > - vc_sm_cma_cache_e cache = VC_SM_CMA_CACHE_NONE)\n> > - {\n> > - unsigned int pageSize = getpagesize();\n> > - void *user_ptr;\n> > - int ret;\n> > -\n> > - if (!name)\n> > - return nullptr;\n> > -\n> > - /* Ask for page aligned allocation. */\n> > - size = (size + pageSize - 1) & ~(pageSize - 1);\n> > -\n> > - struct vc_sm_cma_ioctl_alloc alloc;\n> > - memset(&alloc, 0, sizeof(alloc));\n> > - alloc.size = size;\n> > - alloc.num = 1;\n> > - alloc.cached = cache;\n> > - alloc.handle = 0;\n> > - memcpy(alloc.name, name, 32);\n> > -\n> > - ret = ::ioctl(vcsmHandle_, VC_SM_CMA_IOCTL_MEM_ALLOC, &alloc);\n> > -\n> > - if (ret < 0 || alloc.handle < 0) {\n> > - std::cerr << \"vcsm allocation failure for \"\n> > - << name << std::endl;\n> > - return nullptr;\n> > - }\n> > -\n> > - /* Map the buffer into user space. */\n> > - user_ptr = ::mmap(0, alloc.size, PROT_READ | PROT_WRITE,\n> > - MAP_SHARED, alloc.handle, 0);\n> > -\n> > - if (user_ptr == MAP_FAILED) {\n> > - std::cerr << \"vcsm mmap failure for \" << name << std::endl;\n> > - ::close(alloc.handle);\n> > - return nullptr;\n> > - }\n> > -\n> > - std::lock_guard lock(lock_);\n> > - allocMap_.emplace(user_ptr, AllocInfo(alloc.handle,\n> > - alloc.size, alloc.vc_handle));\n> > -\n> > - return user_ptr;\n> > - }\n> > -\n> > - void free(void *user_ptr)\n> > - {\n> > - std::lock_guard lock(lock_);\n> > - remove(user_ptr);\n> > - }\n> > -\n> > - unsigned int getVCHandle(void *user_ptr)\n> > - {\n> > - std::lock_guard lock(lock_);\n> > - auto it = allocMap_.find(user_ptr);\n> > - if (it != allocMap_.end())\n> > - return it->second.vcHandle;\n> > -\n> > - return 0;\n> > - }\n> > -\n> > -private:\n> > - struct AllocInfo {\n> > - AllocInfo(int handle_, int size_, int vcHandle_)\n> > - : handle(handle_), size(size_), vcHandle(vcHandle_)\n> > - {\n> > - }\n> > -\n> > - int handle;\n> > - int size;\n> > - uint32_t vcHandle;\n> > - };\n> > -\n> > - /* Map of all allocations that have been requested. */\n> > - using AllocMap = std::map;\n> > -\n> > - AllocMap::iterator remove(void *user_ptr)\n> > - {\n> > - auto it = allocMap_.find(user_ptr);\n> > - if (it != allocMap_.end()) {\n> > - int handle = it->second.handle;\n> > - int size = it->second.size;\n> > - ::munmap(user_ptr, size);\n> > - ::close(handle);\n> > - /*\n> > - * Remove the allocation from the map. This returns\n> > - * an iterator to the next element.\n> > - */\n> > - it = allocMap_.erase(it);\n> > - }\n> > -\n> > - /* Returns an iterator to the next element. */\n> > - return it;\n> > - }\n> > -\n> > - AllocMap allocMap_;\n> > - int vcsmHandle_;\n> > - std::mutex lock_;\n> > -};\n> > -\n> > -} /* namespace RPi */\n> > -\n> > -#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 518BFC2E7A\n\tfor ;\n\tThu, 16 Jul 2020 07:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B21361070;\n\tThu, 16 Jul 2020 09:23:55 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8E3660550\n\tfor ;\n\tThu, 16 Jul 2020 09:23:54 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id e4so5998169ljn.4\n\tfor ;\n\tThu, 16 Jul 2020 00:23:54 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IKwFUDgg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=P8TGY6ABzNf7o5526SfqMJROSFbB5qug034Nc/cXDHQ=;\n\tb=IKwFUDgguK0099+MRThq8Al4Mh7gVxDCwNk78tQ3jreNiCs0OYXcSMJ7gxc03KONIS\n\tpLV99l5wecCaXmT9/IMliJVLX2rKl5tE18RF7OG89QIK148N02buDyQGxtJJwnoZXUkr\n\tVYL91M9OkFAKFit2sTtihcXDf8uS5Ex4W/9o8gE7SyPY+VUspFRK3F4w97c0A1hfUMZ6\n\tiIYNJBVTnufMLUKwpCFonuKSLnCtEOTjrUQdVsQh9B9fcth7pwHbaByPRBTDioU9leqe\n\tWq9rfHaaKE1oxhSBeJYgZ609Ag97/uIaMLB9MFgdDrx3fh3zgfUagsugJUues6Tg7ph/\n\tVkEw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=P8TGY6ABzNf7o5526SfqMJROSFbB5qug034Nc/cXDHQ=;\n\tb=fnqG+fbtbCIjDXwPFnYq4bejZNGu2IERFkxE5wIHa7nCeidSIKPZMLSIjaxQxu6Mda\n\txApEmvB1LEZsM70nMW1zXSJaZOl5tHOiCaFP1E26dSFGeWQ/aJvquGeTmIH7yJ548WWn\n\tCHj13OwJWNwRxE5294h9R9xCSKrttPghAbbm05tsekKin5fApfnSEti3Dcn0LO0t1gNe\n\tN1SHK/o5aA5gKnyOKc5iqSb2ldDb2wXh2+RyECQpH8RcSxuhlJxPNU/H+JzS6mgHQtEq\n\tlr0KFneCdBAUHk8BfRxjKf+YOi94Jf5gevV63EEIxnaIBwz1FFb8ot8ZCLoVC9tJtUrF\n\tN8YQ==","X-Gm-Message-State":"AOAM533NtJ8zp5I8C5nWsDfR8VtcZofupVVkRrEnMSAkCEZ5Hx5jc/TG\n\ta8PAMv1OtiyIRDYtHOc9rrCpexvePkH91z6oU7un4KUd","X-Google-Smtp-Source":"ABdhPJySdN5zg1tmeAI/PnUopouSCRXlFnaTkau6pbdmBwVj6/Mg6qZhf1aKFQUUupJnlDnGksJ2fUO4eBgNadbLpnM=","X-Received":"by 2002:a2e:2a84:: with SMTP id q126mr1289592ljq.1.1594884233499;\n\tThu, 16 Jul 2020 00:23:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20200715093911.554684-1-naush@raspberrypi.com>\n\t<20200715093911.554684-2-naush@raspberrypi.com>\n\t<20200715232015.GI6144@pendragon.ideasonboard.com>","In-Reply-To":"<20200715232015.GI6144@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 16 Jul 2020 08:23:36 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]