[{"id":12065,"web_url":"https://patchwork.libcamera.org/comment/12065/","msgid":"<20200820095246.j6smlm6xuujwtcjl@uno.localdomain>","date":"2020-08-20T09:52:46","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: rkisp1:\n\tTrack buffers for self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 13, 2020 at 02:52:44AM +0200, Niklas Söderlund wrote:\n> In preparation of supporting both the main and self path extend\n> RkISP1FrameInfo to track buffers from the self path stream.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++---\n> 1 file changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 671098e11a2e2750..3761b7ef7a9386e3 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -67,6 +67,7 @@ struct RkISP1FrameInfo {\n> \tFrameBuffer *paramBuffer;\n> \tFrameBuffer *statBuffer;\n> \tFrameBuffer *mainPathBuffer;\n> +\tFrameBuffer *selfPathBuffer;\n>\n> \tbool paramFilled;\n> \tbool paramDequeued;\n> @@ -270,7 +271,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \tFrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();\n>\n> \tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> -\tif (!mainPathBuffer) {\n> +\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> +\tif (!mainPathBuffer && !selfPathBuffer) {\n\nI don't see in the rest of the function anything checking if either of\nthe two is valid, hence I assum you expect -both- of them to be valid.\nIf that's the case, shouldn't the condition be || instead of && ?\n\n> \t\tLOG(RkISP1, Error)\n> \t\t\t<< \"Attempt to queue request with invalid stream\";\n> \t\treturn nullptr;\n> @@ -285,6 +287,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \tinfo->request = request;\n> \tinfo->paramBuffer = paramBuffer;\n> \tinfo->mainPathBuffer = mainPathBuffer;\n> +\tinfo->selfPathBuffer = selfPathBuffer;\n> \tinfo->statBuffer = statBuffer;\n> \tinfo->paramFilled = false;\n> \tinfo->paramDequeued = false;\n> @@ -343,7 +346,8 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>\n> \t\tif (info->paramBuffer == buffer ||\n> \t\t info->statBuffer == buffer ||\n> -\t\t info->mainPathBuffer == buffer)\n> +\t\t info->mainPathBuffer == buffer ||\n> +\t\t info->selfPathBuffer == buffer)\n> \t\t\treturn info;\n> \t}\n>\n> @@ -415,7 +419,12 @@ protected:\n>\n> \t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> -\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> +\n> +\t\tif (info->mainPathBuffer)\n> +\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> +\n> +\t\tif (info->selfPathBuffer)\n> +\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n\nAh, maybe that's where you check them, so I guess a nullptr is fine in\nthe above create() function.\n\n> \t}\n>\n> private:\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 52378BD87C\n\tfor ;\n\tThu, 20 Aug 2020 09:49:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEA9A61F61;\n\tThu, 20 Aug 2020 11:49:04 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0990A61ED9\n\tfor ;\n\tThu, 20 Aug 2020 11:49:03 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 457D8FF80B;\n\tThu, 20 Aug 2020 09:49:03 +0000 (UTC)"],"X-Originating-IP":"82.52.18.94","Date":"Thu, 20 Aug 2020 11:52:46 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200820095246.j6smlm6xuujwtcjl@uno.localdomain>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-12-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-12-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: rkisp1:\n\tTrack buffers for self path","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12494,"web_url":"https://patchwork.libcamera.org/comment/12494/","msgid":"<20200914111303.GJ1127199@oden.dyn.berto.se>","date":"2020-09-14T11:13:03","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: rkisp1:\n\tTrack buffers for self path","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-08-20 11:52:46 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:44AM +0200, Niklas Söderlund wrote:\n> > In preparation of supporting both the main and self path extend\n> > RkISP1FrameInfo to track buffers from the self path stream.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 ++++++++++++---\n> > 1 file changed, 12 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 671098e11a2e2750..3761b7ef7a9386e3 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -67,6 +67,7 @@ struct RkISP1FrameInfo {\n> > \tFrameBuffer *paramBuffer;\n> > \tFrameBuffer *statBuffer;\n> > \tFrameBuffer *mainPathBuffer;\n> > +\tFrameBuffer *selfPathBuffer;\n> >\n> > \tbool paramFilled;\n> > \tbool paramDequeued;\n> > @@ -270,7 +271,8 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > \tFrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();\n> >\n> > \tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > -\tif (!mainPathBuffer) {\n> > +\tFrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> > +\tif (!mainPathBuffer && !selfPathBuffer) {\n> \n> I don't see in the rest of the function anything checking if either of\n> the two is valid, hence I assum you expect -both- of them to be valid.\n> If that's the case, shouldn't the condition be || instead of && ?\n\nI expect one of them to be !nullptr as that would imply a Request have \nbeen queued without any buffers in it, something we currently can't \nhandle. But it brings up an interesting point, going forward is queueing \nof empty Requests something we like to support? I'm thinking it could be \nuseful to keep the IPA running even if the application is not really \ninterested in frames at that point. Anyhow I think this is something for \nfuture work and this change only builds on the current behavior but \nextends it to the self path.\n\n> \n> > \t\tLOG(RkISP1, Error)\n> > \t\t\t<< \"Attempt to queue request with invalid stream\";\n> > \t\treturn nullptr;\n> > @@ -285,6 +287,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > \tinfo->request = request;\n> > \tinfo->paramBuffer = paramBuffer;\n> > \tinfo->mainPathBuffer = mainPathBuffer;\n> > +\tinfo->selfPathBuffer = selfPathBuffer;\n> > \tinfo->statBuffer = statBuffer;\n> > \tinfo->paramFilled = false;\n> > \tinfo->paramDequeued = false;\n> > @@ -343,7 +346,8 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> >\n> > \t\tif (info->paramBuffer == buffer ||\n> > \t\t info->statBuffer == buffer ||\n> > -\t\t info->mainPathBuffer == buffer)\n> > +\t\t info->mainPathBuffer == buffer ||\n> > +\t\t info->selfPathBuffer == buffer)\n> > \t\t\treturn info;\n> > \t}\n> >\n> > @@ -415,7 +419,12 @@ protected:\n> >\n> > \t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> > \t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > -\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> > +\n> > +\t\tif (info->mainPathBuffer)\n> > +\t\t\tpipe_->mainPathVideo_->queueBuffer(info->mainPathBuffer);\n> > +\n> > +\t\tif (info->selfPathBuffer)\n> > +\t\t\tpipe_->selfPathVideo_->queueBuffer(info->selfPathBuffer);\n> \n> Ah, maybe that's where you check them, so I guess a nullptr is fine in\n> the above create() function.\n> \n> > \t}\n> >\n> > private:\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 07D19C3B5B\n\tfor ;\n\tMon, 14 Sep 2020 11:13:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 927A762D99;\n\tMon, 14 Sep 2020 13:13:05 +0200 (CEST)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC4F762B90\n\tfor ;\n\tMon, 14 Sep 2020 13:13:04 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id y17so12955158lfa.8\n\tfor ;\n\tMon, 14 Sep 2020 04:13:04 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tc28sm3327141lfh.98.2020.09.14.04.13.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Sep 2020 04:13:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Rh6dsz1o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=SQkrKsJk7JbOBwFxHlO2T3d3vek9a12TPBQcxHGDejk=;\n\tb=Rh6dsz1oE8tICPAIT8k40w6cT/P+MGD1IyGJf1NnCaTj28YrDGZ2xB8sTHqvFC9OUF\n\tpLuBECnYCMXdGlmOKkUXMDLTjjX2v13QHhhouNPwCejlKHaDAkoXbYgKgoZkPjApJakK\n\tQ1JLqzvrFdBXUc7AaUPFHcfj2596G+9Y6DKj1K+biOmULPWdoGBghVVM0KkG6ATefD2f\n\t3cnehgVkynpPkQsMDVMGOndLzHhu0SMsSnP2XxY623QGbLm9MXR4PdokT/BMSsg23NfP\n\toXoisHzHYgLzQNKnH37vEud1yakfj9Fqepja7w72H06k/If8Y9UQKOWUK6cAqBmi90/4\n\t8w6w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=SQkrKsJk7JbOBwFxHlO2T3d3vek9a12TPBQcxHGDejk=;\n\tb=QQAW3gQRSz/cQgL7knjJJGAAuUo9oue7Kx6cC5uaJshIou7f4O29hoBZVxePXkObH7\n\tJefq20uzol8vPuLbhYO2IR12HlHNbhWU4zfPX+D/F7wKzUrqqj5DVYRHwfLD4bqrd61d\n\tn39Y9L451OXktvO/a9wqo/6v7k4Awq/UdSuzZgy9NwAgTRriJ8Eqh/xZBeG0lW51ComW\n\t+rk2bh9wYx4JTcM0ArAsTxupYNWazW+A1Taf0rsY+RPO3/iUrz3wPP2MWmQ2cBm9rnzv\n\tRsDu9dPAm8h534WKUFVPQcr2yTBEvW2fa+w5reUGbYvgPL8MngJr3ncN0/au8Ge0QW4E\n\t/4dg==","X-Gm-Message-State":"AOAM532fC3BEmdUIlCBTYY7749QU5Oz/w02FOCZTbjyqWxv1oEhnpa9c\n\tJFLcQxKfSuBYXMYULDXCxJudHWJNAbJChA==","X-Google-Smtp-Source":"ABdhPJzKH4fVfwtw0vHJtBUzDhGn5ocGJqJShISWZKj+gMR5yWf3qi2LQXbE0ixk3flqJhHZ8cXhYg==","X-Received":"by 2002:a19:f00c:: with SMTP id\n\tp12mr4412311lfc.357.1600081984114; \n\tMon, 14 Sep 2020 04:13:04 -0700 (PDT)","Date":"Mon, 14 Sep 2020 13:13:03 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200914111303.GJ1127199@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-12-niklas.soderlund@ragnatech.se>\n\t<20200820095246.j6smlm6xuujwtcjl@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820095246.j6smlm6xuujwtcjl@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: rkisp1:\n\tTrack buffers for self path","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]