[alsa-devel] [PATCH 0/7] ALSA: hda: Enable HDAudio compress

[alsa-devel] [PATCH 0/7] ALSA: hda: Enable HDAudio compress

[Cezary Rojewski]

This set of patches provides new interfaces - page allocation - and
runtime flow adjustments - PM support - for compress operations.
For HDA part, work has been done to account for compress streams when
servicing IRQs, setting up BDLs and assigning DMAs.

End goal is to make room for one of DSP debug features: data probing.
It takes advantage of compress streams when extracting data from
running audio pipeline.

Initial review and development of probes can be found under:
https://github.com/thesofproject/linux/pull/1276

with this very set of patches being separated and reviewed on:
https://github.com/thesofproject/linux/pull/1571


Cezary Rojewski (7):
  ALSA: hda: Allow for compress stream to hdac_ext_stream assignment
  ALSA: hda: Prepare for compress stream support
  ALSA: hda: Interrupt servicing and BDL setup for compress streams
  ALSA: core: Expand DMA buffer information
  ALSA: core: Implement compress page allocation and free routines
  ASoC: compress: Add pm_runtime support
  ASoC: SOF: Intel: Account for compress streams when servicing IRQs

 include/sound/compress_driver.h  | 40 ++++++++++++++++++------
 include/sound/hdaudio.h          |  2 ++
 include/sound/hdaudio_ext.h      |  2 ++
 sound/core/compress_offload.c    | 42 ++++++++++++++++++++++++++
 sound/hda/ext/hdac_ext_stream.c  | 46 +++++++++++++++++++++++++---
 sound/hda/hdac_controller.c      |  4 +--
 sound/hda/hdac_stream.c          | 52 ++++++++++++++++++++------------
 sound/soc/soc-compress.c         | 29 +++++++++++++++++-
 sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++--
 9 files changed, 205 insertions(+), 38 deletions(-)

-- 
2.17.1

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

[alsa-devel] [PATCH 1/7] ALSA: hda: Allow for compress stream to hdac_ext_stream assignment

[Cezary Rojewski]

Currently only PCM streams can enlist hdac_stream for their data
transfer. Add cstream field to hdac_ext_stream to expose possibility of
compress stream assignment in place of PCM one.
Limited to HOST-type only.

Rather than copying entire hdac_ext_host_stream_assign, declare separate
PCM and compress wrappers and reuse it for both cases.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h         |  1 +
 include/sound/hdaudio_ext.h     |  2 ++
 sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++----
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index e05b95e83d5a..9a8bf1eb7d69 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -481,6 +481,7 @@ struct hdac_stream {
 	struct snd_pcm_substream *substream;	/* assigned substream,
 						 * set in PCM open
 						 */
+	struct snd_compr_stream *cstream;
 	unsigned int format_val;	/* format value to be set in the
 					 * controller and the codec
 					 */
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index ef88b20c7b0a..ec01f2024f0b 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -84,6 +84,8 @@ int snd_hdac_ext_stream_init_all(struct hdac_bus *bus, int start_idx,
 		int num_stream, int dir);
 void snd_hdac_stream_free_all(struct hdac_bus *bus);
 void snd_hdac_link_free_all(struct hdac_bus *bus);
+struct hdac_ext_stream *snd_hdac_ext_cstream_assign(struct hdac_bus *bus,
+					   struct snd_compr_stream *cstream);
 struct hdac_ext_stream *snd_hdac_ext_stream_assign(struct hdac_bus *bus,
 					   struct snd_pcm_substream *substream,
 					   int type);
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index 6b1b4b834bae..b898b6c0b55d 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -14,6 +14,7 @@
 #include <sound/pcm.h>
 #include <sound/hda_register.h>
 #include <sound/hdaudio_ext.h>
+#include <sound/compress_driver.h>
 
 /**
  * snd_hdac_ext_stream_init - initialize each stream (aka device)
@@ -281,8 +282,7 @@ hdac_ext_link_stream_assign(struct hdac_bus *bus,
 }
 
 static struct hdac_ext_stream *
-hdac_ext_host_stream_assign(struct hdac_bus *bus,
-				struct snd_pcm_substream *substream)
+hdac_ext_host_stream_assign(struct hdac_bus *bus, int direction)
 {
 	struct hdac_ext_stream *res = NULL;
 	struct hdac_stream *stream = NULL;
@@ -296,7 +296,7 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus,
 		struct hdac_ext_stream *hstream = container_of(stream,
 						struct hdac_ext_stream,
 						hstream);
-		if (stream->direction != substream->stream)
+		if (stream->direction != direction)
 			continue;
 
 		if (!stream->opened) {
@@ -310,13 +310,49 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus,
 		spin_lock_irq(&bus->reg_lock);
 		res->hstream.opened = 1;
 		res->hstream.running = 0;
-		res->hstream.substream = substream;
+		res->hstream.substream = NULL;
+		res->hstream.cstream = NULL;
 		spin_unlock_irq(&bus->reg_lock);
 	}
 
 	return res;
 }
 
+static struct hdac_ext_stream *
+hdac_ext_host_stream_pcm_assign(struct hdac_bus *bus,
+				struct snd_pcm_substream *substream)
+{
+	struct hdac_ext_stream *res;
+
+	res = hdac_ext_host_stream_assign(bus, substream->stream);
+	if (res)
+		res->hstream.substream = substream;
+
+	return res;
+}
+
+/**
+ * snd_hdac_ext_cstream_assign - assign a host stream for compress
+ * @bus: HD-audio core bus
+ * @cstream: Compress stream to assign
+ *
+ * Assign an unused host stream for the given compress stream.
+ * If no stream is free, NULL is returned. Stream is decoupled
+ * before assignment.
+ */
+struct hdac_ext_stream *snd_hdac_ext_cstream_assign(struct hdac_bus *bus,
+					   struct snd_compr_stream *cstream)
+{
+	struct hdac_ext_stream *res;
+
+	res = hdac_ext_host_stream_assign(bus, cstream->direction);
+	if (res)
+		res->hstream.cstream = cstream;
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_ext_cstream_assign);
+
 /**
  * snd_hdac_ext_stream_assign - assign a stream for the PCM
  * @bus: HD-audio core bus
@@ -350,7 +386,7 @@ struct hdac_ext_stream *snd_hdac_ext_stream_assign(struct hdac_bus *bus,
 		return hstream;
 
 	case HDAC_EXT_STREAM_TYPE_HOST:
-		return hdac_ext_host_stream_assign(bus, substream);
+		return hdac_ext_host_stream_pcm_assign(bus, substream);
 
 	case HDAC_EXT_STREAM_TYPE_LINK:
 		return hdac_ext_link_stream_assign(bus, substream);
-- 
2.17.1

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

[alsa-devel] [PATCH 2/7] ALSA: hda: Prepare for compress stream support

[Cezary Rojewski]

Before introducing compress specific changes, adjust BDL and parameters
setup functions so these are not tightly coupled with PCM streams.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/hda/hdac_stream.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index f9707fb05efe..d00c81e34cde 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -410,11 +410,15 @@ int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 {
 	struct hdac_bus *bus = azx_dev->bus;
 	struct snd_pcm_substream *substream = azx_dev->substream;
-	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_pcm_runtime *runtime;
+	struct snd_dma_buffer *dmab;
 	__le32 *bdl;
 	int i, ofs, periods, period_bytes;
 	int pos_adj, pos_align;
 
+	runtime = substream->runtime;
+	dmab = snd_pcm_get_dma_buf(substream);
+
 	/* reset BDL address */
 	snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
 	snd_hdac_stream_writel(azx_dev, SD_BDLPU, 0);
@@ -428,7 +432,7 @@ int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 	azx_dev->frags = 0;
 
 	pos_adj = bus->bdl_pos_adj;
-	if (!azx_dev->no_period_wakeup && pos_adj > 0) {
+	if (runtime && !azx_dev->no_period_wakeup && pos_adj > 0) {
 		pos_align = pos_adj;
 		pos_adj = (pos_adj * runtime->rate + 47999) / 48000;
 		if (!pos_adj)
@@ -442,8 +446,7 @@ int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 				 pos_adj);
 			pos_adj = 0;
 		} else {
-			ofs = setup_bdle(bus, snd_pcm_get_dma_buf(substream),
-					 azx_dev,
+			ofs = setup_bdle(bus, dmab, azx_dev,
 					 &bdl, ofs, pos_adj, true);
 			if (ofs < 0)
 				goto error;
@@ -453,13 +456,11 @@ int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 
 	for (i = 0; i < periods; i++) {
 		if (i == periods - 1 && pos_adj)
-			ofs = setup_bdle(bus, snd_pcm_get_dma_buf(substream),
-					 azx_dev, &bdl, ofs,
-					 period_bytes - pos_adj, 0);
+			ofs = setup_bdle(bus, dmab, azx_dev,
+					 &bdl, ofs, period_bytes - pos_adj, 0);
 		else
-			ofs = setup_bdle(bus, snd_pcm_get_dma_buf(substream),
-					 azx_dev, &bdl, ofs,
-					 period_bytes,
+			ofs = setup_bdle(bus, dmab, azx_dev,
+					 &bdl, ofs, period_bytes,
 					 !azx_dev->no_period_wakeup);
 		if (ofs < 0)
 			goto error;
@@ -484,26 +485,25 @@ EXPORT_SYMBOL_GPL(snd_hdac_stream_setup_periods);
 int snd_hdac_stream_set_params(struct hdac_stream *azx_dev,
 				 unsigned int format_val)
 {
-
-	unsigned int bufsize, period_bytes;
 	struct snd_pcm_substream *substream = azx_dev->substream;
-	struct snd_pcm_runtime *runtime;
+	unsigned int bufsize, period_bytes;
+	unsigned int no_period_wakeup;
 	int err;
 
 	if (!substream)
 		return -EINVAL;
-	runtime = substream->runtime;
 	bufsize = snd_pcm_lib_buffer_bytes(substream);
 	period_bytes = snd_pcm_lib_period_bytes(substream);
+	no_period_wakeup = substream->runtime->no_period_wakeup;
 
 	if (bufsize != azx_dev->bufsize ||
 	    period_bytes != azx_dev->period_bytes ||
 	    format_val != azx_dev->format_val ||
-	    runtime->no_period_wakeup != azx_dev->no_period_wakeup) {
+	    no_period_wakeup != azx_dev->no_period_wakeup) {
 		azx_dev->bufsize = bufsize;
 		azx_dev->period_bytes = period_bytes;
 		azx_dev->format_val = format_val;
-		azx_dev->no_period_wakeup = runtime->no_period_wakeup;
+		azx_dev->no_period_wakeup = no_period_wakeup;
 		err = snd_hdac_stream_setup_periods(azx_dev);
 		if (err < 0)
 			return err;
-- 
2.17.1

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

[alsa-devel] [PATCH 3/7] ALSA: hda: Interrupt servicing and BDL setup for compress streams

[Cezary Rojewski]

Account for compress streams when receiving and servicing buffer
completed interrupts. In case of compress stream enlisting hdac_stream
for data transfer, setup BDL entries much like it is the case for PCM
streams.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Divya Prakash <divya1.prakash@intel.com>
---
 sound/hda/hdac_controller.c |  4 ++--
 sound/hda/hdac_stream.c     | 26 ++++++++++++++++++++------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 7e7be8e4dcf9..585908f58028 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -553,8 +553,8 @@ int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned int status,
 			sd_status = snd_hdac_stream_readb(azx_dev, SD_STS);
 			snd_hdac_stream_writeb(azx_dev, SD_STS, SD_INT_MASK);
 			handled |= 1 << azx_dev->index;
-			if (!azx_dev->substream || !azx_dev->running ||
-			    !(sd_status & SD_INT_COMPLETE))
+			if ((!azx_dev->substream && !azx_dev->cstream) ||
+			    !azx_dev->running || !(sd_status & SD_INT_COMPLETE))
 				continue;
 			if (ack)
 				ack(bus, azx_dev);
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index d00c81e34cde..bf6795d73bcd 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -7,6 +7,7 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/clocksource.h>
+#include <sound/compress_driver.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/hdaudio.h>
@@ -410,14 +411,20 @@ int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
 {
 	struct hdac_bus *bus = azx_dev->bus;
 	struct snd_pcm_substream *substream = azx_dev->substream;
+	struct snd_compr_stream *cstream = azx_dev->cstream;
 	struct snd_pcm_runtime *runtime;
 	struct snd_dma_buffer *dmab;
 	__le32 *bdl;
 	int i, ofs, periods, period_bytes;
 	int pos_adj, pos_align;
 
-	runtime = substream->runtime;
-	dmab = snd_pcm_get_dma_buf(substream);
+	if (substream) {
+		runtime = substream->runtime;
+		dmab = snd_pcm_get_dma_buf(substream);
+	} else {
+		runtime = NULL;
+		dmab = snd_pcm_get_dma_buf(cstream);
+	}
 
 	/* reset BDL address */
 	snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
@@ -486,15 +493,22 @@ int snd_hdac_stream_set_params(struct hdac_stream *azx_dev,
 				 unsigned int format_val)
 {
 	struct snd_pcm_substream *substream = azx_dev->substream;
+	struct snd_compr_stream *cstream = azx_dev->cstream;
 	unsigned int bufsize, period_bytes;
 	unsigned int no_period_wakeup;
 	int err;
 
-	if (!substream)
+	if (substream) {
+		bufsize = snd_pcm_lib_buffer_bytes(substream);
+		period_bytes = snd_pcm_lib_period_bytes(substream);
+		no_period_wakeup = substream->runtime->no_period_wakeup;
+	} else if (cstream) {
+		bufsize = cstream->runtime->buffer_size;
+		period_bytes = cstream->runtime->fragment_size;
+		no_period_wakeup = 0;
+	} else {
 		return -EINVAL;
-	bufsize = snd_pcm_lib_buffer_bytes(substream);
-	period_bytes = snd_pcm_lib_period_bytes(substream);
-	no_period_wakeup = substream->runtime->no_period_wakeup;
+	}
 
 	if (bufsize != azx_dev->bufsize ||
 	    period_bytes != azx_dev->period_bytes ||
-- 
2.17.1

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

[alsa-devel] [PATCH 4/7] ALSA: core: Expand DMA buffer information

[Cezary Rojewski]

Update DMA buffer definition for snd_compr_runtime so it is represented
similarly as in snd_pcm_runtime. While at it, modify
snd_compr_set_runtime_buffer to account for newly added members.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/compress_driver.h | 35 ++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index bc88d6f964da..00f633c0c3ba 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -23,7 +23,6 @@ struct snd_compr_ops;
  * struct snd_compr_runtime: runtime stream description
  * @state: stream state
  * @ops: pointer to DSP callbacks
- * @dma_buffer_p: runtime dma buffer pointer
  * @buffer: pointer to kernel buffer, valid only when not in mmap mode or
  *	DSP doesn't implement copy
  * @buffer_size: size of the above buffer
@@ -34,11 +33,14 @@ struct snd_compr_ops;
  * @total_bytes_transferred: cumulative bytes transferred by offload DSP
  * @sleep: poll sleep
  * @private_data: driver private data pointer
+ * @dma_area: virtual buffer address
+ * @dma_addr: physical buffer address (not accessible from main CPU)
+ * @dma_bytes: size of DMA area
+ * @dma_buffer_p: runtime dma buffer pointer
  */
 struct snd_compr_runtime {
 	snd_pcm_state_t state;
 	struct snd_compr_ops *ops;
-	struct snd_dma_buffer *dma_buffer_p;
 	void *buffer;
 	u64 buffer_size;
 	u32 fragment_size;
@@ -47,6 +49,11 @@ struct snd_compr_runtime {
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
 	void *private_data;
+
+	unsigned char *dma_area;
+	dma_addr_t dma_addr;
+	size_t dma_bytes;
+	struct snd_dma_buffer *dma_buffer_p;
 };
 
 /**
@@ -180,19 +187,29 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 
 /**
  * snd_compr_set_runtime_buffer - Set the Compress runtime buffer
- * @substream: compress substream to set
+ * @stream: compress stream to set
  * @bufp: the buffer information, NULL to clear
  *
  * Copy the buffer information to runtime buffer when @bufp is non-NULL.
  * Otherwise it clears the current buffer information.
  */
-static inline void snd_compr_set_runtime_buffer(
-					struct snd_compr_stream *substream,
-					struct snd_dma_buffer *bufp)
+static inline void
+snd_compr_set_runtime_buffer(struct snd_compr_stream *stream,
+			     struct snd_dma_buffer *bufp)
 {
-	struct snd_compr_runtime *runtime = substream->runtime;
-
-	runtime->dma_buffer_p = bufp;
+	struct snd_compr_runtime *runtime = stream->runtime;
+
+	if (bufp) {
+		runtime->dma_buffer_p = bufp;
+		runtime->dma_area = bufp->area;
+		runtime->dma_addr = bufp->addr;
+		runtime->dma_bytes = bufp->bytes;
+	} else {
+		runtime->dma_buffer_p = NULL;
+		runtime->dma_area = NULL;
+		runtime->dma_addr = 0;
+		runtime->dma_bytes = 0;
+	}
 }
 
 int snd_compr_stop_error(struct snd_compr_stream *stream,
-- 
2.17.1

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

[alsa-devel] [PATCH 5/7] ALSA: core: Implement compress page allocation and free routines

[Cezary Rojewski]

Add simple malloc and free methods for memory management for compress
streams. Based on snd_pcm_lib_malloc_pages and snd_pcm_lib_free_pages
implementation.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Divya Prakash <divya1.prakash@intel.com>
---
 include/sound/compress_driver.h |  5 ++++
 sound/core/compress_offload.c   | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 00f633c0c3ba..6ce8effa0b12 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -67,6 +67,7 @@ struct snd_compr_runtime {
  * @metadata_set: metadata set flag, true when set
  * @next_track: has userspace signal next track transition, true when set
  * @private_data: pointer to DSP private data
+ * @dma_buffer: allocated buffer if any
  */
 struct snd_compr_stream {
 	const char *name;
@@ -78,6 +79,7 @@ struct snd_compr_stream {
 	bool metadata_set;
 	bool next_track;
 	void *private_data;
+	struct snd_dma_buffer dma_buffer;
 };
 
 /**
@@ -212,6 +214,9 @@ snd_compr_set_runtime_buffer(struct snd_compr_stream *stream,
 	}
 }
 
+int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size);
+int snd_compr_free_pages(struct snd_compr_stream *stream);
+
 int snd_compr_stop_error(struct snd_compr_stream *stream,
 			 snd_pcm_state_t state);
 
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index f34ce564d92c..dfb20ceb2d30 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -488,6 +488,48 @@ snd_compr_get_codec_caps(struct snd_compr_stream *stream, unsigned long arg)
 }
 #endif /* !COMPR_CODEC_CAPS_OVERFLOW */
 
+int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size)
+{
+	struct snd_dma_buffer *dmab;
+	int ret;
+
+	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
+		return -EINVAL;
+	dmab = kzalloc(sizeof(*dmab), GFP_KERNEL);
+	if (!dmab)
+		return -ENOMEM;
+	dmab->dev = stream->dma_buffer.dev;
+	ret = snd_dma_alloc_pages(dmab->dev.type, dmab->dev.dev, size, dmab);
+	if (ret < 0) {
+		kfree(dmab);
+		return ret;
+	}
+
+	snd_compr_set_runtime_buffer(stream, dmab);
+	stream->runtime->dma_bytes = size;
+	return 1;
+}
+EXPORT_SYMBOL(snd_compr_malloc_pages);
+
+int snd_compr_free_pages(struct snd_compr_stream *stream)
+{
+	struct snd_compr_runtime *runtime = stream->runtime;
+
+	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
+		return -EINVAL;
+	if (!runtime->dma_area)
+		return 0;
+	if (runtime->dma_buffer_p != &stream->dma_buffer) {
+		/* It's a newly allocated buffer. Release it now. */
+		snd_dma_free_pages(runtime->dma_buffer_p);
+		kfree(runtime->dma_buffer_p);
+	}
+
+	snd_compr_set_runtime_buffer(stream, NULL);
+	return 0;
+}
+EXPORT_SYMBOL(snd_compr_free_pages);
+
 /* revisit this with snd_pcm_preallocate_xxx */
 static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
 		struct snd_compr_params *params)
-- 
2.17.1

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

[alsa-devel] [PATCH 6/7] ASoC: compress: Add pm_runtime support

[Cezary Rojewski]

For some devices, components need to be powered-up before stream startup
sequence commences. Update soc_compr_open to provide such functionality.
Based on soc_pcm_open. Adjust soc_compr_free accordingly to power down
components once compress stream is closed.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/soc-compress.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 6615ef64c7f5..b2a5351b1a11 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -19,6 +19,7 @@
 #include <sound/soc.h>
 #include <sound/initval.h>
 #include <sound/soc-dpcm.h>
+#include <linux/pm_runtime.h>
 
 static int soc_compr_components_open(struct snd_compr_stream *cstream,
 				     struct snd_soc_component **last)
@@ -72,10 +73,20 @@ static int soc_compr_components_free(struct snd_compr_stream *cstream,
 static int soc_compr_open(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_component *component;
+	struct snd_soc_component *component, *save = NULL;
+	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	int ret;
 
+	for_each_rtd_components(rtd, rtdcom, component) {
+		ret = pm_runtime_get_sync(component->dev);
+		if (ret < 0 && ret != -EACCES) {
+			pm_runtime_put_noidle(component->dev);
+			save = component;
+			goto pm_err;
+		}
+	}
+
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
@@ -115,6 +126,14 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
 out:
 	mutex_unlock(&rtd->card->pcm_mutex);
+pm_err:
+	for_each_rtd_components(rtd, rtdcom, component) {
+		if (component == save)
+			break;
+		pm_runtime_mark_last_busy(component->dev);
+		pm_runtime_put_autosuspend(component->dev);
+	}
+
 	return ret;
 }
 
@@ -239,6 +258,8 @@ static void close_delayed_work(struct snd_soc_pcm_runtime *rtd)
 static int soc_compr_free(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_component *component;
+	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	int stream;
@@ -287,6 +308,12 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 	}
 
 	mutex_unlock(&rtd->card->pcm_mutex);
+
+	for_each_rtd_components(rtd, rtdcom, component) {
+		pm_runtime_mark_last_busy(component->dev);
+		pm_runtime_put_autosuspend(component->dev);
+	}
+
 	return 0;
 }
 
-- 
2.17.1

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

[alsa-devel] [PATCH 7/7] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

[Cezary Rojewski]

Update stream irq handler definition to correctly set hdac_stream
current position when servicing stream interrupts for compress streams.

Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h          |  1 +
 sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 9a8bf1eb7d69..9a24d57f0cf2 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -496,6 +496,7 @@ struct hdac_stream {
 	bool locked:1;
 	bool stripe:1;			/* apply stripe control */
 
+	unsigned long curr_pos;
 	/* timestamp */
 	unsigned long start_wallclk;	/* start + minimum wallclk */
 	unsigned long period_wallclk;	/* wallclk for period */
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index c0ab9bb2a797..ddf755a5a730 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
 	return ret;
 }
 
+static void hda_dsp_update_bytes_transferred(struct hdac_stream *hstream,
+					     u64 buffer_size)
+{
+	unsigned int prev_pos;
+	int pos, num_bytes;
+
+	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);
+	pos = snd_hdac_stream_get_pos_posbuf(hstream);
+
+	if (pos < prev_pos)
+		num_bytes = (buffer_size - prev_pos) +  pos;
+	else
+		num_bytes = pos - prev_pos;
+
+	hstream->curr_pos += num_bytes;
+}
+
 static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
 {
 	struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
@@ -588,14 +605,19 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
 			snd_hdac_stream_writeb(s, SD_STS, sd_status);
 
 			active = true;
-			if (!s->substream ||
+			if ((!s->substream && !s->cstream) ||
 			    !s->running ||
 			    (sd_status & SOF_HDA_CL_DMA_SD_INT_COMPLETE) == 0)
 				continue;
 
 			/* Inform ALSA only in case not do that with IPC */
-			if (sof_hda->no_ipc_position)
+			if (s->substream && sof_hda->no_ipc_position) {
 				snd_sof_pcm_period_elapsed(s->substream);
+			} else if (s->cstream) {
+				hda_dsp_update_bytes_transferred(s,
+					s->cstream->runtime->buffer_size);
+				snd_compr_fragment_elapsed(s->cstream);
+			}
 		}
 	}
 
-- 
2.17.1

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

Re: [alsa-devel] [PATCH 1/7] ALSA: hda: Allow for compress stream to hdac_ext_stream assignment

[Takashi Iwai]

On Tue, 17 Dec 2019 10:58:45 +0100,
Cezary Rojewski wrote:
> 
> Currently only PCM streams can enlist hdac_stream for their data
> transfer. Add cstream field to hdac_ext_stream to expose possibility of
> compress stream assignment in place of PCM one.
> Limited to HOST-type only.
> 
> Rather than copying entire hdac_ext_host_stream_assign, declare separate
> PCM and compress wrappers and reuse it for both cases.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio.h         |  1 +
>  include/sound/hdaudio_ext.h     |  2 ++
>  sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++----
>  3 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index e05b95e83d5a..9a8bf1eb7d69 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -481,6 +481,7 @@ struct hdac_stream {
>  	struct snd_pcm_substream *substream;	/* assigned substream,
>  						 * set in PCM open
>  						 */
> +	struct snd_compr_stream *cstream;
>  	unsigned int format_val;	/* format value to be set in the
>  					 * controller and the codec
>  					 */

One might use union for pointing to either PCM or compr stream and
identify the type with some flag.

	struct hdac_stream {
		union {
			struct snd_pcm_substream *substream;
			struct snd_compr_stream *cstream;
		};
		bool is_compr;
		....

But, I'm not advocating for this.  Although this makes the stream
assignment more handy, it might lead to refer to a wrong object if you
don't check the flag properly, too.  It really depends on the code.


thanks,

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

Re: [alsa-devel] [PATCH 4/7] ALSA: core: Expand DMA buffer information

[Takashi Iwai]

On Tue, 17 Dec 2019 10:58:48 +0100,
Cezary Rojewski wrote:
> 
> Update DMA buffer definition for snd_compr_runtime so it is represented
> similarly as in snd_pcm_runtime. While at it, modify
> snd_compr_set_runtime_buffer to account for newly added members.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/compress_driver.h | 35 ++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index bc88d6f964da..00f633c0c3ba 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -23,7 +23,6 @@ struct snd_compr_ops;
>   * struct snd_compr_runtime: runtime stream description
>   * @state: stream state
>   * @ops: pointer to DSP callbacks
> - * @dma_buffer_p: runtime dma buffer pointer
>   * @buffer: pointer to kernel buffer, valid only when not in mmap mode or
>   *	DSP doesn't implement copy
>   * @buffer_size: size of the above buffer
> @@ -34,11 +33,14 @@ struct snd_compr_ops;
>   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>   * @sleep: poll sleep
>   * @private_data: driver private data pointer
> + * @dma_area: virtual buffer address
> + * @dma_addr: physical buffer address (not accessible from main CPU)
> + * @dma_bytes: size of DMA area
> + * @dma_buffer_p: runtime dma buffer pointer
>   */
>  struct snd_compr_runtime {
>  	snd_pcm_state_t state;
>  	struct snd_compr_ops *ops;
> -	struct snd_dma_buffer *dma_buffer_p;
>  	void *buffer;
>  	u64 buffer_size;
>  	u32 fragment_size;
> @@ -47,6 +49,11 @@ struct snd_compr_runtime {
>  	u64 total_bytes_transferred;
>  	wait_queue_head_t sleep;
>  	void *private_data;
> +
> +	unsigned char *dma_area;
> +	dma_addr_t dma_addr;
> +	size_t dma_bytes;
> +	struct snd_dma_buffer *dma_buffer_p;

Why do we need to have both dma_buffer_p and its values?
For consistency with PCM stream?

For PCM, dma_area, dma_addr and dma_bytes are the primary data, which
aren't necessarily set by the dma_buffer but manually set up by the
driver.

Just wondering.


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

Re: [alsa-devel] [PATCH 5/7] ALSA: core: Implement compress page allocation and free routines

[Takashi Iwai]

On Tue, 17 Dec 2019 10:58:49 +0100,
Cezary Rojewski wrote:
> 
> Add simple malloc and free methods for memory management for compress
> streams. Based on snd_pcm_lib_malloc_pages and snd_pcm_lib_free_pages
> implementation.

I see no user of these functions in the series.  How these are
supposed to be used?


Takashi

> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Signed-off-by: Divya Prakash <divya1.prakash@intel.com>
> ---
>  include/sound/compress_driver.h |  5 ++++
>  sound/core/compress_offload.c   | 42 +++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 00f633c0c3ba..6ce8effa0b12 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -67,6 +67,7 @@ struct snd_compr_runtime {
>   * @metadata_set: metadata set flag, true when set
>   * @next_track: has userspace signal next track transition, true when set
>   * @private_data: pointer to DSP private data
> + * @dma_buffer: allocated buffer if any
>   */
>  struct snd_compr_stream {
>  	const char *name;
> @@ -78,6 +79,7 @@ struct snd_compr_stream {
>  	bool metadata_set;
>  	bool next_track;
>  	void *private_data;
> +	struct snd_dma_buffer dma_buffer;
>  };
>  
>  /**
> @@ -212,6 +214,9 @@ snd_compr_set_runtime_buffer(struct snd_compr_stream *stream,
>  	}
>  }
>  
> +int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size);
> +int snd_compr_free_pages(struct snd_compr_stream *stream);
> +
>  int snd_compr_stop_error(struct snd_compr_stream *stream,
>  			 snd_pcm_state_t state);
>  
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index f34ce564d92c..dfb20ceb2d30 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -488,6 +488,48 @@ snd_compr_get_codec_caps(struct snd_compr_stream *stream, unsigned long arg)
>  }
>  #endif /* !COMPR_CODEC_CAPS_OVERFLOW */
>  
> +int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size)
> +{
> +	struct snd_dma_buffer *dmab;
> +	int ret;
> +
> +	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
> +		return -EINVAL;
> +	dmab = kzalloc(sizeof(*dmab), GFP_KERNEL);
> +	if (!dmab)
> +		return -ENOMEM;
> +	dmab->dev = stream->dma_buffer.dev;
> +	ret = snd_dma_alloc_pages(dmab->dev.type, dmab->dev.dev, size, dmab);
> +	if (ret < 0) {
> +		kfree(dmab);
> +		return ret;
> +	}
> +
> +	snd_compr_set_runtime_buffer(stream, dmab);
> +	stream->runtime->dma_bytes = size;
> +	return 1;
> +}
> +EXPORT_SYMBOL(snd_compr_malloc_pages);
> +
> +int snd_compr_free_pages(struct snd_compr_stream *stream)
> +{
> +	struct snd_compr_runtime *runtime = stream->runtime;
> +
> +	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
> +		return -EINVAL;
> +	if (!runtime->dma_area)
> +		return 0;
> +	if (runtime->dma_buffer_p != &stream->dma_buffer) {
> +		/* It's a newly allocated buffer. Release it now. */
> +		snd_dma_free_pages(runtime->dma_buffer_p);
> +		kfree(runtime->dma_buffer_p);
> +	}
> +
> +	snd_compr_set_runtime_buffer(stream, NULL);
> +	return 0;
> +}
> +EXPORT_SYMBOL(snd_compr_free_pages);
> +
>  /* revisit this with snd_pcm_preallocate_xxx */
>  static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
>  		struct snd_compr_params *params)
> -- 
> 2.17.1
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 7/7] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

[Takashi Iwai]

On Tue, 17 Dec 2019 10:58:51 +0100,
Cezary Rojewski wrote:
> 
> Update stream irq handler definition to correctly set hdac_stream
> current position when servicing stream interrupts for compress streams.
> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio.h          |  1 +
>  sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++++++++++++--
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 9a8bf1eb7d69..9a24d57f0cf2 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -496,6 +496,7 @@ struct hdac_stream {
>  	bool locked:1;
>  	bool stripe:1;			/* apply stripe control */
>  
> +	unsigned long curr_pos;

Unless the actual implementation of this user is shown, it's a bit
hard to judge whether this addition is really OK or not....

I don't believe it'd be a big problem at all, but still we need the
usage implementation for a proper evaluation.

And, IMO, such a change should be split to two parts: the common API
change and its user.


thanks,

Takashi


>  	/* timestamp */
>  	unsigned long start_wallclk;	/* start + minimum wallclk */
>  	unsigned long period_wallclk;	/* wallclk for period */
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index c0ab9bb2a797..ddf755a5a730 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -571,6 +571,23 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
>  	return ret;
>  }
>  
> +static void hda_dsp_update_bytes_transferred(struct hdac_stream *hstream,
> +					     u64 buffer_size)
> +{
> +	unsigned int prev_pos;
> +	int pos, num_bytes;
> +
> +	div_u64_rem(hstream->curr_pos, buffer_size, &prev_pos);
> +	pos = snd_hdac_stream_get_pos_posbuf(hstream);
> +
> +	if (pos < prev_pos)
> +		num_bytes = (buffer_size - prev_pos) +  pos;
> +	else
> +		num_bytes = pos - prev_pos;
> +
> +	hstream->curr_pos += num_bytes;
> +}
> +
>  static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
>  {
>  	struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
> @@ -588,14 +605,19 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
>  			snd_hdac_stream_writeb(s, SD_STS, sd_status);
>  
>  			active = true;
> -			if (!s->substream ||
> +			if ((!s->substream && !s->cstream) ||
>  			    !s->running ||
>  			    (sd_status & SOF_HDA_CL_DMA_SD_INT_COMPLETE) == 0)
>  				continue;
>  
>  			/* Inform ALSA only in case not do that with IPC */
> -			if (sof_hda->no_ipc_position)
> +			if (s->substream && sof_hda->no_ipc_position) {
>  				snd_sof_pcm_period_elapsed(s->substream);
> +			} else if (s->cstream) {
> +				hda_dsp_update_bytes_transferred(s,
> +					s->cstream->runtime->buffer_size);
> +				snd_compr_fragment_elapsed(s->cstream);
> +			}
>  		}
>  	}
>  
> -- 
> 2.17.1
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/7] ALSA: hda: Allow for compress stream to hdac_ext_stream assignment

[Cezary Rojewski]

On 2019-12-17 11:19, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:45 +0100,
> Cezary Rojewski wrote:
>>
>> Currently only PCM streams can enlist hdac_stream for their data
>> transfer. Add cstream field to hdac_ext_stream to expose possibility of
>> compress stream assignment in place of PCM one.
>> Limited to HOST-type only.
>>
>> Rather than copying entire hdac_ext_host_stream_assign, declare separate
>> PCM and compress wrappers and reuse it for both cases.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   include/sound/hdaudio.h         |  1 +
>>   include/sound/hdaudio_ext.h     |  2 ++
>>   sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++----
>>   3 files changed, 44 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
>> index e05b95e83d5a..9a8bf1eb7d69 100644
>> --- a/include/sound/hdaudio.h
>> +++ b/include/sound/hdaudio.h
>> @@ -481,6 +481,7 @@ struct hdac_stream {
>>   	struct snd_pcm_substream *substream;	/* assigned substream,
>>   						 * set in PCM open
>>   						 */
>> +	struct snd_compr_stream *cstream;
>>   	unsigned int format_val;	/* format value to be set in the
>>   					 * controller and the codec
>>   					 */
> 
> One might use union for pointing to either PCM or compr stream and
> identify the type with some flag.
> 
> 	struct hdac_stream {
> 		union {
> 			struct snd_pcm_substream *substream;
> 			struct snd_compr_stream *cstream;
> 		};
> 		bool is_compr;
> 		....
> 
> But, I'm not advocating for this.  Although this makes the stream
> assignment more handy, it might lead to refer to a wrong object if you
> don't check the flag properly, too.  It really depends on the code.
> 

I'm happy with both - existing - and your variant. In essence, this 
causes simply: s/if (hstream->cstream)/if (hstream->is_compr)/g to occur.

In general, I'm strong supporter of a "PCM-compr marriage" idea - both 
being combined in sense of having similar base in the future so one 
could make use of "snd_base_stream", checkout the is_compr field and 
cast into actual type (_pcm_ -or- _compr_) via container_of macro.

This is more of a wish or song of the future for now, though. Compress 
and PCM ops streamlining is not within the scope of probes and requires 
much more work : )

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

Re: [alsa-devel] [PATCH 4/7] ALSA: core: Expand DMA buffer information

[Cezary Rojewski]

On 2019-12-17 11:23, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:48 +0100,
> Cezary Rojewski wrote:
>>
>> Update DMA buffer definition for snd_compr_runtime so it is represented
>> similarly as in snd_pcm_runtime. While at it, modify
>> snd_compr_set_runtime_buffer to account for newly added members.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   include/sound/compress_driver.h | 35 ++++++++++++++++++++++++---------
>>   1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
>> index bc88d6f964da..00f633c0c3ba 100644
>> --- a/include/sound/compress_driver.h
>> +++ b/include/sound/compress_driver.h
>> @@ -23,7 +23,6 @@ struct snd_compr_ops;
>>    * struct snd_compr_runtime: runtime stream description
>>    * @state: stream state
>>    * @ops: pointer to DSP callbacks
>> - * @dma_buffer_p: runtime dma buffer pointer
>>    * @buffer: pointer to kernel buffer, valid only when not in mmap mode or
>>    *	DSP doesn't implement copy
>>    * @buffer_size: size of the above buffer
>> @@ -34,11 +33,14 @@ struct snd_compr_ops;
>>    * @total_bytes_transferred: cumulative bytes transferred by offload DSP
>>    * @sleep: poll sleep
>>    * @private_data: driver private data pointer
>> + * @dma_area: virtual buffer address
>> + * @dma_addr: physical buffer address (not accessible from main CPU)
>> + * @dma_bytes: size of DMA area
>> + * @dma_buffer_p: runtime dma buffer pointer
>>    */
>>   struct snd_compr_runtime {
>>   	snd_pcm_state_t state;
>>   	struct snd_compr_ops *ops;
>> -	struct snd_dma_buffer *dma_buffer_p;
>>   	void *buffer;
>>   	u64 buffer_size;
>>   	u32 fragment_size;
>> @@ -47,6 +49,11 @@ struct snd_compr_runtime {
>>   	u64 total_bytes_transferred;
>>   	wait_queue_head_t sleep;
>>   	void *private_data;
>> +
>> +	unsigned char *dma_area;
>> +	dma_addr_t dma_addr;
>> +	size_t dma_bytes;
>> +	struct snd_dma_buffer *dma_buffer_p;
> 
> Why do we need to have both dma_buffer_p and its values?
> For consistency with PCM stream?
> 
> For PCM, dma_area, dma_addr and dma_bytes are the primary data, which
> aren't necessarily set by the dma_buffer but manually set up by the
> driver.
> 
> Just wondering.

Yeah, this is simply for consistency reasons. As referred to in previous 
reply, I'd like to see compr & PCM being tightly coupled in the future 
so separate page allocation functions are not required. Whether this is 
entirely possible or not I not know yet, although I'm an optimist when 
it comes to that subject.

If it indeed comes to that, having shared concepts and consistent API 
makes it much easier for one to refactor.

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

Re: [alsa-devel] [PATCH 5/7] ALSA: core: Implement compress page allocation and free routines

[Cezary Rojewski]

On 2019-12-17 11:24, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:49 +0100,
> Cezary Rojewski wrote:
>>
>> Add simple malloc and free methods for memory management for compress
>> streams. Based on snd_pcm_lib_malloc_pages and snd_pcm_lib_free_pages
>> implementation.
> 
> I see no user of these functions in the series.  How these are
> supposed to be used?
> 
> 
> Takashi
> 

I've given github links in the cover letter although I could have been 
more descriptive here too..

Probing pull request link:
https://github.com/thesofproject/linux/pull/1276/

Commits implementing compr flow:
- ASoC: SOF: Generic probe compress operations
https://github.com/thesofproject/linux/pull/1276/commits/217025f0fdad7523645a141ad2965dff4923a229

- ASoC: SOF: Intel: Probe compress operations
https://github.com/thesofproject/linux/pull/1276/commits/fb3e724a54bf859f039b2b0b03503a52e1740375

Methods: sof_probe_compr_set_params and sof_probe_compr_free of "ASoC: 
SOF: Generic probe compress operations" commit make use of these.

Basically it shares the concept of simple HDA PCM stream setup. During 
the development process, we decided to split the "introduction" and 
"usage" parts and thus this set of 7patches had spawned - covers the 
introduction. Actual probing patches take care of the "usage".

Czarek

>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> Signed-off-by: Divya Prakash <divya1.prakash@intel.com>
>> ---
>>   include/sound/compress_driver.h |  5 ++++
>>   sound/core/compress_offload.c   | 42 +++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
>> index 00f633c0c3ba..6ce8effa0b12 100644
>> --- a/include/sound/compress_driver.h
>> +++ b/include/sound/compress_driver.h
>> @@ -67,6 +67,7 @@ struct snd_compr_runtime {
>>    * @metadata_set: metadata set flag, true when set
>>    * @next_track: has userspace signal next track transition, true when set
>>    * @private_data: pointer to DSP private data
>> + * @dma_buffer: allocated buffer if any
>>    */
>>   struct snd_compr_stream {
>>   	const char *name;
>> @@ -78,6 +79,7 @@ struct snd_compr_stream {
>>   	bool metadata_set;
>>   	bool next_track;
>>   	void *private_data;
>> +	struct snd_dma_buffer dma_buffer;
>>   };
>>   
>>   /**
>> @@ -212,6 +214,9 @@ snd_compr_set_runtime_buffer(struct snd_compr_stream *stream,
>>   	}
>>   }
>>   
>> +int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size);
>> +int snd_compr_free_pages(struct snd_compr_stream *stream);
>> +
>>   int snd_compr_stop_error(struct snd_compr_stream *stream,
>>   			 snd_pcm_state_t state);
>>   
>> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
>> index f34ce564d92c..dfb20ceb2d30 100644
>> --- a/sound/core/compress_offload.c
>> +++ b/sound/core/compress_offload.c
>> @@ -488,6 +488,48 @@ snd_compr_get_codec_caps(struct snd_compr_stream *stream, unsigned long arg)
>>   }
>>   #endif /* !COMPR_CODEC_CAPS_OVERFLOW */
>>   
>> +int snd_compr_malloc_pages(struct snd_compr_stream *stream, size_t size)
>> +{
>> +	struct snd_dma_buffer *dmab;
>> +	int ret;
>> +
>> +	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
>> +		return -EINVAL;
>> +	dmab = kzalloc(sizeof(*dmab), GFP_KERNEL);
>> +	if (!dmab)
>> +		return -ENOMEM;
>> +	dmab->dev = stream->dma_buffer.dev;
>> +	ret = snd_dma_alloc_pages(dmab->dev.type, dmab->dev.dev, size, dmab);
>> +	if (ret < 0) {
>> +		kfree(dmab);
>> +		return ret;
>> +	}
>> +
>> +	snd_compr_set_runtime_buffer(stream, dmab);
>> +	stream->runtime->dma_bytes = size;
>> +	return 1;
>> +}
>> +EXPORT_SYMBOL(snd_compr_malloc_pages);
>> +
>> +int snd_compr_free_pages(struct snd_compr_stream *stream)
>> +{
>> +	struct snd_compr_runtime *runtime = stream->runtime;
>> +
>> +	if (snd_BUG_ON(!(stream) || !(stream)->runtime))
>> +		return -EINVAL;
>> +	if (!runtime->dma_area)
>> +		return 0;
>> +	if (runtime->dma_buffer_p != &stream->dma_buffer) {
>> +		/* It's a newly allocated buffer. Release it now. */
>> +		snd_dma_free_pages(runtime->dma_buffer_p);
>> +		kfree(runtime->dma_buffer_p);
>> +	}
>> +
>> +	snd_compr_set_runtime_buffer(stream, NULL);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(snd_compr_free_pages);
>> +
>>   /* revisit this with snd_pcm_preallocate_xxx */
>>   static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
>>   		struct snd_compr_params *params)
>> -- 
>> 2.17.1
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 7/7] ASoC: SOF: Intel: Account for compress streams when servicing IRQs

[Cezary Rojewski]

On 2019-12-17 11:30, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:51 +0100,
> Cezary Rojewski wrote:
>>
>> Update stream irq handler definition to correctly set hdac_stream
>> current position when servicing stream interrupts for compress streams.
>>
>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   include/sound/hdaudio.h          |  1 +
>>   sound/soc/sof/intel/hda-stream.c | 26 ++++++++++++++++++++++++--
>>   2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
>> index 9a8bf1eb7d69..9a24d57f0cf2 100644
>> --- a/include/sound/hdaudio.h
>> +++ b/include/sound/hdaudio.h
>> @@ -496,6 +496,7 @@ struct hdac_stream {
>>   	bool locked:1;
>>   	bool stripe:1;			/* apply stripe control */
>>   
>> +	unsigned long curr_pos;
> 
> Unless the actual implementation of this user is shown, it's a bit
> hard to judge whether this addition is really OK or not....
> 
> I don't believe it'd be a big problem at all, but still we need the
> usage implementation for a proper evaluation.
> 
> And, IMO, such a change should be split to two parts: the common API
> change and its user.
> 
> 
> thanks,
> 
> Takashi
> 

The actual user of this is _pointer op for hda compr stream.

Probing pull request link:
https://github.com/thesofproject/linux/pull/1276/

- ASoC: SOF: Intel: Probe compress operations
https://github.com/thesofproject/linux/pull/1276/commits/fb3e724a54bf859f039b2b0b03503a52e1740375

Method hda_probe_compr_pointer initializes 
snd_compr_tstamp::copied_total with hdac_stream::curr_pos value.

If this can be done better or in more elegant way, let's do so.

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

[alsa-devel] Applied "ASoC: compress: Add pm_runtime support" to the asoc tree

[Mark Brown]

The patch

   ASoC: compress: Add pm_runtime support

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 4137f4b65df7608e52f307f4aa9792b984bad7de Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Tue, 17 Dec 2019 10:58:50 +0100
Subject: [PATCH] ASoC: compress: Add pm_runtime support

For some devices, components need to be powered-up before stream startup
sequence commences. Update soc_compr_open to provide such functionality.
Based on soc_pcm_open. Adjust soc_compr_free accordingly to power down
components once compress stream is closed.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Link: https://lore.kernel.org/r/20191217095851.19629-7-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-compress.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 6615ef64c7f5..b2a5351b1a11 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -19,6 +19,7 @@
 #include <sound/soc.h>
 #include <sound/initval.h>
 #include <sound/soc-dpcm.h>
+#include <linux/pm_runtime.h>
 
 static int soc_compr_components_open(struct snd_compr_stream *cstream,
 				     struct snd_soc_component **last)
@@ -72,10 +73,20 @@ static int soc_compr_components_free(struct snd_compr_stream *cstream,
 static int soc_compr_open(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
-	struct snd_soc_component *component;
+	struct snd_soc_component *component, *save = NULL;
+	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	int ret;
 
+	for_each_rtd_components(rtd, rtdcom, component) {
+		ret = pm_runtime_get_sync(component->dev);
+		if (ret < 0 && ret != -EACCES) {
+			pm_runtime_put_noidle(component->dev);
+			save = component;
+			goto pm_err;
+		}
+	}
+
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
@@ -115,6 +126,14 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
 out:
 	mutex_unlock(&rtd->card->pcm_mutex);
+pm_err:
+	for_each_rtd_components(rtd, rtdcom, component) {
+		if (component == save)
+			break;
+		pm_runtime_mark_last_busy(component->dev);
+		pm_runtime_put_autosuspend(component->dev);
+	}
+
 	return ret;
 }
 
@@ -239,6 +258,8 @@ static void close_delayed_work(struct snd_soc_pcm_runtime *rtd)
 static int soc_compr_free(struct snd_compr_stream *cstream)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_component *component;
+	struct snd_soc_rtdcom_list *rtdcom;
 	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
 	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	int stream;
@@ -287,6 +308,12 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
 	}
 
 	mutex_unlock(&rtd->card->pcm_mutex);
+
+	for_each_rtd_components(rtd, rtdcom, component) {
+		pm_runtime_mark_last_busy(component->dev);
+		pm_runtime_put_autosuspend(component->dev);
+	}
+
 	return 0;
 }
 
-- 
2.20.1

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

Re: [alsa-devel] [PATCH 5/7] ALSA: core: Implement compress page allocation and free routines

[Pierre-Louis Bossart]

On 12/17/19 4:24 AM, Takashi Iwai wrote:
> On Tue, 17 Dec 2019 10:58:49 +0100,
> Cezary Rojewski wrote:
>>
>> Add simple malloc and free methods for memory management for compress
>> streams. Based on snd_pcm_lib_malloc_pages and snd_pcm_lib_free_pages
>> implementation.
> 
> I see no user of these functions in the series.  How these are
> supposed to be used?

I asked Cezary to post those patches on alsa-devel to make sure there is 
no disagreement on the initial solution.

The next steps would be probes (data injection/extraction in the DSP 
firmware) and compressed audio support for i.MX and Intel platforms.
Both would be coming in early Q1 next year.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH 1/7] ALSA: hda: Allow for compress stream to hdac_ext_stream assignment

[Cezary Rojewski]

On 2019-12-17 13:06, Cezary Rojewski wrote:
> 
> 
> On 2019-12-17 11:19, Takashi Iwai wrote:
>> On Tue, 17 Dec 2019 10:58:45 +0100,
>> Cezary Rojewski wrote:
>>>
>>> Currently only PCM streams can enlist hdac_stream for their data
>>> transfer. Add cstream field to hdac_ext_stream to expose possibility of
>>> compress stream assignment in place of PCM one.
>>> Limited to HOST-type only.
>>>
>>> Rather than copying entire hdac_ext_host_stream_assign, declare separate
>>> PCM and compress wrappers and reuse it for both cases.
>>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>   include/sound/hdaudio.h         |  1 +
>>>   include/sound/hdaudio_ext.h     |  2 ++
>>>   sound/hda/ext/hdac_ext_stream.c | 46 +++++++++++++++++++++++++++++----
>>>   3 files changed, 44 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
>>> index e05b95e83d5a..9a8bf1eb7d69 100644
>>> --- a/include/sound/hdaudio.h
>>> +++ b/include/sound/hdaudio.h
>>> @@ -481,6 +481,7 @@ struct hdac_stream {
>>>       struct snd_pcm_substream *substream;    /* assigned substream,
>>>                            * set in PCM open
>>>                            */
>>> +    struct snd_compr_stream *cstream;
>>>       unsigned int format_val;    /* format value to be set in the
>>>                        * controller and the codec
>>>                        */
>>
>> One might use union for pointing to either PCM or compr stream and
>> identify the type with some flag.
>>
>>     struct hdac_stream {
>>         union {
>>             struct snd_pcm_substream *substream;
>>             struct snd_compr_stream *cstream;
>>         };
>>         bool is_compr;
>>         ....
>>
>> But, I'm not advocating for this.  Although this makes the stream
>> assignment more handy, it might lead to refer to a wrong object if you
>> don't check the flag properly, too.  It really depends on the code.
>>
> 
> I'm happy with both - existing - and your variant. In essence, this 
> causes simply: s/if (hstream->cstream)/if (hstream->is_compr)/g to occur.
> 
> In general, I'm strong supporter of a "PCM-compr marriage" idea - both 
> being combined in sense of having similar base in the future so one 
> could make use of "snd_base_stream", checkout the is_compr field and 
> cast into actual type (_pcm_ -or- _compr_) via container_of macro.
> 
> This is more of a wish or song of the future for now, though. Compress 
> and PCM ops streamlining is not within the scope of probes and requires 
> much more work : )
> 

After thinking more about it, I'd rather stick to the current approach.

Patch 3 of the series ([PATCH 3/7] ALSA: hda: Interrupt servicing and 
BDL setup for compress streams):

(...)
  	/* reset BDL address */
  	snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
@@ -486,15 +493,22 @@ int snd_hdac_stream_set_params(struct hdac_stream 
*azx_dev,
  				 unsigned int format_val)
  {
  	struct snd_pcm_substream *substream = azx_dev->substream;
+	struct snd_compr_stream *cstream = azx_dev->cstream;
  	unsigned int bufsize, period_bytes;
  	unsigned int no_period_wakeup;
  	int err;

-	if (!substream)
+	if (substream) {
+		bufsize = snd_pcm_lib_buffer_bytes(substream);
+		period_bytes = snd_pcm_lib_period_bytes(substream);
+		no_period_wakeup = substream->runtime->no_period_wakeup;
+	} else if (cstream) {
+		bufsize = cstream->runtime->buffer_size;
+		period_bytes = cstream->runtime->fragment_size;
+		no_period_wakeup = 0;
+	} else {
  		return -EINVAL;
-	bufsize = snd_pcm_lib_buffer_bytes(substream);
-	period_bytes = snd_pcm_lib_period_bytes(substream);
-	no_period_wakeup = substream->runtime->no_period_wakeup;
+	}

  	if (bufsize != azx_dev->bufsize ||
  	    period_bytes != azx_dev->period_bytes ||

(...)

the if/ else if/ else block would have to be reorganized and start with 
pointer validity first (and return -EINVAL if evaluated to true), e.g.:
	if (!azx_dev->substream) {
		return -EINVAL;
	} else if (axz_dev->is_compr) {
		// compr stuff
	} else {
		// pcm stuff
	}

Now, with union { substream; cstream }; approach, this is valid but may 
be confusing for a reader - code checks for substream ptr _only_ as 
additional cstream-check would be redundant.

On the other hand:
	if (substream) {
		// pcm stuff
	} else if (cstream) {
		// compr stuff
	} else {
		return -EINVAL;
	}

is clear to everyone. It's true though that only one ptr may be assigned 
(substream -or- cstream) so union had its point too. I'd value 
readability over that, though.


With that said, I don't see any other suggestions for said series. 
Should I resend as v2 with no changes (minus "[PATCH 6/7] ASoC: 
compress: Add pm_runtime support" patch as it has already been accepted 
by Mark) or leave as is?

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