[{"id":11395,"web_url":"https://patchwork.libcamera.org/comment/11395/","msgid":"<20200714230614.GK5854@pendragon.ideasonboard.com>","date":"2020-07-14T23:06:14","subject":"Re: [libcamera-devel] [PATCH 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 Tue, Jul 14, 2020 at 01:08:44PM +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\nNice :-)\n\n> Also use libcamera::FileDescriptor for fd lifetime management.\n\nShould the commit message mention the dependency on\nhttps://github.com/raspberrypi/linux/pull/3715 ?\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 | 27 +++-\n> .../pipeline/raspberrypi/dma-heaps.h | 102 ++++++++++++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++----\n> src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> 6 files changed, 138 insertions(+), 323 deletions(-)\n> delete mode 100644 include/linux/vc_sm_cma_ioctl.h\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..562907e5 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), lsTableHandle_(FileDescriptor()), lsTable_(nullptr)\n\nYou can write this\n\n\t\t mistrust_count_(0), lsTable_(nullptr)\n\nlsTableHandle_ is a FileDescriptor instance so it will be constructed\nusing the default FileDescriptor constructor by default.\n\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,22 @@ 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\t\tlsTable_ = nullptr;\n\t\t}\n\notherwise if lsTableHandle_.isValid() fails you'll end up with lsTable_\npointing to unmapped memory.\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 +1043,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.h b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n> new file mode 100644\n> index 00000000..9f9137d8\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n\nWould you mind already splitting this into a .h and a .cpp file ? I\nforesee more users for dma heaps, so I'll likely move this to the\nlibcamera core at some point.\n\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * dma-heap.h - Helper class for dma-heap allocations.\n\ns/heap/heaps/\n\n> + */\n> +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nlogging.h is for the public API that allows modifying the log level.\nWhat you need here is libcamera/internal/log.h.\n\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> +class DmaHeap\n> +{\n> +public:\n> +\tDmaHeap()\n> +\t{\n> +\t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n> +\t\tif (dmaHeapHandle_ == -1) {\n> +\t\t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n> +\t\t\tif (dmaHeapHandle_ == -1) {\n> +\t\t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t~DmaHeap()\n> +\t{\n> +\t\tif (dmaHeapHandle_)\n> +\t\t\t::close(dmaHeapHandle_);\n> +\t}\n> +\n> +\tFileDescriptor alloc(const char *name, unsigned int size)\n\nMaybe std::size_t instead of unsigned int ?\n\n> +\t{\n> +\t\tunsigned int pageSize = getpagesize();\n> +\t\tint ret;\n> +\n> +\t\tif (!name)\n> +\t\t\treturn FileDescriptor();\n\nYou can also write this\n\n\t\t\treturn {};\n\n(same below). Up to you.\n\n> +\n> +\t\t/* Ask for page aligned allocation. */\n> +\t\tsize = (size + pageSize - 1) & ~(pageSize - 1);\n> +\n> +\t\tstruct dma_heap_allocation_data alloc;\n> +\n> +\t\tmemset(&alloc, 0, sizeof(alloc));\n\nThese two lines can be replaced with\n\n\t\tstruct dma_heap_allocation_data alloc = {};\n\n> +\t\talloc.len = size;\n> +\t\talloc.fd_flags = O_CLOEXEC | O_RDWR;\n> +\n> +\t\tret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);\n> +\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(RPI, Error) << \"dmaHeap allocation failure for \"\n> +\t\t\t\t\t<< name;\n> +\t\t\treturn FileDescriptor();\n> +\t\t}\n> +\n> +\t\tret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);\n\nWhat is the name used for, is it only for debugging purpose inside the\nkernel, or does it have more uses ? Does the name have to be unique ?\n\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(RPI, Error) << \"dmaHeap naming failure for \"\n> +\t\t\t\t\t<< name;\n> +\t\t\t::close(alloc.fd);\n> +\t\t\treturn FileDescriptor();\n> +\t\t}\n> +\n> +\t\treturn FileDescriptor(std::move(alloc.fd));\n> +\t}\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/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7450062f..4486e04e 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: CameraData(pipe), sensor_(nullptr), lsTable_(FileDescriptor()),\n\nSame as above, just drop lsTable_.\n\n> \t\t state_(State::Stopped), 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\nVery nice ! As the Raspberry Pi IPA runs in the same process without IPC\nthis change will not have an immediately visible effect, but it moves\ntowards more standard APIs and will allow us to test IPA isolation on\nRaspberry Pi too.\n\nOn a side note, sending a file descriptor over IPC in a IPAOperationData\nis not something we support currently, but that's a shortcoming of the\ncurrent implementation, not something you need to be concerned about.\nWe'll fix it as part of the ongoing work on IPC.\n\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 5A6A1BD792\n\tfor ;\n\tTue, 14 Jul 2020 23:06:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9CC26091A;\n\tWed, 15 Jul 2020 01:06:22 +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 9F7AD60745\n\tfor ;\n\tWed, 15 Jul 2020 01:06:21 +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 2534671D;\n\tWed, 15 Jul 2020 01:06:21 +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=\"k1XgXsfi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594767981;\n\tbh=tqvif1N7RYTfwrg9v1zMi+3eZdAeN482XTBE6mVb3IY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k1XgXsfiBlTMyw5G2NJ9QHpvNGVe9ZLs5rByp17KrmAiUJrIDj/Tz235czQ9rP0xV\n\tlvd5Z1AqAx8acQwHwYgiW7RiYf69CWoZkrvFZB0tor3h0g+U8P17xMAZ3dLDvkBjjG\n\tSafVpQ3ECV7599/xBVhewrjG4oB1feS60RORifLo=","Date":"Wed, 15 Jul 2020 02:06:14 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200714230614.GK5854@pendragon.ideasonboard.com>","References":"<20200714120844.520732-1-naush@raspberrypi.com>\n\t<20200714120844.520732-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200714120844.520732-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 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":11404,"web_url":"https://patchwork.libcamera.org/comment/11404/","msgid":"","date":"2020-07-15T09:05:20","subject":"Re: [libcamera-devel] [PATCH 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\nThank you for the review comments.\n\nOn Wed, 15 Jul 2020 at 00:06, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 14, 2020 at 01:08:44PM +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> Nice :-)\n>\n> > Also use libcamera::FileDescriptor for fd lifetime management.\n>\n> Should the commit message mention the dependency on\n> https://github.com/raspberrypi/linux/pull/3715 ?\n\nAck\n\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 | 27 +++-\n> > .../pipeline/raspberrypi/dma-heaps.h | 102 ++++++++++++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++----\n> > src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> > 6 files changed, 138 insertions(+), 323 deletions(-)\n> > delete mode 100644 include/linux/vc_sm_cma_ioctl.h\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..562907e5 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), lsTableHandle_(FileDescriptor()), lsTable_(nullptr)\n>\n> You can write this\n>\n> mistrust_count_(0), lsTable_(nullptr)\n>\n> lsTableHandle_ is a FileDescriptor instance so it will be constructed\n> using the default FileDescriptor constructor by default.\n\nAck\n\n>\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,22 @@ 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\nAck\n\n>\n> otherwise if lsTableHandle_.isValid() fails you'll end up with lsTable_\n> pointing to unmapped memory.\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 +1043,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.h b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n> > new file mode 100644\n> > index 00000000..9f9137d8\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n>\n> Would you mind already splitting this into a .h and a .cpp file ? I\n> foresee more users for dma heaps, so I'll likely move this to the\n> libcamera core at some point.\n\nWill do.\n\n>\n> > @@ -0,0 +1,102 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * dma-heap.h - Helper class for dma-heap allocations.\n>\n> s/heap/heaps/\n\nAck.\n\n>\n> > + */\n> > +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n>\n> logging.h is for the public API that allows modifying the log level.\n> What you need here is libcamera/internal/log.h.\n\nAck.\n\n>\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> > +class DmaHeap\n> > +{\n> > +public:\n> > + 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()\n> > + {\n> > + if (dmaHeapHandle_)\n> > + ::close(dmaHeapHandle_);\n> > + }\n> > +\n> > + FileDescriptor alloc(const char *name, unsigned int size)\n>\n> Maybe std::size_t instead of unsigned int ?\n\nAck.\n\n>\n> > + {\n> > + unsigned int pageSize = getpagesize();\n> > + int ret;\n> > +\n> > + if (!name)\n> > + return FileDescriptor();\n>\n> You can also write this\n>\n> return {};\n>\n> (same below). Up to you.\n\nIf it's ok, I would prefer FileDescriptor() - feels more descriptive to me.\n\n>\n> > +\n> > + /* Ask for page aligned allocation. */\n> > + size = (size + pageSize - 1) & ~(pageSize - 1);\n> > +\n> > + struct dma_heap_allocation_data alloc;\n> > +\n> > + memset(&alloc, 0, sizeof(alloc));\n>\n> These two lines can be replaced with\n>\n> struct dma_heap_allocation_data alloc = {};\n\nAck\n\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>\n> What is the name used for, is it only for debugging purpose inside the\n> kernel, or does it have more uses ? Does the name have to be unique ?\n\nIt is only for debug purposes. I don't think there is anything\nenforcing them to be unique.\n\n>\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> > +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/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7450062f..4486e04e 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> > + : CameraData(pipe), sensor_(nullptr), lsTable_(FileDescriptor()),\n>\n> Same as above, just drop lsTable_.\n>\n> > state_(State::Stopped), 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>\n> Very nice ! As the Raspberry Pi IPA runs in the same process without IPC\n> this change will not have an immediately visible effect, but it moves\n> towards more standard APIs and will allow us to test IPA isolation on\n> Raspberry Pi too.\n>\n> On a side note, sending a file descriptor over IPC in a IPAOperationData\n> is not something we support currently, but that's a shortcoming of the\n> current implementation, not something you need to be concerned about.\n> We'll fix it as part of the ongoing work on IPC.\n\nYes, I was hoping you would take care of that :-)\n\n>\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\nWill post and update with the changes shortly.\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 30E6BBD792\n\tfor ;\n\tWed, 15 Jul 2020 09:05:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B24860962;\n\tWed, 15 Jul 2020 11:05:39 +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 DA2866048C\n\tfor ;\n\tWed, 15 Jul 2020 11:05:37 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id q7so1777950ljm.1\n\tfor ;\n\tWed, 15 Jul 2020 02:05:37 -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=\"rbZXsNX4\"; 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=ZhycdysrkKUnT/ClkPtWaluTCmJf4D8x7MLK3vRwMhM=;\n\tb=rbZXsNX4U0zjBG5ZoKsxCUz8JIifEiBk+FxY5MotaZX7fVoMbGAOkQjQPdFZoLjg2n\n\t7V9ENkFTWHHZ6wp5AzSkz9g62vmTbMN9pBjLnyZekpyrnJ7INx1p+oHTf6t11Mb1cWlQ\n\tnPa3TdECYu4Lyo6JQIOVI13NRg4b6WTJw+RKTQjWvbSj7O4BTbMoJNdTTYxsEa/Lkp7M\n\tOMi28olRXnSaYvRz+laF5GBEWgRxBvlrYzi1lDc8RKbaOJt2wPclRyajl1HoJXCN+Flv\n\tmxypJFjIC5W1cXnlGCN6a77s7t/JUNUD+yA9gtUXbhTG8Y9AX3RqPoMLl0+W+K1I7KPV\n\tYFMA==","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=ZhycdysrkKUnT/ClkPtWaluTCmJf4D8x7MLK3vRwMhM=;\n\tb=qk8n/LIzzOFViEitA8kd/pSSh5fWh9NhZvDqrhFI+DXQGOw1SRSHAbzhenknxasgPW\n\twXmDvR1C3kHd+ayyGR6i4tr7QLmbZL5oifTLco7R2sutKkfU/pOzhT6yUUOYz4PCepG2\n\tg34+QWGFSWMWIOF3FeTi2VAGO5bgBl7jbLmffSazgjBW1SgRPV68W7GypvQTKrBN2K3n\n\tM3ZQJne/XcLw2jqqb1AD/1brMLIVPMX8usbanAObs6TS5nsssy4SrRqgSKLo3Gv9ay0F\n\t+rqU4cjx/flIpMEoNYEl3qrdl3G4VR8yQPuF8BbGhLyo4PKS2gkTVLK17vfhtbtsn6Ec\n\ts+eQ==","X-Gm-Message-State":"AOAM5333z6x/1iNKf+s72kEU4AkCkpXAEFUmFAfHN4mx+BzpYO36paTD\n\tK/O2mVRcyG7H/UWLlrzBVbwE68KjgPSgnhpIAsTyTLOJLZw=","X-Google-Smtp-Source":"ABdhPJywa1TAYE8/+n2tfwP0mNJ1GyyesXaMB3Oc53as/oWCEB1t4E3HVmmPgBGv1s/IXrPHoPbTqk/IUw85I/BBQvs=","X-Received":"by 2002:a2e:9dcc:: with SMTP id\n\tx12mr4243957ljj.415.1594803936877; \n\tWed, 15 Jul 2020 02:05:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20200714120844.520732-1-naush@raspberrypi.com>\n\t<20200714120844.520732-3-naush@raspberrypi.com>\n\t<20200714230614.GK5854@pendragon.ideasonboard.com>","In-Reply-To":"<20200714230614.GK5854@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 15 Jul 2020 10:05:20 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 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":11407,"web_url":"https://patchwork.libcamera.org/comment/11407/","msgid":"","date":"2020-07-15T11:28:03","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipa:\n\traspberrypi: Use dma heap allocs for LS tables","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Laurent\n\nOn Wed, 15 Jul 2020 at 10:05, Naushir Patuck wrote:\n>\n> Hi Laurent,\n>\n> Thank you for the review comments.\n>\n> On Wed, 15 Jul 2020 at 00:06, Laurent Pinchart\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Jul 14, 2020 at 01:08:44PM +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> > Nice :-)\n> >\n> > > Also use libcamera::FileDescriptor for fd lifetime management.\n> >\n> > Should the commit message mention the dependency on\n> > https://github.com/raspberrypi/linux/pull/3715 ?\n>\n> Ack\n>\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 | 27 +++-\n> > > .../pipeline/raspberrypi/dma-heaps.h | 102 ++++++++++++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++----\n> > > src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------\n> > > 6 files changed, 138 insertions(+), 323 deletions(-)\n> > > delete mode 100644 include/linux/vc_sm_cma_ioctl.h\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..562907e5 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), lsTableHandle_(FileDescriptor()), lsTable_(nullptr)\n> >\n> > You can write this\n> >\n> > mistrust_count_(0), lsTable_(nullptr)\n> >\n> > lsTableHandle_ is a FileDescriptor instance so it will be constructed\n> > using the default FileDescriptor constructor by default.\n>\n> Ack\n>\n> >\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,22 @@ 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>\n> Ack\n>\n> >\n> > otherwise if lsTableHandle_.isValid() fails you'll end up with lsTable_\n> > pointing to unmapped memory.\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 +1043,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.h b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n> > > new file mode 100644\n> > > index 00000000..9f9137d8\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/raspberrypi/dma-heaps.h\n> >\n> > Would you mind already splitting this into a .h and a .cpp file ? I\n> > foresee more users for dma heaps, so I'll likely move this to the\n> > libcamera core at some point.\n>\n> Will do.\n>\n> >\n> > > @@ -0,0 +1,102 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > > + *\n> > > + * dma-heap.h - Helper class for dma-heap allocations.\n> >\n> > s/heap/heaps/\n>\n> Ack.\n>\n> >\n> > > + */\n> > > +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > > +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> >\n> > logging.h is for the public API that allows modifying the log level.\n> > What you need here is libcamera/internal/log.h.\n>\n> Ack.\n>\n> >\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> > > +class DmaHeap\n> > > +{\n> > > +public:\n> > > + 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()\n> > > + {\n> > > + if (dmaHeapHandle_)\n> > > + ::close(dmaHeapHandle_);\n> > > + }\n> > > +\n> > > + FileDescriptor alloc(const char *name, unsigned int size)\n> >\n> > Maybe std::size_t instead of unsigned int ?\n>\n> Ack.\n>\n> >\n> > > + {\n> > > + unsigned int pageSize = getpagesize();\n> > > + int ret;\n> > > +\n> > > + if (!name)\n> > > + return FileDescriptor();\n> >\n> > You can also write this\n> >\n> > return {};\n> >\n> > (same below). Up to you.\n>\n> If it's ok, I would prefer FileDescriptor() - feels more descriptive to me.\n>\n> >\n> > > +\n> > > + /* Ask for page aligned allocation. */\n> > > + size = (size + pageSize - 1) & ~(pageSize - 1);\n> > > +\n> > > + struct dma_heap_allocation_data alloc;\n> > > +\n> > > + memset(&alloc, 0, sizeof(alloc));\n> >\n> > These two lines can be replaced with\n> >\n> > struct dma_heap_allocation_data alloc = {};\n>\n> Ack\n>\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> >\n> > What is the name used for, is it only for debugging purpose inside the\n> > kernel, or does it have more uses ? Does the name have to be unique ?\n>\n> It is only for debug purposes. I don't think there is anything\n> enforcing them to be unique.\n\nYes, it's only debug. It should show up if you do cat\n/sys/kernel/debug/dma_buf/bufinfo, otherwise you get a long list of\nanonymous allocations and have to try to guess their usage based on\nsize and exporter.\nNo need for it to be unique.\n\nAdded in 5.3 - https://github.com/torvalds/linux/commit/bb2bb903042517b8fb17b2bc21e00512f2dcac01,\nalthough there are a couple of later fixes.\n\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> > > +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/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 7450062f..4486e04e 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> > > + : CameraData(pipe), sensor_(nullptr), lsTable_(FileDescriptor()),\n> >\n> > Same as above, just drop lsTable_.\n> >\n> > > state_(State::Stopped), 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> >\n> > Very nice ! As the Raspberry Pi IPA runs in the same process without IPC\n> > this change will not have an immediately visible effect, but it moves\n> > towards more standard APIs and will allow us to test IPA isolation on\n> > Raspberry Pi too.\n> >\n> > On a side note, sending a file descriptor over IPC in a IPAOperationData\n> > is not something we support currently, but that's a shortcoming of the\n> > current implementation, not something you need to be concerned about.\n> > We'll fix it as part of the ongoing work on IPC.\n>\n> Yes, I was hoping you would take care of that :-)\n>\n> >\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>\n> Will post and update with the changes shortly.\n>\n> Regards,\n> Naush\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 0098BBD790\n\tfor ;\n\tWed, 15 Jul 2020 11:28:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C0206099A;\n\tWed, 15 Jul 2020 13:28:21 +0200 (CEST)","from mail-wr1-x442.google.com (mail-wr1-x442.google.com\n\t[IPv6:2a00:1450:4864:20::442])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 634226048B\n\tfor ;\n\tWed, 15 Jul 2020 13:28:19 +0200 (CEST)","by mail-wr1-x442.google.com with SMTP id f7so2320560wrw.1\n\tfor ;\n\tWed, 15 Jul 2020 04:28:19 -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=\"keK9+PtA\"; 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=VqeezwB0/TNf1bTCTHmsZSANb6RqPrxknZKC8MU/ivo=;\n\tb=keK9+PtA0MmCJHf4KvGnR/dguWbT3oPVKlYfzx9RCqygK3Pgu+gXrd043HvQTT5CJ5\n\tJ5N+UiqkzoF5iG32CLibQ4UEK5iG3PlBBd2IaGjv3/W/G7ckbhg99cFE6JHKiC3lREa0\n\t57Ksvm0CjSnrzBj93jlBBseGK0ZlljhZC3fae7mz1Midv/BF3w3QTEJLXxARTgA+h27s\n\tqqlZzk45fcgdyzyeKGh1CyUgQqwjES086x8slEd+bk+0lnDMM0j+OtsVzOFSgOIhFUbS\n\tC79Z7xYcsR0vIrczQGc7xqhMnq18aZWGjXpFfeYc6EooDhIN3HyG/3U19Vm9FyNRGB9W\n\t6k/g==","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=VqeezwB0/TNf1bTCTHmsZSANb6RqPrxknZKC8MU/ivo=;\n\tb=s0EJEn6mMmcOU0g3soWJBXez8r3a2Q66Ji7i+V0+5ouDKbqoc5nL/Kp1WK2KW7WJt7\n\tZF+cLOKAiJV2Gc49UThpYJdqyy/hiF8T9/W3KvCt1GUb6lquyC/Xjf2j2pcYPVpchAGh\n\t4QLb08YxQC/8VXqArWJbJyKm8RfnXnN2DBc0NtzCIAr40OmOIVczyR39UuY/TGiim03S\n\tU78LZCVqGK3xQlldWndcmOSqaCez+gcBB70dTo+vEncMvfwEBi2rU8zRTxowqJb6WW2l\n\tdMmaveaxdkADS6O8sfHsYA3Oh/rGEC3JuzYtRBIIOsmcvAJir82TMDkBxKqeIa4o1Jf7\n\tE2Kg==","X-Gm-Message-State":"AOAM530nxD6sbKlTMTukuN3wkHWQAqZNp0E2lLkbFamzuoA4lMvCafSo\n\tlBR+Y7/8DJcq3958pZfBxc3Y9qoHw3nqgxkZ5DtuIg==","X-Google-Smtp-Source":"ABdhPJzHRE7SesZSzoxs3Qk4CZquDFwCF61hS2ex0XiBOc1jXEMDjcReNXgrcAd75ojQKk/CTkh5G4WQtaFPPxyEZ9k=","X-Received":"by 2002:adf:db86:: with SMTP id u6mr11201411wri.27.1594812498657;\n\tWed, 15 Jul 2020 04:28:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20200714120844.520732-1-naush@raspberrypi.com>\n\t<20200714120844.520732-3-naush@raspberrypi.com>\n\t<20200714230614.GK5854@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Wed, 15 Jul 2020 12:28:03 +0100","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]