[libcamera-devel,v2,1/2] pipeline: raspberrypi: Use MappedFrameBuffer for embedded data buffers

10424 diff mbox series

Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline
handler to use in the cases where the sensor does not fill it in. This
avoids the need to mmap and unmap on every frame.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 33 ++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

Hi Naush,

Thank you for the patch.

On Mon, Nov 16, 2020 at 11:24:57AM +0000, Naushir Patuck wrote:
> Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline
> handler to use in the cases where the sensor does not fill it in. This
> avoids the need to mmap and unmap on every frame.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 33 ++++++++++++++-----
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index dd62dfc7..faa06c00 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -24,6 +24,7 @@
>  #include <linux/videodev2.h>
>  
>  #include "libcamera/internal/bayer_format.h"
> +#include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/ipa_manager.h"
> @@ -165,6 +166,12 @@ public:
>  	/* Stores the ids of the buffers mapped in the IPA. */
>  	std::unordered_set<unsigned int> ipaBuffers_;
>  
> +	/*
> +	 * Map of (internal) mmaped embedded data buffers, to avoid having to
> +	 * map/unmap on every frame.
> +	 */
> +	std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_;
> +
>  	/* DMAHEAP allocation helper. */
>  	RPi::DmaHeap dmaHeap_;
>  	FileDescriptor lsTable_;
> @@ -1040,6 +1047,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			return ret;
>  	}
>  
> +	if (!data->sensorMetadata_) {
> +		for (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) {
> +			data->mappedEmbeddedBuffers_.emplace(it.first,
> +							     MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));

That's a fairly long line, could we write

			MappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE);
			data->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb));

Alternatively, if you want to avoid the move construction of the
element, you could write

			data->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,
							     std::forward_as_tuple(it.first),
							     std::forward_as_tuple(it.second,
										   PROT_READ | PROT_WRITE));

> +		}
> +	}
> +
>  	/*
>  	 * Pass the stats and embedded data buffers to the IPA. No other
>  	 * buffers need to be passed.
> @@ -1078,6 +1092,7 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)
>  	std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());
>  	data->ipa_->unmapBuffers(ipaBuffers);
>  	data->ipaBuffers_.clear();
> +	data->mappedEmbeddedBuffers_.clear();
>  
>  	for (auto const stream : data->streams_)
>  		stream->releaseBuffers();
> @@ -1310,14 +1325,16 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  		 * metadata buffer.
>  		 */
>  		if (!sensorMetadata_) {
> -			const FrameBuffer &fb = buffer->planes();
> -			uint32_t *mem = static_cast<uint32_t *>(::mmap(nullptr, fb.planes()[0].length,
> -								       PROT_READ | PROT_WRITE,
> -								       MAP_SHARED,
> -								       fb.planes()[0].fd.fd(), 0));
> -			mem[0] = ctrl[V4L2_CID_EXPOSURE];
> -			mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];
> -			munmap(mem, fb.planes()[0].length);
> +			unsigned int bufferId = unicam_[Unicam::Embedded].getBufferId(buffer);
> +			auto it = mappedEmbeddedBuffers_.find(bufferId);
> +			if (it != mappedEmbeddedBuffers_.end()) {
> +				uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data());
> +				mem[0] = ctrl[V4L2_CID_EXPOSURE];
> +				mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];
> +			} else {
> +				LOG(RPI, Warning) << "Failed to find embedded buffer "
> +						  << bufferId;
> +			}

Out of scope for this series, but I wonder if we shouldn't improve this
with a cleaner API to the IPA instead of faking an embedded data buffer.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		}
>  	}
>

Hi Laurent,

Thank you for your comments.  I will apply your suggestions for both
patches in this series and submit a v3 to merge.

Regards,
Naush


On Mon, 16 Nov 2020 at 12:28, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

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