[PATCH 0/2] ALSA: pcm: add local header for module-local functions

[PATCH 0/2] ALSA: pcm: add local header for module-local functions

[Takashi Sakamoto]

Hi,

This patchset is a supplement for merged patchset[1].

As I suggested[2], an added helper function is useful to some parts of PCM
core module. Although it's better to replace existent code block with it,
there's an issue to share it with several objects, because there is no
header files for external symbols not exported to kernel space.

This patchset adds a header file for the module-local symbols, then
adds calls of the function in several parts of the module.

[1] [alsa-devel] [PATCH 0/7] ALSA: Fix/improve PCM ack callback
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120886.html
[2] [alsa-devel] [PATCH 0/7] ALSA: Fix/improve PCM ack callback
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120910.html

Takashi Sakamoto (2):
  ALSA: pcm: add local header file for snd-pcm module
  ALSA: pcm: add arrangement for applying appl_ptr

 include/sound/pcm.h     | 32 -----------------------------
 sound/core/pcm.c        |  2 ++
 sound/core/pcm_lib.c    | 14 +++++++------
 sound/core/pcm_local.h  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 sound/core/pcm_misc.c   |  3 +++
 sound/core/pcm_native.c | 13 +++++++-----
 6 files changed, 75 insertions(+), 43 deletions(-)
 create mode 100644 sound/core/pcm_local.h

-- 
2.11.0

[PATCH 1/2] ALSA: pcm: add local header file for snd-pcm module

[Takashi Sakamoto]

Several files are used to construct PCM core module, a.k.a snd-pcm.
Although available APIs are described in 'include/sound/pcm.h', some of
them are not exported as symbols in kernel space. Such APIs are just for
module local usage.

This commit adds module local header file and move some function prototypes
into it so that scopes of them are controlled properly and developers
get no confusion from unavailable symbols.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/pcm.h     | 32 -------------------------------
 sound/core/pcm.c        |  2 ++
 sound/core/pcm_lib.c    |  2 ++
 sound/core/pcm_local.h  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 sound/core/pcm_misc.c   |  3 +++
 sound/core/pcm_native.c |  2 ++
 6 files changed, 60 insertions(+), 32 deletions(-)
 create mode 100644 sound/core/pcm_local.h

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index c609b891c4c2..79fedf517070 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -969,12 +969,6 @@ static inline unsigned int params_buffer_bytes(const struct snd_pcm_hw_params *p
 }
 
 int snd_interval_refine(struct snd_interval *i, const struct snd_interval *v);
-void snd_interval_mul(const struct snd_interval *a, const struct snd_interval *b, struct snd_interval *c);
-void snd_interval_div(const struct snd_interval *a, const struct snd_interval *b, struct snd_interval *c);
-void snd_interval_muldivk(const struct snd_interval *a, const struct snd_interval *b, 
-			  unsigned int k, struct snd_interval *c);
-void snd_interval_mulkdiv(const struct snd_interval *a, unsigned int k,
-			  const struct snd_interval *b, struct snd_interval *c);
 int snd_interval_list(struct snd_interval *i, unsigned int count,
 		      const unsigned int *list, unsigned int mask);
 int snd_interval_ranges(struct snd_interval *i, unsigned int count,
@@ -985,15 +979,9 @@ int snd_interval_ratnum(struct snd_interval *i,
 
 void _snd_pcm_hw_params_any(struct snd_pcm_hw_params *params);
 void _snd_pcm_hw_param_setempty(struct snd_pcm_hw_params *params, snd_pcm_hw_param_t var);
-int snd_pcm_hw_params_choose(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params);
 
 int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params);
 
-int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream);
-int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream);
-
-int snd_pcm_hw_constraint_mask(struct snd_pcm_runtime *runtime, snd_pcm_hw_param_t var,
-			       u_int32_t mask);
 int snd_pcm_hw_constraint_mask64(struct snd_pcm_runtime *runtime, snd_pcm_hw_param_t var,
 				 u_int64_t mask);
 int snd_pcm_hw_constraint_minmax(struct snd_pcm_runtime *runtime, snd_pcm_hw_param_t var,
@@ -1081,10 +1069,6 @@ void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
 void snd_pcm_set_sync(struct snd_pcm_substream *substream);
 int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 		      unsigned int cmd, void *arg);                      
-int snd_pcm_update_state(struct snd_pcm_substream *substream,
-			 struct snd_pcm_runtime *runtime);
-int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr);
 void snd_pcm_period_elapsed(struct snd_pcm_substream *substream);
 snd_pcm_sframes_t snd_pcm_lib_write(struct snd_pcm_substream *substream,
 				    const void __user *buf,
@@ -1096,8 +1080,6 @@ snd_pcm_sframes_t snd_pcm_lib_writev(struct snd_pcm_substream *substream,
 snd_pcm_sframes_t snd_pcm_lib_readv(struct snd_pcm_substream *substream,
 				    void __user **bufs, snd_pcm_uframes_t frames);
 
-extern const struct snd_pcm_hw_constraint_list snd_pcm_known_rates;
-
 int snd_pcm_limit_hw_rates(struct snd_pcm_runtime *runtime);
 unsigned int snd_pcm_rate_to_rate_bit(unsigned int rate);
 unsigned int snd_pcm_rate_bit_to_rate(unsigned int rate_bit);
@@ -1131,20 +1113,6 @@ static inline void snd_pcm_set_runtime_buffer(struct snd_pcm_substream *substrea
 	}
 }
 
-/*
- *  Timer interface
- */
-
-#ifdef CONFIG_SND_PCM_TIMER
-void snd_pcm_timer_resolution_change(struct snd_pcm_substream *substream);
-void snd_pcm_timer_init(struct snd_pcm_substream *substream);
-void snd_pcm_timer_done(struct snd_pcm_substream *substream);
-#else
-static inline void
-snd_pcm_timer_resolution_change(struct snd_pcm_substream *substream) {}
-static inline void snd_pcm_timer_init(struct snd_pcm_substream *substream) {}
-static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
-#endif
 /**
  * snd_pcm_gettime - Fill the timespec depending on the timestamp mode
  * @runtime: PCM runtime instance
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index d30dba0ee688..4b3290447398 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -31,6 +31,8 @@
 #include <sound/control.h>
 #include <sound/info.h>
 
+#include "pcm_local.h"
+
 MODULE_AUTHOR("Jaroslav Kysela <perex@perex.cz>, Abramo Bagnara <abramo@alsa-project.org>");
 MODULE_DESCRIPTION("Midlevel PCM code for ALSA.");
 MODULE_LICENSE("GPL");
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index ab4b1d1e44ee..e50548af4004 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -33,6 +33,8 @@
 #include <sound/pcm_params.h>
 #include <sound/timer.h>
 
+#include "pcm_local.h"
+
 #ifdef CONFIG_SND_PCM_XRUN_DEBUG
 #define CREATE_TRACE_POINTS
 #include "pcm_trace.h"
diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
new file mode 100644
index 000000000000..34c66decaaf2
--- /dev/null
+++ b/sound/core/pcm_local.h
@@ -0,0 +1,51 @@
+/*
+ * pcm_local.h - a local header file for snd-pcm module.
+ *
+ * Copyright (c) Takashi Sakamoto <o-takashi@sakamocchi.jp>
+ *
+ * Licensed under the terms of the GNU General Public License, version 2.
+ */
+
+#ifndef __SOUND_CORE_PCM_LOCAL_H
+#define __SOUND_CORE_PCM_LOCAL_H
+
+extern const struct snd_pcm_hw_constraint_list snd_pcm_known_rates;
+
+void snd_interval_mul(const struct snd_interval *a,
+		      const struct snd_interval *b, struct snd_interval *c);
+void snd_interval_div(const struct snd_interval *a,
+		      const struct snd_interval *b, struct snd_interval *c);
+void snd_interval_muldivk(const struct snd_interval *a,
+			  const struct snd_interval *b,
+			  unsigned int k, struct snd_interval *c);
+void snd_interval_mulkdiv(const struct snd_interval *a, unsigned int k,
+			  const struct snd_interval *b, struct snd_interval *c);
+
+int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream);
+int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream);
+
+int snd_pcm_hw_params_choose(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params);
+
+int snd_pcm_hw_constraint_mask(struct snd_pcm_runtime *runtime,
+			       snd_pcm_hw_param_t var, u_int32_t mask);
+
+int snd_pcm_update_state(struct snd_pcm_substream *substream,
+			 struct snd_pcm_runtime *runtime);
+int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
+
+void snd_pcm_playback_silence(struct snd_pcm_substream *substream,
+			      snd_pcm_uframes_t new_hw_ptr);
+
+#ifdef CONFIG_SND_PCM_TIMER
+void snd_pcm_timer_resolution_change(struct snd_pcm_substream *substream);
+void snd_pcm_timer_init(struct snd_pcm_substream *substream);
+void snd_pcm_timer_done(struct snd_pcm_substream *substream);
+#else
+static inline void
+snd_pcm_timer_resolution_change(struct snd_pcm_substream *substream) {}
+static inline void snd_pcm_timer_init(struct snd_pcm_substream *substream) {}
+static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
+#endif
+
+#endif	/* __SOUND_CORE_PCM_LOCAL_H */
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 53dc37357bca..dd8383e29315 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -23,6 +23,9 @@
 #include <linux/export.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
+
+#include "pcm_local.h"
+
 #define SND_PCM_FORMAT_UNKNOWN (-1)
 
 /* NOTE: "signed" prefix must be given below since the default char is
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 5be549cf91e5..bf5d0f2acfb9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -37,6 +37,8 @@
 #include <sound/minors.h>
 #include <linux/uio.h>
 
+#include "pcm_local.h"
+
 /*
  *  Compatibility
  */
-- 
2.11.0

[PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Sakamoto]

In callgraphs from below kernel APIs, position of application pointer on
PCM buffer is changes, according to given parameters.

 - snd_pcm_lib_read()
 - snd_pcm_lib_readv()
 - snd_pcm_lib_write()
 - snd_pcm_lib_writev()

This operation corresponds to application of new position to runtime of
PCM substream and callback of driver's implementation for
'struct snd_pcm_ops.ack'.

In a former commit, a new local function is added, 'apply_appl_ptr'. The
above code block can be replaced with this function.

This commit replaces the code block with call of the function. The function
is renamed to be shared by several objects.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_lib.c    | 12 ++++++------
 sound/core/pcm_local.h  |  3 +++
 sound/core/pcm_native.c | 11 ++++++-----
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index e50548af4004..63a172455e00 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2091,9 +2091,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 		appl_ptr += frames;
 		if (appl_ptr >= runtime->boundary)
 			appl_ptr -= runtime->boundary;
-		runtime->control->appl_ptr = appl_ptr;
-		if (substream->ops->ack)
-			substream->ops->ack(substream);
+		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
+		if (err < 0)
+			goto _end_unlock;
 
 		offset += frames;
 		size -= frames;
@@ -2323,9 +2323,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 		appl_ptr += frames;
 		if (appl_ptr >= runtime->boundary)
 			appl_ptr -= runtime->boundary;
-		runtime->control->appl_ptr = appl_ptr;
-		if (substream->ops->ack)
-			substream->ops->ack(substream);
+		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
+		if (err < 0)
+			goto _end_unlock;
 
 		offset += frames;
 		size -= frames;
diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
index 34c66decaaf2..5ac60cd6f4d9 100644
--- a/sound/core/pcm_local.h
+++ b/sound/core/pcm_local.h
@@ -48,4 +48,7 @@ static inline void snd_pcm_timer_init(struct snd_pcm_substream *substream) {}
 static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
 #endif
 
+int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
+			   snd_pcm_uframes_t appl_ptr);
+
 #endif	/* __SOUND_CORE_PCM_LOCAL_H */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index bf5d0f2acfb9..514f6707280e 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2454,8 +2454,8 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream)
 /* update to the given appl_ptr and call ack callback if needed;
  * when an error is returned, take back to the original value
  */
