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

[Kieran Bingham]

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