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

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

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

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

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