Hi Hans, > -----Original Message----- > From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > Sent: Thursday, December 1, 2022 8:33 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@xxxxxxxxxxxxx>; posciak@xxxxxxxxxxxx; > paul.kocialkowski@xxxxxxxxxxx; mchehab+samsung@xxxxxxxxxx; > linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Tomasz Figa > <tfiga@xxxxxxxxxxxx> > Subject: Re: Question for an accepted patch: use of DMA-BUF based videobuf2 > capture buffer with no-HW-cache-coherent HW > > Hi Yuji, > > On 26/10/2022 11:16, yuji2.ishikawa@xxxxxxxxxxxxx wrote: > > 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/201901240951 > >>> 56 .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. > > The cache syncing is supposed to happen in __vb2_buf_mem_finish() where the > 'finish' memop is called. > > But for DMABUF it notes that: > > /* > * DMA exporter should take care of cache syncs, so we can avoid > * explicit ->prepare()/->finish() syncs. For other ->memory types > * we always need ->prepare() or/and ->finish() cache sync. > */ It seems I have misunderstood how DMA-BUF's cache syncs are maintained along with videobuf2 API calls. I understand that cache syncs are expected to be handled before prepare() and after finish(). The "ownership" transition along QBUF/DQBUF came from my misunderstanding, please forget. > And here https://docs.kernel.org/driver-api/dma-buf.html I read that userspace > must call DMA_BUF_IOCTL_SYNC to ensure the caches are synced before > using the buffer. > > Are you calling DMA_BUF_IOCTL_SYNC? Missing calling ioctl(DMA_BUF_IOCTL_SYNC) in userland was exactly the cause. I read the document, carried out experiments and found it worked completely. Very sorry to bother you. Regards, Yuji > I suspect that vb2_dc_unmap_dma_buf() caused a cache sync, so you never > noticed issues. > > Regards, > > Hans > > > > >>> > >>> > >>> > >>> 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@to > >>> sh > >>> iba.co.jp/ > >>> <https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@t > >>> os > >>> hiba.co.jp/> > >>> > >>> > >>> > >>> > >>> > >>> Regards, > >>> > >>> Yuji Ishikawa > >>>
