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

Groups > linux.kernel > #1670775 > unrolled thread

Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool

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 v5 4/7] drivers: dma-coherent: Introduce default DMA pool Christoph Hellwig <hch@infradead.org> - 2017-06-20 15:50 +0200
    Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Robin Murphy <robin.murphy@arm.com> - 2017-06-20 16:30 +0200
      Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Christoph Hellwig <hch@infradead.org> - 2017-06-26 11:50 +0200
        Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Vladimir Murzin <vladimir.murzin@arm.com> - 2017-06-26 16:10 +0200
        Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Robin Murphy <robin.murphy@arm.com> - 2017-06-27 16:40 +0200
          Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Christoph Hellwig <hch@infradead.org> - 2017-06-27 17:30 +0200
    Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Vladimir Murzin <vladimir.murzin@arm.com> - 2017-06-22 15:20 +0200
      Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool Christoph Hellwig <hch@infradead.org> - 2017-06-26 11:50 +0200

#1670775 — Re: [PATCH v5 4/7] drivers: dma-coherent: Introduce default DMA pool

On Wed, May 24, 2017 at 11:24:29AM +0100, Vladimir Murzin wrote:
> This patch introduces default coherent DMA pool similar to default CMA
> area concept. To keep other users safe code kept under CONFIG_ARM.

I don't see a CONFIG_ARM in the code, although parts of it are added
under CONFIG_OF_RESERVED_MEM.

But overall this code look a bit odd to me.  As far as I can tell
the dma-coherent.c code is for the case where we have a special
piece of coherent memory close to a device.

If you're allocating out of the global allocator the memory should
come from the normal dma_ops ->alloc allocator - and also take
the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)

[toc] | [next] | [standalone]

#1670822

On 20/06/17 14:49, Christoph Hellwig wrote:
> On Wed, May 24, 2017 at 11:24:29AM +0100, Vladimir Murzin wrote:
>> This patch introduces default coherent DMA pool similar to default CMA
>> area concept. To keep other users safe code kept under CONFIG_ARM.
> 
> I don't see a CONFIG_ARM in the code, although parts of it are added
> under CONFIG_OF_RESERVED_MEM.

It's in rmem_dma_setup() (line 325) currently enforcing no-map for ARM.

> But overall this code look a bit odd to me.  As far as I can tell
> the dma-coherent.c code is for the case where we have a special
> piece of coherent memory close to a device.

True, but the case here is where we need a special piece of coherent
memory for *all* devices, and it was more complicated *not* to reuse the
existing infrastructure. This would already be achievable by specifying
a separate rmem carveout per device, but the shared pool just makes life
easier, and mirrors the functionality dma-contiguous already supports.

> If you're allocating out of the global allocator the memory should
> come from the normal dma_ops ->alloc allocator - and also take
> the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
> DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)

The context here is noMMU but with caches - the problem being that the
normal allocator will give back kernel memory, and there's no way to
make that coherent with devices short of not enabling the caches in the
first place, which is obviously undesirable. The trick is that RAM is
aliased (in hardware) at two addresses, one of which makes CPU accesses
non-cacheable, so by only ever accessing the RAM set aside for the
coherent DMA pool using the non-cacheable alias (represented by the
dma_pfn_offset) we can achieve DMA coherency.

It perhaps seems a bit backwards, but we do actually end up honouring
DMA_ATTR_NON_CONSISTENT to a degree in patch #5, as such requests are
the only ones allowed to fall back to the normal dma_ops allocator.

Robin.

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

#1674579

On Tue, Jun 20, 2017 at 03:24:21PM +0100, Robin Murphy wrote:
> True, but the case here is where we need a special piece of coherent
> memory for *all* devices, and it was more complicated *not* to reuse the
> existing infrastructure. This would already be achievable by specifying
> a separate rmem carveout per device, but the shared pool just makes life
> easier, and mirrors the functionality dma-contiguous already supports.

І'm really worried about the code in dma-coherent.c - the original
version clearly intends to have a coherent pool per device, declared
in the driver.  Then Marek added the reserved_mem interface, and
now we get another variant of it.  Conceptually the per-device
and global pool are very different, and to me it seems like the
reserved mem should be a different interface.

> > If you're allocating out of the global allocator the memory should
> > come from the normal dma_ops ->alloc allocator - and also take
> > the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
> > DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)
> 
> The context here is noMMU but with caches - the problem being that the
> normal allocator will give back kernel memory, and there's no way to
> make that coherent with devices short of not enabling the caches in the
> first place, which is obviously undesirable. The trick is that RAM is
> aliased (in hardware) at two addresses, one of which makes CPU accesses
> non-cacheable, so by only ever accessing the RAM set aside for the
> coherent DMA pool using the non-cacheable alias (represented by the
> dma_pfn_offset) we can achieve DMA coherency.

