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

[Laurent Pinchart]

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.

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