Date:   Wed, 17 Jul 2019 10:30:26 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Curtis Malainey <cujomalainey@...gle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     ALSA development <alsa-devel@...a-project.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Ben Zhang <benzh@...omium.org>,
        Mark Brown <broonie@...nel.org>,
        Curtis Malainey <cujomalainey@...omium.org>,
        Alex Levin <levinale@...omium.org>
Subject: Re: [alsa-devel] [PATCH] ASoC: Intel: Atom: read timestamp moved to
 period_elapsed



On 7/10/19 6:15 PM, Curtis Malainey wrote:
> On Tue, Jul 9, 2019 at 4:45 AM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
>>
>> On Mon, Jul 08, 2019 at 09:01:47PM -0700, Alex Levin wrote:
>>> sst_platform_pcm_pointer is called from both snd_pcm_period_elapsed and
>>> from snd_pcm_ioctl. Calling read timestamp results in recalculating
>>> pcm_delay and buffer_ptr (sst_calc_tstamp) which consumes buffers in a
>>> faster rate than intended.
>>> In a tested BSW system with chtrt5650, for a rate of 48000, the
>>> measured rate was sometimes 10 times more than that.
>>> After moving the timestamp read to period elapsed, buffer consumption is
>>> as expected.
>>
>>  From code prospective it looks good. You may take mine
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>>
>> Though I'm not an expert in the area, Pierre and / or Liam should give
>> their blessing.
>>
> Agreed, Liam or Pierre should also give their ok since this is one of
> the closed source firmware drivers.
> 
> Reviewed-by: Curtis Malainey <cujomalainey@...omium.org>

Humm, this first review after my Summer break isn't straightforward.

By moving the timestamp update to the period elapsed event, don't you 
prevent the use of this driver in no-interrupt mode - which I understood 
as the baseline for Chrome?

And I also don't get how this timestamp might lead to 10x speed issues, 
this driver has been around for a number of years and that specific 
error was never seen. What is different in this platform and can this be 
seen e.g. on a Cyan device?


>>>
>>> Signed-off-by: Alex Levin <levinale@...omium.org>
>>> ---
>>>   sound/soc/intel/atom/sst-mfld-platform-pcm.c | 23 +++++++++++++-------
>>>   1 file changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
>>> index 0e8b1c5eec88..196af0b30b41 100644
>>> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
>>> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
>>> @@ -265,16 +265,28 @@ static void sst_period_elapsed(void *arg)
>>>   {
>>>        struct snd_pcm_substream *substream = arg;
>>>        struct sst_runtime_stream *stream;
>>> -     int status;
>>> +     struct snd_soc_pcm_runtime *rtd;
>>> +     int status, ret_val;
>>>
>>>        if (!substream || !substream->runtime)
>>>                return;
>>>        stream = substream->runtime->private_data;
>>>        if (!stream)
>>>                return;
>>> +
>>> +     rtd = substream->private_data;
>>> +     if (!rtd)
>>> +             return;
>>> +
>>>        status = sst_get_stream_status(stream);
>>>        if (status != SST_PLATFORM_RUNNING)
>>>                return;
>>> +
>>> +     ret_val = stream->ops->stream_read_tstamp(sst->dev, &stream->stream_info);
>>> +     if (ret_val) {
>>> +             dev_err(rtd->dev, "stream_read_tstamp err code = %d\n", ret_val);
>>> +             return;
>>> +     }
>>>        snd_pcm_period_elapsed(substream);
>>>   }
>>>
>>> @@ -658,20 +670,15 @@ static snd_pcm_uframes_t sst_platform_pcm_pointer
>>>                        (struct snd_pcm_substream *substream)
>>>   {
>>>        struct sst_runtime_stream *stream;
>>> -     int ret_val, status;
>>> +     int status;
>>>        struct pcm_stream_info *str_info;
>>> -     struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>>
>>>        stream = substream->runtime->private_data;
>>>        status = sst_get_stream_status(stream);
>>>        if (status == SST_PLATFORM_INIT)
>>>                return 0;
>>> +
>>>        str_info = &stream->stream_info;
>>> -     ret_val = stream->ops->stream_read_tstamp(sst->dev, str_info);
>>> -     if (ret_val) {
>>> -             dev_err(rtd->dev, "sst: error code = %d\n", ret_val);
>>> -             return ret_val;
>>> -     }
>>>        substream->runtime->delay = str_info->pcm_delay;
>>>        return str_info->buffer_ptr;
>>>   }
>>> --
>>> 2.22.0.410.gd8fdbe21b5-goog
>>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@...a-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Powered by blists - more mailing lists