| Message ID | 20240430161012.4011064-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: pcm: reinvent the stream synchronization ID API | expand |
Hi, On Tue, Apr 30, 2024 at 06:10:12PM +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. > > It seems that pipewire can benefit from this API (disable adaptive > resampling for perfectly synchronized PCM streams) now. > > Cc: Takashi Sakamoto <takaswie@kernel.org> > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > --- > include/sound/pcm.h | 9 +++++++++ > include/uapi/sound/asound.h | 8 +++++--- > sound/core/pcm_lib.c | 13 +++++++++++++ > sound/core/pcm_native.c | 6 ++++++ > 4 files changed, 33 insertions(+), 3 deletions(-) Thanks for the reference to the commit to cancel exposing the data of snd_pcm_sync_id union in runtime of PCM substream. I'm waiting for seven years and it is time to delete it from the runtime structure and post the series of changes for it[1]. I have no objection about the reinvention of some parts of UAPI to provide any information between several PCM substreams. However, the reuse of snd_pcm_sync_id union is a bit disapproving idea, since the data have not been utilized effectively past 25 years or so. The re-introduction would be a kind of 'history repeats'. I think this is a good opportunity to invent for your purpose. Let us work for more sophisticated way? [1] https://lore.kernel.org/linux-sound/20240501113445.100817-1-o-takashi@sakamocchi.jp/ Regards Takashi Sakamoto (not sound subsystem maintainer)
On 01. 05. 24 13:44, Takashi Sakamoto wrote: > Hi, > > On Tue, Apr 30, 2024 at 06:10:12PM +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. >> >> It seems that pipewire can benefit from this API (disable adaptive >> resampling for perfectly synchronized PCM streams) now. >> >> Cc: Takashi Sakamoto <takaswie@kernel.org> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz> >> --- >> include/sound/pcm.h | 9 +++++++++ >> include/uapi/sound/asound.h | 8 +++++--- >> sound/core/pcm_lib.c | 13 +++++++++++++ >> sound/core/pcm_native.c | 6 ++++++ >> 4 files changed, 33 insertions(+), 3 deletions(-) > > Thanks for the reference to the commit to cancel exposing the data of > snd_pcm_sync_id union in runtime of PCM substream. I'm waiting for seven > years and it is time to delete it from the runtime structure and post the > series of changes for it[1]. > > I have no objection about the reinvention of some parts of UAPI to > provide any information between several PCM substreams. However, the reuse > of snd_pcm_sync_id union is a bit disapproving idea, since the data have > not been utilized effectively past 25 years or so. The re-introduction > would be a kind of 'history repeats'. > > I think this is a good opportunity to invent for your purpose. Let us > work for more sophisticated way? Sorry, but I really do not follow your intention. You're proposing to remove something which can be used in pipewire those days and I just proposed to use this sync_id scheme. If you propose different API, implement it as first and then remove the current proposed and implemented scheme (at least in the old drivers). Removal and no replacement looks as no good solution. If you have a better API proposal, just go ahead. Jaroslav
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. > 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. > > --- 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? thanks, Takashi
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
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
On 02. 05. 24 17:27, Takashi Iwai wrote: >>>> --- 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). It makes sense. I'll keep only unsigned char id then. Jaroslav
On 4/30/2024 6:10 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. > > Cc: Takashi Sakamoto <takaswie@kernel.org> > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > --- > include/sound/pcm.h | 9 +++++++++ > include/uapi/sound/asound.h | 8 +++++--- > sound/core/pcm_lib.c | 13 +++++++++++++ > sound/core/pcm_native.c | 6 ++++++ > 4 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index 210096f124ee..f716e0d0d174 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -93,6 +93,7 @@ struct snd_pcm_ops { > #define SNDRV_PCM_IOCTL1_CHANNEL_INFO 2 > /* 3 is absent slot. */ > #define SNDRV_PCM_IOCTL1_FIFO_SIZE 4 > +#define SNDRV_PCM_IOCTL1_SYNC_ID 5 > > #define SNDRV_PCM_TRIGGER_STOP 0 > #define SNDRV_PCM_TRIGGER_START 1 > @@ -1565,6 +1566,14 @@ static inline u64 pcm_format_to_bits(snd_pcm_format_t pcm_format) > (__force int)(f) <= (__force int)SNDRV_PCM_FORMAT_LAST; \ > (f) = (__force snd_pcm_format_t)((__force int)(f) + 1)) > > +/** > + * is sync id (clock id) empty? > + */ > +static inline bool pcm_sync_empty(union snd_pcm_sync_id *sync) > +{ > + return sync->id64[0] == 0 && sync->id64[1] == 0; Can you use SNDRV_PCM_STREAM_PLAYBACK and SNDRV_PCM_STREAM_CAPTURE here, for someone not familiar with ALSA code above will be confusing. > +} > + > /* printk helpers */ > #define pcm_err(pcm, fmt, args...) \ > dev_err((pcm)->card->dev, fmt, ##args) > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 628d46a0da92..13f8a08f3f33 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image { > * * > *****************************************************************************/ > > -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 17) > +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 18) > > typedef unsigned long snd_pcm_uframes_t; > typedef signed long snd_pcm_sframes_t; > @@ -334,6 +334,7 @@ union snd_pcm_sync_id { > unsigned char id[16]; > unsigned short id16[8]; > unsigned int id32[4]; > + __u64 id64[2]; Similarly here, can SNDRV_PCM_STREAM_LAST be used?
On 06. 05. 24 11:02, Amadeusz Sławiński wrote: > On 4/30/2024 6:10 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. >> >> Cc: Takashi Sakamoto <takaswie@kernel.org> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz> >> --- >> include/sound/pcm.h | 9 +++++++++ >> include/uapi/sound/asound.h | 8 +++++--- >> sound/core/pcm_lib.c | 13 +++++++++++++ >> sound/core/pcm_native.c | 6 ++++++ >> 4 files changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/include/sound/pcm.h b/include/sound/pcm.h >> index 210096f124ee..f716e0d0d174 100644 >> --- a/include/sound/pcm.h >> +++ b/include/sound/pcm.h >> @@ -93,6 +93,7 @@ struct snd_pcm_ops { >> #define SNDRV_PCM_IOCTL1_CHANNEL_INFO 2 >> /* 3 is absent slot. */ >> #define SNDRV_PCM_IOCTL1_FIFO_SIZE 4 >> +#define SNDRV_PCM_IOCTL1_SYNC_ID 5 >> >> #define SNDRV_PCM_TRIGGER_STOP 0 >> #define SNDRV_PCM_TRIGGER_START 1 >> @@ -1565,6 +1566,14 @@ static inline u64 pcm_format_to_bits(snd_pcm_format_t pcm_format) >> (__force int)(f) <= (__force int)SNDRV_PCM_FORMAT_LAST; \ >> (f) = (__force snd_pcm_format_t)((__force int)(f) + 1)) >> >> +/** >> + * is sync id (clock id) empty? >> + */ >> +static inline bool pcm_sync_empty(union snd_pcm_sync_id *sync) >> +{ >> + return sync->id64[0] == 0 && sync->id64[1] == 0; > > Can you use SNDRV_PCM_STREAM_PLAYBACK and SNDRV_PCM_STREAM_CAPTURE here, > for someone not familiar with ALSA code above will be confusing. It's not related to stream direction. Anyway, I'll send an updated patch based on Takashi's comment soon with some other changes. Thanks, Jaroslav
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 210096f124ee..f716e0d0d174 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -93,6 +93,7 @@ struct snd_pcm_ops { #define SNDRV_PCM_IOCTL1_CHANNEL_INFO 2 /* 3 is absent slot. */ #define SNDRV_PCM_IOCTL1_FIFO_SIZE 4 +#define SNDRV_PCM_IOCTL1_SYNC_ID 5 #define SNDRV_PCM_TRIGGER_STOP 0 #define SNDRV_PCM_TRIGGER_START 1 @@ -1565,6 +1566,14 @@ static inline u64 pcm_format_to_bits(snd_pcm_format_t pcm_format) (__force int)(f) <= (__force int)SNDRV_PCM_FORMAT_LAST; \ (f) = (__force snd_pcm_format_t)((__force int)(f) + 1)) +/** + * is sync id (clock id) empty? + */ +static inline bool pcm_sync_empty(union snd_pcm_sync_id *sync) +{ + return sync->id64[0] == 0 && sync->id64[1] == 0; +} + /* printk helpers */ #define pcm_err(pcm, fmt, args...) \ dev_err((pcm)->card->dev, fmt, ##args) diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 628d46a0da92..13f8a08f3f33 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/ -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 17) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 18) typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -334,6 +334,7 @@ union snd_pcm_sync_id { unsigned char id[16]; unsigned short id16[8]; unsigned int id32[4]; + __u64 id64[2]; }; struct snd_pcm_info { @@ -348,7 +349,7 @@ struct snd_pcm_info { int dev_subclass; /* SNDRV_PCM_SUBCLASS_* */ unsigned int subdevices_count; unsigned int subdevices_avail; - union snd_pcm_sync_id sync; /* hardware synchronization ID */ + unsigned char pad1[16]; /* was: hardware synchronization ID */ unsigned char reserved[64]; /* reserved for future... */ }; @@ -420,7 +421,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 */ + union snd_pcm_sync_id sync; /* R: synchronization ID (perfect sync - one clock source) */ + unsigned char reserved[48]; /* reserved for future */ }; enum { diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 6f73b3c2c205..8ae63e4cc5a5 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1810,6 +1810,17 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream, return 0; } +static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream, + void *arg) +{ + struct snd_pcm_hw_params *params = arg; + union snd_pcm_sync_id *sync = ¶ms->sync; + + if (pcm_sync_empty(sync)) + memcpy(sync, &substream->runtime->sync, sizeof(*sync)); + return 0; +} + /** * snd_pcm_lib_ioctl - a generic PCM ioctl callback * @substream: the pcm substream instance @@ -1831,6 +1842,8 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream, return snd_pcm_lib_ioctl_channel_info(substream, arg); case SNDRV_PCM_IOCTL1_FIFO_SIZE: return snd_pcm_lib_ioctl_fifo_size(substream, arg); + case SNDRV_PCM_IOCTL1_SYNC_ID: + return snd_pcm_lib_ioctl_sync_id(substream, arg); } return -ENXIO; } diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0b76e76823d2..63fcb08ee93d 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -533,6 +533,12 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, SNDRV_PCM_INFO_MMAP_VALID); } + err = snd_pcm_ops_ioctl(substream, + SNDRV_PCM_IOCTL1_SYNC_ID, + params); + if (err < 0) + return err; + return 0; }
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. Cc: Takashi Sakamoto <takaswie@kernel.org> Signed-off-by: Jaroslav Kysela <perex@perex.cz> --- include/sound/pcm.h | 9 +++++++++ include/uapi/sound/asound.h | 8 +++++--- sound/core/pcm_lib.c | 13 +++++++++++++ sound/core/pcm_native.c | 6 ++++++ 4 files changed, 33 insertions(+), 3 deletions(-)