RE: Question for an accepted patch: use of DMA-BUF based videobuf2 capture buffer with no-HW-cache-coherent HW

[yuji2.ishikawa]

Hi Hans,

> -----Original Message-----
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Sent: Monday, October 24, 2022 4:49 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; posciak@xxxxxxxxxxxx;
> paul.kocialkowski@xxxxxxxxxxx; mchehab+samsung@xxxxxxxxxx;
> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: Question for an accepted patch: use of DMA-BUF based videobuf2
> capture buffer with no-HW-cache-coherent HW
> 
> Hi Yuji,
> 
> On 10/24/22 06:02, yuji2.ishikawa@xxxxxxxxxxxxx wrote:
> > Hi,
> >
> > I'm porting a V4L2 capture driver from 4.19.y to 5.10.y [1].
> >
> > When I test the ported driver, I sometimes find a corruption on a captured
> image.
> >
> > Because the corruption is exactly aligned with cacheline, I started
> investigation from map/unmap of DMA-BUF.
> >
> >
> >
> > The capture driver uses DMA-BUF for videobuf2.
> >
> > The capture hardware does not have HW-mantained cache coherency with
> CPU, that is, explicit map/unmap is essential on QBUF/DQBUF.
> >
> > After some hours of struggle, I found a patch removing cache synchronizations
> on QBUF/DQBUF.
> >
> >
> >
> > https://patchwork.kernel.org/project/linux-media/patch/20190124095156.
> > 21898-1-paul.kocialkowski@xxxxxxxxxxx/
> > <https://patchwork.kernel.org/project/linux-media/patch/20190124095156
> > .21898-1-paul.kocialkowski@xxxxxxxxxxx/>
> >
> >
> >
> > When I removed this patch from my 5.10.y working-tree, the driver
> > yielded images without any defects.v
> >
> >
> >
> > ***************
> >
> > Sorry for a mention to a patch released 4 years ago.
> >
> > The patch removes map/unmap on QBUF/DQBUF to improve the
> performance of V4L2 decoder device, by reusing previously decoded frames.
> >
> > However, there seems no cares nor compensations for modifying lifecycle of
> DMA-BUF, especially on video capture devices.
> 
> I'm not entirely sure what you mean exactly.
> 
My concern is consistency between ioctls and the state transition of capture buffers.
Generally, streaming I/O (DMA-BUF importing) buffers are handled following by userland.

Ioctl(VIDIOC_QBUF) -> /* DMA transfer from HW*/ -> ioctl(VIDIOC_DQBUF) -> /* access from CPU */ -> ioctl(VIDIOC_QBUF) -> ...

Therefore, expected semantics is that a buffer is owned by HW after QBUF, and owned by CPU after DQBUF.
In practice, ioctl(QBUF) kicks vb2_dc_map_dma_buf() and ioctl(DQBUF) kicks vb2_dc_unmap_dma_buf() before applying the patch.
This implementation keeps consistency in terms of cache coherency as cache-clean is done in vb2_dc_map_dma_buf().

By applying the patch, ioctl(DQBUF) does not kick unmap_dma() anymore. The similar for ioctl(QBUF).
Therefore, in practice, a buffer is not owned by CPU just after ioctl(DQBUF).
To keep compatibility of buffer operations, there should be delayed map_dma()/unmap_dma() call just before DMA-transfer/CPU-access.
However, no one referred to such function in the v4l2 framework in the examination of the patch.
Also, there is no advice for individual video device drivers; such that adding map_dma()/unmap_dma() explicitly.

> >
> >
> >
> > Would you tell me some idea on this patch:
> >
> > * Do well-implemented capture drivers work well even if this patch is applied?
> 
> Yes, dmabuf is used extensively and I have not had any reports of issues.

Many architectures can avoid this problem.
A problem statistically occurs, only if a video capture HW does not have HW-maintained cache coherency with CPU.
Does this patch consider such case?

> >
> > * How should a video capture driver call V4L2/videobuf2 APIs, especially
> when the hardware does not support cache coherency?
> 
> It should all be handled correctly by the core frameworks.
> 
> I think you need to debug more inside videobuf2-core.c. Some printk's that show
> the dmabuf fd when the buffer is mapped and when it is unmapped + the length
> it is mapping should hopefully help a bit.

I added printk and dump_stack() to several functions.
The patched function __prepare_dmabuf() is called every ioctl(QBUF).
Function vb2_dc_map_dmabuf() is called only for the 1st call of ioctl(QBUF) for a buffer instance.
After that, vb2_dc_map_dmabuf() was never called, as the patch intended.

Regards,
	Yuji

> 
> Regards,
> 
> 	Hans
> 
> >
> >
> >
> > ***************
> >
> > [1] FYI: the capture driver is not on mainline yet; the candidate is,
> >
> > https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@tosh
> > iba.co.jp/
> > <https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@tos
> > hiba.co.jp/>
> >
> >
> >
> >
> >
> > Regards,
> >
> >               Yuji Ishikawa
> >