[libcamera-devel] [RFC 6/6] libcamera: ipu3: Report pipeline depth

[Kieran Bingham]

Hi Jacopo,

On 08/10/2020 11:37, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Oct 07, 2020 at 07:41:56PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 11/09/2020 17:20, Jacopo Mondi wrote:
>>> Report for each request the pipeline depth describing the number of
>>> processing steps the frames went into.
>>>
>>> This change shows a limitation in the current implementation as the
>>> number of processing steps a frame has been run through is a property
>>> of each Stream. Currently we report the maximum depth among all the
>>> returned frames.
>>
>> So do we need 'stream properties' ?
>>
> 
> As well as we might need 'stream controls'.
> 
>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 9ce329a83f5d..26ea26430f8e 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -12,6 +12,7 @@
>>>  #include <vector>
>>>
>>>  #include <libcamera/camera.h>
>>> +#include <libcamera/control_ids.h>
>>>  #include <libcamera/formats.h>
>>>  #include <libcamera/property_ids.h>
>>>  #include <libcamera/request.h>
>>> @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
>>>  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
>>>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
>>>
>>> +static const ControlInfoMap IPU3Controls = {
>>> +	{ &controls::DraftPipelineDepth, ControlInfo(2, 3) }
>>> +};
>>> +
>>
>> I presume we're going to need to expose controls that are read from the
>> V4L2 device too. Things like (given my recent discussion with David) the
>> DigitalGain perhaps, or other things which are controls of the ISP in
>> particular ?
>>
> 
> To support manual control of the sensor we'll have to register a ControlInfo
> for each sensor control, yes.
> 
> This opens another question: metadata vs controls.
> This is here just to be able to report a metadata, not a control which
> can be set by the application. So this is not needed and the below
> part is wrong
> 
>> This might need to be updated at load/runtime in that case, so I don't
>> think the static will be suitable - but that's a future problem.
> 
> More than the static, it's the const the problem here.
> The ControlInfoMap will have to be augmented at runtime yes, but for
> controls, not metadata as I'm doing here.
> 
>>
>> I'm actually not worried about that here right now - though perhaps we
>> should add some sort of a \todo.
>>
>>
>>>  class IPU3CameraData : public CameraData
>>>  {
>>>  public:
>>> @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras()
>>>  		data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes,
>>>  				      { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) });
>>>
>>> +		data->controlInfo_ = IPU3Controls;
>>> +
>>>  		/**
>>>  		 * \todo Dynamically assign ImgU and output devices to each
>>>  		 * stream and camera; as of now, limit support to two cameras
>>> @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>>>  		return;
>>>
>>>  	/* Mark the request as complete. */
>>> +	request->controls().set(controls::DraftPipelineDepth, 3);
> 
> This should be request->metadata()

Oh - yes of course - the app can't 'set' the depth ;-)

> 
>>>  	pipe_->completeRequest(camera_, request);
>>>  }
>>>
>>> @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>>>  	if (request->findBuffer(&rawStream_)) {
>>>  		bool isComplete = pipe_->completeBuffer(camera_, request, buffer);
>>>  		if (isComplete) {
>>> +			request->controls().set(controls::DraftPipelineDepth, 2);
> 
> Same here.
> 
> I had this in a fixup in my branch, I noticed this when I tried wiring
> this up to Android HAL.
> 
>>>  			pipe_->completeRequest(camera_, request);
>>>  			return;
>>>  		}
>>>
>>
>> Otherwise, this patch is actually simple enough that I think I can give
>> this already:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>> Although, I expect there to be some minimal changes in here as this
>> series develops anyway, but nothing that I foresee being troublesome
> 
> Yeah, I think I'll drop all tags in next series, as I think I've mixed
> up quire a few things in this first attempt.

That's fine too!

Look forward to the updates.

Kieran


> 
> Thanks
>   j
> 
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran