[{"id":16096,"web_url":"https://patchwork.libcamera.org/comment/16096/","msgid":"","date":"2021-04-03T00:05:33","subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nI'd write the subject line as \"android: mm: cros: Handle buffer\nregistration failure\" to shorten it.\n\nOn Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:\n> cros::CameraBufferManager::Register() fails if a buffer handle\n> is invalid. We should mark CameraBuffer as invalid on the failure\n> of Register().\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----\n> 1 file changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 7df4f47c..5f98ee44 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> \n> -\tbufferManager_->Register(camera3Buffer);\n> +\tint ret = bufferManager_->Register(camera3Buffer);\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed registering a buffer: \" << ret;\n\ns/a buffer/buffer/\n\nThis looks good,\n\nReviewed-by: Laurent Pinchart \n\nI'll apply the patch if you're OK with the proposed changes.\n\nOne additional question though. The CameraBuffer::Private destructors\ncalls\n\n\tbufferManager_->Unlock(handle_);\n\tbufferManager_->Deregister(handle_);\n\nunconditionally. Is that safe to do if the buffer hasn't been registered\nand locked properly ?\n\n> +\t\treturn;\n> +\t}\n> \n> \tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> \tswitch (numPlanes_) {\n> \tcase 1: {\n> -\t\tint ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> +\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> \t\t\treturn;\n> @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \t}\n> \tcase 2:\n> \tcase 3: {\n> -\t\tint ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> -\t\t\t\t\t\t &mem.ycbcr);\n> +\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> +\t\t\t\t\t\t&mem.ycbcr);\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> \t\t\treturn;","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 5BFD8BD695\n\tfor ;\n\tSat, 3 Apr 2021 00:06:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE31268786;\n\tSat, 3 Apr 2021 02:06:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F890602CF\n\tfor ;\n\tSat, 3 Apr 2021 02:06:18 +0200 (CEST)","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 A7C923D7;\n\tSat, 3 Apr 2021 02:06:17 +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=\"f/Y3GkvG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617408377;\n\tbh=aSw4oWwmoL4au1wFOTcUorsJC9K+pWJfVU7Z6yMA03I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f/Y3GkvGbP0D8P05pRaHJdctyX5zpmf3YfCIC6+lbMYSCxVEqJvIWJ6nGAR3sK5NO\n\tLq7PTD4pdQBR0APmG7xChkhU6mjJcJWWhcuo8lHCmOPME/auKEs9sUB7YOvVWvREzC\n\tjKEo9smwk6kRAdN43sApTkI3uqlPZvFGUA9XK0ng=","Date":"Sat, 3 Apr 2021 03:05:33 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210402040340.1364749-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210402040340.1364749-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","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":16110,"web_url":"https://patchwork.libcamera.org/comment/16110/","msgid":"","date":"2021-04-03T10:49:51","subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> I'd write the subject line as \"android: mm: cros: Handle buffer\n> registration failure\" to shorten it.\n>\n> On Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:\n> > cros::CameraBufferManager::Register() fails if a buffer handle\n> > is invalid. We should mark CameraBuffer as invalid on the failure\n> > of Register().\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----\n> > 1 file changed, 8 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index 7df4f47c..5f98ee44 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > {\n> > bufferManager_ = cros::CameraBufferManager::GetInstance();\n> >\n> > - bufferManager_->Register(camera3Buffer);\n> > + int ret = bufferManager_->Register(camera3Buffer);\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"Failed registering a buffer: \" << ret;\n>\n> s/a buffer/buffer/\n>\n> This looks good,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> I'll apply the patch if you're OK with the proposed changes.\n>\n\nI'm fine indeed.\n\n> One additional question though. The CameraBuffer::Private destructors\n> calls\n>\n> bufferManager_->Unlock(handle_);\n> bufferManager_->Deregister(handle_);\n>\n> unconditionally. Is that safe to do if the buffer hasn't been registered\n> and locked properly ?\n>\n\nI looked the implementation of CameraBufferManagerImpl. [1]\nLooks like both functions return -EINVAL if Register() and\nLock/LockYCbCr() are not successful.\nWell, it should be safer to check here, but we add the if-condition check here.\nShall I upload the new patch with them?\n\n[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc\n\nBest Regards,\n-Hiro\n> > + return;\n> > + }\n> >\n> > numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> > switch (numPlanes_) {\n> > case 1: {\n> > - int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > if (ret) {\n> > LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > return;\n> > @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > }\n> > case 2:\n> > case 3: {\n> > - int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > - &mem.ycbcr);\n> > + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > + &mem.ycbcr);\n> > if (ret) {\n> > LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > return;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 CD75FC0DA3\n\tfor ;\n\tSat, 3 Apr 2021 10:50:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 352746877E;\n\tSat, 3 Apr 2021 12:50:05 +0200 (CEST)","from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com\n\t[IPv6:2607:f8b0:4864:20::b32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1D0E602CF\n\tfor ;\n\tSat, 3 Apr 2021 12:50:03 +0200 (CEST)","by mail-yb1-xb32.google.com with SMTP id g38so7478011ybi.12\n\tfor ;\n\tSat, 03 Apr 2021 03:50:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"IETPjFsm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=396tZw1PSBvGJf2cGqDdtsi2m0v6Hy+v6JcrhAlKU5U=;\n\tb=IETPjFsmarSiU6eGgeR5+TTmfIQIydvxaNjqCA5l/rEir5gDOURMrcIK9kcZh8sjIC\n\tKA5bPfK7Wegl8MhBRJh9eMxxRkyeQxy4yZUN7n2ia+4pQkmXv+cRih8bGfXILgsVFBWS\n\tyx0DXdkPKQtkCojKgWL2IQewDyTLYAWA5hJAY=","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=396tZw1PSBvGJf2cGqDdtsi2m0v6Hy+v6JcrhAlKU5U=;\n\tb=nMt6j8ZaxZK1MlSLQklXyxnBMqqhTrmSSLxSkVWHmADe3BxP05Zy/e4SWkP7W4w8Qv\n\ttDLZYx6feFEIyeWrflgz6mikzsLPD7qoY3Q9YO9TzXS/Ft4AdGZ5KHzhDb0JUUW/ThNM\n\t+3HqKiX64Hvqts7ufHLSG145bRFfbKa763rbCB3BCTmXv5wW/ZSLUemkayWVetCMeRAy\n\tdHyL1K/q7b5+RjXPIpGV2BgnfNcN5nTGh8vWy3gQFKqKjYFAvyJCmkTOefIGVOhlCVaO\n\t05cM+uRvtdxqZX4sT2Mq7wKjakcdQ1R6xU9aCCvH7XInxWCRCvU1Z0WXFH3lZAOgTJmJ\n\tBL3w==","X-Gm-Message-State":"AOAM53234nFLI24KlhRkev2BtXy4I5K0eHWVoo1+NuXbo7DKL4y33lVh\n\tBS8nUC0Xiezj4cXlh4JP/Gwg7DqUsiGAURS+IjbX1aP0hhYPzA==","X-Google-Smtp-Source":"ABdhPJxEdt8s4jS4u2dQ+k2wL6Jc9gdVyVjSa9BrR9imoSVeCgKYF2Kn4OxFAiC3Vx4A19iBFI8SyrYno4fEpthkC9s=","X-Received":"by 2002:a25:850a:: with SMTP id\n\tw10mr23537358ybk.402.1617447002752; \n\tSat, 03 Apr 2021 03:50:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210402040340.1364749-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Sat, 3 Apr 2021 19:49:51 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16114,"web_url":"https://patchwork.libcamera.org/comment/16114/","msgid":"","date":"2021-04-03T16:03:38","subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Sat, Apr 03, 2021 at 07:49:51PM +0900, Hirokazu Honda wrote:\n> On Sat, Apr 3, 2021 at 9:06 AM Laurent Pinchart wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > I'd write the subject line as \"android: mm: cros: Handle buffer\n> > registration failure\" to shorten it.\n> >\n> > On Fri, Apr 02, 2021 at 01:03:40PM +0900, Hirokazu Honda wrote:\n> > > cros::CameraBufferManager::Register() fails if a buffer handle\n> > > is invalid. We should mark CameraBuffer as invalid on the failure\n> > > of Register().\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/mm/cros_camera_buffer.cpp | 12 ++++++++----\n> > > 1 file changed, 8 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index 7df4f47c..5f98ee44 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -53,12 +53,16 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > > {\n> > > bufferManager_ = cros::CameraBufferManager::GetInstance();\n> > >\n> > > - bufferManager_->Register(camera3Buffer);\n> > > + int ret = bufferManager_->Register(camera3Buffer);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"Failed registering a buffer: \" << ret;\n> >\n> > s/a buffer/buffer/\n> >\n> > This looks good,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > I'll apply the patch if you're OK with the proposed changes.\n> \n> I'm fine indeed.\n> \n> > One additional question though. The CameraBuffer::Private destructors\n> > calls\n> >\n> > bufferManager_->Unlock(handle_);\n> > bufferManager_->Deregister(handle_);\n> >\n> > unconditionally. Is that safe to do if the buffer hasn't been registered\n> > and locked properly ?\n> \n> I looked the implementation of CameraBufferManagerImpl. [1]\n> Looks like both functions return -EINVAL if Register() and\n> Lock/LockYCbCr() are not successful.\n> Well, it should be safer to check here, but we add the if-condition check here.\n> Shall I upload the new patch with them?\n\nBoth will log an error in those cases, so it's best to avoid that. Also,\nthe API documentation doesn't explicitly state that calling those\nfunctions on buffers that haven't been registered and/or locked is safe,\nso even if it is today, the implementation may change later. It's best\nto be defensive (or get the CameraBufferManager API to explicitly state\nthat this is fine, and drop the error messages).\n\n> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_manager_impl.cc\n>\n> > > + return;\n> > > + }\n> > >\n> > > numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> > > switch (numPlanes_) {\n> > > case 1: {\n> > > - int ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > > return;\n> > > @@ -67,8 +71,8 @@ CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > > }\n> > > case 2:\n> > > case 3: {\n> > > - int ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > - &mem.ycbcr);\n> > > + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > + &mem.ycbcr);\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > > return;","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 0DB2CC0DA3\n\tfor ;\n\tSat, 3 Apr 2021 16:04:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 625FB602E6;\n\tSat, 3 Apr 2021 18:04:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4362602CF\n\tfor ;\n\tSat, 3 Apr 2021 18:04:22 +0200 (CEST)","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 413A6AF0;\n\tSat, 3 Apr 2021 18:04:22 +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=\"bDe0ZEGN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617465862;\n\tbh=n+bMiY6ceLp0TdkfSdJCLRqaGFA0TxrjwTV5ZRPHdjA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bDe0ZEGNez2N74ZQbU+1oxfn+dtrJDgD3ITEUfTFBk5JJLPYeuyRYKMLSea5Qu0VS\n\t4p8u8xV8+EPAE8wRyvDvcVCrflk8w9R9B9fTybfRPTLqy8mZQtFyoqjz1ZwT4Lzw8s\n\ttvMJ37ObVJiY6SB9xKbGHl3sG4bupwSXssuigebQ=","Date":"Sat, 3 Apr 2021 19:03:38 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210402040340.1364749-1-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] android: CameraBuffer: Mark as\n\tinvalid if cros::CameraBufferManager::Register() fails","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]