Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[James Bottomley]

On Fri, 2009-07-10 at 15:13 +0200, Takashi Iwai wrote:
> A lazy version of dma_mmap_coherent() implementation for PA-RISC.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  arch/parisc/include/asm/dma-mapping.h |   13 +++++++++++++
>  arch/parisc/kernel/pci-dma.c          |   16 ++++++++++++++++
>  drivers/parisc/ccio-dma.c             |    1 +
>  drivers/parisc/iommu-helpers.h        |   16 ++++++++++++++++
>  drivers/parisc/sba_iommu.c            |    1 +
>  5 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/parisc/include/asm/dma-mapping.h b/arch/parisc/include/asm/dma-mapping.h
> index da69433..b5f810e 100644
> --- a/arch/parisc/include/asm/dma-mapping.h
> +++ b/arch/parisc/include/asm/dma-mapping.h
> @@ -19,6 +19,9 @@ struct hppa_dma_ops {
>  	void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction);
>  	void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
>  	void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
> +	int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma,
> +			     void *cpu_addr, dma_addr_t handle, size_t size);
> +
>  };
>  
>  /*
> @@ -204,6 +207,16 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>  		flush_kernel_dcache_range((unsigned long)vaddr, size);
>  }
>  
> +#define ARCH_HAS_DMA_MMAP_COHERENT
> +static inline int
> +dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
> +		  void *cpu_addr, dma_addr_t handle, size_t size)
> +{
> +	if (!hppa_dma_ops->mmap_coherent)
> +		return -ENXIO;
> +	return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size);
> +}
> +
>  static inline void *
>  parisc_walk_tree(struct device *dev)
>  {
> diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c
> index c07f618..e8c68e3 100644
> --- a/arch/parisc/kernel/pci-dma.c
> +++ b/arch/parisc/kernel/pci-dma.c
> @@ -539,6 +539,20 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist *
>  		flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length);
>  }
>  
> +static int pa11_dma_mmap_coherent(struct device *dev,
> +				  struct vm_area_struct *vma,
> +				  void *cpu_addr, dma_addr_t handle,
> +				  size_t size)
> +{
> +	struct page *pg;
> +	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
> +	cpu_addr = __va(handle);
> +	pg = virt_to_page(cpu_addr);
> +	return remap_pfn_range(vma, vma->vm_start,
> +			       page_to_pfn(pg) + vma->vm_pgoff,
> +			       size, vma->vm_page_prot);
> +}
> +
>  struct hppa_dma_ops pcxl_dma_ops = {
>  	.dma_supported =	pa11_dma_supported,
>  	.alloc_consistent =	pa11_dma_alloc_consistent,
> @@ -552,6 +566,7 @@ struct hppa_dma_ops pcxl_dma_ops = {
>  	.dma_sync_single_for_device = pa11_dma_sync_single_for_device,
>  	.dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
>  	.dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
> +	.mmap_coherent =	pa11_dma_mmap_coherent,
>  };
>  
>  static void *fail_alloc_consistent(struct device *dev, size_t size,
> @@ -592,4 +607,5 @@ struct hppa_dma_ops pcx_dma_ops = {
>  	.dma_sync_single_for_device =	pa11_dma_sync_single_for_device,
>  	.dma_sync_sg_for_cpu =		pa11_dma_sync_sg_for_cpu,
>  	.dma_sync_sg_for_device =	pa11_dma_sync_sg_for_device,
> +	.mmap_coherent =	pa11_dma_mmap_coherent,
>  };
> diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
> index 0f0e0b9..0dd67ae 100644
> --- a/drivers/parisc/ccio-dma.c
> +++ b/drivers/parisc/ccio-dma.c
> @@ -1016,6 +1016,7 @@ static struct hppa_dma_ops ccio_ops = {
>  	.dma_sync_single_for_device =	NULL,	/* NOP for U2/Uturn */
>  	.dma_sync_sg_for_cpu =		NULL,	/* ditto */
>  	.dma_sync_sg_for_device =		NULL,	/* ditto */
> +	.mmap_coherent =	iommu_dma_mmap_coherent,
>  };
>  
>  #ifdef CONFIG_PROC_FS
> diff --git a/drivers/parisc/iommu-helpers.h b/drivers/parisc/iommu-helpers.h
> index a9c46cc..9f86f3f 100644
> --- a/drivers/parisc/iommu-helpers.h
> +++ b/drivers/parisc/iommu-helpers.h
> @@ -174,3 +174,19 @@ iommu_coalesce_chunks(struct ioc *ioc, struct device *dev,
>  	return n_mappings;
>  }
>  
> +/* dma_mmap_coherent callback function */
> +/* Note that this is no inline function -- this function is eventually
> + * included in ccio_ops in both ccio-dma.c and sba_iommu.c
> + */
> +static int iommu_dma_mmap_coherent(struct device *dev,
> +				   struct vm_area_struct *vma,
> +				   void *cpu_addr, dma_addr_t handle,
> +				   size_t size)
> +{
> +	struct page *pg;
> +	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
> +	pg = virt_to_page(cpu_addr);
> +	return remap_pfn_range(vma, vma->vm_start,
> +			       page_to_pfn(pg) + vma->vm_pgoff,
> +			       size, vma->vm_page_prot);
> +}
> diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
> index 123d8fe..8ccb3e4 100644
> --- a/drivers/parisc/sba_iommu.c
> +++ b/drivers/parisc/sba_iommu.c
> @@ -1070,6 +1070,7 @@ static struct hppa_dma_ops sba_ops = {
>  	.dma_sync_single_for_device =	NULL,
>  	.dma_sync_sg_for_cpu =		NULL,
>  	.dma_sync_sg_for_device =	NULL,
> +	.mmap_coherent =	iommu_dma_mmap_coherent,
>  };

