diff mbox series

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

Message ID 20240624131127.498605-3-perex@perex.cz (mailing list archive)
State Superseded
Headers show
Series ALSA: pcm: reinvent the stream synchronization ID API | expand

Commit Message

Jaroslav Kysela June 24, 2024, 1:07 p.m. UTC
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(-)

Comments

Amadeusz Sławiński June 24, 2024, 2:10 p.m. UTC | #1
On 6/24/2024 3:07 PM, 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>
> ---

(...)

>   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

Above doc line seems like mistake in this place? This function is 
documented below ;)

> + * @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 a6d59ee9eb52..8d1a24a2c43d 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -516,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

Here ^

>    * @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.
>    */
Jaroslav Kysela June 24, 2024, 2:20 p.m. UTC | #2
On 24. 06. 24 16:10, Amadeusz Sławiński wrote:

>> +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
> 
> Above doc line seems like mistake in this place? This function is
> documented below ;)

Yes, the second line in this comment should be removed. I'll wait for other 
feedback and then I will post v6 with this fix tomorrow.

	Thanks for your review,
			Jaroslav
diff mbox series

Patch

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index dbce137d8806..d3de660d1476 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -402,7 +402,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;
@@ -1156,7 +1156,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 a6d59ee9eb52..8d1a24a2c43d 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -516,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
@@ -1808,22 +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 sync[0] == 0 && sync[1] == 0 && sync[2] == 0 && sync[3] == 0 &&
-	       sync[4] == 0 && sync[5] == 0 && sync[6] == 0 && sync[7] == 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,