[alsa-devel] [PATCH 0/8] ALSA: pcm: API cleanups and extensions

newer
[alsa-devel] [PATCH AUTOSEL 4.19...

older
[alsa-devel] alsa-lib Plugin: File...

Takashi Iwai

17 Nov 2019 17 Nov '19

8:53 a.m.

Hi,

this is a patch series for a few ALSA PCM API changes.

Basically the patch set introduces three changes:

* Add the "managed buffer allocation" mode; this allows many drivers to drop hw_params and hw_free callbacks that simply call snd_pcm_lib_malloc_pages() and *_free_pages().

* Make PCM ioctl ops optional; almost all driver can drop the lines to define pcm_ops.ioctl.

* The new sync_stop PCM ops and card->sync_irq; it's used to synchronize the pending task after stopping the stream, for fixing the use-after-free or such problem.

I planned originally merging these changes to 5.6. But since the API changes would influence on many drivers outside sound git tree, it might be easier to merge only these core changes at first during 5.5 merge window while keeping the rest (the actual driver implementations) intact -- that's why I post now for reviews. The changes are additional, and they won't break things by themselves. The drivers can / need to change the call patterns for following these new APIs.

thanks,

Takashi

===

Takashi Iwai (8): ALSA: pcm: Introduce managed buffer allocation mode ALSA: docs: Update for managed buffer allocation mode ALSA: pcm: Allow NULL ioctl ops ALSA: docs: Update document about the default PCM ioctl ops ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header ALSA: pcm: Add the support for sync-stop operation ALSA: pcm: Add card sync_irq field ALSA: docs: Update about the new PCM sync_stop ops

-- 2.16.4

Show replies by date

Takashi Iwai

17 Nov 17 Nov

8:53 a.m.

New subject: [alsa-devel] [PATCH 1/8] ALSA: pcm: Introduce managed buffer allocation mode

This patch adds the support for the feature to automatically allocate and free PCM buffers, so called "managed buffer allocation" mode. It's set up via new PCM helpers, snd_pcm_set_managed_buffer() and snd_pcm_set_managed_buffer_all(), both of which correspond to the existing preallocator helpers, snd_pcm_lib_preallocate_pages() and snd_pcm_lib_preallocate_pages_for_all(). When the new helper is used, it not only performs the pre-allocation of buffers, but also it manages to call snd_pcm_lib_malloc_pages() before the PCM hw_params ops and snd_lib_pcm_free() after the PCM hw_free ops inside PCM core, respectively. This allows drivers to drop the explicit calls of the memory allocation / release functions, and it will be a good amount of code reduction in the end of this patch series.

When the PCM substream is set to the managed buffer allocation mode, the managed_buffer_alloc flag is set in the substream object. Since some drivers want to know when a buffer is newly allocated or re-allocated at hw_params callback (e.g. want to set up the additional stuff for the given buffer only at allocation time), now PCM core turns on buffer_changed flag when the buffer has changed.

The standard conversions to use the new API will be straightforward: - Replace snd_pcm_lib_preallocate*() calls with the corresponding snd_pcm_set_managed_buffer*(); the arguments should be unchanged - Drop superfluous snd_pcm_lib_malloc() and snd_pcm_lib_free() calls; the check of snd_pcm_lib_malloc() returns should be replaced with the check of runtime->buffer_changed flag. - If hw_params or hw_free becomes empty, drop them from PCM ops

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 8 +++++ sound/core/pcm_memory.c | 83 +++++++++++++++++++++++++++++++++++++++++-------- sound/core/pcm_native.c | 12 +++++++ 3 files changed, 90 insertions(+), 13 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 2c0aa884f5f1..253d15c61ce2 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -414,6 +414,7 @@ struct snd_pcm_runtime { size_t dma_bytes; /* size of DMA area */

struct snd_dma_buffer *dma_buffer_p; /* allocated buffer */ + unsigned int buffer_changed:1; /* buffer allocation changed; set only in managed mode */

/* -- audio timestamp config -- */ struct snd_pcm_audio_tstamp_config audio_tstamp_config; @@ -475,6 +476,7 @@ struct snd_pcm_substream { #endif /* CONFIG_SND_VERBOSE_PROCFS */ /* misc flags */ unsigned int hw_opened: 1; + unsigned int managed_buffer_alloc:1; };

#define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0) @@ -1186,6 +1188,12 @@ void snd_pcm_lib_preallocate_pages_for_all(struct snd_pcm *pcm, int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size); int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream);

+void snd_pcm_set_managed_buffer(struct snd_pcm_substream *substream, int type, + struct device *data, size_t size, size_t max); +void snd_pcm_set_managed_buffer_all(struct snd_pcm *pcm, int type, + struct device *data, + size_t size, size_t max); + int _snd_pcm_lib_alloc_vmalloc_buffer(struct snd_pcm_substream *substream, size_t size, gfp_t gfp_flags); int snd_pcm_lib_free_vmalloc_buffer(struct snd_pcm_substream *substream); diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c index 286f333f8e4c..73b770db2372 100644 --- a/sound/core/pcm_memory.c +++ b/sound/core/pcm_memory.c @@ -193,9 +193,15 @@ static inline void preallocate_info_init(struct snd_pcm_substream *substream) /* * pre-allocate the buffer and create a proc file for the substream */ -static void snd_pcm_lib_preallocate_pages1(struct snd_pcm_substream *substream, - size_t size, size_t max) +static void preallocate_pages(struct snd_pcm_substream *substream, + int type, struct device *data, + size_t size, size_t max, bool managed) { + if (snd_BUG_ON(substream->dma_buffer.dev.type)) + return; + + substream->dma_buffer.dev.type = type; + substream->dma_buffer.dev.dev = data;

if (size > 0 && preallocate_dma && substream->number < maximum_substreams) preallocate_pcm_pages(substream, size); @@ -205,8 +211,23 @@ static void snd_pcm_lib_preallocate_pages1(struct snd_pcm_substream *substream, substream->dma_max = max; if (max > 0) preallocate_info_init(substream); + if (managed) + substream->managed_buffer_alloc = 1; }

+static void preallocate_pages_for_all(struct snd_pcm *pcm, int type, + void *data, size_t size, size_t max, + bool managed) +{ + struct snd_pcm_substream *substream; + int stream; + + for (stream = 0; stream < 2; stream++) + for (substream = pcm->streams[stream].substream; substream; + substream = substream->next) + preallocate_pages(substream, type, data, size, max, + managed); +}

/** * snd_pcm_lib_preallocate_pages - pre-allocation for the given DMA type @@ -222,11 +243,7 @@ void snd_pcm_lib_preallocate_pages(struct snd_pcm_substream *substream, int type, struct device *data, size_t size, size_t max) { - if (snd_BUG_ON(substream->dma_buffer.dev.type)) - return; - substream->dma_buffer.dev.type = type; - substream->dma_buffer.dev.dev = data; - snd_pcm_lib_preallocate_pages1(substream, size, max); + preallocate_pages(substream, type, data, size, max, false); } EXPORT_SYMBOL(snd_pcm_lib_preallocate_pages);

@@ -245,15 +262,55 @@ void snd_pcm_lib_preallocate_pages_for_all(struct snd_pcm *pcm, int type, void *data, size_t size, size_t max) { - struct snd_pcm_substream *substream; - int stream; - - for (stream = 0; stream < 2; stream++) - for (substream = pcm->streams[stream].substream; substream; substream = substream->next) - snd_pcm_lib_preallocate_pages(substream, type, data, size, max); + preallocate_pages_for_all(pcm, type, data, size, max, false); } EXPORT_SYMBOL(snd_pcm_lib_preallocate_pages_for_all);

+/** + * snd_pcm_set_managed_buffer - set up buffer management for a substream + * @substream: the pcm substream instance + * @type: DMA type (SNDRV_DMA_TYPE_*) + * @data: DMA type dependent data + * @size: the requested pre-allocation size in bytes + * @max: the max. allowed pre-allocation size + * + * Do pre-allocation for the given DMA buffer type, and set the managed + * buffer allocation mode to the given substream. + * In this mode, PCM core will allocate a buffer automatically before PCM + * hw_params ops call, and release the buffer after PCM hw_free ops call + * as well, so that the driver doesn't need to invoke the allocation and + * the release explicitly in its callback. + * When a buffer is actually allocated before the PCM hw_params call, it + * turns on the runtime buffer_changed flag for drivers changing their h/w + * parameters accordingly. + */ +void snd_pcm_set_managed_buffer(struct snd_pcm_substream *substream, int type, + struct device *data, size_t size, size_t max) +{ + preallocate_pages(substream, type, data, size, max, true); +} +EXPORT_SYMBOL(snd_pcm_set_managed_buffer); + +/** + * snd_pcm_set_managed_buffer_all - set up buffer management for all substreams + * for all substreams + * @pcm: the pcm instance + * @type: DMA type (SNDRV_DMA_TYPE_*) + * @data: DMA type dependent data + * @size: the requested pre-allocation size in bytes + * @max: the max. allowed pre-allocation size + * + * Do pre-allocation to all substreams of the given pcm for the specified DMA + * type and size, and set the managed_buffer_alloc flag to each substream. + */ +void snd_pcm_set_managed_buffer_all(struct snd_pcm *pcm, int type, + struct device *data, + size_t size, size_t max) +{ + preallocate_pages_for_all(pcm, type, data, size, max, true); +} +EXPORT_SYMBOL(snd_pcm_set_managed_buffer_all); + #ifdef CONFIG_SND_DMA_SGBUF /* * snd_pcm_sgbuf_ops_page - get the page struct at the given offset diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0c27009dc3df..f1646735bde6 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -662,6 +662,14 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if (err < 0) goto _error;

+ if (substream->managed_buffer_alloc) { + err = snd_pcm_lib_malloc_pages(substream, + params_buffer_bytes(params)); + if (err < 0) + goto _error; + runtime->buffer_changed = err > 0; + } + if (substream->ops->hw_params != NULL) { err = substream->ops->hw_params(substream, params); if (err < 0) @@ -723,6 +731,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); if (substream->ops->hw_free != NULL) substream->ops->hw_free(substream); + if (substream->managed_buffer_alloc) + snd_pcm_lib_free_pages(substream); return err; }

@@ -769,6 +779,8 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) return -EBADFD; if (substream->ops->hw_free) result = substream->ops->hw_free(substream); + if (substream->managed_buffer_alloc) + snd_pcm_lib_free_pages(substream); snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); pm_qos_remove_request(&substream->latency_pm_qos_req); return result;

-- 2.16.4

Pierre-Louis Bossart

18 Nov 18 Nov

4:24 p.m.

New subject: [alsa-devel] [PATCH 1/8] ALSA: pcm: Introduce managed buffer allocation mode

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...

This patch adds the support for the feature to automatically allocate and free PCM buffers, so called "managed buffer allocation" mode. It's set up via new PCM helpers, snd_pcm_set_managed_buffer() and snd_pcm_set_managed_buffer_all(), both of which correspond to the existing preallocator helpers, snd_pcm_lib_preallocate_pages() and snd_pcm_lib_preallocate_pages_for_all(). When the new helper is used, it not only performs the pre-allocation of buffers, but also it manages to call snd_pcm_lib_malloc_pages() before the PCM hw_params ops and snd_lib_pcm_free() after the PCM hw_free ops inside PCM core, respectively. This allows drivers to drop the explicit calls of the memory allocation / release functions, and it will be a good amount of code reduction in the end of this patch series.

When the PCM substream is set to the managed buffer allocation mode, the managed_buffer_alloc flag is set in the substream object. Since some drivers want to know when a buffer is newly allocated or re-allocated at hw_params callback (e.g. want to set up the additional stuff for the given buffer only at allocation time), now PCM core turns on buffer_changed flag when the buffer has changed.

I am a bit lost on the directions: a) is this introducing a new API that will eventually replace the existing one? b) or are we going to have two options, managed and non-managed buffers? in this case, what would drive an implementer to keep using non-managed buffers? It'd be useful to provide examples.