This isn't the right thing to do on parisc.  The only case that will
actually work is the small subset of pa11 systems where the only way we
can manufacture coherent memory is to turn the cache off.  One more
thing: even on pa11 you have to flush the cache lines before you make a
page uncached otherwise you can get coherency problems.

For pa20 systems, we have a cache flushing system on bus writes.  What
your patch does is turn off caching over the user page only.  This would
leave us with a cache over the kernel page, so any write the kernel
makes would need flushing before it's visible to the user, which isn't
how the kernel operates with coherent memory.

The only way to fix the above is either to make the user virtual address
congruent with the kernel one (VI congruence in parisc is steps of 4MB)
so they share the same cache (in which case they don't need the uncached
bit).  Or to make the coherent memory uncached when it's first
allocated ... this is a performance hit.

The design of coherent memory was for memory based device mailboxes
managed by the kernel ... trying to give userspace coherent access to
the same mailbox is problematic because it gives a direct way for the
process to interfere with a device function ... shouldn't whatever
you're trying to do be better accomplished by using an API to control
the device and keeping the coherent mailbox fully in the kernel address
space?

James

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[Russell King]

On Fri, Jul 10, 2009 at 03:11:29PM +0000, James Bottomley wrote:
> The design of coherent memory was for memory based device mailboxes
> managed by the kernel ... trying to give userspace coherent access to
> the same mailbox is problematic because it gives a direct way for the
> process to interfere with a device function ... shouldn't whatever
> you're trying to do be better accomplished by using an API to control
> the device and keeping the coherent mailbox fully in the kernel address
> space?

As far as sound DMA goes, it's not about mailboxes.  It's about a circular
buffer which you want the device to DMA from direct to/from the DAC/ADC
and have the application write/read data directly to/from that same
buffer.

Without this, you end up having to copy the sound data - at something
around 200KB/s from applications into a driver managed buffer, which is
quite an unnecessary overhead for the CPU.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[James Bottomley]

On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> On Fri, Jul 10, 2009 at 03:11:29PM +0000, James Bottomley wrote:
> > The design of coherent memory was for memory based device mailboxes
> > managed by the kernel ... trying to give userspace coherent access to
> > the same mailbox is problematic because it gives a direct way for the
> > process to interfere with a device function ... shouldn't whatever
> > you're trying to do be better accomplished by using an API to control
> > the device and keeping the coherent mailbox fully in the kernel address
> > space?
> 
> As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> buffer which you want the device to DMA from direct to/from the DAC/ADC
> and have the application write/read data directly to/from that same
> buffer.

But that makes it sound like ordinary streaming DMA from a device to
user space ... that's what the dma_map_xx APIs are already designed to
handle:  I don't understand why you need coherent memory for this (which
can be a scarce resource on some platforms).

> Without this, you end up having to copy the sound data - at something
> around 200KB/s from applications into a driver managed buffer, which is
> quite an unnecessary overhead for the CPU.

