<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 31, 2024 at 8:39 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@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 Harvey<br>
<br>
On Tue, Aug 27, 2024 at 05:34:43PM GMT, Cheng-Hao Yang wrote:<br>
> Thanks Jacopo for the review.<br>
><br>
> The corresponding updates are included in v9.1. Please check.<br>
><br>
> On Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Harvey<br>
> ><br>
> > On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:<br>
> > > Add a helper function exportBuffers in DmaBufAllocator to make it easier<br>
> > > to use.<br>
> > ><br>
> > > It'll be used in Virtual Pipeline Handler and SoftwareIsp.<br>
> > ><br>
> > > Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > > ---<br>
> > > .../libcamera/internal/dma_buf_allocator.h | 12 ++++<br>
> > > src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++-<br>
> > > 2 files changed, 74 insertions(+), 2 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/internal/dma_buf_allocator.h<br>
> > b/include/libcamera/internal/dma_buf_allocator.h<br>
> > > index 36ec1696b..3a9b56b1c 100644<br>
> > > --- a/include/libcamera/internal/dma_buf_allocator.h<br>
> > > +++ b/include/libcamera/internal/dma_buf_allocator.h<br>
> > > @@ -8,12 +8,16 @@<br>
> > > #pragma once<br>
> > ><br>
> > > #include <stddef.h><br>
> > > +#include <vector><br>
> > ><br>
> > > #include <libcamera/base/flags.h><br>
> > > #include <libcamera/base/unique_fd.h><br>
> > ><br>
> > > namespace libcamera {<br>
> > ><br>
> > > +class FrameBuffer;<br>
> > > +struct StreamConfiguration;<br>
> ><br>
> > Where do you use this ?<br>
> ><br>
> ><br>
> Right, it's only used in the previous versions. Removed.<br>
><br>
><br>
> > > +<br>
> > > class DmaBufAllocator<br>
> > > {<br>
> > > public:<br>
> > > @@ -30,7 +34,15 @@ public:<br>
> > > bool isValid() const { return providerHandle_.isValid(); }<br>
> > > UniqueFD alloc(const char *name, std::size_t size);<br>
> > ><br>
> > > + int exportBuffers(<br>
> ><br>
> > weird indent<br>
> ><br>
> > int exportBuffers(std::size_t count,<br>
> > const std::vector<std::size_t> &frameSize,<br>
> > std::vector<std::unique_ptr<FrameBuffer>><br>
> > *buffers);<br>
> ><br>
> > Done<br>
<br>
Sometimes you replies are indendented at the same level as the<br>
previous email you replied to, making it really hard to distinguish<br>
the two. Could you check why it happens ?<br>
<br></blockquote><div><br></div><div>Ah, thanks for letting me know. I normally wouldn't leave an empty</div><div>line before my replies, and if the reply line above is an empty line,</div><div>this issue happens.</div><div> </div><div>I'll leave one extra line above every reply from now on.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
><br>
> > > + std::size_t count,<br>
> ><br>
> > #include <cstddef><br>
> ><br>
> Removed std::size_t.<br>
><br>
><br>
> ><br>
> > > + std::vector<std::size_t> frameSize,<br>
> > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
> ><br>
> > #include <memory><br>
> ><br>
> > Done<br>
><br>
><br>
> > > +<br>
> > > private:<br>
> > > + std::unique_ptr<FrameBuffer> createBuffer(<br>
> > > + std::string name, std::vector<std::size_t> frameSizes);<br>
> ><br>
> > #include <string><br>
> ><br>
> Done<br>
><br>
><br>
> > > +<br>
> > > UniqueFD allocFromHeap(const char *name, std::size_t size);<br>
> > > UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);<br>
> > > UniqueFD providerHandle_;<br>
> > > diff --git a/src/libcamera/dma_buf_allocator.cpp<br>
> > b/src/libcamera/dma_buf_allocator.cpp<br>
> > > index c06eca7d0..2d88b8b2b 100644<br>
> > > --- a/src/libcamera/dma_buf_allocator.cpp<br>
> > > +++ b/src/libcamera/dma_buf_allocator.cpp<br>
> > > @@ -23,6 +23,11 @@<br>
> > ><br>
> > > #include <libcamera/base/log.h><br>
> > ><br>
> > > +#include <libcamera/framebuffer.h><br>
> > > +#include <libcamera/stream.h><br>
> ><br>
> > Not used<br>
> ><br>
> Removed<br>
><br>
><br>
> ><br>
> > > +<br>
> > > +#include "libcamera/internal/formats.h"<br>
> > > +<br>
> > > /**<br>
> > > * \file dma_buf_allocator.cpp<br>
> > > * \brief dma-buf allocator<br>
> > > @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;<br>
> > > /* uClibc doesn't provide the file sealing API. */<br>
> > > #ifndef __DOXYGEN__<br>
> > > #if not HAVE_FILE_SEALS<br>
> > > -#define F_ADD_SEALS 1033<br>
> > > -#define F_SEAL_SHRINK 0x0002<br>
> > > +#define F_ADD_SEALS 1033<br>
> > > +#define F_SEAL_SHRINK 0x0002<br>
> ><br>
> > Unrelated<br>
> ><br>
> > Removed the changes<br>
><br>
><br>
> > > #endif<br>
> > > #endif<br>
> > ><br>
> > > @@ -243,4 +248,59 @@ UniqueFD DmaBufAllocator::alloc(const char *name,<br>
> > std::size_t size)<br>
> > > return allocFromHeap(name, size);<br>
> > > }<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Allocate and export buffers for \a stream from the<br>
> > DmaBufAllocator<br>
> ><br>
> > There's no \a stream<br>
> ><br>
> Removed.<br>
><br>
><br>
> ><br>
> > > + * \param[in] count The number of FrameBuffers required<br>
> ><br>
> > "of requested FrameBuffers" ?<br>
> ><br>
> Thanks! Adopted.<br>
><br>
><br>
> ><br>
> > > + * \param[in] frameSizes The sizes of planes in the FrameBuffer<br>
> > > + * \param[out] buffers Array of buffers successfully allocated<br>
> > > + *<br>
> > > + * \return The number of allocated buffers on success or a negative<br>
> > error code<br>
> > > + * otherwise<br>
> > > + */<br>
> > > +int DmaBufAllocator::exportBuffers(<br>
> > > + std::size_t count,<br>
> ><br>
> > nit: This can be an unsigned int maybe<br>
> ><br>
> Done<br>
><br>
><br>
> ><br>
> > > + std::vector<std::size_t> frameSizes,<br>
> ><br>
> > can this be passed as a const reference ?<br>
> ><br>
> Done<br>
><br>
><br>
> ><br>
> > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> ><br>
> > weird indentation, I think<br>
> ><br>
> > int DmaBufAllocator::exportBuffers(std::size_t count,<br>
> > const std::vector<std::size_t><br>
> > &frameSizes,<br>
> ><br>
> > std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> ><br>
> > would do (some heretics consider 120 cols to be ok for libcamera, and<br>
> > nowadays it is accepted for linux as well !! )<br>
> ><br>
> Thanks! Adopted.<br>
><br>
><br>
> ><br>
> > > +{<br>
> > > + for (unsigned i = 0; i < count; ++i) {<br>
> ><br>
> > I wonder if we should be stricter when defining this interface: is<br>
> > there a maximum number of buffers that can be allocated ? How many<br>
> > planes can a buffer have ? (I don't see this addressed in our<br>
> > FrameBuffer class, so it can be left out from here too ?)<br>
> ><br>
><br>
> Hmm, as it's just a helper function, I think the users should know what<br>
> they're doing, unless we have more specific rules, like the maximum<br>
> number of planes in FrameBuffer.<br>
<br>
Never assume how an API could be wrongly used :)<br>
<br>
><br>
> We can't stop users allocating too many buffers with or without this<br>
> helper function, right :p ?<br>
><br>
<br>
True indeed, but a good API should minimize the surface for making it<br>
used wrongly, even more when it gives access to system resource.<br>
<br>
See below.<br>
<br>
><br>
> ><br>
> > > + std::unique_ptr<FrameBuffer> buffer =<br>
> > > + createBuffer("frame-" + std::to_string(i),<br>
> > frameSizes);<br>
> > > + if (!buffer) {<br>
> > > + LOG(DmaBufAllocator, Error) << "Unable to create<br>
> > buffer";<br>
> > > +<br>
> > > + buffers->clear();<br>
> > > + return -EINVAL;<br>
> > > + }<br>
> > > +<br>
> > > + buffers->push_back(std::move(buffer));<br>
> > > + }<br>
> > > +<br>
> > > + return count;<br>
> ><br>
> > if you want to have all the requested buffers to be allocated, and do<br>
> > not allow less than that, there's no point in returning count here, you<br>
> > can return 0<br>
> ><br>
><br>
> Hmm, you're right, while I wonder if it's the same case for<br>
> V4L2VideoDevice::exportBuffers/createBuffers? I want to align with<br>
> other exportBuffers API.<br>
><br>
<br>
I think it's fine<br>
<br>
><br>
> ><br>
> > > +}<br>
> > > +<br>
> > > +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(<br>
> > > + std::string name, std::vector<std::size_t> frameSizes)<br>
> ><br>
> > Or<br>
> ><br>
> > std::unique_ptr<FrameBuffer><br>
> > DmaBufAllocator::createBuffer(std::string name,<br>
> > const std::vector<std::size_t> &frameSizes)<br>
> ><br>
> > can frameSize be passed as a const reference ?<br>
> ><br>
> Thanks! Adopted.<br>
><br>
><br>
> ><br>
> ><br>
> > > +{<br>
> > > + std::vector<FrameBuffer::Plane> planes;<br>
> > > +<br>
> > > + std::size_t bufferSize = 0, offset = 0;<br>
> > > + for (auto frameSize : frameSizes)<br>
> > > + bufferSize += frameSize;<br>
> ><br>
> > This API allows unbounded size allocation, just sayin', maybe it's<br>
> > fine<br>
> ><br>
><br>
> Yeah... as it's just a helper function, I still think that the users should<br>
> know what they're doing.<br>
><br>
<br>
As long as they don't :)<br>
<br>
I'm particularly concerned about udmabuf, as it uses memfd as backing<br>
storage, and we allow to allocate a lof of memory (we did already to<br>
be completely honest, as a user can call alloc with any size).<br>
<br>
Also, I've now noticed the dma_buf_allocator API are not public, so<br>
this makes me way less concerned.<br></blockquote><div><br></div><div>Yes, pipeline handlers' developers should be more careful than the</div><div>applications'.</div><div><br></div><div>So let's say we keep the API as is for now, until there are more guidelines</div><div>on FrameBuffer's limitation?</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>
><br>
> ><br>
> > > +<br>
> > > + SharedFD fd(alloc(name.c_str(), bufferSize));<br>
> > > + if (!fd.isValid())<br>
> > > + return nullptr;<br>
> > > +<br>
> > > + for (auto frameSize : frameSizes) {<br>
> > > + FrameBuffer::Plane plane;<br>
> > > + plane.fd = fd;<br>
> > > + plane.offset = offset;<br>
> > > + plane.length = frameSize;<br>
> > > + planes.push_back(std::move(plane));<br>
> > > + offset += plane.length;<br>
> > > + }<br>
> ><br>
> > With a little implicit type casting, you can<br>
> ><br>
> > unsigned int offset = 0;<br>
> > for (unsigned int frameSize : frameSizes) {<br>
> > planes.emplace_back(FrameBuffer::Plane{fd, offset,<br>
> > frameSize});<br>
> > offset += frameSize;<br>
> > }<br>
> ><br>
> > But it won't save you going a temporary object I'm afraid. I'm not<br>
> > sure it's actually any better, up to you.<br>
> ><br>
> > Thanks, adopted. Also, as the offset's and length's types are both<br>
> unsigned int, I changed `frameSize` to be an array of unsigned as<br>
> well.<br>
><br>
><br>
> > > +<br>
> > > + return std::make_unique<FrameBuffer>(planes);<br>
> > > +}<br>
> > > +<br>
> > > } /* namespace libcamera */<br>
> > > --<br>
> > > 2.46.0.184.g6999bdac58-goog<br>
> > ><br>
> ><br>
</blockquote></div></div>