[alsa-devel] [PATCH 0/3] ALSA: Add the explicit appl_ptr sync support

[Takashi Sakamoto]

Hi,

On Jun 23 2017 01:06, Takashi Iwai wrote:
> On Thu, 22 Jun 2017 17:35:34 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Jun 22 2017 17:44, Takashi Iwai wrote:
>>>>> Currently alsa-lib issues sync_ptr() also for obtaining the PCM state
>>>>> from the kernel, and this also triggers ack unless SYNC_APPL_PTR flag
>>>>> is set.  The commit f8ff2f28ba49 tried to reduce the ack calls  by
>>>>> filtering in pcm_lib_apply_appl_ptr() side, but it has a side-effect
>>>>> as you noticed.
>>>>>
>>>>> There can be a few options to cover this for the newer alsa-lib, for
>>>>> example:
>>>>>
>>>>> A. Add a new bit flag to sync_ptr() allowing only the state update
>>>>>       and/or the explicit applptr update.
>>>>>
>>>>> B. Introduce a completely new ioctl for the appl_ptr sync.
>>>>>
>>>>> C. Keep disabling control mmap while allowing state mmap.
>>>>>
>>>>> D. Track the last appl_ptr individually from runtime->control->appl_ptr
>>>>>       and compare/update with this value in pcm_lib_apply_appl_ptr().
>>>>>
>>>>> E. Remove the filtering from pcm_lib_apply_appl_ptr(), let each driver
>>>>>       optimize if needed.
>>>>>
>>>>> Currently I'm thinking of (A), but open for all and other possible
>>>>> options.  All these may need the check of user_pversion so that the
>>>>> old code still works as is.
>>>>
>>>> I prefer C) and I'm working for it, because protocol validation of
>>>> userland is not needed in kernel land, as long as I understand. But if
>>>> allowed to add more changes to protocol/interface, A) and B) could be
>>>> within my candidates.
>>>
>>> (C) was my original idea, too, so it doesn't sound bad to me, too :)
>>> But, we're bumping the protocol version, so more intensive changes
>>> should be OK.  Let's see.
>>
>> I posted patchset for alsa-lib for your C) idea. In this patchset,
>> mapping for status/control data is processed independently,
>>
>> [alsa-devel] [alsa-lib][RFC][PATCH 0/9] pcm: handle status/control
>> mapping independently
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122130.html
> 
> The patchset looks good through a quick glance.
> I'd need to test it more intensively later, but I'm basically fine to
> go in that direction.  In general, this change wouldn't conflict with
> other improvements.

Thanks for your positive comment. I'm preparing for new patchset with my 
further self-check.

>> Additionally, some helper functions to call ioctl(2) with
>> SNDRV_PCM_IOCTL_SYNC_PTR for several purposes. I think to get help
>> from it for our arrangement to A) and B).
>>
>> I agree with the idea in A) to add a flag to update state only. For
>> the other probabilities, I need time to consider about it.
> 
> Yeah, let's take some time.
> 
> Meanwhile, I'd like to merge the basic part in order to make
> maintenance easier.  Let's sort out which ones to be applied right
> now.
> 
> At least, the following one should be OK:
> 
>    [PATCH v2 1/3] ALSA: pcm: Add the explicit appl_ptr sync support
> 
> May I re-apply your reviewed-by tag for this?

I did it just now. Let's merge it.

> For alsa-lib side, a few of yours and my patches are merely cleanups,
> so we should apply them beforehand, too.
> 
>    [PATCH alsa-lib 3/4] pcm: hw: Remove superfluous call of snd_pcm_set_appl_ptr()
>    [alsa-lib][RFC][PATCH 1/9] pcm: obsolete 'mmap_emulation' parameter of snd_pcm_hw_open_fd()
>    [alsa-lib][RFC][PATCH 2/9] pcm: minor code refactoring for ioctl call

I'm OK to apply the cleanups. I already reviewed the first patch so 
let's merge it. I'll post the rest of two without RFC rebasing to 
current master a few hours later.


Thanks

Takashi Sakamoto