[alsa-devel] [PATCH 0/4] ALSA: enhancement / cleanup on memalloc stuff

[alsa-devel] [PATCH 0/4] ALSA: enhancement / cleanup on memalloc stuff

[Takashi Iwai]

Hi,

this is a small series of patches to enhance / clean up the core
memory allocation stuff.  The basic changes are:

- The memalloc code accepts NULL device pointer to treat as the
  default mode for the continuous pages
- The new SNDRV_DMA_TYPE_VMALLOC type in the core allocator, so that
  we can drop the PCM-specific helpers
- The PCM mmap default handler checks the buffer type, and the PCM
  page ops can be dropped in almost all cases.

These whole core changes are still compatible with the old code.

The actual cleanup patch for each driver will be posted later once
when this core change set is accepted.


thanks,

Takashi

===

Takashi Iwai (4):
  ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type
  ALSA: memalloc: Add vmalloc buffer allocation support
  ALSA: pcm: Handle special page mapping in the default mmap handler
  ALSA: docs: Update documentation about SG- and vmalloc-buffers

 .../sound/kernel-api/writing-an-alsa-driver.rst    | 80 ++++++++++++----------
 include/sound/memalloc.h                           |  1 +
 sound/core/memalloc.c                              | 23 ++++++-
 sound/core/pcm_native.c                            | 14 +++-
 4 files changed, 80 insertions(+), 38 deletions(-)

-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Takashi Iwai]

Currently we pass the artificial device pointer to the allocation
helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
flags.  But all common cases are the allocations with GFP_KERNEL, and
it's messy to put this in each place.

In this patch, the memalloc core helper is changed to accept the NULL
device pointer and it treats as the default mode, GFP_KERNEL, so that
all callers can omit the complex argument but just leave NULL.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14 ++++++++------
 sound/core/memalloc.c                                     |  9 ++++++++-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 132f5eb9b530..5385618fd881 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -3523,12 +3523,14 @@ The second argument (type) and the third argument (device pointer) are
 dependent on the bus. For normal devices, pass the device pointer
 (typically identical as ``card->dev``) to the third argument with
 ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to the
-bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type and the
-``snd_dma_continuous_data(GFP_KERNEL)`` device pointer, where
-``GFP_KERNEL`` is the kernel allocation flag to use. For the
-scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the device
-pointer (see the `Non-Contiguous Buffers`_
-section).
+bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type.
+You can pass NULL to the device pointer in that case, which is the
+default mode implying to allocate with ``GFP_KRENEL`` flag.
+If you need a different GFP flag, you can pass it by encoding the flag
+into the device pointer via a special macro
+:c:func:`snd_dma_continuous_data()`.
+For the scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the
+device pointer (see the `Non-Contiguous Buffers`_ section).
 
 Once the buffer is pre-allocated, you can use the allocator in the
 ``hw_params`` callback:
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 6850d13aa98c..e56f84fbd659 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -99,6 +99,13 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab)
  *
  */
 
+static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
+{
+	if (!dev)
+		return GFP_KRENEL;
+	else
+		return (__force gfp_t)(unsigned long)dev;
+}
 
 /**
  * snd_dma_alloc_pages - allocate the buffer area according to the given type
@@ -129,7 +136,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 	switch (type) {
 	case SNDRV_DMA_TYPE_CONTINUOUS:
 		dmab->area = alloc_pages_exact(size,
-					       (__force gfp_t)(unsigned long)device);
+					       snd_mem_get_gfp_flags(device));
 		dmab->addr = 0;
 		break;
 #ifdef CONFIG_HAS_DMA
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 2/4] ALSA: memalloc: Add vmalloc buffer allocation support

[Takashi Iwai]

This patch adds the vmalloc buffer support to ALSA memalloc core.  A
new type, SNDRV_DMA_TYPE_VMALLOC was added.

The vmalloc buffer has been already supported in the PCM via a few own
helper functions, but the user sometimes get confused and misuse
them.  With this patch, the whole buffer management is integrated into
the memalloc core, so they can be used in a sole common way.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/memalloc.h |  1 +
 sound/core/memalloc.c    | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h
index 240622d89c0b..6ada3b8ede4e 100644
--- a/include/sound/memalloc.h
+++ b/include/sound/memalloc.h
@@ -44,6 +44,7 @@ struct snd_dma_device {
 #else
 #define SNDRV_DMA_TYPE_DEV_IRAM	SNDRV_DMA_TYPE_DEV
 #endif
+#define SNDRV_DMA_TYPE_VMALLOC		7	/* vmalloc'ed buffer */
 
 /*
  * info for buffer allocation
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index e56f84fbd659..ac4cd94dbd66 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -10,6 +10,7 @@
 #include <linux/mm.h>
 #include <linux/dma-mapping.h>
 #include <linux/genalloc.h>
+#include <linux/vmalloc.h>
 #ifdef CONFIG_X86
 #include <asm/set_memory.h>
 #endif
@@ -99,10 +100,11 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab)
  *
  */
 
