Re: [PATCH] ALSA: pcm: reinvent the stream synchronization ID API

[Takashi Iwai <tiwai@suse.de>]

On Thu, 02 May 2024 10:02:36 +0200,
Jaroslav Kysela wrote:
> 
> On 02. 05. 24 9:31, Takashi Iwai wrote:
> > On Tue, 30 Apr 2024 18:10:12 +0200,
> > 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.
> > 
> > What's the benefit of moving to hw_params?  The current inquiry via
> > ioctl can also follow the dynamic change, no?  I'm not entirely
> > objecting to the idea, but need to understand better.
> 
> In the removed code, there was a comment from Abramo that this ioctl
> is not so much appropriate to carry this information because it
> depends on the runtime status. Also, this info ioctl is callable
> through the control API thus the result depends on the stream open
> state. The revert is just a simple solution, but given the fact that
> the API was suspended for several years, we can do this cleanup now
> IMHO.

OK, makes sense.

> >> It seems that pipewire can benefit from this API (disable adaptive
> >> resampling for perfectly synchronized PCM streams) now.
> > 
> > If the support by pipewire is planned, it'd be good to revive the
> > feature, yes.
> 
> Pipewire would like to be able to detect the synchronized streams and
> Wim is aware about sync_id API, but it does not work now without
> obsoleting this in alsa-lib. I think that we did a wrong job here.

OK.

> >> --- a/include/uapi/sound/asound.h
> >> +++ b/include/uapi/sound/asound.h
> > (snip)
> >> @@ -334,6 +334,7 @@ union snd_pcm_sync_id {
> >>   	unsigned char id[16];
> >>   	unsigned short id16[8];
> >>   	unsigned int id32[4];
> >> +	__u64 id64[2];
> >>   };
> > 
> > What's the reason for this __u64 addition?  For the easiness of
> > zero-compare?
> 
> Basically yes, but this extensions is aligned with other fields. Also,
> I think that it may be usable to work with 64-bit words here. Perhaps,
> other fields may use __uXX declarations, too.

IMO, if we are allowed to redesign, it'd be better to avoid a union
for an API call.  It's a nightmare if one needs a conversion of API
(e.g. between different endianess for emulator).


thanks,

Takashi