[libcamera-devel] [PATCH v3 2/3] libcamera: Give MappedFrameBuffer its own implementation

[Hirokazu Honda]

Hi Kieran, thank you for the patch.

On Tue, Aug 10, 2021 at 1:40 AM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> On 09/08/2021 15:28, Laurent Pinchart wrote:
> > Hi Kieran,
> >
> > Thank you for the patch.
> >
> > On Mon, Aug 09, 2021 at 02:29:28PM +0100, Kieran Bingham wrote:
> >> The MappedFrameBuffer is a convenience feature which sits on top of the
> >> FrameBuffer and facilitates mapping it to CPU accessible memory with
> >> mmap.
> >>
> >> This implementation is internal and currently sits in the same internal
> >> files as the internal FrameBuffer, thus exposing those internals to
> >> users of the MappedFramebuffer implementation.
> >>
> >> Move the MappedFrameBuffer and MappedBuffer implementation to its own
> >> implementation files, and fix the sources throughout to use that
> >> accordingly.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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

> >> ---
> >>  include/libcamera/internal/framebuffer.h      |  36 ----
>
> <snip>
>
> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> >> new file mode 100644
> >> index 000000000000..0e30fc542154
> >> --- /dev/null
> >> +++ b/src/libcamera/mapped_framebuffer.cpp
> >> @@ -0,0 +1,171 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Google Inc.
> >> + *
> >> + * mapped_framebuffer.cpp - Mapped Framebuffer support
> >> + */
> >> +
> >> +#include "libcamera/internal/mapped_framebuffer.h"
> >> +
> >> +#include <errno.h>
> >> +#include <string.h>
> >> +#include <sys/mman.h>
> >> +#include <unistd.h>
> >
> > I think you can drop string.h and unistd.h.
> >
>
>
> iwyu says:
>
> ../src/libcamera/mapped_framebuffer.cpp should add these lines:
> #include <algorithm>                    // for max
> #include <ostream>                      // for operator<<
> #include <utility>                      // for move
> #include "libcamera/file_descriptor.h"  // for FileDescriptor
> #include "libcamera/framebuffer.h"      // for FrameBuffer,
> FrameBuffer::Plane
>
> ../src/libcamera/mapped_framebuffer.cpp should remove these lines:
> - #include <string.h>  // lines 11-11
> - #include <unistd.h>  // lines 13-13
>
>
> and also:
>
> ../src/libcamera/framebuffer.cpp should remove these lines:
> - #include <errno.h>  // lines 11-11
> - #include <string.h>  // lines 12-12
> - #include <sys/mman.h>  // lines 13-13
> - #include <unistd.h>  // lines 14-14
>
>
> while we're moving code on these files.
>
> I will remove those extra unused headers during this patch.
>
>
> I don't know if we want to go as far as including headers for
> /everything/ we use yet ... All of the above suggestions are already
> brought in by the other headers, and are not required to compile of
> course...
>
>
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Thanks
>
>
> >
> >> +
> >> +#include <libcamera/base/log.h>