[libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset in mapping IPABuffer

[Kieran Bingham]

Hi,

On 23/08/2021 23:55, Laurent Pinchart wrote:
> Hi Hiro,
> 
> Thank you for the patch.
> 
> On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:
>> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has
>> now an offset. This uses the offset variable to map the IPABuffer.
> 
> The commit message and subject line should be updated, this patch now
> moves to MappedFrameBuffer.
> 
>> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
>> ---
>>  .../libcamera/internal/mapped_framebuffer.h   |  1 +
>>  src/ipa/rkisp1/rkisp1.cpp                     | 31 +++++++------------
>>  src/libcamera/mapped_framebuffer.cpp          |  7 +++++
>>  3 files changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
>> index 42479541..ee0583d0 100644
>> --- a/include/libcamera/internal/mapped_framebuffer.h
>> +++ b/include/libcamera/internal/mapped_framebuffer.h
>> @@ -55,6 +55,7 @@ public:
>>  
>>  	using MapFlags = Flags<MapFlag>;
>>  
>> +	MappedFrameBuffer();
>>  	MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);
>>  };
>>  
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 06fb9640..54cf2885 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -10,7 +10,6 @@
>>  #include <queue>
>>  #include <stdint.h>
>>  #include <string.h>
>> -#include <sys/mman.h>
>>  
>>  #include <linux/rkisp1-config.h>
>>  #include <linux/v4l2-controls.h>
>> @@ -24,6 +23,8 @@
>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>>  #include <libcamera/request.h>
>>  
>> +#include <libcamera/internal/mapped_framebuffer.h>
>> +
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(IPARkISP1)
>> @@ -54,7 +55,7 @@ private:
>>  	void metadataReady(unsigned int frame, unsigned int aeState);
>>  
>>  	std::map<unsigned int, FrameBuffer> buffers_;
>> -	std::map<unsigned int, void *> buffersMemory_;
>> +	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>>  
>>  	ControlInfoMap ctrls_;
>>  
>> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>>  					     std::forward_as_tuple(buffer.planes));
>>  		const FrameBuffer &fb = elem.first->second;
>>  
>> -		/*
>> -		 * \todo Provide a helper to mmap() buffers (possibly exposed
>> -		 * to applications).
>> -		 */
>> -		buffersMemory_[buffer.id] = mmap(NULL,
>> -						 fb.planes()[0].length,
>> -						 PROT_READ | PROT_WRITE,
>> -						 MAP_SHARED,
>> -						 fb.planes()[0].fd.fd(),
>> -						 0);
>> -
>> -		if (buffersMemory_[buffer.id] == MAP_FAILED) {
>> -			int ret = -errno;
>> +		mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);
> 
> We could use emplace() here to avoid the need for a default constructor,
> but we wouldn't be able to use operator[]() below, which isn't nice. I
> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in
> mappedBuffers_, to avoid the default constructor ?
> 
> Kieran, you've designed the MappedFrameBuffer class, what do you think ?

Hrm, firstly the error_ flag needs to be fixed up if there's a default
constructor - mentioned below in the Constructor code.

But otherwise, even though we do have the isValid() check, I don't think
we have many users that actually check it.

The aim was to make it simple and easy to create the mapping, which is
why it happens on construction.

If this is causing problems, we can separate construction and mapping...
but then I'd expect to have map()/unmap() as well.


Looking at existing users of MappedFrameBuffer, I can see that:

 IPAVimc::mapBuffers() currently uses emplace, (with a
piecewise_construct, to be able to specify the mapping flags as
read-only), and only uses find() to get them.


So does IPU3. ...

And unsurprisingly, so does RPi.


However none of those implementations are checking the isValid() flag.
So perhaps they need to be updated to do so.

And if we mandate that the isValid() should be checked, then it's not
much different to separating construction and mapping...


The main goal for the current design is to allow other buffer types to
be constructed, so perhaps there could be a MappedDRMBuffer or
MappedDMABuffer or such.

As long as that's kept in mind, I don't mind if the main interface
changes to suit needs as we determine them.

Would using [], require checking the .isValid() flag at every usage?
(Because we might be given a new empty mapping, if the search failed?)

That might become less convenient ... ?



> Apart from this,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +		if (!mappedBuffers_[buffer.id].isValid()) {
>>  			LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: "
>> -					      << strerror(-ret);
>> +					      << strerror(mappedBuffers_[buffer.id].error());
>>  		}
>>  	}
>>  }
>> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>>  		if (fb == buffers_.end())
>>  			continue;
>>  
>> -		munmap(buffersMemory_[id], fb->second.planes()[0].length);
>> -		buffersMemory_.erase(id);
>> +		mappedBuffers_.erase(id);
>>  		buffers_.erase(id);
>>  	}
>>  }
>> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>>  		unsigned int bufferId = event.bufferId;
>>  
>>  		const rkisp1_stat_buffer *stats =
>> -			static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);
>> +			reinterpret_cast<rkisp1_stat_buffer *>(
>> +				mappedBuffers_[bufferId].maps()[0].data());
>>  
>>  		updateStatistics(frame, stats);
>>  		break;
>> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>>  		unsigned int bufferId = event.bufferId;
>>  
>>  		rkisp1_params_cfg *params =
>> -			static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]);
>> +			reinterpret_cast<rkisp1_params_cfg *>(
>> +				mappedBuffers_[bufferId].maps()[0].data());
>>  
>>  		queueRequest(frame, params, event.controls);
>>  		break;
>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
>> index 03425dea..34d9564d 100644
>> --- a/src/libcamera/mapped_framebuffer.cpp
>> +++ b/src/libcamera/mapped_framebuffer.cpp
>> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()
>>   * \brief A bitwise combination of MappedFrameBuffer::MapFlag values
>>   */
>>  
>> +/**
>> + * \brief Construct an empty MappedFrameBuffer
>> + */
>> +MappedFrameBuffer::MappedFrameBuffer()
>> +	: MappedBuffer()
>> +{

If this is created, error_ would have to be set to an ... error here.

Perhaps something like -ENOMEM, so that 'default constructed' empty
mappings are not 'valid'.

That would then necessitate changine the ordering of how error_ is used,
currently it's only set when an error occurs on mapping.

It would have to be set to 0 on successful mappings as well/instead.



>> +}
>>  /**
>>   * \brief Map all planes of a FrameBuffer
>>   * \param[in] buffer FrameBuffer to be mapped
>