* [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API
@ 2024-05-07 13:30 Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 1/2] " Jaroslav Kysela
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2024-05-07 13:30 UTC (permalink / raw
To: linux-sound; +Cc: Takashi Iwai, Jaroslav Kysela
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.
v3->v4:
- more code shuffle as suggested by Takashi
- remove unused snd_pcm_empty function in the second patch
- put back snd_pcm_set_sync documentation
v2->v3:
- fix pcm_sync_empty() function (wrong comparison) [thanks Takashi Sakamoto]
- more documentation for snd_pcm_set_sync_per_card (ID composition)
v1->v2:
- remove union usage per Takashi's request
- reduce memory usage
- use standard ID generation scheme
Jaroslav Kysela (2):
ALSA: pcm: reinvent the stream synchronization ID API
ALSA: pcm: optimize and clarify stream sychronization ID API
include/sound/pcm.h | 17 +++++++++++--
include/uapi/sound/asound.h | 13 +++-------
sound/core/pcm_lib.c | 50 +++++++++++++++++++++++++++++--------
sound/core/pcm_native.c | 6 +++++
sound/pci/emu10k1/p16v.c | 17 +++++++++----
5 files changed, 77 insertions(+), 26 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-05-07 13:30 [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API Jaroslav Kysela
@ 2024-05-07 13:30 ` Jaroslav Kysela
2024-06-13 12:56 ` Takashi Sakamoto
2024-05-07 13:30 ` [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization " Jaroslav Kysela
2024-06-13 12:14 ` [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization " Takashi Iwai
2 siblings, 1 reply; 9+ messages in thread
From: Jaroslav Kysela @ 2024-05-07 13:30 UTC (permalink / raw
To: linux-sound; +Cc: Takashi Iwai, Jaroslav Kysela, Takashi Sakamoto
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 | 3 ++-
include/uapi/sound/asound.h | 13 ++++---------
sound/core/pcm_lib.c | 27 +++++++++++++++++++++++----
sound/core/pcm_native.c | 6 ++++++
sound/pci/emu10k1/p16v.c | 7 +++----
5 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 210096f124ee..f187d4c9b420 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
@@ -396,7 +397,7 @@ struct snd_pcm_runtime {
snd_pcm_uframes_t silence_start; /* starting pointer to silence area */
snd_pcm_uframes_t silence_filled; /* already filled part of silence area */
- union snd_pcm_sync_id sync; /* hardware synchronization ID */
+ unsigned char sync[16]; /* hardware synchronization ID */
/* -- mmap -- */
struct snd_pcm_mmap_status *status;
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 628d46a0da92..c458172b32d5 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;
@@ -330,12 +330,6 @@ enum {
#endif
};
-union snd_pcm_sync_id {
- unsigned char id[16];
- unsigned short id16[8];
- unsigned int id32[4];
-};
-
struct snd_pcm_info {
unsigned int device; /* RO/WR (control): device number */
unsigned int subdevice; /* RO/WR (control): subdevice number */
@@ -348,7 +342,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 +414,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) */
+ unsigned char reserved[48]; /* reserved for future */
};
enum {
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 6f73b3c2c205..57ed4ffc891a 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -5,6 +5,7 @@
* Abramo Bagnara <abramo@alsa-project.org>
*/
+#include <asm/unaligned.h>
#include <linux/slab.h>
#include <linux/sched/signal.h>
#include <linux/time.h>
@@ -525,10 +526,8 @@ void snd_pcm_set_sync(struct snd_pcm_substream *substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
- runtime->sync.id32[0] = substream->pcm->card->number;
- runtime->sync.id32[1] = -1;
- runtime->sync.id32[2] = -1;
- runtime->sync.id32[3] = -1;
+ *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
+ memset(runtime->sync + 4, 0xff, sizeof(runtime->sync) - 4);
}
EXPORT_SYMBOL(snd_pcm_set_sync);
@@ -1810,6 +1809,24 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
return 0;
}
+/**
+ * is sync id (clock id) empty?
+ */
+static inline bool pcm_sync_empty(const unsigned char *sync)
+{
+ return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
+}
+
+static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
+ void *arg)
+{
+ struct snd_pcm_hw_params *params = arg;
+
+ if (pcm_sync_empty(params->sync))
+ memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
+ return 0;
+}
+
/**
* snd_pcm_lib_ioctl - a generic PCM ioctl callback
* @substream: the pcm substream instance
@@ -1831,6 +1848,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;
}
diff --git a/sound/pci/emu10k1/p16v.c b/sound/pci/emu10k1/p16v.c
index e7f097cae574..773725dbdfbd 100644
--- a/sound/pci/emu10k1/p16v.c
+++ b/sound/pci/emu10k1/p16v.c
@@ -174,10 +174,9 @@ static int snd_p16v_pcm_open_playback_channel(struct snd_pcm_substream *substrea
if (err < 0)
return err;
- runtime->sync.id32[0] = substream->pcm->card->number;
- runtime->sync.id32[1] = 'P';
- runtime->sync.id32[2] = 16;
- runtime->sync.id32[3] = 'V';
+ *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
+ memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
+ strncpy(runtime->sync + 4, "P16V", 4);
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization ID API
2024-05-07 13:30 [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 1/2] " Jaroslav Kysela
@ 2024-05-07 13:30 ` Jaroslav Kysela
2024-06-13 13:00 ` Takashi Sakamoto
2024-06-13 12:14 ` [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization " Takashi Iwai
2 siblings, 1 reply; 9+ messages in thread
From: Jaroslav Kysela @ 2024-05-07 13:30 UTC (permalink / raw
To: linux-sound; +Cc: Takashi Iwai, Jaroslav Kysela
Optimize the memory usage in struct snd_pcm_runtime - use boolean
value for the standard sync ID scheme.
Introduce snd_pcm_set_sync_per_card function to build synchronization
IDs.
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
include/sound/pcm.h | 16 +++++++++++--
sound/core/pcm_lib.c | 51 ++++++++++++++++++++++++----------------
sound/pci/emu10k1/p16v.c | 16 +++++++++----
3 files changed, 57 insertions(+), 26 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index f187d4c9b420..66cb470490fe 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -397,7 +397,7 @@ struct snd_pcm_runtime {
snd_pcm_uframes_t silence_start; /* starting pointer to silence area */
snd_pcm_uframes_t silence_filled; /* already filled part of silence area */
- unsigned char sync[16]; /* hardware synchronization ID */
+ bool sync_flag; /* hardware synchronization - standard per card ID */
/* -- mmap -- */
struct snd_pcm_mmap_status *status;
@@ -1151,7 +1151,19 @@ int snd_pcm_format_set_silence(snd_pcm_format_t format, void *buf, unsigned int
void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
const struct snd_pcm_ops *ops);
-void snd_pcm_set_sync(struct snd_pcm_substream *substream);
+void snd_pcm_set_sync_per_card(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params,
+ const unsigned char *id, unsigned int len);
+/**
+ * snd_pcm_set_sync - set the PCM sync id
+ * snd_pcm_set_sync_per_card - set the PCM sync id with card number
+ * @substream: the pcm substream
+ *
+ * Use the default PCM sync identifier for the specific card.
+ */
+static inline void snd_pcm_set_sync(struct snd_pcm_substream *substream)
+{
+ substream->runtime->sync_flag = true;
+}
int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
unsigned int cmd, void *arg);
void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substream);
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 57ed4ffc891a..8d1a24a2c43d 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -5,7 +5,6 @@
* Abramo Bagnara <abramo@alsa-project.org>
*/
-#include <asm/unaligned.h>
#include <linux/slab.h>
#include <linux/sched/signal.h>
#include <linux/time.h>
@@ -517,19 +516,37 @@ void snd_pcm_set_ops(struct snd_pcm *pcm, int direction,
EXPORT_SYMBOL(snd_pcm_set_ops);
/**
- * snd_pcm_set_sync - set the PCM sync id
+ * snd_pcm_set_sync_per_card - set the PCM sync id with card number
* @substream: the pcm substream
+ * @params: modified hardware parameters
+ * @id: identifier (max 12 bytes)
+ * @len: identifier length (max 12 bytes)
*
- * Sets the PCM sync identifier for the card.
+ * Sets the PCM sync identifier for the card with zero padding.
+ *
+ * User space or any user should use this 16-byte identifier for a comparison only
+ * to check if two IDs are similar or different. Special case is the identifier
+ * containing only zeros. Interpretation for this combination is - empty (not set).
+ * The contents of the identifier should not be interpreted in any other way.
+ *
+ * The synchronization ID must be unique per clock source (usually one sound card,
+ * but multiple soundcard may use one PCM word clock source which means that they
+ * are fully synchronized).
+ *
+ * This routine composes this ID using card number in first four bytes and
+ * 12-byte additional ID. When other ID composition is used (e.g. for multiple
+ * sound cards), make sure that the composition does not clash with this
+ * composition scheme.
*/
-void snd_pcm_set_sync(struct snd_pcm_substream *substream)
+void snd_pcm_set_sync_per_card(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ const unsigned char *id, unsigned int len)
{
- struct snd_pcm_runtime *runtime = substream->runtime;
-
- *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
- memset(runtime->sync + 4, 0xff, sizeof(runtime->sync) - 4);
+ *(__u32 *)params->sync = cpu_to_le32(substream->pcm->card->number);
+ len = max(12, len);
+ strncpy(params->sync + 4, id, len);
+ memset(params->sync + 4 + len, 0, 12 - len);
}
-EXPORT_SYMBOL(snd_pcm_set_sync);
/*
* Standard ioctl routine
@@ -1809,21 +1826,15 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
return 0;
}
-/**
- * is sync id (clock id) empty?
- */
-static inline bool pcm_sync_empty(const unsigned char *sync)
-{
- return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
-}
-
static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
void *arg)
{
- struct snd_pcm_hw_params *params = arg;
+ static const unsigned char id[12] = { 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff };
- if (pcm_sync_empty(params->sync))
- memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
+ if (substream->runtime->sync_flag)
+ snd_pcm_set_sync_per_card(substream, arg, id, sizeof(id));
return 0;
}
diff --git a/sound/pci/emu10k1/p16v.c b/sound/pci/emu10k1/p16v.c
index 773725dbdfbd..a9a75891f1da 100644
--- a/sound/pci/emu10k1/p16v.c
+++ b/sound/pci/emu10k1/p16v.c
@@ -174,10 +174,6 @@ static int snd_p16v_pcm_open_playback_channel(struct snd_pcm_substream *substrea
if (err < 0)
return err;
- *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
- memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
- strncpy(runtime->sync + 4, "P16V", 4);
-
return 0;
}
@@ -225,6 +221,17 @@ static int snd_p16v_pcm_open_capture(struct snd_pcm_substream *substream)
return snd_p16v_pcm_open_capture_channel(substream, 0);
}
+static int snd_p16v_pcm_ioctl_playback(struct snd_pcm_substream *substream,
+ unsigned int cmd, void *arg)
+{
+ if (cmd == SNDRV_PCM_IOCTL1_SYNC_ID) {
+ static const unsigned char id[4] = { 'P', '1', '6', 'V' };
+ snd_pcm_set_sync_per_card(substream, arg, id, 4);
+ return 0;
+ }
+ return snd_pcm_lib_ioctl(substream, cmd, arg);
+}
+
/* prepare playback callback */
static int snd_p16v_pcm_prepare_playback(struct snd_pcm_substream *substream)
{
@@ -530,6 +537,7 @@ snd_p16v_pcm_pointer_capture(struct snd_pcm_substream *substream)
static const struct snd_pcm_ops snd_p16v_playback_front_ops = {
.open = snd_p16v_pcm_open_playback_front,
.close = snd_p16v_pcm_close_playback,
+ .ioctl = snd_p16v_pcm_ioctl_playback,
.prepare = snd_p16v_pcm_prepare_playback,
.trigger = snd_p16v_pcm_trigger_playback,
.pointer = snd_p16v_pcm_pointer_playback,
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-05-07 13:30 [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 1/2] " Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization " Jaroslav Kysela
@ 2024-06-13 12:14 ` Takashi Iwai
2 siblings, 0 replies; 9+ messages in thread
From: Takashi Iwai @ 2024-06-13 12:14 UTC (permalink / raw
To: Jaroslav Kysela; +Cc: linux-sound, Takashi Iwai
On Tue, 07 May 2024 15:30:49 +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.
>
> v3->v4:
> - more code shuffle as suggested by Takashi
> - remove unused snd_pcm_empty function in the second patch
> - put back snd_pcm_set_sync documentation
>
> v2->v3:
> - fix pcm_sync_empty() function (wrong comparison) [thanks Takashi Sakamoto]
> - more documentation for snd_pcm_set_sync_per_card (ID composition)
>
> v1->v2:
> - remove union usage per Takashi's request
> - reduce memory usage
> - use standard ID generation scheme
>
> Jaroslav Kysela (2):
> ALSA: pcm: reinvent the stream synchronization ID API
> ALSA: pcm: optimize and clarify stream sychronization ID API
Do people have any concerns about this proposal?
Otherwise I'm going to merge them for 6.11.
thanks,
Takashi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-05-07 13:30 ` [PATCH v4 1/2] " Jaroslav Kysela
@ 2024-06-13 12:56 ` Takashi Sakamoto
2024-06-13 14:20 ` Takashi Iwai
2024-06-24 13:17 ` Jaroslav Kysela
0 siblings, 2 replies; 9+ messages in thread
From: Takashi Sakamoto @ 2024-06-13 12:56 UTC (permalink / raw
To: Jaroslav Kysela; +Cc: linux-sound, Takashi Iwai, Takashi Sakamoto
Hi,
On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 628d46a0da92..c458172b32d5 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;
> @@ -330,12 +330,6 @@ enum {
> #endif
> };
>
> -union snd_pcm_sync_id {
> - unsigned char id[16];
> - unsigned short id16[8];
> - unsigned int id32[4];
> -};
It can bring FTBFS for any userspace application which uses the
structure. If getting rid of such public structure, we should have the
term to deprecate it for a while.
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 6f73b3c2c205..57ed4ffc891a 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -5,6 +5,7 @@
> * Abramo Bagnara <abramo@alsa-project.org>
> */
>
> +#include <asm/unaligned.h>
> #include <linux/slab.h>
> #include <linux/sched/signal.h>
> #include <linux/time.h>
> @@ -525,10 +526,8 @@ void snd_pcm_set_sync(struct snd_pcm_substream *substream)
> {
> struct snd_pcm_runtime *runtime = substream->runtime;
>
> - runtime->sync.id32[0] = substream->pcm->card->number;
> - runtime->sync.id32[1] = -1;
> - runtime->sync.id32[2] = -1;
> - runtime->sync.id32[3] = -1;
> + *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
> + memset(runtime->sync + 4, 0xff, sizeof(runtime->sync) - 4);
> }
> EXPORT_SYMBOL(snd_pcm_set_sync);
>
> @@ -1810,6 +1809,24 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
> return 0;
> }
>
> +/**
> + * is sync id (clock id) empty?
> + */
> +static inline bool pcm_sync_empty(const unsigned char *sync)
> +{
> + return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
> +}
This kind of usage of C inline qualifier is not useless, since the modern
compilers can optimize it into the local code.
> +static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
> + void *arg)
> +{
> + struct snd_pcm_hw_params *params = arg;
> +
> + if (pcm_sync_empty(params->sync))
> + memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
> + return 0;
> +}
Furthermore, it does not work as expected in the case that any of driver sets
zeros to char[16] intentionally for its own purpose at PCM .open callback.
I think it is a kind of 'Separation of mechanism and policy' argument,
and a kind of designs which should be avoided.
> /**
> * snd_pcm_lib_ioctl - a generic PCM ioctl callback
> * @substream: the pcm substream instance
> @@ -1831,6 +1848,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;
> }
>
> diff --git a/sound/pci/emu10k1/p16v.c b/sound/pci/emu10k1/p16v.c
> index e7f097cae574..773725dbdfbd 100644
> --- a/sound/pci/emu10k1/p16v.c
> +++ b/sound/pci/emu10k1/p16v.c
> @@ -174,10 +174,9 @@ static int snd_p16v_pcm_open_playback_channel(struct snd_pcm_substream *substrea
> if (err < 0)
> return err;
>
> - runtime->sync.id32[0] = substream->pcm->card->number;
> - runtime->sync.id32[1] = 'P';
> - runtime->sync.id32[2] = 16;
> - runtime->sync.id32[3] = 'V';
> + *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
> + memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
> + strncpy(runtime->sync + 4, "P16V", 4);
I'm sorry if I overlooked your reply to my point, however the above is
not equivalent.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization ID API
2024-05-07 13:30 ` [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization " Jaroslav Kysela
@ 2024-06-13 13:00 ` Takashi Sakamoto
0 siblings, 0 replies; 9+ messages in thread
From: Takashi Sakamoto @ 2024-06-13 13:00 UTC (permalink / raw
To: Jaroslav Kysela; +Cc: linux-sound, Takashi Iwai
On Tue, May 07, 2024 at 03:30:51PM +0200, Jaroslav Kysela wrote:
> Optimize the memory usage in struct snd_pcm_runtime - use boolean
> value for the standard sync ID scheme.
>
> Introduce snd_pcm_set_sync_per_card function to build synchronization
> IDs.
>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> ---
> include/sound/pcm.h | 16 +++++++++++--
> sound/core/pcm_lib.c | 51 ++++++++++++++++++++++++----------------
> sound/pci/emu10k1/p16v.c | 16 +++++++++----
> 3 files changed, 57 insertions(+), 26 deletions(-)
Ah... OK. The most of changes in the previous patch is dismissed. I
think it better to respin the patchset if it would be merged after
passing adequate reviewing process.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-06-13 12:56 ` Takashi Sakamoto
@ 2024-06-13 14:20 ` Takashi Iwai
2024-06-13 14:28 ` Jaroslav Kysela
2024-06-24 13:17 ` Jaroslav Kysela
1 sibling, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2024-06-13 14:20 UTC (permalink / raw
To: Takashi Sakamoto
Cc: Jaroslav Kysela, linux-sound, Takashi Iwai, Takashi Sakamoto
On Thu, 13 Jun 2024 14:56:49 +0200,
Takashi Sakamoto wrote:
>
> Hi,
>
> On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index 628d46a0da92..c458172b32d5 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;
> > @@ -330,12 +330,6 @@ enum {
> > #endif
> > };
> >
> > -union snd_pcm_sync_id {
> > - unsigned char id[16];
> > - unsigned short id16[8];
> > - unsigned int id32[4];
> > -};
>
> It can bring FTBFS for any userspace application which uses the
> structure. If getting rid of such public structure, we should have the
> term to deprecate it for a while.
A good point. We can keep the union definition there but add a
comment that it's deprecated and not actually used in any code.
Takashi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-06-13 14:20 ` Takashi Iwai
@ 2024-06-13 14:28 ` Jaroslav Kysela
0 siblings, 0 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2024-06-13 14:28 UTC (permalink / raw
To: Takashi Iwai; +Cc: linux-sound, Takashi Sakamoto
On 13. 06. 24 16:20, Takashi Iwai wrote:
> On Thu, 13 Jun 2024 14:56:49 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
>>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>>> index 628d46a0da92..c458172b32d5 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;
>>> @@ -330,12 +330,6 @@ enum {
>>> #endif
>>> };
>>>
>>> -union snd_pcm_sync_id {
>>> - unsigned char id[16];
>>> - unsigned short id16[8];
>>> - unsigned int id32[4];
>>> -};
>>
>> It can bring FTBFS for any userspace application which uses the
>> structure. If getting rid of such public structure, we should have the
>> term to deprecate it for a while.
>
> A good point. We can keep the union definition there but add a
> comment that it's deprecated and not actually used in any code.
The question is, if someone is using this directly (outside alsa-lib) when the
implementation was vanished for several years.
Anyway, I'll add __attribute__((deprecated)) to this structure in respin.
Thanks for the review.
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] ALSA: pcm: reinvent the stream synchronization ID API
2024-06-13 12:56 ` Takashi Sakamoto
2024-06-13 14:20 ` Takashi Iwai
@ 2024-06-24 13:17 ` Jaroslav Kysela
1 sibling, 0 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2024-06-24 13:17 UTC (permalink / raw
To: Takashi Sakamoto; +Cc: linux-sound, Takashi Iwai, Takashi Sakamoto
On 13. 06. 24 14:56, Takashi Sakamoto wrote:
> Hi,
>
> On Tue, May 07, 2024 at 03:30:50PM +0200, Jaroslav Kysela wrote:
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 628d46a0da92..c458172b32d5 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;
>> @@ -330,12 +330,6 @@ enum {
>> #endif
>> };
>>
>> -union snd_pcm_sync_id {
>> - unsigned char id[16];
>> - unsigned short id16[8];
>> - unsigned int id32[4];
>> -};
>
> It can bring FTBFS for any userspace application which uses the
> structure. If getting rid of such public structure, we should have the
> term to deprecate it for a while.
Marked with __attribute__((deprecated)) in v5.
>> +/**
>> + * is sync id (clock id) empty?
>> + */
>> +static inline bool pcm_sync_empty(const unsigned char *sync)
>> +{
>> + return get_unaligned((__u64 *)(sync + 0)) == 0 && get_unaligned((__u64 *)(sync + 8)) == 0;
>> +}
>
> This kind of usage of C inline qualifier is not useless, since the modern
> compilers can optimize it into the local code.
Removed get_unaligned() calls. Using standard C code because this code is
removed in the second patch.
>> +static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
>> + void *arg)
>> +{
>> + struct snd_pcm_hw_params *params = arg;
>> +
>> + if (pcm_sync_empty(params->sync))
>> + memcpy(params->sync, substream->runtime->sync, sizeof(params->sync));
>> + return 0;
>> +}
>
> Furthermore, it does not work as expected in the case that any of driver sets
> zeros to char[16] intentionally for its own purpose at PCM .open callback.
>
> I think it is a kind of 'Separation of mechanism and policy' argument,
> and a kind of designs which should be avoided.
The second patch resolves this.
>> - runtime->sync.id32[0] = substream->pcm->card->number;
>> - runtime->sync.id32[1] = 'P';
>> - runtime->sync.id32[2] = 16;
>> - runtime->sync.id32[3] = 'V';
>> + *(__u32 *)runtime->sync = cpu_to_le32(substream->pcm->card->number);
>> + memset(runtime->sync + 4, 0, sizeof(runtime->sync) - 4);
>> + strncpy(runtime->sync + 4, "P16V", 4);
>
> I'm sorry if I overlooked your reply to my point, however the above is
> not equivalent.
It does not matter. The ID should not be used for a direct comparison. The ID
must be only unique for the system and applications should compare those IDs
between PCM streams (including multiple sound cards). If equal - the source
clock source is equal. I tried to explain this in the commit message in v5.
Thanks for your review,
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-06-24 13:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-07 13:30 [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization ID API Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 1/2] " Jaroslav Kysela
2024-06-13 12:56 ` Takashi Sakamoto
2024-06-13 14:20 ` Takashi Iwai
2024-06-13 14:28 ` Jaroslav Kysela
2024-06-24 13:17 ` Jaroslav Kysela
2024-05-07 13:30 ` [PATCH v4 2/2] ALSA: pcm: optimize and clarify stream sychronization " Jaroslav Kysela
2024-06-13 13:00 ` Takashi Sakamoto
2024-06-13 12:14 ` [PATCH v4 0/2] ALSA: pcm: reinvent the stream synchronization " Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).