-static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
+static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev,
+					  gfp_t default_gfp)
 {
 	if (!dev)
-		return GFP_KRENEL;
+		return default_gfp;
 	else
 		return (__force gfp_t)(unsigned long)dev;
 }
@@ -123,6 +125,8 @@ static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
 int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 			struct snd_dma_buffer *dmab)
 {
+	gfp_t gfp;
+
 	if (WARN_ON(!size))
 		return -ENXIO;
 	if (WARN_ON(!dmab))
@@ -135,8 +139,13 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 	dmab->bytes = 0;
 	switch (type) {
 	case SNDRV_DMA_TYPE_CONTINUOUS:
-		dmab->area = alloc_pages_exact(size,
-					       snd_mem_get_gfp_flags(device));
+		gfp = snd_mem_get_gfp_flags(device, GFP_KERNEL);
+		dmab->area = alloc_pages_exact(size, gfp);
+		dmab->addr = 0;
+		break;
+	case SNDRV_DMA_TYPE_VMALLOC:
+		gfp = snd_mem_get_gfp_flags(device, GFP_KERNEL | __GFP_HIGHMEM);
+		dmab->area = __vmalloc(size, gfp, PAGE_KERNEL);
 		dmab->addr = 0;
 		break;
 #ifdef CONFIG_HAS_DMA
@@ -222,6 +231,9 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab)
 	case SNDRV_DMA_TYPE_CONTINUOUS:
 		free_pages_exact(dmab->area, dmab->bytes);
 		break;
+	case SNDRV_DMA_TYPE_VMALLOC:
+		vfree(dmab->area);
+		break;
 #ifdef CONFIG_HAS_DMA
 #ifdef CONFIG_GENERIC_ALLOCATOR
 	case SNDRV_DMA_TYPE_DEV_IRAM:
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 3/4] ALSA: pcm: Handle special page mapping in the default mmap handler

[Takashi Iwai]

When a driver needs to deal with a special buffer like a SG or a
vmalloc buffer, it has to set up the PCM page ops explicitly for the
corresponding helper function.  This is rather error-prone and many
people forgot or incorrectly used it.

For simplifying the call patterns and avoiding such a potential bug,
this patch enhances the PCM default mmap handler to check the
(pre-)allocated buffer type and handles the page gracefully depending
on the buffer type.  If the PCM page ops is given, the ops is still
used in a higher priority.  The new code path is only for the default
(NULL page ops) case.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c3a139436ac2..998c63192ae4 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -13,6 +13,7 @@
 #include <linux/pm_qos.h>
 #include <linux/io.h>
 #include <linux/dma-mapping.h>
+#include <linux/vmalloc.h>
 #include <sound/core.h>
 #include <sound/control.h>
 #include <sound/info.h>