Why?  The dma_map_xx API is designed to be zero copy.

James

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[Russell King]

On Fri, Jul 10, 2009 at 06:30:50PM +0000, James Bottomley wrote:
> On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> > As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> > buffer which you want the device to DMA from direct to/from the DAC/ADC
> > and have the application write/read data directly to/from that same
> > buffer.
> 
> But that makes it sound like ordinary streaming DMA from a device to
> user space ... that's what the dma_map_xx APIs are already designed to
> handle:  I don't understand why you need coherent memory for this (which
> can be a scarce resource on some platforms).

The streaming APIs are inefficient for this.  Consider the overhead of
having to writeback and invalidate caches at 200KB/s (which is what
you're requiring ARM to do).  That's far too much CPU overhead.

It's much more efficient to use non-cached memory for this on ARM.

We've been doing this for years and years, it's well proven.

> Why?  The dma_map_xx API is designed to be zero copy.

Except with a rather large overhead of repetitive cache handling.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[James Bottomley]

On Fri, 2009-07-10 at 19:39 +0100, Russell King wrote:
> On Fri, Jul 10, 2009 at 06:30:50PM +0000, James Bottomley wrote:
> > On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> > > As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> > > buffer which you want the device to DMA from direct to/from the DAC/ADC
> > > and have the application write/read data directly to/from that same
> > > buffer.
> > 
> > But that makes it sound like ordinary streaming DMA from a device to
> > user space ... that's what the dma_map_xx APIs are already designed to
> > handle:  I don't understand why you need coherent memory for this (which
> > can be a scarce resource on some platforms).