Yes, and I think this is something we already have to deal with
for example on mips.  A simple genalloc allocator from your pool
in the normal dma_ops implementation should do the work just fine.

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

#1674807

On 26/06/17 10:42, Christoph Hellwig wrote:
> On Tue, Jun 20, 2017 at 03:24:21PM +0100, Robin Murphy wrote:
>> True, but the case here is where we need a special piece of coherent
>> memory for *all* devices, and it was more complicated *not* to reuse the
>> existing infrastructure. This would already be achievable by specifying
>> a separate rmem carveout per device, but the shared pool just makes life
>> easier, and mirrors the functionality dma-contiguous already supports.
> 
> І'm really worried about the code in dma-coherent.c - the original
> version clearly intends to have a coherent pool per device, declared
> in the driver.  Then Marek added the reserved_mem interface, and
> now we get another variant of it.  Conceptually the per-device
> and global pool are very different, and to me it seems like the
> reserved mem should be a different interface.
> 
>>> If you're allocating out of the global allocator the memory should
>>> come from the normal dma_ops ->alloc allocator - and also take
>>> the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
>>> DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)
>>
>> The context here is noMMU but with caches - the problem being that the
>> normal allocator will give back kernel memory, and there's no way to
>> make that coherent with devices short of not enabling the caches in the
>> first place, which is obviously undesirable. The trick is that RAM is
>> aliased (in hardware) at two addresses, one of which makes CPU accesses
>> non-cacheable, so by only ever accessing the RAM set aside for the
>> coherent DMA pool using the non-cacheable alias (represented by the
>> dma_pfn_offset) we can achieve DMA coherency.
> 
> Yes, and I think this is something we already have to deal with
> for example on mips.  A simple genalloc allocator from your pool
> in the normal dma_ops implementation should do the work just fine.
> 

Are you proposing keeping pool handling under arch?

Cheers
Vladimir

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

#1675834

On 26/06/17 10:42, Christoph Hellwig wrote:
> On Tue, Jun 20, 2017 at 03:24:21PM +0100, Robin Murphy wrote:
>> True, but the case here is where we need a special piece of coherent
>> memory for *all* devices, and it was more complicated *not* to reuse the
>> existing infrastructure. This would already be achievable by specifying
>> a separate rmem carveout per device, but the shared pool just makes life
>> easier, and mirrors the functionality dma-contiguous already supports.
> 
> І'm really worried about the code in dma-coherent.c - the original
> version clearly intends to have a coherent pool per device, declared
> in the driver.  Then Marek added the reserved_mem interface, and
> now we get another variant of it.  Conceptually the per-device
> and global pool are very different, and to me it seems like the
> reserved mem should be a different interface.

Per-device reserved mem is still a private per-device pool though, it's
just discovered and declared by common firmware code rather than in some
device-specific way by driver code - once it's assigned there's no
distinction. The global/per-device issue is essentially entirely
orthogonal, and has the dubious pleasure of being a massive conceptual
difference yet a much smaller implementation difference.

>>> If you're allocating out of the global allocator the memory should
>>> come from the normal dma_ops ->alloc allocator - and also take
>>> the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
>>> DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)
>>
>> The context here is noMMU but with caches - the problem being that the
>> normal allocator will give back kernel memory, and there's no way to
>> make that coherent with devices short of not enabling the caches in the
>> first place, which is obviously undesirable. The trick is that RAM is
>> aliased (in hardware) at two addresses, one of which makes CPU accesses
>> non-cacheable, so by only ever accessing the RAM set aside for the
>> coherent DMA pool using the non-cacheable alias (represented by the
>> dma_pfn_offset) we can achieve DMA coherency.
> 
> Yes, and I think this is something we already have to deal with
> for example on mips.  A simple genalloc allocator from your pool
> in the normal dma_ops implementation should do the work just fine.

I admit I'm almost in agreement, were it not for the fact that
dma-contiguous already supports all four combinations of both per-device
and global pools, and both reserved mem and direct declarations from
arch/platform code, all through the same interface to boot, and nobody's
complaining about that. The only real difference for dma-coherent seems
to be the way it's baked into the existing API.

