[PATCH 1/2] pcm: pass hw_params flags to slave

[PATCH 1/2] pcm: pass hw_params flags to slave

[Pierre-Louis Bossart]

fix required before interrupt disabling routines patch can be applied.
Without this fix, the interrupts are only disabled when accessing
directly hw devices.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 src/pcm/pcm_params.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/pcm/pcm_params.c b/src/pcm/pcm_params.c
index 3a90bcd..416b081 100644
--- a/src/pcm/pcm_params.c
+++ b/src/pcm/pcm_params.c
@@ -2126,6 +2126,7 @@ int _snd_pcm_hw_params_refine(snd_pcm_hw_params_t *params,
 			err = changed;
 	}
 	params->info &= src->info;
+        params->flags = src->flags; /* propagate flags to slave */
 	return err;
 }
 
-- 
1.7.2.3

[PATCH 2/2] add API to allow disabling period interrupt

[Pierre-Louis Bossart]

cleaned-up version of the patch posted on May 17, 2010 by
Clemens Ladisch <clemens@ladisch.de>
(No filtering in pcm_multi and pcm_direct info fields)

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 configure.in           |    2 +-
 include/pcm.h          |    3 ++
 include/sound/asound.h |    5 +++-
 src/Versions.in        |    7 +++++
 src/pcm/pcm.c          |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/pcm/pcm_local.h    |    3 ++
 6 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index c353759..0a67de7 100644
--- a/configure.in
+++ b/configure.in
@@ -12,7 +12,7 @@ dnl add API = c+1:0:a+1
 dnl remove API = c+1:0:0
 dnl *************************************************
 AC_CANONICAL_HOST
-AM_INIT_AUTOMAKE(alsa-lib, 1.0.23)
+AM_INIT_AUTOMAKE(alsa-lib, 1.0.24)
 eval LIBTOOL_VERSION_INFO="2:0:0"
 dnl *************************************************
 AM_CONDITIONAL(INSTALL_M4, test -n "${ACLOCAL}")
