[alsa-devel] [PATCH 0/4] ASoC: Implement new caching API
This patch series introduces the new caching API. The idea behind this caching interface is that we can provide different means of organizing and accessing the register cache. This is useful for large and sparse register maps, where one can use some kind of compression algorithm to reduce the memory footprint. The caching API is designed in such way to eliminate the need for modifying any existing drivers.
Dimitris Papastamos (4): ASoC: soc.h: Add new caching API prototypes and hooks ASoC: soc-cache: Add support for standard register caching ASoC: soc-core: Adapt soc-core to fit the new caching API ASoC: soc-cache: Add support for LZO based register caching
include/sound/soc.h | 27 ++ sound/soc/soc-cache.c | 646 ++++++++++++++++++++++++++++++++++++++++++++++--- sound/soc/soc-core.c | 38 ++-- 3 files changed, 657 insertions(+), 54 deletions(-)
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- include/sound/soc.h | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 5c3bce8..29b4aef 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -238,6 +238,7 @@ struct soc_enum; struct snd_soc_ac97_ops; struct snd_soc_jack; struct snd_soc_jack_pin; +struct snd_soc_cache_ops;
#ifdef CONFIG_GPIOLIB struct snd_soc_jack_gpio; @@ -253,6 +254,11 @@ enum snd_soc_control_type { SND_SOC_SPI, };
+enum snd_soc_compress_type { + SND_SOC_NO_COMPRESSION, + SND_SOC_LZO_COMPRESSION +}; + int snd_soc_register_platform(struct device *dev, struct snd_soc_platform_driver *platform_drv); void snd_soc_unregister_platform(struct device *dev); @@ -264,6 +270,13 @@ int snd_soc_codec_volatile_register(struct snd_soc_codec *codec, int reg); int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, int addr_bits, int data_bits, enum snd_soc_control_type control); +int snd_soc_cache_sync(struct snd_soc_codec *codec); +int snd_soc_cache_init(struct snd_soc_codec *codec); +int snd_soc_cache_deinit(struct snd_soc_codec *codec); +int snd_soc_cache_write(struct snd_soc_codec *codec, + unsigned int reg, unsigned int value); +int snd_soc_cache_read(struct snd_soc_codec *codec, + unsigned int reg, unsigned int *value);
/* Utility functions to get clock rates from various things */ int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots); @@ -420,6 +433,18 @@ struct snd_soc_ops { int (*trigger)(struct snd_pcm_substream *, int); };
+/* SoC cache ops */ +struct snd_soc_cache_ops { + int id; /* corresponds to snd_soc_compress_type */ + int (*init)(struct snd_soc_codec *codec); + int (*deinit)(struct snd_soc_codec *codec); + int (*read)(struct snd_soc_codec *codec, unsigned int reg, + unsigned int *value); + int (*write)(struct snd_soc_codec *codec, unsigned int reg, + unsigned int value); + int (*sync)(struct snd_soc_codec *codec); +}; + /* SoC Audio Codec device */ struct snd_soc_codec { const char *name; @@ -450,6 +475,7 @@ struct snd_soc_codec { hw_write_t hw_write; unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int); void *reg_cache; + const struct snd_soc_cache_ops *cache_ops;
/* dapm */ u32 pop_time; @@ -488,6 +514,7 @@ struct snd_soc_codec_driver { short reg_cache_step; short reg_word_size; const void *reg_cache_default; + enum snd_soc_compress_type compress_type;
/* codec bias level */ int (*set_bias_level)(struct snd_soc_codec *,
On Fri, Oct 22, 2010 at 03:28:19PM +0100, Dimitris Papastamos wrote:
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com
Where possible you should try to split your series up such that each is useful individually - splitting things up by file is mostly done for the addition of large new blocks of code. This means that the patches can be applied one at a time without leaving the tree in an "interesting" state.
+int snd_soc_cache_sync(struct snd_soc_codec *codec); +int snd_soc_cache_init(struct snd_soc_codec *codec); +int snd_soc_cache_deinit(struct snd_soc_codec *codec); +int snd_soc_cache_write(struct snd_soc_codec *codec,
unsigned int reg, unsigned int value);+int snd_soc_cache_read(struct snd_soc_codec *codec,
unsigned int reg, unsigned int *value);
For example, a patch adding the cache operations but supporting only the current flat cache would introduce the cache API and factor out some code usefully by itself.
This patch adds support for normal caching (no compression). One can still access codec->reg_cache directly but this is not advised as that will not be portable across different caching strategies.
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- sound/soc/soc-cache.c | 267 +++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 236 insertions(+), 31 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d214f02..a8ec23a 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -18,7 +18,8 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, unsigned int reg) { - u16 *cache = codec->reg_cache; + int ret; + unsigned int val;
if (reg >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg)) { @@ -28,13 +29,15 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, return codec->hw_read(codec, reg); }
- return cache[reg]; + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + return val; }
static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u16 *cache = codec->reg_cache; u8 data[2]; int ret;
@@ -42,8 +45,11 @@ static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, data[1] = value & 0x00ff;
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -94,7 +100,8 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data, static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec, unsigned int reg) { - u16 *cache = codec->reg_cache; + int ret; + unsigned int val;
if (reg >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg)) { @@ -104,13 +111,15 @@ static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec, return codec->hw_read(codec, reg); }
- return cache[reg]; + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + return val; }
static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u16 *cache = codec->reg_cache; u8 data[2]; int ret;
@@ -118,8 +127,11 @@ static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg, data[1] = value & 0x00ff;
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -170,16 +182,19 @@ static int snd_soc_7_9_spi_write(void *control_data, const char *data, static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u8 *cache = codec->reg_cache; u8 data[2]; + int ret;
reg &= 0xff; data[0] = reg; data[1] = value & 0xff;
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -197,7 +212,8 @@ static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg, static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec, unsigned int reg) { - u8 *cache = codec->reg_cache; + int ret; + unsigned int val;
reg &= 0xff; if (reg >= codec->driver->reg_cache_size || @@ -208,7 +224,10 @@ static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec, return codec->hw_read(codec, reg); }
- return cache[reg]; + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + return val; }
#if defined(CONFIG_SPI_MASTER) @@ -244,16 +263,19 @@ static int snd_soc_8_8_spi_write(void *control_data, const char *data, static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u16 *reg_cache = codec->reg_cache; u8 data[3]; + int ret;
data[0] = reg; data[1] = (value >> 8) & 0xff; data[2] = value & 0xff;
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - reg_cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -271,7 +293,8 @@ static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg, static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec, unsigned int reg) { - u16 *cache = codec->reg_cache; + int ret; + unsigned int val;
if (reg >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg)) { @@ -279,9 +302,12 @@ static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec, return -1;
return codec->hw_read(codec, reg); - } else { - return cache[reg]; } + + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + return val; }
#if defined(CONFIG_SPI_MASTER) @@ -420,7 +446,8 @@ static unsigned int snd_soc_16_8_read_i2c(struct snd_soc_codec *codec, static unsigned int snd_soc_16_8_read(struct snd_soc_codec *codec, unsigned int reg) { - u8 *cache = codec->reg_cache; + int ret; + unsigned int val;
reg &= 0xff; if (reg >= codec->driver->reg_cache_size || @@ -431,13 +458,15 @@ static unsigned int snd_soc_16_8_read(struct snd_soc_codec *codec, return codec->hw_read(codec, reg); }
- return cache[reg]; + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + return val; }
static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u8 *cache = codec->reg_cache; u8 data[3]; int ret;
@@ -447,8 +476,11 @@ static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
reg &= 0xff; if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -534,7 +566,8 @@ static unsigned int snd_soc_16_16_read_i2c(struct snd_soc_codec *codec, static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec, unsigned int reg) { - u16 *cache = codec->reg_cache; + int ret; + unsigned int val;
if (reg >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg)) { @@ -544,13 +577,16 @@ static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec, return codec->hw_read(codec, reg); }
- return cache[reg]; + ret = snd_soc_cache_read(codec, reg, &val); + if (ret < 0) + return -1; + + return val; }
static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u16 *cache = codec->reg_cache; u8 data[4]; int ret;
@@ -560,8 +596,11 @@ static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg, data[3] = value & 0xff;
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size) - cache[reg] = value; + reg < codec->driver->reg_cache_size) { + ret = snd_soc_cache_write(codec, reg, value); + if (ret < 0) + return -1; + }
if (codec->cache_only) { codec->cache_sync = 1; @@ -724,3 +763,169 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, return 0; } EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io); + +static int snd_soc_cache_default_sync(struct snd_soc_codec *codec) +{ + const u8 *cache; + struct snd_soc_codec_driver *codec_drv; + unsigned int val; + int n; + + codec_drv = codec->driver; + for (n = 0; n < codec_drv->reg_cache_size; ++n) { + snd_soc_cache_read(codec, n, &val); + if (codec_drv->reg_cache_default) { + cache = codec_drv->reg_cache_default; + cache += (n * codec_drv->reg_word_size); + if (!memcmp(&val, cache, codec_drv->reg_word_size)) + continue; + } + snd_soc_write(codec, n, val); + } + return 0; +} + +static int snd_soc_cache_default_write(struct snd_soc_codec *codec, + unsigned int reg, unsigned int value) +{ + u8 *cache; + + cache = codec->reg_cache; + memcpy(&cache[reg * codec->driver->reg_word_size], + &value, codec->driver->reg_word_size); + return 0; +} + +static int snd_soc_cache_default_read(struct snd_soc_codec *codec, + unsigned int reg, unsigned int *value) +{ + u8 *cache; + + *value = 0; + cache = codec->reg_cache; + memcpy(value, &cache[reg * codec->driver->reg_word_size], + codec->driver->reg_word_size); + return 0; +} + +static int snd_soc_cache_default_deinit(struct snd_soc_codec *codec) +{ + kfree(codec->reg_cache); + return 0; +}; + +static int snd_soc_cache_default_init(struct snd_soc_codec *codec) +{ + struct snd_soc_codec_driver *codec_drv; + size_t reg_size; + + codec_drv = codec->driver; + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + + if (codec_drv->reg_cache_default) + codec->reg_cache = kmemdup(codec_drv->reg_cache_default, + reg_size, GFP_KERNEL); + else + codec->reg_cache = kzalloc(reg_size, GFP_KERNEL); + if (!codec->reg_cache) + return -EINVAL; + + return 0; +} + +/* an array of all supported compression types */ +static const struct snd_soc_cache_ops cache_types[] = { + { + .id = SND_SOC_NO_COMPRESSION, + .init = snd_soc_cache_default_init, + .deinit = snd_soc_cache_default_deinit, + .read = snd_soc_cache_default_read, + .write = snd_soc_cache_default_write, + .sync = snd_soc_cache_default_sync + } +}; + +int snd_soc_cache_init(struct snd_soc_codec *codec) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(cache_types); ++i) + if (cache_types[i].id == codec->driver->compress_type) + break; + if (i == ARRAY_SIZE(cache_types)) { + dev_err(codec->dev, "Could not match compress type: %d\n", + codec->driver->compress_type); + return -EINVAL; + } + + codec->cache_ops = &cache_types[i]; + + if (codec->cache_ops->init) + return codec->cache_ops->init(codec); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(snd_soc_cache_init); + +int snd_soc_cache_deinit(struct snd_soc_codec *codec) +{ + if (codec->cache_ops && codec->cache_ops->deinit) + return codec->cache_ops->deinit(codec); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(snd_soc_cache_deinit); + +/** + * snd_soc_cache_read: Fetch the value of a given register from the cache. + * + * @codec: CODEC to configure. + * @reg: The register index. + * @value: The value to be returned. + */ +int snd_soc_cache_read(struct snd_soc_codec *codec, + unsigned int reg, unsigned int *value) +{ + if (value && codec->cache_ops && codec->cache_ops->read) + return codec->cache_ops->read(codec, reg, value); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(snd_soc_cache_read); + +/** + * snd_soc_cache_write: Set the value of a given register in the cache. + * + * @codec: CODEC to configure. + * @reg: The register index. + * @value: The new register value. + */ +int snd_soc_cache_write(struct snd_soc_codec *codec, + unsigned int reg, unsigned int value) +{ + if (codec->cache_ops && codec->cache_ops->write) + return codec->cache_ops->write(codec, reg, value); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(snd_soc_cache_write); + +/** + * snd_soc_cache_sync: Sync the register cache with the hardware. + * + * @codec: CODEC to configure. + * + * Any registers that should not be synced should be marked as + * volatile. + */ +int snd_soc_cache_sync(struct snd_soc_codec *codec) +{ + int ret; + + if (!codec->cache_sync) + return 0; + if (codec->cache_ops && codec->cache_ops->sync) { + ret = codec->cache_ops->sync(codec); + if (!ret) + codec->cache_sync = 0; + return ret; + } + return -EINVAL; +} +EXPORT_SYMBOL_GPL(snd_soc_cache_sync);
On Fri, Oct 22, 2010 at 03:28:20PM +0100, Dimitris Papastamos wrote:
+static int snd_soc_cache_default_sync(struct snd_soc_codec *codec) +{
const u8 *cache;struct snd_soc_codec_driver *codec_drv;unsigned int val;codec_drv = codec->driver;
for (n = 0; n < codec_drv->reg_cache_size; ++n) {
Please use i as an array index unless using something meaningful.
if (!memcmp(&val, cache, codec_drv->reg_word_size))continue;
This memcmp() looks very suspicious - we're copying from an unsigned int into a variable of another type. That seems to have a bit of an endianness assumption, doesn't it? It certainly needs comments explaining how it works; a similar thing applies to the other memcpy() and memcmp() operations in the code.
+static int snd_soc_cache_default_deinit(struct snd_soc_codec *codec) +{
_exit or _free would be traditional.
- kfree(codec->reg_cache);
- return 0;
+};
Extra ; (I'm surprised nothing warns).
On Fri, 2010-10-22 at 09:03 -0700, Mark Brown wrote:
On Fri, Oct 22, 2010 at 03:28:20PM +0100, Dimitris Papastamos wrote:
+static int snd_soc_cache_default_sync(struct snd_soc_codec *codec) +{
const u8 *cache;struct snd_soc_codec_driver *codec_drv;unsigned int val;codec_drv = codec->driver;
for (n = 0; n < codec_drv->reg_cache_size; ++n) {
Please use i as an array index unless using something meaningful.
if (!memcmp(&val, cache, codec_drv->reg_word_size))continue;This memcmp() looks very suspicious - we're copying from an unsigned int into a variable of another type. That seems to have a bit of an endianness assumption, doesn't it? It certainly needs comments explaining how it works; a similar thing applies to the other memcpy() and memcmp() operations in the code.
Consider the following example. (unsigned int is 4 bytes).
unsigned int old = 0xABCD, new = 0; void *p;
On a little-endian system this will be stored in memory as DCBA with D being at a lower address. Now consider the following code.
p = &old; memcpy(&new, p, sizeof (unsigned int));
Now the value of new will be 0xABCD (stored in memory as DCBA again). This holds both on a little-endian system as well as a big-endian system.
The only problem I see with the above code, is when codec_drv->reg_word_size > sizeof (unsigned int) but that can't really happen in practice.
Thanks, Dimitrios
On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos dp@opensource.wolfsonmicro.com wrote:
The only problem I see with the above code, is when codec_drv->reg_word_size > sizeof (unsigned int) but that can't really happen in practice.
I'm going to have to agree with Mark that this code is suspect. I understand everything you said, but it makes me nervous. Unless this code is in some kind of fast-path, I would prefer to see it rewritten to avoid any assumption about the sizes of the types involved.
On Sun, Oct 24, 2010 at 08:18:59AM -0500, Timur Tabi wrote:
On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos
The only problem I see with the above code, is when codec_drv->reg_word_size > sizeof (unsigned int) but that can't really happen in practice.
Plus if it did happen the rest of the code would fall over fairly badly since we've got that assumption embedded in the register read and write APIs.
I'm going to have to agree with Mark that this code is suspect. I understand everything you said, but it makes me nervous. Unless this code is in some kind of fast-path, I would prefer to see it rewritten to avoid any assumption about the sizes of the types involved.
I think the important thing here is that the code is clear - from a maintainability so long as it's clear how and why the code works things should be fine, otherwise we'll have people scratching their heads over it every time someone looks at the code which is going to be painful. This could be done with documentation as well as with code changes, though code changes should definitely be considered.
On Sun, 2010-10-24 at 14:35 -0700, Mark Brown wrote:
On Sun, Oct 24, 2010 at 08:18:59AM -0500, Timur Tabi wrote:
On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos
The only problem I see with the above code, is when codec_drv->reg_word_size > sizeof (unsigned int) but that can't really happen in practice.
Plus if it did happen the rest of the code would fall over fairly badly since we've got that assumption embedded in the register read and write APIs.
I'm going to have to agree with Mark that this code is suspect. I understand everything you said, but it makes me nervous. Unless this code is in some kind of fast-path, I would prefer to see it rewritten to avoid any assumption about the sizes of the types involved.
I think the important thing here is that the code is clear - from a maintainability so long as it's clear how and why the code works things should be fine, otherwise we'll have people scratching their heads over it every time someone looks at the code which is going to be painful. This could be done with documentation as well as with code changes, though code changes should definitely be considered.
Yes that makes sense. The reason why I did this in the first place was to make it work with 8/16-byte reads/writes without using branching. I will change this to explicitly dereference a u8/u16 pointer.
Thanks, Dimitrios
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- sound/soc/soc-core.c | 38 +++++++++++++++----------------------- 1 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 70d9a73..c77f2b5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3213,23 +3213,6 @@ int snd_soc_register_codec(struct device *dev, return -ENOMEM; }
- /* allocate CODEC register cache */ - if (codec_drv->reg_cache_size && codec_drv->reg_word_size) { - - if (codec_drv->reg_cache_default) - codec->reg_cache = kmemdup(codec_drv->reg_cache_default, - codec_drv->reg_cache_size * codec_drv->reg_word_size, GFP_KERNEL); - else - codec->reg_cache = kzalloc(codec_drv->reg_cache_size * - codec_drv->reg_word_size, GFP_KERNEL); - - if (codec->reg_cache == NULL) { - kfree(codec->name); - kfree(codec); - return -ENOMEM; - } - } - codec->dev = dev; codec->driver = codec_drv; codec->bias_level = SND_SOC_BIAS_OFF; @@ -3238,6 +3221,16 @@ int snd_soc_register_codec(struct device *dev, INIT_LIST_HEAD(&codec->dapm_widgets); INIT_LIST_HEAD(&codec->dapm_paths);
+ /* allocate CODEC register cache */ + if (codec_drv->reg_cache_size && codec_drv->reg_word_size) { + ret = snd_soc_cache_init(codec); + if (ret < 0) { + dev_err(codec->dev, "Failed to set cache compression type: %d\n", + ret); + goto error_cache; + } + } + for (i = 0; i < num_dai; i++) { fixup_codec_formats(&dai_drv[i].playback); fixup_codec_formats(&dai_drv[i].capture); @@ -3247,7 +3240,7 @@ int snd_soc_register_codec(struct device *dev, if (num_dai) { ret = snd_soc_register_dais(dev, dai_drv, num_dai); if (ret < 0) - goto error; + goto error_dais; }
mutex_lock(&client_mutex); @@ -3258,12 +3251,12 @@ int snd_soc_register_codec(struct device *dev, pr_debug("Registered codec '%s'\n", codec->name); return 0;
-error: +error_dais: for (i--; i >= 0; i--) snd_soc_unregister_dai(dev);
- if (codec->reg_cache) - kfree(codec->reg_cache); + snd_soc_cache_deinit(codec); +error_cache: kfree(codec->name); kfree(codec); return ret; @@ -3297,8 +3290,7 @@ found:
pr_debug("Unregistered codec '%s'\n", codec->name);
- if (codec->reg_cache) - kfree(codec->reg_cache); + snd_soc_cache_deinit(codec); kfree(codec->name); kfree(codec); }
This patch adds support for LZO compression when storing the register cache. The initial register defaults cache is marked as __devinitconst and the only change required for a driver to use LZO compression is to set the compress_type member in codec->driver to SND_SOC_LZO_COMPRESSION.
Signed-off-by: Dimitris Papastamos dp@opensource.wolfsonmicro.com --- sound/soc/soc-cache.c | 379 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 379 insertions(+), 0 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index a8ec23a..ea9d80f 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -14,6 +14,8 @@ #include <linux/i2c.h> #include <linux/spi/spi.h> #include <sound/soc.h> +#include <linux/lzo.h> +#include <linux/bitmap.h>
static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, unsigned int reg) @@ -764,6 +766,375 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, } EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
+struct snd_soc_lzo_ctx { + void *wmem; + void *dst; + const void *src; + size_t src_len; + size_t dst_len; + size_t decompressed_size; + unsigned long *sync_bmp; + int sync_bmp_nbits; +}; + +#define LZO_BLOCK_NUM 8 +static int snd_soc_lzo_block_count(void) +{ + return LZO_BLOCK_NUM; +} + +static int snd_soc_lzo_prepare(struct snd_soc_lzo_ctx *lzo_ctx) +{ + lzo_ctx->wmem = kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL); + if (!lzo_ctx->wmem) + return -ENOMEM; + return 0; +} + +static int snd_soc_lzo_compress(struct snd_soc_lzo_ctx *lzo_ctx) +{ + size_t compress_size; + int ret; + + ret = lzo1x_1_compress(lzo_ctx->src, lzo_ctx->src_len, + lzo_ctx->dst, &compress_size, lzo_ctx->wmem); + if (ret != LZO_E_OK || compress_size > lzo_ctx->dst_len) + return -EINVAL; + lzo_ctx->dst_len = compress_size; + return 0; +} + +static int snd_soc_lzo_decompress(struct snd_soc_lzo_ctx *lzo_ctx) +{ + size_t dst_len; + int ret; + + dst_len = lzo_ctx->dst_len; + ret = lzo1x_decompress_safe(lzo_ctx->src, lzo_ctx->src_len, + lzo_ctx->dst, &dst_len); + if (ret != LZO_E_OK || dst_len != lzo_ctx->dst_len) + return -EINVAL; + return 0; +} + +static int snd_soc_compress_cache_block(struct snd_soc_codec *codec, + struct snd_soc_lzo_ctx *lzo_ctx) +{ + int ret; + + lzo_ctx->dst_len = lzo1x_worst_compress(PAGE_SIZE); + lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL); + if (!lzo_ctx->dst) { + lzo_ctx->dst_len = 0; + return -ENOMEM; + } + + ret = snd_soc_lzo_compress(lzo_ctx); + if (ret < 0) + return ret; + return 0; +} + +static int snd_soc_decompress_cache_block(struct snd_soc_codec *codec, + struct snd_soc_lzo_ctx *lzo_ctx) +{ + int ret; + + lzo_ctx->dst_len = lzo_ctx->decompressed_size; + lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL); + if (!lzo_ctx->dst) { + lzo_ctx->dst_len = 0; + return -ENOMEM; + } + + ret = snd_soc_lzo_decompress(lzo_ctx); + if (ret < 0) + return ret; + return 0; +} + +static inline int snd_soc_get_lzo_blkindex(struct snd_soc_codec *codec, + unsigned int reg) +{ + struct snd_soc_codec_driver *codec_drv; + size_t reg_size; + + codec_drv = codec->driver; + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + return (reg * codec_drv->reg_word_size) / + DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count()); +} + +static inline int snd_soc_get_lzo_blkpos(struct snd_soc_codec *codec, + unsigned int reg) +{ + struct snd_soc_codec_driver *codec_drv; + size_t reg_size; + + codec_drv = codec->driver; + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + return reg % (DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count()) / + codec_drv->reg_word_size); +} + +static inline int snd_soc_get_lzo_blksize(struct snd_soc_codec *codec) +{ + struct snd_soc_codec_driver *codec_drv; + size_t reg_size; + + codec_drv = codec->driver; + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + return DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count()); +} + +static int snd_soc_cache_lzo_sync(struct snd_soc_codec *codec) +{ + struct snd_soc_lzo_ctx **lzo_blocks; + unsigned int val; + int n; + + lzo_blocks = codec->reg_cache; + for_each_set_bit(n, lzo_blocks[0]->sync_bmp, lzo_blocks[0]->sync_bmp_nbits) { + snd_soc_cache_read(codec, n, &val); + snd_soc_write(codec, n, val); + } + + return 0; +} + +static int snd_soc_cache_lzo_write(struct snd_soc_codec *codec, + unsigned int reg, unsigned int value) +{ + struct snd_soc_lzo_ctx *lzo_block, **lzo_blocks; + u8 *cache; + int ret, blkindex, blkpos; + size_t blksize, tmp_dst_len; + void *tmp_dst; + + /* index of the compressed lzo block */ + blkindex = snd_soc_get_lzo_blkindex(codec, reg); + /* register index within the decompressed block */ + blkpos = snd_soc_get_lzo_blkpos(codec, reg); + blkpos *= codec->driver->reg_word_size; + /* size of the compressed block */ + blksize = snd_soc_get_lzo_blksize(codec); + lzo_blocks = codec->reg_cache; + lzo_block = lzo_blocks[blkindex]; + + /* save the pointer and length of the compressed block */ + tmp_dst = lzo_block->dst; + tmp_dst_len = lzo_block->dst_len; + + /* prepare the source to be the compressed block */ + lzo_block->src = lzo_block->dst; + lzo_block->src_len = lzo_block->dst_len; + + /* decompress the block */ + ret = snd_soc_decompress_cache_block(codec, lzo_block); + if (ret < 0) { + kfree(lzo_block->dst); + goto out; + } + + cache = lzo_block->dst; + /* bail out early, no change in value */ + if (!memcmp(&cache[blkpos], &value, + codec->driver->reg_word_size)) { + kfree(lzo_block->dst); + goto out; + } + /* set the new register value */ + memcpy(&cache[blkpos], &value, codec->driver->reg_word_size); + + /* prepare the source to be the decompressed block */ + lzo_block->src = lzo_block->dst; + lzo_block->src_len = lzo_block->dst_len; + + /* compress the block */ + ret = snd_soc_compress_cache_block(codec, lzo_block); + if (ret < 0) { + kfree(lzo_block->dst); + kfree(lzo_block->src); + goto out; + } + + /* set the bit so we know we have to sync this register */ + set_bit(reg, lzo_block->sync_bmp); + kfree(tmp_dst); + kfree(lzo_block->src); + return 0; +out: + lzo_block->dst = tmp_dst; + lzo_block->dst_len = tmp_dst_len; + return ret; +} + +static int snd_soc_cache_lzo_read(struct snd_soc_codec *codec, + unsigned int reg, unsigned int *value) +{ + struct snd_soc_lzo_ctx *lzo_block, **lzo_blocks; + u8 *cache; + int ret, blkindex, blkpos; + size_t blksize, tmp_dst_len; + void *tmp_dst; + + *value = 0; + /* index of the compressed lzo block */ + blkindex = snd_soc_get_lzo_blkindex(codec, reg); + /* register index within the decompressed block */ + blkpos = snd_soc_get_lzo_blkpos(codec, reg); + blkpos *= codec->driver->reg_word_size; + /* size of the compressed block */ + blksize = snd_soc_get_lzo_blksize(codec); + lzo_blocks = codec->reg_cache; + lzo_block = lzo_blocks[blkindex]; + + /* save the pointer and length of the compressed block */ + tmp_dst = lzo_block->dst; + tmp_dst_len = lzo_block->dst_len; + + /* prepare the source to be the compressed block */ + lzo_block->src = lzo_block->dst; + lzo_block->src_len = lzo_block->dst_len; + + /* decompress the block */ + ret = snd_soc_decompress_cache_block(codec, lzo_block); + if (ret >= 0) { + cache = lzo_block->dst; + memcpy(value, &cache[blkpos], + codec->driver->reg_word_size); + } + + kfree(lzo_block->dst); + /* restore the pointer and length of the compressed block */ + lzo_block->dst = tmp_dst; + lzo_block->dst_len = tmp_dst_len; + return 0; +} + +static int snd_soc_cache_lzo_deinit(struct snd_soc_codec *codec) +{ + struct snd_soc_lzo_ctx **lzo_blocks; + int i, blkcount; + + lzo_blocks = codec->reg_cache; + if (!lzo_blocks) + return -EINVAL; + + blkcount = snd_soc_lzo_block_count(); + if (lzo_blocks[0]) + kfree(lzo_blocks[0]->sync_bmp); + for (i = 0; i < blkcount; ++i) { + if (lzo_blocks[i]) { + kfree(lzo_blocks[i]->wmem); + kfree(lzo_blocks[i]->dst); + } + kfree(lzo_blocks[i]); + lzo_blocks[i] = NULL; + } + kfree(lzo_blocks); + codec->reg_cache = NULL; + return 0; +} + +static int snd_soc_cache_lzo_init(struct snd_soc_codec *codec) +{ + struct snd_soc_lzo_ctx **lzo_blocks; + size_t reg_size; + struct snd_soc_codec_driver *codec_drv; + int ret, tofree, i, blksize, blkcount; + const char *p, *end; + unsigned long *sync_bmp; + + ret = 0; + codec_drv = codec->driver; + reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size; + + /* + * If we have not been given a default register cache + * then allocate a dummy zero-ed out region, compress it + * and remember to free it afterwards. + */ + tofree = 0; + if (!codec_drv->reg_cache_default) + tofree = 1; + + if (!codec_drv->reg_cache_default) { + codec_drv->reg_cache_default = kzalloc(reg_size, + GFP_KERNEL); + if (!codec_drv->reg_cache_default) + return -ENOMEM; + } + + blkcount = snd_soc_lzo_block_count(); + codec->reg_cache = kzalloc(blkcount * sizeof(*lzo_blocks), + GFP_KERNEL); + if (!codec->reg_cache) { + ret = -ENOMEM; + goto err_tofree; + } + lzo_blocks = codec->reg_cache; + + /* + * allocate a bitmap to be used when syncing the cache with + * the hardware. Each time a register is modified, the corresponding + * bit is set in the bitmap, so we know that we have to sync + * that register. + */ + sync_bmp = kmalloc(BITS_TO_LONGS(reg_size) * sizeof(unsigned long), + GFP_KERNEL); + if (!sync_bmp) { + ret = -ENOMEM; + goto err; + } + bitmap_zero(sync_bmp, reg_size); + + /* allocate the lzo blocks and initialize them */ + for (i = 0; i < blkcount; ++i) { + lzo_blocks[i] = kzalloc(sizeof **lzo_blocks, + GFP_KERNEL); + if (!lzo_blocks[i]) { + kfree(sync_bmp); + ret = -ENOMEM; + goto err; + } + lzo_blocks[i]->sync_bmp = sync_bmp; + lzo_blocks[i]->sync_bmp_nbits = reg_size; + /* alloc the working space for the compressed block */ + ret = snd_soc_lzo_prepare(lzo_blocks[i]); + if (ret < 0) + goto err; + } + + blksize = snd_soc_get_lzo_blksize(codec); + p = codec_drv->reg_cache_default; + end = codec_drv->reg_cache_default + reg_size; + /* compress the register map and fill the lzo blocks */ + for (i = 0; i < blkcount; ++i, p += blksize) { + lzo_blocks[i]->src = p; + if (p + blksize > end) + lzo_blocks[i]->src_len = end - p; + else + lzo_blocks[i]->src_len = blksize; + ret = snd_soc_compress_cache_block(codec, + lzo_blocks[i]); + if (ret < 0) + goto err; + lzo_blocks[i]->decompressed_size = + lzo_blocks[i]->src_len; + } + + if (tofree) + kfree(codec_drv->reg_cache_default); + return 0; +err: + snd_soc_cache_deinit(codec); +err_tofree: + if (tofree) + kfree(codec_drv->reg_cache_default); + return ret; +} + static int snd_soc_cache_default_sync(struct snd_soc_codec *codec) { const u8 *cache; @@ -842,6 +1213,14 @@ static const struct snd_soc_cache_ops cache_types[] = { .read = snd_soc_cache_default_read, .write = snd_soc_cache_default_write, .sync = snd_soc_cache_default_sync + }, + { + .id = SND_SOC_LZO_COMPRESSION, + .init = snd_soc_cache_lzo_init, + .deinit = snd_soc_cache_lzo_deinit, + .read = snd_soc_cache_lzo_read, + .write = snd_soc_cache_lzo_write, + .sync = snd_soc_cache_lzo_sync } };
On Fri, Oct 22, 2010 at 03:28:22PM +0100, Dimitris Papastamos wrote:
This patch adds support for LZO compression when storing the register cache. The initial register defaults cache is marked as __devinitconst and the only change required for a driver to use LZO compression is
This looks good, though the changelog could use a bit more discussion as to the tradeoffs involved - clearly we're trading CPU consumption for memory consumption here, but things like numbers about the sorts of compression you can get and the amount of CPU time burned relative to the actual I/O operations would help people understand when and how to use this.
Actually, now that I think about it debug data either via debugfs or via dev_dbg() showing the before and after memory sizes would be very useful for people trying to decide if their register map compresses down well enough to really benefit from compression.
It looks like this patch also needs to add selects for LZO_COMPRESS and LZO_DECOMPRESS to Kconfig, otherwise we'll fail to build if nothing else has enabled them.
to set the compress_type member in codec->driver to SND_SOC_LZO_COMPRESSION.
It would be good if machine drivers were able to override this,
+static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
struct snd_soc_lzo_ctx *lzo_ctx)+{
This is all rather assuming that LZO is the only compression method we can use? It doesn't really matter, though, as this is all internal to the cache code so we can deal with adding new compression types as and when we want them.
On Fri, 2010-10-22 at 09:18 -0700, Mark Brown wrote:
On Fri, Oct 22, 2010 at 03:28:22PM +0100, Dimitris Papastamos wrote:
This patch adds support for LZO compression when storing the register cache. The initial register defaults cache is marked as __devinitconst and the only change required for a driver to use LZO compression is
This looks good, though the changelog could use a bit more discussion as to the tradeoffs involved - clearly we're trading CPU consumption for memory consumption here, but things like numbers about the sorts of compression you can get and the amount of CPU time burned relative to the actual I/O operations would help people understand when and how to use this.
Yes, that makes sense.
Actually, now that I think about it debug data either via debugfs or via dev_dbg() showing the before and after memory sizes would be very useful for people trying to decide if their register map compresses down well enough to really benefit from compression.
I'll send an incremental patch for that.
It looks like this patch also needs to add selects for LZO_COMPRESS and LZO_DECOMPRESS to Kconfig, otherwise we'll fail to build if nothing else has enabled them.
to set the compress_type member in codec->driver to SND_SOC_LZO_COMPRESSION.
It would be good if machine drivers were able to override this,
I'll think about this to see how it can be overriden.
+static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
struct snd_soc_lzo_ctx *lzo_ctx)+{
This is all rather assuming that LZO is the only compression method we can use? It doesn't really matter, though, as this is all internal to the cache code so we can deal with adding new compression types as and when we want them.
The function naming is wrong. This function is a helper function for the LZO compression type. I've prefixed this function with LZO to avoid misunderstanding. Ideally this would live in a separate file.
Thanks, Dimitrios
On Sat, Oct 23, 2010 at 07:28:01PM +0100, Dimitris Papastamos wrote:
The function naming is wrong. This function is a helper function for the LZO compression type. I've prefixed this function with LZO to avoid misunderstanding. Ideally this would live in a separate file.
Go ahead and split out if it seems useful.
participants (3)
-
Dimitris Papastamos -
Mark Brown -
Timur Tabi