> The streaming APIs are inefficient for this.  Consider the overhead of
> having to writeback and invalidate caches at 200KB/s (which is what
> you're requiring ARM to do).  That's far too much CPU overhead.

Um, but streaming APIs are what we use for all block and network I/O
from user space ... we don't see huge performance problems running up to
gigabits.  Even on parisc where we have to flush several times to make
this happen we can get up to several hundred megabytes per second.

> It's much more efficient to use non-cached memory for this on ARM.
> 
> We've been doing this for years and years, it's well proven.
> 
> > Why?  The dma_map_xx API is designed to be zero copy.
> 
> Except with a rather large overhead of repetitive cache handling.

OK, so this is the bit I don't get ... why isn't the streaming a problem
for I/O then, which even on arm wants far more than 200kb/s?

James

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[Takashi Iwai]

[Sorry for the late follow-up as I've been absent]

At Fri, 10 Jul 2009 18:59:23 +0000,
James Bottomley wrote:
> 
> On Fri, 2009-07-10 at 19:39 +0100, Russell King wrote:
> > On Fri, Jul 10, 2009 at 06:30:50PM +0000, James Bottomley wrote:
> > > On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> > > > As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> > > > buffer which you want the device to DMA from direct to/from the DAC/ADC
> > > > and have the application write/read data directly to/from that same
> > > > buffer.
> > > 
> > > But that makes it sound like ordinary streaming DMA from a device to
> > > user space ... that's what the dma_map_xx APIs are already designed to
> > > handle:  I don't understand why you need coherent memory for this (which
> > > can be a scarce resource on some platforms).
> 
> > The streaming APIs are inefficient for this.  Consider the overhead of
> > having to writeback and invalidate caches at 200KB/s (which is what
> > you're requiring ARM to do).  That's far too much CPU overhead.
> 
> Um, but streaming APIs are what we use for all block and network I/O
> from user space ... we don't see huge performance problems running up to
> gigabits.  Even on parisc where we have to flush several times to make
> this happen we can get up to several hundred megabytes per second.

Well, the requirement of the non-streaming mmap is rather a historical
reason.  From the fairly early time, the sound driver provided the
mmap of a whole ring buffer since it's practical and efficient for
real-time sound processes.  But, usually the mmap mode is optional.
If it's not supported, apps should fall back to the normal read/write
mode.

Now, the problem is that we have no way to tell whether the DMA
ring-buffer mmap is available or not on each architecture.  So, my
proposal has basically two meanings:

- clarify which arch / platform supports the coherent DMA mapping
  of a whole ring-buffer
- provide the same API to cover the possible archs / platforms

In that sense, if a few (or all) PA-RISC platforms don't give the
proper coherent mapping that the sound apps require, it's OK.  We
just drop the flag in the driver as "unsupported", then.

So... before going to the detail of PA-RISC implementation, I'd like
to know your opinions: whether applying dma_mmap_coherent() to
possible archs/platforms is a reasonable solution for such a
scenario.

[Why not using the standard map->sync procedure is another level of
 question :)  My goal here is to improve the current (partly broken)
 situation in a minimal effort.  The change of mmap procedure would
 lead to major rewrites of API, and thus has to be discussed more
 deeply.]

Any comments appreciated.


thanks,

Takashi

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[James Bottomley]

On Fri, 2009-07-17 at 16:13 +0200, Takashi Iwai wrote:
> [Sorry for the late follow-up as I've been absent]
> 
> At Fri, 10 Jul 2009 18:59:23 +0000,
> James Bottomley wrote:
> > 
> > On Fri, 2009-07-10 at 19:39 +0100, Russell King wrote:
> > > On Fri, Jul 10, 2009 at 06:30:50PM +0000, James Bottomley wrote:
> > > > On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> > > > > As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> > > > > buffer which you want the device to DMA from direct to/from the DAC/ADC
> > > > > and have the application write/read data directly to/from that same
> > > > > buffer.
> > > > 
> > > > But that makes it sound like ordinary streaming DMA from a device to
> > > > user space ... that's what the dma_map_xx APIs are already designed to
> > > > handle:  I don't understand why you need coherent memory for this (which
> > > > can be a scarce resource on some platforms).
> > 
> > > The streaming APIs are inefficient for this.  Consider the overhead of
> > > having to writeback and invalidate caches at 200KB/s (which is what
> > > you're requiring ARM to do).  That's far too much CPU overhead.
> > 
> > Um, but streaming APIs are what we use for all block and network I/O
> > from user space ... we don't see huge performance problems running up to
> > gigabits.  Even on parisc where we have to flush several times to make
> > this happen we can get up to several hundred megabytes per second.
> 
> Well, the requirement of the non-streaming mmap is rather a historical
> reason.  From the fairly early time, the sound driver provided the
> mmap of a whole ring buffer since it's practical and efficient for
> real-time sound processes.  But, usually the mmap mode is optional.
> If it's not supported, apps should fall back to the normal read/write
> mode.

But what I don't understand is why you can't treat the ring buffer as
streaming.  Sure, you have a head pointer and a tail pointer, but
everything between head and tail is owned by the device and everything
between tail and head is owned by the kernel, surely?  In that case, you
can manipulate head and tail movements simply via the streaming API?
(when head moves, map the delta to the device; when tail moves, map the
delta to the kernel) and we can simply then use the standard APIs?

> Now, the problem is that we have no way to tell whether the DMA
> ring-buffer mmap is available or not on each architecture.  So, my
> proposal has basically two meanings:
> 
> - clarify which arch / platform supports the coherent DMA mapping
>   of a whole ring-buffer
> - provide the same API to cover the possible archs / platforms
> 
> In that sense, if a few (or all) PA-RISC platforms don't give the
> proper coherent mapping that the sound apps require, it's OK.  We
> just drop the flag in the driver as "unsupported", then.

That would work for us too ... I can see that x86 will have little
problem with this, but I can predict that most other architectures will
have some difficulty.

> So... before going to the detail of PA-RISC implementation, I'd like
> to know your opinions: whether applying dma_mmap_coherent() to
> possible archs/platforms is a reasonable solution for such a
> scenario.

Realistically, the only way to make it work as you want on parisc is to
have a kernel/user congruence (it's an address rule [virtual addresses
are equal modulo the congruence step, which is 4MB]) which would ensure
this would work without flushing ... that's hard for the kernel to make
happen on its own.

> [Why not using the standard map->sync procedure is another level of
>  question :)  My goal here is to improve the current (partly broken)
>  situation in a minimal effort.  The change of mmap procedure would
>  lead to major rewrites of API, and thus has to be discussed more
>  deeply.]

So, yes, there is an equivalent used by the frame buffer which basically
tries to place a dma mapped page into a user address space ... that
might also work in this case.

> Any comments appreciated.

James

Re: [PATCH 3/8] parisc: implement dma_mmap_coherent()

[Takashi Iwai]

