Message ID | 20230609-alsa-hda-maple-v1-1-a2b725c8b8f5@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp1580889vqr; Sat, 10 Jun 2023 07:43:07 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7NG7W1DGO0rwUO0dXeRHGmTXDgonr/CkU8b+RFSRjqxEBrBgLFA9+1MWm9xYy9n+U0dX+1 X-Received: by 2002:a17:907:7d92:b0:969:9c0c:4c97 with SMTP id oz18-20020a1709077d9200b009699c0c4c97mr5135420ejc.1.1686408187328; Sat, 10 Jun 2023 07:43:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686408187; cv=none; d=google.com; s=arc-20160816; b=NrTdBkY38zeu0YoJ5xz0phTsRTL7RCGtLAfUM/2Pr4BZOZhImpL3MVayVm5m9tO17E aVoJzlrRJRSYrTzAkzwbikFMt65r75l7cXu5507eWwp396AFbuCbMtenC+pbc4oHVbJW vnXlcF4BYND1rScqD/ceCHgxRnzuqTY+0gMdgcVLxgPHftdNH6FlxlHoWUbiFmn9fTdA EXeYH1FmEDR4opCUI5Rs66xnb6+j4yMB0mtpoiiwTmrI3ifw6THZo8qNhSPlBCeWRjoG Sy4ffmbuOoRNnxOHDiYbsDQjGMBKZsU2k4JYGJv2iSfJvF/ezrjyrW8viYxuGPfCFyrm oTDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:message-id:content-transfer-encoding :mime-version:subject:date:from:dkim-signature; bh=DSord+ISYGbL3Jaqr7FJkl57zU6VWaQQPPFfQejyPaM=; b=0cCY9Xa2z2JPlRWtJfvw8CamskFHA6YYUR9uTL2/lmnoOKLPtiywsdNMusnDEYITqs ZdyjA0VQldaaV1LAD5l3AbmQuSJaczUovdCwZb3jtLnDx7eMQug9gdjbNb5ny1DaW+O3 ni+KI5ZSgD+TkyqQAHbzpck6QKLuesymOlji94oX08Tp9BTStBxF5IBjjkgIZTVI0jwC Vcb1VN5DFDEN5yVBL4c8AUkUjh8gmOtKZih/g1Zbr7eZuGjGto3DkN+fj/x17Q8zzADq yDwPZS/Ej138MybAS64r/KYTBGWxY1uBV9WesyTyszW1k1+Q/5PP02+B7+GTnWTCSwkK 7YwA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Vp+4iCH8; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t13-20020a170906948d00b0094f6e186c50si3083734ejx.295.2023.06.10.07.42.42; Sat, 10 Jun 2023 07:43:07 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Vp+4iCH8; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234987AbjFJO0p (ORCPT <rfc822;rust.linux@gmail.com> + 99 others); Sat, 10 Jun 2023 10:26:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38176 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233239AbjFJO0n (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 10 Jun 2023 10:26:43 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2724CE5 for <linux-kernel@vger.kernel.org>; Sat, 10 Jun 2023 07:26:43 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B4C4660DFE for <linux-kernel@vger.kernel.org>; Sat, 10 Jun 2023 14:26:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 12705C433EF; Sat, 10 Jun 2023 14:26:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1686407202; bh=jaJzpJyj+5E2A//gUROcGxEPk881SMtYkFToNDsbtAM=; h=From:Date:Subject:To:Cc:From; b=Vp+4iCH89fmnVLPXUIJ9NkzYPo2Rcx88sQBzH03pUwpZujed/cNh2DDSunN9zcWcP yUB0Z8ikq7Xkmycqm+oDECWgi1HsOZAa4sim7ffz7w8WbLG5hOZgPLCDVfpxE8MUXX A/53+qgowMM7IAtmvq+/boRyslujb5lS7sOMx3sYXf/4JgnjlIC8JO5L5heSh8Z7bS 5VGLZdJHsbVVP0OrHcUAPk4Ym3xGICFqfvNwyOi2JLhkW0d7HqvG2ciNdvUekqL9VG U41KoFSCEoixXJdCmJGSD1YeYGojByuTlbQsPa4J677vgzETtAxGppzljvpqnD1gLg EELLfJ4fUaYCg== From: Mark Brown <broonie@kernel.org> Date: Sat, 10 Jun 2023 15:26:37 +0100 Subject: [PATCH] ALSA: hda: Use maple tree register cache MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20230609-alsa-hda-maple-v1-1-a2b725c8b8f5@kernel.org> X-B4-Tracking: v=1; b=H4sIAByIhGQC/x2NQQqDQAwAvyI5NxC3VKlfkR6yGruhukoCbUH8e 9ceZ2CYHVxMxaGrdjB5q+uaC9SXCobE+SmoY2EIFK7U0B15dsY0Mi68zYIUpvYmMVLTCpQosgt G4zykM/us9jr1ZjLp9//pH8fxA5q+90p3AAAA To: Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com> Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org> X-Mailer: b4 0.13-dev-bfdf5 X-Developer-Signature: v=1; a=openpgp-sha256; l=1082; i=broonie@kernel.org; h=from:subject:message-id; bh=jaJzpJyj+5E2A//gUROcGxEPk881SMtYkFToNDsbtAM=; b=owEBbQGS/pANAwAKASTWi3JdVIfQAcsmYgBkhIgfdHrblT9jwwcTTb8u7IUpTl5wXqj820MyIg71 lYQUT5+JATMEAAEKAB0WIQSt5miqZ1cYtZ/in+ok1otyXVSH0AUCZISIHwAKCRAk1otyXVSH0NznB/ 91sSHt6gyZfeq5HaKZVlOfypm8FPsMiE5ScY12lFPB5hGhX5M7JRYsyWc/ywdN4+PxWiRRNtMtmgpr rRVnKCrYWBQuhuo4zj6WUlTr6k0zT4193uio9FqHPGCds/6jlnvZNl4ceW9ViIUNI4Is9jS4YdfM+e CfqyxVeqbq55m/AcVFHPpLBqnoPeVdgVWDYCKLG7ZF+BNeLBDUItxH+iFW1sslX5aK5n4/Ewl4WHLN RXe93oRz2aDYq78MptC8AuwEMiTwtN1wIybG6+Q42afsHQJXdjXsuLG44fCADBhdiiijgjDsOFZ54S /O0nQJSoHOXeQXE+fagxjUNYoDv+a6 X-Developer-Key: i=broonie@kernel.org; a=openpgp; fpr=3F2568AAC26998F9E813A1C5C3F436CA30F5D8EB X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768327151055019614?= X-GMAIL-MSGID: =?utf-8?q?1768327151055019614?= |
Series |
ALSA: hda: Use maple tree register cache
|
|
Commit Message
Mark Brown
June 10, 2023, 2:26 p.m. UTC
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,
Comments
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,