Re: [PATCH v5 1/2] ALSA: pcm: reinvent the stream synchronization ID API

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 6/24/2024 3:56 PM, Jaroslav Kysela wrote:
> On 24. 06. 24 15:38, Amadeusz Sławiński wrote:
>> On 6/24/2024 3:07 PM, Jaroslav Kysela wrote:
>>> Until the commit e11f0f90a626 ("ALSA: pcm: remove SNDRV_PCM_IOCTL1_INFO
>>> internal command"), there was a possibility to pass information
>>> about the synchronized streams to the user space. The mentioned
>>> commit removed blindly the appropriate code with an irrelevant comment.
>>>
>>> The revert may be appropriate, but since this API was lost for several
>>> years without any complains, it's time to improve it. The hardware
>>> parameters may change the used stream clock source (e.g. USB hardware)
>>> so move this synchronization ID to hw_params as read-only field.
>>>
>>> It seems that pipewire can benefit from this API (disable adaptive
>>> resampling for perfectly synchronized PCM streams) now.
>>>
>>> Note that the contents of ID is not supposed to be used for direct
>>> comparison with a specific byte sequence. The "empty" case is when
>>> all bytes are zero (driver does not offer this information)
>>> and all other cases must be only used for equal comparison among
>>> PCM streams (including different sound cards) if they are using
>>> identical hardware clock.
>>>
>>> Cc: Takashi Sakamoto <takaswie@kernel.org>
>>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
>>> ---
>>
>> (...)
>>
>>> @@ -420,7 +420,8 @@ struct snd_pcm_hw_params {
>>>        unsigned int rate_num;        /* R: rate numerator */
>>>        unsigned int rate_den;        /* R: rate denominator */
>>>        snd_pcm_uframes_t fifo_size;    /* R: chip FIFO size in frames */
>>> -    unsigned char reserved[64];    /* reserved for future */
>>> +    unsigned char sync[16];        /* R: synchronization ID (perfect 
>>> sync - one clock source) */
>>
>> If it is introduced as new API, can't this be done better? Maybe like:
>> struct sync {
>>     char cardnum[4];
>>     char id[12];
>> };
>> or maybe just:
>> struct sync {
>>     u32 cardnum;
>>     char id[12];
>> };
>> or something like that? It is bit hard to follow in next patch all this
>> params->sync + 4 and memset/strncpy. And having named fields would help.0
> 
> The ID may be not related to one sound card. Multiple cards can share 
> one clock source. It's internal kernel ID generation scheme which may be 
> changed later when there's another demand in future. The applications 
> should handle this as 16-byte blob which is used only for equal 
> comparison among multiple PCM streams.

Ah, yes rereading it few more times it makes sense.

Thanks,
Amadeusz