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

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

[yuji2.ishikawa]

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@bootlin.com/

When I removed this patch from my 5.10.y working-tree, the driver yielded images without any defects.

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

Would you tell me some idea on this patch:
* Do well-implemented capture drivers work well even if this patch is applied?
* How should a video capture driver call V4L2/videobuf2 APIs, especially when the hardware does not support cache coherency?

***************
[1] FYI: the capture driver is not on mainline yet; the candidate is,
https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@toshiba.co.jp/

***************
Sorry for sending the same email message again. I wrongly posted previous one with HTML format.

Regards,
              Yuji Ishikawa

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@xs4all.nl>
> Sent: Thursday, December 1, 2022 8:33 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@toshiba.co.jp>; posciak@chromium.org;
> paul.kocialkowski@bootlin.com; mchehab+samsung@kernel.org;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Tomasz Figa
> <tfiga@chromium.org>
> 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@toshiba.co.jp wrote:
> > Hi Hans,
> >
> >> -----Original Message-----
> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Sent: Monday, October 24, 2022 4:49 PM
> >> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> >> <yuji2.ishikawa@toshiba.co.jp>; posciak@chromium.org;
> >> paul.kocialkowski@bootlin.com; mchehab+samsung@kernel.org;
> >> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> >> 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@toshiba.co.jp 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@bootlin.com/
> >>> <https://patchwork.kernel.org/project/linux-media/patch/201901240951
> >>> 56 .21898-1-paul.kocialkowski@bootlin.com/>
> >>>
> >>>
> >>>
> >>> 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
> >>>

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

[Hans Verkuil]

Hi Yuji,

On 26/10/2022 11:16, yuji2.ishikawa@toshiba.co.jp wrote:
> Hi Hans,
> 
>> -----Original Message-----
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Sent: Monday, October 24, 2022 4:49 PM
>> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
>> <yuji2.ishikawa@toshiba.co.jp>; posciak@chromium.org;
>> paul.kocialkowski@bootlin.com; mchehab+samsung@kernel.org;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
>> 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@toshiba.co.jp 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@bootlin.com/
>>> <https://patchwork.kernel.org/project/linux-media/patch/20190124095156
>>> .21898-1-paul.kocialkowski@bootlin.com/>
>>>
>>>
>>>
>>> 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.
         */

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?

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@tosh
>>> iba.co.jp/
>>> <https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@tos
>>> hiba.co.jp/>
>>>
>>>
>>>
>>>
>>>
>>> Regards,
>>>
>>>               Yuji Ishikawa
>>>

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@xs4all.nl>
> Sent: Monday, October 24, 2022 4:49 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@toshiba.co.jp>; posciak@chromium.org;
> paul.kocialkowski@bootlin.com; mchehab+samsung@kernel.org;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@toshiba.co.jp 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@bootlin.com/
> > <https://patchwork.kernel.org/project/linux-media/patch/20190124095156
> > .21898-1-paul.kocialkowski@bootlin.com/>
> >
> >
> >
> > 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
> >

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

[Hans Verkuil]

Hi Yuji,

On 10/24/22 06:02, yuji2.ishikawa@toshiba.co.jp 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@bootlin.com/ <https://patchwork.kernel.org/project/linux-media/patch/20190124095156.21898-1-paul.kocialkowski@bootlin.com/>
> 
>  
> 
> 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.

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

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

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@toshiba.co.jp/ <https://lore.kernel.org/all/20220810132822.32534-1-yuji2.ishikawa@toshiba.co.jp/>
> 
>  
> 
>  
> 
> Regards,
> 
>               Yuji Ishikawa
>