Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1679821 > unrolled thread

Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-03 11:30 +0200
    Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable         MMAP Christoph Hellwig <hch@lst.de> - 2017-07-05 19:40 +0200
      Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-13 13:20 +0200
        Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable  MMAP Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-07-13 15:30 +0200
          Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable         MMAP Christoph Hellwig <hch@lst.de> - 2017-07-13 15:40 +0200

#1679821 — Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

Hi All,

On 2016-10-26 10:52, Thierry Escande wrote:
> This series adds support for cacheable MMAP in DMA coherent allocator.
>
> The first patch moves the vb2_dc_get_base_sgt() function above mmap
> callbacks for calls introduced by the second patch. This avoids a
> forward declaration.

I'm sorry for late review. Sylwester kicked me for pending v4l2/vb2 patches
and I've just found this thread in my TODO folder.

The main question here if we want to merge incomplete solution or not. As
for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
attribute is not fully implemented nor even defined in mainline.

I know that it works fine for some vendor kernel trees, but supporting it in
mainline was a bit controversial. There is no proper way to sync cache 
for such
buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
a proper DMA API.

> Changes in v2:
> - Put function move in a separate patch
> - Added comments
>
> Changes in v3:
> - Remove redundant test on NO_KERNEL_MAPPING DMA attribute in mmap()
>
> Heng-Ruey Hsu (1):
>    [media] videobuf2-dc: Support cacheable MMAP
>
> Thierry Escande (1):
>    [media] videobuf2-dc: Move vb2_dc_get_base_sgt() above mmap callbacks
>
>   drivers/media/v4l2-core/videobuf2-dma-contig.c | 60 ++++++++++++++++----------
>   1 file changed, 38 insertions(+), 22 deletions(-)
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [next] | [standalone]

#1681680 — Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:
> The main question here if we want to merge incomplete solution or not. As
> for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
> Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
> attribute is not fully implemented nor even defined in mainline.
>

DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
semantics through the dma_alloc_attr API, and as such I think it is
pretty well defined, although the documentation in
Documentation/DMA-attributes.txt is really bad and we need to improve
it, by merging it with the dma_alloc_noncoherent description in
Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
we should probably merge Documentation/DMA-API.txt,
Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
into a single coherent document.


> I know that it works fine for some vendor kernel trees, but supporting it in
> mainline was a bit controversial. There is no proper way to sync cache for 
> such
> buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
> a proper DMA API.

As documented in Documentation/DMA-API.txt the proper way to sync
noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
like it generally is the same as dma_sync_range/sg so if we could
eventually merge these APIs that should reduce the confusion further.

[toc] | [prev] | [next] | [standalone]

#1686450

Hi Christoph,

On 2017-07-05 19:33, Christoph Hellwig wrote:
> On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:
>> The main question here if we want to merge incomplete solution or not. As
>> for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
>> Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
>> attribute is not fully implemented nor even defined in mainline.
>>
> DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
> semantics through the dma_alloc_attr API, and as such I think it is
> pretty well defined, although the documentation in
> Documentation/DMA-attributes.txt is really bad and we need to improve
> it, by merging it with the dma_alloc_noncoherent description in
> Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
> updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
> we should probably merge Documentation/DMA-API.txt,
> Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
> into a single coherent document.

Right. I started conversion of dma_alloc_noncoherent to NON_CONSISTENT
DMA attribute, but later I got stuck at the details of cache 
synchronization.

>> I know that it works fine for some vendor kernel trees, but supporting it in
>> mainline was a bit controversial. There is no proper way to sync cache for
>> such
>> buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
>> a proper DMA API.
> As documented in Documentation/DMA-API.txt the proper way to sync
> noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
> like it generally is the same as dma_sync_range/sg so if we could
> eventually merge these APIs that should reduce the confusion further.

Original dma_alloc_noncoherent utilized dma_cache_sync() function, which had
some flaws, which prevented me to continue that task and introduce it to ARM
architecture. The dma_alloc_noncoherent() and dma_cache_sync() API lacks
buffer ownership and imprecisely defines how and when the caches has to be
synchronized. dma_cache_sync() also lacks DMA address argument, what also
complicates potential lightweight implementation.

IMHO it would make sense to change it to work similar to the other
dma_sync_*_for_{cpu,device} functions, but I didn't find enough time to
finally take a try.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[toc] | [prev] | [next] | [standalone]

#1686533 — Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

On Thu, Jul 13, 2017 at 01:13:28PM +0200, Marek Szyprowski wrote:
> Hi Christoph,
> 
> On 2017-07-05 19:33, Christoph Hellwig wrote:
> >On Mon, Jul 03, 2017 at 11:27:32AM +0200, Marek Szyprowski wrote:
> >>The main question here if we want to merge incomplete solution or not. As
> >>for now, there is no support in ARM/ARM64 for NON_CONSISTENT attribute.
> >>Also none of the v4l2 drivers use it. Sadly support for NON_CONSISTENT
> >>attribute is not fully implemented nor even defined in mainline.
> >>
> >DMA_ATTR_NON_CONSISTENT is the way to get the dma_alloc_noncoherent
> >semantics through the dma_alloc_attr API, and as such I think it is
> >pretty well defined, although the documentation in
> >Documentation/DMA-attributes.txt is really bad and we need to improve
> >it, by merging it with the dma_alloc_noncoherent description in
> >Documentation/DMA-API.txt. My series to remove dma_alloc_noncoherent
> >updates the latter to mention DMA_ATTR_NON_CONSISTENT, but
> >we should probably merge Documentation/DMA-API.txt,
> >Documentation/DMA-attributes.txt and Documentation/DMA-API-HOWTO.txt
> >into a single coherent document.
> 
> Right. I started conversion of dma_alloc_noncoherent to NON_CONSISTENT
> DMA attribute, but later I got stuck at the details of cache
> synchronization.
> 
> >>I know that it works fine for some vendor kernel trees, but supporting it in
> >>mainline was a bit controversial. There is no proper way to sync cache for
> >>such
> >>buffers. Calling dma_sync_sg worked so far, but it has to be first agreed as
> >>a proper DMA API.
> >As documented in Documentation/DMA-API.txt the proper way to sync
> >noncoherent/nonconsistent regions is to call dma_cache_sync.  It seems
> >like it generally is the same as dma_sync_range/sg so if we could
> >eventually merge these APIs that should reduce the confusion further.
> 
> Original dma_alloc_noncoherent utilized dma_cache_sync() function, which had
> some flaws, which prevented me to continue that task and introduce it to ARM
> architecture. The dma_alloc_noncoherent() and dma_cache_sync() API lacks
> buffer ownership and imprecisely defines how and when the caches has to be
> synchronized. dma_cache_sync() also lacks DMA address argument, what also
> complicates potential lightweight implementation.

My conclusion of the dma_alloc_noncoherent() and dma_cache_sync() API
when it was introduced is that it's basically a completely broken
interface, and I've never seen any point to it.  Maybe some of that is
because it's badly documented - which in turn makes it badly designed
(because there's no specification detailing what it's supposed to be
doing.)

I'd like to see that thing die...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1686536 — Re: [PATCH v3 0/2] [media] videobuf2-dc: Add support for cacheable MMAP

On Thu, Jul 13, 2017 at 02:21:53PM +0100, Russell King - ARM Linux wrote:
> My conclusion of the dma_alloc_noncoherent() and dma_cache_sync() API
> when it was introduced is that it's basically a completely broken
> interface, and I've never seen any point to it.  Maybe some of that is
> because it's badly documented - which in turn makes it badly designed
> (because there's no specification detailing what it's supposed to be
> doing.)
> 
> I'd like to see that thing die...

It's not exactly the best interface ever, so any improvement is welcome.

I've posted a series to kill dma_alloc_noncoherent in favor of
dma_alloc_attrs a short while ago, and a big chunk of it should have
made it into 4.12.  I plan to kill it off entirely for 4.13.

That leaves dma_cache_sync() - it's used by 6 drivers:

drivers/net/ethernet/i825xx/lasi_82596.c
drivers/net/ethernet/seeq/sgiseeq.c
drivers/scsi/53c700.c
drivers/scsi/sgiwd93.c
drivers/sh/maple/maple.c
drivers/tty/serial/mpsc.c

Those are used on parisc, mips for a few old SGI systems, the SH
dreamcast and powerpc marvell mv64x60 devices.

So it shouldn't be too hard to figure out if they could be moved
to the normal dma_sync_* calls.

On parisc dma_cache_sync is equivalent to dma_sync_single_for_cpu,
so that should be fine.

On mips the implementation of dma_sync_single_for_cpu is a little
more complicated, but both dma_sync_single_for_cpu and dma_cache_sync
end up calling __dma_sync_virtual, so they look like the same in
the end as well.

On SH sync_single_for_device is implemented using dma_cache_sync,
and there is no dma_sync_single_for_cpu.

On powerpc both dma_sync_single_for_cpu and dma_sync_single_for_device
are implemented using the same primitive as dma_cache_sync.

In short: killing off dma_cache_sync and using the existing and
better defined syncing primitives looks entirely feasible.

I'll add it to my TODO list for 4.13.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web