[PATCH 0/2] ALSA: pcm: reinvent the stream synchronization ID API

[PATCH 0/2] ALSA: pcm: reinvent the stream synchronization ID API

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

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         | 18 ++++++++++++++++--
 include/uapi/sound/asound.h | 13 ++++---------
 sound/core/pcm_lib.c        | 36 ++++++++++++++++++++++++++----------
 sound/core/pcm_native.c     |  6 ++++++
 sound/pci/emu10k1/p16v.c    | 17 ++++++++++++-----
 5 files changed, 64 insertions(+), 26 deletions(-)

-- 
2.43.0

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

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

Cc: Takashi Sakamoto <takaswie@kernel.org>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 include/sound/pcm.h         | 11 ++++++++++-
 include/uapi/sound/asound.h | 13 ++++---------
 sound/core/pcm_lib.c        | 18 ++++++++++++++----
 sound/core/pcm_native.c     |  6 ++++++
 sound/pci/emu10k1/p16v.c    |  7 +++----
 5 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 210096f124ee..dae41b100517 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;
@@ -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(const unsigned char *sync)
+{
+	return strnlen((const char *)sync, 16) == 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..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..84b948db3393 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -525,10 +525,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 +1808,16 @@ 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;
+
+	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 +1839,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

[PATCH 2/2] ALSA: pcm: optimize and clarify stream sychronization ID API

[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      |  9 +++++++--
 sound/core/pcm_lib.c     | 28 +++++++++++++++++-----------
 sound/pci/emu10k1/p16v.c | 16 ++++++++++++----
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index dae41b100517..71c80ad8d4d3 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,12 @@ 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);
+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 84b948db3393..82452c2e4887 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -516,19 +516,23 @@ 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.
  */
-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
@@ -1811,10 +1815,12 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
 static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
 				     void *arg)
 {
-	struct snd_pcm_hw_params *params = arg;
+	static 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..36b8a4ce2081 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 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

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

[Takashi Sakamoto]

On Mon, May 06, 2024 at 05:11:49PM +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         | 11 ++++++++++-
>  include/uapi/sound/asound.h | 13 ++++---------
>  sound/core/pcm_lib.c        | 18 ++++++++++++++----
>  sound/core/pcm_native.c     |  6 ++++++
>  sound/pci/emu10k1/p16v.c    |  7 +++----
>  5 files changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 210096f124ee..dae41b100517 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;
> @@ -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(const unsigned char *sync)
> +{
> +	return strnlen((const char *)sync, 16) == 0;
> +}
> +

For the sound card indexed by 0, it is problematic, since in the later
change, the first element of array has 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..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 {

As long as I checked, the above change are safe for
backward-compatibility and compat ioctl. They keep the same size of
structure and the same offset of member in each of i386/ILP32/LP64 data
models,

> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 6f73b3c2c205..84b948db3393 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -525,10 +525,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);

I mentioned about the issue in the above.

> @@ -1810,6 +1808,16 @@ 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;
> +
> +	if (pcm_sync_empty(params->sync))
> +		memcpy(params->sync, &substream->runtime->sync, sizeof(params->sync));
> +	return 0;
> +}
> +

I mentioned about the issue in the above.

>  /**
>   * snd_pcm_lib_ioctl - a generic PCM ioctl callback
>   * @substream: the pcm substream instance
> @@ -1831,6 +1839,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);
  
The serialization would not keep the same data before the change, since
the third, fourth, and fifth elements are 0x31, 0x36, and 0x56, instead
of 0x10, 0x56, and 0x00.

>  	return 0;
>  }
> -- 
> 2.43.0
> 

Re: [PATCH 2/2] ALSA: pcm: optimize and clarify stream sychronization ID API

[Takashi Sakamoto]

On Mon, May 06, 2024 at 05:11:50PM +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      |  9 +++++++--
>  sound/core/pcm_lib.c     | 28 +++++++++++++++++-----------
>  sound/pci/emu10k1/p16v.c | 16 ++++++++++++----
>  3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index dae41b100517..71c80ad8d4d3 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,12 @@ 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);
> +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 84b948db3393..82452c2e4887 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -516,19 +516,23 @@ 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.
>   */
> -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);
  
As I mentioned to the previous patch, the value of zero in the first
element of array is problematic, since it is not distinguishable from
the card indexed by zero.

Additionally, I'm worried the second, third, and fourth element of array
is kept as zero, against 0xff in the original serialization code.

I think the series of '0x00 0x00 0x00 numeric-card-id' in the first four
elements is more preferable, however I wonder why the data is typed as
character array instead of integer array.

If the intension of API is not the kind of 'forcing-policy', the byte data
passed from drivers to user space application is reasonable. However,
in the call of above kernel API results in the breakage of policy. I think
more sophisticated way should be investigated, instead of the change in
this patch, in a view of programming interface for device driver developers
and user space application developers.

>  /*
>   *  Standard ioctl routine
> @@ -1811,10 +1815,12 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
>  static int snd_pcm_lib_ioctl_sync_id(struct snd_pcm_substream *substream,
>  				     void *arg)
>  {
> -	struct snd_pcm_hw_params *params = arg;
> +	static 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..36b8a4ce2081 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 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);
> +}
> +

'static const unsigned char id[4]' is preferable, since the data is
immutable during the lifetime of kernel module.

Anyway, as I mentioned to the previous patch, the serialization is not
compatible.

>  /* 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

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

[Takashi Sakamoto]

On Mon, May 06, 2024 at 05:11:48PM +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.
> 
> 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         | 18 ++++++++++++++++--
>  include/uapi/sound/asound.h | 13 ++++---------
>  sound/core/pcm_lib.c        | 36 ++++++++++++++++++++++++++----------
>  sound/core/pcm_native.c     |  6 ++++++
>  sound/pci/emu10k1/p16v.c    | 17 ++++++++++++-----
>  5 files changed, 64 insertions(+), 26 deletions(-)

I reviewed and added some comments per each.

Anyway, we are at the last week of kernel development process. I think
it better to postpone the discussion about this kind of changes until
releasing -rc1, if people working for the task of quality assurance to
the new kernel release.


Regards

Takashi (not this subsystem maintainer) Sakamoto

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

[Jaroslav Kysela]

On 07. 05. 24 5:04, Takashi Sakamoto wrote:

>> +/**
>> + * is sync id (clock id) empty?
>> + */
>> +static inline bool pcm_sync_empty(const unsigned char *sync)
>> +{
>> +	return strnlen((const char *)sync, 16) == 0;
>> +}
>> +
> 
> For the sound card indexed by 0, it is problematic, since in the later
> change, the first element of array has 0.

Oops. It was a thinko. I replaced this with a full zero comparison in v3 of 
this patch.

...

>> @@ -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 {
> 
> As long as I checked, the above change are safe for
> backward-compatibility and compat ioctl. They keep the same size of
> structure and the same offset of member in each of i386/ILP32/LP64 data
> models,

Thank you for this verification and review.

>> +	*(__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);
>    
> The serialization would not keep the same data before the change, since
> the third, fourth, and fifth elements are 0x31, 0x36, and 0x56, instead
> of 0x10, 0x56, and 0x00.

It does not matter. See the extended comment for snd_pcm_set_sync_per_card in 
v3. The ID should be used only for a comparison among PCM devices without any 
interpretation.

				Thank you,
					Jaroslav

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