[{"id":10967,"web_url":"https://patchwork.libcamera.org/comment/10967/","msgid":"<20200629145109.GF1852218@oden.dyn.berto.se>","date":"2020-06-29T14:51:09","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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 work.\n\nOn 2020-06-29 02:19:31 +0300, Laurent Pinchart wrote:\n> The IPAInterface now accepts custom configuration data. Use it to pass\n> the lens shading table instead of using a custom IPA event. This will\n> allow starting the IPA when starting the camera, instead of pre-starting\n> it early in order to process the lens shading table allocation event.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> 3 files changed, 14 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index a18ce9a884b6..c335d0259549 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -10,6 +10,10 @@\n> #include \n> #include \n> \n> +enum RPiConfigParameters {\n> +\tRPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> +};\n> +\n> enum RPiOperations {\n> \tRPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> \tRPI_IPA_ACTION_V4L2_SET_ISP,\n> @@ -21,7 +25,6 @@ enum RPiOperations {\n> \tRPI_IPA_EVENT_SIGNAL_STAT_READY,\n> \tRPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> -\tRPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> };\n> \n> enum RPiIpaMask {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 860be22ddb5d..c9f7dea375a5 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tapplyAGC(&agcStatus);\n> \n> \tlastMode_ = mode_;\n> +\n> +\t/* Store the lens shading table pointer and handle if available. */\n> +\tif (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n> +\t\tlsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> +\t\tlsTableHandle_ = ipaConfig.data[1];\n> +\t}\n> }\n> \n> void IPARPi::mapBuffers(const std::vector &buffers)\n> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\tbreak;\n> \t}\n> \n> -\tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> -\t\tlsTableHandle_ = event.data[1];\n> -\t\tbreak;\n> -\t}\n> -\n> \tdefault:\n> \t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> \t\tbreak;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0f9237a7f346..903796790f44 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> {\n> \tstd::map streamConfig;\n> \tstd::map entityControls;\n> +\tIPAOperationData ipaConfig = {};\n> \n> \t/* Get the device format to pass to the IPA. */\n> \tV4L2DeviceFormat sensorFormat;\n> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> \t\t * The vcsm allocation will always be in the memory region\n> \t\t * < 32-bits to allow Videocore to access the memory.\n> \t\t */\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> -\t\top.data = { static_cast(ptr & 0xffffffff),\n> -\t\t\t vcsm_.getVCHandle(lsTable_) };\n> -\t\tipa_->processEvent(op);\n> +\t\tipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> +\t\tipaConfig.data = { static_cast(ptr & 0xffffffff),\n> +\t\t\t\t vcsm_.getVCHandle(lsTable_) };\n\nSending a pointer to the IPA caught my eye as potentially dangerous. If \nI understand the situation correctly this is a temporary workaround \nwhile vc_sm_cma is reworked to support dmabuf? Do we know the status of \nthat work? In the mean time should we add a warning/todo here so we know \nwhats going on if we ever troubleshoot an issue to this location?\n\nThis have however nothing to do with this patch,\n\nReviewed-by: Niklas Söderlund \n\n> \t}\n> \n> \tCameraSensorInfo sensorInfo = {};\n> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> \t}\n> \n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tIPAOperationData ipaConfig;\n> \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> \t\t\tnullptr);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\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 E8CCEBFFE2\n\tfor ;\n\tMon, 29 Jun 2020 14:51:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BDD5609C9;\n\tMon, 29 Jun 2020 16:51:12 +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 D2106603B4\n\tfor ;\n\tMon, 29 Jun 2020 16:51:11 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id b25so14747672ljp.6\n\tfor ;\n\tMon, 29 Jun 2020 07:51:11 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq2sm25398ljp.135.2020.06.29.07.51.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Jun 2020 07:51:10 -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=\"dX+59Efv\"; 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=wEQL0A2Zgr60JYYz6vDT6137Ktsl/HF5LDRePVWrgKw=;\n\tb=dX+59EfvclfaXqQhgnD5nOpWyj/Mi0lP9TlkUYQxWGzyefKmG7ba+kSaIXrDOh4ZFJ\n\tBfoa5chkfUAMHfRMc0E+FqU5QjuGWdFOazRkda2Wm+qRACoGUj9NXNxgDCDrFmFO0ZEt\n\tBpDDGg+qe4yALJyINTQ/qc3fjXwrGFsrvxEXXllOzGI7/Hc8s33GCW0NIuDybcMi18+V\n\t0sey2SlK3KhvJMAGpLLVWmBX84a/YUlC6/FSMPLn9As8yi66KrWMqo2/fM+mn5vSezLQ\n\t8m0NMNsagf4mmIWAuJBdMtWQBMnAfkLMHFobr8y5hbdafAGiv+j4lcETHq6fhEzTKhUH\n\t/Pmg==","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=wEQL0A2Zgr60JYYz6vDT6137Ktsl/HF5LDRePVWrgKw=;\n\tb=p33eUsgQbLL+4MBxIYDe2dV+5wGNLeJOsuQdFn8c3ginlGBTCsIrDbxE2EhmJzNodo\n\tljCUddz/Kj2h0nmwEm5OW5GqEqzAeKVuPH+unxtG3J1Xaa082bTe4PYG54YZjYwVq1H3\n\tmfNuIRZoTI/veZQAXAhweUz24aC7fml/311xMdSM6usxTw76uTKkCAdcTENcSr+7tqsc\n\t4ovoYlgu5Yf/jpRHIkXhK46cvAYOVp/751NrblcfGb4j9M1JtxlC9F3As5mCqhCBf6gq\n\tyOhcVeZbrsj4roUmvAOIFkwvOUhy9g3lH9yIJO7qd0Mm/JgOMb58K8BNXtyUgztApdQr\n\tbotA==","X-Gm-Message-State":"AOAM531paRzZ9fqbVo0M0XLvt91WrVQLN6eQw14WwtHt/FshZ3ez1/Wp\n\tFqmeLteHdnJt0wjfnEfVqd48LgeAeEo=","X-Google-Smtp-Source":"ABdhPJyzUuMqdUVCG5QsjxWT1t5ITP+jhZLjZH9Vze4jDxv02Ewlks3c6jdaG66Fd5YS8Q84owOisQ==","X-Received":"by 2002:a2e:312:: with SMTP id 18mr7846954ljd.423.1593442271254; \n\tMon, 29 Jun 2020 07:51:11 -0700 (PDT)","Date":"Mon, 29 Jun 2020 16:51:09 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200629145109.GF1852218@oden.dyn.berto.se>","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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\" "}},{"id":10971,"web_url":"https://patchwork.libcamera.org/comment/10971/","msgid":"<20200629150356.GH10681@pendragon.ideasonboard.com>","date":"2020-06-29T15:03:56","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\n(CC'ing Dave)\n\nOn Mon, Jun 29, 2020 at 04:51:09PM +0200, Niklas Söderlund wrote:\n> On 2020-06-29 02:19:31 +0300, Laurent Pinchart wrote:\n> > The IPAInterface now accepts custom configuration data. Use it to pass\n> > the lens shading table instead of using a custom IPA event. This will\n> > allow starting the IPA when starting the camera, instead of pre-starting\n> > it early in order to process the lens shading table allocation event.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> > 3 files changed, 14 insertions(+), 13 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index a18ce9a884b6..c335d0259549 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -10,6 +10,10 @@\n> > #include \n> > #include \n> > \n> > +enum RPiConfigParameters {\n> > +\tRPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> > +};\n> > +\n> > enum RPiOperations {\n> > \tRPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> > \tRPI_IPA_ACTION_V4L2_SET_ISP,\n> > @@ -21,7 +25,6 @@ enum RPiOperations {\n> > \tRPI_IPA_EVENT_SIGNAL_STAT_READY,\n> > \tRPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> > \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> > -\tRPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> > };\n> > \n> > enum RPiIpaMask {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 860be22ddb5d..c9f7dea375a5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\tapplyAGC(&agcStatus);\n> > \n> > \tlastMode_ = mode_;\n> > +\n> > +\t/* Store the lens shading table pointer and handle if available. */\n> > +\tif (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n> > +\t\tlsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> > +\t\tlsTableHandle_ = ipaConfig.data[1];\n> > +\t}\n> > }\n> > \n> > void IPARPi::mapBuffers(const std::vector &buffers)\n> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \t\tbreak;\n> > \t}\n> > \n> > -\tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> > -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> > -\t\tlsTableHandle_ = event.data[1];\n> > -\t\tbreak;\n> > -\t}\n> > -\n> > \tdefault:\n> > \t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > \t\tbreak;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0f9237a7f346..903796790f44 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> > {\n> > \tstd::map streamConfig;\n> > \tstd::map entityControls;\n> > +\tIPAOperationData ipaConfig = {};\n> > \n> > \t/* Get the device format to pass to the IPA. */\n> > \tV4L2DeviceFormat sensorFormat;\n> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> > \t\t * The vcsm allocation will always be in the memory region\n> > \t\t * < 32-bits to allow Videocore to access the memory.\n> > \t\t */\n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> > -\t\top.data = { static_cast(ptr & 0xffffffff),\n> > -\t\t\t vcsm_.getVCHandle(lsTable_) };\n> > -\t\tipa_->processEvent(op);\n> > +\t\tipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> > +\t\tipaConfig.data = { static_cast(ptr & 0xffffffff),\n> > +\t\t\t\t vcsm_.getVCHandle(lsTable_) };\n> \n> Sending a pointer to the IPA caught my eye as potentially dangerous. If \n> I understand the situation correctly this is a temporary workaround \n> while vc_sm_cma is reworked to support dmabuf? Do we know the status of \n> that work?\n\nDave, do you have any update on this ?\n\n> In the mean time should we add a warning/todo here so we know \n> whats going on if we ever troubleshoot an issue to this location?\n\nA warning would be a bit too verbose I think. We can add a \\todo\ncomment. Would you like to submit a patch ? You can use master as a\nbase, and I'll handle the rebase.\n\n> This have however nothing to do with this patch,\n> \n> Reviewed-by: Niklas Söderlund \n> \n> > \t}\n> > \n> > \tCameraSensorInfo sensorInfo = {};\n> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> > \t}\n> > \n> > \t/* Ready the IPA - it must know about the sensor resolution. */\n> > -\tIPAOperationData ipaConfig;\n> > \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > \t\t\tnullptr);\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 B157CBF415\n\tfor ;\n\tMon, 29 Jun 2020 15:04:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39D85609D6;\n\tMon, 29 Jun 2020 17:04:02 +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 DC9FF603B4\n\tfor ;\n\tMon, 29 Jun 2020 17:04:00 +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 33FFF299;\n\tMon, 29 Jun 2020 17:04:00 +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=\"TW2Ze+sP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593443040;\n\tbh=V570liSj8ewPPQJ7fcFdimushn4TWD85s01E74Fp+ns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TW2Ze+sP7jOIiHawy9BtxeGGMUgjCupkO1ttq4pyNcxkjnZNiQtKfy+B7YeBWfI7t\n\tsCap1IEJabMF0lDI1Xo1KUgKHspVw+Q39YdcRh6w6/f+vNHurITCEb59w2DTGfRFpq\n\tspzuypF+BwCT2ID72Sb86FKro8xGYS7SvpPHZi7M=","Date":"Mon, 29 Jun 2020 18:03:56 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200629150356.GH10681@pendragon.ideasonboard.com>","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>\n\t<20200629145109.GF1852218@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200629145109.GF1852218@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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":11010,"web_url":"https://patchwork.libcamera.org/comment/11010/","msgid":"","date":"2020-06-30T10:38:21","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your patch.\n\nOn Mon, 29 Jun 2020 at 00:19, Laurent Pinchart\n wrote:\n>\n> The IPAInterface now accepts custom configuration data. Use it to pass\n> the lens shading table instead of using a custom IPA event. This will\n> allow starting the IPA when starting the camera, instead of pre-starting\n> it early in order to process the lens shading table allocation event.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> 3 files changed, 14 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index a18ce9a884b6..c335d0259549 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -10,6 +10,10 @@\n> #include \n> #include \n>\n> +enum RPiConfigParameters {\n> + RPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> +};\n\nMinor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to\nbe consistent with other labels?\n\nOtherwise all looks good.\n\nReviewed-by: Naushir Patuck \n\n\n\n> +\n> enum RPiOperations {\n> RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> RPI_IPA_ACTION_V4L2_SET_ISP,\n> @@ -21,7 +25,6 @@ enum RPiOperations {\n> RPI_IPA_EVENT_SIGNAL_STAT_READY,\n> RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> RPI_IPA_EVENT_QUEUE_REQUEST,\n> - RPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> };\n>\n> enum RPiIpaMask {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 860be22ddb5d..c9f7dea375a5 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> applyAGC(&agcStatus);\n>\n> lastMode_ = mode_;\n> +\n> + /* Store the lens shading table pointer and handle if available. */\n> + if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n> + lsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> + lsTableHandle_ = ipaConfig.data[1];\n> + }\n> }\n>\n> void IPARPi::mapBuffers(const std::vector &buffers)\n> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> break;\n> }\n>\n> - case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> - lsTable_ = reinterpret_cast(event.data[0]);\n> - lsTableHandle_ = event.data[1];\n> - break;\n> - }\n> -\n> default:\n> LOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> break;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0f9237a7f346..903796790f44 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> {\n> std::map streamConfig;\n> std::map entityControls;\n> + IPAOperationData ipaConfig = {};\n>\n> /* Get the device format to pass to the IPA. */\n> V4L2DeviceFormat sensorFormat;\n> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> * The vcsm allocation will always be in the memory region\n> * < 32-bits to allow Videocore to access the memory.\n> */\n> - IPAOperationData op;\n> - op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> - op.data = { static_cast(ptr & 0xffffffff),\n> - vcsm_.getVCHandle(lsTable_) };\n> - ipa_->processEvent(op);\n> + ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> + ipaConfig.data = { static_cast(ptr & 0xffffffff),\n> + vcsm_.getVCHandle(lsTable_) };\n> }\n>\n> CameraSensorInfo sensorInfo = {};\n> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> }\n>\n> /* Ready the IPA - it must know about the sensor resolution. */\n> - IPAOperationData ipaConfig;\n> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> nullptr);\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 D651ABFFE2\n\tfor ;\n\tTue, 30 Jun 2020 10:38:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B8F60C59;\n\tTue, 30 Jun 2020 12:38:40 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEA31609C7\n\tfor ;\n\tTue, 30 Jun 2020 12:38:38 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id b25so18264592ljp.6\n\tfor ;\n\tTue, 30 Jun 2020 03:38:38 -0700 (PDT)"],"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=\"oSVdfErv\"; 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=YaiFYjKP4KUlead9G0UV6P8PQhY27nFkLpQGMGE32hE=;\n\tb=oSVdfErvboH/r24loQNzNyPjKQLw3vAtOHicI9+3ESitXJWO5hcEQ/bMDwIJb1AwGg\n\tuv+AqSMLUWH0pU7GJJqxXuTqCschVSBYnQ3c9L/ze4qINkpVBuLtVBhYphUWEtRe+SRX\n\tZNj1GHfFHNIHc+zNIJ9wjltprYAx13gDDwQ9YNnOtU4/J/6HiU/vE3ev7O73HJXTr0DU\n\toLE/lmW59AsUEOwsUlJlnDoThdmn8ghjBDfBUNJ7SeUloxoZFFFhEHzqmYPqHWWE5o+K\n\tPOn+0Q7fvbTVV9siEkkYKQAokcbP5z+l2OFFzVyaJjAQGHMMUIQudcpdpOwZFSYe29LE\n\tSjSQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=YaiFYjKP4KUlead9G0UV6P8PQhY27nFkLpQGMGE32hE=;\n\tb=LDWEkSVSq0LMNs4qB2bsIJ2ZQ55+WENsiINZrYiqfoDQd3l9tWnikfBrw0Bfq3pvNt\n\tZDgnSwjLdmHvQjLra8ub3KW4WQG1ukVUAP8QxeZlLT++BBk2w+ZC3g+boz23sA2Va7M/\n\tUMBz9G3N+3nQNVH95Oz57oQRtEjvFyMPvMRLQizDTTsCWGDhTrJNfcRKFzNLl3K+rZ+4\n\ttZKulrodHS0db8cpupGgqG2e5eqTDlVUDjqkUX7N8PvLkSjB2Vgnra/AEuxym/wGkK8m\n\tRa2OGyRePkILzcU+rbGG66xGX3w5YjkfHdkfvZEYvmXOCZ7UZ2xt39t4SYGNTGDAUPZ3\n\t9T9w==","X-Gm-Message-State":"AOAM533TbSl2Mmr+VBlzN1bcNh3uvN5ouGFZN1I54PefclM6xG6l7tTf\n\tNfzJvFK341GJ7Lk/6i5bhw2agzhb/Z/CEa4FgiR4Vw==","X-Google-Smtp-Source":"ABdhPJwqRKcO2wd4tFOi46eR9JIUS+diWMhyeHaLMQnG2UZAS+tN2qCGIJ/reA1glHEV2G7PQVFVF+y3B0P0Q11sVFY=","X-Received":"by 2002:a05:651c:508:: with SMTP id\n\to8mr9891761ljp.112.1593513518069; \n\tTue, 30 Jun 2020 03:38:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 30 Jun 2020 11:38:21 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11011,"web_url":"https://patchwork.libcamera.org/comment/11011/","msgid":"<20200630104508.c2ytfp3fiqfsucsf@uno.localdomain>","date":"2020-06-30T10:45:08","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 29, 2020 at 02:19:31AM +0300, Laurent Pinchart wrote:\n> The IPAInterface now accepts custom configuration data. Use it to pass\n\nThe IPAInterface::configure()\n\n> the lens shading table instead of using a custom IPA event. This will\n> allow starting the IPA when starting the camera, instead of pre-starting\n> it early in order to process the lens shading table allocation event.\n\nIf the IPA is meant to be started at Camera::start() time, and we pass\nthe lens shading table at ipa_->configure() time which happens before,\nwhat is different ?\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> 3 files changed, 14 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index a18ce9a884b6..c335d0259549 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -10,6 +10,10 @@\n> #include \n> #include \n>\n> +enum RPiConfigParameters {\n> +\tRPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> +};\n> +\n\nIs there any value in keeping this enum separated if not creating a\ndistinction between operations run through processEvent() and\noperations run at configure() time ? Can't the list of operation be\nunique (which would also avoid calshing, if the same operations has\nto be handled both at processEvent() and configure() time.\n\nAlso, why (1 << 0) in an enum ? Can't we start from 0 (or 1 as\nRPiOperations does) and increase ?\n\n> enum RPiOperations {\n> \tRPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> \tRPI_IPA_ACTION_V4L2_SET_ISP,\n> @@ -21,7 +25,6 @@ enum RPiOperations {\n> \tRPI_IPA_EVENT_SIGNAL_STAT_READY,\n> \tRPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> -\tRPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> };\n>\n> enum RPiIpaMask {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 860be22ddb5d..c9f7dea375a5 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tapplyAGC(&agcStatus);\n>\n> \tlastMode_ = mode_;\n> +\n> +\t/* Store the lens shading table pointer and handle if available. */\n> +\tif (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n\nThat's why (1 << 0). What is the advantage of using flags, instead of\nswitching on the operation identifiers as done below in\nprocessEvent() ?\n\n> +\t\tlsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> +\t\tlsTableHandle_ = ipaConfig.data[1];\n> +\t}\n> }\n>\n> void IPARPi::mapBuffers(const std::vector &buffers)\n> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> \t\tbreak;\n> \t}\n>\n> -\tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> -\t\tlsTableHandle_ = event.data[1];\n> -\t\tbreak;\n> -\t}\n> -\n> \tdefault:\n> \t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> \t\tbreak;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0f9237a7f346..903796790f44 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> {\n> \tstd::map streamConfig;\n> \tstd::map entityControls;\n> +\tIPAOperationData ipaConfig = {};\n>\n> \t/* Get the device format to pass to the IPA. */\n> \tV4L2DeviceFormat sensorFormat;\n> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> \t\t * The vcsm allocation will always be in the memory region\n> \t\t * < 32-bits to allow Videocore to access the memory.\n> \t\t */\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> -\t\top.data = { static_cast(ptr & 0xffffffff),\n> -\t\t\t vcsm_.getVCHandle(lsTable_) };\n> -\t\tipa_->processEvent(op);\n> +\t\tipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> +\t\tipaConfig.data = { static_cast(ptr & 0xffffffff),\n> +\t\t\t\t vcsm_.getVCHandle(lsTable_) };\n> \t}\n>\n> \tCameraSensorInfo sensorInfo = {};\n> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> \t}\n>\n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tIPAOperationData ipaConfig;\n> \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> \t\t\tnullptr);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 6DF16BFFE2\n\tfor ;\n\tTue, 30 Jun 2020 10:41:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11CB660C57;\n\tTue, 30 Jun 2020 12:41:41 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC862609C7\n\tfor ;\n\tTue, 30 Jun 2020 12:41:39 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5EEEF1C0014;\n\tTue, 30 Jun 2020 10:41:39 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 30 Jun 2020 12:45:08 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200630104508.c2ytfp3fiqfsucsf@uno.localdomain>","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11084,"web_url":"https://patchwork.libcamera.org/comment/11084/","msgid":"<20200702213123.GS12562@pendragon.ideasonboard.com>","date":"2020-07-02T21:31:23","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Jun 30, 2020 at 12:45:08PM +0200, Jacopo Mondi wrote:\n> On Mon, Jun 29, 2020 at 02:19:31AM +0300, Laurent Pinchart wrote:\n> > The IPAInterface now accepts custom configuration data. Use it to pass\n> \n> The IPAInterface::configure()\n> \n> > the lens shading table instead of using a custom IPA event. This will\n> > allow starting the IPA when starting the camera, instead of pre-starting\n> > it early in order to process the lens shading table allocation event.\n> \n> If the IPA is meant to be started at Camera::start() time, and we pass\n> the lens shading table at ipa_->configure() time which happens before,\n> what is different ?\n\nToday the RPi IPA is started much earlier, to work around the lack of\nAPI to pass custom configuration data. This change allows starting the\nIPA at the right time.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> > 3 files changed, 14 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index a18ce9a884b6..c335d0259549 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -10,6 +10,10 @@\n> > #include \n> > #include \n> >\n> > +enum RPiConfigParameters {\n> > +\tRPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> > +};\n> > +\n> \n> Is there any value in keeping this enum separated if not creating a\n> distinction between operations run through processEvent() and\n> operations run at configure() time ? Can't the list of operation be\n> unique (which would also avoid calshing, if the same operations has\n> to be handled both at processEvent() and configure() time.\n\nThey're different things though, so I think separate enums are best.\nIdeally the IPAOperationData operation field should be an enum field of\nthe appropriate type. See the comment about IPC below.\n\n> Also, why (1 << 0) in an enum ? Can't we start from 0 (or 1 as\n> RPiOperations does) and increase ?\n\nSee below :-)\n\n> > enum RPiOperations {\n> > \tRPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> > \tRPI_IPA_ACTION_V4L2_SET_ISP,\n> > @@ -21,7 +25,6 @@ enum RPiOperations {\n> > \tRPI_IPA_EVENT_SIGNAL_STAT_READY,\n> > \tRPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> > \tRPI_IPA_EVENT_QUEUE_REQUEST,\n> > -\tRPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> > };\n> >\n> > enum RPiIpaMask {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 860be22ddb5d..c9f7dea375a5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\tapplyAGC(&agcStatus);\n> >\n> > \tlastMode_ = mode_;\n> > +\n> > +\t/* Store the lens shading table pointer and handle if available. */\n> > +\tif (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n> \n> That's why (1 << 0). What is the advantage of using flags, instead of\n> switching on the operation identifiers as done below in\n> processEvent() ?\n\nSee the rest of the series. This is used to pass multiple configuration\nparameters, while processEvents() and queueFrameAction only handle a\nsingle operation. I'd like to make the two more similar though, but I\nbelieve it will involve a rework of how we handle IPA-specific\nparameters, including for IPC serialization, and possibly involve some\ntype of IDL. I'd thus rather not invest too much time now in something\nthat will very likely change.\n\n> > +\t\tlsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> > +\t\tlsTableHandle_ = ipaConfig.data[1];\n> > +\t}\n> > }\n> >\n> > void IPARPi::mapBuffers(const std::vector &buffers)\n> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > \t\tbreak;\n> > \t}\n> >\n> > -\tcase RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> > -\t\tlsTable_ = reinterpret_cast(event.data[0]);\n> > -\t\tlsTableHandle_ = event.data[1];\n> > -\t\tbreak;\n> > -\t}\n> > -\n> > \tdefault:\n> > \t\tLOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > \t\tbreak;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0f9237a7f346..903796790f44 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> > {\n> > \tstd::map streamConfig;\n> > \tstd::map entityControls;\n> > +\tIPAOperationData ipaConfig = {};\n> >\n> > \t/* Get the device format to pass to the IPA. */\n> > \tV4L2DeviceFormat sensorFormat;\n> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> > \t\t * The vcsm allocation will always be in the memory region\n> > \t\t * < 32-bits to allow Videocore to access the memory.\n> > \t\t */\n> > -\t\tIPAOperationData op;\n> > -\t\top.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> > -\t\top.data = { static_cast(ptr & 0xffffffff),\n> > -\t\t\t vcsm_.getVCHandle(lsTable_) };\n> > -\t\tipa_->processEvent(op);\n> > +\t\tipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> > +\t\tipaConfig.data = { static_cast(ptr & 0xffffffff),\n> > +\t\t\t\t vcsm_.getVCHandle(lsTable_) };\n> > \t}\n> >\n> > \tCameraSensorInfo sensorInfo = {};\n> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> > \t}\n> >\n> > \t/* Ready the IPA - it must know about the sensor resolution. */\n> > -\tIPAOperationData ipaConfig;\n> > \tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > \t\t\tnullptr);\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 7AC12BFFE2\n\tfor ;\n\tThu, 2 Jul 2020 21:31:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14FA4609C7;\n\tThu, 2 Jul 2020 23:31:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22E5F603B4\n\tfor ;\n\tThu, 2 Jul 2020 23:31:28 +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 46F049CB;\n\tThu, 2 Jul 2020 23:31:27 +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=\"CFL7ujwn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593725487;\n\tbh=5RLj1jzq2Vkjp5ksdso3CeOq5UvxyFvFafGkPgwzm64=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CFL7ujwnWN0OJsfhBbId7N8rLFIMossCEU4Nfyn+wZvctNVUEpItlCOvbeF9QqtgU\n\tmS8KxOPbw0qzoqYmMes/NjLwIK939vDD61oDOZFn9UZECvV4aknaqnvL/gy6VHRwvo\n\tSXerugliIv7cJ5PvNLhpNfSKQNyMQ7k8IrI3KaCQ=","Date":"Fri, 3 Jul 2020 00:31:23 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200702213123.GS12562@pendragon.ideasonboard.com>","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>\n\t<20200630104508.c2ytfp3fiqfsucsf@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200630104508.c2ytfp3fiqfsucsf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11164,"web_url":"https://patchwork.libcamera.org/comment/11164/","msgid":"<20200703192329.GA26361@pendragon.ideasonboard.com>","date":"2020-07-03T19:23:29","subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 30, 2020 at 11:38:21AM +0100, Naushir Patuck wrote:\n> On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart wrote:\n> >\n> > The IPAInterface now accepts custom configuration data. Use it to pass\n> > the lens shading table instead of using a custom IPA event. This will\n> > allow starting the IPA when starting the camera, instead of pre-starting\n> > it early in order to process the lens shading table allocation event.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 5 ++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++------\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------\n> > 3 files changed, 14 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index a18ce9a884b6..c335d0259549 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -10,6 +10,10 @@\n> > #include \n> > #include \n> >\n> > +enum RPiConfigParameters {\n> > + RPI_IPA_CONFIG_LSTABLE = (1 << 0),\n> > +};\n> \n> Minor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to\n> be consistent with other labels?\n\nSure, will do.\n\n> Otherwise all looks good.\n> \n> Reviewed-by: Naushir Patuck \n> \n> > +\n> > enum RPiOperations {\n> > RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,\n> > RPI_IPA_ACTION_V4L2_SET_ISP,\n> > @@ -21,7 +25,6 @@ enum RPiOperations {\n> > RPI_IPA_EVENT_SIGNAL_STAT_READY,\n> > RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,\n> > RPI_IPA_EVENT_QUEUE_REQUEST,\n> > - RPI_IPA_EVENT_LS_TABLE_ALLOCATION,\n> > };\n> >\n> > enum RPiIpaMask {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 860be22ddb5d..c9f7dea375a5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > applyAGC(&agcStatus);\n> >\n> > lastMode_ = mode_;\n> > +\n> > + /* Store the lens shading table pointer and handle if available. */\n> > + if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {\n> > + lsTable_ = reinterpret_cast(ipaConfig.data[0]);\n> > + lsTableHandle_ = ipaConfig.data[1];\n> > + }\n> > }\n> >\n> > void IPARPi::mapBuffers(const std::vector &buffers)\n> > @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)\n> > break;\n> > }\n> >\n> > - case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {\n> > - lsTable_ = reinterpret_cast(event.data[0]);\n> > - lsTableHandle_ = event.data[1];\n> > - break;\n> > - }\n> > -\n> > default:\n> > LOG(IPARPI, Error) << \"Unknown event \" << event.operation;\n> > break;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0f9237a7f346..903796790f44 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()\n> > {\n> > std::map streamConfig;\n> > std::map entityControls;\n> > + IPAOperationData ipaConfig = {};\n> >\n> > /* Get the device format to pass to the IPA. */\n> > V4L2DeviceFormat sensorFormat;\n> > @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()\n> > * The vcsm allocation will always be in the memory region\n> > * < 32-bits to allow Videocore to access the memory.\n> > */\n> > - IPAOperationData op;\n> > - op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;\n> > - op.data = { static_cast(ptr & 0xffffffff),\n> > - vcsm_.getVCHandle(lsTable_) };\n> > - ipa_->processEvent(op);\n> > + ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;\n> > + ipaConfig.data = { static_cast(ptr & 0xffffffff),\n> > + vcsm_.getVCHandle(lsTable_) };\n> > }\n> >\n> > CameraSensorInfo sensorInfo = {};\n> > @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()\n> > }\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > - IPAOperationData ipaConfig;\n> > ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > nullptr);\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 B12CEBE905\n\tfor ;\n\tFri, 3 Jul 2020 19:23:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41A9D609C5;\n\tFri, 3 Jul 2020 21:23:36 +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 5AF98609A9\n\tfor ;\n\tFri, 3 Jul 2020 21:23:34 +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 C837229E;\n\tFri, 3 Jul 2020 21:23:33 +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=\"HZ5y8k+E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593804214;\n\tbh=FCUdQJxCKe2St88qEka222yHpuDe53a8iklvGW7xeVM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HZ5y8k+Ero68zTt4fc95zs1n09/+Rlx2QeDhYqHjQ58sox0ejdyilZO2Q+BXzPuR3\n\tM1oQV2BrTsgrofbMhZwse9QrT8KG+TZOAAJRhDgvpLBM24nqmZ54ANVnyvRVOu6Ijq\n\te31CpFri60lMnssI4BC2cRJDKxjmaRBthpnFj1d0=","Date":"Fri, 3 Jul 2020 22:23:29 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200703192329.GA26361@pendragon.ideasonboard.com>","References":"<20200628231934.29025-1-laurent.pinchart@ideasonboard.com>\n\t<20200628231934.29025-7-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens\n\tshading table through configure() function","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]