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 9075AC433EF for ; Sat, 5 Feb 2022 07:42:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Tiuhy7U7gapW+tT+Yaj9kRd8s+zpTGTcMGi9r4xT15I=; b=G7nd0Ny3oPGYJA wou+KyJPE3+vxUkF4TqW2f6wXIjw++SZDyR/fbERqQIsFph+1VVlhx6FerwuwFUNUdokYKkT7igWN L96foTE/eckFUb/iA+vMjBfqvljlxgMZe02Q7JKoHyGdXGnqCj/fCLXD4W5l5f99WVqGFjma8rFxX m9cevOSZZ+qV0wG2AJTJzO0qQlcDbbZsrCil3ENRrKbAkaDZN+USdtg3nvShnK2pNqkjMCFi1F4kV BeD5lj+TjyndRe7rblO9atNwVanIavYRtj38W0jiApKTs2OaHR5EnG/qZK3Bms3AKAMyGCaKEf6Vc aZW9oETcTEnJpr2wT4Yw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nGFgL-006DWQ-91; Sat, 05 Feb 2022 07:40:29 +0000 Received: from mail-ed1-x52c.google.com ([2a00:1450:4864:20::52c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nGFgH-006DVz-7h for linux-arm-kernel@lists.infradead.org; Sat, 05 Feb 2022 07:40:27 +0000 Received: by mail-ed1-x52c.google.com with SMTP id ee12so17132893edb.8 for ; Fri, 04 Feb 2022 23:40:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=GE8YLnpOfkXJ/M62/L0jrAV8ZjZli33TcMgWh60BUns=; b=Wlzm0NooyGpEjD+HwFPz8xB5kXfctHqd86/eBwkrMC3Ct1wmVtH98U+ow768ValQMt yUtnwHwYVKHPP8gXMJcBerEZdfM3UYDhdt4WmY7SkDzNxMDmgW4rv/FmHVy0xwF+/ucM /sBZCrku493Xo+R5DAM9vgyBIb17/T1ULzNAZZ2aJCtbeRTDMrHcFm5Vf5nvYE9Zpojw 0tfUXlVoDZ1eoj71dQSvCUf1oSDBY7L9o8Q2srPAnVT9xqdvkFnYju0VzxETOSh89nxE 206UoW7tNtixLYu9Vo6S4TVfccQXogrOeCi3a4uuSbqch2Er6QF86cbBvoVhMwYKI8Qh pB/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=GE8YLnpOfkXJ/M62/L0jrAV8ZjZli33TcMgWh60BUns=; b=HjSoY9AP12GpDB7ixnrNr9pRm5EeRAjhjFUkZpqor5YnLmY28VTECmgA3/YE906wo9 AmFcV+UsyOcONrkb60HA/0ErIgLb0OazUZKmBI8XAS3P7dGSbYebPkpestWpfHkaUdFZ FLCvk4QcJViLHKS/5ad68G7/NO5Y4381XkzCUMUtX3l8ZixvX6KnhHjod2UElwojC3HU aJ7GEcr29IiN49mSuzrve+uBKbPmTZYkQwF9dMcYiIdhZEpAcVW2trlajWDyIglj4XXZ AsjW5w68s7MCPFUOo1H0G6PRJ7mkbE8mWdfDiPlbn3Aq1neN4N5T4wl958K6+52PKhgo 25OA== X-Gm-Message-State: AOAM532sEExdkegKtZmuoSWOrRufvs+aNbvLvlz3wSHgcSI0DD7u0gtb WRk+3ZZRsxC1cW74amFal+M= X-Google-Smtp-Source: ABdhPJxqSj0Uo5Bw1vRYUdo+ZYd4jLgGloe3T4Y9MH0ZC7QfaEj/rcbDexSZg5tbPiKOhRWDZgqc6A== X-Received: by 2002:a05:6402:2707:: with SMTP id y7mr3114321edd.30.1644046822919; Fri, 04 Feb 2022 23:40:22 -0800 (PST) Received: from jernej-laptop.localnet (89-212-178-211.dynamic.t-2.net. [89.212.178.211]) by smtp.gmail.com with ESMTPSA id lt23sm1387639ejb.173.2022.02.04.23.40.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Feb 2022 23:40:22 -0800 (PST) From: Jernej =?utf-8?B?xaBrcmFiZWM=?= To: Paul Kocialkowski Cc: mripard@kernel.org, mchehab@kernel.org, wens@csie.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH] media: cedrus: Add watchdog for job completion Date: Sat, 05 Feb 2022 08:40:21 +0100 Message-ID: <12946098.uLZWGnKmhe@jernej-laptop> In-Reply-To: References: <20220201183324.493542-1-jernej.skrabec@gmail.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220204_234025_355461_354BBE6B X-CRM114-Status: GOOD ( 36.21 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Dne petek, 04. februar 2022 ob 10:09:56 CET je Paul Kocialkowski napisal(a): > Hi Jernej, > > On Tue 01 Feb 22, 19:33, Jernej Skrabec wrote: > > Currently, if job is not completed for whatever reason, userspace > > application can hang on ioctl and thus become unkillable. > > > > In order to prevent that, implement watchdog, which will complete job > > after 2 seconds with error state. > > > > Concept is borrowed from hantro driver. > > Good idea to implement a watchdog here, thanks! > See comments below. > > > Signed-off-by: Jernej Skrabec > > --- > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 ++ > > drivers/staging/media/sunxi/cedrus/cedrus.h | 3 +++ > > .../staging/media/sunxi/cedrus/cedrus_dec.c | 4 +++ > > .../staging/media/sunxi/cedrus/cedrus_hw.c | 25 +++++++++++++++++++ > > .../staging/media/sunxi/cedrus/cedrus_hw.h | 2 ++ > > 5 files changed, 36 insertions(+) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > 4a4b714b0f26..68b3dcdb5df3 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > @@ -439,6 +439,8 @@ static int cedrus_probe(struct platform_device *pdev) > > > > mutex_init(&dev->dev_mutex); > > > > + INIT_DELAYED_WORK(&dev->watchdog_work, cedrus_watchdog); > > + > > > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > if (ret) { > > > > dev_err(&pdev->dev, "Failed to register V4L2 device\n"); > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index > > c345f2984041..3bc094eb497f 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > > @@ -24,6 +24,7 @@ > > > > #include > > #include > > > > +#include > > > > #define CEDRUS_NAME "cedrus" > > > > @@ -194,6 +195,8 @@ struct cedrus_dev { > > > > struct reset_control *rstc; > > > > unsigned int capabilities; > > > > + > > + struct delayed_work watchdog_work; > > > > }; > > > > extern struct cedrus_dec_ops cedrus_dec_ops_mpeg2; > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index > > a16c1422558f..9c7200299465 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > @@ -97,4 +97,8 @@ void cedrus_device_run(void *priv) > > > > v4l2_ctrl_request_complete(src_req, &ctx->hdl); > > > > dev->dec_ops[ctx->current_codec]->trigger(ctx); > > > > + > > + /* Start the watchdog timer. */ > > + schedule_delayed_work(&dev->watchdog_work, > > + msecs_to_jiffies(2000)); > > > > } > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index > > 2d7663726467..a6470a89851e 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > @@ -118,6 +118,13 @@ static irqreturn_t cedrus_irq(int irq, void *data) > > > > enum vb2_buffer_state state; > > enum cedrus_irq_status status; > > > > + /* > > + * If cancel_delayed_work returns false it means watchdog already > > + * executed and finished the job. > > + */ > > + if (!cancel_delayed_work(&dev->watchdog_work)) > > + return IRQ_HANDLED; > > + > > > > ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev); > > if (!ctx) { > > > > v4l2_err(&dev->v4l2_dev, > > > > @@ -143,6 +150,24 @@ static irqreturn_t cedrus_irq(int irq, void *data) > > > > return IRQ_HANDLED; > > > > } > > > > +void cedrus_watchdog(struct work_struct *work) > > +{ > > + struct cedrus_dev *dev; > > + struct cedrus_ctx *ctx; > > + > > + dev = container_of(to_delayed_work(work), > > + struct cedrus_dev, watchdog_work); > > + > > + ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev); > > + if (!ctx) > > + return; > > + > > + v4l2_err(&dev->v4l2_dev, "frame processing timed out!\n"); > > + reset_control_reset(dev->rstc); > > I don't think playing with the reset is the right approach here. > First we don't really know if the reset is shared or not, so this might have > no effect. AFAIK only few reset lines are shared in all Allwinner SoC, never for Cedrus and even then, this is considered as HW issue. So, I'm good with using reset line. This principle is also taken from Hantro driver. > Then even if it does, wouldn't this just reset the state of the > registers to an unconfigured state, making it impossible to decode any > future frame in the same context? Being stateless core, all context is held in auxiliary buffers, reference frames and controls, which are not reset with pulsing reset line, so no, state is not lost. Anyway, if decoding fails, you're generally screwed until next key frame. You'll have to deal with decoding issues/artefacts nevertheless. > > Honestly I'm not sure what a good process would be to get back on track > here so I would be tempted to just do nothing an return errors. > > That's already better than being stuck. Doing nothing will solve only current job, but HW will still be stuck in decoding state. I doubt reprogramming registers and triggering new decoding will actually do anything. I'll check BSP lib sources again. Maybe selecting non-existing decoding mode would reset the core. That is already suggested as good thing to do in order to put core in low power mode. IMO we have to do something. Doing nothing will probably just lock up the core until next reboot or maybe until trying different decoding mode. Anyway, I have to find another way to cause decoding job to time out. Currently I'm doing this with IOMMU on H6, but that brings down several other things, which requires reboot anyway. Best regards, Jernej > > Paul > > > + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx- >fh.m2m_ctx, > > + VB2_BUF_STATE_ERROR); > > +} > > + > > > > int cedrus_hw_suspend(struct device *device) > > { > > > > struct cedrus_dev *dev = dev_get_drvdata(device); > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index > > 45f641f0bfa2..7c92f00e36da 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > @@ -28,4 +28,6 @@ int cedrus_hw_resume(struct device *device); > > > > int cedrus_hw_probe(struct cedrus_dev *dev); > > void cedrus_hw_remove(struct cedrus_dev *dev); > > > > +void cedrus_watchdog(struct work_struct *work); > > + > > > > #endif _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel