[PATCH] ASoC: SOF: compr: Add compress ops implementation

[PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Implement snd_compress_ops. There are a lot of similarities with
PCM implementation.

For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
firmware.

This will be changed in the future once we either add new compress
parameters to SOF or enhance existing sof_ipc_pcm_params structure
to support all native compress params.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/compress.c | 270 ++++++++++++++++++++++++++++++++++++++-
 sound/soc/sof/sof-priv.h |   6 +
 2 files changed, 271 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
index 01ca85f0b87f..2db1517d906d 100644
--- a/sound/soc/sof/compress.c
+++ b/sound/soc/sof/compress.c
@@ -10,6 +10,22 @@
 #include "sof-audio.h"
 #include "sof-priv.h"
 
+static void sof_set_transferred_bytes(struct sof_compr_stream *sstream,
+				      u64 host_pos, u64 buffer_size)
+{
+	u64 prev_pos;
+	unsigned int copied;
+
+	div64_u64_rem(sstream->copied_total, buffer_size, &prev_pos);
+
+	if (host_pos < prev_pos)
+		copied = (buffer_size - prev_pos) + host_pos;
+	else
+		copied = host_pos - prev_pos;
+
+	sstream->copied_total += copied;
+}
+
 static void snd_sof_compr_fragment_elapsed_work(struct work_struct *work)
 {
 	struct snd_sof_pcm_stream *sps =
@@ -29,16 +45,16 @@ void snd_sof_compr_init_elapsed_work(struct work_struct *work)
  */
 void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 {
-	struct snd_soc_component *component;
-	struct snd_soc_pcm_runtime *rtd;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+	struct snd_soc_component *component =
+		snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
 	struct snd_sof_pcm *spcm;
 
 	if (!cstream)
 		return;
 
-	rtd = cstream->private_data;
-	component = snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
-
 	spcm = snd_sof_find_spcm_dai(component, rtd);
 	if (!spcm) {
 		dev_err(component->dev,
@@ -46,6 +62,250 @@ void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 		return;
 	}
 
+	sof_set_transferred_bytes(sstream, spcm->stream[cstream->direction].posn.host_posn,
+				  runtime->buffer_size);
+
 	/* use the same workqueue-based solution as for PCM, cf. snd_sof_pcm_elapsed */
 	schedule_work(&spcm->stream[cstream->direction].period_elapsed_work);
 }
+
+static int create_page_table(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     unsigned char *dma_area, size_t size)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
+	int dir = cstream->direction;
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	return snd_sof_create_page_table(component->dev, dmab,
+					 spcm->stream[dir].page_table.area, size);
+}
+
+int sof_compr_open(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream;
+	struct snd_sof_pcm *spcm;
+	int dir;
+
+	sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
+	if (!sstream)
+		return -ENOMEM;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm) {
+		kfree(sstream);
+		return -EINVAL;
+	}
+
+	dir = cstream->direction;
+
+	if (spcm->stream[dir].cstream) {
+		kfree(sstream);
+		return -EBUSY;
+	}
+
+	spcm->stream[dir].cstream = cstream;
+	spcm->stream[dir].posn.host_posn = 0;
+	spcm->stream[dir].posn.dai_posn = 0;
+	spcm->prepared[dir] = false;
+
+	runtime->private_data = sstream;
+
+	return 0;
+}
+
+int sof_compr_free(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_sof_pcm *spcm;
+	int ret = 0;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	if (spcm->prepared[cstream->direction]) {
+		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+					 &stream, sizeof(stream),
+					 &reply, sizeof(reply));
+		if (!ret)
+			spcm->prepared[cstream->direction] = false;
+	}
+
+	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
+	spcm->stream[cstream->direction].cstream = NULL;
+	kfree(sstream);
+
+	return ret;
+}
+
+int sof_compr_set_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	struct sof_compr_stream *sstream = rtd->private_data;
+	struct sof_ipc_pcm_params_reply ipc_params_reply;
+	struct sof_ipc_pcm_params pcm;
+	struct snd_sof_pcm *spcm;
+	int ret;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
+	if (!spcm)
+		return -EINVAL;
+
+	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
+	cstream->dma_buffer.dev.dev = sdev->dev;
+	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
+	if (ret < 0)
+		return ret;
+
+	create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);
+
+	memset(&pcm, 0, sizeof(pcm));
+
+	pcm.params.buffer.pages = PFN_UP(rtd->dma_bytes);
+	pcm.hdr.size = sizeof(pcm);
+	pcm.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_PARAMS;
+
+	pcm.comp_id = spcm->stream[cstream->direction].comp_id;
+	pcm.params.hdr.size = sizeof(pcm.params);
+	pcm.params.buffer.phy_addr = spcm->stream[cstream->direction].page_table.addr;
+	pcm.params.buffer.size = rtd->dma_bytes;
+	pcm.params.direction = cstream->direction;
+	pcm.params.channels = params->codec.ch_out;
+	pcm.params.rate = params->codec.sample_rate;
+	pcm.params.buffer_fmt = SOF_IPC_BUFFER_INTERLEAVED;
+	pcm.params.frame_fmt = SOF_IPC_FRAME_S32_LE;
+	pcm.params.sample_container_bytes =
+		snd_pcm_format_physical_width(SNDRV_PCM_FORMAT_S32) >> 3;
+	pcm.params.host_period_bytes = params->buffer.fragment_size;
+
+	ret = sof_ipc_tx_message(sdev->ipc, pcm.hdr.cmd, &pcm, sizeof(pcm),
+				 &ipc_params_reply, sizeof(ipc_params_reply));
+	if (ret < 0) {
+		dev_err(component->dev, "error ipc failed\n");
+		return ret;
+	}
+
+	sstream->posn_offset = sdev->stream_box.offset + ipc_params_reply.posn_offset;
+	sstream->sample_rate = params->codec.sample_rate;
+	spcm->prepared[cstream->direction] = true;
+
+	return 0;
+}
+
+int sof_compr_get_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_codec *params)
+{
+	return 0;
+}
+
+int sof_compr_trigger(struct snd_soc_component *component,
+		      struct snd_compr_stream *cstream, int cmd)
+{
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START;
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_STOP;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_PAUSE;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE;
+		break;
+	default:
+		dev_err(component->dev, "error: unhandled trigger cmd %d\n", cmd);
+		break;
+	}
+
+	return sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+				  &stream, sizeof(stream),
+				  &reply, sizeof(reply));
+}
+
+int sof_compr_copy(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream,
+		   char __user *buf, size_t count)
+{
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	unsigned int offset, n;
+	void *ptr;
+	int ret;
+
+	if (count > rtd->buffer_size)
+		count = rtd->buffer_size;
+
+	div_u64_rem(rtd->total_bytes_available, rtd->buffer_size, &offset);
+	ptr = rtd->dma_area + offset;
+	n = rtd->buffer_size - offset;
+
+	if (count < n) {
+		ret = copy_from_user(ptr, buf, count);
+	} else {
+		ret = copy_from_user(ptr, buf, n);
+		ret += copy_from_user(rtd->dma_area, buf + n, count - n);
+	}
+
+	return count - ret;
+}
+
+static int sof_compr_pointer(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     struct snd_compr_tstamp *tstamp)
+{
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+
+	tstamp->sampling_rate = sstream->sample_rate;
+	tstamp->copied_total = sstream->copied_total;
+
+	return 0;
+}
+
+struct snd_compress_ops sof_compressed_ops = {
+	.open		= sof_compr_open,
+	.free		= sof_compr_free,
+	.set_params	= sof_compr_set_params,
+	.get_params	= sof_compr_get_params,
+	.trigger	= sof_compr_trigger,
+	.pointer	= sof_compr_pointer,
+	.copy		= sof_compr_copy,
+};
+EXPORT_SYMBOL(sof_compressed_ops);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 087935192ce8..d001a762a866 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
 	SOF_DEBUGFS_ACCESS_D0_ONLY,
 };
 
+struct sof_compr_stream {
+	unsigned int copied_total;
+	unsigned int sample_rate;
+	size_t posn_offset;
+};
+
 struct snd_sof_dev;
 struct snd_sof_ipc_msg;
 struct snd_sof_ipc;
-- 
2.27.0

[PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

From: Daniel Baluta <daniel.baluta@nxp.com>

Implement snd_compress_ops. There are a lot of similarities with
PCM implementation.

For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
firmware.

This will be changed in the future once we either add new compress
parameters to SOF or enhance existing sof_ipc_pcm_params structure
to support all native compress params.

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/sof/compress.c | 270 ++++++++++++++++++++++++++++++++++++++-
 sound/soc/sof/sof-priv.h |   6 +
 2 files changed, 271 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
index 01ca85f0b87f..2db1517d906d 100644
--- a/sound/soc/sof/compress.c
+++ b/sound/soc/sof/compress.c
@@ -10,6 +10,22 @@
 #include "sof-audio.h"
 #include "sof-priv.h"
 
+static void sof_set_transferred_bytes(struct sof_compr_stream *sstream,
+				      u64 host_pos, u64 buffer_size)
+{
+	u64 prev_pos;
+	unsigned int copied;
+
+	div64_u64_rem(sstream->copied_total, buffer_size, &prev_pos);
+
+	if (host_pos < prev_pos)
+		copied = (buffer_size - prev_pos) + host_pos;
+	else
+		copied = host_pos - prev_pos;
+
+	sstream->copied_total += copied;
+}
+
 static void snd_sof_compr_fragment_elapsed_work(struct work_struct *work)
 {
 	struct snd_sof_pcm_stream *sps =
@@ -29,16 +45,16 @@ void snd_sof_compr_init_elapsed_work(struct work_struct *work)
  */
 void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 {
-	struct snd_soc_component *component;
-	struct snd_soc_pcm_runtime *rtd;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+	struct snd_soc_component *component =
+		snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
 	struct snd_sof_pcm *spcm;
 
 	if (!cstream)
 		return;
 
-	rtd = cstream->private_data;
-	component = snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
-
 	spcm = snd_sof_find_spcm_dai(component, rtd);
 	if (!spcm) {
 		dev_err(component->dev,
@@ -46,6 +62,250 @@ void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
 		return;
 	}
 
+	sof_set_transferred_bytes(sstream, spcm->stream[cstream->direction].posn.host_posn,
+				  runtime->buffer_size);
+
 	/* use the same workqueue-based solution as for PCM, cf. snd_sof_pcm_elapsed */
 	schedule_work(&spcm->stream[cstream->direction].period_elapsed_work);
 }
+
+static int create_page_table(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     unsigned char *dma_area, size_t size)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
+	int dir = cstream->direction;
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	return snd_sof_create_page_table(component->dev, dmab,
+					 spcm->stream[dir].page_table.area, size);
+}
+
+int sof_compr_open(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream;
+	struct snd_sof_pcm *spcm;
+	int dir;
+
+	sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
+	if (!sstream)
+		return -ENOMEM;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm) {
+		kfree(sstream);
+		return -EINVAL;
+	}
+
+	dir = cstream->direction;
+
+	if (spcm->stream[dir].cstream) {
+		kfree(sstream);
+		return -EBUSY;
+	}
+
+	spcm->stream[dir].cstream = cstream;
+	spcm->stream[dir].posn.host_posn = 0;
+	spcm->stream[dir].posn.dai_posn = 0;
+	spcm->prepared[dir] = false;
+
+	runtime->private_data = sstream;
+
+	return 0;
+}
+
+int sof_compr_free(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_sof_pcm *spcm;
+	int ret = 0;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	if (spcm->prepared[cstream->direction]) {
+		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+					 &stream, sizeof(stream),
+					 &reply, sizeof(reply));
+		if (!ret)
+			spcm->prepared[cstream->direction] = false;
+	}
+
+	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
+	spcm->stream[cstream->direction].cstream = NULL;
+	kfree(sstream);
+
+	return ret;
+}
+
+int sof_compr_set_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
+{
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	struct sof_compr_stream *sstream = rtd->private_data;
+	struct sof_ipc_pcm_params_reply ipc_params_reply;
+	struct sof_ipc_pcm_params pcm;
+	struct snd_sof_pcm *spcm;
+	int ret;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
+	if (!spcm)
+		return -EINVAL;
+
+	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
+	cstream->dma_buffer.dev.dev = sdev->dev;
+	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
+	if (ret < 0)
+		return ret;
+
+	create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);
+
+	memset(&pcm, 0, sizeof(pcm));
+
+	pcm.params.buffer.pages = PFN_UP(rtd->dma_bytes);
+	pcm.hdr.size = sizeof(pcm);
+	pcm.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_PARAMS;
+
+	pcm.comp_id = spcm->stream[cstream->direction].comp_id;
+	pcm.params.hdr.size = sizeof(pcm.params);
+	pcm.params.buffer.phy_addr = spcm->stream[cstream->direction].page_table.addr;
+	pcm.params.buffer.size = rtd->dma_bytes;
+	pcm.params.direction = cstream->direction;
+	pcm.params.channels = params->codec.ch_out;
+	pcm.params.rate = params->codec.sample_rate;
+	pcm.params.buffer_fmt = SOF_IPC_BUFFER_INTERLEAVED;
+	pcm.params.frame_fmt = SOF_IPC_FRAME_S32_LE;
+	pcm.params.sample_container_bytes =
+		snd_pcm_format_physical_width(SNDRV_PCM_FORMAT_S32) >> 3;
+	pcm.params.host_period_bytes = params->buffer.fragment_size;
+
+	ret = sof_ipc_tx_message(sdev->ipc, pcm.hdr.cmd, &pcm, sizeof(pcm),
+				 &ipc_params_reply, sizeof(ipc_params_reply));
+	if (ret < 0) {
+		dev_err(component->dev, "error ipc failed\n");
+		return ret;
+	}
+
+	sstream->posn_offset = sdev->stream_box.offset + ipc_params_reply.posn_offset;
+	sstream->sample_rate = params->codec.sample_rate;
+	spcm->prepared[cstream->direction] = true;
+
+	return 0;
+}
+
+int sof_compr_get_params(struct snd_soc_component *component,
+			 struct snd_compr_stream *cstream, struct snd_codec *params)
+{
+	return 0;
+}
+
+int sof_compr_trigger(struct snd_soc_component *component,
+		      struct snd_compr_stream *cstream, int cmd)
+{
+	struct sof_ipc_stream stream;
+	struct sof_ipc_reply reply;
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
+	struct snd_sof_pcm *spcm;
+
+	spcm = snd_sof_find_spcm_dai(component, rtd);
+	if (!spcm)
+		return -EINVAL;
+
+	stream.hdr.size = sizeof(stream);
+	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG;
+	stream.comp_id = spcm->stream[cstream->direction].comp_id;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START;
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_STOP;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_PAUSE;
+		break;
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE;
+		break;
+	default:
+		dev_err(component->dev, "error: unhandled trigger cmd %d\n", cmd);
+		break;
+	}
+
+	return sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
+				  &stream, sizeof(stream),
+				  &reply, sizeof(reply));
+}
+
+int sof_compr_copy(struct snd_soc_component *component,
+		   struct snd_compr_stream *cstream,
+		   char __user *buf, size_t count)
+{
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	unsigned int offset, n;
+	void *ptr;
+	int ret;
+
+	if (count > rtd->buffer_size)
+		count = rtd->buffer_size;
+
+	div_u64_rem(rtd->total_bytes_available, rtd->buffer_size, &offset);
+	ptr = rtd->dma_area + offset;
+	n = rtd->buffer_size - offset;
+
+	if (count < n) {
+		ret = copy_from_user(ptr, buf, count);
+	} else {
+		ret = copy_from_user(ptr, buf, n);
+		ret += copy_from_user(rtd->dma_area, buf + n, count - n);
+	}
+
+	return count - ret;
+}
+
+static int sof_compr_pointer(struct snd_soc_component *component,
+			     struct snd_compr_stream *cstream,
+			     struct snd_compr_tstamp *tstamp)
+{
+	struct snd_compr_runtime *runtime = cstream->runtime;
+	struct sof_compr_stream *sstream = runtime->private_data;
+
+	tstamp->sampling_rate = sstream->sample_rate;
+	tstamp->copied_total = sstream->copied_total;
+
+	return 0;
+}
+
+struct snd_compress_ops sof_compressed_ops = {
+	.open		= sof_compr_open,
+	.free		= sof_compr_free,
+	.set_params	= sof_compr_set_params,
+	.get_params	= sof_compr_get_params,
+	.trigger	= sof_compr_trigger,
+	.pointer	= sof_compr_pointer,
+	.copy		= sof_compr_copy,
+};
+EXPORT_SYMBOL(sof_compressed_ops);
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index 087935192ce8..d001a762a866 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
 	SOF_DEBUGFS_ACCESS_D0_ONLY,
 };
 
+struct sof_compr_stream {
+	unsigned int copied_total;
+	unsigned int sample_rate;
+	size_t posn_offset;
+};
+
 struct snd_sof_dev;
 struct snd_sof_ipc_msg;
 struct snd_sof_ipc;
-- 
2.27.0

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Cezary Rojewski]

On 2022-01-13 5:13 PM, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Implement snd_compress_ops. There are a lot of similarities with
> PCM implementation.
> 
> For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
> firmware.
> 
> This will be changed in the future once we either add new compress
> parameters to SOF or enhance existing sof_ipc_pcm_params structure
> to support all native compress params.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

...

> +static int create_page_table(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     unsigned char *dma_area, size_t size)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> +	int dir = cstream->direction;
> +	struct snd_sof_pcm *spcm;

The layout of this declaration block is weird - it's neither a 
reversed-christmas-tree nor higher->lower complexity (e.g. structs -> 
primitives). Could you explain why it is shaped as is?

> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	return snd_sof_create_page_table(component->dev, dmab,
> +					 spcm->stream[dir].page_table.area, size);
> +}
> +
> +int sof_compr_open(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Making use of 'rtd' and 'runtime' simultaneously within a function make 
it less readable.

> +	struct sof_compr_stream *sstream;
> +	struct snd_sof_pcm *spcm;
> +	int dir;
> +
> +	sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
> +	if (!sstream)
> +		return -ENOMEM;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm) {
> +		kfree(sstream);
> +		return -EINVAL;
> +	}
> +
> +	dir = cstream->direction;
> +
> +	if (spcm->stream[dir].cstream) {
> +		kfree(sstream);
> +		return -EBUSY;
> +	}

