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

Message ID 20200911162039.61933-7-jacopo@jmondi.org
State Superseded, archived
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: Define draft controls and properties
Related show

Commit Message

Jacopo Mondi Sept. 11, 2020, 4:20 p.m. UTC
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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Niklas Söderlund Sept. 13, 2020, 11:46 a.m. UTC | #1
Hi Jacopo,

Thanks for your work.

On 2020-09-11 18:20:39 +0200, 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.

Is this something we should address (at lest on how we want this to be 
handled) before adding this control to the IPU3 pipeline? I'm a bit 
worried about adding it without a plan.

> 
> Signed-off-by: Jacopo Mondi <jacopo@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) }
> +};
> +
>  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);
>  	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);
>  			pipe_->completeRequest(camera_, request);
>  			return;
>  		}
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 21, 2020, 2:58 p.m. UTC | #2
Hi Niklas,

On Sun, Sep 13, 2020 at 01:46:37PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-11 18:20:39 +0200, 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.
>
> Is this something we should address (at lest on how we want this to be
> handled) before adding this control to the IPU3 pipeline? I'm a bit
> worried about adding it without a plan.
>

mmm, I might have been pessimistic. For the sake of reporting the
draft control to android I think this is fine for now, as Android as
well represents this with a metadata relatively to the request and not
to each Stream.

Thanks
  j

> >
> > Signed-off-by: Jacopo Mondi <jacopo@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) }
> > +};
> > +
> >  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);
> >  	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);
> >  			pipe_->completeRequest(camera_, request);
> >  			return;
> >  		}
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Kieran Bingham Oct. 7, 2020, 6:41 p.m. UTC | #3
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' ?


> Signed-off-by: Jacopo Mondi <jacopo@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 ?

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.

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);
>  	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);
>  			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@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
Jacopo Mondi Oct. 8, 2020, 10:37 a.m. UTC | #4
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@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@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
Kieran Bingham Oct. 8, 2020, 10:42 a.m. UTC | #5
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@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@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

Patch

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) }
+};
+
 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);
 	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);
 			pipe_->completeRequest(camera_, request);
 			return;
 		}