[libcamera-devel,05/10] libcamera: ipu3: Register pipeline properties
diff mbox series

Message ID 20201009122101.73858-6-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 9, 2020, 12:20 p.m. UTC 
Register three pipeline properties in the IPU3 pipeline handler:
- properties::draft::PipelineMaxDepth
- properties::draft::AvailableNoiseReductionModes
- properties::draft::AvailableColorCorrectionAberrationModes
- properties::draft::AvailableLensShadingMapModes

IPU3 reports a maximum of 3 processing stages: exposure, capture and
ISP processing. It does not support noise reduction, color
aberration and lens shading map modes.

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

Comments

Kieran Bingham Oct. 9, 2020, 12:31 p.m. UTC | #1 
Hi Jacopo,

On 09/10/2020 13:20, Jacopo Mondi wrote:
> Register three pipeline properties in the IPU3 pipeline handler:
> - properties::draft::PipelineMaxDepth
> - properties::draft::AvailableNoiseReductionModes
> - properties::draft::AvailableColorCorrectionAberrationModes
> - properties::draft::AvailableLensShadingMapModes
> 
> IPU3 reports a maximum of 3 processing stages: exposure, capture and
> ISP processing. It does not support noise reduction, color
> aberration and lens shading map modes.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f5a20d30fd03..f7ade2a6d5f3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -770,6 +771,13 @@ int PipelineHandlerIPU3::registerCameras()
>  
>  		/* Initialize the camera properties. */
>  		data->properties_ = cio2->sensor()->properties();
> +		data->properties_.set(properties::draft::PipelineMaxDepth, 3);

You've detailed in the commit message that the 3 stages are Exposure
Capture and ISP processing.

It might be nice to add a comment above the PipelineMaxDepth entry here,
to state that (to explain why 3 is set here), and to make it clear
why/when/what is updated in the future.

But otherwise, or anyway,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +		data->properties_.set(properties::draft::AvailableNoiseReductionModes,
> +				      { static_cast<int32_t>(properties::draft::NoiseReductionModeOff) });
> +		data->properties_.set(properties::draft::AvailableColorCorrectionAberrationModes,
> +				      { static_cast<int32_t>(properties::draft::ColorCorrectionAberrationOff) });
> +		data->properties_.set(properties::draft::AvailableLensShadingMapModes,
> +				      { static_cast<int32_t>(properties::draft::LensShadingMapModeOff) });
>  
>  		/**
>  		 * \todo Dynamically assign ImgU and output devices to each
>
Laurent Pinchart Oct. 12, 2020, 1:05 a.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Fri, Oct 09, 2020 at 02:20:56PM +0200, Jacopo Mondi wrote:
> Register three pipeline properties in the IPU3 pipeline handler:
> - properties::draft::PipelineMaxDepth
> - properties::draft::AvailableNoiseReductionModes
> - properties::draft::AvailableColorCorrectionAberrationModes
> - properties::draft::AvailableLensShadingMapModes
> 
> IPU3 reports a maximum of 3 processing stages: exposure, capture and
> ISP processing. It does not support noise reduction, color
> aberration and lens shading map modes.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f5a20d30fd03..f7ade2a6d5f3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -770,6 +771,13 @@ int PipelineHandlerIPU3::registerCameras()
>  
>  		/* Initialize the camera properties. */
>  		data->properties_ = cio2->sensor()->properties();
> +		data->properties_.set(properties::draft::PipelineMaxDepth, 3);
> +		data->properties_.set(properties::draft::AvailableNoiseReductionModes,
> +				      { static_cast<int32_t>(properties::draft::NoiseReductionModeOff) });
> +		data->properties_.set(properties::draft::AvailableColorCorrectionAberrationModes,
> +				      { static_cast<int32_t>(properties::draft::ColorCorrectionAberrationOff) });
> +		data->properties_.set(properties::draft::AvailableLensShadingMapModes,
> +				      { static_cast<int32_t>(properties::draft::LensShadingMapModeOff) });

When the only supported mode is off, how about not declaring the
property in the pipeline handler ? Otherwise that will be lots of
boilerplate code in pipeline handlers.

>  
>  		/**
>  		 * \todo Dynamically assign ImgU and output devices to each
Jacopo Mondi Oct. 13, 2020, 8:58 p.m. UTC | #3 
Hi Laurent,

On Mon, Oct 12, 2020 at 04:05:10AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Oct 09, 2020 at 02:20:56PM +0200, Jacopo Mondi wrote:
> > Register three pipeline properties in the IPU3 pipeline handler:
> > - properties::draft::PipelineMaxDepth
> > - properties::draft::AvailableNoiseReductionModes
> > - properties::draft::AvailableColorCorrectionAberrationModes
> > - properties::draft::AvailableLensShadingMapModes
> >
> > IPU3 reports a maximum of 3 processing stages: exposure, capture and
> > ISP processing. It does not support noise reduction, color
> > aberration and lens shading map modes.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f5a20d30fd03..f7ade2a6d5f3 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -13,6 +13,7 @@
> >
> >  #include <libcamera/camera.h>
> >  #include <libcamera/formats.h>
> > +#include <libcamera/property_ids.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> > @@ -770,6 +771,13 @@ int PipelineHandlerIPU3::registerCameras()
> >
> >  		/* Initialize the camera properties. */
> >  		data->properties_ = cio2->sensor()->properties();
> > +		data->properties_.set(properties::draft::PipelineMaxDepth, 3);
> > +		data->properties_.set(properties::draft::AvailableNoiseReductionModes,
> > +				      { static_cast<int32_t>(properties::draft::NoiseReductionModeOff) });
> > +		data->properties_.set(properties::draft::AvailableColorCorrectionAberrationModes,
> > +				      { static_cast<int32_t>(properties::draft::ColorCorrectionAberrationOff) });
> > +		data->properties_.set(properties::draft::AvailableLensShadingMapModes,
> > +				      { static_cast<int32_t>(properties::draft::LensShadingMapModeOff) });
>
> When the only supported mode is off, how about not declaring the
> property in the pipeline handler ? Otherwise that will be lots of
> boilerplate code in pipeline handlers.
>

Yes, I think for now it's a good idea and will save quite some
boilerplate code in pipeline handlers.

> >
> >  		/**
> >  		 * \todo Dynamically assign ImgU and output devices to each
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index f5a20d30fd03..f7ade2a6d5f3 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -13,6 +13,7 @@ 
 
 #include <libcamera/camera.h>
 #include <libcamera/formats.h>
+#include <libcamera/property_ids.h>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
@@ -770,6 +771,13 @@  int PipelineHandlerIPU3::registerCameras()
 
 		/* Initialize the camera properties. */
 		data->properties_ = cio2->sensor()->properties();
+		data->properties_.set(properties::draft::PipelineMaxDepth, 3);
+		data->properties_.set(properties::draft::AvailableNoiseReductionModes,
+				      { static_cast<int32_t>(properties::draft::NoiseReductionModeOff) });
+		data->properties_.set(properties::draft::AvailableColorCorrectionAberrationModes,
+				      { static_cast<int32_t>(properties::draft::ColorCorrectionAberrationOff) });
+		data->properties_.set(properties::draft::AvailableLensShadingMapModes,
+				      { static_cast<int32_t>(properties::draft::LensShadingMapModeOff) });
 
 		/**
 		 * \todo Dynamically assign ImgU and output devices to each