[PATCH] ASoC: rt5682: Use a maple tree based register cache
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,
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,
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?
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.
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;
participants (2)
-
Mark Brown -
Pierre-Louis Bossart