[{"id":16865,"web_url":"https://patchwork.libcamera.org/comment/16865/","msgid":"<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>","date":"2021-05-10T20:28:16","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:\n> Capture requests are queued by the PipelineHandler base class to each\n> pipeline handler implementation using the virtual queueRequestDevice()\n> function.\n> \n> However, if the pipeline handler fails to queue the request to the\n> hardware, the request gets silently deleted from the list of queued\n> ones, without notifying application of the error.\n> \n> Allowing applications to know if a Request has failed to queue\n> by emitting the Camera::bufferCompleted and Camera::requestCompleted\n> signals allows them to maintain a state consistent with the one\n> internal to the library.\n> \n> To do so, if queuing a request to the device fails, cancel the request\n> and emit the completion signal to report the failure to applications.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-\n>  1 file changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index f41b7a7b3308..260a52018a4d 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n>   * This method queues a capture request to the pipeline handler for processing.\n>   * The request is first added to the internal list of queued requests, and\n>   * then passed to the pipeline handler with a call to queueRequestDevice().\n> + * If the pipeline handler fails in queuing the request to the hardware the\n> + * request is immediately completed with error.\n>   *\n>   * Keeping track of queued requests ensures automatic completion of all requests\n>   * when the pipeline handler is stopped with stop(). Request completion shall be\n> @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)\n>  \trequest->sequence_ = data->requestSequence_++;\n>  \n>  \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\t/*\n> +\t\t * Cancel the request and notify completion of its buffers in\n> +\t\t * error state. Then complete the cancelled request and remove\n> +\t\t * it from the queued requests list.\n> +\t\t */\n> +\t\trequest->cancel();\n> +\t\tfor (auto bufferMap : request->buffers())\n> +\t\t\tcamera->bufferCompleted.emit(request, bufferMap.second);\n\nI wonder if the emitting of bufferCompleted should be moved to \nCamera::requestComplete()? This would be a common thing for all requests \nthat completes in the cancel state right?\n\n> +\n> +\t\tcamera->requestComplete(request);\n>  \t\tdata->queuedRequests_.remove(request);\n> +\t}\n>  }\n>  \n>  /**\n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4A7CCBF829\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 May 2021 20:28:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BDAD6891A;\n\tMon, 10 May 2021 22:28:19 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCC4F602BE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 May 2021 22:28:18 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id b21so22373989ljf.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 May 2021 13:28:18 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\th12sm2398758lfc.240.2021.05.10.13.28.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 10 May 2021 13:28:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"tczwdk1x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Icc9FNLvAo8+Mc3tX32ZdX48hMTBgsCClHCerp31IKw=;\n\tb=tczwdk1xQ1VvB02VBvmK+r7aYaCJ5jbiccKUX2iSnhfTwgTphrnFiM9CXc2wXjLUsW\n\t9NCDgt9ahOPn2XFu0jq4M/5Ak7UbGoK4WMvgawIxM1VF5y51qCjkBJxyRxjd8yNgVWzm\n\tUwjwc5NzgL/pYEZjNwUl5mVTRvjqP1W9+fdtwZPd6Lghu6mOojTC/ae6gjHKIPTp7eCa\n\tW0x2l55J6qQcWVePgP5tbgsUqD7qjTEZ/pdRoeEF2vT23ttpxgZ6YsuMFV9QzagBgoiP\n\tEQT6M+zMUC+fLs5iLbhLcbgsdfyyD+VNoj1AkAx/ipcZO5zKaiNFJRuozLo2QHCMcWCS\n\tYZsA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Icc9FNLvAo8+Mc3tX32ZdX48hMTBgsCClHCerp31IKw=;\n\tb=XI/ut9lKw8w9sgiv/BgycW1uRPPR3Ls6DawzpLlo6H6u0S4fDjsCjLt/s7Q5HMzijU\n\tVX/o/EWu2HQPys+q8ltdG66CpwCNNwktC6v/p3MZJIFuYgfsSr8ONJ3A2w4AR6lcCKOj\n\tXV1PFqVeRm8AuTmHDgmFwRepfU+RAEVurwKwYrZeuAAUn7T63I/b6xUFtEA7LOPKuz4E\n\tdCCTBLb/uAPUkROUtV7rKR9RoymSyo7PklANMdqrG6NSEc75BLb1HhOzX6r1TCOt3Dmp\n\tcekR0arok3yMVi9Vg5uEw+KVaK4k6HdcS8VsPYjxLFjFLeYKStQn0qZg8wxWj9894V6M\n\tm6+A==","X-Gm-Message-State":"AOAM532EctLaICP4enyAil7W3DZqP6CojYKkkEp8Z/PGEuWgrEaB+Cvf\n\tudFDkGoqy+8oThtye/fzS+5Tzw==","X-Google-Smtp-Source":"ABdhPJxN5TQoBo/jEx1bFRQ70XyXZQQ7tSwdzleWzB6lrbYPLEYToH28y/ai2HfuV34XoNkWo0vxnw==","X-Received":"by 2002:a2e:8e9a:: with SMTP id\n\tz26mr14623814ljk.301.1620678498131; \n\tMon, 10 May 2021 13:28:18 -0700 (PDT)","Date":"Mon, 10 May 2021 22:28:16 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210510105235.28319-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16880,"web_url":"https://patchwork.libcamera.org/comment/16880/","msgid":"<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>","date":"2021-05-11T05:00:32","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch.\n\nOn Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:\n> > Capture requests are queued by the PipelineHandler base class to each\n> > pipeline handler implementation using the virtual queueRequestDevice()\n> > function.\n> >\n> > However, if the pipeline handler fails to queue the request to the\n> > hardware, the request gets silently deleted from the list of queued\n> > ones, without notifying application of the error.\n> >\n> > Allowing applications to know if a Request has failed to queue\n> > by emitting the Camera::bufferCompleted and Camera::requestCompleted\n> > signals allows them to maintain a state consistent with the one\n> > internal to the library.\n> >\n> > To do so, if queuing a request to the device fails, cancel the request\n> > and emit the completion signal to report the failure to applications.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-\n> >  1 file changed, 14 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> > index f41b7a7b3308..260a52018a4d 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const\n> Camera *camera) const\n> >   * This method queues a capture request to the pipeline handler for\n> processing.\n> >   * The request is first added to the internal list of queued requests,\n> and\n> >   * then passed to the pipeline handler with a call to\n> queueRequestDevice().\n> > + * If the pipeline handler fails in queuing the request to the hardware\n> the\n> > + * request is immediately completed with error.\n> >   *\n> >   * Keeping track of queued requests ensures automatic completion of all\n> requests\n> >   * when the pipeline handler is stopped with stop(). Request completion\n> shall be\n> > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)\n> >       request->sequence_ = data->requestSequence_++;\n> >\n> >       int ret = queueRequestDevice(camera, request);\n> > -     if (ret)\n> > +     if (ret) {\n> > +             /*\n> > +              * Cancel the request and notify completion of its buffers\n> in\n> > +              * error state. Then complete the cancelled request and\n> remove\n> > +              * it from the queued requests list.\n> > +              */\n> > +             request->cancel();\n> > +             for (auto bufferMap : request->buffers())\n> > +                     camera->bufferCompleted.emit(request,\n> bufferMap.second);\n>\n> I wonder if the emitting of bufferCompleted should be moved to\n> Camera::requestComplete()? This would be a common thing for all requests\n> that completes in the cancel state right?\n>\n>\nI think this should be done in Request::cancel().\n\n\n> > +\n> > +             camera->requestComplete(request);\n> >               data->queuedRequests_.remove(request);\n>\n\nI wonder if we should call completeRequest() to keep the request\nreturn order same as the request queue order.\n\nBy the way, a discussion is needed about how to report the error code. cf.)\nhttps://patchwork.libcamera.org/patch/11764/.\nBut this patch can go without it.\n\n> +     }\n> >  }\n> >\n> >  /**\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 25504BF829\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 11 May 2021 05:00:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F9AD6891C;\n\tTue, 11 May 2021 07:00:44 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B9E8968915\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 11 May 2021 07:00:43 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id y26so21296148eds.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 May 2021 22:00:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PK2DGAMN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=M4M4zaVAqUHFrp7cM8FhuOuU8z+DCBalU2lD2ZmDShU=;\n\tb=PK2DGAMNtmxveghey15t1+fn5hBPwSRwCtnc6NiTTQWRDJExQXYOs20RHxYfxvvuEA\n\tXleNRTK0owvSTHQXUUhibbtBS53PRxD0YHkqHx2yMnrVZo/sOtmOjj9r1ULAi/VPJGPU\n\tE9epRWwb4tCOxR1cLc/JPSKjkFwnGDOkJYvj8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=M4M4zaVAqUHFrp7cM8FhuOuU8z+DCBalU2lD2ZmDShU=;\n\tb=G5/7/CuHawtTLC4Vw0kBSf5LuipyP7YYBo+T1B4jUpPOSX+uGUhrjBFrHrviCrEMM/\n\tA4+99Z+NP4sZFobLPWTTKjQXxiEcTx026JTRE39Qq7O6Fbko7cD1bWcbVp8Mbt0NzOBv\n\tAnEC2SzH0adDGTKSo4eX3s1s7YV/nNSdUL/K3GGYyugpLIkVCoXohk7QL6IdUHzGiUip\n\tSK7L5b4NPGujq5722CURDMcTmQXukjCjl6Z4YemCd7g3Ps21xOSNBwozFq8mVFDYrClH\n\tbcnTvf5sGmoX90oWq+Nau1ha2LY4Z9qV5d3dRHcIlRtatquD83b1YXGD4nPH/GG3sKgY\n\ttRhg==","X-Gm-Message-State":"AOAM53006utYlDflaF4zhK760Md+nbF5XdYf5zEOSwk30SQu8nB1hIlV\n\tBB4Ky9jzdC21uwtPdZC+NaitMbWcXcUx97JQxjKO9FmFvU8=","X-Google-Smtp-Source":"ABdhPJxBuVw764R9qmRXT9h1CXgEwOrLWoPa++PT4aVzUqLI6ISoe9e82G1L1RBxHcXCAOublaqjwae07ztyeEYuPZ0=","X-Received":"by 2002:aa7:c610:: with SMTP id\n\th16mr33233217edq.202.1620709243435; \n\tMon, 10 May 2021 22:00:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-4-jacopo@jmondi.org>\n\t<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>","In-Reply-To":"<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Tue, 11 May 2021 14:00:32 +0900","Message-ID":"<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============5188601263195922128==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16917,"web_url":"https://patchwork.libcamera.org/comment/16917/","msgid":"<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","date":"2021-05-12T09:36:32","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch.\n>\n> On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your patch.\n> >\n> > On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:\n> > > Capture requests are queued by the PipelineHandler base class to each\n> > > pipeline handler implementation using the virtual queueRequestDevice()\n> > > function.\n> > >\n> > > However, if the pipeline handler fails to queue the request to the\n> > > hardware, the request gets silently deleted from the list of queued\n> > > ones, without notifying application of the error.\n> > >\n> > > Allowing applications to know if a Request has failed to queue\n> > > by emitting the Camera::bufferCompleted and Camera::requestCompleted\n> > > signals allows them to maintain a state consistent with the one\n> > > internal to the library.\n> > >\n> > > To do so, if queuing a request to the device fails, cancel the request\n> > > and emit the completion signal to report the failure to applications.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-\n> > >  1 file changed, 14 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > b/src/libcamera/pipeline_handler.cpp\n> > > index f41b7a7b3308..260a52018a4d 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const\n> > Camera *camera) const\n> > >   * This method queues a capture request to the pipeline handler for\n> > processing.\n> > >   * The request is first added to the internal list of queued requests,\n> > and\n> > >   * then passed to the pipeline handler with a call to\n> > queueRequestDevice().\n> > > + * If the pipeline handler fails in queuing the request to the hardware\n> > the\n> > > + * request is immediately completed with error.\n> > >   *\n> > >   * Keeping track of queued requests ensures automatic completion of all\n> > requests\n> > >   * when the pipeline handler is stopped with stop(). Request completion\n> > shall be\n> > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)\n> > >       request->sequence_ = data->requestSequence_++;\n> > >\n> > >       int ret = queueRequestDevice(camera, request);\n> > > -     if (ret)\n> > > +     if (ret) {\n> > > +             /*\n> > > +              * Cancel the request and notify completion of its buffers\n> > in\n> > > +              * error state. Then complete the cancelled request and\n> > remove\n> > > +              * it from the queued requests list.\n> > > +              */\n> > > +             request->cancel();\n> > > +             for (auto bufferMap : request->buffers())\n> > > +                     camera->bufferCompleted.emit(request,\n> > bufferMap.second);\n> >\n> > I wonder if the emitting of bufferCompleted should be moved to\n> > Camera::requestComplete()? This would be a common thing for all requests\n> > that completes in the cancel state right?\n> >\n> >\n> I think this should be done in Request::cancel().\n\nI don't think Camera::requestComplete() is the right place where to\nemit the buffer completion signal, as those are two separate actions.\nIf we want to emit buffer complete for cancelled buffers we should\nmake Camera::requestComplete() inspect the status of the request,\nsomething that doesn't seem to belong there.\n\nOTOH emitting the buffer complete signal in Request::cancel() might be\na better fit. Although this opens a whole new problem, which is the\none of partial requests completion. If we allow requests to complete\nby pieces, we might want to allow ph to complete buffers which have\nbeen succesfull singularly, but signal that a request has been\ncancelled and has failed because not -all- of them have been\ncompleted.\n\nIn this specific case, where the request is cancelled because it has\nfailed to queue it would make sense to emit buffers completion signal\nin Request::cancel() : no buffers have been queued, so none of them\ncan be completed.\n\nIn the case the Request is cancelled later, by the ph, after one of\nthe buffers has failed but other ones have completed, the ph should be\nin charge to emit buffer completion signals with the single buffers\nstatus.\n\nIOW, I would for the time being, not tie cancelling the request to\nemit buffer completion. True, Request::cancel() sets all buffers state\nto error, but before this happen a buffer can be completed succesfully\nand application could have had access to it.\n\n>\n>\n> > > +\n> > > +             camera->requestComplete(request);\n> > >               data->queuedRequests_.remove(request);\n> >\n>\n> I wonder if we should call completeRequest() to keep the request\n> return order same as the request queue order.\n\nGood question, this has direct consequences on the libcamera API.\n\nIf queuing a Request fails, do we want to be notified immediately\nabout it, or let the requests that have been queued before that one\ncomplete first ?\n\nThe typical application I can think of will re-use and re-queue a Request\nimmediately in the request completion handler. If we delay reporting\nthe queueing error there is a risk multiple requests are queued after\nthe one that has failed. If we fail immediately, the application can\nhandle the error condition immediately, and stop the re-queuing\nmechanism.\n\nLaurent, what's your take here ?\n\n>\n> By the way, a discussion is needed about how to report the error code. cf.)\n> https://patchwork.libcamera.org/patch/11764/.\n> But this patch can go without it.\n>\n> > +     }\n> > >  }\n> > >\n> > >  /**\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7FF4DC31E4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 12 May 2021 09:35:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEBD468920;\n\tWed, 12 May 2021 11:35:51 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DA56688E4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 May 2021 11:35:50 +0200 (CEST)","from uno.localdomain (host-79-50-130-20.retail.telecomitalia.it\n\t[79.50.130.20]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id EC8A460007;\n\tWed, 12 May 2021 09:35:48 +0000 (UTC)"],"X-Originating-IP":"79.50.130.20","Date":"Wed, 12 May 2021 11:36:32 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-4-jacopo@jmondi.org>\n\t<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>\n\t<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16923,"web_url":"https://patchwork.libcamera.org/comment/16923/","msgid":"<CAO5uPHPZfFB1nX_TJ11ngCymrN9ZZ_J6cBmA+UERVZmia6MTdw@mail.gmail.com>","date":"2021-05-13T02:37:23","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Wed, May 12, 2021 at 6:35 PM Jacopo Mondi <jacopo@jmondi.org> wrote:\n\n> Hello,\n>\n> On Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the patch.\n> >\n> > On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your patch.\n> > >\n> > > On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:\n> > > > Capture requests are queued by the PipelineHandler base class to each\n> > > > pipeline handler implementation using the virtual\n> queueRequestDevice()\n> > > > function.\n> > > >\n> > > > However, if the pipeline handler fails to queue the request to the\n> > > > hardware, the request gets silently deleted from the list of queued\n> > > > ones, without notifying application of the error.\n> > > >\n> > > > Allowing applications to know if a Request has failed to queue\n> > > > by emitting the Camera::bufferCompleted and Camera::requestCompleted\n> > > > signals allows them to maintain a state consistent with the one\n> > > > internal to the library.\n> > > >\n> > > > To do so, if queuing a request to the device fails, cancel the\n> request\n> > > > and emit the completion signal to report the failure to applications.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > ---\n> > > >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-\n> > > >  1 file changed, 14 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > b/src/libcamera/pipeline_handler.cpp\n> > > > index f41b7a7b3308..260a52018a4d 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const\n> > > Camera *camera) const\n> > > >   * This method queues a capture request to the pipeline handler for\n> > > processing.\n> > > >   * The request is first added to the internal list of queued\n> requests,\n> > > and\n> > > >   * then passed to the pipeline handler with a call to\n> > > queueRequestDevice().\n> > > > + * If the pipeline handler fails in queuing the request to the\n> hardware\n> > > the\n> > > > + * request is immediately completed with error.\n> > > >   *\n> > > >   * Keeping track of queued requests ensures automatic completion of\n> all\n> > > requests\n> > > >   * when the pipeline handler is stopped with stop(). Request\n> completion\n> > > shall be\n> > > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request\n> *request)\n> > > >       request->sequence_ = data->requestSequence_++;\n> > > >\n> > > >       int ret = queueRequestDevice(camera, request);\n> > > > -     if (ret)\n> > > > +     if (ret) {\n> > > > +             /*\n> > > > +              * Cancel the request and notify completion of its\n> buffers\n> > > in\n> > > > +              * error state. Then complete the cancelled request and\n> > > remove\n> > > > +              * it from the queued requests list.\n> > > > +              */\n> > > > +             request->cancel();\n> > > > +             for (auto bufferMap : request->buffers())\n> > > > +                     camera->bufferCompleted.emit(request,\n> > > bufferMap.second);\n> > >\n> > > I wonder if the emitting of bufferCompleted should be moved to\n> > > Camera::requestComplete()? This would be a common thing for all\n> requests\n> > > that completes in the cancel state right?\n> > >\n> > >\n> > I think this should be done in Request::cancel().\n>\n> I don't think Camera::requestComplete() is the right place where to\n> emit the buffer completion signal, as those are two separate actions.\n> If we want to emit buffer complete for cancelled buffers we should\n> make Camera::requestComplete() inspect the status of the request,\n> something that doesn't seem to belong there.\n>\n> OTOH emitting the buffer complete signal in Request::cancel() might be\n> a better fit. Although this opens a whole new problem, which is the\n> one of partial requests completion. If we allow requests to complete\n> by pieces, we might want to allow ph to complete buffers which have\n> been succesfull singularly, but signal that a request has been\n> cancelled and has failed because not -all- of them have been\n> completed.\n>\n> In this specific case, where the request is cancelled because it has\n> failed to queue it would make sense to emit buffers completion signal\n> in Request::cancel() : no buffers have been queued, so none of them\n> can be completed.\n>\n> In the case the Request is cancelled later, by the ph, after one of\n> the buffers has failed but other ones have completed, the ph should be\n> in charge to emit buffer completion signals with the single buffers\n> status.\n>\n> IOW, I would for the time being, not tie cancelling the request to\n> emit buffer completion. True, Request::cancel() sets all buffers state\n> to error, but before this happen a buffer can be completed succesfully\n> and application could have had access to it.\n>\n>\nSounds good to me.\n\n\n> >\n> >\n> > > > +\n> > > > +             camera->requestComplete(request);\n> > > >               data->queuedRequests_.remove(request);\n> > >\n> >\n> > I wonder if we should call completeRequest() to keep the request\n> > return order same as the request queue order.\n>\n> Good question, this has direct consequences on the libcamera API.\n>\n> If queuing a Request fails, do we want to be notified immediately\n> about it, or let the requests that have been queued before that one\n> complete first ?\n>\n> The typical application I can think of will re-use and re-queue a Request\n> immediately in the request completion handler. If we delay reporting\n> the queueing error there is a risk multiple requests are queued after\n> the one that has failed. If we fail immediately, the application can\n> handle the error condition immediately, and stop the re-queuing\n> mechanism.\n>\n> Laurent, what's your take here ?\n>\n> >\n> > By the way, a discussion is needed about how to report the error code.\n> cf.)\n> > https://patchwork.libcamera.org/patch/11764/.\n> > But this patch can go without it.\n> >\n> > > +     }\n> > > >  }\n> > > >\n> > > >  /**\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8451AC31E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 May 2021 02:37:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4E426891D;\n\tThu, 13 May 2021 04:37:35 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B91D61538\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 May 2021 04:37:34 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id k10so6640169ejj.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 May 2021 19:37:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"K7pNiqDR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=NrKOu8E3QST/RWt2eMffzTxmuMAtt92RgVqdRSo6BqE=;\n\tb=K7pNiqDRBx66PWkWZZXyNrkpKBYuLQImU7ZxRejvdYWQtSE8phHWW7R63WqhMQL/Yr\n\t7Li5hCpV3PxIlt4ON8QP+73rwqI0qBov7MmyJyOwfqc5tZo9rCdg/mN8OfLY0m1Fuezp\n\tRo+W5yHxkoyY3D0lISMgMpCL9XnmvUFj7huQQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NrKOu8E3QST/RWt2eMffzTxmuMAtt92RgVqdRSo6BqE=;\n\tb=aCasuOmjmV/L257U7i8QzRh8kWQiq15CbalsvixNFJIZy7uVMnLD45TZrWKbRei+Ev\n\t8ijSHqpNg7hNuh3ZhtFSuzF4K45fh4gp0HVtiqd+iLjcckNOMfkZUk7IHf0G58orIW/f\n\te9EKXTGsV8QCf6tZv4+omyxV7kMKuXaV1GJRufiGz+86WWBA7PfS7WJ5ggxM0hr3WNA6\n\t0AxCaz5LnPv/WI0FB3Mo75w3Az8/CCTxS54m1sQk30mB5jjnJcs1XaPTvXeEsyDX6Lwb\n\tVT5g5nFCviGyzhSeiIh2ju74zGlJfC0/a3WpHQ9uZcUIWy/I6JrT9Gj7eJoSeBe13UVY\n\t3P4g==","X-Gm-Message-State":"AOAM532oKzq6vVWRsl8LSGdEUvKKQJgxYkpTmgOtjRfUZMKqkdZJ9GgJ\n\tcP05rUstriY3OoU0BihatyshoLQ2ZiSLPnigpquUKA==","X-Google-Smtp-Source":"ABdhPJzAU8x0iZ7ebYFXswmXBmlALAalTAvLF6hPXuD9IzB+Hhut/bFl+wbxnWxjtU3XBHd9os+PixQSnrPc/AN/C+0=","X-Received":"by 2002:a17:906:55c5:: with SMTP id\n\tz5mr40341667ejp.306.1620873453859; \n\tWed, 12 May 2021 19:37:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-4-jacopo@jmondi.org>\n\t<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>\n\t<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>\n\t<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","In-Reply-To":"<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Thu, 13 May 2021 11:37:23 +0900","Message-ID":"<CAO5uPHPZfFB1nX_TJ11ngCymrN9ZZ_J6cBmA+UERVZmia6MTdw@mail.gmail.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============8396168941604758969==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16980,"web_url":"https://patchwork.libcamera.org/comment/16980/","msgid":"<YKIa/VXi+BPBaoZD@pendragon.ideasonboard.com>","date":"2021-05-17T07:27:57","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, May 12, 2021 at 11:36:32AM +0200, Jacopo Mondi wrote:\n> On Tue, May 11, 2021 at 02:00:32PM +0900, Hirokazu Honda wrote:\n> > On Tue, May 11, 2021 at 5:28 AM Niklas Söderlund wrote:\n> > > On 2021-05-10 12:52:30 +0200, Jacopo Mondi wrote:\n> > > > Capture requests are queued by the PipelineHandler base class to each\n> > > > pipeline handler implementation using the virtual queueRequestDevice()\n> > > > function.\n> > > >\n> > > > However, if the pipeline handler fails to queue the request to the\n> > > > hardware, the request gets silently deleted from the list of queued\n> > > > ones, without notifying application of the error.\n> > > >\n> > > > Allowing applications to know if a Request has failed to queue\n> > > > by emitting the Camera::bufferCompleted and Camera::requestCompleted\n> > > > signals allows them to maintain a state consistent with the one\n> > > > internal to the library.\n> > > >\n> > > > To do so, if queuing a request to the device fails, cancel the request\n> > > > and emit the completion signal to report the failure to applications.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > ---\n> > > >  src/libcamera/pipeline_handler.cpp | 15 ++++++++++++++-\n> > > >  1 file changed, 14 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index f41b7a7b3308..260a52018a4d 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > > >   * This method queues a capture request to the pipeline handler for processing.\n> > > >   * The request is first added to the internal list of queued requests, and\n> > > >   * then passed to the pipeline handler with a call to queueRequestDevice().\n> > > > + * If the pipeline handler fails in queuing the request to the hardware the\n> > > > + * request is immediately completed with error.\n> > > >   *\n> > > >   * Keeping track of queued requests ensures automatic completion of all requests\n> > > >   * when the pipeline handler is stopped with stop(). Request completion shall be\n> > > > @@ -409,8 +411,19 @@ void PipelineHandler::queueRequest(Request *request)\n> > > >       request->sequence_ = data->requestSequence_++;\n> > > >\n> > > >       int ret = queueRequestDevice(camera, request);\n> > > > -     if (ret)\n> > > > +     if (ret) {\n> > > > +             /*\n> > > > +              * Cancel the request and notify completion of its buffers in\n> > > > +              * error state. Then complete the cancelled request and remove\n> > > > +              * it from the queued requests list.\n> > > > +              */\n> > > > +             request->cancel();\n> > > > +             for (auto bufferMap : request->buffers())\n> > > > +                     camera->bufferCompleted.emit(request, bufferMap.second);\n> > >\n> > > I wonder if the emitting of bufferCompleted should be moved to\n> > > Camera::requestComplete()? This would be a common thing for all requests\n> > > that completes in the cancel state right?\n> >\n> > I think this should be done in Request::cancel().\n> \n> I don't think Camera::requestComplete() is the right place where to\n> emit the buffer completion signal, as those are two separate actions.\n> If we want to emit buffer complete for cancelled buffers we should\n> make Camera::requestComplete() inspect the status of the request,\n> something that doesn't seem to belong there.\n> \n> OTOH emitting the buffer complete signal in Request::cancel() might be\n> a better fit. Although this opens a whole new problem, which is the\n> one of partial requests completion. If we allow requests to complete\n> by pieces, we might want to allow ph to complete buffers which have\n> been succesfull singularly, but signal that a request has been\n> cancelled and has failed because not -all- of them have been\n> completed.\n> \n> In this specific case, where the request is cancelled because it has\n> failed to queue it would make sense to emit buffers completion signal\n> in Request::cancel() : no buffers have been queued, so none of them\n> can be completed.\n> \n> In the case the Request is cancelled later, by the ph, after one of\n> the buffers has failed but other ones have completed, the ph should be\n> in charge to emit buffer completion signals with the single buffers\n> status.\n> \n> IOW, I would for the time being, not tie cancelling the request to\n> emit buffer completion. True, Request::cancel() sets all buffers state\n> to error, but before this happen a buffer can be completed succesfully\n> and application could have had access to it.\n> \n> > > > +\n> > > > +             camera->requestComplete(request);\n> > > >               data->queuedRequests_.remove(request);\n> >\n> > I wonder if we should call completeRequest() to keep the request\n> > return order same as the request queue order.\n> \n> Good question, this has direct consequences on the libcamera API.\n> \n> If queuing a Request fails, do we want to be notified immediately\n> about it, or let the requests that have been queued before that one\n> complete first ?\n> \n> The typical application I can think of will re-use and re-queue a Request\n> immediately in the request completion handler. If we delay reporting\n> the queueing error there is a risk multiple requests are queued after\n> the one that has failed. If we fail immediately, the application can\n> handle the error condition immediately, and stop the re-queuing\n> mechanism.\n> \n> Laurent, what's your take here ?\n\nBreaking the ordering guarantee is a really big change. I'm not\nconvinced at this point that it would be worth it. We can still consider\nit, but it should then be considered very carefully, with all its\nimplications. It shouldn't be part of this series.\n\n> > By the way, a discussion is needed about how to report the error code. cf.)\n> > https://patchwork.libcamera.org/patch/11764/.\n> > But this patch can go without it.\n> >\n> > > +     }\n> > > >  }\n> > > >\n> > > >  /**","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B1BD9C31FC\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 May 2021 07:28:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2885D6891E;\n\tMon, 17 May 2021 09:28:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7A95602B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 May 2021 09:28:08 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4FE3488F;\n\tMon, 17 May 2021 09:28:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"q7JuUJjL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621236488;\n\tbh=TUY/kttd3qkBfarKaQZaI8ecA49b6hb7ChDfJ5+jYLA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q7JuUJjLh5+2PxAGSxE3TqfBcG2M1y26nf+RO/lQff63ST9rRF0q/Mz/JdMi3Yw46\n\tMKGqHgwJUrmNPP91oJy7+EqWpPGV/DO7jRpqmiuBsgIhbyPvSDc8FWDnlLdRDuUbuM\n\tr/grWp0lZI5Z/J9eimUGBnrGWY7xHOBJdNar3tJE=","Date":"Mon, 17 May 2021 10:27:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YKIa/VXi+BPBaoZD@pendragon.ideasonboard.com>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-4-jacopo@jmondi.org>\n\t<YJmXYFoU2jnLr61Z@oden.dyn.berto.se>\n\t<CAO5uPHMqOf+nFip4h1D47p99QoXdiUQC6zX2Yib4Jgn5uTn5QQ@mail.gmail.com>\n\t<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210512093632.o457oazh5r7lqdrt@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera: pipeline_handler:\n\tNotify Request queueing failure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]