[{"id":12170,"web_url":"https://patchwork.libcamera.org/comment/12170/","msgid":"<1bdf6ce2-11e0-73aa-72aa-916f498ad132@ideasonboard.com>","date":"2020-08-27T11:02:19","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nI've got my eye on this class, and thinking we should try to move it to\ncore libcamera too, as I already want to be able to allocate\nintermediate buffers for the Android HAL.\n\nBut that's separate, so improving the usage in this series seems like a\ngood thing to do:\n\n\nOn 27/08/2020 09:20, Jacopo Mondi wrote:\n> The dma_heaps device is opened in the class constructor, making\n> it impossible (or rather ugly) to catch errors before the allocator gets\n> actually used.\n> \n> Provide an open() and a close() method to allow users to catch errors\n> earlier.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> ---\n>  .../pipeline/raspberrypi/dma_heaps.cpp        | 22 ++++++++++++++++---\n>  .../pipeline/raspberrypi/dma_heaps.h          |  2 ++\n>  2 files changed, 21 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> index 6769c04640f1..739f05d3d4d8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI)\n>  namespace RPi {\n>  \n>  DmaHeap::DmaHeap()\n> +\t: dmaHeapHandle_(-1)\n> +{\n> +}\n> +\n> +DmaHeap::~DmaHeap()\n> +{\n> +\tclose();\n> +}\n> +\n> +int DmaHeap::open()\n>  {\n>  \tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n>  \tif (dmaHeapHandle_ == -1) {\n>  \t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n>  \t\tif (dmaHeapHandle_ == -1) {\n>  \t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> +\t\t\treturn dmaHeapHandle_;\n>  \t\t}\n>  \t}\n> +\n> +\treturn 0;\n>  }\n>  \n> -DmaHeap::~DmaHeap()\n> +void DmaHeap::close()\n>  {\n> -\tif (dmaHeapHandle_)\n> -\t\t::close(dmaHeapHandle_);\n> +\tif (dmaHeapHandle_ < 0)\n> +\t\treturn;\n> +\n> +\t::close(dmaHeapHandle_);\n> +\tdmaHeapHandle_ = -1;\n>  }\n>  \n>  FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> index ae6be1135f17..3e17993097ef 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> @@ -18,6 +18,8 @@ class DmaHeap\n>  public:\n>  \tDmaHeap();\n>  \t~DmaHeap();\n> +\tint open();\n> +\tvoid close();\n>  \tFileDescriptor alloc(const char *name, std::size_t size);\n>  \n>  private:\n>","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 D5DCEBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 27 Aug 2020 11:02:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6682362901;\n\tThu, 27 Aug 2020 13:02:25 +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 4D34F628D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 27 Aug 2020 13:02:24 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 687779B1;\n\tThu, 27 Aug 2020 13:02: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=\"UVaYIORO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598526143;\n\tbh=AkxzIhHiYf6CEgvjq6K8Iyn3Nvq2eJ3ewjk3Rz4Qphs=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=UVaYIORO/ZvC/hyWMjjJOzAggH/59Uto3IGimG/67cAPrU+EEn++1GHbohSwLNvtP\n\t/I12ZysqlYEHO8KqkWeSXIqi3k2r1Uo3hb4o/J8l5Rf+7f1DZ79lfClQCuIUMDNiBj\n\t8rLuiC8CICWVQt8N5td0/dn+KjsjeZHl//S7bNFA=","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org,\n\tDave Stevenson <dave.stevenson@raspberrypi.com>,\n\tNaushir Patuck <naush@raspberrypi.com>","References":"<20200827082038.40758-1-jacopo@jmondi.org>\n\t<20200827082038.40758-2-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<1bdf6ce2-11e0-73aa-72aa-916f498ad132@ideasonboard.com>","Date":"Thu, 27 Aug 2020 12:02:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200827082038.40758-2-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","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>","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12183,"web_url":"https://patchwork.libcamera.org/comment/12183/","msgid":"<20200827141109.GG6042@pendragon.ideasonboard.com>","date":"2020-08-27T14:11:09","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Aug 27, 2020 at 10:20:36AM +0200, Jacopo Mondi wrote:\n> The dma_heaps device is opened in the class constructor, making\n> it impossible (or rather ugly) to catch errors before the allocator gets\n> actually used.\n> \n> Provide an open() and a close() method to allow users to catch errors\n> earlier.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  .../pipeline/raspberrypi/dma_heaps.cpp        | 22 ++++++++++++++++---\n>  .../pipeline/raspberrypi/dma_heaps.h          |  2 ++\n>  2 files changed, 21 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> index 6769c04640f1..739f05d3d4d8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI)\n>  namespace RPi {\n>  \n>  DmaHeap::DmaHeap()\n> +\t: dmaHeapHandle_(-1)\n> +{\n> +}\n> +\n> +DmaHeap::~DmaHeap()\n> +{\n> +\tclose();\n> +}\n> +\n> +int DmaHeap::open()\n>  {\n>  \tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n>  \tif (dmaHeapHandle_ == -1) {\n>  \t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n>  \t\tif (dmaHeapHandle_ == -1) {\n>  \t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> +\t\t\treturn dmaHeapHandle_;\n>  \t\t}\n>  \t}\n> +\n> +\treturn 0;\n>  }\n\nDoesn't this break bisection ?\n\nPlease also note that you could alternatively add a bool isValid() const\n(or similar) function to query whether the open succeeded. I think that\nwould be less intrusive in this case. Otherwise you should guard about\ndouble-open for instance.\n\n>  \n> -DmaHeap::~DmaHeap()\n> +void DmaHeap::close()\n>  {\n> -\tif (dmaHeapHandle_)\n> -\t\t::close(dmaHeapHandle_);\n> +\tif (dmaHeapHandle_ < 0)\n> +\t\treturn;\n> +\n> +\t::close(dmaHeapHandle_);\n> +\tdmaHeapHandle_ = -1;\n>  }\n>  \n>  FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> index ae6be1135f17..3e17993097ef 100644\n> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> @@ -18,6 +18,8 @@ class DmaHeap\n>  public:\n>  \tDmaHeap();\n>  \t~DmaHeap();\n> +\tint open();\n> +\tvoid close();\n>  \tFileDescriptor alloc(const char *name, std::size_t size);\n>  \n>  private:","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 B6561BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 27 Aug 2020 14:11:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3010D62901;\n\tThu, 27 Aug 2020 16:11:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3448B628D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 27 Aug 2020 16:11:31 +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 34F941872;\n\tThu, 27 Aug 2020 16:11:29 +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=\"NFZlZXTe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598537489;\n\tbh=6fUCABgvrkjT+DfRzGKJ9UoUhSwb67Nav9pp+DexX3Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NFZlZXTejLCVEPeuoJTQC85FgrTth7eZsXSXC8OddWQi1B+nCJ7ynaSLuW32BjgIe\n\t0r+F/UeDLWIkTeTBDhoBmvdFVQF8X4Ke7+r7dNImo7RzeLLU5ndYYdfDtNNERyWe1N\n\tvz5Mmprlh+GM8rGo3S8Oy087QU9gtQUyUURnXFLM=","Date":"Thu, 27 Aug 2020 17:11:09 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200827141109.GG6042@pendragon.ideasonboard.com>","References":"<20200827082038.40758-1-jacopo@jmondi.org>\n\t<20200827082038.40758-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200827082038.40758-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12184,"web_url":"https://patchwork.libcamera.org/comment/12184/","msgid":"<20200827141620.GH6042@pendragon.ideasonboard.com>","date":"2020-08-27T14:16:20","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Aug 27, 2020 at 12:02:19PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n> \n> I've got my eye on this class, and thinking we should try to move it to\n> core libcamera too, as I already want to be able to allocate\n> intermediate buffers for the Android HAL.\n\nFully agreed, I think this will graduate to a core helper once we need\nit in other places.\n\nWhat kind of intermediate buffers do you need to allocate for the HAL\nthough ? Buffers for YUV frames when all that the camera service\nrequests is JPEG ? That's something we probably want to handle with a\nFramebufferAllocator. Down the line FramebufferAllocator should gain\ndmabuf heaps support, but that will require more work (in particular\ngiven that the dmabuf heaps API is still fairly limited in mainline,\nwithout support to handle device-specific constraints).\n\n> But that's separate, so improving the usage in this series seems like a\n> good thing to do:\n> \n> On 27/08/2020 09:20, Jacopo Mondi wrote:\n> > The dma_heaps device is opened in the class constructor, making\n> > it impossible (or rather ugly) to catch errors before the allocator gets\n> > actually used.\n> > \n> > Provide an open() and a close() method to allow users to catch errors\n> > earlier.\n> > \n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> > ---\n> >  .../pipeline/raspberrypi/dma_heaps.cpp        | 22 ++++++++++++++++---\n> >  .../pipeline/raspberrypi/dma_heaps.h          |  2 ++\n> >  2 files changed, 21 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > index 6769c04640f1..739f05d3d4d8 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp\n> > @@ -32,20 +32,36 @@ LOG_DECLARE_CATEGORY(RPI)\n> >  namespace RPi {\n> >  \n> >  DmaHeap::DmaHeap()\n> > +\t: dmaHeapHandle_(-1)\n> > +{\n> > +}\n> > +\n> > +DmaHeap::~DmaHeap()\n> > +{\n> > +\tclose();\n> > +}\n> > +\n> > +int DmaHeap::open()\n> >  {\n> >  \tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_NAME, O_RDWR, 0);\n> >  \tif (dmaHeapHandle_ == -1) {\n> >  \t\tdmaHeapHandle_ = ::open(DMA_HEAP_CMA_ALT_NAME, O_RDWR, 0);\n> >  \t\tif (dmaHeapHandle_ == -1) {\n> >  \t\t\tLOG(RPI, Error) << \"Could not open dmaHeap device\";\n> > +\t\t\treturn dmaHeapHandle_;\n> >  \t\t}\n> >  \t}\n> > +\n> > +\treturn 0;\n> >  }\n> >  \n> > -DmaHeap::~DmaHeap()\n> > +void DmaHeap::close()\n> >  {\n> > -\tif (dmaHeapHandle_)\n> > -\t\t::close(dmaHeapHandle_);\n> > +\tif (dmaHeapHandle_ < 0)\n> > +\t\treturn;\n> > +\n> > +\t::close(dmaHeapHandle_);\n> > +\tdmaHeapHandle_ = -1;\n> >  }\n> >  \n> >  FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > index ae6be1135f17..3e17993097ef 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h\n> > @@ -18,6 +18,8 @@ class DmaHeap\n> >  public:\n> >  \tDmaHeap();\n> >  \t~DmaHeap();\n> > +\tint open();\n> > +\tvoid close();\n> >  \tFileDescriptor alloc(const char *name, std::size_t size);\n> >  \n> >  private:","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 B08FCBF019\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 27 Aug 2020 14:16:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D27062901;\n\tThu, 27 Aug 2020 16:16: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 92D9B628D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 27 Aug 2020 16:16:40 +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 F3EA01872;\n\tThu, 27 Aug 2020 16:16:39 +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=\"tjF5hJpo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598537800;\n\tbh=Oi05lbXG3NF5AxXP05w5cwN1ImJEhPcuEOvzUdvW8A8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tjF5hJpofpBzeVZXMUuVPRzoGT9JbyTmn1mSB6qTn5Ul7pcRk+zhkMWgbWSL9Fkom\n\tYRWS8g0Q1MMCPvl1nYdCF+1/GrgaK/Cw40sLMh3yt8fbRAy3aNB3gHBMBNySKjqfTp\n\tyYvo1nSv71hZM2RwthsvHXxgq6wn+/QrwvOoq/gg=","Date":"Thu, 27 Aug 2020 17:16:20 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20200827141620.GH6042@pendragon.ideasonboard.com>","References":"<20200827082038.40758-1-jacopo@jmondi.org>\n\t<20200827082038.40758-2-jacopo@jmondi.org>\n\t<1bdf6ce2-11e0-73aa-72aa-916f498ad132@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<1bdf6ce2-11e0-73aa-72aa-916f498ad132@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: raspberrypi:\n\tdma_heaps: Add open/close","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]