| Message ID | 20230609-alsa-hda-maple-v1-1-a2b725c8b8f5@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: Use maple tree register cache | expand |
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
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
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.
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
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.
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
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.
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
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?
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
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,
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(-) --- base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7 change-id: 20230609-alsa-hda-maple-02f75ebb067e Best regards,