Hey Matthijs, On 07.04.2024 23:16, Matthijs Kooijman wrote:
Hi,
I've been chasing a decoder bug in the cedrus h264 hardware decoder and after a
few days have been able to work around it by removing the
DMA_ATTR_NO_KERNEL_MAPPING argument when allocating the neighbour info dma
buffer (full patch at the end of the mail):
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -581,7 +581,7 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
ctx->codec.h264.neighbor_info_buf =
dma_alloc_attrs(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
&ctx->codec.h264.neighbor_info_buf_dma,
- GFP_KERNEL, DMA_ATTR_NO_KERNEL_MAPPING);
+ GFP_KERNEL, 0);
This does not seem like a proper fix to me (or maybe it is - I do not really
understand the problem yet), but it very effective.
thanks for the patch, I don't think we can take it as due to, as you highlighted, it relies on unknown side effects. Perhaps if you or someone else in the community can investigate this further and answer some more questions we can move it forward. Some questions to help move the discussions along: - Does the platform have an IOMMU? - What does the IOMMU driver do when it sees the DMA_ATTR_NO_KERNEL_MAPPING flag? - On the platform in question do coherent allocations get handled with cached snooped mappings or uncached? - Does changing that flag affect any cacheablility - Does the codec block expect any particular intialized data in that buffer? Greetings, Sebastian
The actual problem is that sometimes (about 10%-50%) the first frame
of a h264 stream incorrectly decodes, producing all-zero data in the bottom
part of the frame. See attached error-green.png for an example. In some even
more rare cases, more subtle decoding errors would occur, for example a lighter
rectangle in the bottom left dark blue band in error-blue-bar.png, or some
misplaced pixels in the bottom right "noise" area (compare error-noise.png with
error-blue-bar.png to see).
To reproduce:
# Create a h264-encoded 320x240 single-frame stream
gst-launch-1.0 videotestsrc num-buffers=1 ! openh264enc ! filesink location=test.h264
# Decode it again using hardware decoding (repeat until seeing a broken frame)
gst-launch-1.0 filesrc location=test.h264 ! v4l2slh264dec ! videoconvert ! pngenc ! filesink location=test.png
I've been testing on an Orange Pi PC with an allwinner H3 running
Armbian 22.04 (based on Ubuntu Jammy), a 6.7.12 kernel (mostly with
Armbian-patched, but also clean mainline) and gstreamer
1.20.3. The problem also occurred on older kernel versions (tested clean
mainline down to 6.2 and then a bit earlier, but 6.1 mainline would
not boot for me). I have also observed the problem (or at least the same
symptom) on the Armbian-patched 6.1 kernel, but there it was extremely unlikely
(saw it once in 700 or so playbacks).
I've seen three variants of this issue:
1. With the 320x240 test pattern, showing the green area in the frame.
In this case, the decoder would actually return an error IRQ status
(VE_H264_STATUS register would be 0x10010003 or 0x70010003, instead
of the normal 0x60000001, bit 0 is VE_H264_STATUS_SLICE_DECODE_INT,
bit 1 is VE_H264_STATUS_DECODE_ERR_INT).
2. With the 320x240 test pattern, showing more subtle decoding errors (as
shown in attachments). In these cases, the IRQ status would not indicate a
decoding error.
3. With another test video (1920x1080) that I cannot share, there would
be the same green areas (occasionally also green squares splattered
around the frame), but *no* decoding error in the IRQ status.
None of these three issues occur anymore with the workaround applied (both on
clean 6.7.12 and on top of the Armbian patches), the gstreamer decoding then
produces bit-for-bit identical results every time (tested hundreds of
decodings).
I have no clue why removing DMA_ATTR_NO_KERNEL_MAPPING from this
allocation helps - I just tried it on a whim when I nothing else seemed
to help. I suspect that mapping the memory into kernel space might have
some side effect that helps, but I am not familiar enough with either
the cedrus hardware or the DMA system to tell.
I initially thought there might be an alignment issue and tried
increasing the allocation sizes of all DMA buffers to 256K (which
I think enforces a bigger alignment), but that did not help. The reason
I was looking at memory allocation (and also timing differences, but no
luck there) was that bisecting pointed me at commit fec94f8c995 ("media:
cedrus: h264: Optimize mv col buffer allocation") which seemed to make
the problem 2-4× likely to occur (but looking back, I think I might have
been bisecting noise there).
I hope you folks can make more sense of this to figure out a proper fix.
For the short term my problem is solved (I'll just apply this patch and ship
it), so will not be investing more time right now.
If there's anything I can contribute, just let me know (I have done some tests
with extra debug output added to the kernel, and a testscript for repeatedly
testing the decoding that I could share, and I am happy to test any
patches/hypothesis that you have).
Gr.
Matthijs
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index dfb401df138..7cd3b6a5434 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -581,7 +581,7 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
ctx->codec.h264.neighbor_info_buf =
dma_alloc_attrs(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
&ctx->codec.h264.neighbor_info_buf_dma,
- GFP_KERNEL, DMA_ATTR_NO_KERNEL_MAPPING);
+ GFP_KERNEL, 0);
if (!ctx->codec.h264.neighbor_info_buf) {
ret = -ENOMEM;
goto err_pic_buf;
@@ -634,7 +634,7 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
dma_free_attrs(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
ctx->codec.h264.neighbor_info_buf,
ctx->codec.h264.neighbor_info_buf_dma,
- DMA_ATTR_NO_KERNEL_MAPPING);
+ 0);
err_pic_buf:
dma_free_attrs(dev->dev, ctx->codec.h264.pic_info_buf_size,
@@ -670,7 +670,7 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx)
dma_free_attrs(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
ctx->codec.h264.neighbor_info_buf,
ctx->codec.h264.neighbor_info_buf_dma,
- DMA_ATTR_NO_KERNEL_MAPPING);
+ 0);
dma_free_attrs(dev->dev, ctx->codec.h264.pic_info_buf_size,
ctx->codec.h264.pic_info_buf,
ctx->codec.h264.pic_info_buf_dma,
Attachment:
signature.asc
Description: PGP signature
