[alsa-devel] [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress
We don't always need soc-compress in soc, here add a config item SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to driver Kconfig when it is needed.
Signed-off-by: Jie Yang yang.jie@intel.com --- include/sound/soc.h | 7 +++++++ sound/soc/Kconfig | 5 ++++- sound/soc/Makefile | 3 ++- sound/soc/intel/Kconfig | 1 + 4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 93df8bf..cc1cd4f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -440,7 +440,14 @@ int snd_soc_platform_read(struct snd_soc_platform *platform, int snd_soc_platform_write(struct snd_soc_platform *platform, unsigned int reg, unsigned int val); int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num); +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS) int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num); +#else +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) +{ + return -EPERM; +} +#endif
struct snd_pcm_substream *snd_soc_get_dai_substream(struct snd_soc_card *card, const char *dai_link, int stream); diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index e2828e1..a124759 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -9,7 +9,6 @@ menuconfig SND_SOC select SND_JACK if INPUT=y || INPUT=SND select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER - select SND_COMPRESS_OFFLOAD ---help---
If you want ASoC support, you should say Y here and also to the @@ -30,6 +29,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM bool select SND_DMAENGINE_PCM
+config SND_SOC_COMPRESS + tristate + select SND_COMPRESS_OFFLOAD + # All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/atmel/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index a0e1ee6..184c1e6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,7 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o soc-devres.o soc-ops.o +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate + select SND_SOC_COMPRESS
config SND_SST_IPC tristate
Split soc-compress into a separate module so we only compile/load it when it's needed.
Signed-off-by: Jie Yang yang.jie@intel.com --- sound/soc/Makefile | 4 +++- sound/soc/soc-compress.c | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/sound/soc/Makefile b/sound/soc/Makefile index 184c1e6..2936088 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,7 +1,9 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o -snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o +snd-soc-compress-objs := soc-compress.o + +obj-$(CONFIG_SND_SOC_COMPRESS) += snd-soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 025c38f..a93ac8a 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -19,6 +19,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/workqueue.h> +#include <linux/module.h> #include <sound/core.h> #include <sound/compress_params.h> #include <sound/compress_driver.h> @@ -703,3 +704,10 @@ compr_err: kfree(compr); return ret; } +EXPORT_SYMBOL_GPL(soc_new_compress); + +/* Module information */ +MODULE_AUTHOR("Ramesh Babu K V ramesh.babu@linux.intel.com"); +MODULE_AUTHOR("Vinod Koul vinod.koul@linux.intel.com"); +MODULE_DESCRIPTION("ALSA SoC Compress"); +MODULE_LICENSE("GPL v2");
On Mon, Jun 15, 2015 at 11:20:45AM +0800, Jie Yang wrote:
Split soc-compress into a separate module so we only compile/load it when it's needed.
Given that this patch is so much simpler than the previous one it would seem better to make it the first patch - I'd be OK to apply it but it depends on the prior patch.
At Mon, 15 Jun 2015 11:20:44 +0800, Jie Yang wrote:
We don't always need soc-compress in soc, here add a config item SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to driver Kconfig when it is needed.
Signed-off-by: Jie Yang yang.jie@intel.com
include/sound/soc.h | 7 +++++++ sound/soc/Kconfig | 5 ++++- sound/soc/Makefile | 3 ++- sound/soc/intel/Kconfig | 1 + 4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 93df8bf..cc1cd4f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -440,7 +440,14 @@ int snd_soc_platform_read(struct snd_soc_platform *platform, int snd_soc_platform_write(struct snd_soc_platform *platform, unsigned int reg, unsigned int val); int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num); +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS) int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num); +#else +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) +{
return -EPERM;+} +#endif
A dummy function in such a case has both merit and demerit. The demerit is that you won't get errors if the driver really wants the compress support but just forgot to select the dependency.
Takashi
struct snd_pcm_substream *snd_soc_get_dai_substream(struct snd_soc_card *card, const char *dai_link, int stream); diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index e2828e1..a124759 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -9,7 +9,6 @@ menuconfig SND_SOC select SND_JACK if INPUT=y || INPUT=SND select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER
select SND_COMPRESS_OFFLOAD ---help---
If you want ASoC support, you should say Y here and also to the
@@ -30,6 +29,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM bool select SND_DMAENGINE_PCM
+config SND_SOC_COMPRESS
- tristate
- select SND_COMPRESS_OFFLOAD
# All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/atmel/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index a0e1ee6..184c1e6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,7 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o soc-devres.o soc-ops.o +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate
- select SND_SOC_COMPRESS
config SND_SST_IPC tristate -- 1.9.1
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 15, 2015 7:34 PM To: Jie, Yang Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Mon, 15 Jun 2015 11:20:44 +0800, Jie Yang wrote:
We don't always need soc-compress in soc, here add a config item SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to driver Kconfig when it is needed.
Signed-off-by: Jie Yang yang.jie@intel.com
include/sound/soc.h | 7 +++++++ sound/soc/Kconfig | 5 ++++- sound/soc/Makefile | 3 ++- sound/soc/intel/Kconfig | 1 + 4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 93df8bf..cc1cd4f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -440,7 +440,14 @@ int snd_soc_platform_read(struct
snd_soc_platform
*platform, int snd_soc_platform_write(struct snd_soc_platform *platform, unsigned int reg, unsigned int val);
int soc_new_pcm(struct
snd_soc_pcm_runtime *rtd, int num); +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS) int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num); +#else +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, +int num) {
return -EPERM;+} +#endif
A dummy function in such a case has both merit and demerit. The demerit is that you won't get errors if the driver really wants the compress support but just forgot to select the dependency.
Here I used -EPERM to return and tell caller that compress operation is not permitted, does it make sense, Takashi?
Or can you suggest a better solution, according to this specific case?
Thanks, ~Keyon
Takashi
struct snd_pcm_substream *snd_soc_get_dai_substream(struct
snd_soc_card *card,
const char *dai_link, int stream);diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index e2828e1..a124759 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -9,7 +9,6 @@ menuconfig SND_SOC select SND_JACK if INPUT=y || INPUT=SND select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER
select SND_COMPRESS_OFFLOAD ---help---
If you want ASoC support, you should say Y here and also to the @@
-30,6 +29,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM bool select SND_DMAENGINE_PCM
+config SND_SOC_COMPRESS
- tristate
- select SND_COMPRESS_OFFLOAD
# All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/atmel/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index a0e1ee6..184c1e6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,7 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o soc-devres.o soc-ops.o +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate
- select SND_SOC_COMPRESS
config SND_SST_IPC tristate -- 1.9.1
At Mon, 15 Jun 2015 14:15:40 +0000, Jie, Yang wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 15, 2015 7:34 PM To: Jie, Yang Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Mon, 15 Jun 2015 11:20:44 +0800, Jie Yang wrote:
We don't always need soc-compress in soc, here add a config item SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to driver Kconfig when it is needed.
Signed-off-by: Jie Yang yang.jie@intel.com
include/sound/soc.h | 7 +++++++ sound/soc/Kconfig | 5 ++++- sound/soc/Makefile | 3 ++- sound/soc/intel/Kconfig | 1 + 4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 93df8bf..cc1cd4f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -440,7 +440,14 @@ int snd_soc_platform_read(struct
snd_soc_platform
*platform, int snd_soc_platform_write(struct snd_soc_platform *platform, unsigned int reg, unsigned int val);
int soc_new_pcm(struct
snd_soc_pcm_runtime *rtd, int num); +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS) int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num); +#else +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, +int num) {
return -EPERM;+} +#endif
A dummy function in such a case has both merit and demerit. The demerit is that you won't get errors if the driver really wants the compress support but just forgot to select the dependency.
Here I used -EPERM to return and tell caller that compress operation is not permitted, does it make sense, Takashi?
The question is whether a runtime error is the best option. A runtime error won't be caught by build tests but only when actually running by a user.
Or can you suggest a better solution, according to this specific case?
Well, there are several ways, obviously (e.g. give the preprocessor error if CONFIG_SND_SOC_COMPRESS is enabled). But I'm not sure what is the best to fit, too. It's rather a matter of taste.
Takashi
Thanks, ~Keyon
Takashi
struct snd_pcm_substream *snd_soc_get_dai_substream(struct
snd_soc_card *card,
const char *dai_link, int stream);diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index e2828e1..a124759 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -9,7 +9,6 @@ menuconfig SND_SOC select SND_JACK if INPUT=y || INPUT=SND select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER
select SND_COMPRESS_OFFLOAD ---help---
If you want ASoC support, you should say Y here and also to the @@
-30,6 +29,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM bool select SND_DMAENGINE_PCM
+config SND_SOC_COMPRESS
- tristate
- select SND_COMPRESS_OFFLOAD
# All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/atmel/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index a0e1ee6..184c1e6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,7 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o soc-devres.o soc-ops.o +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate
- select SND_SOC_COMPRESS
config SND_SST_IPC tristate -- 1.9.1
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 15, 2015 10:27 PM To: Jie, Yang Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Mon, 15 Jun 2015 14:15:40 +0000, Jie, Yang wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, June 15, 2015 7:34 PM To: Jie, Yang Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Mon, 15 Jun 2015 11:20:44 +0800, Jie Yang wrote:
We don't always need soc-compress in soc, here add a config item SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to
driver
Kconfig when it is needed.
Signed-off-by: Jie Yang yang.jie@intel.com
include/sound/soc.h | 7 +++++++ sound/soc/Kconfig | 5 ++++- sound/soc/Makefile | 3 ++- sound/soc/intel/Kconfig | 1 + 4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 93df8bf..cc1cd4f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -440,7 +440,14 @@ int snd_soc_platform_read(struct
snd_soc_platform
*platform, int snd_soc_platform_write(struct snd_soc_platform
*platform,
unsigned int reg, unsigned int val);int soc_new_pcm(struct
snd_soc_pcm_runtime *rtd, int num); +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS) int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num); +#else +static inline int soc_new_compress(struct snd_soc_pcm_runtime +*rtd, int num) {
return -EPERM;+} +#endif
A dummy function in such a case has both merit and demerit. The demerit is that you won't get errors if the driver really wants the compress support but just forgot to select the dependency.
Here I used -EPERM to return and tell caller that compress operation is not permitted, does it make sense, Takashi?
The question is whether a runtime error is the best option. A runtime error won't be caught by build tests but only when actually running by a user.
Unfortunately, here cpu_dai->driver->compress_dai is a runtime value, which means we need compress API when it is true. Seems it is not easy to decide it at compile stage?
Or can you suggest a better solution, according to this specific case?
Well, there are several ways, obviously (e.g. give the preprocessor error if CONFIG_SND_SOC_COMPRESS is enabled). But I'm not sure what is the best
Do you mean giving error, when CONFIG_SND_SOC_COMPRESS is disabled but the driver wants the compress support at the same time?
As I stated above, it's not easy to detect whether the driver wants the compress support, at the preprocessor/compile stage. :(
~Keyon
to fit, too. It's rather a matter of taste.
Takashi
Thanks, ~Keyon
Takashi
struct snd_pcm_substream *snd_soc_get_dai_substream(struct
snd_soc_card *card,
const char *dai_link, int stream); diff --gita/sound/soc/Kconfig b/sound/soc/Kconfig index e2828e1..a124759 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -9,7 +9,6 @@ menuconfig SND_SOC select SND_JACK if INPUT=y || INPUT=SND select REGMAP_I2C if I2C select REGMAP_SPI if SPI_MASTER
select SND_COMPRESS_OFFLOAD ---help---
If you want ASoC support, you should say Y here and also to
the @@ -30,6 +29,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM bool select SND_DMAENGINE_PCM
+config SND_SOC_COMPRESS
- tristate
- select SND_COMPRESS_OFFLOAD
# All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/atmel/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index a0e1ee6..184c1e6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,7 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o soc-devres.o soc-ops.o +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o snd-soc-core-objs += soc-topology.o +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),) snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate
- select SND_SOC_COMPRESS
config SND_SST_IPC tristate -- 1.9.1
On Mon, Jun 15, 2015 at 02:46:23PM +0000, Jie, Yang wrote:
Here I used -EPERM to return and tell caller that compress operation is not permitted, does it make sense, Takashi?
The question is whether a runtime error is the best option. A runtime error won't be caught by build tests but only when actually running by a user.
Unfortunately, here cpu_dai->driver->compress_dai is a runtime value, which means we need compress API when it is true. Seems it is not easy to decide it at compile stage?
The machine driver (which is presumably the thing that should be doing the select here, it's not user visible) really ought to know if the DAI links it is creating are for compressed audio. This is why I'm still surprised there's no driver updates as part of this patch. If the Intel drivers referencing compressed don't actually implement it then I'd expect to see patches cleaning up the references to compressed audio.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Monday, June 15, 2015 11:06 PM To: Jie, Yang Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
On Mon, Jun 15, 2015 at 02:46:23PM +0000, Jie, Yang wrote:
Here I used -EPERM to return and tell caller that compress operation is not permitted, does it make sense, Takashi?
The question is whether a runtime error is the best option. A runtime error won't be caught by build tests but only when actually
running by a user.
Unfortunately, here cpu_dai->driver->compress_dai is a runtime value, which means we need compress API when it is true. Seems it is not easy to decide it at compile stage?
The machine driver (which is presumably the thing that should be doing the select here, it's not user visible) really ought to know if the DAI links it is creating are for compressed audio. This is why I'm still surprised there's no driver updates as part of this patch. If the Intel drivers referencing compressed don't actually implement it then I'd expect to see patches cleaning up the references to compressed audio.
Mark, as you may see in this patch(1/2), I have added this selection to sound/ Soc/intel/Kconfig, we can find .compress_dai =1 in sst-mfld-platform-pcm.c, the compressed is referenced in sst-mfld-platform-pcm.c:sst_platform_dai, here copy the related part from the patch:
b/sound/soc/intel/Kconfig index 791953f..e559174 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
config SND_SST_MFLD_PLATFORM tristate + select SND_SOC_COMPRESS
config SND_SST_IPC tristate
~Keyon
On Tue, Jun 16, 2015 at 12:49:37AM +0000, Jie, Yang wrote:
Mark, as you may see in this patch(1/2), I have added this selection to sound/ Soc/intel/Kconfig, we can find .compress_dai =1 in sst-mfld-platform-pcm.c, the compressed is referenced in sst-mfld-platform-pcm.c:sst_platform_dai, here copy the related part from the patch:
OK, that should have been in the commit message.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Tuesday, June 16, 2015 6:54 PM To: Jie, Yang Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
On Tue, Jun 16, 2015 at 12:49:37AM +0000, Jie, Yang wrote:
Mark, as you may see in this patch(1/2), I have added this selection to sound/ Soc/intel/Kconfig, we can find .compress_dai =1 in sst-mfld-platform-pcm.c, the compressed is referenced in sst-mfld-platform-pcm.c:sst_platform_dai, here copy the related part from the patch:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
~Keyon
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Takashi
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Takashi
On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai links. Since ASoC drivers don't use any of compress APIs directly, it would be difficult to add a compile time failure so we are stuck with silent runtime failures :(
At Tue, 16 Jun 2015 21:55:07 +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai links. Since ASoC drivers don't use any of compress APIs directly, it would be difficult to add a compile time failure so we are stuck with silent runtime failures :(
One feasible option would be to make the driver's dai creation a function pointer to the generic constructor. That is, soc_probe_link_dais() will run like:
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
Takashi
On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 21:55:07 +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai links. Since ASoC drivers don't use any of compress APIs directly, it would be difficult to add a compile time failure so we are stuck with silent runtime failures :(
One feasible option would be to make the driver's dai creation a function pointer to the generic constructor. That is, soc_probe_link_dais() will run like:
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
and the default would be snd_pcm_new so that *most* drivers dont need to add this. Only Intel atom driver should add soc_new_compress() as create ops, hence direct reference so no gaurds required
Looks good to me then, Mark you okay with this approach?
On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:
Please remember to delete unneeded context from replies.
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
and the default would be snd_pcm_new so that *most* drivers dont need to add this. Only Intel atom driver should add soc_new_compress() as create ops, hence direct reference so no gaurds required
Looks good to me then, Mark you okay with this approach?
I'm not sure this fully helps. This will put the reference in the CPU driver which if there's sharing with non-compressed DAIs means that any machine driver using the platform needs to select the compressed audio code. It also means that we're still in the situation whre if a machine *can* support compressed audio it *must* support compressed audio, it's not clear to me that people doing this sort of memory optimisation are always going to be doing it on hardware that doesn't have the capacity for compressed audio (or won't in the future). But then I don't know exactly how much memory is being saved here...
On Wed, Jun 17, 2015 at 01:23:40PM +0100, Mark Brown wrote:
On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
and the default would be snd_pcm_new so that *most* drivers dont need to add this. Only Intel atom driver should add soc_new_compress() as create ops, hence direct reference so no gaurds required
Looks good to me then, Mark you okay with this approach?
I'm not sure this fully helps. This will put the reference in the CPU driver which if there's sharing with non-compressed DAIs means that any machine driver using the platform needs to select the compressed audio code. It also means that we're still in the situation whre if a machine *can* support compressed audio it *must* support compressed audio, it's not clear to me that people doing this sort of memory optimisation are always going to be doing it on hardware that doesn't have the capacity for compressed audio (or won't in the future). But then I don't know exactly how much memory is being saved here...
Yes this is a good point but then driver can define the compressed dai only when SND_SOC_COMPRESS is enabled. So if machine has enabled this symbol then only dai get added and machine can create dai-link
Also for the size question, in sound-next we have 248KB soc-compress.o
Thanks
On Wed, Jun 17, 2015 at 09:52:56PM +0530, Vinod Koul wrote:
On Wed, Jun 17, 2015 at 01:23:40PM +0100, Mark Brown wrote:
I'm not sure this fully helps. This will put the reference in the CPU driver which if there's sharing with non-compressed DAIs means that any machine driver using the platform needs to select the compressed audio code. It also means that we're still in the situation whre if a machine *can* support compressed audio it *must* support compressed audio, it's not clear to me that people doing this sort of memory optimisation are always going to be doing it on hardware that doesn't have the capacity for compressed audio (or won't in the future). But then I don't know exactly how much memory is being saved here...
Yes this is a good point but then driver can define the compressed dai only when SND_SOC_COMPRESS is enabled. So if machine has enabled this symbol then only dai get added and machine can create dai-link
Well, if it's a user configurable thing then the stubs start to make sense - the driver defines the DAI and then the core doesn't bother to instantiate it.
Also for the size question, in sound-next we have 248KB soc-compress.o
How big is the actual code - that sounds like it's got the debug symbols? I suppose I should go look myself...
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel- bounces@alsa-project.org] On Behalf Of Mark Brown Sent: Wednesday, June 17, 2015 8:24 PM To: Koul, Vinod Cc: Takashi Iwai; alsa-devel@alsa-project.org; Zhang, Vivian; Babu, Ramesh; Girdwood, Liam R Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress
On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:
Please remember to delete unneeded context from replies.
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
and the default would be snd_pcm_new so that *most* drivers dont need to add this. Only Intel atom driver should add soc_new_compress() as create ops, hence direct reference so no gaurds required
Looks good to me then, Mark you okay with this approach?
I'm not sure this fully helps. This will put the reference in the CPU driver which if there's sharing with non-compressed DAIs means that any machine driver using the platform needs to select the compressed audio code. It also means that we're still in the situation whre if a machine *can* support compressed audio it *must* support compressed audio, it's not clear to me that people doing this sort of memory optimisation are always going to be doing it on hardware that doesn't have the capacity for compressed audio (or won't in the future). But then I don't know exactly how much memory is being saved here...
It saves ~12KB memory usage on i386 platform on my side.
~Keyon
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, June 17, 2015 12:36 AM To: Koul, Vinod Cc: Mark Brown; Jie, Yang; alsa-devel@alsa-project.org; Girdwood, Liam R; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Tue, 16 Jun 2015 21:55:07 +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked
actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai links. Since ASoC drivers don't use any of compress APIs directly, it would be difficult to add a compile time failure so we are stuck with silent runtime failures :(
One feasible option would be to make the driver's dai creation a function pointer to the generic constructor. That is, soc_probe_link_dais() will run like:
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
Then we may need this in driver code:
static struct snd_soc_dai_driver sst_platform_dai[] = { ..., { .name = "compress-cpu-dai", .create = soc_new_compress, //.compress_dai = 1, // .create = NULL, //.compress_dai = 0, .ops = &sst_compr_dai_ops, .playback = { .stream_name = "Compress Playback", .channels_min = SST_STEREO, .channels_max = SST_STEREO, .rates = SNDRV_PCM_RATE_48000, .formats = SNDRV_PCM_FMTBIT_S16_LE, }, }, }
With this implementation, for driver which needed compress, if we forget to add 'select SND_SOC_COMPRESS' in Kconfig of the driver, compiler will check and report error(no NULL inline function defined).
Sounds this feasible for me, any extra comment for it?
~Keyon
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, June 17, 2015 12:36 AM To: Koul, Vinod Cc: Mark Brown; Jie, Yang; alsa-devel@alsa-project.org; Girdwood, Liam R; ramesh.babu@linux.intel.com; Zhang, Vivian Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc- compress
At Tue, 16 Jun 2015 21:55:07 +0530, Vinod Koul wrote:
On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked
actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai links. Since ASoC drivers don't use any of compress APIs directly, it would be difficult to add a compile time failure so we are stuck with silent runtime failures :(
One feasible option would be to make the driver's dai creation a function pointer to the generic constructor. That is, soc_probe_link_dais() will run like:
if (cpu_dai->driver->create) ret = cpu_dai->driver->create(rtd, num);
and each driver passes soc_new_compress (or whatever) there. This is flexible and can be extended if any new stuff has to be handled. Even PCM creation or dai widget links can be passed in that form, too.
How time flies! About 3 months passed away when I have chance to resume back to this thread.
Takashi, so as your suggestion, I will add a member to struct snd_soc_dai_driver:
struct snd_soc_dai_driver { ... + int (*create)(struct snd_soc_dai *dai); - bool compress_dai; ... }
And then change sst_platform_dai[] in sst-mfld-platform-pcm.c such as:
{ .name = "compress-cpu-dai", - .compress_dai = 1, + .create = soc_new_compress, .ops = &sst_compr_dai_ops, .playback = { .stream_name = "Compress Playback", .channels_min = SST_STEREO, .channels_max = SST_STEREO, .rates = SNDRV_PCM_RATE_48000, .formats = SNDRV_PCM_FMTBIT_S16_LE, }, },
And then, we can remove the dummy soc_new_compress() inline definition, is that right?
~Keyon
Takashi
On 06/16/2015 05:17 PM, Takashi Iwai wrote:
At Tue, 16 Jun 2015 18:14:40 +0200, Takashi Iwai wrote:
At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
OK, that should have been in the commit message.
OK, so let me add it to the commit message and resend?
Well, there's still the stubs to consider. Should we really be returning an error or should we silently ignore the error and hide the DAI if the user deconfigured compressed audio? Or rearrange things so we don't need stubs? Given that the machine driver has to select compressed support it's not something that should ever really hit the stubs, it's not truly a runtime thing...
Yes, I guess that leaving without dummy function will give unresolved symbol errors at module link time, so the user can catch before actually running it. Of course, this should be checked actually.
Oh, scratch this. It won't work well in the current code. Possibly with weak linking, but...
I don't know if I am missing something, but am I the only one who thinks these patches and the proposed changes are too complex for what seems to be little gain? Why is it that bad to always compile in soc-compress?
Qais
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (6)
-
Jie Yang -
Jie, Yang
-
Mark Brown -
Qais Yousef -
Takashi Iwai -
Vinod Koul