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 2661CC43334 for ; Mon, 11 Jul 2022 21:23:13 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ZQWGbYk492K0+j5FajLjyi8tdKGUPL/qx0ip1p1Dt9w=; b=ZHulvNSTuYkYu4 +u4F//bZZQfkGueDUQ1OcGJsYM8QTuT52IZ4qSsALu2qo92NPUl15aAcKLy5updv2MwX9dP+x0cSe Uu2kuTukylfNc/wTXY3wLBdj6JhBwbzbuDGE2w+h7+vckTwJKIAa79ZD6cs8Lg/yHH6g5P9Au60o3 xnCVhqan3fSXgWtH0wbpauQKUIqQRnVvXVJsbAEl9ACBTm/aRXUfcXipQZLD3gg/lPFBpAdFCwCUu +8gEWpqWjwet4JJ3ISphpQWUe4tQd+pWkrc13/iELau578siYREAm2XQMg8oox7QRcqV4OYrwV/jW eqVBbcCDhGlm3h14HfJQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oB0r6-004tey-S7; Mon, 11 Jul 2022 21:22:12 +0000 Received: from mail-oi1-x242.google.com ([2607:f8b0:4864:20::242]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oB0r4-004tbd-1D for linux-arm-kernel@lists.infradead.org; Mon, 11 Jul 2022 21:22:11 +0000 Received: by mail-oi1-x242.google.com with SMTP id r82so8262006oig.2 for ; Mon, 11 Jul 2022 14:22:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vanguardiasur-com-ar.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=DlVNbxuhHiE0YW6IfxJk+i5c5jEODpePbQ6g9q+lpxA=; b=LwQWUEinUCMBYYBc7qN0jExmAf2FO1r7LHie57nyLt7LixbPeeSEj7hUEJpKkzW/Fm WyRVBjp5IRx/nfORkIB8sLI8hBJqSZArmXI2x/3EMVw9B7BLhREJIzGu6w189Cab8vie PT8LeimE+0s5AQVydZTLezmrvDOrxHH0Tehe6doNqv9q6kwQ9HJkpwgco8LU30Xo8y12 0NBcP/pqmrWALkCaprRW3cVGgJKAu1HBjRd2GhtH+Iuv0xFhQb+k5PnugA14UIPqH0R8 Gp9qXg7dtiXDb2quprYaa5Dk3F2PUEIU3ircOaoZN9Wko4fdp2Yd6KZOur8FC6n9Dypl vgFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=DlVNbxuhHiE0YW6IfxJk+i5c5jEODpePbQ6g9q+lpxA=; b=RAHncIc2PWgEAM0VyS9MyCKK61CuKlsuO0tqLHoWOrwnYyonKoVUuLS8x8lZ9RAmX4 PdgUswrVxyKmw5GMzOew1elnvtSpWPH3vvk1iliiFqSHQmlDdibFA17NVQnO807djpa6 Ush1Z9KiDgI74buP9zm5h+jAfUrHkZ4oERBNzSK71Ep1WwG8QyTKp1mT0NihAFRmkqDw jkVx06o65dtbKGUHdl5zcSVh1UPT9mtUjnMql0dV1+7gjdr/QHkom5fdMKZuhAeSBx7i 9rQll3d16RTANFmHYnJqB9CDd2sI+P5MNaloma/5Zgv715Eko80LP37A4cBnUZ7LKsUn KkwA== X-Gm-Message-State: AJIora934K32CrBI2WNVQrX78WAZQtDmrNW7pUQJ5Wdrtb3mNZtMNp/8 azszfwYTAfcyYRw5ewEIM6aUsQ== X-Google-Smtp-Source: AGRyM1suX5HCZoxqdbv5pRbmEJaW0Gk9ndmhHnqFYwBqop5sOUVwue3ket7abTy1gwO4u/a3nPw/mg== X-Received: by 2002:a05:6808:1a11:b0:33a:87d:d9c3 with SMTP id bk17-20020a0568081a1100b0033a087dd9c3mr211788oib.175.1657574522456; Mon, 11 Jul 2022 14:22:02 -0700 (PDT) Received: from eze-laptop ([190.190.187.68]) by smtp.gmail.com with ESMTPSA id q4-20020a9d6644000000b00616d98ad780sm3032840otm.52.2022.07.11.14.21.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Jul 2022 14:22:01 -0700 (PDT) Date: Mon, 11 Jul 2022 18:21:56 -0300 From: Ezequiel Garcia To: Jernej Skrabec Cc: mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org, samuel@sholland.org, mchehab@kernel.org, gregkh@linuxfoundation.org, hverkuil-cisco@xs4all.nl, 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 4/7] media: cedrus: Add error handling for failed setup Message-ID: References: <20220620175517.648767-1-jernej.skrabec@gmail.com> <20220620175517.648767-5-jernej.skrabec@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220620175517.648767-5-jernej.skrabec@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220711_142210_098067_E5B60F54 X-CRM114-Status: GOOD ( 27.90 ) 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 Hi Jernej, On Mon, Jun 20, 2022 at 07:55:14PM +0200, Jernej Skrabec wrote: > During decoding setup stage for complex codecs like HEVC driver can > detect inconsistent values in controls or some other task, like > allocating memory, can fail. > > Currently setup stage has no way of signalling error. Change return type > of setup callback to int and if returned value is not zero, skip > decoding and finish job immediately with error flag. > > While currently there is only one place when setup can fail, it's > expected that there will be more such cases in the future, when HEVC > decoding is improved. > > Signed-off-by: Jernej Skrabec Looks good and it's very typical to have a setup stage to put actions that can be allowed to fail. Reviewed-by: Ezequiel Garcia > --- > drivers/staging/media/sunxi/cedrus/cedrus.h | 2 +- > .../staging/media/sunxi/cedrus/cedrus_dec.c | 21 ++++++++++++++----- > .../staging/media/sunxi/cedrus/cedrus_h264.c | 5 +++-- > .../staging/media/sunxi/cedrus/cedrus_h265.c | 8 +++---- > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 4 +++- > .../staging/media/sunxi/cedrus/cedrus_vp8.c | 5 +++-- > 6 files changed, 30 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h > index 3bc094eb497f..d2b697a9ded2 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > @@ -162,7 +162,7 @@ struct cedrus_dec_ops { > void (*irq_clear)(struct cedrus_ctx *ctx); > void (*irq_disable)(struct cedrus_ctx *ctx); > enum cedrus_irq_status (*irq_status)(struct cedrus_ctx *ctx); > - void (*setup)(struct cedrus_ctx *ctx, struct cedrus_run *run); > + int (*setup)(struct cedrus_ctx *ctx, struct cedrus_run *run); > int (*start)(struct cedrus_ctx *ctx); > void (*stop)(struct cedrus_ctx *ctx); > void (*trigger)(struct cedrus_ctx *ctx); > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index aabe6253078e..b0944abaacbd 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -28,6 +28,7 @@ void cedrus_device_run(void *priv) > struct cedrus_dev *dev = ctx->dev; > struct cedrus_run run = {}; > struct media_request *src_req; > + int error; > > run.src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); > run.dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx); > @@ -89,16 +90,26 @@ void cedrus_device_run(void *priv) > > cedrus_dst_format_set(dev, &ctx->dst_fmt); > > - dev->dec_ops[ctx->current_codec]->setup(ctx, &run); > + error = dev->dec_ops[ctx->current_codec]->setup(ctx, &run); > + if (error) > + v4l2_err(&ctx->dev->v4l2_dev, > + "Failed to setup decoding job: %d\n", error); > > /* Complete request(s) controls if needed. */ > > if (src_req) > v4l2_ctrl_request_complete(src_req, &ctx->hdl); > > - dev->dec_ops[ctx->current_codec]->trigger(ctx); > + /* Trigger decoding if setup went well, bail out otherwise. */ > + if (!error) { > + dev->dec_ops[ctx->current_codec]->trigger(ctx); > > - /* Start the watchdog timer. */ > - schedule_delayed_work(&dev->watchdog_work, > - msecs_to_jiffies(2000)); > + /* Start the watchdog timer. */ > + schedule_delayed_work(&dev->watchdog_work, > + msecs_to_jiffies(2000)); > + } else { > + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, > + ctx->fh.m2m_ctx, > + VB2_BUF_STATE_ERROR); > + } > } > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index d8fb93035470..c345e67ba9bc 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -493,8 +493,7 @@ static void cedrus_h264_irq_disable(struct cedrus_ctx *ctx) > reg & ~VE_H264_CTRL_INT_MASK); > } > > -static void cedrus_h264_setup(struct cedrus_ctx *ctx, > - struct cedrus_run *run) > +static int cedrus_h264_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > { > struct cedrus_dev *dev = ctx->dev; > > @@ -510,6 +509,8 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx, > cedrus_write_frame_list(ctx, run); > > cedrus_set_params(ctx, run); > + > + return 0; > } > > static int cedrus_h264_start(struct cedrus_ctx *ctx) > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > index 46119912c387..cfde4ccf6011 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > @@ -326,8 +326,7 @@ static int cedrus_h265_is_low_delay(struct cedrus_run *run) > return 0; > } > > -static void cedrus_h265_setup(struct cedrus_ctx *ctx, > - struct cedrus_run *run) > +static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > { > struct cedrus_dev *dev = ctx->dev; > const struct v4l2_ctrl_hevc_sps *sps; > @@ -385,8 +384,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx, > GFP_KERNEL, DMA_ATTR_NO_KERNEL_MAPPING); > if (!ctx->codec.h265.mv_col_buf) { > ctx->codec.h265.mv_col_buf_size = 0; > - // TODO: Abort the process here. > - return; > + return -ENOMEM; > } > } > > @@ -703,6 +701,8 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx, > > /* Enable appropriate interruptions. */ > cedrus_write(dev, VE_DEC_H265_CTRL, VE_DEC_H265_CTRL_IRQ_MASK); > + > + return 0; > } > > static int cedrus_h265_start(struct cedrus_ctx *ctx) > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > index 5dad2f296c6d..4cfc4a3c8a7f 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > @@ -48,7 +48,7 @@ static void cedrus_mpeg2_irq_disable(struct cedrus_ctx *ctx) > cedrus_write(dev, VE_DEC_MPEG_CTRL, reg); > } > > -static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > +static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > { > const struct v4l2_ctrl_mpeg2_sequence *seq; > const struct v4l2_ctrl_mpeg2_picture *pic; > @@ -185,6 +185,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > VE_DEC_MPEG_CTRL_MC_CACHE_EN; > > cedrus_write(dev, VE_DEC_MPEG_CTRL, reg); > + > + return 0; > } > > static void cedrus_mpeg2_trigger(struct cedrus_ctx *ctx) > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > index f4016684b32d..3f750d1795b6 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c > @@ -651,8 +651,7 @@ static void cedrus_vp8_irq_disable(struct cedrus_ctx *ctx) > reg & ~VE_H264_CTRL_INT_MASK); > } > > -static void cedrus_vp8_setup(struct cedrus_ctx *ctx, > - struct cedrus_run *run) > +static int cedrus_vp8_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > { > const struct v4l2_ctrl_vp8_frame *slice = run->vp8.frame_params; > struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; > @@ -855,6 +854,8 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx, > ctx->codec.vp8.last_sharpness_level = > slice->lf.sharpness_level; > } > + > + return 0; > } > > static int cedrus_vp8_start(struct cedrus_ctx *ctx) > -- > 2.36.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel