[libcamera-devel] [PATCH 2/2] libcamera: pipeline: ipa: raspberrypi: Use dma heap allocs for LS tables
Dave Stevenson
dave.stevenson at raspberrypi.com
Wed Jul 15 13:28:03 CEST 2020
Hi Laurent
On Wed, 15 Jul 2020 at 10:05, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Laurent,
>
> Thank you for the review comments.
>
> On Wed, 15 Jul 2020 at 00:06, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Tue, Jul 14, 2020 at 01:08:44PM +0100, Naushir Patuck wrote:
> > > Remove use of vcsm allocations and replace with dma heap allocations.
> > > The pipeline handler now passes the fd of the allocation over to the IPA
> > > instead of the raw pointer.
> >
> > Nice :-)
> >
> > > Also use libcamera::FileDescriptor for fd lifetime management.
> >
> > Should the commit message mention the dependency on
> > https://github.com/raspberrypi/linux/pull/3715 ?
>
> Ack
>
> >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > include/linux/bcm2835-isp.h | 4 +-
> > > include/linux/vc_sm_cma_ioctl.h | 135 ----------------
> > > src/ipa/raspberrypi/raspberrypi.cpp | 27 +++-
> > > .../pipeline/raspberrypi/dma-heaps.h | 102 ++++++++++++
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 44 ++----
> > > src/libcamera/pipeline/raspberrypi/vcsm.h | 149 ------------------
> > > 6 files changed, 138 insertions(+), 323 deletions(-)
> > > delete mode 100644 include/linux/vc_sm_cma_ioctl.h
> > > create mode 100644 src/libcamera/pipeline/raspberrypi/dma-heaps.h
> > > delete mode 100644 src/libcamera/pipeline/raspberrypi/vcsm.h
> > >
> > > diff --git a/include/linux/bcm2835-isp.h b/include/linux/bcm2835-isp.h
> > > index e7afc367..45abb681 100644
> > > --- a/include/linux/bcm2835-isp.h
> > > +++ b/include/linux/bcm2835-isp.h
> > > @@ -108,7 +108,7 @@ enum bcm2835_isp_gain_format {
> > > * @grid_stride: Row to row distance (in grid cells) between grid cells
> > > * in the same horizontal location.
> > > * @grid_height: Height of lens shading tables in grid cells.
> > > - * @mem_handle_table: Memory handle to the tables.
> > > + * @dmabuf: dmabuf file handle containing the table.
> > > * @ref_transform: Reference transform - unsupported, please pass zero.
> > > * @corner_sampled: Whether the gains are sampled at the corner points
> > > * of the grid cells or in the cell centres.
> > > @@ -120,7 +120,7 @@ struct bcm2835_isp_lens_shading {
> > > __u32 grid_width;
> > > __u32 grid_stride;
> > > __u32 grid_height;
> > > - __u32 mem_handle_table;
> > > + __s32 dmabuf;
> > > __u32 ref_transform;
> > > __u32 corner_sampled;
> > > __u32 gain_format;
> > > diff --git a/include/linux/vc_sm_cma_ioctl.h b/include/linux/vc_sm_cma_ioctl.h
> > > deleted file mode 100644
> > > index 21b8758e..00000000
> > > --- a/include/linux/vc_sm_cma_ioctl.h
> > > +++ /dev/null
> > > @@ -1,135 +0,0 @@
> > > -/*
> > > -Copyright (c) 2012, Broadcom Europe Ltd
> > > -All rights reserved.
> > > -
> > > -Redistribution and use in source and binary forms, with or without
> > > -modification, are permitted provided that the following conditions are met:
> > > - * Redistributions of source code must retain the above copyright
> > > - notice, this list of conditions and the following disclaimer.
> > > - * Redistributions in binary form must reproduce the above copyright
> > > - notice, this list of conditions and the following disclaimer in the
> > > - documentation and/or other materials provided with the distribution.
> > > - * Neither the name of the copyright holder nor the
> > > - names of its contributors may be used to endorse or promote products
> > > - derived from this software without specific prior written permission.
> > > -
> > > -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> > > -ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> > > -WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> > > -DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY
> > > -DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> > > -(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> > > -LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> > > -ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > > -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> > > -SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > -*/
> > > -
> > > -/*
> > > - * Copyright 2019 Raspberry Pi (Trading) Ltd. All rights reserved.
> > > - *
> > > - * Based on vmcs_sm_ioctl.h Copyright Broadcom Corporation.
> > > - */
> > > -
> > > -#ifndef __VC_SM_CMA_IOCTL_H
> > > -#define __VC_SM_CMA_IOCTL_H
> > > -
> > > -/* ---- Include Files ---------------------------------------------------- */
> > > -
> > > -#include <linux/types.h> /* Needed for standard types */
> > > -
> > > -#include <linux/ioctl.h>
> > > -
> > > -/* ---- Constants and Types ---------------------------------------------- */
> > > -
> > > -#define VC_SM_CMA_RESOURCE_NAME 32
> > > -#define VC_SM_CMA_RESOURCE_NAME_DEFAULT "sm-host-resource"
> > > -
> > > -/* Type define used to create unique IOCTL number */
> > > -#define VC_SM_CMA_MAGIC_TYPE 'J'
> > > -
> > > -/* IOCTL commands on /dev/vc-sm-cma */
> > > -enum vc_sm_cma_cmd_e {
> > > - VC_SM_CMA_CMD_ALLOC = 0x5A, /* Start at 0x5A arbitrarily */
> > > -
> > > - VC_SM_CMA_CMD_IMPORT_DMABUF,
> > > -
> > > - VC_SM_CMA_CMD_CLEAN_INVALID2,
> > > -
> > > - VC_SM_CMA_CMD_LAST /* Do not delete */
> > > -};
> > > -
> > > -/* Cache type supported, conveniently matches the user space definition in
> > > - * user-vcsm.h.
> > > - */
> > > -enum vc_sm_cma_cache_e {
> > > - VC_SM_CMA_CACHE_NONE,
> > > - VC_SM_CMA_CACHE_HOST,
> > > - VC_SM_CMA_CACHE_VC,
> > > - VC_SM_CMA_CACHE_BOTH,
> > > -};
> > > -
> > > -/* IOCTL Data structures */
> > > -struct vc_sm_cma_ioctl_alloc {
> > > - /* user -> kernel */
> > > - __u32 size;
> > > - __u32 num;
> > > - __u32 cached; /* enum vc_sm_cma_cache_e */
> > > - __u32 pad;
> > > - __u8 name[VC_SM_CMA_RESOURCE_NAME];
> > > -
> > > - /* kernel -> user */
> > > - __s32 handle;
> > > - __u32 vc_handle;
> > > - __u64 dma_addr;
> > > -};
> > > -
> > > -struct vc_sm_cma_ioctl_import_dmabuf {
> > > - /* user -> kernel */
> > > - __s32 dmabuf_fd;
> > > - __u32 cached; /* enum vc_sm_cma_cache_e */
> > > - __u8 name[VC_SM_CMA_RESOURCE_NAME];
> > > -
> > > - /* kernel -> user */
> > > - __s32 handle;
> > > - __u32 vc_handle;
> > > - __u32 size;
> > > - __u32 pad;
> > > - __u64 dma_addr;
> > > -};
> > > -
> > > -/*
> > > - * Cache functions to be set to struct vc_sm_cma_ioctl_clean_invalid2
> > > - * invalidate_mode.
> > > - */
> > > -#define VC_SM_CACHE_OP_NOP 0x00
> > > -#define VC_SM_CACHE_OP_INV 0x01
> > > -#define VC_SM_CACHE_OP_CLEAN 0x02
> > > -#define VC_SM_CACHE_OP_FLUSH 0x03
> > > -
> > > -struct vc_sm_cma_ioctl_clean_invalid2 {
> > > - __u32 op_count;
> > > - __u32 pad;
> > > - struct vc_sm_cma_ioctl_clean_invalid_block {
> > > - __u32 invalidate_mode;
> > > - __u32 block_count;
> > > - void * /*__user */start_address;
> > > - __u32 block_size;
> > > - __u32 inter_block_stride;
> > > - } s[0];
> > > -};
> > > -
> > > -/* IOCTL numbers */
> > > -#define VC_SM_CMA_IOCTL_MEM_ALLOC\
> > > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_ALLOC,\
> > > - struct vc_sm_cma_ioctl_alloc)
> > > -
> > > -#define VC_SM_CMA_IOCTL_MEM_IMPORT_DMABUF\
> > > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_IMPORT_DMABUF,\
> > > - struct vc_sm_cma_ioctl_import_dmabuf)
> > > -
> > > -#define VC_SM_CMA_IOCTL_MEM_CLEAN_INVALID2\
> > > - _IOR(VC_SM_CMA_MAGIC_TYPE, VC_SM_CMA_CMD_CLEAN_INVALID2,\
> > > - struct vc_sm_cma_ioctl_clean_invalid2)
> > > -
> > > -#endif /* __VC_SM_CMA_IOCTL_H */
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index b1f27861..562907e5 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -15,6 +15,7 @@
> > > #include <libcamera/buffer.h>
> > > #include <libcamera/control_ids.h>
> > > #include <libcamera/controls.h>
> > > +#include <libcamera/file_descriptor.h>
> > > #include <libcamera/ipa/ipa_interface.h>
> > > #include <libcamera/ipa/ipa_module_info.h>
> > > #include <libcamera/ipa/raspberrypi.h>
> > > @@ -65,12 +66,14 @@ public:
> > > IPARPi()
> > > : lastMode_({}), controller_(), controllerInit_(false),
> > > frame_count_(0), check_count_(0), hide_count_(0),
> > > - mistrust_count_(0), lsTableHandle_(0), lsTable_(nullptr)
> > > + mistrust_count_(0), lsTableHandle_(FileDescriptor()), lsTable_(nullptr)
> >
> > You can write this
> >
> > mistrust_count_(0), lsTable_(nullptr)
> >
> > lsTableHandle_ is a FileDescriptor instance so it will be constructed
> > using the default FileDescriptor constructor by default.
>
> Ack
>
> >
> > > {
> > > }
> > >
> > > ~IPARPi()
> > > {
> > > + if (lsTable_)
> > > + munmap(lsTable_, MAX_LS_GRID_SIZE);
> > > }
> > >
> > > int init(const IPASettings &settings) override;
> > > @@ -137,7 +140,7 @@ private:
> > > /* How many frames we should avoid running control algos on. */
> > > unsigned int mistrust_count_;
> > > /* LS table allocation passed in from the pipeline handler. */
> > > - uint32_t lsTableHandle_;
> > > + FileDescriptor lsTableHandle_;
> > > void *lsTable_;
> > > };
> > >
> > > @@ -357,8 +360,22 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > }
> > >
> > > case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> > > - lsTable_ = reinterpret_cast<void *>(event.data[0]);
> > > - lsTableHandle_ = event.data[1];
> > > + /* Remove any previous table, if there was one. */
> > > + if (lsTable_)
> > > + munmap(lsTable_, MAX_LS_GRID_SIZE);
> >
> > lsTable_ = nullptr;
> > }
>
> Ack
>
> >
> > otherwise if lsTableHandle_.isValid() fails you'll end up with lsTable_
> > pointing to unmapped memory.
> >
> > > +
> > > + /* Map the LS table buffer into user space. */
> > > + lsTableHandle_ = FileDescriptor(event.data[0]);
> > > + if (lsTableHandle_.isValid()) {
> > > + lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> > > + MAP_SHARED, lsTableHandle_.fd(), 0);
> > > +
> > > + if (lsTable_ == MAP_FAILED) {
> > > + LOG(IPARPI, Error) << "dmaHeap mmap failure for LS table.";
> > > + lsTable_ = nullptr;
> > > + }
> > > + }
> > > +
> > > break;
> > > }
> > >
> > > @@ -1026,7 +1043,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > > .grid_width = w,
> > > .grid_stride = w,
> > > .grid_height = h,
> > > - .mem_handle_table = lsTableHandle_,
> > > + .dmabuf = lsTableHandle_.fd(),
> > > .ref_transform = 0,
> > > .corner_sampled = 1,
> > > .gain_format = GAIN_FORMAT_U4P10
> > > diff --git a/src/libcamera/pipeline/raspberrypi/dma-heaps.h b/src/libcamera/pipeline/raspberrypi/dma-heaps.h
> > > new file mode 100644
> > > index 00000000..9f9137d8
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/raspberrypi/dma-heaps.h
> >
> > Would you mind already splitting this into a .h and a .cpp file ? I
> > foresee more users for dma heaps, so I'll likely move this to the
> > libcamera core at some point.
>
> Will do.
>
> >
> > > @@ -0,0 +1,102 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> > > + *
> > > + * dma-heap.h - Helper class for dma-heap allocations.
> >
> > s/heap/heaps/
>
> Ack.
>
> >
> > > + */
> > > +#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__
> > > +#define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__
> > > +
> > > +#include <mutex>
> > > +
> > > +#include <fcntl.h>
> > > +#include <linux/dma-buf.h>
> > > +#include <linux/dma-heap.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/mman.h>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/file_descriptor.h>
> > > +#include <libcamera/logging.h>
> >
> > logging.h is for the public API that allows modifying the log level.
> > What you need here is libcamera/internal/log.h.
>
> Ack.
>
> >
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(RPI)
> > > +
> > > +namespace RPi {
> > > +
> > > +/*
> > > + * /dev/dma-heap/linux,cma is the dma-heap allocator, which allows dmaheap-cma to
> > > + * only have to worry about importing.
> > > + *
> > > + * Annoyingly, should the cma heap size be specified on the kernel command line
> > > + * instead of DT, the heap gets named "reserved" instead.
> > > + */
> > > +#define DMA_HEAP_CMA_NAME "/dev/dma_heap/linux,cma"
> > > +#define DMA_HEAP_CMA_ALT_NAME "/dev/dma_heap/reserved"
> > > +
> > > +class DmaHeap
> > > +{
> > > +public:
> > > + DmaHeap()
> > > + {
> > > + dmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);
> > > + if (dmaHeapHandle_ == -1) {
> > > + dmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);
> > > + if (dmaHeapHandle_ == -1) {
> > > + LOG(RPI, Error) << "Could not open dmaHeap device";
> > > + }
> > > + }
> > > + }
> > > +
> > > + ~DmaHeap()
> > > + {
> > > + if (dmaHeapHandle_)
> > > + ::close(dmaHeapHandle_);
> > > + }
> > > +
> > > + FileDescriptor alloc(const char *name, unsigned int size)
> >
> > Maybe std::size_t instead of unsigned int ?
>
> Ack.
>
> >
> > > + {
> > > + unsigned int pageSize = getpagesize();
> > > + int ret;
> > > +
> > > + if (!name)
> > > + return FileDescriptor();
> >
> > You can also write this
> >
> > return {};
> >
> > (same below). Up to you.
>
> If it's ok, I would prefer FileDescriptor() - feels more descriptive to me.
>
> >
> > > +
> > > + /* Ask for page aligned allocation. */
> > > + size = (size + pageSize - 1) & ~(pageSize - 1);
> > > +
> > > + struct dma_heap_allocation_data alloc;
> > > +
> > > + memset(&alloc, 0, sizeof(alloc));
> >
> > These two lines can be replaced with
> >
> > struct dma_heap_allocation_data alloc = {};
>
> Ack
>
> >
> > > + alloc.len = size;
> > > + alloc.fd_flags = O_CLOEXEC | O_RDWR;
> > > +
> > > + ret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);
> > > +
> > > + if (ret < 0) {
> > > + LOG(RPI, Error) << "dmaHeap allocation failure for "
> > > + << name;
> > > + return FileDescriptor();
> > > + }
> > > +
> > > + ret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);
> >
> > What is the name used for, is it only for debugging purpose inside the
> > kernel, or does it have more uses ? Does the name have to be unique ?
>
> It is only for debug purposes. I don't think there is anything
> enforcing them to be unique.
Yes, it's only debug. It should show up if you do cat
/sys/kernel/debug/dma_buf/bufinfo, otherwise you get a long list of
anonymous allocations and have to try to guess their usage based on
size and exporter.
No need for it to be unique.
Added in 5.3 - https://github.com/torvalds/linux/commit/bb2bb903042517b8fb17b2bc21e00512f2dcac01,
although there are a couple of later fixes.
> > > + if (ret < 0) {
> > > + LOG(RPI, Error) << "dmaHeap naming failure for "
> > > + << name;
> > > + ::close(alloc.fd);
> > > + return FileDescriptor();
> > > + }
> > > +
> > > + return FileDescriptor(std::move(alloc.fd));
> > > + }
> > > +
> > > +private:
> > > + int dmaHeapHandle_;
> > > +};
> > > +
> > > +} /* namespace RPi */
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__ */
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 7450062f..4486e04e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -13,6 +13,7 @@
> > >
> > > #include <libcamera/camera.h>
> > > #include <libcamera/control_ids.h>
> > > +#include <libcamera/file_descriptor.h>
> > > #include <libcamera/formats.h>
> > > #include <libcamera/ipa/raspberrypi.h>
> > > #include <libcamera/logging.h>
> > > @@ -30,8 +31,8 @@
> > > #include "libcamera/internal/v4l2_controls.h"
> > > #include "libcamera/internal/v4l2_videodevice.h"
> > >
> > > +#include "dma-heaps.h"
> > > #include "staggered_ctrl.h"
> > > -#include "vcsm.h"
> > >
> > > namespace libcamera {
> > >
> > > @@ -285,23 +286,13 @@ class RPiCameraData : public CameraData
> > > {
> > > public:
> > > RPiCameraData(PipelineHandler *pipe)
> > > - : CameraData(pipe), sensor_(nullptr), lsTable_(nullptr),
> > > + : CameraData(pipe), sensor_(nullptr), lsTable_(FileDescriptor()),
> >
> > Same as above, just drop lsTable_.
> >
> > > state_(State::Stopped), dropFrame_(false), ispOutputCount_(0)
> > > {
> > > }
> > >
> > > ~RPiCameraData()
> > > {
> > > - /*
> > > - * Free the LS table if we have allocated one. Another
> > > - * allocation will occur in applyLS() with the appropriate
> > > - * size.
> > > - */
> > > - if (lsTable_) {
> > > - vcsm_.free(lsTable_);
> > > - lsTable_ = nullptr;
> > > - }
> > > -
> > > /* Stop the IPA proxy thread. */
> > > if (ipa_)
> > > ipa_->stop();
> > > @@ -330,9 +321,9 @@ public:
> > > /* Buffers passed to the IPA. */
> > > std::vector<IPABuffer> ipaBuffers_;
> > >
> > > - /* VCSM allocation helper. */
> > > - ::RPi::Vcsm vcsm_;
> > > - void *lsTable_;
> > > + /* DMAHEAP allocation helper. */
> > > + RPi::DmaHeap dmaHeap_;
> > > + FileDescriptor lsTable_;
> > >
> > > RPi::StaggeredCtrl staggeredCtrl_;
> > > uint32_t expectedSequence_;
> > > @@ -1006,27 +997,16 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> > > entityControls.emplace(0, data->unicam_[Unicam::Image].dev()->controls());
> > > entityControls.emplace(1, data->isp_[Isp::Input].dev()->controls());
> > >
> > > - /* Allocate the lens shading table via vcsm and pass to the IPA. */
> > > - if (!data->lsTable_) {
> > > - data->lsTable_ = data->vcsm_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > > - uintptr_t ptr = reinterpret_cast<uintptr_t>(data->lsTable_);
> > > -
> > > - if (!data->lsTable_)
> > > + /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > > + if (!data->lsTable_.isValid()) {
> > > + data->lsTable_ = data->dmaHeap_.alloc("ls_grid", MAX_LS_GRID_SIZE);
> > > + if (!data->lsTable_.isValid())
> > > return -ENOMEM;
> > >
> > > - /*
> > > - * The vcsm allocation will always be in the memory region
> > > - * < 32-bits to allow Videocore to access the memory.
> > > - *
> > > - * \todo Sending a pointer to the IPA is a workaround for
> > > - * vc_sm_cma not yet supporting dmabuf. This will not work with
> > > - * IPA module isolation and should be reworked when vc_sma_cma
> > > - * will permit.
> > > - */
> > > + /* Allow the IPA to mmap the LS table via the file descriptor. */
> > > IPAOperationData op;
> > > op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> > > - op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> > > - data->vcsm_.getVCHandle(data->lsTable_) };
> > > + op.data = { static_cast<unsigned int>(data->lsTable_.fd()) };
> >
> > Very nice ! As the Raspberry Pi IPA runs in the same process without IPC
> > this change will not have an immediately visible effect, but it moves
> > towards more standard APIs and will allow us to test IPA isolation on
> > Raspberry Pi too.
> >
> > On a side note, sending a file descriptor over IPC in a IPAOperationData
> > is not something we support currently, but that's a shortcoming of the
> > current implementation, not something you need to be concerned about.
> > We'll fix it as part of the ongoing work on IPC.
>
> Yes, I was hoping you would take care of that :-)
>
> >
> > > data->ipa_->processEvent(op);
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/vcsm.h b/src/libcamera/pipeline/raspberrypi/vcsm.h
> > > deleted file mode 100644
> > > index bebe86a7..00000000
> > > --- a/src/libcamera/pipeline/raspberrypi/vcsm.h
> > > +++ /dev/null
> > > @@ -1,149 +0,0 @@
> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > -/*
> > > - * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> > > - *
> > > - * vcsm.h - Helper class for vcsm allocations.
> > > - */
> > > -#ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__
> > > -#define __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__
> > > -
> > > -#include <iostream>
> > > -#include <mutex>
> > > -
> > > -#include <fcntl.h>
> > > -#include <linux/vc_sm_cma_ioctl.h>
> > > -#include <sys/ioctl.h>
> > > -#include <sys/mman.h>
> > > -#include <unistd.h>
> > > -
> > > -namespace RPi {
> > > -
> > > -#define VCSM_CMA_DEVICE_NAME "/dev/vcsm-cma"
> > > -
> > > -class Vcsm
> > > -{
> > > -public:
> > > - Vcsm()
> > > - {
> > > - vcsmHandle_ = ::open(VCSM_CMA_DEVICE_NAME, O_RDWR, 0);
> > > - if (vcsmHandle_ == -1) {
> > > - std::cerr << "Could not open vcsm device: "
> > > - << VCSM_CMA_DEVICE_NAME;
> > > - }
> > > - }
> > > -
> > > - ~Vcsm()
> > > - {
> > > - /* Free all existing allocations. */
> > > - auto it = allocMap_.begin();
> > > - while (it != allocMap_.end())
> > > - it = remove(it->first);
> > > -
> > > - if (vcsmHandle_)
> > > - ::close(vcsmHandle_);
> > > - }
> > > -
> > > - void *alloc(const char *name, unsigned int size,
> > > - vc_sm_cma_cache_e cache = VC_SM_CMA_CACHE_NONE)
> > > - {
> > > - unsigned int pageSize = getpagesize();
> > > - void *user_ptr;
> > > - int ret;
> > > -
> > > - if (!name)
> > > - return nullptr;
> > > -
> > > - /* Ask for page aligned allocation. */
> > > - size = (size + pageSize - 1) & ~(pageSize - 1);
> > > -
> > > - struct vc_sm_cma_ioctl_alloc alloc;
> > > - memset(&alloc, 0, sizeof(alloc));
> > > - alloc.size = size;
> > > - alloc.num = 1;
> > > - alloc.cached = cache;
> > > - alloc.handle = 0;
> > > - memcpy(alloc.name, name, 32);
> > > -
> > > - ret = ::ioctl(vcsmHandle_, VC_SM_CMA_IOCTL_MEM_ALLOC, &alloc);
> > > -
> > > - if (ret < 0 || alloc.handle < 0) {
> > > - std::cerr << "vcsm allocation failure for "
> > > - << name << std::endl;
> > > - return nullptr;
> > > - }
> > > -
> > > - /* Map the buffer into user space. */
> > > - user_ptr = ::mmap(0, alloc.size, PROT_READ | PROT_WRITE,
> > > - MAP_SHARED, alloc.handle, 0);
> > > -
> > > - if (user_ptr == MAP_FAILED) {
> > > - std::cerr << "vcsm mmap failure for " << name << std::endl;
> > > - ::close(alloc.handle);
> > > - return nullptr;
> > > - }
> > > -
> > > - std::lock_guard<std::mutex> lock(lock_);
> > > - allocMap_.emplace(user_ptr, AllocInfo(alloc.handle,
> > > - alloc.size, alloc.vc_handle));
> > > -
> > > - return user_ptr;
> > > - }
> > > -
> > > - void free(void *user_ptr)
> > > - {
> > > - std::lock_guard<std::mutex> lock(lock_);
> > > - remove(user_ptr);
> > > - }
> > > -
> > > - unsigned int getVCHandle(void *user_ptr)
> > > - {
> > > - std::lock_guard<std::mutex> lock(lock_);
> > > - auto it = allocMap_.find(user_ptr);
> > > - if (it != allocMap_.end())
> > > - return it->second.vcHandle;
> > > -
> > > - return 0;
> > > - }
> > > -
> > > -private:
> > > - struct AllocInfo {
> > > - AllocInfo(int handle_, int size_, int vcHandle_)
> > > - : handle(handle_), size(size_), vcHandle(vcHandle_)
> > > - {
> > > - }
> > > -
> > > - int handle;
> > > - int size;
> > > - uint32_t vcHandle;
> > > - };
> > > -
> > > - /* Map of all allocations that have been requested. */
> > > - using AllocMap = std::map<void *, AllocInfo>;
> > > -
> > > - AllocMap::iterator remove(void *user_ptr)
> > > - {
> > > - auto it = allocMap_.find(user_ptr);
> > > - if (it != allocMap_.end()) {
> > > - int handle = it->second.handle;
> > > - int size = it->second.size;
> > > - ::munmap(user_ptr, size);
> > > - ::close(handle);
> > > - /*
> > > - * Remove the allocation from the map. This returns
> > > - * an iterator to the next element.
> > > - */
> > > - it = allocMap_.erase(it);
> > > - }
> > > -
> > > - /* Returns an iterator to the next element. */
> > > - return it;
> > > - }
> > > -
> > > - AllocMap allocMap_;
> > > - int vcsmHandle_;
> > > - std::mutex lock_;
> > > -};
> > > -
> > > -} /* namespace RPi */
> > > -
> > > -#endif /* __LIBCAMERA_PIPELINE_RASPBERRYPI_VCSM_H__ */
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> Will post and update with the changes shortly.
>
> Regards,
> Naush
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list