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;
MIME-Version: 1.0
References: <20220113161341.371345-1-daniel.baluta@oss.nxp.com> <9b3d28f3-aa2b-ade9-4160-752b349bdeab@linux.intel.com>
In-Reply-To: <9b3d28f3-aa2b-ade9-4160-752b349bdeab@linux.intel.com>
From:   Daniel Baluta <daniel.baluta@gmail.com>
Date:   Tue, 18 Jan 2022 20:26:58 +0200
Message-ID: <CAEnQRZBsJbcdx_H9ChcPTGEbe+Ok8mVGG+1wVYe2pDF+9RqbQA@mail.gmail.com>
Subject: Re: [PATCH] ASoC: SOF: compr: Add compress ops implementation
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     Daniel Baluta <daniel.baluta@oss.nxp.com>,
        Mark Brown <broonie@kernel.org>,
        Linux-ALSA <alsa-devel@alsa-project.org>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Takashi Iwai <tiwai@suse.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Daniel Baluta <daniel.baluta@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks Pierre for comments.

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