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

[Jaroslav Kysela <perex@xxxxxxxx>]

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.

> > 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.

> > --- 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.

					Jaroslav

--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.