[PATCH] ASoC: rt5682: Use a maple tree based register cache

[PATCH] ASoC: rt5682: Use a maple tree based register cache

[Mark Brown]

regmap has introduced a maple tree based register cache which makes use of
this more advanced data structure which has been added to the kernel
recently. Maple trees are much flatter than rbtrees, meaning that they do
not grow to such depths when the register map is sparse which makes access
a bit more efficient. The maple tree cache type is still a bit of a work
in progress but should be effective for some devices already.

RT5682 seems like a good candidate for maple tree. It only supports single
register read/write operations so will gain minimal benefit from storing
the register data in device native format like rbtree does (none for
SoundWire) and has some sparsity in the register map which is a good fit
for maple tree.

Convert to use maple tree. There should be little if any visible difference
at runtime.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/rt5682-sdw.c | 2 +-
 sound/soc/codecs/rt5682s.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
index 5f80a5d59b65..fb7951f11c92 100644
--- a/sound/soc/codecs/rt5682-sdw.c
+++ b/sound/soc/codecs/rt5682-sdw.c
@@ -79,7 +79,7 @@ static const struct regmap_config rt5682_sdw_indirect_regmap = {
 	.max_register = RT5682_I2C_MODE,
 	.volatile_reg = rt5682_volatile_register,
 	.readable_reg = rt5682_readable_register,
-	.cache_type = REGCACHE_RBTREE,
+	.cache_type = REGCACHE_MAPLE,
 	.reg_defaults = rt5682_reg,
 	.num_reg_defaults = RT5682_REG_NUM,
 	.use_single_read = true,
diff --git a/sound/soc/codecs/rt5682s.c b/sound/soc/codecs/rt5682s.c
index 9c34dca58f54..36102fa2b806 100644
--- a/sound/soc/codecs/rt5682s.c
+++ b/sound/soc/codecs/rt5682s.c
@@ -3046,7 +3046,7 @@ static const struct regmap_config rt5682s_regmap = {
 	.max_register = RT5682S_MAX_REG,
 	.volatile_reg = rt5682s_volatile_register,
 	.readable_reg = rt5682s_readable_register,
-	.cache_type = REGCACHE_RBTREE,
+	.cache_type = REGCACHE_MAPLE,
 	.reg_defaults = rt5682s_reg,
 	.num_reg_defaults = ARRAY_SIZE(rt5682s_reg),
 	.use_single_read = true,

---
base-commit: 4a670ac3e75e517c96cbd01ef870dbd598c3ce71
change-id: 20230419-asoc-rt5682-maple-7da060991ca4

Best regards,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] ASoC: rt5682: Use a maple tree based register cache

[Pierre-Louis Bossart]

On 4/25/23 12:22, Mark Brown wrote:
> regmap has introduced a maple tree based register cache which makes use of
> this more advanced data structure which has been added to the kernel
> recently. Maple trees are much flatter than rbtrees, meaning that they do
> not grow to such depths when the register map is sparse which makes access
> a bit more efficient. The maple tree cache type is still a bit of a work
> in progress but should be effective for some devices already.
> 
> RT5682 seems like a good candidate for maple tree. It only supports single
> register read/write operations so will gain minimal benefit from storing
> the register data in device native format like rbtree does (none for
> SoundWire) and has some sparsity in the register map which is a good fit
> for maple tree.
> 
> Convert to use maple tree. There should be little if any visible difference
> at runtime.

Wondering if this is the root cause of the regression we're seeing in
[1] on a Chromebook with rt5682 in SoundWire mode?

I don't see any other changes to this codec driver and the first problem
detected seemed to happen when we did an upstream merge last week.
Unfortunately the last merge was on April 24 (sof-dev-rebase-20230424)
which is just the day before this commit was added...

[1] https://github.com/thesofproject/linux/issues/4371

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  sound/soc/codecs/rt5682-sdw.c | 2 +-
>  sound/soc/codecs/rt5682s.c    | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
> index 5f80a5d59b65..fb7951f11c92 100644
> --- a/sound/soc/codecs/rt5682-sdw.c
> +++ b/sound/soc/codecs/rt5682-sdw.c
> @@ -79,7 +79,7 @@ static const struct regmap_config rt5682_sdw_indirect_regmap = {
>  	.max_register = RT5682_I2C_MODE,
>  	.volatile_reg = rt5682_volatile_register,
>  	.readable_reg = rt5682_readable_register,
> -	.cache_type = REGCACHE_RBTREE,
> +	.cache_type = REGCACHE_MAPLE,
>  	.reg_defaults = rt5682_reg,
>  	.num_reg_defaults = RT5682_REG_NUM,
>  	.use_single_read = true,
> diff --git a/sound/soc/codecs/rt5682s.c b/sound/soc/codecs/rt5682s.c
> index 9c34dca58f54..36102fa2b806 100644
> --- a/sound/soc/codecs/rt5682s.c
> +++ b/sound/soc/codecs/rt5682s.c
> @@ -3046,7 +3046,7 @@ static const struct regmap_config rt5682s_regmap = {
>  	.max_register = RT5682S_MAX_REG,
>  	.volatile_reg = rt5682s_volatile_register,
>  	.readable_reg = rt5682s_readable_register,
> -	.cache_type = REGCACHE_RBTREE,
> +	.cache_type = REGCACHE_MAPLE,
>  	.reg_defaults = rt5682s_reg,
>  	.num_reg_defaults = ARRAY_SIZE(rt5682s_reg),
>  	.use_single_read = true,
> 
> ---
> base-commit: 4a670ac3e75e517c96cbd01ef870dbd598c3ce71
> change-id: 20230419-asoc-rt5682-maple-7da060991ca4
> 
> Best regards,

Re: [PATCH] ASoC: rt5682: Use a maple tree based register cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On Tue, May 23, 2023 at 02:24:53PM -0500, Pierre-Louis Bossart wrote:

> Wondering if this is the root cause of the regression we're seeing in
> [1] on a Chromebook with rt5682 in SoundWire mode?

> I don't see any other changes to this codec driver and the first problem
> detected seemed to happen when we did an upstream merge last week.
> Unfortunately the last merge was on April 24 (sof-dev-rebase-20230424)
> which is just the day before this commit was added...

Try a revert?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: rt5682: Use a maple tree based register cache

[Pierre-Louis Bossart]

On 5/23/23 14:28, Mark Brown wrote:
> On Tue, May 23, 2023 at 02:24:53PM -0500, Pierre-Louis Bossart wrote:
> 
>> Wondering if this is the root cause of the regression we're seeing in
>> [1] on a Chromebook with rt5682 in SoundWire mode?
> 
>> I don't see any other changes to this codec driver and the first problem
>> detected seemed to happen when we did an upstream merge last week.
>> Unfortunately the last merge was on April 24 (sof-dev-rebase-20230424)
>> which is just the day before this commit was added...
> 
> Try a revert?

I can try, unfortunately that device is not directly testable with a
simple PR test so it'll take time.

I was just hoping that someone smarter than me had an explanation on the
locking issue. We only use interrupt threads and workqueues, not sure
why sleeping is an issue.

Re: [PATCH] ASoC: rt5682: Use a maple tree based register cache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

On Tue, May 23, 2023 at 02:24:53PM -0500, Pierre-Louis Bossart wrote:

> I don't see any other changes to this codec driver and the first problem
> detected seemed to happen when we did an upstream merge last week.
> Unfortunately the last merge was on April 24 (sof-dev-rebase-20230424)
> which is just the day before this commit was added...

Try this:

From 5953e9de359674ff2d95fe4c241bc7880d6d0d82 Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@kernel.org>
Date: Tue, 23 May 2023 20:40:22 +0100
Subject: [PATCH] regmap: maple: Drop the RCU read lock while syncing registers

Unfortunately the maple tree requires us to explicitly lock it so we need
to take the RCU read lock while iterating. When syncing this means that we
end up trying to write out register values while holding the RCU read lock
which triggers lockdep issues since that is an atomic context but most
buses can't be used in atomic context. Pause the iteration and drop the
lock for each register we check to avoid this.

Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regcache-maple.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c
index 2d2d5d7ee447..14f6f49af097 100644
--- a/drivers/base/regmap/regcache-maple.c
+++ b/drivers/base/regmap/regcache-maple.c
@@ -203,15 +203,18 @@ static int regcache_maple_sync(struct regmap *map, unsigned int min,
 
 	mas_for_each(&mas, entry, max) {
 		for (r = max(mas.index, lmin); r <= min(mas.last, lmax); r++) {
+			mas_pause(&mas);
+			rcu_read_unlock();
 			ret = regcache_sync_val(map, r, entry[r - mas.index]);
 			if (ret != 0)
 				goto out;
+			rcu_read_lock();
 		}
 	}
 
-out:
 	rcu_read_unlock();
 
+out:
 	map->cache_bypass = false;
 
 	return ret;
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]