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

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

[Takashi Iwai]

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

 .../sound/kernel-api/writing-an-alsa-driver.rst    | 148 +++++++++++++++------
 include/sound/core.h                               |   1 +
 include/sound/pcm.h                                |  12 +-
 sound/core/init.c                                  |   1 +
 sound/core/pcm_local.h                             |   2 +
 sound/core/pcm_memory.c                            |  83 ++++++++++--
 sound/core/pcm_native.c                            |  48 ++++++-
 7 files changed, 237 insertions(+), 58 deletions(-)

-- 
2.16.4

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5939 bytes --]

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-and-extensions/20191117-170136
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28158 bytes --]

[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

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

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

[Takashi Iwai]

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-and-extensions/20191117-170136
> 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(-)

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 */
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

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6559 bytes --]

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-and-extensions/20191117-170136
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

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27868 bytes --]

[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

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

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

[Pierre-Louis Bossart]

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;
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Pierre-Louis Bossart]

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;
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Pierre-Louis Bossart]

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);
>   	}
>   }
>   
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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

 .../sound/kernel-api/writing-an-alsa-driver.rst    | 148 +++++++++++++++------
 .../gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c    |  48 ++++---
 drivers/media/pci/cobalt/cobalt-alsa-pcm.c         |  61 +--------
 drivers/media/pci/cx18/cx18-alsa-pcm.c             |  62 +--------
 drivers/media/pci/ivtv/ivtv-alsa-pcm.c             |  63 +--------
 drivers/media/pci/solo6x10/solo6x10-g723.c         |  23 +---
 drivers/media/pci/tw686x/tw686x-audio.c            |  15 +--
 drivers/media/usb/cx231xx/cx231xx-audio.c          |  78 +----------
 drivers/media/usb/em28xx/em28xx-audio.c            |  86 +-----------
 drivers/media/usb/go7007/snd-go7007.c              |  24 +---
 drivers/media/usb/tm6000/tm6000-alsa.c             |  81 +----------
 drivers/media/usb/usbtv/usbtv-audio.c              |  28 +---
 drivers/staging/most/sound/sound.c                 |  44 +-----
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c      |  17 +--
 drivers/usb/gadget/function/u_audio.c              |  18 +--
 include/sound/core.h                               |   1 +
 include/sound/pcm.h                                |  52 ++------
 sound/aoa/soundbus/i2sbus/pcm.c                    |  11 +-
 sound/arm/aaci.c                                   |  40 +++---
 sound/atmel/ac97c.c                                |  20 +--
 sound/core/init.c                                  |   1 +
 sound/core/pcm_local.h                             |   2 +
 sound/core/pcm_memory.c                            | 143 ++++++++++----------
 sound/core/pcm_native.c                            |  48 ++++++-
 sound/drivers/aloop.c                              |  12 +-
 sound/drivers/dummy.c                              |  14 +-
 sound/drivers/ml403-ac97cr.c                       |  29 +---
 sound/drivers/pcsp/pcsp_lib.c                      |  17 +--
 sound/drivers/vx/vx_pcm.c                          |  27 +---
 sound/firewire/bebob/bebob_pcm.c                   |  11 +-
 sound/firewire/dice/dice-pcm.c                     |  13 +-
 sound/firewire/digi00x/digi00x-pcm.c               |  11 +-
 sound/firewire/fireface/ff-pcm.c                   |   9 +-
 sound/firewire/fireworks/fireworks_pcm.c           |  11 +-
 sound/firewire/isight.c                            |  10 +-
 sound/firewire/motu/motu-pcm.c                     |  11 +-
 sound/firewire/oxfw/oxfw-pcm.c                     |  17 +--
 sound/firewire/tascam/tascam-pcm.c                 |  11 +-
 sound/isa/ad1816a/ad1816a_lib.c                    |  20 +--
 sound/isa/cmi8330.c                                |   5 +-
 sound/isa/es1688/es1688_lib.c                      |  20 +--
 sound/isa/es18xx.c                                 |  24 +---
 sound/isa/gus/gus_pcm.c                            |  28 ++--
 sound/isa/sb/sb16_main.c                           |  21 +--
 sound/isa/sb/sb8_main.c                            |  21 +--
 sound/isa/wss/wss_lib.c                            |  23 +---
 sound/mips/hal2.c                                  |  25 +---
 sound/mips/sgio2audio.c                            |  20 +--
 sound/parisc/harmony.c                             |  18 +--
 sound/pci/ad1889.c                                 |  24 +---
 sound/pci/ali5451/ali5451.c                        |  30 +----
 sound/pci/als300.c                                 |  23 +---
 sound/pci/als4000.c                                |  23 +---
 sound/pci/asihpi/asihpi.c                          |  10 +-
 sound/pci/atiixp.c                                 |  14 +-
 sound/pci/atiixp_modem.c                           |   9 +-
 sound/pci/au88x0/au88x0_pcm.c                      |  15 +--
 sound/pci/aw2/aw2-alsa.c                           |  45 ++-----
 sound/pci/azt3328.c                                |  30 +----
 sound/pci/bt87x.c                                  |  14 +-
 sound/pci/ca0106/ca0106_main.c                     |  56 +-------
 sound/pci/cmipci.c                                 |  36 ++---
 sound/pci/cs4281.c                                 |  20 +--
 sound/pci/cs5535audio/cs5535audio_pcm.c            |  12 +-
 sound/pci/ctxfi/ctpcm.c                            |  15 +--
 sound/pci/echoaudio/echoaudio.c                    |  19 +--
 sound/pci/emu10k1/emu10k1x.c                       |  15 +--
 sound/pci/emu10k1/emupcm.c                         |  41 +-----
 sound/pci/emu10k1/p16v.c                           |  48 +------
 sound/pci/ens1370.c                                |  27 +---
 sound/pci/es1938.c                                 |  28 +---
 sound/pci/fm801.c                                  |  20 +--
 sound/pci/hda/hda_controller.c                     |  13 +-
 sound/pci/ice1712/ice1712.c                        |  42 ++----
 sound/pci/ice1712/ice1724.c                        |  25 ++--
 sound/pci/intel8x0.c                               |  11 +-
 sound/pci/intel8x0m.c                              |  23 +---
 sound/pci/lola/lola_pcm.c                          |  11 +-
 sound/pci/lx6464es/lx6464es.c                      |  14 +-
 sound/pci/maestro3.c                               |   9 +-
 sound/pci/mixart/mixart.c                          |  14 +-
 sound/pci/oxygen/oxygen_pcm.c                      |  52 ++++----
 sound/pci/pcxhr/pcxhr.c                            |  31 +----
 sound/pci/riptide/riptide.c                        |  10 +-
 sound/pci/rme32.c                                  |  35 +----
 sound/pci/sis7019.c                                |  25 +---
 sound/pci/sonicvibes.c                             |  20 +--
 sound/pci/trident/trident_main.c                   |  49 +++----
 sound/pci/via82xx.c                                |  45 +++----
 sound/pci/via82xx_modem.c                          |   9 +-
 sound/pci/ymfpci/ymfpci_main.c                     |  33 ++---
 sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c             |  25 +---
 sound/ppc/pmac.c                                   |  28 +---
 sound/ppc/snd_ps3.c                                |  28 +---
 sound/sh/aica.c                                    |  29 +---
 sound/sh/sh_dac_audio.c                            |  20 +--
 sound/soc/amd/acp-pcm-dma.c                        |  58 +++-----
 sound/soc/amd/raven/acp3x-pcm-dma.c                |  30 +----
 sound/soc/au1x/dbdma2.c                            |  14 +-
 sound/soc/au1x/dma.c                               |  21 +--
 sound/soc/codecs/cros_ec_codec.c                   |   8 +-
 sound/soc/codecs/rt5514-spi.c                      |  10 +-
 sound/soc/codecs/rt5677-spi.c                      |  10 +-
 sound/soc/dwc/dwc-pcm.c                            |  24 +---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c       |  25 +---
 sound/soc/intel/baytrail/sst-baytrail-pcm.c        |  19 +--
 sound/soc/intel/haswell/sst-haswell-pcm.c          |  17 +--
 sound/soc/intel/skylake/skl-pcm.c                  |  26 +---
 sound/soc/mediatek/common/mtk-afe-fe-dai.c         |  14 +-
 sound/soc/mediatek/common/mtk-afe-fe-dai.h         |   2 -
 .../soc/mediatek/common/mtk-afe-platform-driver.c  |  12 +-
 .../soc/mediatek/common/mtk-afe-platform-driver.h  |   2 -
 sound/soc/mediatek/mt2701/mt2701-afe-pcm.c         |   2 -
 sound/soc/mediatek/mt6797/mt6797-afe-pcm.c         |   1 -
 sound/soc/mediatek/mt8183/mt8183-afe-pcm.c         |   1 -
 sound/soc/meson/axg-fifo.c                         |  13 +-
 sound/soc/sh/dma-sh7760.c                          |  14 +-
 sound/soc/sh/fsi.c                                 |  18 +--
 sound/soc/sh/rcar/core.c                           |  23 +---
 sound/soc/sh/siu_pcm.c                             |  39 +-----
 sound/soc/soc-generic-dmaengine-pcm.c              |  12 +-
 sound/soc/sof/pcm.c                                |  34 ++---
 sound/soc/stm/stm32_adfsdm.c                       |  29 +---
 sound/soc/txx9/txx9aclc.c                          |  14 +-
 sound/soc/uniphier/aio-dma.c                       |  30 +----
 sound/soc/xilinx/xlnx_formatter_pcm.c              |  13 +-
 sound/soc/xtensa/xtfpga-i2s.c                      |   9 +-
 sound/sparc/amd7930.c                              |  20 +--
 sound/sparc/cs4231.c                               |  17 +--
 sound/sparc/dbri.c                                 |  13 +-
 sound/spi/at73c213.c                               |  11 +-
 sound/usb/6fire/pcm.c                              |  17 +--
 sound/usb/caiaq/audio.c                            |  13 +-
 sound/usb/hiface/pcm.c                             |  18 +--
 sound/usb/line6/pcm.c                              |  13 +-
 sound/usb/misc/ua101.c                             |  23 +---
 sound/usb/pcm.c                                    |  15 +--
 sound/usb/usx2y/usbusx2yaudio.c                    |  26 ++--
 sound/usb/usx2y/usx2yhwdeppcm.c                    |  18 +--
 sound/x86/intel_hdmi_audio.c                       |  16 +--
 140 files changed, 781 insertions(+), 2700 deletions(-)

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

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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.
> 
> 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.

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


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

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