@@ -3335,7 +3336,18 @@ static inline struct page *
 snd_pcm_default_page_ops(struct snd_pcm_substream *substream, unsigned long ofs)
 {
 	void *vaddr = substream->runtime->dma_area + ofs;
-	return virt_to_page(vaddr);
+
+	switch (substream->dma_buffer.dev.type) {
+#ifdef CONFIG_SND_DMA_SGBUF
+	case SNDRV_DMA_TYPE_DEV_SG:
+	case SNDRV_DMA_TYPE_DEV_UC_SG:
+		return snd_pcm_sgbuf_ops_page(substream, ofs);
+#endif /* CONFIG_SND_DMA_SGBUF */
+	case SNDRV_DMA_TYPE_VMALLOC:
+		return vmalloc_to_page(vaddr);
+	default:
+		return virt_to_page(vaddr);
+	}
 }
 
 /*
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 4/4] ALSA: docs: Update documentation about SG- and vmalloc-buffers

[Takashi Iwai]

The recent changes simplified the required setup for SG- and vmalloc-
buffers.  Update the documentation accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../sound/kernel-api/writing-an-alsa-driver.rst    | 66 ++++++++++++----------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 5385618fd881..ba008ce28029 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -2095,10 +2095,12 @@ This callback is atomic as default.
 page callback
 ~~~~~~~~~~~~~
 
-This callback is optional too. This callback is used mainly for
-non-contiguous buffers. The mmap calls this callback to get the page
-address. Some examples will be explained in the later section `Buffer
-and Memory Management`_, too.
+This callback is optional too. The mmap calls this callback to get the
+page fault address.
+
+Since the recent changes, you need no special callback any longer for
+the standard SG-buffer or vmalloc-buffer. Hence this callback should
+be rarely used.
 
 mmap calllback
 ~~~~~~~~~~~~~~
@@ -3700,8 +3702,15 @@ For creating the SG-buffer handler, call
 ``SNDRV_DMA_TYPE_DEV_SG`` in the PCM constructor like other PCI
 pre-allocator. You need to pass ``snd_dma_pci_data(pci)``, where pci is
 the :c:type:`struct pci_dev <pci_dev>` pointer of the chip as
-well. The ``struct snd_sg_buf`` instance is created as
-``substream->dma_private``. You can cast the pointer like:
+well.
+
+::
+
+  snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV_SG,
+                                        snd_dma_pci_data(pci), size, max);
+
+The ``struct snd_sg_buf`` instance is created as
+``substream->dma_private`` in turn. You can cast the pointer like:
 
 ::
 
@@ -3717,10 +3726,6 @@ physically non-contiguous. The physical address table is set up in
 ``sgbuf->table``. You can get the physical address at a certain offset
 via :c:func:`snd_pcm_sgbuf_get_addr()`.
 
-When a SG-handler is used, you need to set
-:c:func:`snd_pcm_sgbuf_ops_page()` as the ``page`` callback. (See
-`page callback`_ section.)
-
 To release the data, call :c:func:`snd_pcm_lib_free_pages()` in
 the ``hw_free`` callback as usual.
 
@@ -3728,30 +3733,33 @@ Vmalloc'ed Buffers
 ------------------
 
 It's possible to use a buffer allocated via :c:func:`vmalloc()`, for
-example, for an intermediate buffer. Since the allocated pages are not
-contiguous, you need to set the ``page`` callback to obtain the physical
-address at every offset.
+example, for an intermediate buffer. In the recent version of kernel,
+you can simply allocate it via standard
+:c:func:`snd_pcm_lib_malloc_pages()` and co after setting up the
+buffer preallocation with ``SNDRV_DMA_TYPE_VMALLOC`` type.
 
-The easiest way to achieve it would be to use
-:c:func:`snd_pcm_lib_alloc_vmalloc_buffer()` for allocating the buffer
-via :c:func:`vmalloc()`, and set :c:func:`snd_pcm_sgbuf_ops_page()` to
-the ``page`` callback.  At release, you need to call
-:c:func:`snd_pcm_lib_free_vmalloc_buffer()`.
+::
 
-If you want to implementation the ``page`` manually, it would be like
-this:
+  snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_VMALLOC,
+                                        NULL, 0, 0);
 
-::
+The NULL is passed to the device pointer argument, which indicates
+that the default pages (GFP_KERNEL and GFP_HIGHMEM) will be
+allocated.
 
-  #include <linux/vmalloc.h>
+Also, note that zero is passed to both the size and the max size
+arguments here.  Since each vmalloc call should succeed at any time,
+we don't need to pre-allocate the buffers like other continuous
+pages.
 
-  /* get the physical page pointer on the given offset */
-  static struct page *mychip_page(struct snd_pcm_substream *substream,
-                                  unsigned long offset)
-  {
-          void *pageptr = substream->runtime->dma_area + offset;
-          return vmalloc_to_page(pageptr);
-  }
+If you need the 32bit DMA allocation, pass the device pointer encoded
+by :c:func:`snd_dma_continuous_data()` with ``GFP_KERNEL|__GFP_DMA32``
+argument.
+
+::
+
+  snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_VMALLOC,
+          snd_dma_continuous_data(GFP_KERNEL | __GFP_DMA32), 0, 0);
 
 Proc Interface
 ==============
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Ranjani Sridharan]

