[{"id":31856,"web_url":"https://patchwork.libcamera.org/comment/31856/","msgid":"<172952346881.884742.5281610353225357876@ping.linuxembedded.co.uk>","date":"2024-10-21T15:11:08","subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-10-21 14:37:16)\n> Let's extract the two occurrences of canceling a request to a common\n> helper. This is especially useful for the followup patch, which needs\n> to cancel a request from outside.\n\nSame as v4:\n\nReviewed-by: Kieran Bingham \n\nThis series fixes the assertion at shutdown with a large buffer count:\n\nTested-by: Kieran Bingham \n\n\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 2 files changed, 17 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> void completeRequest(Request *request);\n> + void cancelRequest(Request *request);\n> \n> std::string configurationFile(const std::string &subdir,\n> const std::string &name) const;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> while (!waitingRequests_.empty()) {\n> Request *request = waitingRequests_.front();\n> waitingRequests_.pop();\n> -\n> - request->_d()->cancel();\n> - completeRequest(request);\n> + cancelRequest(request);\n> }\n> \n> /* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> }\n> \n> int ret = queueRequestDevice(camera, request);\n> - if (ret) {\n> - request->_d()->cancel();\n> - completeRequest(request);\n> - }\n> + if (ret)\n> + cancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> }\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> + request->_d()->cancel();\n> + completeRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name\n> -- \n> 2.44.1\n>","headers":{"Return-Path":"","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 9EC46C32A3\n\tfor ;\n\tMon, 21 Oct 2024 15:11:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FB4D65392;\n\tMon, 21 Oct 2024 17:11:13 +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 26FDD6538A\n\tfor ;\n\tMon, 21 Oct 2024 17:11:12 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0373F4CF;\n\tMon, 21 Oct 2024 17:09:25 +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=\"hZjV4fC2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729523366;\n\tbh=eUPoZIDPyvIdq+/sd7LhtJHZZ3dRw0LseHkjqzUDzf0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hZjV4fC2OMgzbMGob0KiG9uTRXRCRLYhUBngxhKKhOeSmlMxS5Sbxib55Y0lSLosw\n\tW6JXJ/ZdHPKaULTI5E01m8dNC8+pe26/DB17HKH30Q42dlaoLzLCAlDZBOTn/EMJz4\n\tnRmQX0LrsbUmpGiK2YYtfmp3MFcHLsjVaA7zYhbk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241021133718.894374-2-mzamazal@redhat.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-2-mzamazal@redhat.com>","Subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","From":"Kieran Bingham ","Cc":"Milan Zamazal , robert.mader@posteo.de,\n\tlaurent.pinchart@ideasonboard.com","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Mon, 21 Oct 2024 16:11:08 +0100","Message-ID":"<172952346881.884742.5281610353225357876@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31995,"web_url":"https://patchwork.libcamera.org/comment/31995/","msgid":"<6d94e220-f1e7-419b-946b-4bbcb49dd08d@redhat.com>","date":"2024-11-03T13:20:39","subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 21-Oct-24 3:37 PM, Milan Zamazal wrote:\n> Let's extract the two occurrences of canceling a request to a common\n> helper. This is especially useful for the followup patch, which needs\n> to cancel a request from outside.\n> \n> Signed-off-by: Milan Zamazal \n\nThanks, patch looks good to me:\n\nReviewed-by: Hans de Goede \n\nRegards,\n\nHans\n\n\n\n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 2 files changed, 17 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n> +\tvoid cancelRequest(Request *request);\n> \n> \tstd::string configurationFile(const std::string &subdir,\n> \t\t\t\t const std::string &name) const;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> \twhile (!waitingRequests_.empty()) {\n> \t\tRequest *request = waitingRequests_.front();\n> \t\twaitingRequests_.pop();\n> -\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> +\t\tcancelRequest(request);\n> \t}\n> \n> \t/* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> \t}\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret) {\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> -\t}\n> +\tif (ret)\n> +\t\tcancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> +\trequest->_d()->cancel();\n> +\tcompleteRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name","headers":{"Return-Path":"","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 83CA4BDB1D\n\tfor ;\n\tSun, 3 Nov 2024 13:20:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2957653BD;\n\tSun, 3 Nov 2024 14:20:47 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71536653A1\n\tfor ;\n\tSun, 3 Nov 2024 14:20:46 +0100 (CET)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-171-PsaD-6b0OEKrkeRqmhakrw-1; Sun, 03 Nov 2024 08:20:43 -0500","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-a99f084683fso292087766b.3\n\tfor ;\n\tSun, 03 Nov 2024 05:20:43 -0800 (PST)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5ceac7ca7a0sm3412928a12.64.2024.11.03.05.20.40\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 03 Nov 2024 05:20:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"KGR5zA61\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730640045;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=LitWsHPI+pQzfQm3fpBvH2w+NEkCY16KSJHef9UHo3g=;\n\tb=KGR5zA61HUc2LDawKKD/XBtygVu0zl7tkt1RDpus9hUJ4xgi2A/Ed6oO6HNjtSkl01vQDe\n\tn3s/g4nU27YbhU7SaKtb79ifCYk88JYcldZXLFA5KB76ePDlITuTZmWPYi3FWht06qJ0gn\n\togfaGkARUnmmn0uCbnGmprw8o6IEEuE=","X-MC-Unique":"PsaD-6b0OEKrkeRqmhakrw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730640042; x=1731244842;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=LitWsHPI+pQzfQm3fpBvH2w+NEkCY16KSJHef9UHo3g=;\n\tb=LwnqYxSvZtmukv/sCP0Cns5yn6lBhRZpkBSB2lMx1KWOMEI7Km0b9DWVMiKL2w1hCU\n\tKz3lzLWHG8YrxZV51doGP8n1B1kEqxXt0g+h2AdDj5aElx3f7qnnl+qbKUsOk9x76z7S\n\tt5MavYNVutSRu3Nd6VzVcX3tF1fKcWWjwgh6QjQ/FTfxNFM+TbyQm0+1U8ra329DfTDc\n\th3lXIbl/K7I4oVF59AzZfLUINnQRx7SCapcuIV8PKPtkIqR3SQ4ieqJReniIjAfxS3cj\n\tECIUMHLJEtm0V243H6RNIHhlqrcYYbRoC1OYB1UzJOoE1Teg/uZgPpee3JvY0OtsStRJ\n\twFVw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUng6R4yWB4mbKVU+kcBZioumQ/YCCsvtTk8zrMXRNANzA5naKwqWhFHt69h7cDCN13C9cORJ/TK5a14tUNP6w=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzAzUF+Oruwok3c7Wo4FV4O++td6LVrQi/55vn7Pr5jBlf2edAb\n\tmTJ3rUJSbVjXDMXzA3Wv87iLZfRsctIjOsco9/wssI4v6V8FPPrPHw73jhrgslnOtIMK4UjLYhh\n\t8sa18YXuLWKu2qeHdSoc2JvZIiHWCBhoNTg4Ew0TUyKEw3/QdiCV0xBYtp4ArmvzmJL4S++yIjC\n\tFYOt+HHJr3","X-Received":["by 2002:a17:906:1983:b0:a9e:8612:eeca with SMTP id\n\ta640c23a62f3a-a9e8612f052mr380597466b.48.1730640042325; \n\tSun, 03 Nov 2024 05:20:42 -0800 (PST)","by 2002:a17:906:1983:b0:a9e:8612:eeca with SMTP id\n\ta640c23a62f3a-a9e8612f052mr380596266b.48.1730640041858; \n\tSun, 03 Nov 2024 05:20:41 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGX6K28p2wBC4dCHxYe9YB4LID8XjwR0XT9YZ1HlnfUSwCMJ0IlcgDbeBZuIGJvE1FLWw4WSA==","Message-ID":"<6d94e220-f1e7-419b-946b-4bbcb49dd08d@redhat.com>","Date":"Sun, 3 Nov 2024 14:20:39 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"robert.mader@posteo.de, kieran.bingham@ideasonboard.com,\n\tlaurent.pinchart@ideasonboard.com","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-2-mzamazal@redhat.com>","From":"Hans de Goede ","In-Reply-To":"<20241021133718.894374-2-mzamazal@redhat.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32047,"web_url":"https://patchwork.libcamera.org/comment/32047/","msgid":"<20241106121044.GC9369@pendragon.ideasonboard.com>","date":"2024-11-06T12:10:44","subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote:\n> Let's extract the two occurrences of canceling a request to a common\n> helper. This is especially useful for the followup patch, which needs\n> to cancel a request from outside.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 2 files changed, 17 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n> +\tvoid cancelRequest(Request *request);\n> \n> \tstd::string configurationFile(const std::string &subdir,\n> \t\t\t\t const std::string &name) const;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> \twhile (!waitingRequests_.empty()) {\n> \t\tRequest *request = waitingRequests_.front();\n> \t\twaitingRequests_.pop();\n> -\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> +\t\tcancelRequest(request);\n> \t}\n> \n> \t/* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> \t}\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret) {\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> -\t}\n> +\tif (ret)\n> +\t\tcancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n\n\"in additiona to its completion\" confuses me. I would simply write\n\n * This function cancels and completes the request. The same rules as for\n * completeRequest() apply.\n\nReviewed-by: Laurent Pinchart \n\nAlthough, I wonder if we could instead add a Request::Status argument to\ncompleteRequest(), default it to Request::Status::RequestComplete, and\navoid adding a new function. I don't mind much either way.\n\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> +\trequest->_d()->cancel();\n> +\tcompleteRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name","headers":{"Return-Path":"","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 0D816BDB13\n\tfor ;\n\tWed, 6 Nov 2024 12:10:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E8BC65426;\n\tWed, 6 Nov 2024 13:10:52 +0100 (CET)","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 89C686541A\n\tfor ;\n\tWed, 6 Nov 2024 13:10:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D5C559D;\n\tWed, 6 Nov 2024 13:10:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IqRMXl0i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730895042;\n\tbh=PWIL9WQlOYKL7bdFT6u3OfTJ7exNsRq00GXA5VM8A5M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IqRMXl0isAzq9ktLbY0+yJWkNXZ6piXJGHYflL954L3FtKMlx4Ds6w5rQPCR7b+SH\n\tWwUfNLl0FiXQb7bfBdi6tUEN2bRgvM2TQqLxYaYJJDk5T/aQuDS7x4hvmV9jFptQN3\n\t0teRBVA5USYBFIUBVR0ER8/jfOpQbGia+V6iHUVQ=","Date":"Wed, 6 Nov 2024 14:10:44 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","Message-ID":"<20241106121044.GC9369@pendragon.ideasonboard.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-2-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241021133718.894374-2-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32062,"web_url":"https://patchwork.libcamera.org/comment/32062/","msgid":"<875xp0gldm.fsf@redhat.com>","date":"2024-11-06T20:25:41","subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote:\n>> Let's extract the two occurrences of canceling a request to a common\n>> helper. This is especially useful for the followup patch, which needs\n>> to cancel a request from outside.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> include/libcamera/internal/pipeline_handler.h | 1 +\n>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> 2 files changed, 17 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index 0d3808036..fb28a18d0 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -60,6 +60,7 @@ public:\n>> \n>> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> \tvoid completeRequest(Request *request);\n>> +\tvoid cancelRequest(Request *request);\n>> \n>> \tstd::string configurationFile(const std::string &subdir,\n>> \t\t\t\t const std::string &name) const;\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index e59404691..c9cb11f0f 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> \twhile (!waitingRequests_.empty()) {\n>> \t\tRequest *request = waitingRequests_.front();\n>> \t\twaitingRequests_.pop();\n>> -\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> +\t\tcancelRequest(request);\n>> \t}\n>> \n>> \t/* Make sure no requests are pending. */\n>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> \t}\n>> \n>> \tint ret = queueRequestDevice(camera, request);\n>> -\tif (ret) {\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> -\t}\n>> +\tif (ret)\n>> +\t\tcancelRequest(request);\n>> }\n>> \n>> /**\n>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> \t}\n>> }\n>> \n>> +/**\n>> + * \\brief Cancel request and signal its completion\n>> + * \\param[in] request The request to cancel\n>> + *\n>> + * This function cancels the request in addition to its completion. The same\n>\n> \"in additiona to its completion\" confuses me. I would simply write\n>\n> * This function cancels and completes the request. The same rules as for\n> * completeRequest() apply.\n\nIndeed, this sounds better.\n\n> Reviewed-by: Laurent Pinchart \n>\n> Although, I wonder if we could instead add a Request::Status argument to\n> completeRequest(), default it to Request::Status::RequestComplete, and\n> avoid adding a new function. I don't mind much either way.\n\nA Request::Status argument could cause questions about possible\nRequest::Status::Pending value. Which could be avoided by using\nsomething like `bool cancel' argument instead but I think it's better to\nstop here and leave things as they are; other than that I also don't\nmind much either way.\n\n>> + * rules as for completeRequest() apply.\n>> + */\n>> +void PipelineHandler::cancelRequest(Request *request)\n>> +{\n>> +\trequest->_d()->cancel();\n>> +\tcompleteRequest(request);\n>> +}\n>> +\n>> /**\n>> * \\brief Retrieve the absolute path to a platform configuration file\n>> * \\param[in] subdir The pipeline handler specific subdirectory name","headers":{"Return-Path":"","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 90870BE173\n\tfor ;\n\tWed, 6 Nov 2024 20:25:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E8996545A;\n\tWed, 6 Nov 2024 21:25:48 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 717DA65438\n\tfor ;\n\tWed, 6 Nov 2024 21:25:46 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-197-UGKRhy6GNyGX4X2VWdsnbg-1; Wed, 06 Nov 2024 15:25:44 -0500","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-43163a40ee0so1303755e9.0\n\tfor ;\n\tWed, 06 Nov 2024 12:25:43 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-432aa6c6569sm34681135e9.21.2024.11.06.12.25.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 06 Nov 2024 12:25:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"JPUTj/gX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730924745;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=J0tfgT7JWPNGWstyh/8vT0wvdoUhl/8BangL3g3EIps=;\n\tb=JPUTj/gXfynEdFy4AGo7fUReBFF/hLwhxeqG64fafNmj0s8z6LHF37lrtlqCkDajwh2kpv\n\tol7JCtEAGdxzI9j0GG3iB6tLjF0Q4r61Nm/K7tXx16zTmwe8jskY61m+kv4wesZAfGFKip\n\tWr1R7oQyPwt/xMgcg0YBoISPTwoiBsU=","X-MC-Unique":"UGKRhy6GNyGX4X2VWdsnbg-1","X-Mimecast-MFC-AGG-ID":"UGKRhy6GNyGX4X2VWdsnbg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730924743; x=1731529543;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=J0tfgT7JWPNGWstyh/8vT0wvdoUhl/8BangL3g3EIps=;\n\tb=R8GEdYpZJ55cOUVulQr+jbZQMY7IC/ExZ23tXc6oPlN5qPy0mBczn004DFQbrWStFX\n\t70QeiS3Xw/gr79RivWAkT/7NhnQu02EV/2ntqPs/lE47cGg29lnrGqaStsDD+cgRpwjo\n\to4Dt4SWG889Oey8vYGhiLLuTSDITfXx3rA5s4mJjzKveL0+S9eMj9w+uQkYltVeFExDR\n\tROeT26TT+1m5bENHhx66H+kwddBBYlQJgwcSS9s/CAEAXz3+0L+UjNHGg6H7OKzPF2lq\n\tUra6siWpiAAnSIlnKNORheNRe4p29/r0EsV/8C70OGPFPf8MUK58c4L+auI5lPn+Z1jY\n\tIEww==","X-Gm-Message-State":"AOJu0YwiBn/X6ZLfWhcoB3KclpJJNGKyAUt/p8eyOgHaqwqdZUJOX5WS\n\tGbPafUFiQRjQXDBiqgHd7rfNEA19Tx5Fj01QbqC+ltKJuzKqEqJVD3YLzqLTx005yfveLuxSWHv\n\tvE8EJjr7A2AR3xDVUJY2npPD66tcIyw89PDml+U666wtsd5bJ5B1+hQiomYXfPi3lJJJLr1l3PJ\n\tCVVy4=","X-Received":["by 2002:a05:600c:358f:b0:426:647b:1bfc with SMTP id\n\t5b1f17b1804b1-4319ad2cc31mr353547775e9.30.1730924742691; \n\tWed, 06 Nov 2024 12:25:42 -0800 (PST)","by 2002:a05:600c:358f:b0:426:647b:1bfc with SMTP id\n\t5b1f17b1804b1-4319ad2cc31mr353547625e9.30.1730924742299; \n\tWed, 06 Nov 2024 12:25:42 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFuiRVgvVEjJtXxwgs96zQgfHuADPDODfL8kBvKx6gBH3L5uA48OL1OfDSpc2OMKiKLUwVcIw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","In-Reply-To":"<20241106121044.GC9369@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 6 Nov 2024 14:10:44 +0200\")","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-2-mzamazal@redhat.com>\n\t<20241106121044.GC9369@pendragon.ideasonboard.com>","Date":"Wed, 06 Nov 2024 21:25:41 +0100","Message-ID":"<875xp0gldm.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"-5wIw-gpuBU7q9OuOyRtr2Iw0GU25bq3vm_gsd_bRzU_1730924743","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32064,"web_url":"https://patchwork.libcamera.org/comment/32064/","msgid":"<20241106224749.GH21454@pendragon.ideasonboard.com>","date":"2024-11-06T22:47:49","subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 06, 2024 at 09:25:41PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote:\n> >> Let's extract the two occurrences of canceling a request to a common\n> >> helper. This is especially useful for the followup patch, which needs\n> >> to cancel a request from outside.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> include/libcamera/internal/pipeline_handler.h | 1 +\n> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> >> 2 files changed, 17 insertions(+), 7 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> index 0d3808036..fb28a18d0 100644\n> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> @@ -60,6 +60,7 @@ public:\n> >> \n> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> >> \tvoid completeRequest(Request *request);\n> >> +\tvoid cancelRequest(Request *request);\n> >> \n> >> \tstd::string configurationFile(const std::string &subdir,\n> >> \t\t\t\t const std::string &name) const;\n> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> index e59404691..c9cb11f0f 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> >> \twhile (!waitingRequests_.empty()) {\n> >> \t\tRequest *request = waitingRequests_.front();\n> >> \t\twaitingRequests_.pop();\n> >> -\n> >> -\t\trequest->_d()->cancel();\n> >> -\t\tcompleteRequest(request);\n> >> +\t\tcancelRequest(request);\n> >> \t}\n> >> \n> >> \t/* Make sure no requests are pending. */\n> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >> \t}\n> >> \n> >> \tint ret = queueRequestDevice(camera, request);\n> >> -\tif (ret) {\n> >> -\t\trequest->_d()->cancel();\n> >> -\t\tcompleteRequest(request);\n> >> -\t}\n> >> +\tif (ret)\n> >> +\t\tcancelRequest(request);\n> >> }\n> >> \n> >> /**\n> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> >> \t}\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Cancel request and signal its completion\n> >> + * \\param[in] request The request to cancel\n> >> + *\n> >> + * This function cancels the request in addition to its completion. The same\n> >\n> > \"in additiona to its completion\" confuses me. I would simply write\n> >\n> > * This function cancels and completes the request. The same rules as for\n> > * completeRequest() apply.\n> \n> Indeed, this sounds better.\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > Although, I wonder if we could instead add a Request::Status argument to\n> > completeRequest(), default it to Request::Status::RequestComplete, and\n> > avoid adding a new function. I don't mind much either way.\n> \n> A Request::Status argument could cause questions about possible\n> Request::Status::Pending value. Which could be avoided by using\n> something like `bool cancel' argument instead but I think it's better to\n> stop here and leave things as they are; other than that I also don't\n> mind much either way.\n\nI was also considering the Pending issue, and I don't like boolean\narguments much in this case, as they're not very readable. Does\n\n\tcompleteRequest(request, true);\n\ncancel the request or complete it successfully ?\n\nLet's leave things as-is.\n\n> >> + * rules as for completeRequest() apply.\n> >> + */\n> >> +void PipelineHandler::cancelRequest(Request *request)\n> >> +{\n> >> +\trequest->_d()->cancel();\n> >> +\tcompleteRequest(request);\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Retrieve the absolute path to a platform configuration file\n> >> * \\param[in] subdir The pipeline handler specific subdirectory name","headers":{"Return-Path":"","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 70A9DBE173\n\tfor ;\n\tWed, 6 Nov 2024 22:47:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C36F6545A;\n\tWed, 6 Nov 2024 23:47:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 187FA65431\n\tfor ;\n\tWed, 6 Nov 2024 23:47:55 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 539CC59D;\n\tWed, 6 Nov 2024 23:47:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"H6BAK+GE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730933266;\n\tbh=lzQZGOq71IOdm+x1DT/hOng7HeX6BJiafKxRreUB4hQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H6BAK+GECRfmjAz4NHQ1bE8pPxRmS2qs6ccTYbXPV71jGnjWlWt6wiLo+gnLCPS44\n\tFZKJ7OvgMDK5Fx8SUarnvf9EyXyZOWPVnHkVaI1np8HaI3uQREM7xcR2ZmsPO1oN7j\n\t/kueKpHFI5pw0oh2SNtUGStC8XfdGi2fvjj0y0yY=","Date":"Thu, 7 Nov 2024 00:47:49 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v5 1/3] libcamera: pipeline_handler: Provide\n\tcancelRequest","Message-ID":"<20241106224749.GH21454@pendragon.ideasonboard.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-2-mzamazal@redhat.com>\n\t<20241106121044.GC9369@pendragon.ideasonboard.com>\n\t<875xp0gldm.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<875xp0gldm.fsf@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]