In the cover letter, the wording 'almost all drivers' is used, which leads be to think option b) it is, but the exceptions are not clear to me.

...

The standard conversions to use the new API will be straightforward:

Replace snd_pcm_lib_preallocate*() calls with the corresponding snd_pcm_set_managed_buffer*(); the arguments should be unchanged

Drop superfluous snd_pcm_lib_malloc() and snd_pcm_lib_free() calls; the check of snd_pcm_lib_malloc() returns should be replaced with the check of runtime->buffer_changed flag.

If hw_params or hw_free becomes empty, drop them from PCM ops

Signed-off-by: Takashi Iwai tiwai@suse.de

include/sound/pcm.h | 8 +++++ sound/core/pcm_memory.c | 83 +++++++++++++++++++++++++++++++++++++++++-------- sound/core/pcm_native.c | 12 +++++++ 3 files changed, 90 insertions(+), 13 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 2c0aa884f5f1..253d15c61ce2 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -414,6 +414,7 @@ struct snd_pcm_runtime { size_t dma_bytes; /* size of DMA area */

struct snd_dma_buffer *dma_buffer_p; /* allocated buffer */

unsigned int buffer_changed:1; /* buffer allocation changed; set only in managed mode */

/* -- audio timestamp config -- */ struct snd_pcm_audio_tstamp_config audio_tstamp_config;

@@ -475,6 +476,7 @@ struct snd_pcm_substream { #endif /* CONFIG_SND_VERBOSE_PROCFS */ /* misc flags */ unsigned int hw_opened: 1;

unsigned int managed_buffer_alloc:1; };

#define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)

@@ -1186,6 +1188,12 @@ void snd_pcm_lib_preallocate_pages_for_all(struct snd_pcm *pcm, int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size); int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream);

+void snd_pcm_set_managed_buffer(struct snd_pcm_substream *substream, int type,
		struct device *data, size_t size, size_t max);
+void snd_pcm_set_managed_buffer_all(struct snd_pcm *pcm, int type,
		    struct device *data,
		    size_t size, size_t max);
int _snd_pcm_lib_alloc_vmalloc_buffer(struct snd_pcm_substream *substream, size_t size, gfp_t gfp_flags); int snd_pcm_lib_free_vmalloc_buffer(struct snd_pcm_substream *substream);
diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c index 286f333f8e4c..73b770db2372 100644 --- a/sound/core/pcm_memory.c +++ b/sound/core/pcm_memory.c @@ -193,9 +193,15 @@ static inline void preallocate_info_init(struct snd_pcm_substream *substream) /*

pre-allocate the buffer and create a proc file for the substream

*/ -static void snd_pcm_lib_preallocate_pages1(struct snd_pcm_substream *substream,
			  size_t size, size_t max)
+static void preallocate_pages(struct snd_pcm_substream *substream,
	      int type, struct device *data,
	      size_t size, size_t max, bool managed)
{
if (snd_BUG_ON(substream->dma_buffer.dev.type))
return;
substream->dma_buffer.dev.type = type;

substream->dma_buffer.dev.dev = data;

if (size > 0 && preallocate_dma && substream->number < maximum_substreams) preallocate_pcm_pages(substream, size);
@@ -205,8 +211,23 @@ static void snd_pcm_lib_preallocate_pages1(struct snd_pcm_substream *substream, substream->dma_max = max; if (max > 0) preallocate_info_init(substream);
if (managed)
substream->managed_buffer_alloc = 1;
}
+static void preallocate_pages_for_all(struct snd_pcm *pcm, int type,
		      void *data, size_t size, size_t max,
		      bool managed)
+{
struct snd_pcm_substream *substream;

int stream;

for (stream = 0; stream < 2; stream++)
for (substream = pcm->streams[stream].substream; substream;
     substream = substream->next)
	preallocate_pages(substream, type, data, size, max,
			  managed);
+}

/**

snd_pcm_lib_preallocate_pages - pre-allocation for the given DMA type

@@ -222,11 +243,7 @@ void snd_pcm_lib_preallocate_pages(struct snd_pcm_substream *substream, int type, struct device *data, size_t size, size_t max) {
if (snd_BUG_ON(substream->dma_buffer.dev.type))
return;
substream->dma_buffer.dev.type = type;

substream->dma_buffer.dev.dev = data;

snd_pcm_lib_preallocate_pages1(substream, size, max);
preallocate_pages(substream, type, data, size, max, false); } EXPORT_SYMBOL(snd_pcm_lib_preallocate_pages);

@@ -245,15 +262,55 @@ void snd_pcm_lib_preallocate_pages_for_all(struct snd_pcm *pcm, int type, void *data, size_t size, size_t max) {
struct snd_pcm_substream *substream;

int stream;

for (stream = 0; stream < 2; stream++)
for (substream = pcm->streams[stream].substream; substream; substream = substream->next)
	snd_pcm_lib_preallocate_pages(substream, type, data, size, max);
preallocate_pages_for_all(pcm, type, data, size, max, false); } EXPORT_SYMBOL(snd_pcm_lib_preallocate_pages_for_all);

+/**

snd_pcm_set_managed_buffer - set up buffer management for a substream

@substream: the pcm substream instance

@type: DMA type (SNDRV_DMA_TYPE_*)

@data: DMA type dependent data

@size: the requested pre-allocation size in bytes

@max: the max. allowed pre-allocation size

Do pre-allocation for the given DMA buffer type, and set the managed

buffer allocation mode to the given substream.

In this mode, PCM core will allocate a buffer automatically before PCM

hw_params ops call, and release the buffer after PCM hw_free ops call

as well, so that the driver doesn't need to invoke the allocation and

the release explicitly in its callback.

When a buffer is actually allocated before the PCM hw_params call, it

turns on the runtime buffer_changed flag for drivers changing their h/w

parameters accordingly.

*/

+void snd_pcm_set_managed_buffer(struct snd_pcm_substream *substream, int type,
		struct device *data, size_t size, size_t max)
+{

preallocate_pages(substream, type, data, size, max, true);

+} +EXPORT_SYMBOL(snd_pcm_set_managed_buffer);

+/**

snd_pcm_set_managed_buffer_all - set up buffer management for all substreams

for all substreams

@pcm: the pcm instance

@type: DMA type (SNDRV_DMA_TYPE_*)

@data: DMA type dependent data

@size: the requested pre-allocation size in bytes

@max: the max. allowed pre-allocation size

Do pre-allocation to all substreams of the given pcm for the specified DMA

type and size, and set the managed_buffer_alloc flag to each substream.

*/

+void snd_pcm_set_managed_buffer_all(struct snd_pcm *pcm, int type,
		    struct device *data,
		    size_t size, size_t max)
+{

preallocate_pages_for_all(pcm, type, data, size, max, true);

+} +EXPORT_SYMBOL(snd_pcm_set_managed_buffer_all);

#ifdef CONFIG_SND_DMA_SGBUF /*

snd_pcm_sgbuf_ops_page - get the page struct at the given offset

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0c27009dc3df..f1646735bde6 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -662,6 +662,14 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if (err < 0) goto _error;
if (substream->managed_buffer_alloc) {
err = snd_pcm_lib_malloc_pages(substream,
			       params_buffer_bytes(params));
if (err < 0)
	goto _error;
runtime->buffer_changed = err > 0;
}

if (substream->ops->hw_params != NULL) { err = substream->ops->hw_params(substream, params); if (err < 0)
@@ -723,6 +731,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); if (substream->ops->hw_free != NULL) substream->ops->hw_free(substream);
if (substream->managed_buffer_alloc)
snd_pcm_lib_free_pages(substream);
return err; }
@@ -769,6 +779,8 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) return -EBADFD; if (substream->ops->hw_free) result = substream->ops->hw_free(substream);
if (substream->managed_buffer_alloc)
snd_pcm_lib_free_pages(substream);
snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN); pm_qos_remove_request(&substream->latency_pm_qos_req); return result;

Takashi Iwai

6:46 p.m.

New subject: [alsa-devel] [PATCH 1/8] ALSA: pcm: Introduce managed buffer allocation mode

On Mon, 18 Nov 2019 17:24:45 +0100, Pierre-Louis Bossart wrote:

...

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...
This patch adds the support for the feature to automatically allocate and free PCM buffers, so called "managed buffer allocation" mode. It's set up via new PCM helpers, snd_pcm_set_managed_buffer() and snd_pcm_set_managed_buffer_all(), both of which correspond to the existing preallocator helpers, snd_pcm_lib_preallocate_pages() and snd_pcm_lib_preallocate_pages_for_all(). When the new helper is used, it not only performs the pre-allocation of buffers, but also it manages to call snd_pcm_lib_malloc_pages() before the PCM hw_params ops and snd_lib_pcm_free() after the PCM hw_free ops inside PCM core, respectively. This allows drivers to drop the explicit calls of the memory allocation / release functions, and it will be a good amount of code reduction in the end of this patch series.

When the PCM substream is set to the managed buffer allocation mode, the managed_buffer_alloc flag is set in the substream object. Since some drivers want to know when a buffer is newly allocated or re-allocated at hw_params callback (e.g. want to set up the additional stuff for the given buffer only at allocation time), now PCM core turns on buffer_changed flag when the buffer has changed.

I am a bit lost on the directions: a) is this introducing a new API that will eventually replace the existing one? b) or are we going to have two options, managed and non-managed buffers? in this case, what would drive an implementer to keep using non-managed buffers? It'd be useful to provide examples.

In the cover letter, the wording 'almost all drivers' is used, which leads be to think option b) it is, but the exceptions are not clear to me.

Yes, it's currently it's (b), the new API is completely optional and the driver may keep the old API. Actually in my upcoming patchset (see below), only three drivers are left with the old API due to the special buffer handling and all the rest are replaced with the new API.

You can find the conversion results in topic/pcm-managed branch. As seen in the patch diff below, it's a long series.

thanks,

Takashi

Takashi Iwai

17 Nov 17 Nov

8:53 a.m.

New subject: [alsa-devel] [PATCH 2/8] ALSA: docs: Update for managed buffer allocation mode

Update the documentation for the newly introduced managed buffer allocation mode accordingly. The old preallocation is no longer recommended.

Signed-off-by: Takashi Iwai tiwai@suse.de --- .../sound/kernel-api/writing-an-alsa-driver.rst | 99 ++++++++++++++-------- 1 file changed, 64 insertions(+), 35 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index dcb7940435d9..1086b35db2af 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -1270,21 +1270,23 @@ shows only the skeleton, how to build up the PCM interfaces. /* the hardware-specific codes will be here */ .... return 0; - }

/* hw_params callback */ static int snd_mychip_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *hw_params) { - return snd_pcm_lib_malloc_pages(substream, - params_buffer_bytes(hw_params)); + /* the hardware-specific codes will be here */ + .... + return 0; }

/* hw_free callback */ static int snd_mychip_pcm_hw_free(struct snd_pcm_substream *substream) { - return snd_pcm_lib_free_pages(substream); + /* the hardware-specific codes will be here */ + .... + return 0; }

/* prepare callback */ @@ -1382,9 +1384,9 @@ shows only the skeleton, how to build up the PCM interfaces. &snd_mychip_capture_ops); /* pre-allocation of buffers */ /* NOTE: this may fail */ - snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - &chip->pci->dev, - 64*1024, 64*1024); + snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV, + &chip->pci->dev, + 64*1024, 64*1024); return 0; }

@@ -1465,13 +1467,14 @@ The operators are defined typically like this: All the callbacks are described in the Operators_ subsection.

After setting the operators, you probably will want to pre-allocate the -buffer. For the pre-allocation, simply call the following: +buffer and set up the managed allocation mode. +For that, simply call the following:

- snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, - &chip->pci->dev, - 64*1024, 64*1024); + snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV, + &chip->pci->dev, + 64*1024, 64*1024);

It will allocate a buffer up to 64kB as default. Buffer management details will be described in the later section `Buffer and Memory @@ -1621,8 +1624,7 @@ For the operators (callbacks) of each sound driver, most of these records are supposed to be read-only. Only the PCM middle-layer changes / updates them. The exceptions are the hardware description (hw) DMA buffer information and the private data. Besides, if you use the -standard buffer allocation method via -:c:func:`snd_pcm_lib_malloc_pages()`, you don't need to set the +standard managed buffer allocation mode, you don't need to set the DMA buffer information by yourself.

In the sections below, important records are explained. @@ -1776,8 +1778,8 @@ the physical address of the buffer. This field is specified only when the buffer is a linear buffer. ``dma_bytes`` holds the size of buffer in bytes. ``dma_private`` is used for the ALSA DMA allocator.

-If you use a standard ALSA function, -:c:func:`snd_pcm_lib_malloc_pages()`, for allocating the buffer, +If you use either the managed buffer allocation mode or the standard +API function :c:func:`snd_pcm_lib_malloc_pages()` for allocating the buffer, these fields are set by the ALSA middle layer, and you should *not* change them by yourself. You can read them but not write them. On the other hand, if you want to allocate the buffer by yourself, you'll @@ -1929,8 +1931,12 @@ Many hardware setups should be done in this callback, including the allocation of buffers.

Parameters to be initialized are retrieved by -:c:func:`params_xxx()` macros. To allocate buffer, you can call a -helper function, +:c:func:`params_xxx()` macros. + +When you set up the managed buffer allocation mode for the substream, +a buffer is already allocated before this callback gets +called. Alternatively, you can call a helper function below for +allocating the buffer, too.

@@ -1964,18 +1970,23 @@ hw_free callback static int snd_xxx_hw_free(struct snd_pcm_substream *substream);

This is called to release the resources allocated via -``hw_params``. For example, releasing the buffer via -:c:func:`snd_pcm_lib_malloc_pages()` is done by calling the -following: - -:: - - snd_pcm_lib_free_pages(substream); +``hw_params``.

This function is always called before the close callback is called. Also, the callback may be called multiple times, too. Keep track whether the resource was already released.

+When you have set up the managed buffer allocation mode for the PCM +substream, the allocated PCM buffer will be automatically released +after this callback gets called. Otherwise you'll have to release the +buffer manually. Typically, when the buffer was allocated from the +pre-allocated pool, you can use the standard API function +:c:func:`snd_pcm_lib_malloc_pages()` like: + +:: + + snd_pcm_lib_free_pages(substream); + prepare callback ~~~~~~~~~~~~~~~~

@@ -3543,6 +3554,25 @@ Once the buffer is pre-allocated, you can use the allocator in the

Note that you have to pre-allocate to use this function.

+Most of drivers use, though, rather the newly introduced "managed +buffer allocation mode" instead of the manual allocation or release. +This is done by calling :c:func:`snd_pcm_set_managed_buffer_all()` +instead of :c:func:`snd_pcm_lib_preallocate_pages_for_all()`. + +:: + + snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV, + &pci->dev, size, max); + +where passed arguments are identical in both functions. +The difference in the managed mode is that PCM core will call +:c:func:`snd_pcm_lib_malloc_pages()` internally already before calling +the PCM ``hw_params`` callback, and call :c:func:`snd_pcm_lib_free_pages()` +after the PCM ``hw_free`` callback automatically. So the driver +doesn't have to call these functions explicitly in its callback any +longer. This made many driver code having NULL ``hw_params`` and +``hw_free`` entries. + External Hardware Buffers -------------------------

@@ -3697,8 +3727,8 @@ provides an interface for handling SG-buffers. The API is provided in ``<sound/pcm.h>``.

For creating the SG-buffer handler, call -:c:func:`snd_pcm_lib_preallocate_pages()` or -:c:func:`snd_pcm_lib_preallocate_pages_for_all()` with +:c:func:`snd_pcm_set_managed_buffer()` or +:c:func:`snd_pcm_set_managed_buffer_all()` with ``SNDRV_DMA_TYPE_DEV_SG`` in the PCM constructor like other PCI pre-allocator. You need to pass ``&pci->dev``, where pci is the :c:type:`struct pci_dev <pci_dev>` pointer of the chip as @@ -3706,8 +3736,8 @@ well.

- snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV_SG, - &pci->dev, size, max); + snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV_SG, + &pci->dev, size, max);

The ``struct snd_sg_buf`` instance is created as ``substream->dma_private`` in turn. You can cast the pointer like: @@ -3716,8 +3746,7 @@ The ``struct snd_sg_buf`` instance is created as

struct snd_sg_buf *sgbuf = (struct snd_sg_buf *)substream->dma_private;

-Then call :c:func:`snd_pcm_lib_malloc_pages()` in the ``hw_params`` -callback as well as in the case of normal PCI buffer. The SG-buffer +Then in :c:func:`snd_pcm_lib_malloc_pages()` call, the common SG-buffer handler will allocate the non-contiguous kernel pages of the given size and map them onto the virtually contiguous memory. The virtual pointer is addressed in runtime->dma_area. The physical address @@ -3726,8 +3755,8 @@ 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()`.

-To release the data, call :c:func:`snd_pcm_lib_free_pages()` in -the ``hw_free`` callback as usual. +If you need to release the SG-buffer data explicitly, call the +standard API function :c:func:`snd_pcm_lib_free_pages()` as usual.

Vmalloc'ed Buffers ------------------ @@ -3740,8 +3769,8 @@ buffer preallocation with ``SNDRV_DMA_TYPE_VMALLOC`` type.

- snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_VMALLOC, - NULL, 0, 0); + snd_pcm_set_managed_buffer_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 @@ -3758,7 +3787,7 @@ argument.

- snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_VMALLOC, + snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_VMALLOC, snd_dma_continuous_data(GFP_KERNEL | __GFP_DMA32), 0, 0);

Proc Interface

-- 2.16.4

Takashi Iwai

8:53 a.m.

New subject: [alsa-devel] [PATCH 3/8] ALSA: pcm: Allow NULL ioctl ops

Currently PCM ioctl ops is a mandatory field but almost all drivers simply pass snd_pcm_lib_ioctl. For simplicity, allow to set NULL in the field and call snd_pcm_lib_ioctl() as default.

Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index f1646735bde6..704ea75199e4 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -178,6 +178,16 @@ void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream, } EXPORT_SYMBOL_GPL(snd_pcm_stream_unlock_irqrestore);

+/* Run PCM ioctl ops */ +static int snd_pcm_ops_ioctl(struct snd_pcm_substream *substream, + unsigned cmd, void *arg) +{ + if (substream->ops->ioctl) + return substream->ops->ioctl(substream, cmd, arg); + else + return snd_pcm_lib_ioctl(substream, cmd, arg); +} + int snd_pcm_info(struct snd_pcm_substream *substream, struct snd_pcm_info *info) { struct snd_pcm *pcm = substream->pcm; @@ -448,8 +458,9 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS); if (snd_mask_single(m) && snd_interval_single(i)) { - err = substream->ops->ioctl(substream, - SNDRV_PCM_IOCTL1_FIFO_SIZE, params); + err = snd_pcm_ops_ioctl(substream, + SNDRV_PCM_IOCTL1_FIFO_SIZE, + params); if (err < 0) return err; } @@ -971,7 +982,7 @@ static int snd_pcm_channel_info(struct snd_pcm_substream *substream, return -EINVAL; memset(info, 0, sizeof(*info)); info->channel = channel; - return substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_CHANNEL_INFO, info); + return snd_pcm_ops_ioctl(substream, SNDRV_PCM_IOCTL1_CHANNEL_INFO, info); }

static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream, @@ -1647,7 +1658,7 @@ static int snd_pcm_pre_reset(struct snd_pcm_substream *substream, int state) static int snd_pcm_do_reset(struct snd_pcm_substream *substream, int state) { struct snd_pcm_runtime *runtime = substream->runtime; - int err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); + int err = snd_pcm_ops_ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); if (err < 0) return err; runtime->hw_ptr_base = 0;

-- 2.16.4

Takashi Iwai

8:53 a.m.

New subject: [alsa-devel] [PATCH 4/8] ALSA: docs: Update document about the default PCM ioctl ops

Mention that it's completely optional now.

Signed-off-by: Takashi Iwai tiwai@suse.de --- Documentation/sound/kernel-api/writing-an-alsa-driver.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index 1086b35db2af..145bf6aad7cb 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -1341,7 +1341,6 @@ shows only the skeleton, how to build up the PCM interfaces. static struct snd_pcm_ops snd_mychip_playback_ops = { .open = snd_mychip_playback_open, .close = snd_mychip_playback_close, - .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_mychip_pcm_hw_params, .hw_free = snd_mychip_pcm_hw_free, .prepare = snd_mychip_pcm_prepare, @@ -1353,7 +1352,6 @@ shows only the skeleton, how to build up the PCM interfaces. static struct snd_pcm_ops snd_mychip_capture_ops = { .open = snd_mychip_capture_open, .close = snd_mychip_capture_close, - .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_mychip_pcm_hw_params, .hw_free = snd_mychip_pcm_hw_free, .prepare = snd_mychip_pcm_prepare, @@ -1456,7 +1454,6 @@ The operators are defined typically like this: static struct snd_pcm_ops snd_mychip_playback_ops = { .open = snd_mychip_pcm_open, .close = snd_mychip_pcm_close, - .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_mychip_pcm_hw_params, .hw_free = snd_mychip_pcm_hw_free, .prepare = snd_mychip_pcm_prepare, @@ -1913,7 +1910,10 @@ ioctl callback ~~~~~~~~~~~~~~

This is used for any special call to pcm ioctls. But usually you can -pass a generic ioctl callback, :c:func:`snd_pcm_lib_ioctl()`. +leave it as NULL, then PCM core calls the generic ioctl callback +function :c:func:`snd_pcm_lib_ioctl()`. If you need to deal with the +unique setup of channel info or reset procedure, you can pass your own +callback function here.

hw_params callback ~~~~~~~~~~~~~~~~~~~

-- 2.16.4

Takashi Iwai

8:53 a.m.

New subject: [alsa-devel] [PATCH 5/8] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header

It should be used only in the PCM core code locally.

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 2 -- sound/core/pcm_local.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 253d15c61ce2..25563317782c 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -1336,8 +1336,6 @@ static inline void snd_pcm_limit_isa_dma_size(int dma, size_t *max) (IEC958_AES1_CON_PCM_CODER<<8)|\ (IEC958_AES3_CON_FS_48000<<24))

-#define PCM_RUNTIME_CHECK(sub) snd_BUG_ON(!(sub) || !(sub)->runtime) - const char *snd_pcm_format_name(snd_pcm_format_t format);

/** diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 5565e1c4196a..384efd002984 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -72,4 +72,6 @@ struct page *snd_pcm_sgbuf_ops_page(struct snd_pcm_substream *substream, unsigned long offset); #endif

+#define PCM_RUNTIME_CHECK(sub) snd_BUG_ON(!(sub) || !(sub)->runtime) + #endif /* __SOUND_CORE_PCM_LOCAL_H */

-- 2.16.4

kbuild test robot

9:42 a.m.

New subject: [alsa-devel] [PATCH 5/8] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header

Hi Takashi,

I love your patch! Yet something to improve:

[auto build test ERROR on sound/for-next] [also build test ERROR on next-20191115] [cannot apply to v5.4-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-pcm-API-cleanups-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: i386-defconfig (attached as .config) compiler: gcc-7 (Debian 7.4.0-14) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=i386

If you fix the issue, kindly add following tag Reported-by: kbuild test robot lkp@intel.com

All errors (new ones prefixed by >>):

sound/core/pcm_memory.c: In function 'snd_pcm_lib_malloc_pages':

...

...
sound/core/pcm_memory.c:351:6: error: implicit declaration of function 'PCM_RUNTIME_CHECK'; did you mean 'APM_FUNC_INST_CHECK'? [-Werror=implicit-function-declaration]

if (PCM_RUNTIME_CHECK(substream)) ^~~~~~~~~~~~~~~~~ APM_FUNC_INST_CHECK cc1: some warnings being treated as errors

vim +351 sound/core/pcm_memory.c

51e9f2e665bf2b Takashi Iwai 2008-07-30 334 ^1da177e4c3f41 Linus Torvalds 2005-04-16 335 /** ^1da177e4c3f41 Linus Torvalds 2005-04-16 336 * snd_pcm_lib_malloc_pages - allocate the DMA buffer ^1da177e4c3f41 Linus Torvalds 2005-04-16 337 * @substream: the substream to allocate the DMA buffer to ^1da177e4c3f41 Linus Torvalds 2005-04-16 338 * @size: the requested buffer size in bytes ^1da177e4c3f41 Linus Torvalds 2005-04-16 339 * ^1da177e4c3f41 Linus Torvalds 2005-04-16 340 * Allocates the DMA buffer on the BUS type given earlier to ^1da177e4c3f41 Linus Torvalds 2005-04-16 341 * snd_pcm_lib_preallocate_xxx_pages(). ^1da177e4c3f41 Linus Torvalds 2005-04-16 342 * eb7c06e8e9c93b Yacine Belkadi 2013-03-11 343 * Return: 1 if the buffer is changed, 0 if not changed, or a negative ^1da177e4c3f41 Linus Torvalds 2005-04-16 344 * code on failure. ^1da177e4c3f41 Linus Torvalds 2005-04-16 345 */ 877211f5e1b119 Takashi Iwai 2005-11-17 346 int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size) ^1da177e4c3f41 Linus Torvalds 2005-04-16 347 { 877211f5e1b119 Takashi Iwai 2005-11-17 348 struct snd_pcm_runtime *runtime; ^1da177e4c3f41 Linus Torvalds 2005-04-16 349 struct snd_dma_buffer *dmab = NULL; ^1da177e4c3f41 Linus Torvalds 2005-04-16 350 7eaa943c8ed8e9 Takashi Iwai 2008-08-08 @351 if (PCM_RUNTIME_CHECK(substream)) 7eaa943c8ed8e9 Takashi Iwai 2008-08-08 352 return -EINVAL; 7eaa943c8ed8e9 Takashi Iwai 2008-08-08 353 if (snd_BUG_ON(substream->dma_buffer.dev.type == 7eaa943c8ed8e9 Takashi Iwai 2008-08-08 354 SNDRV_DMA_TYPE_UNKNOWN)) 7eaa943c8ed8e9 Takashi Iwai 2008-08-08 355 return -EINVAL; ^1da177e4c3f41 Linus Torvalds 2005-04-16 356 runtime = substream->runtime; ^1da177e4c3f41 Linus Torvalds 2005-04-16 357 ^1da177e4c3f41 Linus Torvalds 2005-04-16 358 if (runtime->dma_buffer_p) { ^1da177e4c3f41 Linus Torvalds 2005-04-16 359 /* perphaps, we might free the large DMA memory region ^1da177e4c3f41 Linus Torvalds 2005-04-16 360 to save some space here, but the actual solution ^1da177e4c3f41 Linus Torvalds 2005-04-16 361 costs us less time */ ^1da177e4c3f41 Linus Torvalds 2005-04-16 362 if (runtime->dma_buffer_p->bytes >= size) { ^1da177e4c3f41 Linus Torvalds 2005-04-16 363 runtime->dma_bytes = size; ^1da177e4c3f41 Linus Torvalds 2005-04-16 364 return 0; /* ok, do not change */ ^1da177e4c3f41 Linus Torvalds 2005-04-16 365 } ^1da177e4c3f41 Linus Torvalds 2005-04-16 366 snd_pcm_lib_free_pages(substream); ^1da177e4c3f41 Linus Torvalds 2005-04-16 367 } 877211f5e1b119 Takashi Iwai 2005-11-17 368 if (substream->dma_buffer.area != NULL && 877211f5e1b119 Takashi Iwai 2005-11-17 369 substream->dma_buffer.bytes >= size) { ^1da177e4c3f41 Linus Torvalds 2005-04-16 370 dmab = &substream->dma_buffer; /* use the pre-allocated buffer */ ^1da177e4c3f41 Linus Torvalds 2005-04-16 371 } else { ca2c0966562cfb Takashi Iwai 2005-09-09 372 dmab = kzalloc(sizeof(*dmab), GFP_KERNEL); ^1da177e4c3f41 Linus Torvalds 2005-04-16 373 if (! dmab) ^1da177e4c3f41 Linus Torvalds 2005-04-16 374 return -ENOMEM; ^1da177e4c3f41 Linus Torvalds 2005-04-16 375 dmab->dev = substream->dma_buffer.dev; ^1da177e4c3f41 Linus Torvalds 2005-04-16 376 if (snd_dma_alloc_pages(substream->dma_buffer.dev.type, ^1da177e4c3f41 Linus Torvalds 2005-04-16 377 substream->dma_buffer.dev.dev, ^1da177e4c3f41 Linus Torvalds 2005-04-16 378 size, dmab) < 0) { ^1da177e4c3f41 Linus Torvalds 2005-04-16 379 kfree(dmab); ^1da177e4c3f41 Linus Torvalds 2005-04-16 380 return -ENOMEM; ^1da177e4c3f41 Linus Torvalds 2005-04-16 381 } ^1da177e4c3f41 Linus Torvalds 2005-04-16 382 } ^1da177e4c3f41 Linus Torvalds 2005-04-16 383 snd_pcm_set_runtime_buffer(substream, dmab); ^1da177e4c3f41 Linus Torvalds 2005-04-16 384 runtime->dma_bytes = size; ^1da177e4c3f41 Linus Torvalds 2005-04-16 385 return 1; /* area was changed */ ^1da177e4c3f41 Linus Torvalds 2005-04-16 386 } e88e8ae639a490 Takashi Iwai 2006-04-28 387 EXPORT_SYMBOL(snd_pcm_lib_malloc_pages); e88e8ae639a490 Takashi Iwai 2006-04-28 388

:::::: The code at line 351 was first introduced by commit :::::: 7eaa943c8ed8e91e05d0f5d0dc7a18e3319b45cf ALSA: Kill snd_assert() in sound/core/*

:::::: TO: Takashi Iwai tiwai@suse.de :::::: CC: Jaroslav Kysela perex@perex.cz

--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Takashi Iwai

10:05 a.m.

New subject: [alsa-devel] [PATCH 5/8] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header

On Sun, 17 Nov 2019 10:42:06 +0100, kbuild test robot wrote:

...

Hi Takashi,

I love your patch! Yet something to improve:

[auto build test ERROR on sound/for-next] [also build test ERROR on next-20191115] [cannot apply to v5.4-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-pcm-API-cleanups-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: i386-defconfig (attached as .config) compiler: gcc-7 (Debian 7.4.0-14) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=i386

If you fix the issue, kindly add following tag Reported-by: kbuild test robot lkp@intel.com

All errors (new ones prefixed by >>):

sound/core/pcm_memory.c: In function 'snd_pcm_lib_malloc_pages':

...
...
sound/core/pcm_memory.c:351:6: error: implicit declaration of function 'PCM_RUNTIME_CHECK'; did you mean 'APM_FUNC_INST_CHECK'? [-Werror=implicit-function-declaration]
 if (PCM_RUNTIME_CHECK(substream))
     ^~~~~~~~~~~~~~~~~
     APM_FUNC_INST_CHECK
cc1: some warnings being treated as errors

OK, this was due to a reshuffling of patchset. The revised patch is below.

thanks,

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header

It should be used only in the PCM core code locally.

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 2 -- sound/core/pcm_local.h | 2 ++ sound/core/pcm_memory.c | 1 + 3 files changed, 3 insertions(+), 2 deletions(-)

-#define PCM_RUNTIME_CHECK(sub) snd_BUG_ON(!(sub) || !(sub)->runtime) - const char *snd_pcm_format_name(snd_pcm_format_t format);

+#define PCM_RUNTIME_CHECK(sub) snd_BUG_ON(!(sub) || !(sub)->runtime) + #endif /* __SOUND_CORE_PCM_LOCAL_H */ diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c index 73b770db2372..d4702cc1d376 100644 --- a/sound/core/pcm_memory.c +++ b/sound/core/pcm_memory.c @@ -15,6 +15,7 @@ #include <sound/pcm.h> #include <sound/info.h> #include <sound/initval.h> +#include "pcm_local.h"

static int preallocate_dma = 1; module_param(preallocate_dma, int, 0444);

-- 2.16.4

kbuild test robot

10:28 a.m.

New subject: [alsa-devel] [PATCH 5/8] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header

Hi Takashi,

I love your patch! Yet something to improve:

url: https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-pcm-API-cleanups-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: arc-randconfig-a0031-20191117 (attached as .config) compiler: arc-elf-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag Reported-by: kbuild test robot lkp@intel.com

All error/warnings (new ones prefixed by >>):

In file included from include/linux/init.h:5:0, from include/linux/io.h:10, from sound/core/pcm_memory.c:7: sound/core/pcm_memory.c: In function 'snd_pcm_lib_malloc_pages':

...

...
sound/core/pcm_memory.c:351:6: error: implicit declaration of function 'PCM_RUNTIME_CHECK' [-Werror=implicit-function-declaration]

if (PCM_RUNTIME_CHECK(substream)) ^ include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) ^~~~

...

...
sound/core/pcm_memory.c:351:2: note: in expansion of macro 'if'

if (PCM_RUNTIME_CHECK(substream)) ^~ cc1: some warnings being treated as errors

vim +/PCM_RUNTIME_CHECK +351 sound/core/pcm_memory.c

:::::: The code at line 351 was first introduced by commit :::::: 7eaa943c8ed8e91e05d0f5d0dc7a18e3319b45cf ALSA: Kill snd_assert() in sound/core/*

:::::: TO: Takashi Iwai tiwai@suse.de :::::: CC: Jaroslav Kysela perex@perex.cz

--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Takashi Iwai

8:53 a.m.

New subject: [alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation

The standard programming model of a PCM sound driver is to process snd_pcm_period_elapsed() from an interrupt handler. When a running stream is stopped, PCM core calls the trigger-STOP PCM ops, sets the stream state to SETUP, and moves on to the next step. This is performed in an atomic manner -- this could be called from the interrupt context, after all.

The problem is that, if the stream goes further and reaches to the CLOSE state immediately, the stream might be still being processed in snd_pcm_period_elapsed() in the interrupt context, and hits a NULL dereference. Such a crash happens because of the atomic operation, and we can't wait until the stream-stop finishes.

For addressing such a problem, this commit adds a new PCM ops, sync_stop. This gets called at the appropriate places that need a sync with the stream-stop, i.e. at hw_params, prepare and hw_free.

Some drivers already have a similar mechanism implemented locally, and we'll refactor the code later.

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 2 ++ sound/core/pcm_native.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 25563317782c..8a89fa6fdd5e 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -59,6 +59,7 @@ struct snd_pcm_ops { int (*hw_free)(struct snd_pcm_substream *substream); int (*prepare)(struct snd_pcm_substream *substream); int (*trigger)(struct snd_pcm_substream *substream, int cmd); + int (*sync_stop)(struct snd_pcm_substream *substream); snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream); int (*get_time_info)(struct snd_pcm_substream *substream, struct timespec *system_ts, struct timespec *audio_ts, @@ -395,6 +396,7 @@ struct snd_pcm_runtime { wait_queue_head_t sleep; /* poll sleep */ wait_queue_head_t tsleep; /* transfer sleep */ struct fasync_struct *fasync; + bool stop_operating; /* sync_stop will be called */

/* -- private section -- */ void *private_data; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 704ea75199e4..163d621ff238 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -568,6 +568,15 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream, #endif }

+static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) +{ + if (substream->runtime->stop_operating) { + substream->runtime->stop_operating = false; + if (substream->ops->sync_stop) + substream->ops->sync_stop(substream); + } +} + /** * snd_pcm_hw_param_choose - choose a configuration defined by @params * @pcm: PCM instance @@ -660,6 +669,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if (atomic_read(&substream->mmap_count)) return -EBADFD;

+ snd_pcm_sync_stop(substream); + params->rmask = ~0U; err = snd_pcm_hw_refine(substream, params); if (err < 0) @@ -788,6 +799,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) snd_pcm_stream_unlock_irq(substream); if (atomic_read(&substream->mmap_count)) return -EBADFD; + snd_pcm_sync_stop(substream); if (substream->ops->hw_free) result = substream->ops->hw_free(substream); if (substream->managed_buffer_alloc) @@ -1313,6 +1325,7 @@ static void snd_pcm_post_stop(struct snd_pcm_substream *substream, int state) runtime->status->state = state; snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP); } + runtime->stop_operating = true; wake_up(&runtime->sleep); wake_up(&runtime->tsleep); } @@ -1589,6 +1602,7 @@ static void snd_pcm_post_resume(struct snd_pcm_substream *substream, int state) snd_pcm_trigger_tstamp(substream); runtime->status->state = runtime->status->suspended_state; snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MRESUME); + snd_pcm_sync_stop(substream); }

static const struct action_ops snd_pcm_action_resume = { @@ -1709,6 +1723,7 @@ static int snd_pcm_pre_prepare(struct snd_pcm_substream *substream, static int snd_pcm_do_prepare(struct snd_pcm_substream *substream, int state) { int err; + snd_pcm_sync_stop(substream); err = substream->ops->prepare(substream); if (err < 0) return err;

-- 2.16.4

Pierre-Louis Bossart

18 Nov 18 Nov

4:33 p.m.

New subject: [alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...

The standard programming model of a PCM sound driver is to process snd_pcm_period_elapsed() from an interrupt handler. When a running stream is stopped, PCM core calls the trigger-STOP PCM ops, sets the stream state to SETUP, and moves on to the next step. This is performed in an atomic manner -- this could be called from the interrupt context, after all.

The problem is that, if the stream goes further and reaches to the CLOSE state immediately, the stream might be still being processed in snd_pcm_period_elapsed() in the interrupt context, and hits a NULL dereference. Such a crash happens because of the atomic operation, and we can't wait until the stream-stop finishes.

For addressing such a problem, this commit adds a new PCM ops, sync_stop. This gets called at the appropriate places that need a sync with the stream-stop, i.e. at hw_params, prepare and hw_free.

Some drivers already have a similar mechanism implemented locally, and we'll refactor the code later.

This rings a bell, for the SOF driver Keyon added a workqueue to avoid doing the call to snd_pcm_period_elapsed() while handling IPC interrupts. I believe the reason what that the IPC needs to be serialized, and the call to snd_pcm_period_elapsed could initiate a trigger stop IPC while we were dealing with an IPC, which broke the notion of serialization.

See "ASoC: SOF: PCM: add period_elapsed work to fix race condition in interrupt context" and "ASoC: SOF: ipc: use snd_sof_pcm_period_elapsed"

...

Signed-off-by: Takashi Iwai tiwai@suse.de

include/sound/pcm.h | 2 ++ sound/core/pcm_native.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 25563317782c..8a89fa6fdd5e 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -59,6 +59,7 @@ struct snd_pcm_ops { int (*hw_free)(struct snd_pcm_substream *substream); int (*prepare)(struct snd_pcm_substream *substream); int (*trigger)(struct snd_pcm_substream *substream, int cmd);

int (*sync_stop)(struct snd_pcm_substream *substream); snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream); int (*get_time_info)(struct snd_pcm_substream *substream, struct timespec *system_ts, struct timespec *audio_ts,

@@ -395,6 +396,7 @@ struct snd_pcm_runtime { wait_queue_head_t sleep; /* poll sleep */ wait_queue_head_t tsleep; /* transfer sleep */ struct fasync_struct *fasync;