On Tue, 2019-11-05 at 09:01 +0100, Takashi Iwai wrote:
> Currently we pass the artificial device pointer to the allocation
> helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
> flags.  But all common cases are the allocations with GFP_KERNEL, and
> it's messy to put this in each place.
> 
> In this patch, the memalloc core helper is changed to accept the NULL
> device pointer and it treats as the default mode, GFP_KERNEL, so that
> all callers can omit the complex argument but just leave NULL.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14
> ++++++++------
>  sound/core/memalloc.c                                     |  9
> ++++++++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/sound/kernel-api/writing-an-alsa-
> driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-
> driver.rst
> index 132f5eb9b530..5385618fd881 100644
> --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> @@ -3523,12 +3523,14 @@ The second argument (type) and the third
> argument (device pointer) are
>  dependent on the bus. For normal devices, pass the device pointer
>  (typically identical as ``card->dev``) to the third argument with
>  ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to
> 
Hi Takashi,

I have a question about the usage of snd_dma_alloc_pages() with the
SNDRV_DMA_TYPE_DEV type. I am working on adding a platform-device for
audio which is a child device of the top-level PCI device in SOF.
When I use the handle for the platform-device as the "dev" argument for
snd_dma_alloc_pages(), the dma alloc fails on some platforms (ex: Ice
Lake). But it works fine if I use the top-level PCI device instead.
Why would that be? Are there any restrictions to what devices can be
used for dma alloc?

Thanks,
Ranjani

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Takashi Iwai]

On Tue, 05 Nov 2019 18:58:53 +0100,
Ranjani Sridharan wrote:
> 
> On Tue, 2019-11-05 at 09:01 +0100, Takashi Iwai wrote:
> > Currently we pass the artificial device pointer to the allocation
> > helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
> > flags.  But all common cases are the allocations with GFP_KERNEL, and
> > it's messy to put this in each place.
> > 
> > In this patch, the memalloc core helper is changed to accept the NULL
> > device pointer and it treats as the default mode, GFP_KERNEL, so that
> > all callers can omit the complex argument but just leave NULL.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14
> > ++++++++------
> >  sound/core/memalloc.c                                     |  9
> > ++++++++-
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/sound/kernel-api/writing-an-alsa-
> > driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-
> > driver.rst
> > index 132f5eb9b530..5385618fd881 100644
> > --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> > +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> > @@ -3523,12 +3523,14 @@ The second argument (type) and the third
> > argument (device pointer) are
> >  dependent on the bus. For normal devices, pass the device pointer
> >  (typically identical as ``card->dev``) to the third argument with
> >  ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to
> > 
> Hi Takashi,
> 
> I have a question about the usage of snd_dma_alloc_pages() with the
> SNDRV_DMA_TYPE_DEV type. I am working on adding a platform-device for
> audio which is a child device of the top-level PCI device in SOF.
> When I use the handle for the platform-device as the "dev" argument for
> snd_dma_alloc_pages(), the dma alloc fails on some platforms (ex: Ice
> Lake). But it works fine if I use the top-level PCI device instead.
> Why would that be? Are there any restrictions to what devices can be
> used for dma alloc?

This pretty much depends on the device.  Basically the ALSA memalloc
stuff simply calls dma_alloc_coherent() if the buffer type is
SNDRV_DMA_TYPE_DEV, and the rest goes deeply into the code in
kernel/dma/*.

My wild guess is that the significant difference in your case is about
the DMA coherence mask set on the device.  As default the platform
device keeps 32bit DMA while the modern PCI drivers often sets up
64bit DMA mask.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Sridharan, Ranjani]

>
> > >
> > Hi Takashi,
> >
> > I have a question about the usage of snd_dma_alloc_pages() with the
> > SNDRV_DMA_TYPE_DEV type. I am working on adding a platform-device for
> > audio which is a child device of the top-level PCI device in SOF.
> > When I use the handle for the platform-device as the "dev" argument for
> > snd_dma_alloc_pages(), the dma alloc fails on some platforms (ex: Ice
> > Lake). But it works fine if I use the top-level PCI device instead.
> > Why would that be? Are there any restrictions to what devices can be
> > used for dma alloc?
>
> This pretty much depends on the device.  Basically the ALSA memalloc
> stuff simply calls dma_alloc_coherent() if the buffer type is
> SNDRV_DMA_TYPE_DEV, and the rest goes deeply into the code in
> kernel/dma/*.
>
> My wild guess is that the significant difference in your case is about
> the DMA coherence mask set on the device.  As default the platform
> device keeps 32bit DMA while the modern PCI drivers often sets up
> 64bit DMA mask.
>

Thanks, Takashi. So, in this case, would you recommend to always use the
PCI device for dma alloc?

Thanks,
Ranjani
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Amadeusz Sławiński]

On Tue,  5 Nov 2019 09:01:35 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> Currently we pass the artificial device pointer to the allocation
> helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
> flags.  But all common cases are the allocations with GFP_KERNEL, and
> it's messy to put this in each place.
> 
> In this patch, the memalloc core helper is changed to accept the NULL
> device pointer and it treats as the default mode, GFP_KERNEL, so that
> all callers can omit the complex argument but just leave NULL.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14 ++++++++------
>  sound/core/memalloc.c                                     |  9 ++++++++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> index 132f5eb9b530..5385618fd881 100644
> --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> @@ -3523,12 +3523,14 @@ The second argument (type) and the third argument (device pointer) are
>  dependent on the bus. For normal devices, pass the device pointer
>  (typically identical as ``card->dev``) to the third argument with
>  ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to the
> -bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type and the
> -``snd_dma_continuous_data(GFP_KERNEL)`` device pointer, where
> -``GFP_KERNEL`` is the kernel allocation flag to use. For the
> -scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the device
> -pointer (see the `Non-Contiguous Buffers`_
> -section).
> +bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type.
> +You can pass NULL to the device pointer in that case, which is the
> +default mode implying to allocate with ``GFP_KRENEL`` flag.
> +If you need a different GFP flag, you can pass it by encoding the flag
> +into the device pointer via a special macro
> +:c:func:`snd_dma_continuous_data()`.
> +For the scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the
> +device pointer (see the `Non-Contiguous Buffers`_ section).
>  
>  Once the buffer is pre-allocated, you can use the allocator in the
>  ``hw_params`` callback:
> diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
> index 6850d13aa98c..e56f84fbd659 100644
> --- a/sound/core/memalloc.c
> +++ b/sound/core/memalloc.c
> @@ -99,6 +99,13 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab)
>   *
>   */
>  
> +static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
> +{
> +	if (!dev)
> +		return GFP_KRENEL;

There is a typo, you remove it in next patch, but it may cause problem
with bisects.


> +	else
> +		return (__force gfp_t)(unsigned long)dev;
> +}
>  
>  /**
>   * snd_dma_alloc_pages - allocate the buffer area according to the given type
> @@ -129,7 +136,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
>  	switch (type) {
>  	case SNDRV_DMA_TYPE_CONTINUOUS:
>  		dmab->area = alloc_pages_exact(size,
> -					       (__force gfp_t)(unsigned long)device);
> +					       snd_mem_get_gfp_flags(device));
>  		dmab->addr = 0;
>  		break;
>  #ifdef CONFIG_HAS_DMA

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Takashi Iwai]

On Tue, 05 Nov 2019 19:17:40 +0100,
Sridharan, Ranjani wrote:
> 
>     > >
>     > Hi Takashi,
>     >
>     > I have a question about the usage of snd_dma_alloc_pages() with the
>     > SNDRV_DMA_TYPE_DEV type. I am working on adding a platform-device for
>     > audio which is a child device of the top-level PCI device in SOF.
>     > When I use the handle for the platform-device as the "dev" argument for
>     > snd_dma_alloc_pages(), the dma alloc fails on some platforms (ex: Ice
>     > Lake). But it works fine if I use the top-level PCI device instead.
>     > Why would that be? Are there any restrictions to what devices can be
>     > used for dma alloc?
>    
>     This pretty much depends on the device.  Basically the ALSA memalloc
>     stuff simply calls dma_alloc_coherent() if the buffer type is
>     SNDRV_DMA_TYPE_DEV, and the rest goes deeply into the code in
>     kernel/dma/*.
>    
>     My wild guess is that the significant difference in your case is about
>     the DMA coherence mask set on the device.  As default the platform
>     device keeps 32bit DMA while the modern PCI drivers often sets up
>     64bit DMA mask.
> 
> Thanks, Takashi. So, in this case, would you recommend to always use the PCI
> device for dma alloc?

Yes, if the PCI bus is used in the backend, using the PCI device is a
better choice.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type

[Takashi Iwai]

On Tue, 05 Nov 2019 19:28:44 +0100,
Amadeusz Sławiński wrote:
> 
> On Tue,  5 Nov 2019 09:01:35 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Currently we pass the artificial device pointer to the allocation
> > helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
> > flags.  But all common cases are the allocations with GFP_KERNEL, and
> > it's messy to put this in each place.
> > 
> > In this patch, the memalloc core helper is changed to accept the NULL
> > device pointer and it treats as the default mode, GFP_KERNEL, so that
> > all callers can omit the complex argument but just leave NULL.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14 ++++++++------
> >  sound/core/memalloc.c                                     |  9 ++++++++-
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> > index 132f5eb9b530..5385618fd881 100644
> > --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> > +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
> > @@ -3523,12 +3523,14 @@ The second argument (type) and the third argument (device pointer) are
> >  dependent on the bus. For normal devices, pass the device pointer
> >  (typically identical as ``card->dev``) to the third argument with
> >  ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to the
> > -bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type and the
> > -``snd_dma_continuous_data(GFP_KERNEL)`` device pointer, where
> > -``GFP_KERNEL`` is the kernel allocation flag to use. For the
> > -scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the device
> > -pointer (see the `Non-Contiguous Buffers`_
> > -section).
> > +bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type.
> > +You can pass NULL to the device pointer in that case, which is the
> > +default mode implying to allocate with ``GFP_KRENEL`` flag.
> > +If you need a different GFP flag, you can pass it by encoding the flag
> > +into the device pointer via a special macro
> > +:c:func:`snd_dma_continuous_data()`.
> > +For the scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the
> > +device pointer (see the `Non-Contiguous Buffers`_ section).
> >  
> >  Once the buffer is pre-allocated, you can use the allocator in the
> >  ``hw_params`` callback:
> > diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
> > index 6850d13aa98c..e56f84fbd659 100644
> > --- a/sound/core/memalloc.c
> > +++ b/sound/core/memalloc.c
> > @@ -99,6 +99,13 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab)
> >   *
> >   */
> >  
> > +static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
> > +{
> > +	if (!dev)
> > +		return GFP_KRENEL;
> 
> There is a typo, you remove it in next patch, but it may cause problem
> with bisects.

Indeed, will fix up.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH v2] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINUOUS type

[Takashi Iwai]

Currently we pass the artificial device pointer to the allocation
helper in the case of SNDRV_DMA_TYPE_CONTINUOUS for passing the GFP
flags.  But all common cases are the allocations with GFP_KERNEL, and
it's messy to put this in each place.

In this patch, the memalloc core helper is changed to accept the NULL
device pointer and it treats as the default mode, GFP_KERNEL, so that
all callers can omit the complex argument but just leave NULL.

Link: https://lore.kernel.org/r/20191105080138.1260-2-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: typo fixes, fix a remaining WARN_ON() for NULL device

 Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 14 ++++++++------
 sound/core/memalloc.c                                     | 11 ++++++++---
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 132f5eb9b530..5385618fd881 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -3523,12 +3523,14 @@ The second argument (type) and the third argument (device pointer) are
 dependent on the bus. For normal devices, pass the device pointer
 (typically identical as ``card->dev``) to the third argument with
 ``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to the
-bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type and the
-``snd_dma_continuous_data(GFP_KERNEL)`` device pointer, where
-``GFP_KERNEL`` is the kernel allocation flag to use. For the
-scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the device
-pointer (see the `Non-Contiguous Buffers`_
-section).
+bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type.
+You can pass NULL to the device pointer in that case, which is the
+default mode implying to allocate with ``GFP_KRENEL`` flag.
+If you need a different GFP flag, you can pass it by encoding the flag
+into the device pointer via a special macro
+:c:func:`snd_dma_continuous_data()`.
+For the scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the
+device pointer (see the `Non-Contiguous Buffers`_ section).
 
 Once the buffer is pre-allocated, you can use the allocator in the
 ``hw_params`` callback:
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 6850d13aa98c..1b1c7620cbda 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -99,6 +99,13 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab)
  *
  */
 
+static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev)
+{
+	if (!dev)
+		return GFP_KERNEL;
+	else
+		return (__force gfp_t)(unsigned long)dev;
+}
 
 /**
  * snd_dma_alloc_pages - allocate the buffer area according to the given type
@@ -120,8 +127,6 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 		return -ENXIO;
 	if (WARN_ON(!dmab))
 		return -ENXIO;
-	if (WARN_ON(!device))
-		return -EINVAL;
 
 	dmab->dev.type = type;
 	dmab->dev.dev = device;
@@ -129,7 +134,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 	switch (type) {
 	case SNDRV_DMA_TYPE_CONTINUOUS:
 		dmab->area = alloc_pages_exact(size,
-					       (__force gfp_t)(unsigned long)device);
+					       snd_mem_get_gfp_flags(device));
 		dmab->addr = 0;
 		break;
 #ifdef CONFIG_HAS_DMA
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel