Date:   Tue, 11 Aug 2020 10:39:04 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Yu-Hsuan Hsu <yuhsuan@...omium.org>
Cc:     "Lu, Brent" <brent.lu@...el.com>,
        Guennadi Liakhovetski <guennadi.liakhovetski@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Rojewski, Cezary" <cezary.rojewski@...el.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Sam McNally <sammc@...omium.org>,
        Mark Brown <broonie@...nel.org>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Daniel Stuart <daniel.stuart14@...il.com>,
        "yuhsuan@...gle.com" <yuhsuan@...gle.com>,
        Damian van Soelen <dj.vsoelen@...il.com>
Subject: Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board

On Tue, 11 Aug 2020 10:25:22 +0200,
Yu-Hsuan Hsu wrote:
> 
> Takashi Iwai <tiwai@...e.de> 於 2020年8月11日 週二 下午3:43寫道：
> >
> > On Tue, 11 Aug 2020 04:29:24 +0200,
> > Yu-Hsuan Hsu wrote:
> > >
> > > Lu, Brent <brent.lu@...el.com> 於 2020年8月11日 週二 上午10:17寫道：
> > > >
> > > > >
> > > > > Sorry for the late reply. CRAS does not set the period size when using it.
> > > > > The default period size is 256, which consumes the samples quickly(about 49627
> > > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > > > immidiately.
> > > > >
> > > > > According to Brent, the DSP is using 240 period regardless the hw_param. If the
> > > > > period size is 256, DSP will read 256 samples each time but only consume 240
> > > > > samples until the ring buffer of DSP is full. This behavior makes the samples in
> > > > > the ring buffer of kernel consumed quickly. (Not sure whether the explanation is
> > > > > correct. Need Brent to confirm it.)
> > > > >
> > > > > Unfortunately, we can not change the behavior of DSP. After some experiments,
> > > > > we found that the issue can be fixed if we set the period size to 240. With the
> > > > > same frequency as the DSP, the samples are consumed stably. Because everyone
> > > > > can trigger this issue when using the driver without setting the period size, we
> > > > > think it is a general issue that should be fixed in the kernel.
> > > >
> > > > I check the code and just realized CRAS does nothing but request maximum buffer
> > > > size. As I know the application needs to decide the buffer time and period time so
> > > > ALSA could generate a hw_param structure with proper period size instead of using
> > > > fixed constraint in machine driver because driver has no idea about the latency you
> > > > want.
> > > >
> > > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > > > buffer and period parameters according to the latency requirement. In the CRAS
> > > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it on
> > > > Celes and it looks quite promising. It seems to me that adding constraint in machine
> > > > driver is not necessary.
> > > >
> > > > SectionDevice."Speaker".0 {
> > > >         Value {
> > > >                 PlaybackPCM "hw:chtrt5650,0"
> > > >                 DmaPeriodMicrosecs "5000"
> > > > ...
> > > >
> > > > [   52.434761] sound pcmC1D0p: hw_param
> > > > [   52.434767] sound pcmC1D0p:   ACCESS 0x1
> > > > [   52.434770] sound pcmC1D0p:   FORMAT 0x4
> > > > [   52.434772] sound pcmC1D0p:   SUBFORMAT 0x1
> > > > [   52.434776] sound pcmC1D0p:   SAMPLE_BITS [16:16]
> > > > [   52.434779] sound pcmC1D0p:   FRAME_BITS [32:32]
> > > > [   52.434782] sound pcmC1D0p:   CHANNELS [2:2]
> > > > [   52.434785] sound pcmC1D0p:   RATE [48000:48000]
> > > > [   52.434788] sound pcmC1D0p:   PERIOD_TIME [5000:5000]
> > > > [   52.434791] sound pcmC1D0p:   PERIOD_SIZE [240:240]
> > > > [   52.434794] sound pcmC1D0p:   PERIOD_BYTES [960:960]
> > > > [   52.434797] sound pcmC1D0p:   PERIODS [852:852]
> > > > [   52.434799] sound pcmC1D0p:   BUFFER_TIME [4260000:4260000]
> > > > [   52.434802] sound pcmC1D0p:   BUFFER_SIZE [204480:204480]
> > > > [   52.434805] sound pcmC1D0p:   BUFFER_BYTES [817920:817920]
> > > > [   52.434808] sound pcmC1D0p:   TICK_TIME [0:0]
> > > >
> > > > Regards,
> > > > Brent
> > > Hi Brent,
> > >
> > > Yes, I know we can do it to fix the issue as well. As I mentioned
> > > before, we wanted to fix it in kernel because it is a real issue,
> > > isn't it? Basically, a driver should work with any param it supports.
> > > But in this case, everyone can trigger underrun if he or she does not
> > > the period size to 240. If you still think it's not necessary, I can
> > > modify UCM to make CRAS set the appropriate period size.
> >
> > How does it *not* work if you set other than period size 240, more
> > exactly?
> >
> > The hw_constraint to a fixed period size must be really an exception.
> > If you look at other drivers, you won't find any other doing such.
> > It already indicates that something is wrong.
> >
> > Usually the fixed period size comes from the hardware limitation and
> > defined in snd_pcm_hardware.  Or, sometimes it's an alignment issue.
> > If you need more than that, you should doubt what's really not
> > working.
> >
> >
> > Takashi
> Thank Takashi,
> 
> As I mentioned before, if the period size is set to 256, the measured
> rate of sample-consuming will be around 49627 fps. It causes underrun
> because the rate we set is 48000 fps.

But this explanation rather sounds like the alignment problem.
However...

> This behavior also happen on the
> other period rate except for 240.

... Why only 240?  That's the next logical question.
If you have a clarification for it, it may be the rigid reason to
introduce such a hw constraint.


Takashi

Powered by blists - more mailing lists