[{"id":16963,"web_url":"https://patchwork.libcamera.org/comment/16963/","msgid":"<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>","date":"2021-05-17T02:45:04","subject":"Re: [libcamera-devel] [PATCH v2 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\nThank you for the patch.\n\nOn Thu, May 13, 2021 at 11:22:41AM +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> +\n> +\t\tcamera->requestComplete(request);\n\nThis breaks the request completion ordering. I think you could just call\nPipelineHandler::completeRequest() instead.\n\n>  \t\tdata->queuedRequests_.remove(request);\n> +\t}\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 9CB69C31FB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 May 2021 02:45:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 986EF6891E;\n\tMon, 17 May 2021 04:45:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D24C16890E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 May 2021 04:45:15 +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 26CC488F;\n\tMon, 17 May 2021 04:45:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ETqvBKGK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621219515;\n\tbh=YscUpVUKTnGM+7CFM9t/rvbRPvI5U/TxhfAu76kqvac=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ETqvBKGKDEx8Fn0f4PGfrmmnnnxQCuiMDeaHpFHhkkNp+uVS7oGnHNz6Rl1aX2Po2\n\tF0p3l42LrEWvEodgnQdfnUynbQwFOvTfqrlnQXD3WAotONbJGzPDY/tyrUm/V4js2Z\n\txVg5MgooZS3G7xBjlDnHGU7Hl3QHb/o9q5uSy3RQ=","Date":"Mon, 17 May 2021 05:45:04 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>","References":"<20210513092246.42847-1-jacopo@jmondi.org>\n\t<20210513092246.42847-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210513092246.42847-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16974,"web_url":"https://patchwork.libcamera.org/comment/16974/","msgid":"<CAO5uPHPndMwurjeDHnQ_JYRG3OkmMCZnBi-UVnsJ4T6ox5B7vg@mail.gmail.com>","date":"2021-05-17T04:48:59","subject":"Re: [libcamera-devel] [PATCH v2 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 and Laurent,\n\nOn Mon, May 17, 2021 at 11:45 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, May 13, 2021 at 11:22:41AM +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> > +             camera->requestComplete(request);\n>\n> This breaks the request completion ordering. I think you could just call\n> PipelineHandler::completeRequest() instead.\n>\n\nI had the same comment in the previous version.\nhttps://patchwork.libcamera.org/patch/12243/\nWe were waiting for your reply. Alright we got the reply now.\n\n\n\n> >               data->queuedRequests_.remove(request);\n> > +     }\n> >  }\n> >\n> >  /**\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 ED89DC31FB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 May 2021 04:49:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45EE36891E;\n\tMon, 17 May 2021 06:49:14 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04D74602B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 May 2021 06:49:13 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id a25so5195146edr.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 May 2021 21:49:12 -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=\"WiTabe+v\"; 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=4Nvt+GdGlbZrJ8utAvxPX0GKJMq4d71KoA5SbkfDJWw=;\n\tb=WiTabe+vYx0K/hG5OZerSsByWUOGIYKE1aqUopsWwivCVv4s5JjkG0UTrz196KjbBz\n\tfIG+vkvDlC0Co4n2aqSO63pW8tKHUafRdEZx7uyQUt8JqDWNQ/azrSmfRH6BulgP7+p2\n\tPCvCGSAZ17ATs76wGqIpIOdu80THdrAk9BFqE=","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=4Nvt+GdGlbZrJ8utAvxPX0GKJMq4d71KoA5SbkfDJWw=;\n\tb=qzT2YVCPdGEqK7I88Sv65a1n6nfmeRWY4An4wZ4MALGztYg9v4PGg5XLDkQ/j8CzaA\n\tDCW6ECJOnRInEKxqs/Xbg7JTYTXcO9Ta0VzXMDdEkOTsTqvcpxcv5ptmHy34zayBJXxm\n\tP4Y4b0wolxDEuJg58VvfJY5bkLABfCkMw2PfKW5gRby8FQceDDsjdjmha1eXWaRm6sts\n\t3GRiJ5SCMH/ZrXicx866XAphbUuT/pCoyRWdfP2S0eoSCyjeYKkCpxUoeu8hVYdIY9Pj\n\ttZni0s5ZBzdKqSo97jBQcSLG6+hzUd6xIOvu+lMdgI881KDDjSZGnPv+ZR9wu6lhwnr2\n\tpNxQ==","X-Gm-Message-State":"AOAM5313sGYq7EyMHQteWjFR6urvCXhVxnusetOT6PVSQj1eDzLd7U5h\n\tSwHO5sipnWkrus1nWcx9wT/dYv3g2bprzQ0HAopBr75YYjVYiw==","X-Google-Smtp-Source":"ABdhPJyUz+jrFHwG5eD9G+88wmRfbcg03uCUmCcpISrc27K0ApY/RoFXjIGvgs92gDYzA1Hr6m11HndjZeWruopGIfs=","X-Received":"by 2002:a50:8e44:: with SMTP id 4mr69500056edx.244.1621226952574;\n\tSun, 16 May 2021 21:49:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210513092246.42847-1-jacopo@jmondi.org>\n\t<20210513092246.42847-4-jacopo@jmondi.org>\n\t<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>","In-Reply-To":"<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Mon, 17 May 2021 13:48:59 +0900","Message-ID":"<CAO5uPHPndMwurjeDHnQ_JYRG3OkmMCZnBi-UVnsJ4T6ox5B7vg@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"0000000000009bd1cb05c27f5037\"","Subject":"Re: [libcamera-devel] [PATCH v2 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>"}},{"id":16979,"web_url":"https://patchwork.libcamera.org/comment/16979/","msgid":"<20210517071732.nsmzc6i7uhjjhizs@uno.localdomain>","date":"2021-05-17T07:17:32","subject":"Re: [libcamera-devel] [PATCH v2 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 Mon, May 17, 2021 at 01:48:59PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo and Laurent,\n>\n> On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, May 13, 2021 at 11:22:41AM +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> > > +             camera->requestComplete(request);\n> >\n> > This breaks the request completion ordering. I think you could just call\n> > PipelineHandler::completeRequest() instead.\n> >\n>\n> I had the same comment in the previous version.\n> https://patchwork.libcamera.org/patch/12243/\n> We were waiting for your reply. Alright we got the reply now.\n>\n\nYes, I've cc-ed Laurent in that patch to know his opinion.\n\nLaurent, could you please read my reply and let me know if it add\nanything to the discussion ?\n\nThanks\n   j\n>\n>\n> > >               data->queuedRequests_.remove(request);\n> > > +     }\n> > >  }\n> > >\n> > >  /**\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 75D85C31FB\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 May 2021 07:16:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B90E16891E;\n\tMon, 17 May 2021 09:16:49 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35D3368919\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 May 2021 09:16:48 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 66D57E000D;\n\tMon, 17 May 2021 07:16:47 +0000 (UTC)"],"Date":"Mon, 17 May 2021 09:17:32 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210517071732.nsmzc6i7uhjjhizs@uno.localdomain>","References":"<20210513092246.42847-1-jacopo@jmondi.org>\n\t<20210513092246.42847-4-jacopo@jmondi.org>\n\t<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>\n\t<CAO5uPHPndMwurjeDHnQ_JYRG3OkmMCZnBi-UVnsJ4T6ox5B7vg@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHPndMwurjeDHnQ_JYRG3OkmMCZnBi-UVnsJ4T6ox5B7vg@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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>"}},{"id":16981,"web_url":"https://patchwork.libcamera.org/comment/16981/","msgid":"<YKIbayZ5TtLMsaSV@pendragon.ideasonboard.com>","date":"2021-05-17T07:29:47","subject":"Re: [libcamera-devel] [PATCH v2 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 Mon, May 17, 2021 at 09:17:32AM +0200, Jacopo Mondi wrote:\n> On Mon, May 17, 2021 at 01:48:59PM +0900, Hirokazu Honda wrote:\n> > On Mon, May 17, 2021 at 11:45 AM Laurent Pinchart wrote:\n> > > On Thu, May 13, 2021 at 11:22:41AM +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> > > > +             camera->requestComplete(request);\n> > >\n> > > This breaks the request completion ordering. I think you could just call\n> > > PipelineHandler::completeRequest() instead.\n> > >\n> >\n> > I had the same comment in the previous version.\n> > https://patchwork.libcamera.org/patch/12243/\n> > We were waiting for your reply. Alright we got the reply now.\n> >\n> \n> Yes, I've cc-ed Laurent in that patch to know his opinion.\n> \n> Laurent, could you please read my reply and let me know if it add\n> anything to the discussion ?\n\nDone. Sorry for missing it in the first place.\n\n> > > >               data->queuedRequests_.remove(request);\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 9F9CCC31FC\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 May 2021 07:30:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 12EEA6891E;\n\tMon, 17 May 2021 09:30:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06AFD602B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 May 2021 09:29:59 +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 771178D4;\n\tMon, 17 May 2021 09:29:58 +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=\"lS9OupBR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621236598;\n\tbh=fXaluZQh6Xbev9oh1VsSzF0T1duXFVyS8jt4Lu4eHzs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lS9OupBR7mMCVTUiTaU2jBlUlMYoQjSG7N4otbGJ2KAIniChSg0igX2zgDDTllljD\n\tmuQi6+B4pFowJOgkBWZ0MRQsiBI3yNQO7EUpx8H9PVe+uSbhQB+14PbES3u5580cl+\n\tOB1ErNggTTQFM7kr5INwoC9C1gvntnJg5PUXKjUw=","Date":"Mon, 17 May 2021 10:29:47 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YKIbayZ5TtLMsaSV@pendragon.ideasonboard.com>","References":"<20210513092246.42847-1-jacopo@jmondi.org>\n\t<20210513092246.42847-4-jacopo@jmondi.org>\n\t<YKHYsEdogXvh1IHl@pendragon.ideasonboard.com>\n\t<CAO5uPHPndMwurjeDHnQ_JYRG3OkmMCZnBi-UVnsJ4T6ox5B7vg@mail.gmail.com>\n\t<20210517071732.nsmzc6i7uhjjhizs@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210517071732.nsmzc6i7uhjjhizs@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 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>"}}]