[PATCH] ALSA: hda: Use maple tree register cache

[PATCH] ALSA: hda: Use maple tree register cache

[Mark Brown]

HDA can only support single register read and write operations so does not
benefit from block writes. This means it gets no benefit from using the
rbtree register cache over the maple tree register cache so convert it to
use maple trees instead, it is more modern.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/hda/hdac_regmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/hda/hdac_regmap.c b/sound/hda/hdac_regmap.c
index fe3587547cfe..2caa1f9b858e 100644
--- a/sound/hda/hdac_regmap.c
+++ b/sound/hda/hdac_regmap.c
@@ -358,7 +358,7 @@ static const struct regmap_config hda_regmap_cfg = {
 	.writeable_reg = hda_writeable_reg,
 	.readable_reg = hda_readable_reg,
 	.volatile_reg = hda_volatile_reg,
-	.cache_type = REGCACHE_RBTREE,
+	.cache_type = REGCACHE_MAPLE,
 	.reg_read = hda_reg_read,
 	.reg_write = hda_reg_write,
 	.use_single_read = true,

---
base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
change-id: 20230609-alsa-hda-maple-02f75ebb067e

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

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Sat, 10 Jun 2023 16:26:37 +0200,
Mark Brown wrote:
> 
> HDA can only support single register read and write operations so does not
> benefit from block writes. This means it gets no benefit from using the
> rbtree register cache over the maple tree register cache so convert it to
> use maple trees instead, it is more modern.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Thanks, applied to for-next branch.


Takashi

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Sun, 11 Jun 2023 09:37:21 +0200,
Takashi Iwai wrote:
> 
> On Sat, 10 Jun 2023 16:26:37 +0200,
> Mark Brown wrote:
> > 
> > HDA can only support single register read and write operations so does not
> > benefit from block writes. This means it gets no benefit from using the
> > rbtree register cache over the maple tree register cache so convert it to
> > use maple trees instead, it is more modern.
> > 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> 
> Thanks, applied to for-next branch.

Now I noticed errors like

  snd_hda_codec_realtek hdaudioC0D0: Unable to sync register 0x2f0009. -5

and it turned out that the error comes from this patch.

This is an error from regache_sync_val(), and it indicates that the
synced register is write-only; regcache_maple_sync() tries to sync all
cached values no matter whether it's writable or not, then hitting
this.

I'll submit a fix patch.


thanks,

Takashi

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Mark Brown]

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

On Tue, Jun 13, 2023 at 09:36:34AM +0200, Takashi Iwai wrote:

> This is an error from regache_sync_val(), and it indicates that the
> synced register is write-only; regcache_maple_sync() tries to sync all
> cached values no matter whether it's writable or not, then hitting
> this.

BTW I was just looking at reg_raw_update_once() and I can't figure out
why it's trying to do what it's doing - it does a read to check if it's
seen the register before and then does an _update_bits() if the register
hasn't been cached yet, apparently trying suppress duplicate writes but
possibly deliberately discarding changes to multiple bitfields in the
same register.  That's not what the non-regmap path does, it'll only
discard noop changes to the same bitfield.

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

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Tue, 13 Jun 2023 15:59:14 +0200,
Mark Brown wrote:
> 
> On Tue, Jun 13, 2023 at 09:36:34AM +0200, Takashi Iwai wrote:
> 
> > This is an error from regache_sync_val(), and it indicates that the
> > synced register is write-only; regcache_maple_sync() tries to sync all
> > cached values no matter whether it's writable or not, then hitting
> > this.
> 
> BTW I was just looking at reg_raw_update_once() and I can't figure out
> why it's trying to do what it's doing - it does a read to check if it's
> seen the register before and then does an _update_bits() if the register
> hasn't been cached yet, apparently trying suppress duplicate writes but
> possibly deliberately discarding changes to multiple bitfields in the
> same register.  That's not what the non-regmap path does, it'll only
> discard noop changes to the same bitfield.

Yes, it's a quite hackish way of optimization of the initialization.

Since HD-audio codec has no known default values unlike normal codecs,
it needs to initialize itself only at the first access, and this
helper does it.


Takashi

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Mark Brown]

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

On Tue, Jun 13, 2023 at 04:24:28PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > BTW I was just looking at reg_raw_update_once() and I can't figure out
> > why it's trying to do what it's doing - it does a read to check if it's
> > seen the register before and then does an _update_bits() if the register
> > hasn't been cached yet, apparently trying suppress duplicate writes but
> > possibly deliberately discarding changes to multiple bitfields in the
> > same register.  That's not what the non-regmap path does, it'll only
> > discard noop changes to the same bitfield.

> Yes, it's a quite hackish way of optimization of the initialization.

> Since HD-audio codec has no known default values unlike normal codecs,
> it needs to initialize itself only at the first access, and this
> helper does it.

Ah, if it's just suppressing the write the code should just be removed.
regmap_update_bits() already suppresses noop writes so unless we might
write a different value to the register later the effect will be the
same.  I can send a patch.

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

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Tue, 13 Jun 2023 17:49:41 +0200,
Mark Brown wrote:
> 
> On Tue, Jun 13, 2023 at 04:24:28PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > BTW I was just looking at reg_raw_update_once() and I can't figure out
> > > why it's trying to do what it's doing - it does a read to check if it's
> > > seen the register before and then does an _update_bits() if the register
> > > hasn't been cached yet, apparently trying suppress duplicate writes but
> > > possibly deliberately discarding changes to multiple bitfields in the
> > > same register.  That's not what the non-regmap path does, it'll only
> > > discard noop changes to the same bitfield.
> 
> > Yes, it's a quite hackish way of optimization of the initialization.
> 
> > Since HD-audio codec has no known default values unlike normal codecs,
> > it needs to initialize itself only at the first access, and this
> > helper does it.
> 
> Ah, if it's just suppressing the write the code should just be removed.
> regmap_update_bits() already suppresses noop writes so unless we might
> write a different value to the register later the effect will be the
> same.  I can send a patch.

Oh, I'm afraid that we're seeing different things.  The code there is
rather to *set* some initial value for each amp register (but only
once), and it's not about optimization for writing a same value
again.

That is, the function helps to set an initial (mute) value on each amp
when the driver parses the topology and finds an amp.  But if the
driver already has parsed this amp beforehand by other paths, it skips
the initialization, as the other path may have already unmuted the
amp.

Or I might have misunderstood what you mean about _update_bits()...


thanks,

Takashi

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Mark Brown]

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

On Tue, Jun 13, 2023 at 06:15:12PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Tue, Jun 13, 2023 at 04:24:28PM +0200, Takashi Iwai wrote:

> > > Since HD-audio codec has no known default values unlike normal codecs,
> > > it needs to initialize itself only at the first access, and this
> > > helper does it.

> > Ah, if it's just suppressing the write the code should just be removed.
> > regmap_update_bits() already suppresses noop writes so unless we might
> > write a different value to the register later the effect will be the
> > same.  I can send a patch.

> Oh, I'm afraid that we're seeing different things.  The code there is
> rather to *set* some initial value for each amp register (but only
> once), and it's not about optimization for writing a same value
> again.

> That is, the function helps to set an initial (mute) value on each amp
> when the driver parses the topology and finds an amp.  But if the
> driver already has parsed this amp beforehand by other paths, it skips
> the initialization, as the other path may have already unmuted the
> amp.

> Or I might have misunderstood what you mean about _update_bits()...

So it is possible that we might set two distinct values during setup
then and we're doing this intentionally?  It's not obvious that this
might happen.  A comment wouldn't hurt, and a big part of this is
confusing is that in the non-regmap case all we're doing is suppressing
duplicate writes, in that path it's just checking for changes in the
register value.

None of this is what the non-regmap path does, it just suppresses noop
writes to the hardware.

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

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Tue, 13 Jun 2023 18:41:15 +0200,
Mark Brown wrote:
> 
> On Tue, Jun 13, 2023 at 06:15:12PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Tue, Jun 13, 2023 at 04:24:28PM +0200, Takashi Iwai wrote:
> 
> > > > Since HD-audio codec has no known default values unlike normal codecs,
> > > > it needs to initialize itself only at the first access, and this
> > > > helper does it.
> 
> > > Ah, if it's just suppressing the write the code should just be removed.
> > > regmap_update_bits() already suppresses noop writes so unless we might
> > > write a different value to the register later the effect will be the
> > > same.  I can send a patch.
> 
> > Oh, I'm afraid that we're seeing different things.  The code there is
> > rather to *set* some initial value for each amp register (but only
> > once), and it's not about optimization for writing a same value
> > again.
> 
> > That is, the function helps to set an initial (mute) value on each amp
> > when the driver parses the topology and finds an amp.  But if the
> > driver already has parsed this amp beforehand by other paths, it skips
> > the initialization, as the other path may have already unmuted the
> > amp.
> 
> > Or I might have misunderstood what you mean about _update_bits()...
> 
> So it is possible that we might set two distinct values during setup
> then and we're doing this intentionally?  It's not obvious that this
> might happen.  A comment wouldn't hurt, and a big part of this is
> confusing is that in the non-regmap case all we're doing is suppressing
> duplicate writes, in that path it's just checking for changes in the
> register value.
> 
> None of this is what the non-regmap path does, it just suppresses noop
> writes to the hardware.

Actually, many of HD-audio codec driver code heavily relies on the
regmap, more or less mandatory.  The snd_hda_codec_amp_init() is one
of such.  You may write a codec driver without the regmap, but some
helpers won't work as expected.


Takashi

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Mark Brown]

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

On Tue, Jun 13, 2023 at 07:05:21PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > > Oh, I'm afraid that we're seeing different things.  The code there is
> > > rather to *set* some initial value for each amp register (but only
> > > once), and it's not about optimization for writing a same value
> > > again.

> > > That is, the function helps to set an initial (mute) value on each amp
> > > when the driver parses the topology and finds an amp.  But if the
> > > driver already has parsed this amp beforehand by other paths, it skips
> > > the initialization, as the other path may have already unmuted the
> > > amp.

> > So it is possible that we might set two distinct values during setup
> > then and we're doing this intentionally?  It's not obvious that this
> > might happen.  A comment wouldn't hurt, and a big part of this is
> > confusing is that in the non-regmap case all we're doing is suppressing
> > duplicate writes, in that path it's just checking for changes in the
> > register value.

> > None of this is what the non-regmap path does, it just suppresses noop
> > writes to the hardware.

> Actually, many of HD-audio codec driver code heavily relies on the
> regmap, more or less mandatory.  The snd_hda_codec_amp_init() is one
> of such.  You may write a codec driver without the regmap, but some
> helpers won't work as expected.

Sounds like it might be so thinly used it's becoming mandatory to have a
regmap in order to avoid gotchas like there might be with things getting
muted?

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

Re: [PATCH] ALSA: hda: Use maple tree register cache

[Takashi Iwai]

On Tue, 13 Jun 2023 19:29:19 +0200,
Mark Brown wrote:
> 
> On Tue, Jun 13, 2023 at 07:05:21PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > > Oh, I'm afraid that we're seeing different things.  The code there is
> > > > rather to *set* some initial value for each amp register (but only
> > > > once), and it's not about optimization for writing a same value
> > > > again.
> 
> > > > That is, the function helps to set an initial (mute) value on each amp
> > > > when the driver parses the topology and finds an amp.  But if the
> > > > driver already has parsed this amp beforehand by other paths, it skips
> > > > the initialization, as the other path may have already unmuted the
> > > > amp.
> 
> > > So it is possible that we might set two distinct values during setup
> > > then and we're doing this intentionally?  It's not obvious that this
> > > might happen.  A comment wouldn't hurt, and a big part of this is
> > > confusing is that in the non-regmap case all we're doing is suppressing
> > > duplicate writes, in that path it's just checking for changes in the
> > > register value.
> 
> > > None of this is what the non-regmap path does, it just suppresses noop
> > > writes to the hardware.
> 
> > Actually, many of HD-audio codec driver code heavily relies on the
> > regmap, more or less mandatory.  The snd_hda_codec_amp_init() is one
> > of such.  You may write a codec driver without the regmap, but some
> > helpers won't work as expected.
> 
> Sounds like it might be so thinly used it's becoming mandatory to have a
> regmap in order to avoid gotchas like there might be with things getting
> muted?

It's rather historical reasons.  The caching mechanism was already
present and mandatory from the beginning, but it was implemented in a
different way.  Later on, it was translated to the regmap.  Meanwhile,
we generalized the HD-audio codec driver to be on a generic HD-audio
bus, and this allowed the use without regmap.  So some basic helpers
are designed to work without regmap but some are still tightly tied
with regmap.


Takashi