bcm2835-i2s: Changes for allowing asymmetric sample formats. #1783

gtrainavicius · 2017-01-08T14:11:29Z

Hey,

I've noticed that when overdubbing audio with Audacity using 16-bit sample format for recording, it still uses 32 bits for playback, causing corrupt audio to get recorded. The bcm2835-i2s forces format symmetry between the streams, but it needn't be the case. (I alternatively tried to set .symmetric_formats=1 in bcm2835_i2s_dai, but Audacity wasn't able to detect this, and failed to record completely which would be unacceptable.)

I've made some changes that allow having asymmetric sample formats, and tested various combinations of simultaneous playback and recording with the pisound card, however, I don't own any other audio cards, so I can't test the changes using them.

Anyway, I tried to keep the changes to minimum, the only cases these changes should affect is when the user software is requesting asymmetric sample format for playback and capture, in all other cases, it should behave the same as without the changes.

Thank you,
Giedrius.

This is achieved by making changes only to the requested stream direction format, keeping the other stream direction configuration intact. Signed-off-by: Giedrius Trainavicius <[email protected]>

pelwell · 2017-01-09T10:06:08Z

This change looks reasonable to me - it compiles, and I2S playback still works. @HiassofT @msperl Any comments?

msperl · 2017-01-09T10:09:56Z

more testing and upstreaming maybe?

gtrainavicius · 2017-01-09T12:58:08Z

Let me know if there's anything I can do to help :)

HiassofT · 2017-01-09T13:27:02Z

I'll run some playback/recording tests with the cirrus logic audio card and then report back - could take a few days (busy with work)

kernel: bcm2835-i2s: Changes for allowing asymmetric sample formats See: raspberrypi/linux#1783 firmware: bootcode: Initialise default force_pvt firmware: hdmi: Use hdmi drive when any hdmi modes are supported See: https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=169879 firmware: CEC code cleanup 1: Removed CEC bridge

flatmax

There are unused variables in this patch. There is a risky introduction of restoring mode, rather then starting with it =0.

flatmax · 2017-01-09T19:55:46Z

sound/soc/bcm/bcm2835-i2s.c

+	regmap_read(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, &mode);
+
+	previous_ftxp = mode & BCM2835_I2S_FTXP;
+	previous_frxp = mode & BCM2835_I2S_FRXP;


If these previous variables aren't used, they should be removed.

They are used, see below.

These can still be removed, see my new comment for line number 502

flatmax · 2017-01-09T22:42:19Z

sound/soc/bcm/bcm2835-i2s.c

+		break;
+	default:
+		return -EINVAL;
+	}


The format is the same, why split ?

hw_params is called once for each direction (playback, capture), depending on what the user software wants to do. The substream->stream stores the requested direction. So the whole idea of this pull request is to react and change only the state that is relevant to the requested direction.

OK seems good.

flatmax · 2017-01-09T22:46:49Z

sound/soc/bcm/bcm2835-i2s.c


 	/* Setup the I2S mode */
+	/* Keep existing FTXP and FRXP values. */
+	regmap_read(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, &mode);


The implications of restoring the mode are quite large and risky! Why not let the file set it up from scratch ?
All the lines which treat mode in the file do "mode |=" to alter mode. They all assume that mode starts as : mode = 0;

Only the stream direction specific state is kept, not the whole mode.

There's mode = 0 line still there, see below.

Yep I see it now.

gtrainavicius · 2017-01-10T09:43:40Z

sound/soc/bcm/bcm2835-i2s.c

+
+	switch (substream->stream) {
+	case SNDRV_PCM_STREAM_PLAYBACK:
+		mode |= previous_frxp;


This is the location where previous_... variables are used.

Oh yep, however you can remove this line, not necessary.

gtrainavicius · 2017-01-10T09:45:10Z

sound/soc/bcm/bcm2835-i2s.c

+
+	previous_ftxp = mode & BCM2835_I2S_FTXP;
+	previous_frxp = mode & BCM2835_I2S_FRXP;
+
 	mode = 0;


Here the mode is reset.

Yep, I see, but you can get rid of this for simplification because of changes suggested from line 502

gtrainavicius · 2017-01-10T09:46:43Z

@flatmax It looks like you've missed some lines, the comments you have made are not correct. Please see the places I've pointed out in the code.

pelwell · 2017-01-10T09:53:37Z

The limited context around each difference can make it hard to follow. I wondered whether the two separate previous_f?xp variables could be replaced with previous_mode, with the masking applied when mode is being assembled - something like:

	switch (substream->stream) {
	case SNDRV_PCM_STREAM_PLAYBACK:
		mode = (previous_mode & BCM2835_I2S_FRXP) |
			(packed ? BCM2835_I2S_FTXP : 0);
		break;
	case SNDRV_PCM_STREAM_CAPTURE:
		mode = (previous_mode & BCM2835_I2S_FTXP) |
			(packed ? BCM2835_I2S_FRXP : 0);
		break;

but I do think the patch is correct.

gtrainavicius · 2017-01-10T10:16:50Z

@pelwell My intention was to try to be explicit about what is kept from previous state, but I'd agree with your version too.

flatmax

I see what you are saying, however it is better to simplify this commit further - check my suggestions.

flatmax · 2017-01-10T10:11:56Z

sound/soc/bcm/bcm2835-i2s.c

+		break;
+	default:
+		return -EINVAL;
+	}


OK seems good.

flatmax · 2017-01-10T10:12:09Z

sound/soc/bcm/bcm2835-i2s.c


 	/* Setup the I2S mode */
+	/* Keep existing FTXP and FRXP values. */
+	regmap_read(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, &mode);


Yep I see it now.

flatmax · 2017-01-10T10:12:53Z

sound/soc/bcm/bcm2835-i2s.c

+	regmap_read(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, &mode);
+
+	previous_ftxp = mode & BCM2835_I2S_FTXP;
+	previous_frxp = mode & BCM2835_I2S_FRXP;


These can still be removed, see my new comment for line number 502

flatmax · 2017-01-10T10:15:38Z

sound/soc/bcm/bcm2835-i2s.c

@@ -310,6 +310,7 @@ static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
 	unsigned int sampling_rate = params_rate(params);
 	unsigned int data_length, data_delay, bclk_ratio;
 	unsigned int ch1pos, ch2pos, mode, format;
+	unsigned int previous_ftxp, previous_frxp;


These are still unnecessary.

flatmax · 2017-01-10T10:20:31Z

sound/soc/bcm/bcm2835-i2s.c

+	switch (substream->stream) {
+	case SNDRV_PCM_STREAM_PLAYBACK:
+		mode |= previous_frxp;
+		mode |= packed ? BCM2835_I2S_FTXP : 0;


Replace this with : mode &= packed ? BCM2835_I2S_FTXP : 0;

flatmax · 2017-01-10T10:20:49Z

sound/soc/bcm/bcm2835-i2s.c

+		mode |= packed ? BCM2835_I2S_FTXP : 0;
+		break;
+	case SNDRV_PCM_STREAM_CAPTURE:
+		mode |= previous_ftxp;


Remove this line.

flatmax · 2017-01-10T10:21:06Z

sound/soc/bcm/bcm2835-i2s.c

+		break;
+	case SNDRV_PCM_STREAM_CAPTURE:
+		mode |= previous_ftxp;
+		mode |= packed ? BCM2835_I2S_FRXP : 0;


Replace this with : mode &= packed ? BCM2835_I2S_FRXP : 0;

flatmax · 2017-01-10T10:21:42Z

sound/soc/bcm/bcm2835-i2s.c

+
+	switch (substream->stream) {
+	case SNDRV_PCM_STREAM_PLAYBACK:
+		mode |= previous_frxp;


Oh yep, however you can remove this line, not necessary.

flatmax · 2017-01-10T10:22:03Z

sound/soc/bcm/bcm2835-i2s.c

+
+	previous_ftxp = mode & BCM2835_I2S_FTXP;
+	previous_frxp = mode & BCM2835_I2S_FRXP;
+
 	mode = 0;


Yep, I see, but you can get rid of this for simplification because of changes suggested from line 502

flatmax · 2017-01-10T10:23:59Z

sound/soc/bcm/bcm2835-i2s.c

+	 * direction can be different as long as the frame length is
+	 * shared for both.
+	 */
+	packed = data_length <= 16;


Not sure if it is good practice, but the packed variable can also be removed and this can be inline below.

gtrainavicius · 2017-01-10T10:44:59Z

I'm sure it would work as you describe it, in my opinion being more explicit about the things that's happening (leaving the mode = 0; having explicit variables for what state is kept (previous_mode would work well too)) is better for readability.

Clearing most of mode variable in some switch by doing &= and keeping some of the state is a bit less explicit and easier to miss than an unconditional mode = 0;

Having more variables on stack should be really cheap on performance, it'd be great though if there was no warning for declaring the variable closer to where it's actually used rather than forcing it to be declared at the top of the function... :)

Anyway, up to you guys to decide what style to use.

HiassofT · 2017-01-10T19:58:17Z

I've ran some tests with the Cirrus Logic audio card (play, rec, play+rec at 16 and 32 bit) and everything worked fine. The wm5102 codec has symmetric_samplebits=1 so I couldn't test the asymmetric case - but from monitoring mode via printk it seems to work as expected. About coding style: IMO keep it as it is. Doing acrobatics with bit shifts and masks make the code hard to read, I'm not a huge fan of that. So, from my side, I think this PR is good to merge.

pelwell · 2017-01-10T20:00:05Z

Thanks, everyone.

gtrainavicius · 2017-01-10T20:14:32Z

Thank you!

msperl · 2017-01-11T07:31:59Z

please upstream the patch!

flatmax · 2017-01-11T09:51:01Z

I notice that this has been committed. Don't you think this is far simpler ?
if (data_length <= 16)
mode &= BCM2835_I2S_FRXP
else
mode=0;

Same for the other substream.

kernel: BCM270X_DT: Add pi3-disable-wifi overlay kernel: bcm2835-i2s: Changes for allowing asymmetric sample formats See: raspberrypi/linux#1783 kernel: Add driver_name properties to JustBoom drivers See: raspberrypi/linux#1787

kernel: BCM270X_DT: Add pi3-disable-wifi overlay kernel: Add driver_name properties to JustBoom drivers See: raspberrypi/linux#1787 kernel: bcm2835-i2s: Changes for allowing asymmetric sample formats See: raspberrypi/linux#1783 firmware: bootcode: Don't let total_mem exceed sdram size See: #717 firmware: bootcode: Enhance parsing to handle processor sections See: #716

kernel: BCM270X_DT: Add pi3-disable-wifi overlay kernel: Add driver_name properties to JustBoom drivers See: raspberrypi/linux#1787 kernel: bcm2835-i2s: Changes for allowing asymmetric sample formats See: raspberrypi/linux#1783 firmware: bootcode: Don't let total_mem exceed sdram size See: raspberrypi/firmware#717 firmware: bootcode: Enhance parsing to handle processor sections See: raspberrypi/firmware#716

HiassofT · 2017-01-16T19:04:13Z

Sorry, looks like I missed some nasty bugs when testing this - details are in issue #1799

…1783)" This reverts commit 4897c5c. Signed-off-by: Phil Elwell <[email protected]> See: #1799

pelwell · 2017-01-16T21:04:52Z

Reverted.

kernel: config: More USB config options for bcm2709_defconfig See: raspberrypi/linux#1805 kernel: config: Add CONFIG_MD_M25P80 and CONFIG_MD_SPI_NOR See: raspberrypi/linux#1781 kernel: BCM270X_DT: Enable UART0 on CM3 kernel: BCM270X_DT: Add spi0-cs overlay kernel: Revert bcm2835-i2s: Changes for allowing asymmetric sample formats See: raspberrypi/linux#1783

commit 9008fe8 upstream. On m68k, where the minimum alignment of unsigned long is 2 bytes: Kernel panic - not syncing: __kmem_cache_create_args: Failed to create slab 'io_kiocb'. Error -22 CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted 6.12.0-atari-03776-g7eaa1f99261a #1783 Stack from 0102fe5c: 0102fe5c 00514a2b 00514a2b ffffff00 00000001 0051f5ed 00425e78 00514a2b 0041eb74 ffffffea 00000310 0051f5ed ffffffea ffffffea 00601f60 00000044 0102ff20 000e7a68 0051ab8e 004383b8 0051f5ed ffffffea 000000b8 00000007 01020c00 00000000 000e77f0 0041e5f0 005f67c0 0051f5ed 000000b6 0102fef4 00000310 0102fef4 00000000 00000016 005f676c 0060a34c 00000010 00000004 00000038 0000009a 01000000 000000b8 005f668e 0102e000 00001372 0102ff88 Call Trace: [<00425e78>] dump_stack+0xc/0x10 [<0041eb74>] panic+0xd8/0x26c [<000e7a68>] __kmem_cache_create_args+0x278/0x2e8 [<000e77f0>] __kmem_cache_create_args+0x0/0x2e8 [<0041e5f0>] memset+0x0/0x8c [<005f67c0>] io_uring_init+0x54/0xd2 The minimal alignment of an integral type may differ from its size, hence is not safe to assume that an arbitrary freeptr_t (which is basically an unsigned long) is always aligned to 4 or 8 bytes. As nothing seems to require the additional alignment, it is safe to fix this by relaxing the check to the actual minimum alignment of freeptr_t. Fixes: aaa736b ("io_uring: specify freeptr usage for SLAB_TYPESAFE_BY_RCU io_kiocb cache") Fixes: d345bd2 ("mm: add kmem_cache_create_rcu()") Reported-by: Guenter Roeck <[email protected]> Closes: https://lore.kernel.org/[email protected] Cc: <[email protected]> Signed-off-by: Geert Uytterhoeven <[email protected]> Tested-by: Guenter Roeck <[email protected]> Reviewed-by: Jens Axboe <[email protected]> Signed-off-by: Vlastimil Babka <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>

bcm2835-i2s: Changes for allowing asymmetric sample formats.

d8ad6a6

This is achieved by making changes only to the requested stream direction format, keeping the other stream direction configuration intact. Signed-off-by: Giedrius Trainavicius <[email protected]>

flatmax reviewed Jan 9, 2017

View reviewed changes

gtrainavicius commented Jan 10, 2017

View reviewed changes

flatmax reviewed Jan 10, 2017

View reviewed changes

pelwell merged commit 4897c5c into raspberrypi:rpi-4.4.y Jan 10, 2017

HiassofT mentioned this pull request Jan 16, 2017

bcm2835-i2s regression from asymmetric sample formats change #1799

Closed

pelwell pushed a commit that referenced this pull request Jan 16, 2017

Revert "bcm2835-i2s: Changes for allowing asymmetric sample formats. (#…

cfd1adc

…1783)" This reverts commit 4897c5c. Signed-off-by: Phil Elwell <[email protected]> See: #1799

bcm2835-i2s: Changes for allowing asymmetric sample formats. #1783

bcm2835-i2s: Changes for allowing asymmetric sample formats. #1783

Conversation

gtrainavicius commented Jan 8, 2017

pelwell commented Jan 9, 2017

msperl commented Jan 9, 2017

gtrainavicius commented Jan 9, 2017

HiassofT commented Jan 9, 2017 via email

flatmax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtrainavicius Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

flatmax Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flatmax Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

gtrainavicius commented Jan 10, 2017

pelwell commented Jan 10, 2017 • edited Loading

gtrainavicius commented Jan 10, 2017

flatmax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flatmax Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flatmax Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

flatmax Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtrainavicius commented Jan 10, 2017

HiassofT commented Jan 10, 2017 via email

pelwell commented Jan 10, 2017

gtrainavicius commented Jan 10, 2017

msperl commented Jan 11, 2017

flatmax commented Jan 11, 2017

HiassofT commented Jan 16, 2017

pelwell commented Jan 16, 2017

gtrainavicius Jan 10, 2017 •

edited

Loading

flatmax Jan 10, 2017 •

edited

Loading

flatmax Jan 10, 2017 •

edited

Loading

pelwell commented Jan 10, 2017 •

edited

Loading

flatmax Jan 10, 2017 •

edited

Loading

flatmax Jan 10, 2017 •

edited

Loading

flatmax Jan 10, 2017 •

edited

Loading