Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Dave,

On Tue, Aug 25, 2020 at 06:52:18PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 24 Aug 2020 at 17:36, Jacopo Mondi <jacopo@jmondi.org> 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@suse.de> wrote:
> > > >
> > > > Hi Laurent, Dave,
> > > >
> > > > On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
> > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> > > > >
> > > > > 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.

Of course. Re-looking at it, the lens-shading tables are allocated on
dmabuf heaps and the exported dmabuf fd passed with a custom control to the
ISP, which uses it to set a mmal port parameter. I got lost in the code
base at mmal-vchiq.c:port_parameter_set(), which receives a
struct bcm2835_isp_lens_shading which contains the dmabuf fd. I assume
it then maps it into the VPU memory to access the shading tables.

But of course buffer queueing to the ISP requires dmabuf importing in
the VPU, and that happens by 'submitting' a buffer to mmal-vchiq
vchiq_mmal_submit_buffer() which does that by calling
vc_sm_cma_import_dmabuf().

I hope I have a more clear idea of the two paths now.

>
> > 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.
>

Of course. I was wondering if a fairly big component like vc-sma-cma
isn't too much for just importing, and reading further it seems like
this is a shared concern.

> 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.

Great, so I'll wait for news from your side

>
> 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).

I am running what was meant to be a v2 of this series and I get a
complaint:
vc_sm_cma_import_dmabuf_internal: Expecting an uncached alias for dma_addr

When I read this paragraph from your email yesterday I immediately
thought "this should be the LSC table".

Debugging it further I found out it's actually a vb2 buffer. I have
reduced the list of patches in v2 compared to this long one, and I'm
probably left out something relevant :/
>
> 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.

I could start sending out the unicam driver, yes.

Currently I'm a bit stuck not being able to receive frames from the
unicam driver. I see the buffers being returned by the ISR routine,
but I never get a buffer available notification in libcamera.

There's been a few changes to the downstream unicam driver (ie
requesting the VPU clock frequency) and I see the RPi mainline support
has moved forward quite a bit since v5.8. Are you aware of any trivial
change I might be missing that could cause this ?

Thanks
  j

>
>   Dave
>
> > Thanks
> >   j
> >
> > >
> > >   Dave
> > >
> > > [1] https://github.com/raspberrypi/userland/tree/master/host_applications/linux/libs/sm
> > >
> > > > Regards,
> > > > Nicolas
> > > >