bool stop_operating; /* sync_stop will be called */

/* -- private section -- */ void *private_data;

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 704ea75199e4..163d621ff238 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -568,6 +568,15 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream, #endif }

+static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) +{
if (substream->runtime->stop_operating) {
substream->runtime->stop_operating = false;
if (substream->ops->sync_stop)
	substream->ops->sync_stop(substream);
}
+}

/**

snd_pcm_hw_param_choose - choose a configuration defined by @params

@pcm: PCM instance

@@ -660,6 +669,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if (atomic_read(&substream->mmap_count)) return -EBADFD;

snd_pcm_sync_stop(substream);

params->rmask = ~0U; err = snd_pcm_hw_refine(substream, params); if (err < 0)

@@ -788,6 +799,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream) snd_pcm_stream_unlock_irq(substream); if (atomic_read(&substream->mmap_count)) return -EBADFD;

snd_pcm_sync_stop(substream); if (substream->ops->hw_free) result = substream->ops->hw_free(substream); if (substream->managed_buffer_alloc)

@@ -1313,6 +1325,7 @@ static void snd_pcm_post_stop(struct snd_pcm_substream *substream, int state) runtime->status->state = state; snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP); }

runtime->stop_operating = true; wake_up(&runtime->sleep); wake_up(&runtime->tsleep); }

@@ -1589,6 +1602,7 @@ static void snd_pcm_post_resume(struct snd_pcm_substream *substream, int state) snd_pcm_trigger_tstamp(substream); runtime->status->state = runtime->status->suspended_state; snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MRESUME);

snd_pcm_sync_stop(substream); }

static const struct action_ops snd_pcm_action_resume = {

@@ -1709,6 +1723,7 @@ static int snd_pcm_pre_prepare(struct snd_pcm_substream *substream, static int snd_pcm_do_prepare(struct snd_pcm_substream *substream, int state) { int err;

snd_pcm_sync_stop(substream); err = substream->ops->prepare(substream); if (err < 0) return err;

Takashi Iwai

6:47 p.m.

New subject: [alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation

On Mon, 18 Nov 2019 17:33:51 +0100, Pierre-Louis Bossart wrote:

...

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...
The standard programming model of a PCM sound driver is to process snd_pcm_period_elapsed() from an interrupt handler. When a running stream is stopped, PCM core calls the trigger-STOP PCM ops, sets the stream state to SETUP, and moves on to the next step. This is performed in an atomic manner -- this could be called from the interrupt context, after all.

The problem is that, if the stream goes further and reaches to the CLOSE state immediately, the stream might be still being processed in snd_pcm_period_elapsed() in the interrupt context, and hits a NULL dereference. Such a crash happens because of the atomic operation, and we can't wait until the stream-stop finishes.

For addressing such a problem, this commit adds a new PCM ops, sync_stop. This gets called at the appropriate places that need a sync with the stream-stop, i.e. at hw_params, prepare and hw_free.

Some drivers already have a similar mechanism implemented locally, and we'll refactor the code later.

This rings a bell, for the SOF driver Keyon added a workqueue to avoid doing the call to snd_pcm_period_elapsed() while handling IPC interrupts. I believe the reason what that the IPC needs to be serialized, and the call to snd_pcm_period_elapsed could initiate a trigger stop IPC while we were dealing with an IPC, which broke the notion of serialization.

See "ASoC: SOF: PCM: add period_elapsed work to fix race condition in interrupt context" and "ASoC: SOF: ipc: use snd_sof_pcm_period_elapsed"

The new PCM ops is exactly for resolving such a thing. Basically snd_pcm_period_elapsed() is handled asynchronously, so we need the synchronization. For a simpler driver, it's merely synchronize_irq() call (no matter whether it's a hard-irq or a threaded-irq), but for drivers that deal with another async mechanism (e.g. USB-audio), another sync procedure is required.

It's possible that the workaround you've done in SOF can be simplified by this mechanism, but I didn't take a deeper look at it yet.

thanks,

Takashi

Takashi Iwai

17 Nov 17 Nov

8:53 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

Many PCI and other drivers performs snd_pcm_period_elapsed() simply in its interrupt handler, so the sync_stop operation is just to call synchronize_irq(). Instead of putting this call multiple times, introduce the common card->sync_irq field. When this field is set, PCM core performs synchronize_irq() for sync-stop operation. Each driver just needs to copy its local IRQ number to card->sync_irq, and that's all we need.

Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/core.h | 1 + sound/core/init.c | 1 + sound/core/pcm_native.c | 2 ++ 3 files changed, 4 insertions(+)

diff --git a/include/sound/core.h b/include/sound/core.h index ee238f100f73..af3dce956c17 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -117,6 +117,7 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */ + int sync_irq; /* assigned irq, used for PCM sync */ wait_queue_head_t remove_sleep;

#ifdef CONFIG_PM diff --git a/sound/core/init.c b/sound/core/init.c index db99b7fad6ad..faa9f03c01ca 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -215,6 +215,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, init_waitqueue_head(&card->power_sleep); #endif init_waitqueue_head(&card->remove_sleep); + card->sync_irq = -1;