[Sridharan, Ranjani]

>
> >
> > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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:

 .../gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c    |  1 -
 drivers/media/pci/cobalt/cobalt-alsa-pcm.c         |  8 -----
 drivers/media/pci/cx18/cx18-alsa-pcm.c             | 13 --------
 drivers/media/pci/cx23885/cx23885-alsa.c           |  1 -
 drivers/media/pci/cx25821/cx25821-alsa.c           |  1 -
 drivers/media/pci/cx88/cx88-alsa.c                 |  1 -
 drivers/media/pci/ivtv/ivtv-alsa-pcm.c             | 13 --------
 drivers/media/pci/saa7134/saa7134-alsa.c           |  1 -
 drivers/media/pci/solo6x10/solo6x10-g723.c         |  1 -
 drivers/media/pci/tw686x/tw686x-audio.c            |  1 -
 drivers/media/usb/cx231xx/cx231xx-audio.c          |  1 -
 drivers/media/usb/em28xx/em28xx-audio.c            |  1 -
 drivers/media/usb/go7007/snd-go7007.c              |  1 -
 drivers/media/usb/tm6000/tm6000-alsa.c             |  1 -
 drivers/media/usb/usbtv/usbtv-audio.c              |  1 -
 drivers/staging/most/sound/sound.c                 |  1 -
 .../vc04_services/bcm2835-audio/bcm2835-pcm.c      |  2 --
 drivers/usb/gadget/function/u_audio.c              |  1 -
 include/sound/pcm.h                                |  2 --
 sound/aoa/soundbus/i2sbus/pcm.c                    |  2 --
 sound/arm/aaci.c                                   |  2 --
 sound/arm/pxa2xx-ac97.c                            |  1 -
 sound/atmel/ac97c.c                                |  2 --
 sound/core/pcm_local.h                             |  2 ++
 sound/core/pcm_memory.c                            | 16 ++-------
 sound/drivers/aloop.c                              |  1 -
 sound/drivers/dummy.c                              |  2 --
 sound/drivers/ml403-ac97cr.c                       |  2 --
 sound/drivers/pcsp/pcsp_lib.c                      |  1 -
 sound/drivers/vx/vx_pcm.c                          |  2 --
 sound/firewire/bebob/bebob_pcm.c                   |  2 --
 sound/firewire/dice/dice-pcm.c                     |  2 --
 sound/firewire/digi00x/digi00x-pcm.c               |  2 --
 sound/firewire/fireface/ff-pcm.c                   |  2 --
 sound/firewire/fireworks/fireworks_pcm.c           |  2 --
 sound/firewire/isight.c                            |  1 -
 sound/firewire/motu/motu-pcm.c                     |  2 --
 sound/firewire/oxfw/oxfw-pcm.c                     |  2 --
 sound/firewire/tascam/tascam-pcm.c                 |  2 --
 sound/isa/ad1816a/ad1816a_lib.c                    |  3 +-
 sound/isa/es1688/es1688_lib.c                      |  9 +----
 sound/isa/es18xx.c                                 |  3 +-
 sound/isa/gus/gus_main.c                           |  1 +
 sound/isa/gus/gus_pcm.c                            |  2 --
 sound/isa/gus/gusmax.c                             |  3 +-
 sound/isa/gus/interwave.c                          |  1 +
 sound/isa/msnd/msnd.c                              |  2 --
 sound/isa/msnd/msnd_pinnacle.c                     |  1 +
 sound/isa/opl3sa2.c                                |  1 +
 sound/isa/opti9xx/opti92x-ad1848.c                 |  1 +
 sound/isa/sb/emu8000_pcm.c                         |  1 -
 sound/isa/sb/sb16_main.c                           |  2 --
 sound/isa/sb/sb8_main.c                            |  2 --
 sound/isa/sb/sb_common.c                           |  1 +
 sound/isa/wavefront/wavefront.c                    |  1 +
 sound/isa/wss/wss_lib.c                            |  3 +-
 sound/mips/hal2.c                                  |  2 --
 sound/mips/sgio2audio.c                            |  3 --
 sound/parisc/harmony.c                             |  2 --
 sound/pci/ad1889.c                                 |  4 +--
 sound/pci/ali5451/ali5451.c                        |  7 +---
 sound/pci/als300.c                                 |  4 +--
 sound/pci/als4000.c                                |  2 --
 sound/pci/asihpi/asihpi.c                          | 17 ----------
 sound/pci/atiixp.c                                 |  5 +--
 sound/pci/atiixp_modem.c                           |  4 +--
 sound/pci/au88x0/au88x0.c                          |  1 +
 sound/pci/au88x0/au88x0_pcm.c                      |  1 -
 sound/pci/aw2/aw2-alsa.c                           |  3 +-
 sound/pci/azt3328.c                                |  5 +--
 sound/pci/bt87x.c                                  |  3 +-
 sound/pci/ca0106/ca0106_main.c                     |  9 +----
 sound/pci/cmipci.c                                 |  6 +---
 sound/pci/cs4281.c                                 |  7 +---
 sound/pci/cs46xx/cs46xx_lib.c                      | 11 +-----
 sound/pci/cs5535audio/cs5535audio.c                |  2 +-
 sound/pci/cs5535audio/cs5535audio_pcm.c            |  2 --
 sound/pci/ctxfi/cthw20k1.c                         |  4 +--
 sound/pci/ctxfi/cthw20k2.c                         |  1 +
 sound/pci/ctxfi/ctpcm.c                            |  2 --
 sound/pci/echoaudio/echoaudio.c                    |  7 ++--
 sound/pci/emu10k1/emu10k1_main.c                   |  1 +
 sound/pci/emu10k1/emu10k1x.c                       |  3 +-
 sound/pci/emu10k1/emupcm.c                         |  6 ----
 sound/pci/emu10k1/p16v.c                           |  2 --
 sound/pci/ens1370.c                                |  7 +---
 sound/pci/es1938.c                                 |  5 +--
 sound/pci/es1968.c                                 |  4 ---
 sound/pci/fm801.c                                  |  3 +-
 sound/pci/hda/hda_controller.c                     |  1 -
 sound/pci/hda/hda_intel.c                          |  4 ++-
 sound/pci/hda/hda_tegra.c                          |  4 +--
 sound/pci/ice1712/ice1712.c                        |  7 +---
 sound/pci/ice1712/ice1724.c                        |  7 +---
 sound/pci/intel8x0.c                               | 16 ++-------
 sound/pci/intel8x0m.c                              |  5 +--
 sound/pci/korg1212/korg1212.c                      |  1 +
 sound/pci/lola/lola.c                              |  2 +-
 sound/pci/lola/lola_pcm.c                          |  1 -
 sound/pci/lx6464es/lx6464es.c                      |  3 +-
 sound/pci/maestro3.c                               |  3 +-
 sound/pci/mixart/mixart.c                          |  3 +-
 sound/pci/nm256/nm256.c                            |  4 +--
 sound/pci/oxygen/oxygen_lib.c                      |  2 +-
 sound/pci/oxygen/oxygen_pcm.c                      |  6 ----
 sound/pci/pcxhr/pcxhr.c                            |  2 +-
 sound/pci/riptide/riptide.c                        |  3 +-
 sound/pci/rme32.c                                  |  9 +----
 sound/pci/rme96.c                                  |  5 +--
 sound/pci/rme9652/hdsp.c                           |  1 +
 sound/pci/rme9652/hdspm.c                          |  1 +
 sound/pci/rme9652/rme9652.c                        |  1 +
 sound/pci/sis7019.c                                |  3 +-
 sound/pci/sonicvibes.c                             |  3 +-
 sound/pci/trident/trident_main.c                   | 32 +-----------------
 sound/pci/via82xx.c                                |  8 +----
 sound/pci/via82xx_modem.c                          |  5 +--
 sound/pci/vx222/vx222.c                            |  1 +
 sound/pci/ymfpci/ymfpci_main.c                     |  6 +---
 sound/pcmcia/pdaudiocf/pdaudiocf.c                 |  1 +
 sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c             |  1 -
 sound/pcmcia/vx/vxpocket.c                         |  1 +
 sound/ppc/pmac.c                                   |  2 --
 sound/ppc/snd_ps3.c                                |  1 -
 sound/sh/aica.c                                    |  1 -
 sound/sh/sh_dac_audio.c                            |  1 -
 sound/sparc/amd7930.c                              |  2 --
 sound/sparc/cs4231.c                               |  2 --
 sound/sparc/dbri.c                                 |  1 -
 sound/spi/at73c213.c                               |  1 -
 sound/usb/6fire/pcm.c                              |  1 -
 sound/usb/caiaq/audio.c                            |  1 -
 sound/usb/hiface/pcm.c                             |  1 -
 sound/usb/line6/capture.c                          |  1 -
 sound/usb/line6/playback.c                         |  1 -
 sound/usb/misc/ua101.c                             |  2 --
 sound/usb/pcm.c                                    | 39 ++++++++++++++--------
 sound/usb/usx2y/usbusx2yaudio.c                    |  1 -
 sound/usb/usx2y/usx2yhwdeppcm.c                    |  1 -
 sound/x86/intel_hdmi_audio.c                       |  1 -
 sound/xen/xen_snd_front_alsa.c                     |  2 --
 141 files changed, 104 insertions(+), 394 deletions(-)


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

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

[Ranjani Sridharan]

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

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

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Ranjani Sridharan]

> > 
> > 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

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

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Ranjani Sridharan]

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

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

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

[Takashi Iwai]

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);
+			}
 		}
 	}
 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

>
> > >
>> > > 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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

>
>
> >
> > 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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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.


thanks,

Takashi

> Adding Keyon who added this change to add more and clarify your concerns.
> 
> Thanks,
> Ranjani
> 
>     thanks,
>    
>     Takashi
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

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
> >
> >
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Sridharan, Ranjani]

>
> >
> > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Jie, Yang]

>-----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
>>
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel