[alsa-devel] [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync
Currently the rbtree code will write out the entire register map when doing a cache sync which is wasteful and will slow things down. Check to see if the value we're about to write is the default and don't bother restoring it if it is, either the value will have been retained or the device will have been reset and holds the value already.
We should really store the defaults in the nodes but this resolves the immediate issue.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-cache.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d8ce34c..d82ab42 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -607,7 +607,7 @@ static int snd_soc_rbtree_cache_sync(struct snd_soc_codec *codec) struct rb_node *node; struct snd_soc_rbtree_node *rbnode; unsigned int regtmp; - unsigned int val; + unsigned int val, def; int ret; int i;
@@ -619,6 +619,11 @@ static int snd_soc_rbtree_cache_sync(struct snd_soc_codec *codec) WARN_ON(codec->writable_register && codec->writable_register(codec, regtmp)); val = snd_soc_rbtree_get_register(rbnode, i); + def = snd_soc_get_cache_val(codec->reg_def_copy, i, + rbnode->word_size); + if (val == def) + continue; + codec->cache_bypass = 1; ret = snd_soc_write(codec, regtmp, val); codec->cache_bypass = 0;
On 03/06/11 16:37, Mark Brown wrote:
Currently the rbtree code will write out the entire register map when doing a cache sync which is wasteful and will slow things down. Check to see if the value we're about to write is the default and don't bother restoring it if it is, either the value will have been retained or the device will have been reset and holds the value already.
We should really store the defaults in the nodes but this resolves the immediate issue.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
Acked-by: Liam Girdwood lrg@ti.com
On Fri, Jun 03, 2011 at 04:37:15PM +0100, Mark Brown wrote:
Currently the rbtree code will write out the entire register map when doing a cache sync which is wasteful and will slow things down. Check to see if the value we're about to write is the default and don't bother restoring it if it is, either the value will have been retained or the device will have been reset and holds the value already.
We should really store the defaults in the nodes but this resolves the immediate issue.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
sound/soc/soc-cache.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d8ce34c..d82ab42 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -607,7 +607,7 @@ static int snd_soc_rbtree_cache_sync(struct snd_soc_codec *codec) struct rb_node *node; struct snd_soc_rbtree_node *rbnode; unsigned int regtmp;
- unsigned int val;
- unsigned int val, def; int ret; int i;
@@ -619,6 +619,11 @@ static int snd_soc_rbtree_cache_sync(struct snd_soc_codec *codec) WARN_ON(codec->writable_register && codec->writable_register(codec, regtmp)); val = snd_soc_rbtree_get_register(rbnode, i);
def = snd_soc_get_cache_val(codec->reg_def_copy, i,rbnode->word_size);if (val == def)continue;codec->cache_bypass = 1; ret = snd_soc_write(codec, regtmp, val); codec->cache_bypass = 0;-- 1.7.5.3
There should be a check for reg_def_copy being NULL, the soc-cache code is also built around the idea that the driver might not provide a register defaults cache at all so this change needs to cope with that.
Also I'd expect the reg_def_copy cache to be freed sooner rather than while unregistering the codec, this is not happening at the moment, which is wasting significant space. This is the case because it is only used during initialization.
Note that the only reason we allocate reg_def_copy is to avoid to fiddle with a cache that might be marked as __devinitconst which is also not happening for any of the users at the moment.
Thanks, Dimitris
On Mon, Jun 06, 2011 at 10:18:07AM +0100, Dimitris Papastamos wrote:
There should be a check for reg_def_copy being NULL, the soc-cache code is also built around the idea that the driver might not provide a register defaults cache at all so this change needs to cope with that.
Hrm, at this point I'm rather inclined to say that if you're using a fancy cache method you should be providing defaults.
Also I'd expect the reg_def_copy cache to be freed sooner rather than while unregistering the codec, this is not happening at the moment, which is wasting significant space. This is the case because it is only used during initialization.
That's the bit I was saying at the bottom of the commit message about putting it into the node itself; since we're currently never freeing it we can use it but we should really be copying it into the tree. The memory wasted is a *much* less severe problem than the I/O bandwidth consumed rewriting the entire register map on every sync.
On Mon, Jun 06, 2011 at 10:26:12AM +0100, Mark Brown wrote:
On Mon, Jun 06, 2011 at 10:18:07AM +0100, Dimitris Papastamos wrote:
There should be a check for reg_def_copy being NULL, the soc-cache code is also built around the idea that the driver might not provide a register defaults cache at all so this change needs to cope with that.
Hrm, at this point I'm rather inclined to say that if you're using a fancy cache method you should be providing defaults.
Also I'd expect the reg_def_copy cache to be freed sooner rather than while unregistering the codec, this is not happening at the moment, which is wasting significant space. This is the case because it is only used during initialization.
That's the bit I was saying at the bottom of the commit message about putting it into the node itself; since we're currently never freeing it we can use it but we should really be copying it into the tree. The memory wasted is a *much* less severe problem than the I/O bandwidth consumed rewriting the entire register map on every sync.
Yes, in the previous version of the rbtree code, it was saving it in the node itself. I was planning to have a separate block with the default values for the block based version but haven't yet got around to that.
Thanks, Dimitris
On Mon, Jun 06, 2011 at 10:29:49AM +0100, Dimitris Papastamos wrote:
Yes, in the previous version of the rbtree code, it was saving it in the node itself. I was planning to have a separate block with the default values for the block based version but haven't yet got around to that.
Oh, good - I was wondering how I'd missed this when doing initial testing on rbtree code. If you're removing features like this you really need to call it out in the changelog, this is quite a nasty regression as we can easily end up doing a couple of orders of magnitude more I/O on a slow bus than we were expecting to.
participants (3)
-
Dimitris Papastamos -
Liam Girdwood -
Mark Brown