device_initialize(&card->card_dev); card->card_dev.parent = parent; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 163d621ff238..1fe581167b7b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -574,6 +574,8 @@ static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) substream->runtime->stop_operating = false; if (substream->ops->sync_stop) substream->ops->sync_stop(substream); + else if (substream->pcm->card->sync_irq > 0) + synchronize_irq(substream->pcm->card->sync_irq); } }

-- 2.16.4

Pierre-Louis Bossart

18 Nov 18 Nov

4:38 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...

Many PCI and other drivers performs snd_pcm_period_elapsed() simply in its interrupt handler, so the sync_stop operation is just to call synchronize_irq(). Instead of putting this call multiple times, introduce the common card->sync_irq field. When this field is set, PCM core performs synchronize_irq() for sync-stop operation. Each driver just needs to copy its local IRQ number to card->sync_irq, and that's all we need.

Maybe a red-herring or complete non-sense, but I wonder if this is going to get in the way of Ranjani's multi-client work, where we could have multiple cards created but with a single IRQ handled by the parent PCI device?

Ranjani, you may want to double-check this and chime in, thanks!

...

Signed-off-by: Takashi Iwai tiwai@suse.de

include/sound/core.h | 1 + sound/core/init.c | 1 + sound/core/pcm_native.c | 2 ++ 3 files changed, 4 insertions(+)

diff --git a/include/sound/core.h b/include/sound/core.h index ee238f100f73..af3dce956c17 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -117,6 +117,7 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */

int sync_irq; /* assigned irq, used for PCM sync */ wait_queue_head_t remove_sleep;

#ifdef CONFIG_PM

diff --git a/sound/core/init.c b/sound/core/init.c index db99b7fad6ad..faa9f03c01ca 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -215,6 +215,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, init_waitqueue_head(&card->power_sleep); #endif init_waitqueue_head(&card->remove_sleep);

card->sync_irq = -1;

device_initialize(&card->card_dev); card->card_dev.parent = parent;

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 163d621ff238..1fe581167b7b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -574,6 +574,8 @@ static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) substream->runtime->stop_operating = false; if (substream->ops->sync_stop) substream->ops->sync_stop(substream);
else if (substream->pcm->card->sync_irq > 0)
	synchronize_irq(substream->pcm->card->sync_irq);
} }

Takashi Iwai

6:52 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Mon, 18 Nov 2019 17:38:49 +0100, Pierre-Louis Bossart wrote:

...

On 11/17/19 2:53 AM, Takashi Iwai wrote:

...
Many PCI and other drivers performs snd_pcm_period_elapsed() simply in its interrupt handler, so the sync_stop operation is just to call synchronize_irq(). Instead of putting this call multiple times, introduce the common card->sync_irq field. When this field is set, PCM core performs synchronize_irq() for sync-stop operation. Each driver just needs to copy its local IRQ number to card->sync_irq, and that's all we need.

Maybe a red-herring or complete non-sense, but I wonder if this is going to get in the way of Ranjani's multi-client work, where we could have multiple cards created but with a single IRQ handled by the parent PCI device?

Ranjani, you may want to double-check this and chime in, thanks!

The synchronize_irq() is fairly safe to call multiple times, and I don't think any problem by invoking it for multi-clients sharing the same IRQ. For example, Digigram miXart driver creates multiple card objects from a single PCI entry, and I already thought of that possibility; they set the same card->sync_irq value to all card objects, which eventually will call synchronize_irq() multiple times. From the performance POV, this shouldn't be a big problem, because the place calling this is only at hw_params, prepare and hw_free, neither are hot-path.

thanks,

Takashi

Sridharan, Ranjani

7:20 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Mon, Nov 18, 2019 at 10:54 AM Takashi Iwai tiwai@suse.de wrote:

...

On Mon, 18 Nov 2019 17:38:49 +0100, Pierre-Louis Bossart wrote:

...
On 11/17/19 2:53 AM, Takashi Iwai wrote:

...
Many PCI and other drivers performs snd_pcm_period_elapsed() simply in its interrupt handler, so the sync_stop operation is just to call synchronize_irq(). Instead of putting this call multiple times, introduce the common card->sync_irq field. When this field is set, PCM core performs synchronize_irq() for sync-stop operation. Each driver just needs to copy its local IRQ number to card->sync_irq, and that's all we need.

Maybe a red-herring or complete non-sense, but I wonder if this is going to get in the way of Ranjani's multi-client work, where we could have multiple cards created but with a single IRQ handled by the parent PCI device?

Ranjani, you may want to double-check this and chime in, thanks!

The synchronize_irq() is fairly safe to call multiple times, and I don't think any problem by invoking it for multi-clients sharing the same IRQ. For example, Digigram miXart driver creates multiple card objects from a single PCI entry, and I already thought of that possibility; they set the same card->sync_irq value to all card objects, which eventually will call synchronize_irq() multiple times. From the performance POV, this shouldn't be a big problem, because the place calling this is only at hw_params, prepare and hw_free, neither are hot-path.

Thanks for the clarification, Takashi. But just wondering how would one pass on the sync_irq when the snd_card is created? Typically in the case of the Intel platforms, the card->dev points to the platform device for the machine driver that registers the card and the PCI device is the parent of the machine drv platform device.

Thanks, Ranjani

Takashi Iwai

7:49 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Mon, 18 Nov 2019 20:20:41 +0100, Sridharan, Ranjani wrote:

...

On Mon, Nov 18, 2019 at 10:54 AM Takashi Iwai tiwai@suse.de wrote:

On Mon, 18 Nov 2019 17:38:49 +0100,
Pierre-Louis Bossart wrote:
>
>
>
> On 11/17/19 2:53 AM, Takashi Iwai wrote:
> > Many PCI and other drivers performs snd_pcm_period_elapsed() simply in
> > its interrupt handler, so the sync_stop operation is just to call
> > synchronize_irq().  Instead of putting this call multiple times,
> > introduce the common card->sync_irq field.  When this field is set,
> > PCM core performs synchronize_irq() for sync-stop operation.  Each
> > driver just needs to copy its local IRQ number to card->sync_irq, and
> > that's all we need.
>
> Maybe a red-herring or complete non-sense, but I wonder if this is
> going to get in the way of Ranjani's multi-client work, where we could
> have multiple cards created but with a single IRQ handled by the
> parent PCI device?
>
> Ranjani, you may want to double-check this and chime in, thanks!

The synchronize_irq() is fairly safe to call multiple times, and I
don't think any problem by invoking it for multi-clients sharing the
same IRQ.  For example, Digigram miXart driver creates multiple card
objects from a single PCI entry, and I already thought of that
possibility; they set the same card->sync_irq value to all card
objects, which eventually will call synchronize_irq() multiple times.
 From the performance POV, this shouldn't be a big problem, because
the place calling this is only at hw_params, prepare and hw_free,
neither are hot-path.

It's completely up to the driver implementation :) You can implement the own sync_stop ops if that's easier, too.

In general, the driver can set up card->sync_irq when the request_irq() or its variant is called.

Takashi

Sridharan, Ranjani

7:55 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

...
Thanks for the clarification, Takashi. But just wondering how would one

pass

...
on the sync_irq when the snd_card is created? Typically in the case of

the

...
Intel platforms, the card->dev points to the platform device for the

machine

...
driver that registers the card and the PCI device is the parent of the

machine

...
drv platform device.

It's completely up to the driver implementation :) You can implement the own sync_stop ops if that's easier, too.

I think this would make sense in the case of the SOF driver and we'd probably need to just call synchronize_irq() in the sync_stop() operation. With this change, we can probably remove the workaround we have to address the issue we were facing during snd_pcm_period_elapsed().

I can give this a try. We might need to run some stress tests to make sure it doesn't break anything.

Thanks, Ranjani

Takashi Iwai

8:40 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Mon, 18 Nov 2019 20:55:19 +0100, Sridharan, Ranjani wrote:

...

> Thanks for the clarification, Takashi. But just wondering how would one
pass
> on the sync_irq when the snd_card is created? Typically in the case of
the
> Intel platforms, the card->dev points to the platform device for the
machine
> driver that registers the card and the PCI device is the parent of the
machine
> drv platform device. 

It's completely up to the driver implementation :)
You can implement the own sync_stop ops if that's easier, too.
I think this would make sense in the case of the SOF driver and we'd probably need to just call synchronize_irq() in the sync_stop() operation. With this change, we can probably remove the workaround we have to address the issue we were facing during snd_pcm_period_elapsed().

I can give this a try. We might need to run some stress tests to make sure it doesn't break anything.

If this helps for SOF, it'd be great. I have converted only non-ASoC drivers regarding the sync-stop stuff, so it won't conflict my upcoming changes :)

FWIW, the rest diff (post topic/pcm-managed) looks like:

Takashi

Ranjani Sridharan

11:47 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Mon, 2019-11-18 at 21:40 +0100, Takashi Iwai wrote:

...

On Mon, 18 Nov 2019 20:55:19 +0100, Sridharan, Ranjani wrote:

...
> Thanks for the clarification, Takashi. But just wondering how
would one pass > on the sync_irq when the snd_card is created? Typically in the case of the > Intel platforms, the card->dev points to the platform device for the machine > driver that registers the card and the PCI device is the parent of the machine > drv platform device.
It's completely up to the driver implementation :)
You can implement the own sync_stop ops if that's easier, too.
I think this would make sense in the case of the SOF driver and we'd probably need to just call synchronize_irq() in the sync_stop() operation. With this change, we can probably remove the workaround we have to address the issue we were facing during snd_pcm_period_elapsed().

I can give this a try. We might need to run some stress tests to make sure it doesn't break anything.
If this helps for SOF, it'd be great. I have converted only non-ASoC drivers regarding the sync-stop stuff, so it won't conflict my upcoming changes :)

Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Thanks, Ranjani

Takashi Iwai

19 Nov 19 Nov

6:44 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, 19 Nov 2019 00:47:53 +0100, Ranjani Sridharan wrote:

...

On Mon, 2019-11-18 at 21:40 +0100, Takashi Iwai wrote:

...
On Mon, 18 Nov 2019 20:55:19 +0100, Sridharan, Ranjani wrote:

...
> Thanks for the clarification, Takashi. But just wondering how
would one pass > on the sync_irq when the snd_card is created? Typically in the case of the > Intel platforms, the card->dev points to the platform device for the machine > driver that registers the card and the PCI device is the parent of the machine > drv platform device.
It's completely up to the driver implementation :)
You can implement the own sync_stop ops if that's easier, too.
I think this would make sense in the case of the SOF driver and we'd probably need to just call synchronize_irq() in the sync_stop() operation. With this change, we can probably remove the workaround we have to address the issue we were facing during snd_pcm_period_elapsed().

I can give this a try. We might need to run some stress tests to make sure it doesn't break anything.
If this helps for SOF, it'd be great. I have converted only non-ASoC drivers regarding the sync-stop stuff, so it won't conflict my upcoming changes :)
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

thanks,

Takashi

Ranjani Sridharan

7:40 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

Thanks, Ranjani

...

thanks,

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

Takashi Iwai

8:24 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote:

...

...
...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

thanks,

Takashi

Takashi Iwai

