Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: vc4: Avoid hdmi audio underrun with heavy sdram traffic #4053

Merged
merged 4 commits into from
Jan 7, 2021

Conversation

popcornmix
Copy link
Collaborator

We use 8 channel 192kHz audio for HD passthrough formats.
We've found with v4l2 h.264 video decode that we get fifo underruns in the audio dma.
(That decode path triggers a vpu based frame copy/convert)

See: https://forum.kodi.tv/showthread.php?tid=359450

…lags

Use bits 28 and 29 of the dreq value (the second cell of the DT DMA descriptor)
to request that wide source reads or wide dest writes are required

Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Without this set, DVP_CFG_MAI0_CTL indicates occasional
DLATE errors when configured to 8 channel 192kHz
when sdram bandwidth is high (e.g. playing h.264 video)

Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Set panic priority to 15 and leave normal priority at 0

Signed-off-by: Dom Cobley <popcornmix@gmail.com>
@popcornmix
Copy link
Collaborator Author

I think we should perhaps use DMA_WIDE_SOURCE / DMA_WIDE_DEST more often for memory source/dest.
Without it we are generating 4 times the bus traffic and (I think) 4 times the sdram bandwidth.

@pelwell pelwell merged commit a7eebc0 into raspberrypi:rpi-5.10.y Jan 7, 2021
@pelwell
Copy link
Contributor

pelwell commented Jan 7, 2021

Doing wide transfers for SPI might be quite a saving where it is being used to drive displays. And I2S, to a lesser extent. It's unfortunate that we can't use the same trick for those devices that share a channel for TX and RX as it stands.

@popcornmix popcornmix deleted the hdmiaudpri branch January 7, 2021 17:40
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jan 7, 2021
kernel: V4L2 ISP/Codec additional formats
See: raspberrypi/linux#4052

kernel: vc4: Avoid hdmi audio underrun with heavy sdram traffic
See: raspberrypi/linux#4053

firmware: audioplus: Fix hang when switching destination
See: #1516

firmware: HAT/I2C updates

firmware: MMAL/IL: Add support for the 16bpp Bayer/Grey raw 10/12/14 formats
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jan 7, 2021
kernel: V4L2 ISP/Codec additional formats
See: raspberrypi/linux#4052

kernel: vc4: Avoid hdmi audio underrun with heavy sdram traffic
See: raspberrypi/linux#4053

firmware: audioplus: Fix hang when switching destination
See: raspberrypi/firmware#1516

firmware: HAT/I2C updates

firmware: MMAL/IL: Add support for the 16bpp Bayer/Grey raw 10/12/14 formats
@HiassofT
Copy link
Contributor

HiassofT commented Jan 8, 2021

quick heads up: we got a report that ext4 filesystems got trashed on RPi2 LibreELEC/LibreELEC.tv#4903

Reverting the bcm2835-dma: Add bcm2835-dma: Add DMA_WIDE_SOURCE and DMA_WIDE_DEST flags commit fixes this

@@ -171,6 +171,17 @@ struct bcm2835_desc {
#define WAIT_RESP(x) ((x & BCM2835_DMA_NO_WAIT_RESP) ? \
0 : BCM2835_DMA_WAIT_RESP)

/* A fake bit to request that the driver requires wide reads */
#define BCM2835_DMA_WIDE_SOURCE BIT(28)
#define WIDE_SOURCE(x) ((x & BCM2835_DMA_WIDE_SOURCE) ? \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be wise to put some parentheses around the x here (and WAIT_RESP and WIDE_DEST), just in case.

u32 info = BCM2835_DMA_D_INC | BCM2835_DMA_S_INC;
u32 extra = BCM2835_DMA_INT_EN | WAIT_RESP(c->dreq);
u32 info = BCM2835_DMA_D_INC | BCM2835_DMA_S_INC | WAIT_RESP(c->dreq) |
WIDE_SOURCE(c->dreq) | WIDE_DEST(c->dreq);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the moving of WAIT_RESP to every transfer (and not just the last) and considered change?

And the new WIDE flags won't get applied to DMA40 channels until to_bcm2711_srci and to_bcm2711_dsti are also changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to make more sense to me that way although was intended for discussion (PR got merged before that though). @HiassofT did report that reverting this change didn't avoid the corruption. And excluding this change the commit should be a no-op on Pi2/3 with c->dreq not including bits 28/29, so I'm a bit confused exactly where the cause of corruption is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so am I.

@HiassofT
Copy link
Contributor

HiassofT commented Jan 8, 2021

I'm quite puzzled why these changes would break anything on RPi2 as well, I've looked over them several times and changing things I thought I spotted didn't help - obviously I'm missing something so a second (or third) pair of eyes might help.

Here's the stuff I tested so far - it's not all combinations and better redo the tests yourself in case I might have messed up:

Report of breakage was with 5.10.5 commit a7eebc0 - which I could confirm.

Going back to previous builds showed that 5.10.4 commit b323545 plus the first 2 commits in this PR (the patch here LibreELEC/LibreELEC.tv@d2ea7e0) also corrupted SD cards.

I then tested with above mentioned 5.10.5 with the following results:

reverting eb092d9 fixed the issue

reverting the WAIT_RESP change didn't help

--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -861,9 +861,9 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_memcpy(
 {
        struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
        struct bcm2835_desc *d;
-       u32 info = BCM2835_DMA_D_INC | BCM2835_DMA_S_INC | WAIT_RESP(c->dreq) |
+       u32 info = BCM2835_DMA_D_INC | BCM2835_DMA_S_INC |
                   WIDE_SOURCE(c->dreq) | WIDE_DEST(c->dreq);
-       u32 extra = BCM2835_DMA_INT_EN;
+       u32 extra = BCM2835_DMA_INT_EN | WAIT_RESP(c->dreq);
        size_t max_len = bcm2835_dma_max_frame_length(c);
        size_t frames;

reverting the arch/arm/boot/dts/bcm2835-common.dtsi change in b561d64 didn't help either

--- a/arch/arm/boot/dts/bcm2835-common.dtsi
+++ b/arch/arm/boot/dts/bcm2835-common.dtsi
@@ -123,7 +123,7 @@
                        clocks = <&clocks BCM2835_PLLH_PIX>,
                                 <&clocks BCM2835_CLOCK_HSM>;
                        clock-names = "pixel", "hdmi";
-                       dmas = <&dma (17|(1<<27)|(1<<28))>;
+                       dmas = <&dma (17|(1<<27))>;
                        dma-names = "audio-rx";
                        status = "disabled";
                };

I haven't tested these changes with 5.10.4 with just the first 2 commits (so leaving out the CS flags commit 32de3b3) - can try to do that the next days unless someone beats me to it

@graysky2
Copy link

graysky2 commented Jan 9, 2021

@HiassofT - Quite a bit to follow here and in the 5.10.y thread in the forums.

  1. Is RPi4 unaffected by this? There is at least one users in the forum thread stating otherwise, possibly two.
  2. If not, what is the last-known-good 5.10.y kernel commit and what is the corresponding commit of the firmware in order to avoid the FS corruption?

Are these correct:
Kernel: ff8d655
Firmware: raspberrypi/firmware@481ccbb

@HiassofT
Copy link
Contributor

HiassofT commented Jan 9, 2021

@graysky2 kernel commit 5837bdb (the one before the HDMI audio DMA series) should be fine - I haven't tested this though, in LibreELEC we simply went back to the 5.10.4 version we were using before.

The firmware commit you referenced is fine - newer firmwares might be fine,too, I haven't tested this yet though.

I didn't notice any issues on RPi4, I and several forum users were testing earlier and the latest version of the HDMI audio patch successfully the last 1.5 weeks

@graysky2
Copy link

graysky2 commented Jan 9, 2021

@HiassofT - Thanks for the info. I will just downgrade to 5837bdb until I see the issue has been fixed. Should someone create a new issue for this/I did not see one referencing the FS corruption? It would help users follow the upstream progress in fixing it and help us understand when it has been (officially) fixed.

graysky2 added a commit to graysky2/PKGBUILDs that referenced this pull request Jan 9, 2021
This is technically a downgrade in order to avoid potential file-system
corruption reported on RPi2[1,2].  At the time I made this commit, it is not
totally clear what the scope affected RPi models is.  For sure it hits
RPi2.  Reference 2 is a catch-all thread relating to 5.10.y development and
is not entirely about this issue.  You have to read on several pages beyond
the point I referenced.

References:
1. raspberrypi/linux#4053
2. https://www.raspberrypi.org/forums/viewtopic.php?p=1792526#p1792526
@HiassofT
Copy link
Contributor

HiassofT commented Jan 9, 2021

@pelwell this is most likely the culprit https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/bcm270x.dtsi#L187

&sdhost {
        dmas = <&dma (13|(1<<29))>;

setting DREQ bit 29 seems to be an old debugging leftover added in commit 0c7dd5c

@popcornmix
Copy link
Collaborator Author

@HiassofT good spot. Also arch/arm/boot/dts/bcm2835-common.dtsi has
dmas = <&dma (17|(1<<27)|(1<<28))>;
Looks like bit 28 can mean WAIT_FOR_OUTSTANDING_WRITES and bit 29 can mean DISDEBUG in DMA_CS register (but we've been using them to pass other flags in). Spec says in DMA_CS:
15:09 Reserved
27:24 Reserved
I don't see 24/25 being used, so it may be safe to move to there.

@popcornmix
Copy link
Collaborator Author

@HiassofT does a896a00 avoid the issue?

@HiassofT
Copy link
Contributor

HiassofT commented Jan 9, 2021

@popcornmix thanks, very quick test on RPi2 looks good, LE started up fine. Will do more testing (also on RPi4) the next days

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jan 9, 2021
See: raspberrypi/linux#4053

kernel: Add configuration override for MSEL0 on merus-amp overlay
See: raspberrypi/linux#4027
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this pull request Jan 9, 2021
See: raspberrypi/linux#4053

kernel: Add configuration override for MSEL0 on merus-amp overlay
See: raspberrypi/linux#4027
@popcornmix
Copy link
Collaborator Author

@HiassofT thanks for testing. I've pushed this to rpi-update.
Booted on a Pi3 without issue.

graysky2 added a commit to graysky2/PKGBUILDs that referenced this pull request Jan 9, 2021
This is the putative fix for file system corruption. See discussion
here: raspberrypi/linux#4053
graysky2 added a commit to graysky2/PKGBUILDs that referenced this pull request Jan 9, 2021
This is the putative fix for file system-corruption bug. See discussion
here: raspberrypi/linux#4053
@HiassofT
Copy link
Contributor

testing on RPi2, RPi3B+ and RPi4 looks good. No issues on RPi2/3 and HD audio passthrough on RPi4 worked fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants