[{"id":10925,"web_url":"https://patchwork.libcamera.org/comment/10925/","msgid":"<20200628070637.GB6954@pendragon.ideasonboard.com>","date":"2020-06-28T07:06:37","subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:\n> Do not keep the duplicated ImgUDevice::ImgUOutput information in both\n> the stream and camera data classes. Remove it from the stream and only\n> access it from the camera data class.\n> \n> Which stream is which can instead be checked by comparing it to the\n> known streams in camera data. This match how streams are checked in\n> other parts of the code making the pipeline more coherent.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n> * Changes since v1\n> - s/driver/pipeline/ in commit message.\n> - Reorder if ; if else ; else statement in\n>   PipelineHandlerIPU3::queueRequestDevice()\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------\n>  1 file changed, 18 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index e817f842f1216a7f..c1520ec40fe7b57c 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -35,13 +35,11 @@ class IPU3Stream : public Stream\n>  {\n>  public:\n>  \tIPU3Stream()\n> -\t\t: active_(false), raw_(false), device_(nullptr)\n> +\t\t: active_(false)\n>  \t{\n>  \t}\n>  \n>  \tbool active_;\n> -\tbool raw_;\n> -\tImgUDevice::ImgUOutput *device_;\n>  };\n>  \n>  class IPU3CameraData : public CameraData\n> @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\tconst StreamConfiguration oldCfg = cfg;\n>  \t\tconst IPU3Stream *stream = streams_[i];\n>  \n> -\t\tif (stream->raw_) {\n> +\t\tif (stream == &data_->rawStream_) {\n>  \t\t\tcfg = cio2Configuration_;\n>  \t\t} else {\n>  \t\t\tbool scale = stream == &data_->vfStream_;\n> @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n>  \t\t\t\t\t    std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> -\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n>  \tunsigned int count = stream->configuration().bufferCount;\n>  \n> -\tif (ipu3stream->raw_)\n> +\tif (stream == &data->outStream_)\n> +\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> +\telse if (stream == &data->vfStream_)\n> +\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> +\telse if (stream == &data->rawStream_)\n>  \t\treturn data->cio2_.exportBuffers(count, buffers);\n>  \n> -\treturn ipu3stream->device_->dev->exportBuffers(count, buffers);\n> +\treturn -EINVAL;\n>  }\n>  \n>  /**\n> @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>  \t/* Queue all buffers from the request aimed for the ImgU. */\n>  \tfor (auto it : request->buffers()) {\n>  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(it.first);\n> -\t\tif (stream->raw_)\n> -\t\t\tcontinue;\n> -\n>  \t\tFrameBuffer *buffer = it.second;\n> -\t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> +\n> +\t\tif (stream == &data->rawStream_)\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = 0;\n> +\t\tif (stream == &data->outStream_)\n> +\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> +\t\telse if (stream == &data->vfStream_)\n> +\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n\n\t\telse\n\t\t\tcontinue;\n\nor\n\n\t\telse\n\t\t\tret = 0;\n\nand dropping the previous &data->rawStream_ check would allow not\ninitializing the ret variable to 0.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\n>  \t\tif (ret < 0)\n>  \t\t\terror = ret;\n>  \t}\n> @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t * second.\n>  \t\t */\n>  \t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> -\t\tdata->outStream_.device_ = &data->imgu_->output_;\n> -\t\tdata->vfStream_.device_ = &data->imgu_->viewfinder_;\n> -\t\tdata->rawStream_.raw_ = true;\n>  \n>  \t\t/*\n>  \t\t * Connect video devices' 'bufferReady' signals to their","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 06723C2E69\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 28 Jun 2020 07:06:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9429E609C7;\n\tSun, 28 Jun 2020 09:06:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1AD2603B4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Jun 2020 09:06:40 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C2BA4FB;\n\tSun, 28 Jun 2020 09:06:40 +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=\"Ymtrp4q1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593328000;\n\tbh=oPUZglgkBUoHdDQOL24QrZW0c8t8kZ2vQnGT2B+odPg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ymtrp4q1afKsrZWXIFfaySK8ZBpjIoa9RhwFlxBpSjtNI0bodue1K4W2i5RVx6FTq\n\tpby25DuEv0ADE69KHbNfvn2aswDnajtmBUCEi6rI7uB6UmMK7R60ctdTisPJ7NDjK9\n\th1eoL1V/Q2sOGZiqUGsw+axYplugbi0Ge0R/qR9A=","Date":"Sun, 28 Jun 2020 10:06:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200628070637.GB6954@pendragon.ideasonboard.com>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-10-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628001532.2685967-10-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":10931,"web_url":"https://patchwork.libcamera.org/comment/10931/","msgid":"<20200628120344.GG1105424@oden.dyn.berto.se>","date":"2020-06-28T12:03:44","subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-06-28 10:06:37 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:\n> > Do not keep the duplicated ImgUDevice::ImgUOutput information in both\n> > the stream and camera data classes. Remove it from the stream and only\n> > access it from the camera data class.\n> > \n> > Which stream is which can instead be checked by comparing it to the\n> > known streams in camera data. This match how streams are checked in\n> > other parts of the code making the pipeline more coherent.\n> > \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> > * Changes since v1\n> > - s/driver/pipeline/ in commit message.\n> > - Reorder if ; if else ; else statement in\n> >   PipelineHandlerIPU3::queueRequestDevice()\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------\n> >  1 file changed, 18 insertions(+), 14 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index e817f842f1216a7f..c1520ec40fe7b57c 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream\n> >  {\n> >  public:\n> >  \tIPU3Stream()\n> > -\t\t: active_(false), raw_(false), device_(nullptr)\n> > +\t\t: active_(false)\n> >  \t{\n> >  \t}\n> >  \n> >  \tbool active_;\n> > -\tbool raw_;\n> > -\tImgUDevice::ImgUOutput *device_;\n> >  };\n> >  \n> >  class IPU3CameraData : public CameraData\n> > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >  \t\tconst StreamConfiguration oldCfg = cfg;\n> >  \t\tconst IPU3Stream *stream = streams_[i];\n> >  \n> > -\t\tif (stream->raw_) {\n> > +\t\tif (stream == &data_->rawStream_) {\n> >  \t\t\tcfg = cio2Configuration_;\n> >  \t\t} else {\n> >  \t\t\tbool scale = stream == &data_->vfStream_;\n> > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> >  \t\t\t\t\t    std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > -\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> >  \tunsigned int count = stream->configuration().bufferCount;\n> >  \n> > -\tif (ipu3stream->raw_)\n> > +\tif (stream == &data->outStream_)\n> > +\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> > +\telse if (stream == &data->vfStream_)\n> > +\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> > +\telse if (stream == &data->rawStream_)\n> >  \t\treturn data->cio2_.exportBuffers(count, buffers);\n> >  \n> > -\treturn ipu3stream->device_->dev->exportBuffers(count, buffers);\n> > +\treturn -EINVAL;\n> >  }\n> >  \n> >  /**\n> > @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> >  \t/* Queue all buffers from the request aimed for the ImgU. */\n> >  \tfor (auto it : request->buffers()) {\n> >  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(it.first);\n> > -\t\tif (stream->raw_)\n> > -\t\t\tcontinue;\n> > -\n> >  \t\tFrameBuffer *buffer = it.second;\n> > -\t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> > +\n> > +\t\tif (stream == &data->rawStream_)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tint ret = 0;\n> > +\t\tif (stream == &data->outStream_)\n> > +\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> > +\t\telse if (stream == &data->vfStream_)\n> > +\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> \n> \t\telse\n> \t\t\tcontinue;\n\nI had it like this in v1 but Jacopo preferred the v2 version. I \npreferred the v1 version just like you so I will use it as Jacopo \nexpressed no strong feelings.\n\n> \n> or\n> \n> \t\telse\n> \t\t\tret = 0;\n> \n> and dropping the previous &data->rawStream_ check would allow not\n> initializing the ret variable to 0.\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> > +\n> >  \t\tif (ret < 0)\n> >  \t\t\terror = ret;\n> >  \t}\n> > @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()\n> >  \t\t * second.\n> >  \t\t */\n> >  \t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> > -\t\tdata->outStream_.device_ = &data->imgu_->output_;\n> > -\t\tdata->vfStream_.device_ = &data->imgu_->viewfinder_;\n> > -\t\tdata->rawStream_.raw_ = true;\n> >  \n> >  \t\t/*\n> >  \t\t * Connect video devices' 'bufferReady' signals to their\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 86AB9C2E69\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 28 Jun 2020 12:03:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE88F609DD;\n\tSun, 28 Jun 2020 14:03:47 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 674FA603B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Jun 2020 14:03:46 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id n23so14983194ljh.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Jun 2020 05:03:46 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr23sm4004932ljh.76.2020.06.28.05.03.44\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 28 Jun 2020 05:03:44 -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=\"IW61fvHz\"; 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=eHpPQMWBen+TVRrr5vIc9iWADrFIEMVor9ozEI5xiHo=;\n\tb=IW61fvHzlNntbj2ta/QRzGGXMF/LmwnBmeIwGOY1CYQiVD3Ynb1tHpa0Y0oWUXNTWi\n\t9k1iSvfoCudpSs9nUj22MPcftaJDiBznA+mm3/d+ZXfPQAm1zhGti3A51ktbEDBZC7b/\n\tgyhAdIgx32wjg6a2KNd623dYQzd54pQliNl0O3tuKiP9aSYpMvqRXF+95LRv5dqYUhwC\n\tN069SyC//Vex2Ti2qkyEKkDfK5ZdMBDJmtU3wO9a1cMAlAkvfRRfXM4uBamLvWJmuyBE\n\tZ+84Q4mpaQ1C/E4+eQiQnU8TVf2TdoHGnNdYZlWudalCA7c7DcW4F2FkKMWQZnW032yi\n\tctjQ==","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=eHpPQMWBen+TVRrr5vIc9iWADrFIEMVor9ozEI5xiHo=;\n\tb=PXZSjf8+Erv0Ac9ytYkD2Z3PEoyh5DsiLN43GmV9C0SK8O8Fpftn1V8OvO9dYaxhpS\n\tWJ3uYRonR1qn+DkYy+FD+dppbhAxXtinC5V648BkVnF/EoM7eyYK7OT3emIArm95aRST\n\thpEDjTAcPlprsBQmSyxUzwsVw/3oRT4nUfHuPaxMX9MVR4MjRj57Zx0AKuCeL2PhSl5h\n\twiTECfsz02+fGt7bfmENcAgjDY9pkd4uP+CrhXxAzhrIfPKmxcRz3w1+Kxf3ZK1t843b\n\tN4lasR71tNCRttUfUWFpMZazCYY7MnQZlHL7k1u6ezQbX67o/PmyZK7GctDOGwXQkNLh\n\tLSpw==","X-Gm-Message-State":"AOAM531S7iNcBBb5ETHruvrIoKixC5wbY2u2WURejK4+4WsutGd8FgKz\n\t1zbqVmCYCe0Fj9osgNJOTAdaAhzQf0w=","X-Google-Smtp-Source":"ABdhPJwaNAS4/gCvslcGOo40p9Oy6Ot2jBEzZJ2je9fontJeYQ+tTpTFwsNBfB3qsp8EzVi4ZMiwvg==","X-Received":"by 2002:a2e:154d:: with SMTP id 13mr5194026ljv.21.1593345825434; \n\tSun, 28 Jun 2020 05:03:45 -0700 (PDT)","Date":"Sun, 28 Jun 2020 14:03:44 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20200628120344.GG1105424@oden.dyn.berto.se>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-10-niklas.soderlund@ragnatech.se>\n\t<20200628070637.GB6954@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628070637.GB6954@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":10932,"web_url":"https://patchwork.libcamera.org/comment/10932/","msgid":"<20200628121843.dudnjv333jrxzw7z@uno.localdomain>","date":"2020-06-28T12:18:43","subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sun, Jun 28, 2020 at 02:03:44PM +0200, Niklas Söderlund wrote:\n> Hi Laurent,\n>\n> Thanks for your feedback.\n>\n> On 2020-06-28 10:06:37 +0300, Laurent Pinchart wrote:\n> > Hi Niklas,\n> >\n> > Thank you for the patch.\n> >\n> > On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:\n> > > Do not keep the duplicated ImgUDevice::ImgUOutput information in both\n> > > the stream and camera data classes. Remove it from the stream and only\n> > > access it from the camera data class.\n> > >\n> > > Which stream is which can instead be checked by comparing it to the\n> > > known streams in camera data. This match how streams are checked in\n> > > other parts of the code making the pipeline more coherent.\n> > >\n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > > * Changes since v1\n> > > - s/driver/pipeline/ in commit message.\n> > > - Reorder if ; if else ; else statement in\n> > >   PipelineHandlerIPU3::queueRequestDevice()\n> > > ---\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------\n> > >  1 file changed, 18 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index e817f842f1216a7f..c1520ec40fe7b57c 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream\n> > >  {\n> > >  public:\n> > >  \tIPU3Stream()\n> > > -\t\t: active_(false), raw_(false), device_(nullptr)\n> > > +\t\t: active_(false)\n> > >  \t{\n> > >  \t}\n> > >\n> > >  \tbool active_;\n> > > -\tbool raw_;\n> > > -\tImgUDevice::ImgUOutput *device_;\n> > >  };\n> > >\n> > >  class IPU3CameraData : public CameraData\n> > > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >  \t\tconst StreamConfiguration oldCfg = cfg;\n> > >  \t\tconst IPU3Stream *stream = streams_[i];\n> > >\n> > > -\t\tif (stream->raw_) {\n> > > +\t\tif (stream == &data_->rawStream_) {\n> > >  \t\t\tcfg = cio2Configuration_;\n> > >  \t\t} else {\n> > >  \t\t\tbool scale = stream == &data_->vfStream_;\n> > > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > >  \t\t\t\t\t    std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> > >  {\n> > >  \tIPU3CameraData *data = cameraData(camera);\n> > > -\tIPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);\n> > >  \tunsigned int count = stream->configuration().bufferCount;\n> > >\n> > > -\tif (ipu3stream->raw_)\n> > > +\tif (stream == &data->outStream_)\n> > > +\t\treturn data->imgu_->output_.dev->exportBuffers(count, buffers);\n> > > +\telse if (stream == &data->vfStream_)\n> > > +\t\treturn data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);\n> > > +\telse if (stream == &data->rawStream_)\n> > >  \t\treturn data->cio2_.exportBuffers(count, buffers);\n> > >\n> > > -\treturn ipu3stream->device_->dev->exportBuffers(count, buffers);\n> > > +\treturn -EINVAL;\n> > >  }\n> > >\n> > >  /**\n> > > @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > >  \t/* Queue all buffers from the request aimed for the ImgU. */\n> > >  \tfor (auto it : request->buffers()) {\n> > >  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(it.first);\n> > > -\t\tif (stream->raw_)\n> > > -\t\t\tcontinue;\n> > > -\n> > >  \t\tFrameBuffer *buffer = it.second;\n> > > -\t\tint ret = stream->device_->dev->queueBuffer(buffer);\n> > > +\n> > > +\t\tif (stream == &data->rawStream_)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tint ret = 0;\n> > > +\t\tif (stream == &data->outStream_)\n> > > +\t\t\tret = data->imgu_->output_.dev->queueBuffer(buffer);\n> > > +\t\telse if (stream == &data->vfStream_)\n> > > +\t\t\tret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);\n> >\n> > \t\telse\n> > \t\t\tcontinue;\n>\n> I had it like this in v1 but Jacopo preferred the v2 version. I\n> preferred the v1 version just like you so I will use it as Jacopo\n> expressed no strong feelings.\n>\n\nAbsolutely not, was just a suggestion. Sorry for the ping-pong.\n\n> >\n> > or\n> >\n> > \t\telse\n> > \t\t\tret = 0;\n> >\n> > and dropping the previous &data->rawStream_ check would allow not\n> > initializing the ret variable to 0.\n> >\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >\n> > > +\n> > >  \t\tif (ret < 0)\n> > >  \t\t\terror = ret;\n> > >  \t}\n> > > @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()\n> > >  \t\t * second.\n> > >  \t\t */\n> > >  \t\tdata->imgu_ = numCameras ? &imgu1_ : &imgu0_;\n> > > -\t\tdata->outStream_.device_ = &data->imgu_->output_;\n> > > -\t\tdata->vfStream_.device_ = &data->imgu_->viewfinder_;\n> > > -\t\tdata->rawStream_.raw_ = true;\n> > >\n> > >  \t\t/*\n> > >  \t\t * Connect video devices' 'bufferReady' signals to their\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 0BF5EC2E66\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 28 Jun 2020 12:15:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A4CC609C9;\n\tSun, 28 Jun 2020 14:15:13 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AB74603B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 28 Jun 2020 14:15:12 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 8B0C5240006;\n\tSun, 28 Jun 2020 12:15:11 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sun, 28 Jun 2020 14:18:43 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20200628121843.dudnjv333jrxzw7z@uno.localdomain>","References":"<20200628001532.2685967-1-niklas.soderlund@ragnatech.se>\n\t<20200628001532.2685967-10-niklas.soderlund@ragnatech.se>\n\t<20200628070637.GB6954@pendragon.ideasonboard.com>\n\t<20200628120344.GG1105424@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628120344.GG1105424@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not\n\tduplicate data in IPU3Stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]