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

[Kieran Bingham]

On 25/08/2021 21:49, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tue, Aug 24, 2021 at 01:44:15PM +0100, Kieran Bingham wrote:
>> On 23/08/2021 23:55, Laurent Pinchart wrote:
>>> 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...
> 
> I like having construction and mapping grouped together, but you're
> right that separating them could indeed force users to check for errors
> (if we annotate the map() function with __nodiscard).

Yes, the point was to make it easy to use.

But if it's verging on promotion to an actual API exposed to
applications, it's worth considering correctness

>> 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?)
> 
> Not really, a user could be trusted to pass valid keys to operator[]().
> Storing the instances in a std::map<std::unique_ptr<MappedFrameBuffer>>
> as I've proposed would result in a null pointer dereference if the code
> tried to access an entry with an invalid key, which wouldn't be very
> different than using a default-constructed MappedFrameBuffer. We would
> "just" crash later in that case :-)

So I'm not opposed to supporting the [], as long as it doesn't lead to
potential mis-usages of a MappedFrameBuffer which aren't valid.

And the .at() works too.

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