At Fri, 17 Jul 2009 19:16:49 +0000,
James Bottomley wrote:
> 
> On Fri, 2009-07-17 at 16:13 +0200, Takashi Iwai wrote:
> > [Sorry for the late follow-up as I've been absent]
> > 
> > At Fri, 10 Jul 2009 18:59:23 +0000,
> > James Bottomley wrote:
> > > 
> > > On Fri, 2009-07-10 at 19:39 +0100, Russell King wrote:
> > > > On Fri, Jul 10, 2009 at 06:30:50PM +0000, James Bottomley wrote:
> > > > > On Fri, 2009-07-10 at 19:16 +0100, Russell King wrote:
> > > > > > As far as sound DMA goes, it's not about mailboxes.  It's about a circular
> > > > > > buffer which you want the device to DMA from direct to/from the DAC/ADC
> > > > > > and have the application write/read data directly to/from that same
> > > > > > buffer.
> > > > > 
> > > > > But that makes it sound like ordinary streaming DMA from a device to
> > > > > user space ... that's what the dma_map_xx APIs are already designed to
> > > > > handle:  I don't understand why you need coherent memory for this (which
> > > > > can be a scarce resource on some platforms).
> > > 
> > > > The streaming APIs are inefficient for this.  Consider the overhead of
> > > > having to writeback and invalidate caches at 200KB/s (which is what
> > > > you're requiring ARM to do).  That's far too much CPU overhead.
> > > 
> > > Um, but streaming APIs are what we use for all block and network I/O
> > > from user space ... we don't see huge performance problems running up to
> > > gigabits.  Even on parisc where we have to flush several times to make
> > > this happen we can get up to several hundred megabytes per second.
> > 
> > Well, the requirement of the non-streaming mmap is rather a historical
> > reason.  From the fairly early time, the sound driver provided the
> > mmap of a whole ring buffer since it's practical and efficient for
> > real-time sound processes.  But, usually the mmap mode is optional.
> > If it's not supported, apps should fall back to the normal read/write
> > mode.
> 
> But what I don't understand is why you can't treat the ring buffer as
> streaming.

We could, if we want.  The problem is only that the existing sound
mmap API since over 10 years ago requires the whole ring-buffer to be
exposed, to be accessible without sync operation.

>  Sure, you have a head pointer and a tail pointer, but
> everything between head and tail is owned by the device and everything
> between tail and head is owned by the kernel, surely?  In that case, you
> can manipulate head and tail movements simply via the streaming API?
> (when head moves, map the delta to the device; when tail moves, map the
> delta to the kernel) and we can simply then use the standard APIs?

That's a good thing for the future :)

> > Now, the problem is that we have no way to tell whether the DMA
> > ring-buffer mmap is available or not on each architecture.  So, my
> > proposal has basically two meanings:
> > 
> > - clarify which arch / platform supports the coherent DMA mapping
> >   of a whole ring-buffer
> > - provide the same API to cover the possible archs / platforms
> > 
> > In that sense, if a few (or all) PA-RISC platforms don't give the
> > proper coherent mapping that the sound apps require, it's OK.  We
> > just drop the flag in the driver as "unsupported", then.
> 
> That would work for us too ... I can see that x86 will have little
> problem with this, but I can predict that most other architectures will
> have some difficulty.

Yes, appears like so...

> > So... before going to the detail of PA-RISC implementation, I'd like
> > to know your opinions: whether applying dma_mmap_coherent() to
> > possible archs/platforms is a reasonable solution for such a
> > scenario.
> 
> Realistically, the only way to make it work as you want on parisc is to
> have a kernel/user congruence (it's an address rule [virtual addresses
> are equal modulo the congruence step, which is 4MB]) which would ensure
> this would work without flushing ... that's hard for the kernel to make
> happen on its own.

Hmm.  So, maybe enabling dma_mmap_coherent() for PA-RISC isn't worth,
at least, for the sound device mmap.

> > [Why not using the standard map->sync procedure is another level of
> >  question :)  My goal here is to improve the current (partly broken)
> >  situation in a minimal effort.  The change of mmap procedure would
> >  lead to major rewrites of API, and thus has to be discussed more
> >  deeply.]
> 
> So, yes, there is an equivalent used by the frame buffer which basically
> tries to place a dma mapped page into a user address space ... that
> might also work in this case.

Sorry for ignorance, but doesn't fb-mmap work in a way like the sound
apps expect?  Or does it require any map->sync operation?


thanks,

Takashi