Hi Jacopo On Mon, 24 Aug 2020 at 17:36, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote: > > Hi Dave, Nicolas, Laurent, > > On Wed, May 06, 2020 at 08:24:38PM +0100, Dave Stevenson wrote: > > Hi Nicolas > > > > On Wed, 6 May 2020 at 19:04, Nicolas Saenz Julienne > > <nsaenzjulienne@xxxxxxx> wrote: > > > > > > Hi Laurent, Dave, > > > > > > On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote: > > > > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > > > > > > Add Broadcom VideoCore Shared Memory support. > > > > > > > > This new driver allows contiguous memory blocks to be imported > > > > into the VideoCore VPU memory map, and manages the lifetime of > > > > those objects, only releasing the source dmabuf once the VPU has > > > > confirmed it has finished with it. > > > > > > > > > > I'm still digesting all this, but a question came up, who is using the > > > ioctls? > > > > We have a userspace library that uses it [1]. > > It is used by things like MMAL to share buffers between the VPU and > > ARM, rather than having to get VCHI to copy all the data between > > mirrored buffers. > > > > I think what has happened here is that Laurent has picked up the > > version of the driver from the top of our downstream kernel tree. > > For libcamera and the ISP driver, we need a significantly smaller > > feature set, basically import of dmabufs only, no allocations or cache > > management. For the ISP driver it's mainly dmabuf import from > > videobuf2 for the image buffers, but there's also a need to pass in > > lens shading tables which are relatively large. With a small amount of > > rework in libcamera, we can make it so that we use dma-buf heaps to do > > the allocation, and pass in a dmabuf fd to the ISP driver to then map > > onto the VPU. That removes all the ioctls handling from this driver. > > > > Downstream we do have other use cases that want to be able to do other > > functions on shared memory, but that too should be reworkable into > > using dma-buf heaps for allocations, and vcsm only handles importing > > dmabufs via an ioctl. All that can be hidden away in the vcsm library, > > so applications don't care. > > We've also got some legacy code kicking around, as there was > > originally a version of the driver that mapped the VPU's memory blocks > > to the ARM. That's why the vcsm library has two code paths through > > almost every function - one for each driver. > > > > Laurent: What's your view? Halt the review this particular patch for > > now and rework, or try and get this all integrated? > > Mainline obviously already has dma-buf heaps merged, whilst I have a > > PR cherry-picking it back into our downstream 5.4. The main reason it > > hasn't been merged is that I haven't had a test case to prove it > > works. The rework should be relatively simple, but will need small > > updates to both libcamera and ISP driver. > > As months have passed, libcamera moved to allocate lens shading tables > using dma-buf heaps and the only user I can name of the vc-sm-cma > driver is the actual ISP, that needs to import the dmabuf pointing to > the lens shading maps with vc_sm_cma_import_dmabuf(). You've also got vc04_services/vchiq-mmal/mmal-vchiq.c importing dmabufs, either from vb2_contig or imported from elsewhere when using VB2_MEMORY_DMABUF. > Upstreaming the whole vc-sm-cma driver as it is for this single kAPI > seems a bit a no-go. Dave, what would you prefer here ? Should I > provide a minimal vc-sm-cam driver that only performs buffer importing > to support the ISP driver ? Is the buffer importing into VPU there to > stay or is its usage transitional and can be kept out of the next > submission of this series ? Both imports are here to stay as the VPU needs to be able to use those blocks of memory. This first iteration picked up a fair number of extraneous lumps (eg the caching calls). I got a reminder last week that I promised a reworked version of vc-sm-cma to you and I hadn't done it - sorry, juggling too many things. I'll get on it now, so nudge me if I haven't pushed it to you by the end of the week for your review. We can trim it down significantly now that we have dma-heaps in and working. There's a niggle that the current dma-heaps are always cached on the ARM, but that just means that the user has to be careful to use DMA_BUF_IOCTL_SYNC correctly (which they should be doing anyway). Whilst waiting for that, the Unicam driver, and the prep work in mmal-vchiq could all be pushed first, and ideally as two independent patchsets as there are no inter-dependencies between them. Dave > Thanks > j > > > > > Dave > > > > [1] https://github.com/raspberrypi/userland/tree/master/host_applications/linux/libs/sm > > > > > Regards, > > > Nicolas > > >
