[libcamera-devel] [PATCH 3/9] libcamera: pipeline: Support external buffers

[Kieran Bingham]

Hi Jacopo,

On 06/07/2019 12:45, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-07-05 00:53:28 +0200, Jacopo Mondi wrote:
>> Add support for use external buffers to libcamera pipeline handlers.
>>
>> Use the memory type flag in pipeline handlers (todo: change all pipeline
>> handlers) during buffer setup operations, to decide if the Stream's
>> internal memory has to be exported to applications, or the Stream
>> should prepare to use buffers whose memory is allocated elsewhere.
> 
> I think you can drop the TODO now as it seems all pipeline handlers are 
> now supprted, right?
> 
>>
>> To support the last use case, a translation mechanism between the external
>> buffers provided by applications and internal ones used by pipeline
>> handlers to feed the video device will be implemented on top of this
>> change.
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 31 +++++++++++++++++++++++-
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  9 ++++++-
>>  src/libcamera/pipeline/uvcvideo.cpp      |  8 +++++-
>>  src/libcamera/pipeline/vimc.cpp          |  8 +++++-
>>  4 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 2de0892138a8..6f14da1f8c70 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -71,6 +71,7 @@ public:
>>  
>>  	int importBuffers(BufferPool *pool);
>>  	int exportBuffers(ImgUOutput *output, BufferPool *pool);
>> +	int reserveDmabufBuffers(ImgUOutput *output, BufferPool *pool);
>>  	void freeBuffers();
>>  
>>  	int start();
>> @@ -624,7 +625,11 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>>  		IPU3Stream *stream = static_cast<IPU3Stream *>(s);
>>  		ImgUDevice::ImgUOutput *dev = stream->device_;
>>  
>> -		ret = imgu->exportBuffers(dev, &stream->bufferPool());
>> +		if (stream->memoryType() == ExternalMemory)
>> +			ret = imgu->reserveDmabufBuffers(dev,
>> +							 &stream->bufferPool());
> 
> I would s/reserveDmabufBuffers/importBuffers/ to match the logic in 
> other pipeline handlers.


I concur.

Another option could be to move the common code between importing and
exporting to a private function called something like
setupInternalBuffers(), then call that at the beginning of two functions
called allocateBuffers() and importBuffers()?

then allocateBuffers() would still be named appropriately?




> 
>> +		else
>> +			ret = imgu->exportBuffers(dev, &stream->bufferPool());
>>  		if (ret)
>>  			goto error;
>>  	}
>> @@ -1153,6 +1158,30 @@ int ImgUDevice::exportBuffers(ImgUOutput *output, BufferPool *pool)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * \brief Reserve buffers in \a output from the provided \a pool
>> + * \param[in] output The ImgU output device
>> + * \param[in] pool The buffer pool used to reserve buffers in \a output
>> + *
>> + * Reserve a number of buffers equal to the number of buffers in \a pool
>> + * in the \a output device to prepare for streaming operations using buffers
>> + * whose memory has been reserved elsewhere identified with DMABUF file
>> + * descriptors.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int ImgUDevice::reserveDmabufBuffers(ImgUOutput *output, BufferPool *pool)
>> +{
>> +	int ret = output->dev->importBuffers(pool);
>> +	if (ret) {
>> +		LOG(IPU3, Error) << "Failed to import buffer in "
>> +				 << output->name << " ImgU device";
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * \brief Release buffers for all the ImgU video devices
>>   */
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 4a5898d25f91..dbb61a21354d 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -319,7 +319,14 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>>  					   const std::set<Stream *> &streams)
>>  {
>>  	Stream *stream = *streams.begin();
>> -	return video_->exportBuffers(&stream->bufferPool());
>> +	int ret;
>> +
>> +	if (stream->memoryType() == InternalMemory)
>> +		ret = video_->exportBuffers(&stream->bufferPool());
>> +	else
>> +		ret = video_->importBuffers(&stream->bufferPool());
>> +
>> +	return ret;
>>  }
>>  
>>  int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
>> index 72313cfd246f..5b3c79dda6dd 100644
>> --- a/src/libcamera/pipeline/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
>> @@ -197,10 +197,16 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera,
>>  	UVCCameraData *data = cameraData(camera);
>>  	Stream *stream = *streams.begin();
>>  	const StreamConfiguration &cfg = stream->configuration();
>> +	int ret;
>>  
>>  	LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
>>  
>> -	return data->video_->exportBuffers(&stream->bufferPool());
>> +	if (stream->memoryType() == InternalMemory)
>> +		ret = data->video_->exportBuffers(&stream->bufferPool());
>> +	else
>> +		ret = data->video_->importBuffers(&stream->bufferPool());
>> +
>> +	return ret;
>>  }
>>  
>>  int PipelineHandlerUVC::freeBuffers(Camera *camera,
>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
>> index f8a58be060bb..c67bdb0ecbff 100644
>> --- a/src/libcamera/pipeline/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc.cpp
>> @@ -199,10 +199,16 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera,
>>  	VimcCameraData *data = cameraData(camera);
>>  	Stream *stream = *streams.begin();
>>  	const StreamConfiguration &cfg = stream->configuration();
>> +	int ret;
>>  
>>  	LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
>>  
>> -	return data->video_->exportBuffers(&stream->bufferPool());
>> +	if (stream->memoryType() == InternalMemory)
>> +		ret = data->video_->exportBuffers(&stream->bufferPool());
>> +	else
>> +		ret = data->video_->importBuffers(&stream->bufferPool());
>> +
>> +	return ret;
>>  }
>>  
>>  int PipelineHandlerVimc::freeBuffers(Camera *camera,
>> -- 
>> 2.21.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran