[1/4] ALSA: memalloc: Allow NULL device for SNDRV_DMA_TYPE_CONTINOUS type
diff mbox series

Message ID 20191105080138.1260-2-tiwai@suse.de
State New
Headers show
Series
  • ALSA: enhancement / cleanup on memalloc stuff
Related show

Commit Message

Takashi Iwai Nov. 5, 2019, 8:01 a.m. UTC
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(-)

Comments

Ranjani Sridharan Nov. 5, 2019, 5:58 p.m. UTC | #1
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
Takashi Iwai Nov. 5, 2019, 6:10 p.m. UTC | #2
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
Sridharan, Ranjani Nov. 5, 2019, 6:17 p.m. UTC | #3
>
> > >
> > 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
Amadeusz Sławiński Nov. 5, 2019, 6:28 p.m. UTC | #4
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
Takashi Iwai Nov. 5, 2019, 6:46 p.m. UTC | #5
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
Takashi Iwai Nov. 5, 2019, 6:47 p.m. UTC | #6
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

Patch
diff mbox series

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