<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:48 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:42PM +0900, Hirokazu Honda wrote:<br>
> DmaHeaps owns a file descriptor for a dma handle. It should be<br>
> managed by ScopedFD to avoid the leakage. Furthermore,<br>
> DmaHeaps::alloc() creates a new file descriptor and the returned<br>
> file descriptor is owned by a caller. This also clarifies it by<br>
> changing the returned value to ScopedFD.<br>
> <br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> .../pipeline/raspberrypi/dma_heaps.cpp | 23 ++++++++-----------<br>
> .../pipeline/raspberrypi/dma_heaps.h | 10 ++++----<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 7 ++++--<br>
> 3 files changed, 21 insertions(+), 19 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp<br>
> index 4d5dd6cb..d5b000b8 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp<br>
> @@ -35,7 +35,6 @@ LOG_DECLARE_CATEGORY(RPI)<br>
> namespace RPi {<br>
> <br>
> DmaHeap::DmaHeap()<br>
> - : dmaHeapHandle_(-1)<br>
> {<br>
> for (const char *name : heapNames) {<br>
> int ret = ::open(name, O_RDWR, 0);<br>
> @@ -46,49 +45,47 @@ DmaHeap::DmaHeap()<br>
> continue;<br>
> }<br>
> <br>
> - dmaHeapHandle_ = ret;<br>
> + dmaHeapHandle_ = ScopedFD(ret);<br>
> break;<br>
> }<br>
> <br>
> - if (dmaHeapHandle_ < 0)<br>
> + if (!dmaHeapHandle_.isValid())<br>
> LOG(RPI, Error) << "Could not open any dmaHeap device";<br>
> }<br>
> <br>
> DmaHeap::~DmaHeap()<br>
> {<br>
> - if (dmaHeapHandle_ > -1)<br>
> - ::close(dmaHeapHandle_);<br>
> }<br>
> <br>
> -FileDescriptor DmaHeap::alloc(const char *name, std::size_t size)<br>
> +ScopedFD DmaHeap::alloc(const char *name, std::size_t size)<br>
> {<br>
> int ret;<br>
> <br>
> if (!name)<br>
> - return FileDescriptor();<br>
> + return ScopedFD();<br>
> <br>
> struct dma_heap_allocation_data alloc = {};<br>
> <br>
> alloc.len = size;<br>
> alloc.fd_flags = O_CLOEXEC | O_RDWR;<br>
> <br>
> - ret = ::ioctl(dmaHeapHandle_, DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> + ret = ::ioctl(dmaHeapHandle_.get(), DMA_HEAP_IOCTL_ALLOC, &alloc);<br>
> <br>
> if (ret < 0) {<br>
> LOG(RPI, Error) << "dmaHeap allocation failure for "<br>
> << name;<br>
> - return FileDescriptor();<br>
> + return ScopedFD();<br>
> }<br>
> <br>
> - ret = ::ioctl(alloc.fd, DMA_BUF_SET_NAME, name);<br>
> + ScopedFD allocFd(alloc.fd);<br>
> + ret = ::ioctl(allocFd.get(), DMA_BUF_SET_NAME, name);<br>
> if (ret < 0) {<br>
> LOG(RPI, Error) << "dmaHeap naming failure for "<br>
> << name;<br>
> - ::close(alloc.fd);<br>
> - return FileDescriptor();<br>
> + return ScopedFD();<br>
> }<br>
> <br>
> - return FileDescriptor(std::move(alloc.fd));<br>
> + return allocFd;<br>
> }<br>
> <br>
> } /* namespace RPi */<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
> index 79f39c51..c49772df 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
> +++ b/src/libcamera/pipeline/raspberrypi/dma_heaps.h<br>
> @@ -7,7 +7,9 @@<br>
> #ifndef __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__<br>
> #define __LIBCAMERA_PIPELINE_RASPBERRYPI_DMA_HEAPS_H__<br>
> <br>
> -#include <libcamera/file_descriptor.h><br>
> +#include <cstddef><br>
> +<br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> <br>
> namespace libcamera {<br>
> <br>
> @@ -18,11 +20,11 @@ class DmaHeap<br>
> public:<br>
> DmaHeap();<br>
> ~DmaHeap();<br>
> - bool isValid() const { return dmaHeapHandle_ > -1; }<br>
> - FileDescriptor alloc(const char *name, std::size_t size);<br>
> + bool isValid() const { return dmaHeapHandle_.isValid(); }<br>
> + ScopedFD alloc(const char *name, std::size_t size);<br>
> <br>
> private:<br>
> - int dmaHeapHandle_;<br>
> + ScopedFD dmaHeapHandle_;<br>
> };<br>
> <br>
> } /* namespace RPi */<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index f22e286e..0075fdb1 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1256,10 +1256,13 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> <br>
> /* Allocate the lens shading table via dmaHeap and pass to the IPA. */<br>
> if (!lsTable_.isValid()) {<br>
> - lsTable_ = dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);<br>
> - if (!lsTable_.isValid())<br>
> + ScopedFD scopedFD =<br>
> + dmaHeap_.alloc("ls_grid", ipa::RPi::MaxLsGridSize);<br>
> + if (!scopedFD.isValid())<br>
> return -ENOMEM;<br>
> <br>
> + lsTable_ = FileDescriptor(scopedFD.release());<br>
<br>
This isn't very nice. One option could be to add a FileDescriptor<br>
constructor that takes a ScopedFD. What do you think ?<br>
<br></blockquote><div><br></div><div>That sounds good to me.</div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> /* Allow the IPA to mmap the LS table via the file descriptor. */<br>
> /*<br>
> * \todo Investigate if mapping the lens shading table buffer<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>