Could you explain why this check is needed? i.e. Is is possible for 
compress stream to be opened "twice"?

> +
> +	spcm->stream[dir].cstream = cstream;
> +	spcm->stream[dir].posn.host_posn = 0;
> +	spcm->stream[dir].posn.dai_posn = 0;
> +	spcm->prepared[dir] = false;
> +
> +	runtime->private_data = sstream;
> +
> +	return 0;
> +}
> +
> +int sof_compr_free(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Ditto.

> +	struct sof_compr_stream *sstream = runtime->private_data;
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_sof_pcm *spcm;
> +	int ret = 0;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	stream.hdr.size = sizeof(stream);
> +	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
> +	stream.comp_id = spcm->stream[cstream->direction].comp_id;
> +
> +	if (spcm->prepared[cstream->direction]) {
> +		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
> +					 &stream, sizeof(stream),
> +					 &reply, sizeof(reply));
> +		if (!ret)
> +			spcm->prepared[cstream->direction] = false;

Why is the assignment conditional? If IPC fails, is the ->prepared flag 
respected and addressed later on? It does not seem to happen here.

> +	}
> +
> +	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
> +	spcm->stream[cstream->direction].cstream = NULL;
> +	kfree(sstream);
> +
> +	return ret;
> +}
> +
> +int sof_compr_set_params(struct snd_soc_component *component,
> +			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
> +	struct snd_compr_runtime *rtd = cstream->runtime;
> +	struct sof_compr_stream *sstream = rtd->private_data;
> +	struct sof_ipc_pcm_params_reply ipc_params_reply;
> +	struct sof_ipc_pcm_params pcm;
> +	struct snd_sof_pcm *spcm;
> +	int ret;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
> +	cstream->dma_buffer.dev.dev = sdev->dev;
> +	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
> +	if (ret < 0)
> +		return ret;
> +
> +	create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);

Shouldn't the result of create_page_table() be tested before moving on?


...

> +int sof_compr_trigger(struct snd_soc_component *component,
> +		      struct snd_compr_stream *cstream, int cmd)
> +{
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_sof_pcm *spcm;

Similarly to create_page_table() case, layout of this declaration block 
is weird. Perhaps I'm just unaware of the convention used within this 
directory.


...

> +static int sof_compr_pointer(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     struct snd_compr_tstamp *tstamp)
> +{
> +	struct snd_compr_runtime *runtime = cstream->runtime;
> +	struct sof_compr_stream *sstream = runtime->private_data;

I'd suggest declaring single local variable instead. The 'runtime' one 
is unused except for the initial 'sstream' assignemnt.

> +
> +	tstamp->sampling_rate = sstream->sample_rate;
> +	tstamp->copied_total = sstream->copied_total;
> +
> +	return 0;
> +}

...

> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index 087935192ce8..d001a762a866 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
>   	SOF_DEBUGFS_ACCESS_D0_ONLY,
>   };
>   
> +struct sof_compr_stream {
> +	unsigned int copied_total;
> +	unsigned int sample_rate;
> +	size_t posn_offset;
> +};

Some streaming-related PCM structs follow snd_sof_xxx naming pattern 
instead, e.g.: snd_sof_pcm. Is the naming convention for compress 
streams seen here intentional?

> +
>   struct snd_sof_dev;
>   struct snd_sof_ipc_msg;
>   struct snd_sof_ipc;
> 

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Cezary Rojewski]

On 2022-01-13 5:13 PM, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Implement snd_compress_ops. There are a lot of similarities with
> PCM implementation.
> 
> For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
> firmware.
> 
> This will be changed in the future once we either add new compress
> parameters to SOF or enhance existing sof_ipc_pcm_params structure
> to support all native compress params.
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

...

> +static int create_page_table(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     unsigned char *dma_area, size_t size)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> +	int dir = cstream->direction;
> +	struct snd_sof_pcm *spcm;

The layout of this declaration block is weird - it's neither a 
reversed-christmas-tree nor higher->lower complexity (e.g. structs -> 
primitives). Could you explain why it is shaped as is?

> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	return snd_sof_create_page_table(component->dev, dmab,
> +					 spcm->stream[dir].page_table.area, size);
> +}
> +
> +int sof_compr_open(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Making use of 'rtd' and 'runtime' simultaneously within a function make 
it less readable.

> +	struct sof_compr_stream *sstream;
> +	struct snd_sof_pcm *spcm;
> +	int dir;
> +
> +	sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
> +	if (!sstream)
> +		return -ENOMEM;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm) {
> +		kfree(sstream);
> +		return -EINVAL;
> +	}
> +
> +	dir = cstream->direction;
> +
> +	if (spcm->stream[dir].cstream) {
> +		kfree(sstream);
> +		return -EBUSY;
> +	}

Could you explain why this check is needed? i.e. Is is possible for 
compress stream to be opened "twice"?

> +
> +	spcm->stream[dir].cstream = cstream;
> +	spcm->stream[dir].posn.host_posn = 0;
> +	spcm->stream[dir].posn.dai_posn = 0;
> +	spcm->prepared[dir] = false;
> +
> +	runtime->private_data = sstream;
> +
> +	return 0;
> +}
> +
> +int sof_compr_free(struct snd_soc_component *component,
> +		   struct snd_compr_stream *cstream)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_compr_runtime *runtime = cstream->runtime;

Ditto.

> +	struct sof_compr_stream *sstream = runtime->private_data;
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_sof_pcm *spcm;
> +	int ret = 0;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	stream.hdr.size = sizeof(stream);
> +	stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
> +	stream.comp_id = spcm->stream[cstream->direction].comp_id;
> +
> +	if (spcm->prepared[cstream->direction]) {
> +		ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
> +					 &stream, sizeof(stream),
> +					 &reply, sizeof(reply));
> +		if (!ret)
> +			spcm->prepared[cstream->direction] = false;

Why is the assignment conditional? If IPC fails, is the ->prepared flag 
respected and addressed later on? It does not seem to happen here.

> +	}
> +
> +	cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
> +	spcm->stream[cstream->direction].cstream = NULL;
> +	kfree(sstream);
> +
> +	return ret;
> +}
> +
> +int sof_compr_set_params(struct snd_soc_component *component,
> +			 struct snd_compr_stream *cstream, struct snd_compr_params *params)
> +{
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
> +	struct snd_compr_runtime *rtd = cstream->runtime;
> +	struct sof_compr_stream *sstream = rtd->private_data;
> +	struct sof_ipc_pcm_params_reply ipc_params_reply;
> +	struct sof_ipc_pcm_params pcm;
> +	struct snd_sof_pcm *spcm;
> +	int ret;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
> +	cstream->dma_buffer.dev.dev = sdev->dev;
> +	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
> +	if (ret < 0)
> +		return ret;
> +
> +	create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);

Shouldn't the result of create_page_table() be tested before moving on?


...

> +int sof_compr_trigger(struct snd_soc_component *component,
> +		      struct snd_compr_stream *cstream, int cmd)
> +{
> +	struct sof_ipc_stream stream;
> +	struct sof_ipc_reply reply;
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> +	struct snd_sof_pcm *spcm;

Similarly to create_page_table() case, layout of this declaration block 
is weird. Perhaps I'm just unaware of the convention used within this 
directory.


...

> +static int sof_compr_pointer(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     struct snd_compr_tstamp *tstamp)
> +{
> +	struct snd_compr_runtime *runtime = cstream->runtime;
> +	struct sof_compr_stream *sstream = runtime->private_data;

I'd suggest declaring single local variable instead. The 'runtime' one 
is unused except for the initial 'sstream' assignemnt.

> +
> +	tstamp->sampling_rate = sstream->sample_rate;
> +	tstamp->copied_total = sstream->copied_total;
> +
> +	return 0;
> +}

...

> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index 087935192ce8..d001a762a866 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
>   	SOF_DEBUGFS_ACCESS_D0_ONLY,
>   };
>   
> +struct sof_compr_stream {
> +	unsigned int copied_total;
> +	unsigned int sample_rate;
> +	size_t posn_offset;
> +};

Some streaming-related PCM structs follow snd_sof_xxx naming pattern 
instead, e.g.: snd_sof_pcm. Is the naming convention for compress 
streams seen here intentional?

> +
>   struct snd_sof_dev;
>   struct snd_sof_ipc_msg;
>   struct snd_sof_ipc;
> 

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

On Sat, Jan 15, 2022 at 12:43 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-01-13 5:13 PM, Daniel Baluta wrote:
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Implement snd_compress_ops. There are a lot of similarities with
> > PCM implementation.
> >
> > For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
> > firmware.
> >
> > This will be changed in the future once we either add new compress
> > parameters to SOF or enhance existing sof_ipc_pcm_params structure
> > to support all native compress params.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>
> ...
>
> > +static int create_page_table(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          unsigned char *dma_area, size_t size)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> > +     int dir = cstream->direction;
> > +     struct snd_sof_pcm *spcm;
>
> The layout of this declaration block is weird - it's neither a
> reversed-christmas-tree nor higher->lower complexity (e.g. structs ->
> primitives). Could you explain why it is shaped as is?

You are right, never put too much thought for this. Looking at SOF anyhow,
it doesn't seem that all kind of styles are used including just random order.

Will fix it :).

>
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     return snd_sof_create_page_table(component->dev, dmab,
> > +                                      spcm->stream[dir].page_table.area, size);
> > +}
> > +
> > +int sof_compr_open(struct snd_soc_component *component,
> > +                struct snd_compr_stream *cstream)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
>
> Making use of 'rtd' and 'runtime' simultaneously within a function make
> it less readable.

I see. I will use rtd for snd_soc_pcm_runtime as usual and crtd for
snd_compr_runtime.
Naming is hard.

>
> > +     struct sof_compr_stream *sstream;
> > +     struct snd_sof_pcm *spcm;
> > +     int dir;
> > +
> > +     sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
> > +     if (!sstream)
> > +             return -ENOMEM;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm) {
> > +             kfree(sstream);
> > +             return -EINVAL;
> > +     }
> > +
> > +     dir = cstream->direction;
> > +
> > +     if (spcm->stream[dir].cstream) {
> > +             kfree(sstream);
> > +             return -EBUSY;
> > +     }
>
> Could you explain why this check is needed? i.e. Is is possible for
> compress stream to be opened "twice"?

This is needed because compress upper layers do not forbid opening the
device twice
but also it doesn't make much sense to open it twice.

So, I just have exclusive access to Compr device and the rest of the
calls to return
-EBUSY.

Same approach as in sound/soc/uniphier/aio-compress.c
>
> > +
> > +     spcm->stream[dir].cstream = cstream;
> > +     spcm->stream[dir].posn.host_posn = 0;
> > +     spcm->stream[dir].posn.dai_posn = 0;
> > +     spcm->prepared[dir] = false;
> > +
> > +     runtime->private_data = sstream;
> > +
> > +     return 0;
> > +}
> > +
> > +int sof_compr_free(struct snd_soc_component *component,
> > +                struct snd_compr_stream *cstream)
> > +{
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
>
> Ditto.

Thanks. Will fix.

>
> > +     struct sof_compr_stream *sstream = runtime->private_data;
> > +     struct sof_ipc_stream stream;
> > +     struct sof_ipc_reply reply;
> > +     struct snd_sof_pcm *spcm;
> > +     int ret = 0;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     stream.hdr.size = sizeof(stream);
> > +     stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
> > +     stream.comp_id = spcm->stream[cstream->direction].comp_id;
> > +
> > +     if (spcm->prepared[cstream->direction]) {
> > +             ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
> > +                                      &stream, sizeof(stream),
> > +                                      &reply, sizeof(reply));
> > +             if (!ret)
> > +                     spcm->prepared[cstream->direction] = false;
>
> Why is the assignment conditional? If IPC fails, is the ->prepared flag
> respected and addressed later on? It does not seem to happen here.
>

If this call fails it mean that freeing the pipeline has failed and we
return an error
to the upper layer.

I dont think it makes sense to mark the stream as freed (prepared =
false) if the IPC has failed
we just return an error to the upper layers.

I'm not sure if we can do anything useful with respect to failures in
sof_compr_free other then
report it to upper layers and keep internal error.

If the upper layers decide to call again compr_open() the prepare will
be set at that point to false.

> > +     }
> > +
> > +     cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
> > +     spcm->stream[cstream->direction].cstream = NULL;
> > +     kfree(sstream);
> > +
> > +     return ret;
> > +}
> > +
> > +int sof_compr_set_params(struct snd_soc_component *component,
> > +                      struct snd_compr_stream *cstream, struct snd_compr_params *params)
> > +{
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
> > +     struct snd_compr_runtime *rtd = cstream->runtime;
> > +     struct sof_compr_stream *sstream = rtd->private_data;
> > +     struct sof_ipc_pcm_params_reply ipc_params_reply;
> > +     struct sof_ipc_pcm_params pcm;
> > +     struct snd_sof_pcm *spcm;
> > +     int ret;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
> > +     cstream->dma_buffer.dev.dev = sdev->dev;
> > +     ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);
>
> Shouldn't the result of create_page_table() be tested before moving on?

You are right. Will fix.

>
>
> ...
>
> > +int sof_compr_trigger(struct snd_soc_component *component,
> > +                   struct snd_compr_stream *cstream, int cmd)
> > +{
> > +     struct sof_ipc_stream stream;
> > +     struct sof_ipc_reply reply;
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_sof_pcm *spcm;
>
> Similarly to create_page_table() case, layout of this declaration block
> is weird. Perhaps I'm just unaware of the convention used within this
> directory.

True. I just added the fields at random points while they were used. If you look
at the sof directory this happens all over the place.

>
>
> ...
>
> > +static int sof_compr_pointer(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          struct snd_compr_tstamp *tstamp)
> > +{
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
> > +     struct sof_compr_stream *sstream = runtime->private_data;
>
> I'd suggest declaring single local variable instead. The 'runtime' one
> is unused except for the initial 'sstream' assignemnt.

Will do.
>
> > +
> > +     tstamp->sampling_rate = sstream->sample_rate;
> > +     tstamp->copied_total = sstream->copied_total;
> > +
> > +     return 0;
> > +}
>
> ...
>
> > diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> > index 087935192ce8..d001a762a866 100644
> > --- a/sound/soc/sof/sof-priv.h
> > +++ b/sound/soc/sof/sof-priv.h
> > @@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
> >       SOF_DEBUGFS_ACCESS_D0_ONLY,
> >   };
> >
> > +struct sof_compr_stream {
> > +     unsigned int copied_total;
> > +     unsigned int sample_rate;
> > +     size_t posn_offset;
> > +};
>
> Some streaming-related PCM structs follow snd_sof_xxx naming pattern
> instead, e.g.: snd_sof_pcm. Is the naming convention for compress
> streams seen here intentional?

Hmm, again naming is hard. I will think about it.
>
> > +
> >   struct snd_sof_dev;
> >   struct snd_sof_ipc_msg;
> >   struct snd_sof_ipc;
> >

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

On Sat, Jan 15, 2022 at 12:43 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-01-13 5:13 PM, Daniel Baluta wrote:
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Implement snd_compress_ops. There are a lot of similarities with
> > PCM implementation.
> >
> > For now we use sof_ipc_pcm_params to transfer compress parameters to SOF
> > firmware.
> >
> > This will be changed in the future once we either add new compress
> > parameters to SOF or enhance existing sof_ipc_pcm_params structure
> > to support all native compress params.
> >
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>
> ...
>
> > +static int create_page_table(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          unsigned char *dma_area, size_t size)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> > +     int dir = cstream->direction;
> > +     struct snd_sof_pcm *spcm;
>
> The layout of this declaration block is weird - it's neither a
> reversed-christmas-tree nor higher->lower complexity (e.g. structs ->
> primitives). Could you explain why it is shaped as is?

You are right, never put too much thought for this. Looking at SOF anyhow,
it doesn't seem that all kind of styles are used including just random order.

Will fix it :).

>
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     return snd_sof_create_page_table(component->dev, dmab,
> > +                                      spcm->stream[dir].page_table.area, size);
> > +}
> > +
> > +int sof_compr_open(struct snd_soc_component *component,
> > +                struct snd_compr_stream *cstream)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
>
> Making use of 'rtd' and 'runtime' simultaneously within a function make
> it less readable.

I see. I will use rtd for snd_soc_pcm_runtime as usual and crtd for
snd_compr_runtime.
Naming is hard.

>
> > +     struct sof_compr_stream *sstream;
> > +     struct snd_sof_pcm *spcm;
> > +     int dir;
> > +
> > +     sstream = kzalloc(sizeof(*sstream), GFP_KERNEL);
> > +     if (!sstream)
> > +             return -ENOMEM;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm) {
> > +             kfree(sstream);
> > +             return -EINVAL;
> > +     }
> > +
> > +     dir = cstream->direction;
> > +
> > +     if (spcm->stream[dir].cstream) {
> > +             kfree(sstream);
> > +             return -EBUSY;
> > +     }
>
> Could you explain why this check is needed? i.e. Is is possible for
> compress stream to be opened "twice"?

This is needed because compress upper layers do not forbid opening the
device twice
but also it doesn't make much sense to open it twice.

So, I just have exclusive access to Compr device and the rest of the
calls to return
-EBUSY.

Same approach as in sound/soc/uniphier/aio-compress.c
>
> > +
> > +     spcm->stream[dir].cstream = cstream;
> > +     spcm->stream[dir].posn.host_posn = 0;
> > +     spcm->stream[dir].posn.dai_posn = 0;
> > +     spcm->prepared[dir] = false;
> > +
> > +     runtime->private_data = sstream;
> > +
> > +     return 0;
> > +}
> > +
> > +int sof_compr_free(struct snd_soc_component *component,
> > +                struct snd_compr_stream *cstream)
> > +{
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
>
> Ditto.

Thanks. Will fix.

>
> > +     struct sof_compr_stream *sstream = runtime->private_data;
> > +     struct sof_ipc_stream stream;
> > +     struct sof_ipc_reply reply;
> > +     struct snd_sof_pcm *spcm;
> > +     int ret = 0;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     stream.hdr.size = sizeof(stream);
> > +     stream.hdr.cmd = SOF_IPC_GLB_STREAM_MSG | SOF_IPC_STREAM_PCM_FREE;
> > +     stream.comp_id = spcm->stream[cstream->direction].comp_id;
> > +
> > +     if (spcm->prepared[cstream->direction]) {
> > +             ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd,
> > +                                      &stream, sizeof(stream),
> > +                                      &reply, sizeof(reply));
> > +             if (!ret)
> > +                     spcm->prepared[cstream->direction] = false;
>
> Why is the assignment conditional? If IPC fails, is the ->prepared flag
> respected and addressed later on? It does not seem to happen here.
>

If this call fails it mean that freeing the pipeline has failed and we
return an error
to the upper layer.

I dont think it makes sense to mark the stream as freed (prepared =
false) if the IPC has failed
we just return an error to the upper layers.

I'm not sure if we can do anything useful with respect to failures in
sof_compr_free other then
report it to upper layers and keep internal error.

If the upper layers decide to call again compr_open() the prepare will
be set at that point to false.

> > +     }
> > +
> > +     cancel_work_sync(&spcm->stream[cstream->direction].period_elapsed_work);
> > +     spcm->stream[cstream->direction].cstream = NULL;
> > +     kfree(sstream);
> > +
> > +     return ret;
> > +}
> > +
> > +int sof_compr_set_params(struct snd_soc_component *component,
> > +                      struct snd_compr_stream *cstream, struct snd_compr_params *params)
> > +{
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_soc_pcm_runtime *rtd_pcm = cstream->private_data;
> > +     struct snd_compr_runtime *rtd = cstream->runtime;
> > +     struct sof_compr_stream *sstream = rtd->private_data;
> > +     struct sof_ipc_pcm_params_reply ipc_params_reply;
> > +     struct sof_ipc_pcm_params pcm;
> > +     struct snd_sof_pcm *spcm;
> > +     int ret;
> > +
> > +     spcm = snd_sof_find_spcm_dai(component, rtd_pcm);
> > +     if (!spcm)
> > +             return -EINVAL;
> > +
> > +     cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
> > +     cstream->dma_buffer.dev.dev = sdev->dev;
> > +     ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     create_page_table(component, cstream, rtd->dma_area, rtd->dma_bytes);
>
> Shouldn't the result of create_page_table() be tested before moving on?

You are right. Will fix.

>
>
> ...
>
> > +int sof_compr_trigger(struct snd_soc_component *component,
> > +                   struct snd_compr_stream *cstream, int cmd)
> > +{
> > +     struct sof_ipc_stream stream;
> > +     struct sof_ipc_reply reply;
> > +     struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> > +     struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
> > +     struct snd_sof_pcm *spcm;
>
> Similarly to create_page_table() case, layout of this declaration block
> is weird. Perhaps I'm just unaware of the convention used within this
> directory.

True. I just added the fields at random points while they were used. If you look
at the sof directory this happens all over the place.

>
>
> ...
>
> > +static int sof_compr_pointer(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          struct snd_compr_tstamp *tstamp)
> > +{
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
> > +     struct sof_compr_stream *sstream = runtime->private_data;
>
> I'd suggest declaring single local variable instead. The 'runtime' one
> is unused except for the initial 'sstream' assignemnt.

Will do.
>
> > +
> > +     tstamp->sampling_rate = sstream->sample_rate;
> > +     tstamp->copied_total = sstream->copied_total;
> > +
> > +     return 0;
> > +}
>
> ...
>
> > diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> > index 087935192ce8..d001a762a866 100644
> > --- a/sound/soc/sof/sof-priv.h
> > +++ b/sound/soc/sof/sof-priv.h
> > @@ -108,6 +108,12 @@ enum sof_debugfs_access_type {
> >       SOF_DEBUGFS_ACCESS_D0_ONLY,
> >   };
> >
> > +struct sof_compr_stream {
> > +     unsigned int copied_total;
> > +     unsigned int sample_rate;
> > +     size_t posn_offset;
> > +};
>
> Some streaming-related PCM structs follow snd_sof_xxx naming pattern
> instead, e.g.: snd_sof_pcm. Is the naming convention for compress
> streams seen here intentional?

Hmm, again naming is hard. I will think about it.
>
> > +
> >   struct snd_sof_dev;
> >   struct snd_sof_ipc_msg;
> >   struct snd_sof_ipc;
> >

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Pierre-Louis Bossart]

Thanks for starting this work Daniel.

> +int sof_compr_get_params(struct snd_soc_component *component,
> +			 struct snd_compr_stream *cstream, struct snd_codec *params)
> +{
> +	return 0;
> +}

You should probably add a statement along the lines of:

/* TODO: we don't query the supported codecs for now, if the application
asks for an unsupported codec the set_params() will fail
*/

.get_codec_caps is also missing, it should be documented as something we
want to add.

> +static int sof_compr_pointer(struct snd_soc_component *component,
> +			     struct snd_compr_stream *cstream,
> +			     struct snd_compr_tstamp *tstamp)
> +{
> +	struct snd_compr_runtime *runtime = cstream->runtime;
> +	struct sof_compr_stream *sstream = runtime->private_data;
> +
> +	tstamp->sampling_rate = sstream->sample_rate;
> +	tstamp->copied_total = sstream->copied_total;

Humm, this doesn't return any information on how many PCM samples were
generated (pcm_frames) or rendered (pcm_io_frames).

I don't think the existing SOF firmware has this level of detail for
now, you should at least document it as to be added in the future.

In addition, the .pointer callback can be used at different times, and
for added precision the information should be queried from the firmware
via IPC or by looking up counters in the SRAM windows.

I don't think it's good enough to update the information on a fragment
elapsed event. It will work for sure in terms of reporting compressed
data transfers, but it's not good enough for an application to report
time elapsed.

> +struct sof_compr_stream {
> +	unsigned int copied_total;
> +	unsigned int sample_rate;
> +	size_t posn_offset;
> +};

do you need an SOF-specific definition? This looks awfully similar to
snd_compr_tstamp:

struct snd_compr_tstamp {
	__u32 byte_offset;
	__u32 copied_total;
	__u32 pcm_frames;
	__u32 pcm_io_frames;
	__u32 sampling_rate;
}

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

Thanks Pierre for comments.

On Sat, Jan 15, 2022 at 1:01 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> Thanks for starting this work Daniel.
>
> > +int sof_compr_get_params(struct snd_soc_component *component,
> > +                      struct snd_compr_stream *cstream, struct snd_codec *params)
> > +{
> > +     return 0;
> > +}
>
> You should probably add a statement along the lines of:
>
> /* TODO: we don't query the supported codecs for now, if the application
> asks for an unsupported codec the set_params() will fail
> */
>
> .get_codec_caps is also missing, it should be documented as something we
> want to add.

Will do.

>
> > +static int sof_compr_pointer(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          struct snd_compr_tstamp *tstamp)
> > +{
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
> > +     struct sof_compr_stream *sstream = runtime->private_data;
> > +
> > +     tstamp->sampling_rate = sstream->sample_rate;
> > +     tstamp->copied_total = sstream->copied_total;
>
> Humm, this doesn't return any information on how many PCM samples were
> generated (pcm_frames) or rendered (pcm_io_frames).

This is on my TODO list. I think there is some more work needed to be
done in FW.

>
> I don't think the existing SOF firmware has this level of detail for
> now, you should at least document it as to be added in the future.
>
> In addition, the .pointer callback can be used at different times, and
> for added precision the information should be queried from the firmware
> via IPC or by looking up counters in the SRAM windows.
>
> I don't think it's good enough to update the information on a fragment
> elapsed event. It will work for sure in terms of reporting compressed
> data transfers, but it's not good enough for an application to report
> time elapsed.

Very good observations here.

>
> > +struct sof_compr_stream {
> > +     unsigned int copied_total;
> > +     unsigned int sample_rate;
> > +     size_t posn_offset;
> > +};
>
> do you need an SOF-specific definition? This looks awfully similar to
> snd_compr_tstamp:
>
> struct snd_compr_tstamp {
>         __u32 byte_offset;
>         __u32 copied_total;
>         __u32 pcm_frames;
>         __u32 pcm_io_frames;
>         __u32 sampling_rate;
> }

There is no need for a SOF specific definition. I think we can use
that for now. We can change it later if we
need new fields.

Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

[Daniel Baluta]

Thanks Pierre for comments.

On Sat, Jan 15, 2022 at 1:01 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> Thanks for starting this work Daniel.
>
> > +int sof_compr_get_params(struct snd_soc_component *component,
> > +                      struct snd_compr_stream *cstream, struct snd_codec *params)
> > +{
> > +     return 0;
> > +}
>
> You should probably add a statement along the lines of:
>
> /* TODO: we don't query the supported codecs for now, if the application
> asks for an unsupported codec the set_params() will fail
> */
>
> .get_codec_caps is also missing, it should be documented as something we
> want to add.

Will do.

>
> > +static int sof_compr_pointer(struct snd_soc_component *component,
> > +                          struct snd_compr_stream *cstream,
> > +                          struct snd_compr_tstamp *tstamp)
> > +{
> > +     struct snd_compr_runtime *runtime = cstream->runtime;
> > +     struct sof_compr_stream *sstream = runtime->private_data;
> > +
> > +     tstamp->sampling_rate = sstream->sample_rate;
> > +     tstamp->copied_total = sstream->copied_total;
>
> Humm, this doesn't return any information on how many PCM samples were
> generated (pcm_frames) or rendered (pcm_io_frames).

This is on my TODO list. I think there is some more work needed to be
done in FW.

>
> I don't think the existing SOF firmware has this level of detail for
> now, you should at least document it as to be added in the future.
>
> In addition, the .pointer callback can be used at different times, and
> for added precision the information should be queried from the firmware
> via IPC or by looking up counters in the SRAM windows.
>
> I don't think it's good enough to update the information on a fragment
> elapsed event. It will work for sure in terms of reporting compressed
> data transfers, but it's not good enough for an application to report
> time elapsed.

Very good observations here.

>
> > +struct sof_compr_stream {
> > +     unsigned int copied_total;
> > +     unsigned int sample_rate;
> > +     size_t posn_offset;
> > +};
>
> do you need an SOF-specific definition? This looks awfully similar to
> snd_compr_tstamp:
>
> struct snd_compr_tstamp {
>         __u32 byte_offset;
>         __u32 copied_total;
>         __u32 pcm_frames;
>         __u32 pcm_io_frames;
>         __u32 sampling_rate;
> }

There is no need for a SOF specific definition. I think we can use
that for now. We can change it later if we
need new fields.