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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 18 Jan 2022 20:26:58 +0200
From:   Daniel Baluta <daniel.baluta@...il.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Daniel Baluta <daniel.baluta@....nxp.com>,
        Mark Brown <broonie@...nel.org>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Daniel Baluta <daniel.baluta@....com>
Subject: Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation

Thanks Pierre for comments.

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