| Message ID | 1540826695-16021-1-git-send-email-twischer@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pcm: dmix: Align slave_hw_ptr to slave period boundary | expand |
On Mon, 29 Oct 2018 16:24:55 +0100, <twischer@de.adit-jv.com> wrote: > > From: Laxmi Devi <Laxmi.Devi@in.bosch.com> > > These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4 > ("ASoC: rsnd: update pointer more accurate") > > Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr > is not period aligned. Therefore snd_pcm_wait() will block for a longer > time as required. > > With these rcar driver changes the exact position of the dma is returned. > During snd_pcm_start they read hw_ptr as reference, and this hw_ptr > is now not period aligned, and is a little ahead over the period while it > is read. Therefore when the avail is calculated during snd_pcm_wait(), > it is missing the avail_min by a few frames. > Consider the below example: > > Considering the application is trying to write 0x120 frames and the > period_size = 0x60, avail_min = 0x120 and buffersize = 0x360 : > > rsnd_pointer=0x12c -> dma pointer at the end of second write during > snd_pcm_dmix_start(). > Since another 0x120 buffer is available, application writes 0x120 and goes > to snd_pcm_wait(). > It is woken up after 3 snd_pcm_period_elapsed() to see rsnd_pointer=0x248. > So hw_ptr = new_slave_hw_ptr - reference_slave_hw_ptr = 0x248 - 0x12c = 0x11c. > It needs 4 more frames to be able to write. And so it goes back to waiting. > > But since 3 snd_pcm_period_elapsed(), 3 periods should be available and it > should have been able to write. Well, no, snd_pcm_period_elapsed() calls don't guarantee the expectation above in the case of dmix. It's clearer to assume that the stream were started just one frame before the next period, for example. > If rsnd_pointer during the start was 0x120 which is 3 periods > then 0x248 - 0x120 = 128 it could go on with write. > > Signed-off-by: Laxmi Devi <Laxmi.Devi@in.bosch.com> > Signed-off-by: Timo Wischer <twischer@de.adit-jv.com> > --- > We are not completely sure if this is the best approach but we had also no > better idea to solve this problem (with similar CPU usage). > > Therefore if anyone has a better solution please feel free to describe > this one. The problem is that aligning the start essentially imposes an artificial long latency, and changes the behavior out of sudden. Hence I'm inclined to make it optional; e.g. create a dmix config align_hw_ptr, and let user decide. It'll be a string, and as default (e.g. "auto"), we should keep the current behavior. For other values, it can be translated as a boolean (call snd_config_get_bool_ascii()) to choose whether to align or not. thanks, Takashi > > src/pcm/pcm_dmix.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c > index a6a8f3a..d3e9319 100644 > --- a/src/pcm/pcm_dmix.c > +++ b/src/pcm/pcm_dmix.c > @@ -560,11 +560,10 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm) > static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) > { > dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; > - if (pcm->buffer_size > pcm->period_size * 2) > - return; > - /* If we have too litte periods, better to align the start position > - * to the period boundary so that the interrupt can be handled properly > - * at the right time. > + dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size) > + * dmix->slave_period_size); > + /* Better to align the start position to the period boundary so that > + * the interrupt can be handled properly at the right time. > */ > dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) > / dmix->slave_period_size) * dmix->slave_period_size; > -- > 2.7.4 >
diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index a6a8f3a..d3e9319 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -560,11 +560,10 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm) static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix) { dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr; - if (pcm->buffer_size > pcm->period_size * 2) - return; - /* If we have too litte periods, better to align the start position - * to the period boundary so that the interrupt can be handled properly - * at the right time. + dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size) + * dmix->slave_period_size); + /* Better to align the start position to the period boundary so that + * the interrupt can be handled properly at the right time. */ dmix->slave_appl_ptr = ((dmix->slave_appl_ptr + dmix->slave_period_size - 1) / dmix->slave_period_size) * dmix->slave_period_size;