[{"id":22088,"web_url":"https://patchwork.libcamera.org/comment/22088/","msgid":"","date":"2022-02-01T09:47:09","subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch. Weird how we never noticed this problem before!\n\nOn Tue, 1 Feb 2022 at 09:27, Naushir Patuck wrote:\n>\n> The ISP input stream currently only allocates a single slot in the\n> V4L2VideoDevice cache as it follows the number of buffers allocated for use.\n> However, this is wrong as the ISP input stream imports buffers from Unicam\n> image stream. As a consequence of this, only one cache slot was used during\n> runtime for the ISP input stream, and if multiple buffers were to be queued\n> simultaneously, the queue operation would return a failure.\n>\n> Fix this by passing the same number of RAW buffers available from the Unicam\n> image stream. Additionally, double this count in the cases where buffers could\n> be allocated externally from the application.\n\nI agree this sounds like it wants a better solution, but is definitely\nbeyond the scope of this (relatively small but important) fix. So:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n>\n> Bug: https://github.com/raspberrypi/libcamera-apps/issues/236\n> https://github.com/raspberrypi/libcamera-apps/issues/238\n> https://github.com/raspberrypi/libcamera-apps/issues/240\n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 3 ++-\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6a46e6bc89fa..49af56edc1f9 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> * minimise frame drops.\n> */\n> numBuffers = std::max(2, minBuffers - numRawBuffers);\n> + } else if (stream == &data->isp_[Isp::Input]) {\n> + /*\n> + * ISP input buffers are imported from Unicam, so follow\n> + * similar logic as above to count all the RAW buffers\n> + * available.\n> + */\n> + numBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers);\n> +\n> } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> /*\n> * Embedded data buffers are (currently) for internal use,\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index a4159e20b068..a421ad09ba50 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)\n> * If this is an external stream, we must allocate slots for buffers that\n> * might be externally allocated. We have no indication of how many buffers\n> * may be used, so this might overallocate slots in the buffer cache.\n> + * Similarly, if this stream is only importing buffers, we do the same.\n> *\n> * \\todo Find a better heuristic, or, even better, an exact solution to\n> * this issue.\n> */\n> - if (isExternal())\n> + if (isExternal() || importOnly_)\n> count = count * 2;\n>\n> return dev_->importBuffers(count);\n> --\n> 2.25.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 196D2BF415\n\tfor ;\n\tTue, 1 Feb 2022 09:47:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63D7B609C9;\n\tTue, 1 Feb 2022 10:47:22 +0100 (CET)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D8B5A609AF\n\tfor ;\n\tTue, 1 Feb 2022 10:47:20 +0100 (CET)","by mail-wr1-x42f.google.com with SMTP id w11so30762241wra.4\n\tfor ;\n\tTue, 01 Feb 2022 01:47:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"AjXlps6W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4sC7I0m9FQzz/jqK2oBgQIexUwOR+fnvZJHv8zV4FiM=;\n\tb=AjXlps6W8wgZW1/9p/dyHnk2mD/ovqXjKas4Rkpzy4OjFvfempffoz7aPaK+stxccE\n\t+I+eIZ7zSugUX8lh+BbBUZTMqbyyZMLGwB3cSrdtl+wzrQiWKoK04Bi+0rX1U7e4Kdeh\n\tdIUlFaewdv3AMa7D1ix/u2hlgedwW51Gz9EUIKp9kjbqMmv5yvb1EBQHXbyHBH/DwzEr\n\tCcJQSczL+H8GXR9GhJFmm15kxAKR4BNXKQD2LyI0FGK1Uj5q5/dL8v/Fo4kI2lQSRFit\n\tw3uyfjNWOixgVPhnGkZRaFMu/lvVbGAgI1yT7pWpkZ7PHKkOB14AXNTD1JqfSPiDBb5g\n\tcIjA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4sC7I0m9FQzz/jqK2oBgQIexUwOR+fnvZJHv8zV4FiM=;\n\tb=0Oqqe2aX8big2HfOSoF5Ib6qVXF4ZXyPrWn3gsrPoWO2QBDvtK9FxDHMgLyMxGCnYk\n\tv0ZSS5gXgTmElHzfP5AR5XFK6nUJ9WR0uKuhphRMHRYHVZg7+hshClT3it2luSHwqqcO\n\txm/KnhEW9RB44imnqt6/v11AHa6GyCIugct1HWn9r7F108e9p9q5lJbUPueklz1yPhcZ\n\tLCn/OuS71zLdx0NoUZzEvnfRSqFNcdMIMJ/L2X7RGGLeJc+ovYamyuK15mlf3CQEurGe\n\tS2rK8k8E535hMu0aHNbKeGdEozdnRzU/CllT658cyDMmUhBUggMujrFTWbOI9La4nK4C\n\tnZgA==","X-Gm-Message-State":"AOAM53169upCqzjtPDajfXD+OteIb30xy8zfj+JZSu6qHghbL+y9fNd/\n\t1KOTlWzY7vS5971TIQJoWZcurg1Ynczst9JPAFxrVZmIgS1dtw==","X-Google-Smtp-Source":"ABdhPJz/q5zJbALDPfBW2sTVfzazWjIFTGEXDZssRLeQK64P+iQw1kfDToMs816okAGiqk1elvou4ugmK4fDWSFuqEM=","X-Received":"by 2002:a5d:67c1:: with SMTP id n1mr20350630wrw.3.1643708840471; \n\tTue, 01 Feb 2022 01:47:20 -0800 (PST)","MIME-Version":"1.0","References":"<20220201092738.804028-1-naush@raspberrypi.com>\n\t<20220201092738.804028-2-naush@raspberrypi.com>","In-Reply-To":"<20220201092738.804028-2-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 1 Feb 2022 09:47:09 +0000","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22090,"web_url":"https://patchwork.libcamera.org/comment/22090/","msgid":"<164371070317.208029.8829966281675856036@Monstersaurus>","date":"2022-02-01T10:18:23","subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2022-02-01 09:47:09)\n> Hi Naush\n> \n> Thanks for this patch. Weird how we never noticed this problem before!\n> \n> On Tue, 1 Feb 2022 at 09:27, Naushir Patuck wrote:\n> >\n> > The ISP input stream currently only allocates a single slot in the\n> > V4L2VideoDevice cache as it follows the number of buffers allocated for use.\n> > However, this is wrong as the ISP input stream imports buffers from Unicam\n> > image stream. As a consequence of this, only one cache slot was used during\n> > runtime for the ISP input stream, and if multiple buffers were to be queued\n> > simultaneously, the queue operation would return a failure.\n> >\n> > Fix this by passing the same number of RAW buffers available from the Unicam\n> > image stream. Additionally, double this count in the cases where buffers could\n> > be allocated externally from the application.\n> \n> I agree this sounds like it wants a better solution, but is definitely\n> beyond the scope of this (relatively small but important) fix. So:\n\nDid you ever dig further into what the maximum number of V4L2 buffers\ncan be allocated are?\n\nShould we change the V4L2Device to always just allocate the maximum\nthere instead?\n\nThese are just 'structures' to be able to pass the buffer information\nright ? They're not the actual expensive memory backing?\n\nThat would also then give V4L2BufferCache the best chance of supporting\nmore external buffers?\n\n--\nKieran\n\n\n> Reviewed-by: David Plowman \n> \n> Thanks!\n> David\n> \n> >\n> > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236\n> > https://github.com/raspberrypi/libcamera-apps/issues/238\n> > https://github.com/raspberrypi/libcamera-apps/issues/240\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 3 ++-\n> > 2 files changed, 10 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6a46e6bc89fa..49af56edc1f9 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > * minimise frame drops.\n> > */\n> > numBuffers = std::max(2, minBuffers - numRawBuffers);\n> > + } else if (stream == &data->isp_[Isp::Input]) {\n> > + /*\n> > + * ISP input buffers are imported from Unicam, so follow\n> > + * similar logic as above to count all the RAW buffers\n> > + * available.\n> > + */\n> > + numBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers);\n> > +\n> > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > /*\n> > * Embedded data buffers are (currently) for internal use,\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index a4159e20b068..a421ad09ba50 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)\n> > * If this is an external stream, we must allocate slots for buffers that\n> > * might be externally allocated. We have no indication of how many buffers\n> > * may be used, so this might overallocate slots in the buffer cache.\n> > + * Similarly, if this stream is only importing buffers, we do the same.\n> > *\n> > * \\todo Find a better heuristic, or, even better, an exact solution to\n> > * this issue.\n> > */\n> > - if (isExternal())\n> > + if (isExternal() || importOnly_)\n> > count = count * 2;\n> >\n> > return dev_->importBuffers(count);\n> > --\n> > 2.25.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 31E23BF415\n\tfor ;\n\tTue, 1 Feb 2022 10:18:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B0D4609DF;\n\tTue, 1 Feb 2022 11:18:27 +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 3BE29609AF\n\tfor ;\n\tTue, 1 Feb 2022 11:18:26 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB9D4332;\n\tTue, 1 Feb 2022 11:18:25 +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=\"QXDZqN9m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643710705;\n\tbh=w1Zv6OI6e2S17WOvWfAB8dX8HBwyA8RkegjDwf/X2ng=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QXDZqN9mdcQhDLNsPFIhk5s3A1Ayn6n6wEFTmyu/bflAvmK4ECMYNIcA24UvDTCIn\n\txr6eWUVnsAnLXnLDxQB8wuT3tlFcd7lWv2o2SOEnKlpe8Gkli4M6sTyoNn8jnne/Ir\n\tV97rnjIrxQIVW48mpS7PWlQ5i7b0Cs/wC+LkmEfc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220201092738.804028-1-naush@raspberrypi.com>\n\t<20220201092738.804028-2-naush@raspberrypi.com>\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tNaushir Patuck ","Date":"Tue, 01 Feb 2022 10:18:23 +0000","Message-ID":"<164371070317.208029.8829966281675856036@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22093,"web_url":"https://patchwork.libcamera.org/comment/22093/","msgid":"","date":"2022-02-01T10:35:12","subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Tue, Feb 01, 2022 at 10:18:23AM +0000, Kieran Bingham wrote:\n> Quoting David Plowman (2022-02-01 09:47:09)\n> > Hi Naush\n> > \n> > Thanks for this patch. Weird how we never noticed this problem before!\n> > \n> > On Tue, 1 Feb 2022 at 09:27, Naushir Patuck wrote:\n> > >\n> > > The ISP input stream currently only allocates a single slot in the\n> > > V4L2VideoDevice cache as it follows the number of buffers allocated for use.\n> > > However, this is wrong as the ISP input stream imports buffers from Unicam\n> > > image stream. As a consequence of this, only one cache slot was used during\n> > > runtime for the ISP input stream, and if multiple buffers were to be queued\n> > > simultaneously, the queue operation would return a failure.\n> > >\n> > > Fix this by passing the same number of RAW buffers available from the Unicam\n> > > image stream. Additionally, double this count in the cases where buffers could\n> > > be allocated externally from the application.\n> > \n> > I agree this sounds like it wants a better solution, but is definitely\n> > beyond the scope of this (relatively small but important) fix. So:\n> \n> Did you ever dig further into what the maximum number of V4L2 buffers\n> can be allocated are?\n> \n> Should we change the V4L2Device to always just allocate the maximum\n> there instead?\n> \n> These are just 'structures' to be able to pass the buffer information\n> right ? They're not the actual expensive memory backing?\n\nWhen importing, that's correct, but note that V4L2 doesn't provide an\n\"unprepare\" buffer operation. It means that once you queue a dmabuf fd\nto a given V4L2 buffer, the dmabuf will be mapped to the device and the\nCPU until a new dmabuf comes to replace it for the same V4L2 buffer, or\nuntil the V4L2 buffer is freed (which can only be done with\nVIDIOC_REQBUFS(0) at the moment). It also means that a reference to the\ndmabuf will be kept, which will prevent the memory from being released.\nIf an application queues different buffers from time to time, with a\nlarge number of V4L2 buffer, it may take a long time before stale cache\nentries are being pushed out (if ever). I'd thus be quite cautious about\nallocating too many V4L2 buffers.\n\nThis seems a difficult to solve problem, and it stems from the fact that\nwe don't have explicit release calls in V4L2. We may want to address\nthis in the libcamera public API nonetheless at some point, with hints\nthat applications can pass, for instance to tell if a buffer will be\nsubmitted again or not. V4L2 extensions will then likely be required.\n\n> That would also then give V4L2BufferCache the best chance of supporting\n> more external buffers?\n> \n> > Reviewed-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\n> > >\n> > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236\n> > > https://github.com/raspberrypi/libcamera-apps/issues/238\n> > > https://github.com/raspberrypi/libcamera-apps/issues/240\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 3 ++-\n> > > 2 files changed, 10 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 6a46e6bc89fa..49af56edc1f9 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > > * minimise frame drops.\n> > > */\n> > > numBuffers = std::max(2, minBuffers - numRawBuffers);\n> > > + } else if (stream == &data->isp_[Isp::Input]) {\n> > > + /*\n> > > + * ISP input buffers are imported from Unicam, so follow\n> > > + * similar logic as above to count all the RAW buffers\n> > > + * available.\n> > > + */\n> > > + numBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers);\n> > > +\n> > > } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> > > /*\n> > > * Embedded data buffers are (currently) for internal use,\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > index a4159e20b068..a421ad09ba50 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)\n> > > * If this is an external stream, we must allocate slots for buffers that\n> > > * might be externally allocated. We have no indication of how many buffers\n> > > * may be used, so this might overallocate slots in the buffer cache.\n> > > + * Similarly, if this stream is only importing buffers, we do the same.\n> > > *\n> > > * \\todo Find a better heuristic, or, even better, an exact solution to\n> > > * this issue.\n> > > */\n> > > - if (isExternal())\n> > > + if (isExternal() || importOnly_)\n> > > count = count * 2;\n> > >\n> > > return dev_->importBuffers(count);","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 4BD4EBF415\n\tfor ;\n\tTue, 1 Feb 2022 10:35:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCF84609C9;\n\tTue, 1 Feb 2022 11:35:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 591E1609AF\n\tfor ;\n\tTue, 1 Feb 2022 11:35:37 +0100 (CET)","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 58CE6332;\n\tTue, 1 Feb 2022 11:35:36 +0100 (CET)"],"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=\"kRf3Xk6J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643711737;\n\tbh=mAHZco2rKvl9ZzeIZP8pU7/xXFgQH1Ouj1i+8D/hHGk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kRf3Xk6JDS1ydCqNufWIlcyMyTU+vktbVc5C2W7EH9TxQTa05FnG5WZr9TN6vM6/g\n\tJfDV/t3lAW8jpdRdlqO2Sh3+7cDYrL1/ktxQxSPqs3Vj7dGHEQQDpKSmFtfLde3Fqk\n\thuYiqd/mEjL5pYG32Nl2qAhM8tPJUSeCr4snQW2w=","Date":"Tue, 1 Feb 2022 12:35:12 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20220201092738.804028-1-naush@raspberrypi.com>\n\t<20220201092738.804028-2-naush@raspberrypi.com>\n\t\n\t<164371070317.208029.8829966281675856036@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164371070317.208029.8829966281675856036@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22095,"web_url":"https://patchwork.libcamera.org/comment/22095/","msgid":"","date":"2022-02-01T10:57:45","subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran and Laurent,\n\nOn Tue, 1 Feb 2022 at 10:35, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello,\n>\n> On Tue, Feb 01, 2022 at 10:18:23AM +0000, Kieran Bingham wrote:\n> > Quoting David Plowman (2022-02-01 09:47:09)\n> > > Hi Naush\n> > >\n> > > Thanks for this patch. Weird how we never noticed this problem before!\n> > >\n> > > On Tue, 1 Feb 2022 at 09:27, Naushir Patuck wrote:\n> > > >\n> > > > The ISP input stream currently only allocates a single slot in the\n> > > > V4L2VideoDevice cache as it follows the number of buffers allocated\n> for use.\n> > > > However, this is wrong as the ISP input stream imports buffers from\n> Unicam\n> > > > image stream. As a consequence of this, only one cache slot was\n> used during\n> > > > runtime for the ISP input stream, and if multiple buffers were to be\n> queued\n> > > > simultaneously, the queue operation would return a failure.\n> > > >\n> > > > Fix this by passing the same number of RAW buffers available from\n> the Unicam\n> > > > image stream. Additionally, double this count in the cases where\n> buffers could\n> > > > be allocated externally from the application.\n> > >\n> > > I agree this sounds like it wants a better solution, but is definitely\n> > > beyond the scope of this (relatively small but important) fix. So:\n> >\n> > Did you ever dig further into what the maximum number of V4L2 buffers\n> > can be allocated are?\n> >\n> > Should we change the V4L2Device to always just allocate the maximum\n> > there instead?\n> >\n> > These are just 'structures' to be able to pass the buffer information\n> > right ? They're not the actual expensive memory backing?\n>\n> When importing, that's correct, but note that V4L2 doesn't provide an\n> \"unprepare\" buffer operation. It means that once you queue a dmabuf fd\n> to a given V4L2 buffer, the dmabuf will be mapped to the device and the\n> CPU until a new dmabuf comes to replace it for the same V4L2 buffer, or\n> until the V4L2 buffer is freed (which can only be done with\n> VIDIOC_REQBUFS(0) at the moment). It also means that a reference to the\n> dmabuf will be kept, which will prevent the memory from being released.\n> If an application queues different buffers from time to time, with a\n> large number of V4L2 buffer, it may take a long time before stale cache\n> entries are being pushed out (if ever). I'd thus be quite cautious about\n> allocating too many V4L2 buffers.\n>\n> This seems a difficult to solve problem, and it stems from the fact that\n> we don't have explicit release calls in V4L2. We may want to address\n> this in the libcamera public API nonetheless at some point, with hints\n> that applications can pass, for instance to tell if a buffer will be\n> submitted again or not. V4L2 extensions will then likely be required.\n>\n\nI was going to suggest that we setup the cache to use VB2_MAX_FRAME (32)\nentries, but given the above explanation this may not be the right thing to\ndo to fix this.\n\nNaush\n\n\n\n>\n> > That would also then give V4L2BufferCache the best chance of supporting\n> > more external buffers?\n> >\n> > > Reviewed-by: David Plowman \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > > >\n> > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/236\n> > > > https://github.com/raspberrypi/libcamera-apps/issues/238\n> > > > https://github.com/raspberrypi/libcamera-apps/issues/240\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 3 ++-\n> > > > 2 files changed, 10 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 6a46e6bc89fa..49af56edc1f9 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1382,6 +1382,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > > > * minimise frame drops.\n> > > > */\n> > > > numBuffers = std::max(2, minBuffers -\n> numRawBuffers);\n> > > > + } else if (stream == &data->isp_[Isp::Input]) {\n> > > > + /*\n> > > > + * ISP input buffers are imported from\n> Unicam, so follow\n> > > > + * similar logic as above to count all the\n> RAW buffers\n> > > > + * available.\n> > > > + */\n> > > > + numBuffers = numRawBuffers +\n> std::max(2, minBuffers - numRawBuffers);\n> > > > +\n> > > > } else if (stream ==\n> &data->unicam_[Unicam::Embedded]) {\n> > > > /*\n> > > > * Embedded data buffers are (currently) for\n> internal use,\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > index a4159e20b068..a421ad09ba50 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > @@ -111,11 +111,12 @@ int Stream::prepareBuffers(unsigned int count)\n> > > > * If this is an external stream, we must allocate slots for\n> buffers that\n> > > > * might be externally allocated. We have no indication of\n> how many buffers\n> > > > * may be used, so this might overallocate slots in the\n> buffer cache.\n> > > > + * Similarly, if this stream is only importing buffers, we\n> do the same.\n> > > > *\n> > > > * \\todo Find a better heuristic, or, even better, an exact\n> solution to\n> > > > * this issue.\n> > > > */\n> > > > - if (isExternal())\n> > > > + if (isExternal() || importOnly_)\n> > > > count = count * 2;\n> > > >\n> > > > return dev_->importBuffers(count);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 D8E78BF415\n\tfor ;\n\tTue, 1 Feb 2022 10:58:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 398D4609E5;\n\tTue, 1 Feb 2022 11:58:04 +0100 (CET)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 810CC609AF\n\tfor ;\n\tTue, 1 Feb 2022 11:58:02 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id e17so23531604ljk.5\n\tfor ;\n\tTue, 01 Feb 2022 02:58:02 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"PycMy6iN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=QdWfh5aD+CBV10185bP33XyvB6S9CLmvZzA6+4hPA1w=;\n\tb=PycMy6iNJ+IUhhds89uuQzYvZnmuqSjP+PdLzX9+d+Ueaf/pfLUSv5nK8TNzkwnWg4\n\tLFg24E0xjpGoUjSSMzhCpEIINb9xsuToDFHhuoeSnDLTy4elgfuUSc2NkABMd8APtL9S\n\tC7z5mvbrnOlGuL1jxc8lIvScqNG79rIkcOY7E4uxcyUzYixay4r+KOn0wENy6o7Nj272\n\tzsGIzJf/NXGZKw77tccv4Li4zDBtzcyKzXWpHSH5t5ai0Kglbeo0usp+TnxuMSE2Xa4c\n\t9Bl7bmy7nZb+Pbsp8S1M9vjrHNHLXsCqrv7qglbZHIBqhV8O1juKyTmjOeF4MdsXb4J1\n\tYSIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=QdWfh5aD+CBV10185bP33XyvB6S9CLmvZzA6+4hPA1w=;\n\tb=akprRb2Tp3M3hkVZx0YJ8KKZiDpQtwCHrFktvxAhEoRyz6FBytlVgwWpV9hRt/7XxI\n\tfm0qIdRint4Uv4dk6gquekLZbxrntJ4BqhtVNTijmgC/+d175gsXsdgoDG+1fjFm1j1d\n\tc9lOH6vizOs6FHpQRNJOYdNCmvlvOweVmVR6+L4zWlAIRra7s1rzdXDW3sI3a7HCKy9L\n\tiiZccMAST1xKV0kPu7i22GIC4/IMRN3+JGIMjonFsye6WpuegfcUwv3SuDXg+uuqgcfD\n\t2uHJcYNs8dp+ROBywLRqvL5I6mm5OzJ18zYr8iQhoTU7ZHRwGXKGyyBs8kkt/EpzAS0D\n\tFe9g==","X-Gm-Message-State":"AOAM533SMsVmLOLikeSk3QrsDiuDzLlu55NC+TzFAPXFM/kRWAJMo53e\n\tYlQeBjOcOlptjPOTw+xfmIIjn7rRK00DrWHgzgQT3w==","X-Google-Smtp-Source":"ABdhPJzbyRPxN4JC9WAUSe152QJHxV+g29RZsiu5p+jF5C/P+i5RNcvCk4XDArVJDpTNtX/RKisjOj61FJIfSRcYv8Q=","X-Received":"by 2002:a05:651c:160b:: with SMTP id\n\tf11mr16739146ljq.372.1643713081655; \n\tTue, 01 Feb 2022 02:58:01 -0800 (PST)","MIME-Version":"1.0","References":"<20220201092738.804028-1-naush@raspberrypi.com>\n\t<20220201092738.804028-2-naush@raspberrypi.com>\n\t\n\t<164371070317.208029.8829966281675856036@Monstersaurus>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 1 Feb 2022 10:57:45 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000005866d005d6f2c6e5\"","Subject":"Re: [libcamera-devel] [PATCH v1 2/2] pipeline: raspberrypi: Fix the\n\tbuffer count calculation for the ISP input stream","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]