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

[Jacopo Mondi]

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

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

Thanks
  j

>
> --
> Regards
> --
> Kieran