From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6F79CCD4F24 for ; Tue, 12 May 2026 14:08:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=J2xPrSGSLq0PpqiMQaJdlY8PYtxGArCooKFROiwosD0=; b=I97E4Db8UhHnhse+vPLmNYjoBq sd1DBoy0heXcvymXXW35MBhP+4tBQXRq9+SrDIXyEEPLYULv3+E/09jTZBMAn7eY7V/m62JQAU5nb RuoZTxotteYd/jABqC0zp40AmicGu0HvpK+jPCG7WQc2l/KVVwkrIdR/QcmId/as0U2/XG5cOd4WU Hi2UlOYeZEsFVjTM3cRT8KW9D/XlBI7FSMlvg99iZA4rGDgcRhKHUHD8/WE7IzTv4I8lDXOMpt+LJ 6Avc+8F47C8u1/PTXoA0o646NAFrnaDIEEE3rn/UIYyBMSQ9LICMUKnNq9j/W3uiTfj3Evwwjtbu7 NHBEZZ/Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wMnn0-0000000Gxy7-0kV3; Tue, 12 May 2026 14:08:50 +0000 Received: from metis.whiteo.stw.pengutronix.de ([2a0a:edc0:2:b01:1d::104]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wMnmx-0000000Gxw5-1F3j for linux-arm-kernel@lists.infradead.org; Tue, 12 May 2026 14:08:48 +0000 Received: from ptz.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::77] helo=[127.0.0.1]) by metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1wMnmV-0001ds-N8; Tue, 12 May 2026 16:08:19 +0200 Message-ID: <1f447423-8c63-4545-a4f7-d8d5ef821255@pengutronix.de> Date: Tue, 12 May 2026 16:08:14 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 16/29] media: rockchip: rga: split flip and rotate into separate function To: Nicolas Dufresne , Jacob Chen , Ezequiel Garcia , Mauro Carvalho Chehab , Heiko Stuebner , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Hans Verkuil Cc: linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, sebastian.reichel@collabora.com References: <20260428-spu-rga3-v5-0-eb7f5d019d86@pengutronix.de> <20260428-spu-rga3-v5-16-eb7f5d019d86@pengutronix.de> Content-Language: en-US From: =?UTF-8?Q?Sven_P=C3=BCschel?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:900:1d::77 X-SA-Exim-Mail-From: s.pueschel@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260512_070847_482585_36DE1B0D X-CRM114-Status: GOOD ( 38.35 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Nicolas, On 5/9/26 12:11 AM, Nicolas Dufresne wrote: > Le mardi 28 avril 2026 à 11:00 +0200, Sven Püschel a écrit : >> Split the flip and rotate command configuration into a separate >> function in preparation of filling the command stream at streamon. >> As the userspace can change the flipping and rotation controls while >> streaming, we have to update them with each new frame to prevent the >> user being unable to change them while streaming. >> >> Signed-off-by: Sven Püschel > For code point of view, everything seems fine, but the commit message leave me a > bit wondering. Any rotation that isn't 180 degree will cause the width and > height to be reversed, and a new stride is needed to present the buffer > correctly. Meaning the capture format can be affected by this change. Sorry for missing to properly communicate my intention with this patch. I've stumbled over the RGA pulling a spin lock on the controls, when starting the next job [1]. This made me realize that the driver allows changing the controls while streaming, which my change to move the command buffer setup to streamon breaks (as a potential rotation isn't updated until the next streamon). This commit is the result of trying to not break the old behavior by moving the relevant code parts to be run on every frame instead of being only run at streamon. I didn't think about the 90 degree rotation problems, which are also present in the current RGA state. sashiko.dev also pointed out that my simple code move didn't work for the mirroring case [2], as the relevant command buffer is ore'd with the mirroring flags. Also I've noticed that the rotation mode affects the scaling factor. To avoid these footguns, I've got the idea to set a flag to raise when the controls change. Then I can fully re-initialize the command buffer on the next frame and fully avoid these kind of problems. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/rockchip/rga/rga.c?id=50897c955902c93ae71c38698abb910525ebdc89#n41 [2] https://sashiko.dev/#/patchset/20260428-spu-rga3-v5-0-eb7f5d019d86%40pengutronix.de?part=16 > > To stick with the spec, the capture format needs to be updated, and it needs to > happen in a way user can be able to read it back for the correct frame if > userspace make use of the queues. I see 3 options, let me know what you think, > or what is later implemented if you already thought about that. > > 1. Synchronously update the capture format width/height, document in the > respective control this behaviour, leaving to userspace to remember which frames > the change will apply to. > > This works nicely for this type of HW, but would be a bit complicated for a > deinterlacer, since the buffering might be HW specific. It also make usage of > queues harder, less independent. > > 2. Force a drain/stop/start for any 90 degree rotation > > This might impose a longer idle time for the converter core, and is kind of > opposite of your commit message. But requires no spec work. > > 3. Emit SRC_CH, implement the drain procedure typical to decoder resolution > change. > > Typically it means userspace can keep buffering on the OUTPUT queue, and once > the LAST buffer is met, it can simply read the new format (and new stride, since > due to alignment, this might be hardware specific) and toggle streamoff/on only > on capture queue to reactivate the processing. > > The 3. is more complex for the driver, but its a proven race-free method for > decoders already. 2 would be statusquo to get this series in, and we could post- > poned more advance work for seamless 90degree rorations. 1., I don't really like > that solution, it not quite generic enough. I'll go with option 2 for now to keep it simple and improve the status quo a bit. Sincerely     Sven > feedback welcome, > Nicolas > >> --- >>  drivers/media/platform/rockchip/rga/rga-hw.c | 57 +++++++++++++++++----------- >>  1 file changed, 34 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c >> index dac3cb6aa17d3..6c1956b04f6ba 100644 >> --- a/drivers/media/platform/rockchip/rga/rga-hw.c >> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c >> @@ -156,7 +156,38 @@ static void rga_cmd_set_dst_addr(struct rga_ctx *ctx, dma_addr_t dma_addr) >>   dest[reg >> 2] |= 0x7 << 8; >>  } >> >> -static void rga_cmd_set_trans_info(struct rga_ctx *ctx) >> +static void rga_cmd_set_flip_rotate_info(struct rga_ctx *ctx) >> +{ >> + u32 *dest = ctx->cmdbuf_virt; >> + union rga_src_info src_info; >> + >> + src_info.val = dest[(RGA_SRC_INFO - RGA_MODE_BASE_REG) >> 2]; >> + >> + if (ctx->vflip) >> + src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_X; >> + >> + if (ctx->hflip) >> + src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_Y; >> + >> + switch (ctx->rotate) { >> + case 90: >> + src_info.data.rot_mode = RGA_SRC_ROT_MODE_90_DEGREE; >> + break; >> + case 180: >> + src_info.data.rot_mode = RGA_SRC_ROT_MODE_180_DEGREE; >> + break; >> + case 270: >> + src_info.data.rot_mode = RGA_SRC_ROT_MODE_270_DEGREE; >> + break; >> + default: >> + src_info.data.rot_mode = RGA_SRC_ROT_MODE_0_DEGREE; >> + break; >> + } >> + >> + dest[(RGA_SRC_INFO - RGA_MODE_BASE_REG) >> 2] = src_info.val; >> +} >> + >> +static void rga_cmd_set_format_scale_info(struct rga_ctx *ctx) >>  { >>   struct rockchip_rga *rga = ctx->rga; >>   u32 *dest = ctx->cmdbuf_virt; >> @@ -219,27 +250,6 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx) >>   } >>   } >> >> - if (ctx->vflip) >> - src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_X; >> - >> - if (ctx->hflip) >> - src_info.data.mir_mode |= RGA_SRC_MIRR_MODE_Y; >> - >> - switch (ctx->rotate) { >> - case 90: >> - src_info.data.rot_mode = RGA_SRC_ROT_MODE_90_DEGREE; >> - break; >> - case 180: >> - src_info.data.rot_mode = RGA_SRC_ROT_MODE_180_DEGREE; >> - break; >> - case 270: >> - src_info.data.rot_mode = RGA_SRC_ROT_MODE_270_DEGREE; >> - break; >> - default: >> - src_info.data.rot_mode = RGA_SRC_ROT_MODE_0_DEGREE; >> - break; >> - } >> - >>   /* >>   * Calculate the up/down scaling mode/factor. >>   * >> @@ -431,7 +441,8 @@ static void rga_cmd_set(struct rga_ctx *ctx, >> >>   rga_cmd_set_src_info(ctx, &src->offset); >>   rga_cmd_set_dst_info(ctx, &dst->offset); >> - rga_cmd_set_trans_info(ctx); >> + rga_cmd_set_format_scale_info(ctx); >> + rga_cmd_set_flip_rotate_info(ctx); >> >>   rga_write(rga, RGA_CMD_BASE, ctx->cmdbuf_phy); >>