-static int apply_appl_ptr(struct snd_pcm_substream *substream,
-			  snd_pcm_uframes_t appl_ptr)
+int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
+			   snd_pcm_uframes_t appl_ptr)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
@@ -2488,7 +2488,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream,
 	appl_ptr = runtime->control->appl_ptr + frames;
 	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
 		appl_ptr -= runtime->boundary;
-	ret = apply_appl_ptr(substream, appl_ptr);
+	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
 	return ret < 0 ? ret : frames;
 }
 
@@ -2508,7 +2508,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream,
 	appl_ptr = runtime->control->appl_ptr - frames;
 	if (appl_ptr < 0)
 		appl_ptr += runtime->boundary;
-	ret = apply_appl_ptr(substream, appl_ptr);
+	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
 	return ret < 0 ? ret : frames;
 }
 
@@ -2636,7 +2636,8 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 	}
 	snd_pcm_stream_lock_irq(substream);
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
-		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
+		err = snd_pcm_apply_appl_ptr(substream,
+					     sync_ptr.c.control.appl_ptr);
 		if (err < 0) {
 			snd_pcm_stream_unlock_irq(substream);
 			return err;
-- 
2.11.0

Re: [PATCH 1/2] ALSA: pcm: add local header file for snd-pcm module

[Takashi Iwai]

On Fri, 26 May 2017 02:30:46 +0200,
Takashi Sakamoto wrote:
> 
> Several files are used to construct PCM core module, a.k.a snd-pcm.
> Although available APIs are described in 'include/sound/pcm.h', some of
> them are not exported as symbols in kernel space. Such APIs are just for
> module local usage.
> 
> This commit adds module local header file and move some function prototypes
> into it so that scopes of them are controlled properly and developers
> get no confusion from unavailable symbols.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, now.  But...

> --- /dev/null
> +++ b/sound/core/pcm_local.h
> @@ -0,0 +1,51 @@
> +/*
> + * pcm_local.h - a local header file for snd-pcm module.
> + *
> + * Copyright (c) Takashi Sakamoto <o-takashi@sakamocchi.jp>
> + *
> + * Licensed under the terms of the GNU General Public License, version 2.
> + */

You don't have to declare always your name clearly there as if all
things were all your inventions solely :)


thanks,

Takashi

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Iwai]

On Fri, 26 May 2017 02:30:47 +0200,
Takashi Sakamoto wrote:
> 
> In callgraphs from below kernel APIs, position of application pointer on
> PCM buffer is changes, according to given parameters.
> 
>  - snd_pcm_lib_read()
>  - snd_pcm_lib_readv()
>  - snd_pcm_lib_write()
>  - snd_pcm_lib_writev()
> 
> This operation corresponds to application of new position to runtime of
> PCM substream and callback of driver's implementation for
> 'struct snd_pcm_ops.ack'.
> 
> In a former commit, a new local function is added, 'apply_appl_ptr'. The
> above code block can be replaced with this function.
> 
> This commit replaces the code block with call of the function. The function
> is renamed to be shared by several objects.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Queued now, but will be likely declined later due to the rewrites of
the code.


thanks,

Takashi

> ---
>  sound/core/pcm_lib.c    | 12 ++++++------
>  sound/core/pcm_local.h  |  3 +++
>  sound/core/pcm_native.c | 11 ++++++-----
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index e50548af4004..63a172455e00 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2091,9 +2091,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  		appl_ptr += frames;
>  		if (appl_ptr >= runtime->boundary)
>  			appl_ptr -= runtime->boundary;
> -		runtime->control->appl_ptr = appl_ptr;
> -		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
> +		if (err < 0)
> +			goto _end_unlock;
>  
>  		offset += frames;
>  		size -= frames;
> @@ -2323,9 +2323,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		appl_ptr += frames;
>  		if (appl_ptr >= runtime->boundary)
>  			appl_ptr -= runtime->boundary;
> -		runtime->control->appl_ptr = appl_ptr;
> -		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
> +		if (err < 0)
> +			goto _end_unlock;
>  
>  		offset += frames;
>  		size -= frames;
> diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
> index 34c66decaaf2..5ac60cd6f4d9 100644
> --- a/sound/core/pcm_local.h
> +++ b/sound/core/pcm_local.h
> @@ -48,4 +48,7 @@ static inline void snd_pcm_timer_init(struct snd_pcm_substream *substream) {}
>  static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
>  #endif
>  
> +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
> +			   snd_pcm_uframes_t appl_ptr);
> +
>  #endif	/* __SOUND_CORE_PCM_LOCAL_H */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index bf5d0f2acfb9..514f6707280e 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2454,8 +2454,8 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream)
>  /* update to the given appl_ptr and call ack callback if needed;
>   * when an error is returned, take back to the original value
>   */
> -static int apply_appl_ptr(struct snd_pcm_substream *substream,
> -			  snd_pcm_uframes_t appl_ptr)
> +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
> +			   snd_pcm_uframes_t appl_ptr)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> @@ -2488,7 +2488,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream,
>  	appl_ptr = runtime->control->appl_ptr + frames;
>  	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
>  		appl_ptr -= runtime->boundary;
> -	ret = apply_appl_ptr(substream, appl_ptr);
> +	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
>  	return ret < 0 ? ret : frames;
>  }
>  
> @@ -2508,7 +2508,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream,
>  	appl_ptr = runtime->control->appl_ptr - frames;
>  	if (appl_ptr < 0)
>  		appl_ptr += runtime->boundary;
> -	ret = apply_appl_ptr(substream, appl_ptr);
> +	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
>  	return ret < 0 ? ret : frames;
>  }
>  
> @@ -2636,7 +2636,8 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>  	}
>  	snd_pcm_stream_lock_irq(substream);
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> -		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
> +		err = snd_pcm_apply_appl_ptr(substream,
> +					     sync_ptr.c.control.appl_ptr);
>  		if (err < 0) {
>  			snd_pcm_stream_unlock_irq(substream);
>  			return err;
> -- 
> 2.11.0
> 

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Sakamoto]

On May 26 2017 15:40, Takashi Iwai wrote:
> On Fri, 26 May 2017 02:30:47 +0200,
> Takashi Sakamoto wrote:
>>
>> In callgraphs from below kernel APIs, position of application pointer on
>> PCM buffer is changes, according to given parameters.
>>
>>   - snd_pcm_lib_read()
>>   - snd_pcm_lib_readv()
>>   - snd_pcm_lib_write()
>>   - snd_pcm_lib_writev()
>>
>> This operation corresponds to application of new position to runtime of
>> PCM substream and callback of driver's implementation for
>> 'struct snd_pcm_ops.ack'.
>>
>> In a former commit, a new local function is added, 'apply_appl_ptr'. The
>> above code block can be replaced with this function.
>>
>> This commit replaces the code block with call of the function. The function
>> is renamed to be shared by several objects.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Queued now, but will be likely declined later due to the rewrites of
> the code.

Due to your recent work under reviewing?


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Iwai]

On Fri, 26 May 2017 08:42:41 +0200,
Takashi Sakamoto wrote:
> 
> On May 26 2017 15:40, Takashi Iwai wrote:
> > On Fri, 26 May 2017 02:30:47 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> In callgraphs from below kernel APIs, position of application pointer on
> >> PCM buffer is changes, according to given parameters.
> >>
> >>   - snd_pcm_lib_read()
> >>   - snd_pcm_lib_readv()
> >>   - snd_pcm_lib_write()
> >>   - snd_pcm_lib_writev()
> >>
> >> This operation corresponds to application of new position to runtime of
> >> PCM substream and callback of driver's implementation for
> >> 'struct snd_pcm_ops.ack'.
> >>
> >> In a former commit, a new local function is added, 'apply_appl_ptr'. The
> >> above code block can be replaced with this function.
> >>
> >> This commit replaces the code block with call of the function. The function
> >> is renamed to be shared by several objects.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >
> > Queued now, but will be likely declined later due to the rewrites of
> > the code.
> 
> Due to your recent work under reviewing?

Yes, the code might be changed heavily.
Don't touch a too hot spot if it were only a cleanup.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Sakamoto]

On May 26 2017 15:47, Takashi Iwai wrote:
>>> Queued now, but will be likely declined later due to the rewrites of
>>> the code.
>>
>> Due to your recent work under reviewing?
> 
> Yes, the code might be changed heavily.
> Don't touch a too hot spot if it were only a cleanup.

I've already reviewed it but postpone my reply to this evening. (I'm in 
paid work now.)

Your patchset looks a middle of work. It includes the lack of changes 
for each drivers, thus not bisect-able. I think you will re-post the 
full series of patches several days after reviewed. Then you have chance 
to rebase it to recent HEAD of your tree, don't you?


Thanks

Takashi Sakamoto

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Iwai]

On Fri, 26 May 2017 08:54:27 +0200,
Takashi Sakamoto wrote:
> 
> On May 26 2017 15:47, Takashi Iwai wrote:
> >>> Queued now, but will be likely declined later due to the rewrites of
> >>> the code.
> >>
> >> Due to your recent work under reviewing?
> >
> > Yes, the code might be changed heavily.
> > Don't touch a too hot spot if it were only a cleanup.
> 
> I've already reviewed it but postpone my reply to this evening. (I'm
> in paid work now.)
> 
> Your patchset looks a middle of work. It includes the lack of changes
> for each drivers, thus not bisect-able.

Yes, it was mentioned so in the cover letter.

> I think you will re-post the
> full series of patches several days after reviewed. Then you have
> chance to rebase it to recent HEAD of your tree, don't you?

My patches are already ready.  I didn't post the full set just because
it's too much.  The patch "snippet" was shown as a demonstration.

And, why do you think there is only one patchset?  Usually I write
several different implementations and choose the best one for
submission.  That is, I already have a few other patchsets in my local
tree based on the current code base.  And even further works on PCM
code are pending.

So which is easier to rebase and handle?  Which one has more
significant changes?  A single trivial cleanup patch, or the whole
several sets of patchsets?  The answer is clear to my eyes.


thanks,

Takashi

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Sakamoto]

On May 26 2017 16:10, Takashi Iwai wrote:
> On Fri, 26 May 2017 08:54:27 +0200,
> Takashi Sakamoto wrote:
>>
>> On May 26 2017 15:47, Takashi Iwai wrote:
>>>>> Queued now, but will be likely declined later due to the rewrites of
>>>>> the code.
>>>>
>>>> Due to your recent work under reviewing?
>>>
>>> Yes, the code might be changed heavily.
>>> Don't touch a too hot spot if it were only a cleanup.
>>
>> I've already reviewed it but postpone my reply to this evening. (I'm
>> in paid work now.)
>>
>> Your patchset looks a middle of work. It includes the lack of changes
>> for each drivers, thus not bisect-able.
> 
> Yes, it was mentioned so in the cover letter.
> 
>> I think you will re-post the
>> full series of patches several days after reviewed. Then you have
>> chance to rebase it to recent HEAD of your tree, don't you?
> 
> My patches are already ready.  I didn't post the full set just because
> it's too much.  The patch "snippet" was shown as a demonstration.

Yes, like I did in my previous patchset.

> And, why do you think there is only one patchset?
> Usually I write several different implementations and choose the best one
> for submission.  That is, I already have a few other patchsets in my local
> tree based on the current code base.  And even further works on PCM
> code are pending.

I can easily imagine how you progress the work, because I have some 
topic branch in my local repository as well, for PCM core. They're also 
ready, like yours. However, I accept changes of HEAD of remote branch. 
HEAD tends to move regardless of my work. When posting my patchset, I 
always work to rebase to the HEAD. This often brings me a bit work, and 
next week I'll surely do it, but it's natural to me.

Here, I don't necessarily request you to merge them promptly. I'm just 
interested in your current situation, in short, the reason to postpone 
this patch. In my eyes, this patch surely brings conflict to your future 
patchset, but it has a small changes and the occurred conflict can be 
fixed quickly. I understand that you'd like to save the time for it.

> So which is easier to rebase and handle?  Which one has more
> significant changes?  A single trivial cleanup patch, or the whole
> several sets of patchsets?  The answer is clear to my eyes.

Hm.

It depends on the shape of patchset, regardless of the importance, in my 
humble opinion.

When it includes large changes, it takes longer time to review. When a 
patchset is splitted into small parts and posted sequentially with each 
of topics, reviewer finishes his or her work within shorter time 
relatively. I'll post my comments to your recent work later, but here 
your patchset looks to include refactoring, integration and more. It 
seems takes more time to merge into HEAD and I cannot post the series of 
my works (they depends each other) even if my patches are enough small. 
That's my concern here.


Regards

Takashi Sakamoto

Re: [PATCH 2/2] ALSA: pcm: add arrangement for applying appl_ptr

[Takashi Iwai]

On Fri, 26 May 2017 10:46:07 +0200,
Takashi Sakamoto wrote:
> 
> On May 26 2017 16:10, Takashi Iwai wrote:
> > On Fri, 26 May 2017 08:54:27 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On May 26 2017 15:47, Takashi Iwai wrote:
> >>>>> Queued now, but will be likely declined later due to the rewrites of
> >>>>> the code.
> >>>>
> >>>> Due to your recent work under reviewing?
> >>>
> >>> Yes, the code might be changed heavily.
> >>> Don't touch a too hot spot if it were only a cleanup.
> >>
> >> I've already reviewed it but postpone my reply to this evening. (I'm
> >> in paid work now.)
> >>
> >> Your patchset looks a middle of work. It includes the lack of changes
> >> for each drivers, thus not bisect-able.
> >
> > Yes, it was mentioned so in the cover letter.
> >
> >> I think you will re-post the
> >> full series of patches several days after reviewed. Then you have
> >> chance to rebase it to recent HEAD of your tree, don't you?
> >
> > My patches are already ready.  I didn't post the full set just because
> > it's too much.  The patch "snippet" was shown as a demonstration.
> 
> Yes, like I did in my previous patchset.
> 
> > And, why do you think there is only one patchset?
> > Usually I write several different implementations and choose the best one
> > for submission.  That is, I already have a few other patchsets in my local
> > tree based on the current code base.  And even further works on PCM
> > code are pending.
> 
> I can easily imagine how you progress the work, because I have some
> topic branch in my local repository as well, for PCM core. They're
> also ready, like yours. However, I accept changes of HEAD of remote
> branch. HEAD tends to move regardless of my work. When posting my
> patchset, I always work to rebase to the HEAD. This often brings me a
> bit work, and next week I'll surely do it, but it's natural to me.
> 
> Here, I don't necessarily request you to merge them promptly. I'm just
> interested in your current situation, in short, the reason to postpone
> this patch. In my eyes, this patch surely brings conflict to your
> future patchset, but it has a small changes and the occurred conflict
> can be fixed quickly. I understand that you'd like to save the time
> for it.
> 
> > So which is easier to rebase and handle?  Which one has more
> > significant changes?  A single trivial cleanup patch, or the whole
> > several sets of patchsets?  The answer is clear to my eyes.
> 
> Hm.
> 
> It depends on the shape of patchset, regardless of the importance, in
> my humble opinion.
> 
> When it includes large changes, it takes longer time to review. When a
> patchset is splitted into small parts and posted sequentially with
> each of topics, reviewer finishes his or her work within shorter time
> relatively. I'll post my comments to your recent work later, but here
> your patchset looks to include refactoring, integration and more. It
> seems takes more time to merge into HEAD and I cannot post the series
> of my works (they depends each other) even if my patches are enough
> small. That's my concern here.

A pain-point at this time is that it's involved with other subsystems,
thus the interaction with them should be minimized.  Usually, I prefer
implementing things more gradually, e.g. add copy_kernel ops at first,
then refactor PCM core slowly.  But it'd require more interactions
with other subtrees as long as API change is required.  That's why I
chose the one-time change including refactoring.

That said, if API change is in play, this is the biggest blocker per
its nature, and it has the higher priority than other conflicting
changes (unless it's an urgent fix).


thanks,

Takashi