[libcamera-devel] [PATCH v3 09/14] libcamera: ipu3: Register camera controls

[Laurent Pinchart]

Hi Jacopo,

On Fri, Oct 23, 2020 at 07:06:45PM +0200, Jacopo Mondi wrote:
> On Thu, Oct 22, 2020 at 05:55:06AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 21, 2020 at 04:36:30PM +0200, Jacopo Mondi wrote:
> > > Register controls for the IPU3 pipeline handler. The only supported
> > > Camera control is currently the pipeline depth control.
> > >
> > > Report the minimum and maximum values the pipeline handler supports
> > > for the pipeline processing stages.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > How about squashing this with 10/14 ?
> >
> > > ---
> > >  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 af47739d8d4f..a90683ea523a 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/request.h>
> > >  #include <libcamera/stream.h>
> > > @@ -40,6 +41,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::draft::PipelineDepth, ControlInfo(2, 3) },
> > > +};
> > > +
> > >  class IPU3CameraData : public CameraData
> > >  {
> > >  public:
> > > @@ -771,6 +776,9 @@ int PipelineHandlerIPU3::registerCameras()
> > >  		/* Initialize the camera properties. */
> > >  		data->properties_ = cio2->sensor()->properties();
> > >
> > > +		/* Initialze the camera controls. */
> > > +		data->controlInfo_ = IPU3Controls;
> >
> > Hmmmm... This control can only be reported in metadata, it can't be set
> > in requests. This isn't something that needs to be addressed in this
> > series, but do you have any idea on how we should handle that to avoid
> > giving the impression to applications that they can set the pipeline
> > depth ? Or maybe we shouldn't handle this at compile time or runtime,
> > just document it ?
> 
> We consciously decided to have a single namespace for controls and
> metdata as you very well know. So far we've handled that at the
> documentation level but I see the need to report to application what
> they should expect to be able to modify and what can only be inspected
> upon request completion.
> 
> The easiest way possible would be to add a data->metadataInfo_ where
> to report ControlInfo for the metadata. There might be an overlap
> between data->controlInfo_, but it might be expected.

Thanks for sharing your ideas, that's more or less what I had in mind
too. Let's keep it in mind for later.

> > > +
> > >  		/**
> > >  		 * \todo Dynamically assign ImgU and output devices to each
> > >  		 * stream and camera; as of now, limit support to two cameras

-- 
Regards,

Laurent Pinchart