From patchwork Sun Apr 7 21:16:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Matthijs Kooijman X-Patchwork-Id: 13620294 Received: from tika.stderr.nl (tika.stderr.nl [94.142.244.14]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 04E644AEC5 for ; Sun, 7 Apr 2024 21:32:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=94.142.244.14 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712525578; cv=none; b=h9N2LRE0ognfYfBoEf5JcMSf50Z8kOxoif3aXQQGalh6OLci/5oDi7PtVZRKsBxaLroDj14mOMASJDHkUAD+XaQohlGKnRFqoeKHFF8HWWjaeApSYizmTMe8j7qC2EUNaJ1r1EvktcZ8bdP/xwGnmMYrfdA00PpNRj0uuVrX+y0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712525578; c=relaxed/simple; bh=neA9jwkTinrCbbYwWoOfjDPn84lxHI5PLuSKl5QmCE8=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Disposition; b=YHGr+0gXTpcOz2GssRQCCxQ1GT7TwtXmcWnUPBaR1jk6Tio38QtSfbOeNX1mfywY/Zwoc33oWfAaPIlM2imJ22OvxMfbOM/HFqbjiwX5YrcOqF+Fyuv1s74GUFg14xnnac96GkMv4lwxKvJguMrRvvECNbOej3NtwDtEV1sQkNI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=stdin.nl; spf=fail smtp.mailfrom=stdin.nl; dkim=pass (1024-bit key) header.d=mail.tikatika.nl header.i=@mail.tikatika.nl header.b=BD4yOpzj; arc=none smtp.client-ip=94.142.244.14 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=stdin.nl Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=stdin.nl Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=mail.tikatika.nl header.i=@mail.tikatika.nl header.b="BD4yOpzj" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=mail.tikatika.nl; s=201709.tika; h=Content-Type:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=E2Es9rK9Toy60pqbwKeSb1q7c9jfLeLS59Fzz9ZOEig=; b=BD4yOpzjLEZBE7UCncnUwSEPvz PLVaMSZHOIj1X/m0CUmD1cfLTOTKt66XtuwMzLI0rrrkkw1/t1Ja8afY4hobmj2KvJTA+Pht63DY9 bPhppuRlvl7286G2KKOCL6KbxlX5vBWvQSluoTSGLtuU6hDhiHziwOz1R82DR9OjyW5Q=; X-Preliminary-Spam-Score: -2.5 (--) Received: from [10.42.0.16] (helo=login.tika.stderr.nl) by tika.stderr.nl with smtp (Exim 4.96) (envelope-from ) id 1rtZsT-002DaW-2I; Sun, 07 Apr 2024 23:16:38 +0200 Received: (nullmailer pid 627251 invoked by uid 2001); Sun, 07 Apr 2024 21:16:37 -0000 Date: Sun, 7 Apr 2024 23:16:37 +0200 From: Matthijs Kooijman To: Maxime Ripard , Paul Kocialkowski Cc: linux-media@vger.kernel.org, Jernej Skrabec Subject: media: cedrus: h264: Decoding fails or produces artifacts Message-ID: Mail-Followup-To: Matthijs Kooijman , Maxime Ripard , Paul Kocialkowski , linux-media@vger.kernel.org, Jernej Skrabec Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline X-PGP-Fingerprint: E7D0 C6A7 5BEE 6D84 D638 F60A 3798 AF15 A156 5658 X-PGP-Key: http://www.stderr.nl/static/files/gpg_pubkey.asc 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. 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,