diff mbox series

[2/8] ASoC: tegra: Fix AMX byte map

Message ID 1687433656-7892-3-git-send-email-spujar@nvidia.com
State New
Headers show
Series Few audio fixes on Tegra platforms | expand

Commit Message

Sameer Pujar June 22, 2023, 11:34 a.m. UTC
From: Sheetal <sheetal@nvidia.com>

Byte mask for channel-1 of stream-1 is not getting enabled and this
causes failures during AMX use cases. The enable bit is not set during
put() callback of byte map mixer control.

This happens because the byte map value 0 matches the initial state
of byte map array and put() callback returns without doing anything.

Fix the put() callback by actually looking at the byte mask array
to identify if any change is needed and update the fields accordingly.
Also update get() callback to return 256 if the byte map is disabled.

Fixes: 8db78ace1ba8 ("ASoC: tegra: Fix kcontrol put callback in AMX")
Cc: stable@vger.kernel.org
Signed-off-by: Sheetal <sheetal@nvidia.com>
Reviewed-by: Mohan Kumar D <mkumard@nvidia.com>
Reviewed-by: Sameer Pujar <spujar@nvidia.com>
---
 sound/soc/tegra/tegra210_amx.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

Comments

Sameer Pujar June 23, 2023, 5:39 a.m. UTC | #1
On 22-06-2023 17:37, Mark Brown wrote:
> On Thu, Jun 22, 2023 at 05:04:10PM +0530, Sameer Pujar wrote:
>> From: Sheetal <sheetal@nvidia.com>
>>
>> Byte mask for channel-1 of stream-1 is not getting enabled and this
>> causes failures during AMX use cases. The enable bit is not set during
>> put() callback of byte map mixer control.
>>
>> This happens because the byte map value 0 matches the initial state
>> of byte map array and put() callback returns without doing anything.
>>
>> Fix the put() callback by actually looking at the byte mask array
>> to identify if any change is needed and update the fields accordingly.
> I'm not quite sure I follow the logic here - I'd have expected this to
> mean that there's a bootstrapping issue and that we should be doing some
> more initialisation during startup such that the existing code which
> checks if there is a change will be doing the right thing?
The issue can happen in subsequent cycles as well if once the user 
disables the byte map by putting 256. It happens because of following 
reason where 256 value is reset to 0 since the byte map array is tightly 
packed and it can't store 256 value.

static int tegra210_amx_put_byte_map() {
         ...
         if (value >= 0 && value <= 255)
             mask_val |= (1 << (reg % 32));
         else
             mask_val &= ~(1 << (reg % 32));

         if (mask_val == amx->byte_mask[reg / 32])
             return 0;

         /* Update byte map and slot */
==>     bytes_map[reg] = value % 256;
         amx->byte_mask[reg / 32] = mask_val;

         return 1;
}

>> Also update get() callback to return 256 if the byte map is disabled.
> This will be a user visible change.  It's not clear to me why it's
> needed - it seems like it's a hack to push users to do an update in the
> case where they want to use channel 1 stream 1?

Though it looks like 256 value is forced, but actually the user sees 
whatever value is set before. The 256 value storage is linked to byte 
mask value.

I must admit that this is not easily readable. If you suggest to 
simplify this, I can check if storage space increase for byte map value 
can make it more readable. Thanks for your feedback.
Mark Brown June 23, 2023, 10:15 a.m. UTC | #2
On Fri, Jun 23, 2023 at 11:09:32AM +0530, Sameer Pujar wrote:
> On 22-06-2023 17:37, Mark Brown wrote:
> > On Thu, Jun 22, 2023 at 05:04:10PM +0530, Sameer Pujar wrote:

> > > Byte mask for channel-1 of stream-1 is not getting enabled and this
> > > causes failures during AMX use cases. The enable bit is not set during
> > > put() callback of byte map mixer control.

> > > This happens because the byte map value 0 matches the initial state
> > > of byte map array and put() callback returns without doing anything.

> > > Fix the put() callback by actually looking at the byte mask array
> > > to identify if any change is needed and update the fields accordingly.

> > I'm not quite sure I follow the logic here - I'd have expected this to
> > mean that there's a bootstrapping issue and that we should be doing some
> > more initialisation during startup such that the existing code which
> > checks if there is a change will be doing the right thing?

> The issue can happen in subsequent cycles as well if once the user disables
> the byte map by putting 256. It happens because of following reason where
> 256 value is reset to 0 since the byte map array is tightly packed and it
> can't store 256 value.

...

> > > Also update get() callback to return 256 if the byte map is disabled.
> > This will be a user visible change.  It's not clear to me why it's
> > needed - it seems like it's a hack to push users to do an update in the
> > case where they want to use channel 1 stream 1?

> Though it looks like 256 value is forced, but actually the user sees
> whatever value is set before. The 256 value storage is linked to byte mask
> value.

> I must admit that this is not easily readable. If you suggest to simplify
> this, I can check if storage space increase for byte map value can make it
> more readable. Thanks for your feedback.

This could definitely use more clarification I think.  It's not obvious
that storing 256 won't actually hold (and that should trigger a
complaint from mixer-test if that's what happens).
Sameer Pujar June 26, 2023, 9:41 a.m. UTC | #3
On 23-06-2023 15:45, Mark Brown wrote:
> On Fri, Jun 23, 2023 at 11:09:32AM +0530, Sameer Pujar wrote:
>> On 22-06-2023 17:37, Mark Brown wrote:
>>> On Thu, Jun 22, 2023 at 05:04:10PM +0530, Sameer Pujar wrote:
>>>> Byte mask for channel-1 of stream-1 is not getting enabled and this
>>>> causes failures during AMX use cases. The enable bit is not set during
>>>> put() callback of byte map mixer control.
>>>> This happens because the byte map value 0 matches the initial state
>>>> of byte map array and put() callback returns without doing anything.
>>>> Fix the put() callback by actually looking at the byte mask array
>>>> to identify if any change is needed and update the fields accordingly.
>>> I'm not quite sure I follow the logic here - I'd have expected this to
>>> mean that there's a bootstrapping issue and that we should be doing some
>>> more initialisation during startup such that the existing code which
>>> checks if there is a change will be doing the right thing?
>> The issue can happen in subsequent cycles as well if once the user disables
>> the byte map by putting 256. It happens because of following reason where
>> 256 value is reset to 0 since the byte map array is tightly packed and it
>> can't store 256 value.
> ...
>
>>>> Also update get() callback to return 256 if the byte map is disabled.
>>> This will be a user visible change.  It's not clear to me why it's
>>> needed - it seems like it's a hack to push users to do an update in the
>>> case where they want to use channel 1 stream 1?
>> Though it looks like 256 value is forced, but actually the user sees
>> whatever value is set before. The 256 value storage is linked to byte mask
>> value.
>> I must admit that this is not easily readable. If you suggest to simplify
>> this, I can check if storage space increase for byte map value can make it
>> more readable. Thanks for your feedback.
> This could definitely use more clarification I think.  It's not obvious
> that storing 256 won't actually hold (and that should trigger a
> complaint from mixer-test if that's what happens).

OK. I will provide more clarifications in the commit message and the 
driver for the existing failure. Will put a TODO item in the driver to 
improve the logic and make it more readable.
diff mbox series

Patch

diff --git a/sound/soc/tegra/tegra210_amx.c b/sound/soc/tegra/tegra210_amx.c
index 782a141..1410e8b 100644
--- a/sound/soc/tegra/tegra210_amx.c
+++ b/sound/soc/tegra/tegra210_amx.c
@@ -2,7 +2,7 @@ 
 //
 // tegra210_amx.c - Tegra210 AMX driver
 //
-// Copyright (c) 2021 NVIDIA CORPORATION.  All rights reserved.
+// Copyright (c) 2021-2023 NVIDIA CORPORATION.  All rights reserved.
 
 #include <linux/clk.h>
 #include <linux/device.h>
@@ -206,7 +206,7 @@  static int tegra210_amx_get_byte_map(struct snd_kcontrol *kcontrol,
 	if (enabled)
 		ucontrol->value.integer.value[0] = bytes_map[reg];
 	else
-		ucontrol->value.integer.value[0] = 0;
+		ucontrol->value.integer.value[0] = 256;
 
 	return 0;
 }
@@ -221,25 +221,19 @@  static int tegra210_amx_put_byte_map(struct snd_kcontrol *kcontrol,
 	unsigned char *bytes_map = (unsigned char *)&amx->map;
 	int reg = mc->reg;
 	int value = ucontrol->value.integer.value[0];
+	unsigned int mask_val = amx->byte_mask[reg / 32];
 
-	if (value == bytes_map[reg])
+	if (value >= 0 && value <= 255)
+		mask_val |= (1 << (reg % 32));
+	else
+		mask_val &= ~(1 << (reg % 32));
+
+	if (mask_val == amx->byte_mask[reg / 32])
 		return 0;
 
-	if (value >= 0 && value <= 255) {
-		/* Update byte map and enable slot */
-		bytes_map[reg] = value;
-		if (reg > 31)
-			amx->byte_mask[1] |= (1 << (reg - 32));
-		else
-			amx->byte_mask[0] |= (1 << reg);
-	} else {
-		/* Reset byte map and disable slot */
-		bytes_map[reg] = 0;
-		if (reg > 31)
-			amx->byte_mask[1] &= ~(1 << (reg - 32));
-		else
-			amx->byte_mask[0] &= ~(1 << reg);
-	}
+	/* Update byte map and slot */
+	bytes_map[reg] = value % 256;
+	amx->byte_mask[reg / 32] = mask_val;
 
 	return 1;
 }