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

From: Dave Stevenson <dave.stevenson@raspberrypi.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: "Nicolas Saenz Julienne" <nsaenzjulienne@suse.de>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.org>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Naushir Patuck" <naush@raspberrypi.com>
Subject: Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver
Date: Thu, 27 Aug 2020 18:19:29 +0100	[thread overview]
Message-ID: <CAPY8ntBzSxAnq3h0dWPuG4F825XH28Q+X66AF7KBnEYdBf=kEg@mail.gmail.com> (raw)
In-Reply-To: <20200827164624.6lizqnjs5uewftrc@uno.localdomain>

Hi Jacopo

On Thu, 27 Aug 2020 at 17:42, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave,
>
> On Thu, Aug 27, 2020 at 01:51:07PM +0100, Dave Stevenson wrote:
> > On Thu, 27 Aug 2020 at 11:34, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> [snip]
>
> > > >
> > > > 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 :/
> >
> > vb2 allocated by the ISP (VB2_MEMORY_MMAP), or from Unicam that is
> > then imported into the ISP (VB2_MEMORY_DMABUF)?
> >
> > The former is the dma-ranges being incorrectly set, and the reason for
> > the patch where VCHIQ children inherit the parent's dma config. It's
> > possible something else has changed in the setup since.
>
> Re-introducing those two patches which I left out yesterday waiting
> for more comments made the warning go away, so I presume that was
> memory allocated on the ISP, exported as dmabuf by libcamera, and then
> re-imported in the video device (that's what our FrameBufferAllocator
> does, and I was testing with 'cam' which uses that class to reserve
> memory per-stream).
>
> I have a discussion open with Phil and Nicolas to see how those two
> patches might be made upstream consumable, but I'll remember to keep
> them in when testing.

OK, I'd seen some emails flying around about those patches, but then
couldn't find them when I went looking earlier.

> >
> > The latter is likely to be more involved and depend on what actually
> > allocated it (which may mean I'm using the wrong API calls).
> > I'm getting the dma address via sg_dma_address having attached and
> > mapped the dmabuf. I'm expecting that dma address to therefore follow
> > the dma-ranges of my device (not the allocator), but I'm not 100%
> > certain that is what does happen.
> >
> > A few more details of what exactly the use case that triggers it would
> > be useful.
> >
> > > >
> > > > 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 ?
> >
> > Things never stand still!
>
> Isn't it great ? :D
>
> > Requesting the VPU clock is to avoid a FIFO overflow.
> > We've gained pixel format support (with defines and docs) for Y12P and Y14P.
> > And hopefully I fixed up all the review comments from v1.
>
> Do you think it's worth upstreaming those parts in v2 or should it be
> done on top ? I should check if the VPU clock is exposed or not in
> mainline first...

Adding in the clock would be useful to avoid odd image issues
(particularly on imx477), but you're right it has a dependency on the
clock driver. I thought Maxime had managed to get that merged, but I
haven't checked.

Adding the image formats is fairly uncontentious as long as I've got
the docs for the image formats right. I did jump through the hoops to
test the docs, so I hope they're OK. The changes to the driver to
support them are a dozen lines adding them to a table.

> >
> > It's hard to guess why libcamera isn't happy if we're getting ISRs. We
> > do need to get the appropriate flags in the ISR for frame end (FEI or
> > PI0).
> > Is this Pi3 or Pi4 that you're testing on? Pi3 support should be sound
> > on mainline. AFAIK Pi4 is still a work in progress.
> > Can you stream just using v4l2-ctl --stream-mmap=3 --stream-count=1000
> > --stream-to=/dev/null ? That should work with /dev/video0. Getting the
> > embedded data is more involved over the ordering of opening and
> > starting streaming on the nodes.
>
> It's a Pi4.

Sorry, I've not tried mainline on a Pi4 yet.

> After a pleasant day of debugging I got a nice trace that shows me
> that it's actually the buffer importing part that hangs on a completion
> in vc_sm_cma_vchi_import.
>
> In mainline the whole vchi interface got dropped and I got to port the
> vc-sma-cma driver to use the vchiq interface directly. I might have
> screwed something up and I'm now looking suspiciously at this thread
> function: "vc_sm_cma_vchi_videocore_io()" (also because I had to move it
> to use msg_hold+msg_release, as peek+remove was killed by:
> b5f1547b6e3bd ("staging: vchi: Get rid of vchi_msg_peek()")

I'll look at those vchi/vchiq patches if they've now been merged. It
makes some sense for us to backport them so they get a real thrashing.
If you've gone through any of the existing drivers doing the
conversion, then feel free to throw me the files to have a look at
(and it potentially saves me some effort in doing the same
conversion).

> I'll look into that again.
>
> In the meantime is there any chance this rings any bell to you?
> https://paste.debian.net/1161469/

Nothing obvious - sorry.

> Thanks
>    j
>
> >
> > (We do have an open issue regarding getting the correct DMA address
> > for Unicam on PI0 & 1. Those share the L2 cache for many things so
> > dma-ranges is set differently, and not in a way that currently works
> > with the Unicam driver. That one may need a followup fix.)

Issue resolved. My check in the driver was invalid - we have a
downstream PR for it now.

  Dave