9:39 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, 19 Nov 2019 09:24:33 +0100, Takashi Iwai wrote:

...

On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote:

...
...
...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

... and looking at the code again, I don't think the stop_sync ops would help in your case. The PCM ops is called *after* trigger-STOP is issued, while your case requires the serialization of trigger call itself.

But I'm still wondering whether the current implementation is safe, too. Basically the scheduled work may be executed immediately after queuing, so if it's about the timing issue, it's not solid solution. Pushing to work helps if it's about the locking issue, though.

Takashi

Ranjani Sridharan

4:36 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote:

...

On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote:

...
...
...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the device hang when the stream is stoppped. But that's a bit surprising given that all I tried was using the snd_pcm_period_elapsed() directly instead of scheduling the delayed work to call it.

Thanks, Ranjani

Takashi Iwai

9:27 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, 19 Nov 2019 17:36:46 +0100, Ranjani Sridharan wrote:

...

On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote:

...
On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote:

...
...
...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the device hang when the stream is stoppped. But that's a bit surprising given that all I tried was using the snd_pcm_period_elapsed() directly instead of scheduling the delayed work to call it.

If I read the code correctly, this can't work irrelevantly from the sync_stop stuff. The call of period_elapsed is from hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and this function expects the sleepable context due to snd_sof_dsp_read_poll_timeout() call.

So something like below works?

Takashi

--- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status) continue;

/* Inform ALSA only in case not do that with IPC */ - if (sof_hda->no_ipc_position) - snd_sof_pcm_period_elapsed(s->substream); + if (sof_hda->no_ipc_position) { + spin_unlock_irq(&bus->reg_lock); + snd_pcm_period_elapsed(s->substream); + spin_lock_irq(&bus->reg_lock); + } } }

Sridharan, Ranjani

9:43 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Tue, Nov 19, 2019 at 1:28 PM Takashi Iwai tiwai@suse.de wrote:

...

On Tue, 19 Nov 2019 17:36:46 +0100, Ranjani Sridharan wrote:

...
On Tue, 2019-11-19 at 09:24 +0100, Takashi Iwai wrote:

...
On Tue, 19 Nov 2019 08:40:25 +0100, Ranjani Sridharan wrote:

...
...
...
Hi Takashi,

I just realized that In the SOF driver, we only set the component driver ops. The pcm ops are set when creating the new pcm. So, should I also add the sync_stop op in the component driver and set the pcm sync_stop op to point to the component sync_stop op? Just wanted to confirm if I am on the right track.

Yes, I didn't touch this yet, but that's the way to go I suppose. One caveat is that this ops is optional and needs NULL as default, hence you'd need to set only when defined, like copy_user, page or mmap ops, at least.

Hi Takashi,

This is what I tried in the SOF driver: https://github.com/thesofproject/linux/pull/1513/commits

And it seems to cause the system to hang when I stop the stream and I have no meaningful logs to pinpoint to the problem. Could you please have a look at the 4 commits that I have added to your series and let me know what I could be missing?

I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the device hang when the stream is stoppped. But that's a bit surprising given that all I tried was using the snd_pcm_period_elapsed() directly instead of scheduling the delayed work to call it.

If I read the code correctly, this can't work irrelevantly from the sync_stop stuff. The call of period_elapsed is from hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and this function expects the sleepable context due to snd_sof_dsp_read_poll_timeout() call.

So something like below works?

Takashi

--- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status) continue;
                    /* Inform ALSA only in case not do that with IPC */
                  if (sof_hda->no_ipc_position)
                          snd_sof_pcm_period_elapsed(s->substream);
                  if (sof_hda->no_ipc_position) {
                          spin_unlock_irq(&bus->reg_lock);
                          snd_pcm_period_elapsed(s->substream);
                          spin_lock_irq(&bus->reg_lock);

Thanks, Takashi. Yes, I realized it this morning as well that it is due to the reg_lock. It does work with this change now. I will run some stress tests with this change and get back with the results.

Thanks, Ranjani

Sridharan, Ranjani

21 Nov 21 Nov

7:22 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

...
...
...
I couldn't find anything obvious. Could you try without changing snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and calling sync_stop, in order to see whether the additional stuff broke anything?

It is indeed the removal of snd_sof_pcm_period_elapsed() that makes the device hang when the stream is stoppped. But that's a bit surprising given that all I tried was using the snd_pcm_period_elapsed() directly instead of scheduling the delayed work to call it.

If I read the code correctly, this can't work irrelevantly from the sync_stop stuff. The call of period_elapsed is from hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in hda_dsp_stream_threaded_handler(). Meanwhile, the XRUN trigger goes to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and this function expects the sleepable context due to snd_sof_dsp_read_poll_timeout() call.

So something like below works?

Takashi

--- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status) continue;
                    /* Inform ALSA only in case not do that with IPC
*/
                  if (sof_hda->no_ipc_position)
                          snd_sof_pcm_period_elapsed(s->substream);
                  if (sof_hda->no_ipc_position) {
                          spin_unlock_irq(&bus->reg_lock);
                          snd_pcm_period_elapsed(s->substream);
                          spin_lock_irq(&bus->reg_lock);
Thanks, Takashi. Yes, I realized it this morning as well that it is due to the reg_lock. It does work with this change now. I will run some stress tests with this change and get back with the results.

Hi Takashi,

Sorry the stress tests took a while. As we discussed earlier, adding the sync_stop() op didnt quite help the SOF driver in removing the delayed work for snd_pcm_period_elapsed().

Thanks, Ranjani

...

Thanks, Ranjani

Takashi Iwai

8:34 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Thu, 21 Nov 2019 20:22:03 +0100, Sridharan, Ranjani wrote:

...

    > >
    > > I couldn't find anything obvious.  Could you try without changing
    > > snd_sof_pcm_period_elapsed(), i.e. only adding the stuff and
    calling
    > > sync_stop, in order to see whether the additional stuff broke
    > > anything?
    > It is indeed the removal of snd_sof_pcm_period_elapsed() that makes
    the
    > device hang when the stream is stoppped. But that's a bit surprising
    > given that all I tried was using the snd_pcm_period_elapsed()
    directly
    > instead of scheduling the delayed work to call it.

    If I read the code correctly, this can't work irrelevantly from the
    sync_stop stuff.  The call of period_elapsed is from
    hda_dsp_stream_check() which is performed in bus->reg_lock spinlock in
    hda_dsp_stream_threaded_handler().  Meanwhile, the XRUN trigger goes
    to hda_dsp_pcm_trigger() that follows hda_dsp_stream_trigger(), and
    this function expects the sleepable context due to
    snd_sof_dsp_read_poll_timeout() call.

    So something like below works?

    Takashi

    --- a/sound/soc/sof/intel/hda-stream.c
    +++ b/sound/soc/sof/intel/hda-stream.c
    @@ -592,8 +592,11 @@ static bool hda_dsp_stream_check(struct hdac_bus
    *bus, u32 status)
                                    continue;

                            /* Inform ALSA only in case not do that with
    IPC */
    -                       if (sof_hda->no_ipc_position)
    -                               snd_sof_pcm_period_elapsed(s->
    substream);
    +                       if (sof_hda->no_ipc_position) {
    +                               spin_unlock_irq(&bus->reg_lock);
    +                               snd_pcm_period_elapsed(s->substream);
    +                               spin_lock_irq(&bus->reg_lock);

Thanks, Takashi. Yes, I realized it this morning as well that it is due to
the reg_lock. It does work with this change now. I will run some stress
tests with this change and get back with the results.

Hi Takashi,

Sorry the stress tests took a while. As we discussed earlier, adding the sync_stop() op didnt quite help the SOF driver in removing the delayed work for snd_pcm_period_elapsed().

Yeah, that's understandable. If the stop operation itself needs some serialization, sync_stop() won't influence at all.

However, now after these discussions, I have some concerns in the current code:

- The async work started by schedule_work() may be executed (literally) immediately. So if the timing or the serialization matters, it doesn't guarantee at all. The same level of concurrency can happen at any time.

- The period_elapsed work might be pending at prepare or other operation; the async work means also that it doesn't guarantee its execution in time, and it might be delayed much, and the PCM core might go to prepare or other state even before the work is executed.

The second point can be fixed easily now with sync_stop. You can just put flush_work() in sync_stop in addition to synchronize_irq().

But the first point is still unclear. More exactly, which operation does it conflict? Does it the playback drain? Then it might take very long (up to seconds) to block the next operation?

thanks,

Takashi

Sridharan, Ranjani

8:46 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

...
Hi Takashi,

Sorry the stress tests took a while. As we discussed earlier, adding the sync_stop() op didnt quite help the

SOF

...
driver in removing the delayed work for snd_pcm_period_elapsed().

Yeah, that's understandable. If the stop operation itself needs some serialization, sync_stop() won't influence at all.

However, now after these discussions, I have some concerns in the current code:

The async work started by schedule_work() may be executed (literally) immediately. So if the timing or the serialization matters, it doesn't guarantee at all. The same level of concurrency can happen at any time.

The period_elapsed work might be pending at prepare or other operation; the async work means also that it doesn't guarantee its execution in time, and it might be delayed much, and the PCM core might go to prepare or other state even before the work is executed.

The second point can be fixed easily now with sync_stop. You can just put flush_work() in sync_stop in addition to synchronize_irq().

But the first point is still unclear. More exactly, which operation does it conflict? Does it the playback drain? Then it might take very long (up to seconds) to block the next operation?

Hi Takashi,

As I understand the original intention for adding the period_elapsed_work() was that snd_pcm_period_elapsed() could cause a STOP trigger while the current IPC interrupt is still being handled. In this case, the STOP trigger generates an IPC to the DSP but the host never misses the IPC response from the DSP because it is still handling the previous interrupt.

Adding Keyon who added this change to add more and clarify your concerns.

Thanks, Ranjani

...

thanks,

Takashi

Takashi Iwai

9:13 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Thu, 21 Nov 2019 21:46:17 +0100, Sridharan, Ranjani wrote:

...

>
> Hi Takashi,
>
> Sorry the stress tests took a while. 
> As we discussed earlier, adding the sync_stop() op didnt quite help the
SOF
> driver in removing the delayed work for snd_pcm_period_elapsed(). 

Yeah, that's understandable.  If the stop operation itself needs some
serialization, sync_stop() won't influence at all.

However, now after these discussions, I have some concerns in the
current code:

- The async work started by schedule_work() may be executed
  (literally) immediately.  So if the timing or the serialization
  matters, it doesn't guarantee at all.  The same level of concurrency
  can happen at any time.

- The period_elapsed work might be pending at prepare or other
  operation;
  the async work means also that it doesn't guarantee its execution in
  time, and it might be delayed much, and the PCM core might go to
  prepare or other state even before the work is executed.

The second point can be fixed easily now with sync_stop.  You can just
put flush_work() in sync_stop in addition to synchronize_irq().

But the first point is still unclear.  More exactly, which operation
does it conflict?  Does it the playback drain?  Then it might take
very long (up to seconds) to block the next operation?

Hi Takashi,

OK, that makes sense. So the issue is that the trigger stop itself requires the ack via the interrupt and it can't be caught because it's being called from the irq handler itself.

In that case, though, another solution would be to make the trigger- stop an async work (but conditionally) while processing the normal period_elapsed in the irq handler. That is, set some flag before calling snd_pcm_period_elapsed(), and in the trigger-stop, check the flag. If the flag is set, schedule the work and return. And, you'll sync this async work with sync_stop(). In that way, the period handling is processed without any delay more lightly.

thanks,

Takashi

...

Adding Keyon who added this change to add more and clarify your concerns.

Thanks, Ranjani
thanks,

Takashi

Sridharan, Ranjani

9:17 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai tiwai@suse.de wrote:

...

On Thu, 21 Nov 2019 21:46:17 +0100, Sridharan, Ranjani wrote:

...
>
> Hi Takashi,
>
> Sorry the stress tests took a while.
> As we discussed earlier, adding the sync_stop() op didnt quite
help the

...
SOF
> driver in removing the delayed work for snd_pcm_period_elapsed().

Yeah, that's understandable.  If the stop operation itself needs some
serialization, sync_stop() won't influence at all.

However, now after these discussions, I have some concerns in the
current code:

- The async work started by schedule_work() may be executed
  (literally) immediately.  So if the timing or the serialization
  matters, it doesn't guarantee at all.  The same level of
concurrency

...
  can happen at any time.

- The period_elapsed work might be pending at prepare or other
  operation;
  the async work means also that it doesn't guarantee its execution
in

...
  time, and it might be delayed much, and the PCM core might go to
  prepare or other state even before the work is executed.

The second point can be fixed easily now with sync_stop.  You can
just

...
put flush_work() in sync_stop in addition to synchronize_irq().

But the first point is still unclear.  More exactly, which operation
does it conflict?  Does it the playback drain?  Then it might take
very long (up to seconds) to block the next operation?
Hi Takashi,

As I understand the original intention for adding the
period_elapsed_work()

...
was that snd_pcm_period_elapsed() could cause a STOP trigger while the current IPC interrupt is still being handled. In this case, the STOP trigger generates an IPC to the DSP but the host

never

...
misses the IPC response from the DSP because it is still handling the

previous

...
interrupt.

OK, that makes sense. So the issue is that the trigger stop itself requires the ack via the interrupt and it can't be caught because it's being called from the irq handler itself.

In that case, though, another solution would be to make the trigger- stop an async work (but conditionally) while processing the normal period_elapsed in the irq handler. That is, set some flag before calling snd_pcm_period_elapsed(), and in the trigger-stop, check the flag. If the flag is set, schedule the work and return. And, you'll sync this async work with sync_stop(). In that way, the period handling is processed without any delay more lightly.

OK, that makes sense. Thanks for the suggestion. Regarding your previous comment about adding flush_work() to the sync_stop() op, would that still be required?

Thanks, Ranjani

...

thanks,

Takashi

...
Adding Keyon who added this change to add more and clarify your concerns.

Thanks, Ranjani
thanks,

Takashi

Takashi Iwai

9:28 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Thu, 21 Nov 2019 22:17:41 +0100, Sridharan, Ranjani wrote:

...

On Thu, Nov 21, 2019 at 1:13 PM Takashi Iwai tiwai@suse.de wrote:

On Thu, 21 Nov 2019 21:46:17 +0100,
Sridharan, Ranjani wrote:
>
>     >
>     > Hi Takashi,
>     >
>     > Sorry the stress tests took a while. 
>     > As we discussed earlier, adding the sync_stop() op didnt quite
help the
>     SOF
>     > driver in removing the delayed work for snd_pcm_period_elapsed(). 
>   
>     Yeah, that's understandable.  If the stop operation itself needs
some
>     serialization, sync_stop() won't influence at all.
>   
>     However, now after these discussions, I have some concerns in the
>     current code:
>   
>     - The async work started by schedule_work() may be executed
>       (literally) immediately.  So if the timing or the serialization
>       matters, it doesn't guarantee at all.  The same level of
concurrency
>       can happen at any time.
>   
>     - The period_elapsed work might be pending at prepare or other
>       operation;
>       the async work means also that it doesn't guarantee its execution
in
>       time, and it might be delayed much, and the PCM core might go to
>       prepare or other state even before the work is executed.
>   
>     The second point can be fixed easily now with sync_stop.  You can
just
>     put flush_work() in sync_stop in addition to synchronize_irq().
>   
>     But the first point is still unclear.  More exactly, which operation
>     does it conflict?  Does it the playback drain?  Then it might take
>     very long (up to seconds) to block the next operation?
>
> Hi Takashi,
>
> As I understand the original intention for adding the
period_elapsed_work()
> was  that snd_pcm_period_elapsed() could cause a STOP trigger while the
> current IPC interrupt is still being handled.
> In this case, the STOP trigger generates an IPC to the DSP but the host
never
> misses the IPC response from the DSP because it is still handling the
previous
> interrupt.

OK, that makes sense.  So the issue is that the trigger stop itself
requires the ack via the interrupt and it can't be caught because it's
being called from the irq handler itself.

In that case, though, another solution would be to make the trigger-
stop an async work (but conditionally) while processing the normal
period_elapsed in the irq handler.  That is, set some flag before
calling snd_pcm_period_elapsed(), and in the trigger-stop, check the
flag.  If the flag is set, schedule the work and return.  And, you'll
sync this async work with sync_stop().  In that way, the period
handling is processed without any delay more lightly.

OK, that makes sense. Thanks for the suggestion. Regarding your previous comment about adding flush_work() to the sync_stop() op, would that still be required?

Yes, that's needed no matter which way is used; the pending work must be synced at some point.

Takashi

Sridharan, Ranjani

9:45 p.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

...
OK, that makes sense. Thanks for the suggestion. Regarding your previous comment about adding flush_work() to the

sync_stop()

...
op, would that still be required?

Yes, that's needed no matter which way is used; the pending work must be synced at some point.

Thanks, Takashi. I will follow up with the suggested SOF changes after your patches have been applied.

Thanks, Ranjani

Jie, Yang

22 Nov 22 Nov

4:08 a.m.

New subject: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

...

-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Thursday, November 21, 2019 1:13 PM To: Sridharan, Ranjani ranjani.sridharan@intel.com Cc: Jie, Yang yang.jie@intel.com; Ranjani Sridharan ranjani.sridharan@linux.intel.com; Linux-ALSA alsa-devel@alsa-project.org; Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Subject: Re: [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field

On Thu, 21 Nov 2019 21:46:17 +0100, Sridharan, Ranjani wrote:

...
>
> Hi Takashi,
>
> Sorry the stress tests took a while.
> As we discussed earlier, adding the sync_stop() op didnt quite help the
SOF
> driver in removing the delayed work for
snd_pcm_period_elapsed().
Yeah, that's understandable.  If the stop operation itself needs some
serialization, sync_stop() won't influence at all.

However, now after these discussions, I have some concerns in the
current code:

- The async work started by schedule_work() may be executed
  (literally) immediately.  So if the timing or the serialization
  matters, it doesn't guarantee at all.  The same level of concurrency
  can happen at any time.

- The period_elapsed work might be pending at prepare or other
  operation;
  the async work means also that it doesn't guarantee its execution in
  time, and it might be delayed much, and the PCM core might go to
  prepare or other state even before the work is executed.

The second point can be fixed easily now with sync_stop.  You can just
put flush_work() in sync_stop in addition to synchronize_irq().

But the first point is still unclear.  More exactly, which operation
does it conflict?  Does it the playback drain?  Then it might take
very long (up to seconds) to block the next operation?
Hi Takashi,

As I understand the original intention for adding the period_elapsed_work() was that snd_pcm_period_elapsed() could cause a STOP trigger while the current IPC interrupt is still being handled. In this case, the STOP trigger generates an IPC to the DSP but the host never misses the IPC response from the DSP because it is still handling the previous interrupt.
OK, that makes sense. So the issue is that the trigger stop itself requires the ack via the interrupt and it can't be caught because it's being called from the irq handler itself.

In that case, though, another solution would be to make the trigger- stop an async work (but conditionally) while processing the normal period_elapsed in the irq handler. That is, set some flag before calling snd_pcm_period_elapsed(), and in the trigger-stop, check the flag. If the flag is set, schedule the work and return. And, you'll sync this async work with sync_stop(). In that way, the period handling is processed without any delay more lightly.

Yes, this was actually the method @Uimonen, Jaska suggested on April, since we have sync_stop() to flush the potential un-finished async trigger_stop task now, it's time to switch to use this solution now.

Thanks, ~Keyon

...

thanks,

Takashi

...
Adding Keyon who added this change to add more and clarify your concerns.

Thanks, Ranjani
thanks,

Takashi

Takashi Iwai

17 Nov 17 Nov

8:53 a.m.

New subject: [alsa-devel] [PATCH 8/8] ALSA: docs: Update about the new PCM sync_stop ops

Add the documentation about the new PCM sync_stop ops and card->sync_irq field.

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

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index 145bf6aad7cb..f169d58ca019 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -805,6 +805,7 @@ destructor and PCI entries. Example code is shown first, below. return -EBUSY; } chip->irq = pci->irq; + card->sync_irq = chip->irq;

/* (2) initialization of the chip hardware */ .... /* (not implemented in this document) */ @@ -965,6 +966,15 @@ usually like the following: return IRQ_HANDLED; }

+After requesting the IRQ, you can passed it to ``card->sync_irq`` +field: +:: + + card->irq = chip->irq; + +This allows PCM core automatically performing +:c:func:`synchronize_irq()` at the necessary timing like ``hw_free``. +See the later section `sync_stop callback`_ for details.

Now let's write the corresponding destructor for the resources above. The role of destructor is simple: disable the hardware (if already @@ -2059,6 +2069,37 @@ flag set, and you cannot call functions which may sleep. The triggering the DMA. The other stuff should be initialized ``hw_params`` and ``prepare`` callbacks properly beforehand.

+sync_stop callback +~~~~~~~~~~~~~~~~~~ + +:: + + static int snd_xxx_sync_stop(struct snd_pcm_substream *substream); + +This callback is optional, and NULL can be passed. It's called after +the PCM core stops the stream and changes the stream state +``prepare``, ``hw_params`` or ``hw_free``. +Since the IRQ handler might be still pending, we need to wait until +the pending task finishes before moving to the next step; otherwise it +might lead to a crash due to resource conflicts or access to the freed +resources. A typical behavior is to call a synchronization function +like :c:func:`synchronize_irq()` here. + +For majority of drivers that need only a call of +:c:func:`synchronize_irq()`, there is a simpler setup, too. +While keeping NULL to ``sync_stop`` PCM callback, the driver can set +``card->sync_irq`` field to store the valid interrupt number after +requesting an IRQ, instead. Then PCM core will look call +:c:func:`synchronize_irq()` with the given IRQ appropriately. + +If the IRQ handler is released at the card destructor, you don't need +to clear ``card->sync_irq``, as the card itself is being released. +So, usually you'll need to add just a single line for assigning +``card->sync_irq`` in the driver code unless the driver re-acquires +the IRQ. When the driver frees and re-acquires the IRQ dynamically +(e.g. for suspend/resume), it needs to clear and re-set +``card->sync_irq`` again appropriately. + pointer callback ~~~~~~~~~~~~~~~~

-- 2.16.4

1775

Age (days ago)

1780

Last active (days ago)

List overview

Download

37 comments

6 participants

tags (0)

participants (6)

Jie, Yang
kbuild test robot
Pierre-Louis Bossart
Ranjani Sridharan
Sridharan, Ranjani
Takashi Iwai