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

[Jacopo Mondi]

Hi Laurent,

On Thu, Oct 22, 2020 at 05:55:06AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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
  j

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