If it is just a matter of interfaces, I'd have no objection to exporting
a separate e.g. dma_alloc_from_global_coherent() or somesuch as a
conceptually separate interface to dma_coherent_default_memory, which
the arch code can then call from ->alloc in the same manner they
currently call dma_alloc_from_contiguous(). That seems like a reasonable
way to keep the per-device and global pools conceptually distinct
without needlessly duplicating implementations. In fact, I'm now
wondering if the regular arm/arm64 atomic pools couldn't also make use
of such a thing as well...

Robin.

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

#1675893

On Tue, Jun 27, 2017 at 03:36:16PM +0100, Robin Murphy wrote:
> I admit I'm almost in agreement, were it not for the fact that
> dma-contiguous already supports all four combinations of both per-device
> and global pools, and both reserved mem and direct declarations from
> arch/platform code, all through the same interface to boot, and nobody's
> complaining about that. The only real difference for dma-coherent seems
> to be the way it's baked into the existing API.
> 
> If it is just a matter of interfaces, I'd have no objection to exporting
> a separate e.g. dma_alloc_from_global_coherent() or somesuch as a
> conceptually separate interface to dma_coherent_default_memory, which
> the arch code can then call from ->alloc in the same manner they
> currently call dma_alloc_from_contiguous(). That seems like a reasonable
> way to keep the per-device and global pools conceptually distinct
> without needlessly duplicating implementations. In fact, I'm now
> wondering if the regular arm/arm64 atomic pools couldn't also make use
> of such a thing as well...

Ok.  I think I'll just go ahead with the current patches, and then
we'll try to come up with something better later.  I really don't
want it in actual arch code, but I want it controlled from the
dma_map_ops instance instead of from generic code.  There will be
a lot of churn in this area if my plans go ahead, so I think we can
handle it then.

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

#1672616

On 20/06/17 14:49, Christoph Hellwig wrote:
> On Wed, May 24, 2017 at 11:24:29AM +0100, Vladimir Murzin wrote:
>> This patch introduces default coherent DMA pool similar to default CMA
>> area concept. To keep other users safe code kept under CONFIG_ARM.
> 
> I don't see a CONFIG_ARM in the code, although parts of it are added
> under CONFIG_OF_RESERVED_MEM.

It should look like that:

#ifdef CONFIG_ARM
	if (!of_get_flat_dt_prop(node, "no-map", NULL)) {
		pr_err("Reserved memory: regions without no-map are not yet supported\n");
		return -EINVAL;
	}
+
+	if (of_get_flat_dt_prop(node, "linux,dma-default", NULL)) {
+		WARN(dma_reserved_default_memory,
+		     "Reserved memory: region for default DMA coherent area is redefined\n");
+		dma_reserved_default_memory = rmem;
+	}
#endif


> 
> But overall this code look a bit odd to me.  As far as I can tell
> the dma-coherent.c code is for the case where we have a special
> piece of coherent memory close to a device.
> 
> If you're allocating out of the global allocator the memory should
> come from the normal dma_ops ->alloc allocator - and also take
> the attrs into account (e.g. for DMA_ATTR_NON_CONSISTENT or
> DMA_ATTR_NO_KERNEL_MAPPING requests you don't need coherent memory)
> 

It is how it has been started [1] - defining memory which is not cacheable
(i.e. suitable for coherent allocations) and building custom allocator on top
of it, like it was done for c6x and blackfin. The annoying thing was that we
needed to advertise such memory via command line parameter plus some "mem="
adjustment to hide coherent memory from buddy allocator. So it was suggested
to use reserved memory and this makes things look much better, but on the
other hand require changes on dts side to "bind" devices with reserved memory
- default DMA pool removes such drawback.

[1] https://marc.info/?l=linux-arm-kernel&m=148163694824475&w=2

Cheers
Vladimir

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

#1674573

On Thu, Jun 22, 2017 at 02:18:48PM +0100, Vladimir Murzin wrote:
> It is how it has been started [1] - defining memory which is not cacheable
> (i.e. suitable for coherent allocations) and building custom allocator on top
> of it, like it was done for c6x and blackfin. The annoying thing was that we
> needed to advertise such memory via command line parameter plus some "mem="
> adjustment to hide coherent memory from buddy allocator. So it was suggested
> to use reserved memory and this makes things look much better, but on the
> other hand require changes on dts side to "bind" devices with reserved memory
> - default DMA pool removes such drawback.

I like the idea in general, I'm just worried about the overlap with the
per-device coherent memory, especially when we have slight semantic
mismatches like the one about the physical (or rather dma) address
earlier.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web