From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Daniel Baluta <daniel.baluta@oss.nxp.com>, <broonie@kernel.org>,
<alsa-devel@alsa-project.org>
Cc: <pierre-louis.bossart@linux.intel.com>, <lgirdwood@gmail.com>,
<ranjani.sridharan@linux.intel.com>,
<kai.vehmanen@linux.intel.com>, <daniel.baluta@nxp.com>,
<perex@perex.cz>, <tiwai@suse.com>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation
Date: Fri, 14 Jan 2022 16:49:51 +0100 [thread overview]
Message-ID: <d7e63f52-a98e-0c99-906b-6c03b25da572@intel.com> (raw)
In-Reply-To: <20220113161341.371345-1-daniel.baluta@oss.nxp.com>
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;
>
next prev parent reply other threads:[~2022-01-14 15:50 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-13 16:13 [PATCH] ASoC: SOF: compr: Add compress ops implementation Daniel Baluta
2022-01-14 15:49 ` Cezary Rojewski [this message]
2022-01-18 17:59 ` Daniel Baluta
2022-01-14 17:55 ` Pierre-Louis Bossart
2022-01-18 18:26 ` Daniel Baluta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d7e63f52-a98e-0c99-906b-6c03b25da572@intel.com \
--to=cezary.rojewski@intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=daniel.baluta@nxp.com \
--cc=daniel.baluta@oss.nxp.com \
--cc=kai.vehmanen@linux.intel.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=pierre-louis.bossart@linux.intel.com \
--cc=ranjani.sridharan@linux.intel.com \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).