<div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your comments.  I will apply your suggestions for both patches in this series and submit a v3 to merge.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 16 Nov 2020 at 12:28, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Nov 16, 2020 at 11:24:57AM +0000, Naushir Patuck wrote:<br>
> Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline<br>
> handler to use in the cases where the sensor does not fill it in. This<br>
> avoids the need to mmap and unmap on every frame.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 33 ++++++++++++++-----<br>
>  1 file changed, 25 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index dd62dfc7..faa06c00 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -24,6 +24,7 @@<br>
>  #include <linux/videodev2.h><br>
>  <br>
>  #include "libcamera/internal/bayer_format.h"<br>
> +#include "libcamera/internal/buffer.h"<br>
>  #include "libcamera/internal/camera_sensor.h"<br>
>  #include "libcamera/internal/device_enumerator.h"<br>
>  #include "libcamera/internal/ipa_manager.h"<br>
> @@ -165,6 +166,12 @@ public:<br>
>       /* Stores the ids of the buffers mapped in the IPA. */<br>
>       std::unordered_set<unsigned int> ipaBuffers_;<br>
>  <br>
> +     /*<br>
> +      * Map of (internal) mmaped embedded data buffers, to avoid having to<br>
> +      * map/unmap on every frame.<br>
> +      */<br>
> +     std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_;<br>
> +<br>
>       /* DMAHEAP allocation helper. */<br>
>       RPi::DmaHeap dmaHeap_;<br>
>       FileDescriptor lsTable_;<br>
> @@ -1040,6 +1047,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
>                       return ret;<br>
>       }<br>
>  <br>
> +     if (!data->sensorMetadata_) {<br>
> +             for (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) {<br>
> +                     data->mappedEmbeddedBuffers_.emplace(it.first,<br>
> +                                                          MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));<br>
<br>
That's a fairly long line, could we write<br>
<br>
                        MappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE);<br>
                        data->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb));<br>
<br>
Alternatively, if you want to avoid the move construction of the<br>
element, you could write<br>
<br>
                        data->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,<br>
                                                             std::forward_as_tuple(it.first),<br>
                                                             std::forward_as_tuple(it.second,<br>
                                                                                   PROT_READ | PROT_WRITE));<br>
<br>
> +             }<br>
> +     }<br>
> +<br>
>       /*<br>
>        * Pass the stats and embedded data buffers to the IPA. No other<br>
>        * buffers need to be passed.<br>
> @@ -1078,6 +1092,7 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)<br>
>       std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());<br>
>       data->ipa_->unmapBuffers(ipaBuffers);<br>
>       data->ipaBuffers_.clear();<br>
> +     data->mappedEmbeddedBuffers_.clear();<br>
>  <br>
>       for (auto const stream : data->streams_)<br>
>               stream->releaseBuffers();<br>
> @@ -1310,14 +1325,16 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
>                * metadata buffer.<br>
>                */<br>
>               if (!sensorMetadata_) {<br>
> -                     const FrameBuffer &fb = buffer->planes();<br>
> -                     uint32_t *mem = static_cast<uint32_t *>(::mmap(nullptr, fb.planes()[0].length,<br>
> -                                                                    PROT_READ | PROT_WRITE,<br>
> -                                                                    MAP_SHARED,<br>
> -                                                                    fb.planes()[0].fd.fd(), 0));<br>
> -                     mem[0] = ctrl[V4L2_CID_EXPOSURE];<br>
> -                     mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];<br>
> -                     munmap(mem, fb.planes()[0].length);<br>
> +                     unsigned int bufferId = unicam_[Unicam::Embedded].getBufferId(buffer);<br>
> +                     auto it = mappedEmbeddedBuffers_.find(bufferId);<br>
> +                     if (it != mappedEmbeddedBuffers_.end()) {<br>
> +                             uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data());<br>
> +                             mem[0] = ctrl[V4L2_CID_EXPOSURE];<br>
> +                             mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];<br>
> +                     } else {<br>
> +                             LOG(RPI, Warning) << "Failed to find embedded buffer "<br>
> +                                               << bufferId;<br>
> +                     }<br>
<br>
Out of scope for this series, but I wonder if we shouldn't improve this<br>
with a cleaner API to the IPA instead of faking an embedded data buffer.<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
>               }<br>
>       }<br>
>  <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>