Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation
To:     Daniel Baluta <daniel.baluta@oss.nxp.com>, broonie@kernel.org,
        alsa-devel@alsa-project.org
Cc:     kai.vehmanen@linux.intel.com, lgirdwood@gmail.com,
        linux-kernel@vger.kernel.org, tiwai@suse.com,
        ranjani.sridharan@linux.intel.com, daniel.baluta@nxp.com
References: <20220113161341.371345-1-daniel.baluta@oss.nxp.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Message-ID: <9b3d28f3-aa2b-ade9-4160-752b349bdeab@linux.intel.com>
Date:   Fri, 14 Jan 2022 11:55:31 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20220113161341.371345-1-daniel.baluta@oss.nxp.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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