diff --git a/include/pcm.h b/include/pcm.h
index f3618c3..cd506ab 100644
--- a/include/pcm.h
+++ b/include/pcm.h
@@ -531,6 +531,7 @@ int snd_pcm_hw_params_can_resume(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_is_half_duplex(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_is_joint_duplex(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_can_sync_start(const snd_pcm_hw_params_t *params);
+int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params);
 int snd_pcm_hw_params_get_rate_numden(const snd_pcm_hw_params_t *params,
 				      unsigned int *rate_num,
 				      unsigned int *rate_den);
@@ -626,6 +627,8 @@ int snd_pcm_hw_params_set_rate_resample(snd_pcm_t *pcm, snd_pcm_hw_params_t *par
 int snd_pcm_hw_params_get_rate_resample(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
 int snd_pcm_hw_params_set_export_buffer(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val);
 int snd_pcm_hw_params_get_export_buffer(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
+int snd_pcm_hw_params_set_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val);
+int snd_pcm_hw_params_get_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
 
 int snd_pcm_hw_params_get_period_time(const snd_pcm_hw_params_t *params, unsigned int *val, int *dir);
 int snd_pcm_hw_params_get_period_time_min(const snd_pcm_hw_params_t *params, unsigned int *val, int *dir);
diff --git a/include/sound/asound.h b/include/sound/asound.h
index fa88938..fbcfef9 100644
--- a/include/sound/asound.h
+++ b/include/sound/asound.h
@@ -278,6 +278,8 @@ enum sndrv_pcm_subformat {
 #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
 #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
 #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
+#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
+#define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
 
 enum sndrv_pcm_state {
 	SNDRV_PCM_STATE_OPEN = 0,	/* stream is open */
@@ -346,7 +348,8 @@ enum sndrv_pcm_hw_param {
 
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)  /* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)  /* export buffer */
-
+#define SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ      (1<<2)  /* disable period
+                                                          interrupts */
 struct sndrv_interval {
 	unsigned int min, max;
 	unsigned int openmin:1,
diff --git a/src/Versions.in b/src/Versions.in
index 8d2dd11..112b9ec 100644
--- a/src/Versions.in
+++ b/src/Versions.in
@@ -129,3 +129,10 @@ ALSA_0.9.7 {
     @SYMBOL_PREFIX@alsa_lisp_*;
 } ALSA_0.9.5;
 
+ALSA_1.0.24 {
+  global:
+    @SYMBOL_PREFIX@snd_pcm_hw_params_can_disable_period_irq;
+    @SYMBOL_PREFIX@snd_pcm_hw_params_set_period_irq;
+    @SYMBOL_PREFIX@snd_pcm_hw_params_get_period_irq;
+
+} ALSA_0.9.7;
\ No newline at end of file
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index a49b5b9..6eb7223 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -3081,6 +3081,27 @@ int snd_pcm_hw_params_can_sync_start(const snd_pcm_hw_params_t *params)
 }
 
 /**
+ * \brief Check if hardware can disable period interrupts
+ * \param params Configuration space
+ * \return Boolean value
+ * \retval 0 Hardware cannot disable period interrupts
+ * \retval 1 Hardware can disable period interrupts
+ *
+ * It is not allowed to call this function when given configuration is not exactly one.
+ * Usually, #snd_pcm_hw_params() function chooses one configuration
+ * from the configuration space.
+ */
+int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params)
+{
+        assert(params);
+        if (CHECK_SANITY(params->info == ~0U)) {
+                SNDMSG("invalid PCM info field");
+                return 0; /* FIXME: should be a negative error? */
+        }
+        return !!(params->info & SNDRV_PCM_INFO_NO_PERIOD_IRQ);
+}
+
+/**
  * \brief Get rate exact info from a configuration space
  * \param params Configuration space
  * \param rate_num Pointer to returned rate numerator
@@ -4200,6 +4221,46 @@ int snd_pcm_hw_params_get_export_buffer(snd_pcm_t *pcm, snd_pcm_hw_params_t *par
 }
 
 /**
+ * \brief Restrict a configuration space to settings without period interrupts
+ * \param pcm PCM handle
+ * \param params Configuration space
+ * \param val 0 = disable, 1 = enable (default) period interrupts
+ * \return Zero on success, otherwise a negative error code.
+ *
+ * This function should be called only on devices where
+ * #snd_pcm_hw_params_can_disable_period_irq() returns true. (too late, FIXME)
+ *
+ * Even with disabled period interrupts, the period size/time/count parameters
+ * are valid; it is suggested to use #snd_pcm_hw_params_set_period_size_last().
+ *
+ * When period interrupts are disabled, the application must not use poll() or
+ * any functions that could block on this device.
+ */
+int snd_pcm_hw_params_set_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val)
+{
+       assert(pcm && params);
+       if (!val)
+               params->flags |= SND_PCM_HW_PARAMS_NO_PERIOD_IRQ;
+       else
+               params->flags &= ~SND_PCM_HW_PARAMS_NO_PERIOD_IRQ;
+       return snd_pcm_hw_refine(pcm, params);
+}
+
+/**
+ * \brief Extract period interrupt mask from a configuration space
+ * \param pcm PCM handle
+ * \param params Configuration space
+ * \param val overwritten with 0 = disabled, 1 = enabled period interrupts
+ * \return Zero 
+ */
+int snd_pcm_hw_params_get_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val)
+{
+       assert(pcm && params && val);
+       *val = params->flags & SND_PCM_HW_PARAMS_NO_PERIOD_IRQ ? 0 : 1;
+       return 0;
+}
+
+/**
  * \brief Extract period time from a configuration space
  * \param params Configuration space
  * \param val Returned approximate period duration in us
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 2f6fcd2..768f526 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -91,9 +91,12 @@ typedef enum sndrv_pcm_hw_param snd_pcm_hw_param_t;
 #define SND_PCM_INFO_JOINT_DUPLEX SNDRV_PCM_INFO_JOINT_DUPLEX
 /** device can do a kind of synchronized start */
 #define SND_PCM_INFO_SYNC_START SNDRV_PCM_INFO_SYNC_START
+/** device can disable period interrupts */
+#define SND_PCM_INFO_NO_PERIOD_IRQ SNDRV_PCM_INFO_NO_PERIOD_IRQ
 
 #define SND_PCM_HW_PARAMS_NORESAMPLE SNDRV_PCM_HW_PARAMS_NORESAMPLE
 #define SND_PCM_HW_PARAMS_EXPORT_BUFFER SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER
+#define SND_PCM_HW_PARAMS_NO_PERIOD_IRQ SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ
 
 #define SND_PCM_INFO_MONOTONIC	0x80000000
 
-- 
1.7.2.3

Re: [PATCH 1/2] pcm: pass hw_params flags to slave

[Takashi Iwai]

At Mon,  1 Nov 2010 17:13:33 -0500,
Pierre-Louis Bossart wrote:
> 
> fix required before interrupt disabling routines patch can be applied.
> Without this fix, the interrupts are only disabled when accessing
> directly hw devices.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>

I'm a bit skeptical whether this is safe to apply.
The plugins might not be always transparent.  Double-check whether
this doesn't break all plugins behaviors.


thanks,

Takashi


> ---
>  src/pcm/pcm_params.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/src/pcm/pcm_params.c b/src/pcm/pcm_params.c
> index 3a90bcd..416b081 100644
> --- a/src/pcm/pcm_params.c
> +++ b/src/pcm/pcm_params.c
> @@ -2126,6 +2126,7 @@ int _snd_pcm_hw_params_refine(snd_pcm_hw_params_t *params,
>  			err = changed;
>  	}
>  	params->info &= src->info;
> +        params->flags = src->flags; /* propagate flags to slave */
>  	return err;
>  }
>  
> -- 
> 1.7.2.3
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Takashi Iwai]

At Mon,  1 Nov 2010 17:13:34 -0500,
Pierre-Louis Bossart wrote:
> 
> cleaned-up version of the patch posted on May 17, 2010 by
> Clemens Ladisch <clemens@ladisch.de>
> (No filtering in pcm_multi and pcm_direct info fields)
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
> ---
>  configure.in           |    2 +-
>  include/pcm.h          |    3 ++
>  include/sound/asound.h |    5 +++-
>  src/Versions.in        |    7 +++++
>  src/pcm/pcm.c          |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/pcm/pcm_local.h    |    3 ++
>  6 files changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.in b/configure.in
> index c353759..0a67de7 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -12,7 +12,7 @@ dnl add API = c+1:0:a+1
>  dnl remove API = c+1:0:0
>  dnl *************************************************
>  AC_CANONICAL_HOST
> -AM_INIT_AUTOMAKE(alsa-lib, 1.0.23)
> +AM_INIT_AUTOMAKE(alsa-lib, 1.0.24)
>  eval LIBTOOL_VERSION_INFO="2:0:0"
>  dnl *************************************************
>  AM_CONDITIONAL(INSTALL_M4, test -n "${ACLOCAL}")
> diff --git a/include/pcm.h b/include/pcm.h
> index f3618c3..cd506ab 100644
> --- a/include/pcm.h
> +++ b/include/pcm.h
> @@ -531,6 +531,7 @@ int snd_pcm_hw_params_can_resume(const snd_pcm_hw_params_t *params);
>  int snd_pcm_hw_params_is_half_duplex(const snd_pcm_hw_params_t *params);
>  int snd_pcm_hw_params_is_joint_duplex(const snd_pcm_hw_params_t *params);
>  int snd_pcm_hw_params_can_sync_start(const snd_pcm_hw_params_t *params);
> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params);
>  int snd_pcm_hw_params_get_rate_numden(const snd_pcm_hw_params_t *params,
>  				      unsigned int *rate_num,
>  				      unsigned int *rate_den);
> @@ -626,6 +627,8 @@ int snd_pcm_hw_params_set_rate_resample(snd_pcm_t *pcm, snd_pcm_hw_params_t *par
>  int snd_pcm_hw_params_get_rate_resample(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
>  int snd_pcm_hw_params_set_export_buffer(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val);
>  int snd_pcm_hw_params_get_export_buffer(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
> +int snd_pcm_hw_params_set_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int val);
> +int snd_pcm_hw_params_get_period_irq(snd_pcm_t *pcm, snd_pcm_hw_params_t *params, unsigned int *val);
>  
>  int snd_pcm_hw_params_get_period_time(const snd_pcm_hw_params_t *params, unsigned int *val, int *dir);
>  int snd_pcm_hw_params_get_period_time_min(const snd_pcm_hw_params_t *params, unsigned int *val, int *dir);
> diff --git a/include/sound/asound.h b/include/sound/asound.h
> index fa88938..fbcfef9 100644
> --- a/include/sound/asound.h
> +++ b/include/sound/asound.h
> @@ -278,6 +278,8 @@ enum sndrv_pcm_subformat {
>  #define SNDRV_PCM_INFO_HALF_DUPLEX	0x00100000	/* only half duplex */
>  #define SNDRV_PCM_INFO_JOINT_DUPLEX	0x00200000	/* playback and capture stream are somewhat correlated */
>  #define SNDRV_PCM_INFO_SYNC_START	0x00400000	/* pcm support some kind of sync go */
> +#define SNDRV_PCM_INFO_NO_PERIOD_IRQ    0x00800000      /* period interrupt can be disabled */
> +#define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
>  
>  enum sndrv_pcm_state {
>  	SNDRV_PCM_STATE_OPEN = 0,	/* stream is open */
> @@ -346,7 +348,8 @@ enum sndrv_pcm_hw_param {
>  
>  #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)  /* avoid rate resampling */
>  #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)  /* export buffer */
> -
> +#define SNDRV_PCM_HW_PARAMS_NO_PERIOD_IRQ      (1<<2)  /* disable period
> +                                                          interrupts */
>  struct sndrv_interval {
>  	unsigned int min, max;
>  	unsigned int openmin:1,
> diff --git a/src/Versions.in b/src/Versions.in
> index 8d2dd11..112b9ec 100644
> --- a/src/Versions.in
> +++ b/src/Versions.in
> @@ -129,3 +129,10 @@ ALSA_0.9.7 {
>      @SYMBOL_PREFIX@alsa_lisp_*;
>  } ALSA_0.9.5;
>  
> +ALSA_1.0.24 {
> +  global:
> +    @SYMBOL_PREFIX@snd_pcm_hw_params_can_disable_period_irq;
> +    @SYMBOL_PREFIX@snd_pcm_hw_params_set_period_irq;
> +    @SYMBOL_PREFIX@snd_pcm_hw_params_get_period_irq;
> +
> +} ALSA_0.9.7;
> \ No newline at end of file

In general we don't put this any more.  All snd_* are exported unless
blacklisted.  So, avoid changing the version number in your patch.


thanks,

Takashi

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Clemens Ladisch]

Sorry for not answering sooner.

Pierre-Louis Bossart wrote:
> (No filtering in pcm_multi and pcm_direct info fields)

I'm not sure whether filtering or not filtering would be a good idea for
yet unknown flags.

> + * It is not allowed to call this function when given configuration is not exactly one.
> ...
> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params)

This function is useless because it cannot be called before
snd_pcm_hw_params_set_period_irq().

> + * This function should be called only on devices where
> + * #snd_pcm_hw_params_can_disable_period_irq() returns true. (too late, FIXME)

Therefore, this paragraph needs to go away.

I think there should be a flag that can be added to snd_pcm_open, like
SND_PCM_NO_AUTO_RESAMPLE.

> + * Even with disabled period interrupts, the period size/time/count parameters
> + * are valid; it is suggested to use #snd_pcm_hw_params_set_period_size_last().
> + *
> + * When period interrupts are disabled, the application must not use poll() or
> + * any functions that could block on this device.

We should also mention that the application must call some update
function often enough.


Regards,
Clemens

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Clemens Ladisch wrote:

>> + * When period interrupts are disabled, the application must not use poll() or
>> + * any functions that could block on this device.
>
> We should also mention that the application must call some update
> function often enough.

Reading this - the poll() inside the driver may return POLLERR when no 
period updates are invoked in the driver. It does not make much sense to 
block the application when we know that the wake up function is not 
supported.

Also all wait functions in alsa-lib should be reviewed and changed to 
return an error. Also, the blocking behaviour should not be allowed - it 
means that apps must use SND_PCM_NONBLOCK or snd_pcm_nonblock(pcm, 1) for 
this mode.

 						Jaroslav

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

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Clemens Ladisch wrote:

>> + * It is not allowed to call this function when given configuration is not exactly one.
>> ...
>> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params)
>
> This function is useless because it cannot be called before
> snd_pcm_hw_params_set_period_irq().

I don't think so. The info bits from the driver are used in this check 
which are read-only.

>> + * This function should be called only on devices where
>> + * #snd_pcm_hw_params_can_disable_period_irq() returns true. (too late, FIXME)
>
> Therefore, this paragraph needs to go away.
>
> I think there should be a flag that can be added to snd_pcm_open, like
> SND_PCM_NO_AUTO_RESAMPLE.

It's not required. It's better to allow checking if no period update 
feature is available after snd_pcm_open().

 						Jaroslav

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

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Clemens Ladisch]

Jaroslav Kysela wrote:
> On Tue, 2 Nov 2010, Clemens Ladisch wrote:
>>> + * It is not allowed to call this function when given configuration is not exactly one.
>>> ...
>>> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params)
>>
>> This function is useless because it cannot be called before
>> snd_pcm_hw_params_set_period_irq().
> 
> I don't think so. The info bits from the driver are used in this check 
> which are read-only.

All the snd_pcm_hw_params_can_* functions that read the info bits
are documented to require a completed configuration.  So they actually
don't, and there will never be a flag that depends on the configuration?


Regards,
Clemens

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Jaroslav Kysela]

On Tue, 2 Nov 2010, Clemens Ladisch wrote:

> Jaroslav Kysela wrote:
>> On Tue, 2 Nov 2010, Clemens Ladisch wrote:
>>>> + * It is not allowed to call this function when given configuration is not exactly one.
>>>> ...
>>>> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t *params)
>>>
>>> This function is useless because it cannot be called before
>>> snd_pcm_hw_params_set_period_irq().
>>
>> I don't think so. The info bits from the driver are used in this check
>> which are read-only.
>
> All the snd_pcm_hw_params_can_* functions that read the info bits
> are documented to require a completed configuration.  So they actually
> don't, and there will never be a flag that depends on the configuration?

You're right. The *_can_* check should be removed.

 						Jaroslav

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

Re: [PATCH 1/2] pcm: pass hw_params flags to slave

[pl bossart]

Thanks Takashi for the review.

> I'm a bit skeptical whether this is safe to apply.
> The plugins might not be always transparent.  Double-check whether
> this doesn't break all plugins behaviors.

I am really not sure how I can prove that passing flags does not alter
existing behavior, I identified this issue experimentally after
several hours and I must admit I don't really understand the alsa-lib
code.
The question is really why weren't the flags propagated in the initial
code? All parameters (info, rate, etc) are passed to the slave, except
the flags. Either this was a conscious design decision, in which case
my patch would create a problem, or this was a miss and my patch
corrects it.

>>       params->info &= src->info;
>> +        params->flags = src->flags; /* propagate flags to slave */

Re: [PATCH 2/2] add API to allow disabling period interrupt

[pl bossart]

>>>>> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t
>>>>> *params)
>>>>
>>>> This function is useless because it cannot be called before
>>>> snd_pcm_hw_params_set_period_irq().
>>>
>>> I don't think so. The info bits from the driver are used in this check
>>> which are read-only.
>>
>> All the snd_pcm_hw_params_can_* functions that read the info bits
>> are documented to require a completed configuration.  So they actually
>> don't, and there will never be a flag that depends on the configuration?
>
> You're right. The *_can_* check should be removed.

If I remove the *_can_* check and add a flag for snd_pcm_open,
actually the other set/get routines need to go as well. Everything
would happen at the opening stages instead of when the hardware params
are set, you would try to disable the interrupts and if the open fails
you would fall back to 'normal' mode.
The alternative would be to remove the _*can* but keep the set/get
routines. In that case the hw_params configuration would be rejected
if the hardware doesn't support this feature.
Which solution do you guys prefer?

Either way, it makes sense to check that the app uses the non-blocking
mode as well, otherwise everything would be broken as noted in the
remarks.

-Pierre

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2643 bytes --]

On Tue, 2 Nov 2010, pl bossart wrote:

>>>>>> +int snd_pcm_hw_params_can_disable_period_irq(const snd_pcm_hw_params_t
>>>>>> *params)
>>>>>
>>>>> This function is useless because it cannot be called before
>>>>> snd_pcm_hw_params_set_period_irq().
>>>>
>>>> I don't think so. The info bits from the driver are used in this check
>>>> which are read-only.
>>>
>>> All the snd_pcm_hw_params_can_* functions that read the info bits
>>> are documented to require a completed configuration.  So they actually
>>> don't, and there will never be a flag that depends on the configuration?
>>
>> You're right. The *_can_* check should be removed.
>
> If I remove the *_can_* check and add a flag for snd_pcm_open,
> actually the other set/get routines need to go as well. Everything
> would happen at the opening stages instead of when the hardware params
> are set, you would try to disable the interrupts and if the open fails
> you would fall back to 'normal' mode.

I'm not sure if it's good to have another flag for snd_pcm_open. If we 
accept new open flag, it would be just an optional hint which should be 
verified by the application if it's really enabled using a function.

> The alternative would be to remove the _*can* but keep the set/get
> routines. In that case the hw_params configuration would be rejected
> if the hardware doesn't support this feature.
> Which solution do you guys prefer?

Note that there is a big difference between your new flag and the existing 
ones. The *_can_* functions just reflects the extended read only 
capabilities:

snd_pcm_hw_params_can_mmap_sample_resolution
snd_pcm_hw_params_can_overrange
snd_pcm_hw_params_can_pause
snd_pcm_hw_params_can_resume
snd_pcm_hw_params_can_sync_start

I see two ways:

1) leave set/get functions with this semantics:
    - application tries to set the no period update behaviour for hw_params
      - alsa-lib should check if nonblock flag is set at this moment
    - snd_pcm_hw_params() call
    - application reads back the no period update state and if the flag is
      not set, it will use standard period updates
2) create new optional snd_pcm_open flag
    - application opens device with nonblock and no period update flags
    - snd_pcm_hw_params() call
    - application reads back the no period update state and if the flag is
      not set, it will use standard period updates

Note that we can even implement both methods together like for 
handling the nonblock behaviour.

 						Jaroslav

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/2] add API to allow disabling period interrupt

[pl bossart]

> I see two ways:
>
> 1) leave set/get functions with this semantics:
>   - application tries to set the no period update behaviour for hw_params
>     - alsa-lib should check if nonblock flag is set at this moment
>   - snd_pcm_hw_params() call
>   - application reads back the no period update state and if the flag is
>     not set, it will use standard period updates
> 2) create new optional snd_pcm_open flag
>   - application opens device with nonblock and no period update flags
>   - snd_pcm_hw_params() call
>   - application reads back the no period update state and if the flag is
>     not set, it will use standard period updates
>
> Note that we can even implement both methods together like for handling the
> nonblock behaviour.

I re-implemented the first solution and checked if the mode is indeed
non-blocking. This solution assumes that the apps cannot go back to
blocking mode at a later stage. Is it a valid asumption, or can the
blocking mode be enabled/disabled at any time with snd_pcm_nonblock()?
-Pierre

Re: [PATCH 2/2] add API to allow disabling period interrupt

[Jaroslav Kysela]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1329 bytes --]

On Mon, 8 Nov 2010, pl bossart wrote:

>> I see two ways:
>>
>> 1) leave set/get functions with this semantics:
>>   - application tries to set the no period update behaviour for hw_params
>>     - alsa-lib should check if nonblock flag is set at this moment
>>   - snd_pcm_hw_params() call
>>   - application reads back the no period update state and if the flag is
>>     not set, it will use standard period updates
>> 2) create new optional snd_pcm_open flag
>>   - application opens device with nonblock and no period update flags
>>   - snd_pcm_hw_params() call
>>   - application reads back the no period update state and if the flag is
>>     not set, it will use standard period updates
>>
>> Note that we can even implement both methods together like for handling the
>> nonblock behaviour.
>
> I re-implemented the first solution and checked if the mode is indeed
> non-blocking. This solution assumes that the apps cannot go back to
> blocking mode at a later stage. Is it a valid asumption, or can the
> blocking mode be enabled/disabled at any time with snd_pcm_nonblock()?

Yes, this function must return an error if an app tries to set blocking 
mode..

 						Jaroslav

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel