[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 5:40 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> 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?
At least, I would move offset to before isContiguous.

> > >>> +
> > >>> + 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?

-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