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

Groups > linux.kernel > #1681680 > 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 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

#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] | [next] | [standalone]

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

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

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