[PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Mark Brown]

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

Re: [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Liam Girdwood]

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>

Re: [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Dimitris Papastamos]

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

Re: [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Mark Brown]

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.

Re: [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Dimitris Papastamos]

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

Re: [PATCH] ASoC: Suppress restore of default register values for rbtree cache sync

[Mark Brown]

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.