Thanks Pierre for comments. On Sat, Jan 15, 2022 at 1:01 AM Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> 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.