[libcamera-devel] [RFC PATCH v1 06/12] libcamera: framebuffer: Add a function to check if planes are contiguous

[Hirokazu Honda]

Hi Laurent,

On Fri, Sep 3, 2021 at 8:23 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Sep 03, 2021 at 08:09:07PM +0900, Hirokazu Honda wrote:
> > On Fri, Sep 3, 2021 at 5:40 PM Laurent Pinchart wrote:
> > > On Fri, Sep 03, 2021 at 09:24:02AM +0100, Kieran Bingham wrote:
> > > > On 02/09/2021 19:07, Laurent Pinchart wrote:
> > > > > On Thu, Sep 02, 2021 at 10:43:56AM +0100, Kieran Bingham wrote:
> > > > >> On 02/09/2021 05:22, Laurent Pinchart wrote:
> > > > >>> Multi-planar frame buffers can store their planes contiguously in
> > > > >>> memory, or split them in discontiguous memory areas. Add a private
> > > > >>> function to check in which of these two categories the frame buffer
> > > > >>> belongs. This will be used to correctly handle the differences between
> > > > >>> the V4L2 single and multi planar APIs.
> > > > >>>
> > > > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > >>> ---
> > > > >>>  include/libcamera/internal/framebuffer.h |  2 ++
> > > > >>>  src/libcamera/framebuffer.cpp            | 45 +++++++++++++++++++++++-
> > > > >>>  2 files changed, 46 insertions(+), 1 deletion(-)
> > > > >>>
> > > > >>> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > > > >>> index 606aed2b4782..cd33c295466e 100644
> > > > >>> --- a/include/libcamera/internal/framebuffer.h
> > > > >>> +++ b/include/libcamera/internal/framebuffer.h
> > > > >>> @@ -21,9 +21,11 @@ public:
> > > > >>>   Private();
> > > > >>>
> > > > >>>   void setRequest(Request *request) { request_ = request; }
> > > > >>> + bool isContiguous() const { return isContiguous_; }
> > > > >>>
> > > > >>>  private:
> > > > >>>   Request *request_;
> > > > >>> + bool isContiguous_;
> > > > >>>  };
> > > > >>>
> > > > >>>  } /* namespace libcamera */
> > > > >>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > > >>> index 53ef89bf458f..99265b44da43 100644
> > > > >>> --- a/src/libcamera/framebuffer.cpp
> > > > >>> +++ b/src/libcamera/framebuffer.cpp
> > > > >>> @@ -106,7 +106,7 @@ LOG_DEFINE_CATEGORY(Buffer)
> > > > >>>   */
> > > > >>>
> > > > >>>  FrameBuffer::Private::Private()
> > > > >>> - : request_(nullptr)
> > > > >>> + : request_(nullptr), isContiguous_(true)
> > > > >>>  {
> > > > >>>  }
> > > > >>>
> > > > >>> @@ -120,6 +120,17 @@ FrameBuffer::Private::Private()
> > > > >>>   * handlers, it is called by the pipeline handlers themselves.
> > > > >>>   */
> > > > >>>
> > > > >>> +/**
> > > > >>> + * \fn FrameBuffer::Private::isContiguous()
> > > > >>> + * \brief Check if the frame buffer stores planes contiguously in memory
> > > > >>> + *
> > > > >>> + * Multi-planar frame buffers can store their planes contiguously in memory, or
> > > > >>> + * split them in discontiguous memory areas. This function checks in which of
> > > > >>
> > > > >> 'split them into discontiguous'
> > > > >>
> > > > >>> + * these two categories the frame buffer belongs.
> > > > >>> + *
> > > > >>> + * \return True if the planes are stored contiguously in memory, false otherwise
> > > > >>> + */
> > > > >>> +
> > > > >>>  /**
> > > > >>>   * \class FrameBuffer
> > > > >>>   * \brief Frame buffer data and its associated dynamic metadata
> > > > >>> @@ -199,9 +210,41 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > > > >>>   : Extensible(std::make_unique<Private>()), planes_(planes),
> > > > >>>     cookie_(cookie)
> > > > >>>  {
> > > > >>> + unsigned int offset = 0;
> > > > >>> +
> > > > >>>   /* \todo Remove the assertions after sufficient testing */
> > > > >>>   for (const auto &plane : planes_)
> > > > >>>           ASSERT(plane.offset != Plane::kInvalidOffset);
> >
> > nit:
> > Shall we merge the two for loops?
>
> I've kept them separate to make it easier to remove the first one (and
> now I wonder why I thought it would be easier...), but as the ASSERT is
> now here to stay, I'll merge them.
>
> > At least, I would move offset to before isContiguous.
>
> Done already :-)
>
> > > > >>> +
> > > > >>> + bool isContiguous = true;
> > > > >>> + ino_t inode = 0;
> > > > >>> +
> > > > >>> + for (const auto &plane : planes_) {
> > > > >>> +         if (plane.offset != offset) {
> > > > >>> +                 isContiguous = false;
> > > > >>> +                 break;
> > > > >>> +         }
> > > > >>> +
> > > > >>> +         /*
> > > > >>> +          * Two different dmabuf file descriptors may still refer to the
> > > > >>> +          * same dmabuf instance. Check this using inodes.
> > > > >>
> > > > >> Going back to the FileDescriptor::inode() extension, if that cached the
> > > > >> inode (it can't change) on either first call, or construction, couldn't
> > > > >> this whole check be simplified to:
> > > > >>
> > > > >>            if (plane.fd.inode() != planes_[0].fd.inode()) {
> > > > >>                    isContiguous = false;
> > > > >>                    break;
> > > > >>            }
> > > > >
> > > > > This would call fstat() every time, while the fd comparison here is
> > > > > meant to skip the fstat() calls in case the numerical fd match. We could
> > > > > do
> > > >
> > > > Did you miss the part where I said "If we cache the inode in
> > > > FileDescriptor"?
> > >
> > > No I didn't :-)
> > >
> > > > My intention was that caching there would mean we can simplify
> > > > comparisons here, without incurring repeated fstat calls.
> > >
> > > If the numerical fds are identical, there's no need to compare the
> > > inodes, and we can avoid calling fstat() at all.
> > >
> > > > >             if (plane.fd != planes_[0].fd)
> > > > >
> > > > > if we introduced an operator==(), but as explained in a reply to the
> > > > > patch that adds inode(), I'd like to handle that later, to avoid
> > > > > blocking this series.
> > > >
> > > > Ok, I still don't see the harm in caching the inode within
> > > > FileDescriptor(), it can't change once read the first time.
> > >
> > > What we'd need to do in operator==() is to first compare the numerical
> > > fds, and only compare the inodes if the fds differ. Technically that's
> > > not an issue, but it requires deciding what the semantics of equality is
> > > as explaining in a separate e-mail, and that's the part that I'd like to
> > > defer to avoid delaying the offset fixes.
> > >
> > > > But, it can be deferred:
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > >>> +          */
> > > > >>> +         if (plane.fd.fd() != planes_[0].fd.fd()) {
> > > > >>> +                 if (!inode)
> > > > >>> +                         inode = planes_[0].fd.inode();
> >
> > Shall error case (i.e plane.fd.inode() == 0) be handled?
>
> This can only happen if the application gives us an incorrect fd, I'm
> not sure how much we need to protect against applications doing very
> very bad things.
>

Okay, so let's assume this is caught by other places. :-)

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

-Hiro

> > > > >>> +                 if (plane.fd.inode() != inode) {
> > > > >>> +                         isContiguous = false;
> > > > >>> +                         break;
> > > > >>> +                 }
> > > > >>> +         }
> > > > >>> +
> > > > >>> +         offset += plane.length;
> > > > >>> + }
> > > > >>> +
> > > > >>> + LOG(Buffer, Debug)
> > > > >>> +         << "Buffer is " << (isContiguous ? "not " : "") << "contiguous";
> > > > >>> +
> > > > >>> + _d()->isContiguous_ = isContiguous;
> > > > >>>  }
> > > > >>>
> > > > >>>  /**
>
> --
> Regards,
>
> Laurent Pinchart