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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-01-13 5:13 PM, Daniel Baluta wrote:
From: Daniel Baluta <daniel.baluta@xxxxxxx>

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

...

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




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux