[libcamera-devel,v4,3/3] libcamera: pipeline: ipu3: Apply the requested test pattern mode
diff mbox series

Message ID 20211104064252.2091330-4-hiroh@chromium.org
State Superseded
Headers show
Series
  • Apply a sensor test pattern mode
Related show

Commit Message

Hirokazu Honda Nov. 4, 2021, 6:42 a.m. UTC 
This enables ipu3 pipeline handler to apply the test pattern mode
per frame.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Nov. 8, 2021, 10:16 p.m. UTC | #1 
Quoting Hirokazu Honda (2021-11-04 06:42:52)
> This enables ipu3 pipeline handler to apply the test pattern mode
> per frame.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 63cb7f11..cb141ba9 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -59,6 +59,7 @@ public:
>         void statBufferReady(FrameBuffer *buffer);
>         void queuePendingRequests();
>         void cancelPendingRequests();
> +       void frameStart(uint32_t sequence);
>  
>         CIO2Device cio2_;
>         ImgUDevice *imgu_;
> @@ -78,6 +79,7 @@ public:
>         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
>  
>         std::queue<Request *> pendingRequests_;
> +       std::queue<Request *> processingRequests_;

I'm a bit weary of having requests on multiple queues like this.
It might be better if we could add some documentation describing the
purpose of each queue, and what lifetime a request has on each one.


Perhaps a banner comment before processingRequests_ to describe briefly
that the requests will sit on this queue to synchronise setting controls
at frameStart time? I'm not sure what level will help - but it seems a
bit vague for 'processingRequests' and 'pendingRequests' otherwise.



>  
>         ControlInfoMap ipaControls_;
>  
> @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>         cio2->sensor()->sensorInfo(&sensorInfo);
>         data->cropRegion_ = sensorInfo.analogCrop;
>  
> +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> +

This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())


>         /*
>          * Configure the H/V flip controls based on the combination of
>          * the sensor and user transform.
> @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>         int ret = 0;
>  
>         data->cancelPendingRequests();
> +       data->processingRequests_ = {};

We add requests to the processingRequests_ during
queuePendingRequests(), so maybe we should clear them during
cancelPendingRequests()?

>         data->ipa_->stop();
>  
>         ret |= data->imgu_->stop();
>         ret |= data->cio2_.stop();
> +
>         if (ret)
>                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
>  
> @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
>  
>                 info->rawBuffer = rawBuffer;
>  
> +               processingRequests_.push(request);
> +
>                 ipa::ipu3::IPU3Event ev;
>                 ev.op = ipa::ipu3::EventProcessControls;
>                 ev.frame = info->id;
> @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()
>                 data->delayedCtrls_ =
>                         std::make_unique<DelayedControls>(cio2->sensor()->device(),
>                                                           params);
> -               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> -                                                &DelayedControls::applyControls);
> +               data->cio2_.frameStart().connect(data.get(),
> +                                                &IPU3CameraData::frameStart);

I like having an explicit/clear frameStart event function here.

>  
>                 /* Convert the sensor rotation to a transformation */
>                 int32_t rotation = 0;
> @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>         ipa_->processEvent(ev);
>  }
>  
> +void IPU3CameraData::frameStart(uint32_t sequence)
> +{
> +       if (!processingRequests_.empty()) {
> +               /* Assumes applying the test pattern mode affects immediately. */

That comment seems misplaced now.
Perhaps you could add:

		  /*
		   * Handle controls which are to be set ready for the
		   * next frame to start.
		   */

But I'm not sure of the race/sequencing here. I presume at the point we
have a frameStart event, any controls we apply will take effect for the
/next/ frame ? This frame has already started...

Or do we expect to set a control here and have it take effect on this
frame? (That sounds way too racy to be possible).

We're going to need to be able to set other controls at this point,
perhaps from the IPA too, so I expect we'll see some further rework here 
later.

> +               Request *request = processingRequests_.front();
> +               processingRequests_.pop();
> +
> +               int ret = cio2_.sensor()->applyRequestControls(request);
> +               if (ret)
> +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> +       }
> +
> +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> +       delayedCtrls_->applyControls(sequence);
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>  
>  } /* namespace libcamera */
> -- 
> 2.33.1.1089.g2158813163f-goog
>
Hirokazu Honda Nov. 9, 2021, 6:39 a.m. UTC | #2 
Hi Kieran,

On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Hirokazu Honda (2021-11-04 06:42:52)
> > This enables ipu3 pipeline handler to apply the test pattern mode
> > per frame.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 63cb7f11..cb141ba9 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -59,6 +59,7 @@ public:
> >         void statBufferReady(FrameBuffer *buffer);
> >         void queuePendingRequests();
> >         void cancelPendingRequests();
> > +       void frameStart(uint32_t sequence);
> >
> >         CIO2Device cio2_;
> >         ImgUDevice *imgu_;
> > @@ -78,6 +79,7 @@ public:
> >         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> >
> >         std::queue<Request *> pendingRequests_;
> > +       std::queue<Request *> processingRequests_;
>
> I'm a bit weary of having requests on multiple queues like this.
> It might be better if we could add some documentation describing the
> purpose of each queue, and what lifetime a request has on each one.
>
>
> Perhaps a banner comment before processingRequests_ to describe briefly
> that the requests will sit on this queue to synchronise setting controls
> at frameStart time? I'm not sure what level will help - but it seems a
> bit vague for 'processingRequests' and 'pendingRequests' otherwise.
>
>
>
> >
> >         ControlInfoMap ipaControls_;
> >
> > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >         cio2->sensor()->sensorInfo(&sensorInfo);
> >         data->cropRegion_ = sensorInfo.analogCrop;
> >
> > +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> > +
>
> This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())
>
>
> >         /*
> >          * Configure the H/V flip controls based on the combination of
> >          * the sensor and user transform.
> > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >         int ret = 0;
> >
> >         data->cancelPendingRequests();
> > +       data->processingRequests_ = {};
>
> We add requests to the processingRequests_ during
> queuePendingRequests(), so maybe we should clear them during
> cancelPendingRequests()?
>
> >         data->ipa_->stop();
> >
> >         ret |= data->imgu_->stop();
> >         ret |= data->cio2_.stop();
> > +
> >         if (ret)
> >                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> >
> > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
> >
> >                 info->rawBuffer = rawBuffer;
> >
> > +               processingRequests_.push(request);
> > +
> >                 ipa::ipu3::IPU3Event ev;
> >                 ev.op = ipa::ipu3::EventProcessControls;
> >                 ev.frame = info->id;
> > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()
> >                 data->delayedCtrls_ =
> >                         std::make_unique<DelayedControls>(cio2->sensor()->device(),
> >                                                           params);
> > -               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > -                                                &DelayedControls::applyControls);
> > +               data->cio2_.frameStart().connect(data.get(),
> > +                                                &IPU3CameraData::frameStart);
>
> I like having an explicit/clear frameStart event function here.
>

Sorry, I don't get this comment. Could you elaborate?

> >
> >                 /* Convert the sensor rotation to a transformation */
> >                 int32_t rotation = 0;
> > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >         ipa_->processEvent(ev);
> >  }
> >
> > +void IPU3CameraData::frameStart(uint32_t sequence)
> > +{
> > +       if (!processingRequests_.empty()) {
> > +               /* Assumes applying the test pattern mode affects immediately. */
>
> That comment seems misplaced now.
> Perhaps you could add:
>
>                   /*
>                    * Handle controls which are to be set ready for the
>                    * next frame to start.
>                    */
>
> But I'm not sure of the race/sequencing here. I presume at the point we
> have a frameStart event, any controls we apply will take effect for the
> /next/ frame ? This frame has already started...
>
> Or do we expect to set a control here and have it take effect on this
> frame? (That sounds way too racy to be possible).
>

That's good point.. It is difficult to set control in a proper time as
we're not sure when the proper time is.
We also need to think about the effect to the proceeding frames that
are being processed.

Thanks,
-Hiro
> We're going to need to be able to set other controls at this point,
> perhaps from the IPA too, so I expect we'll see some further rework here
> later.
>
> > +               Request *request = processingRequests_.front();
> > +               processingRequests_.pop();
> > +
> > +               int ret = cio2_.sensor()->applyRequestControls(request);
> > +               if (ret)
> > +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> > +       }
> > +
> > +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> > +       delayedCtrls_->applyControls(sequence);
> > +}
> > +
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> >
> >  } /* namespace libcamera */
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
Kieran Bingham Nov. 9, 2021, 11:54 a.m. UTC | #3 
Quoting Hirokazu Honda (2021-11-09 06:39:43)
> Hi Kieran,
> 
> On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Hirokazu Honda (2021-11-04 06:42:52)
> > > This enables ipu3 pipeline handler to apply the test pattern mode
> > > per frame.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 63cb7f11..cb141ba9 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -59,6 +59,7 @@ public:
> > >         void statBufferReady(FrameBuffer *buffer);
> > >         void queuePendingRequests();
> > >         void cancelPendingRequests();
> > > +       void frameStart(uint32_t sequence);
> > >
> > >         CIO2Device cio2_;
> > >         ImgUDevice *imgu_;
> > > @@ -78,6 +79,7 @@ public:
> > >         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> > >
> > >         std::queue<Request *> pendingRequests_;
> > > +       std::queue<Request *> processingRequests_;
> >
> > I'm a bit weary of having requests on multiple queues like this.
> > It might be better if we could add some documentation describing the
> > purpose of each queue, and what lifetime a request has on each one.
> >
> >
> > Perhaps a banner comment before processingRequests_ to describe briefly
> > that the requests will sit on this queue to synchronise setting controls
> > at frameStart time? I'm not sure what level will help - but it seems a
> > bit vague for 'processingRequests' and 'pendingRequests' otherwise.
> >
> >
> >
> > >
> > >         ControlInfoMap ipaControls_;
> > >
> > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >         cio2->sensor()->sensorInfo(&sensorInfo);
> > >         data->cropRegion_ = sensorInfo.analogCrop;
> > >
> > > +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> > > +
> >
> > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())
> >
> >
> > >         /*
> > >          * Configure the H/V flip controls based on the combination of
> > >          * the sensor and user transform.
> > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > >         int ret = 0;
> > >
> > >         data->cancelPendingRequests();
> > > +       data->processingRequests_ = {};
> >
> > We add requests to the processingRequests_ during
> > queuePendingRequests(), so maybe we should clear them during
> > cancelPendingRequests()?
> >
> > >         data->ipa_->stop();
> > >
> > >         ret |= data->imgu_->stop();
> > >         ret |= data->cio2_.stop();
> > > +
> > >         if (ret)
> > >                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> > >
> > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
> > >
> > >                 info->rawBuffer = rawBuffer;
> > >
> > > +               processingRequests_.push(request);
> > > +
> > >                 ipa::ipu3::IPU3Event ev;
> > >                 ev.op = ipa::ipu3::EventProcessControls;
> > >                 ev.frame = info->id;
> > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()
> > >                 data->delayedCtrls_ =
> > >                         std::make_unique<DelayedControls>(cio2->sensor()->device(),
> > >                                                           params);
> > > -               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > > -                                                &DelayedControls::applyControls);
> > > +               data->cio2_.frameStart().connect(data.get(),
> > > +                                                &IPU3CameraData::frameStart);
> >
> > I like having an explicit/clear frameStart event function here.
> >
> 
> Sorry, I don't get this comment. Could you elaborate?

I meant that I think this is good ;-)

It makes an explicit call on the IPU3CameraData when there is a
frameStart event, rather than the short cut to delayed controls..

> > >
> > >                 /* Convert the sensor rotation to a transformation */
> > >                 int32_t rotation = 0;
> > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > >         ipa_->processEvent(ev);
> > >  }
> > >
> > > +void IPU3CameraData::frameStart(uint32_t sequence)
> > > +{
> > > +       if (!processingRequests_.empty()) {
> > > +               /* Assumes applying the test pattern mode affects immediately. */
> >
> > That comment seems misplaced now.
> > Perhaps you could add:
> >
> >                   /*
> >                    * Handle controls which are to be set ready for the
> >                    * next frame to start.
> >                    */
> >
> > But I'm not sure of the race/sequencing here. I presume at the point we
> > have a frameStart event, any controls we apply will take effect for the
> > /next/ frame ? This frame has already started...
> >
> > Or do we expect to set a control here and have it take effect on this
> > frame? (That sounds way too racy to be possible).
> >
> 
> That's good point.. It is difficult to set control in a proper time as
> we're not sure when the proper time is.
> We also need to think about the effect to the proceeding frames that
> are being processed.
> 
> Thanks,
> -Hiro
> > We're going to need to be able to set other controls at this point,
> > perhaps from the IPA too, so I expect we'll see some further rework here
> > later.
> >
> > > +               Request *request = processingRequests_.front();
> > > +               processingRequests_.pop();
> > > +
> > > +               int ret = cio2_.sensor()->applyRequestControls(request);

Talking with Jacopo, he really dislikes passing the request to the
sensor ;-( - While I dislike having to filter out the test pattern
control (or CameraSensor specific controls) in the frameStart event for
each pipeline handler.

Not sure what the middle ground is yet. But it might just be renaming
applyRequestControls() to applyTestPatternControls() or something that
can't be suggested that it might be used by other controls. Perhaps
that's closer to your original implementation anyway ...


> > > +               if (ret)
> > > +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> > > +       }
> > > +
> > > +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> > > +       delayedCtrls_->applyControls(sequence);
> > > +}
> > > +
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> > >
> > >  } /* namespace libcamera */
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
Hirokazu Honda Nov. 10, 2021, 3:18 a.m. UTC | #4 
Hi Kieran,

On Tue, Nov 9, 2021 at 8:54 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Hirokazu Honda (2021-11-09 06:39:43)
> > Hi Kieran,
> >
> > On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Hirokazu Honda (2021-11-04 06:42:52)
> > > > This enables ipu3 pipeline handler to apply the test pattern mode
> > > > per frame.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
> > > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 63cb7f11..cb141ba9 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -59,6 +59,7 @@ public:
> > > >         void statBufferReady(FrameBuffer *buffer);
> > > >         void queuePendingRequests();
> > > >         void cancelPendingRequests();
> > > > +       void frameStart(uint32_t sequence);
> > > >
> > > >         CIO2Device cio2_;
> > > >         ImgUDevice *imgu_;
> > > > @@ -78,6 +79,7 @@ public:
> > > >         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> > > >
> > > >         std::queue<Request *> pendingRequests_;
> > > > +       std::queue<Request *> processingRequests_;
> > >
> > > I'm a bit weary of having requests on multiple queues like this.
> > > It might be better if we could add some documentation describing the
> > > purpose of each queue, and what lifetime a request has on each one.
> > >
> > >
> > > Perhaps a banner comment before processingRequests_ to describe briefly
> > > that the requests will sit on this queue to synchronise setting controls
> > > at frameStart time? I'm not sure what level will help - but it seems a
> > > bit vague for 'processingRequests' and 'pendingRequests' otherwise.
> > >
> > >
> > >
> > > >
> > > >         ControlInfoMap ipaControls_;
> > > >
> > > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > > >         cio2->sensor()->sensorInfo(&sensorInfo);
> > > >         data->cropRegion_ = sensorInfo.analogCrop;
> > > >
> > > > +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> > > > +
> > >
> > > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())
> > >
> > >
> > > >         /*
> > > >          * Configure the H/V flip controls based on the combination of
> > > >          * the sensor and user transform.
> > > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > >         int ret = 0;
> > > >
> > > >         data->cancelPendingRequests();
> > > > +       data->processingRequests_ = {};
> > >
> > > We add requests to the processingRequests_ during
> > > queuePendingRequests(), so maybe we should clear them during
> > > cancelPendingRequests()?
> > >
> > > >         data->ipa_->stop();
> > > >
> > > >         ret |= data->imgu_->stop();
> > > >         ret |= data->cio2_.stop();
> > > > +
> > > >         if (ret)
> > > >                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> > > >
> > > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
> > > >
> > > >                 info->rawBuffer = rawBuffer;
> > > >
> > > > +               processingRequests_.push(request);
> > > > +
> > > >                 ipa::ipu3::IPU3Event ev;
> > > >                 ev.op = ipa::ipu3::EventProcessControls;
> > > >                 ev.frame = info->id;
> > > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()
> > > >                 data->delayedCtrls_ =
> > > >                         std::make_unique<DelayedControls>(cio2->sensor()->device(),
> > > >                                                           params);
> > > > -               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > > > -                                                &DelayedControls::applyControls);
> > > > +               data->cio2_.frameStart().connect(data.get(),
> > > > +                                                &IPU3CameraData::frameStart);
> > >
> > > I like having an explicit/clear frameStart event function here.
> > >
> >
> > Sorry, I don't get this comment. Could you elaborate?
>
> I meant that I think this is good ;-)
>
> It makes an explicit call on the IPU3CameraData when there is a
> frameStart event, rather than the short cut to delayed controls..
>
> > > >
> > > >                 /* Convert the sensor rotation to a transformation */
> > > >                 int32_t rotation = 0;
> > > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > > >         ipa_->processEvent(ev);
> > > >  }
> > > >
> > > > +void IPU3CameraData::frameStart(uint32_t sequence)
> > > > +{
> > > > +       if (!processingRequests_.empty()) {
> > > > +               /* Assumes applying the test pattern mode affects immediately. */
> > >
> > > That comment seems misplaced now.
> > > Perhaps you could add:
> > >
> > >                   /*
> > >                    * Handle controls which are to be set ready for the
> > >                    * next frame to start.
> > >                    */
> > >
> > > But I'm not sure of the race/sequencing here. I presume at the point we
> > > have a frameStart event, any controls we apply will take effect for the
> > > /next/ frame ? This frame has already started...
> > >
> > > Or do we expect to set a control here and have it take effect on this
> > > frame? (That sounds way too racy to be possible).
> > >
> >
> > That's good point.. It is difficult to set control in a proper time as
> > we're not sure when the proper time is.
> > We also need to think about the effect to the proceeding frames that
> > are being processed.
> >
> > Thanks,
> > -Hiro
> > > We're going to need to be able to set other controls at this point,
> > > perhaps from the IPA too, so I expect we'll see some further rework here
> > > later.
> > >
> > > > +               Request *request = processingRequests_.front();
> > > > +               processingRequests_.pop();
> > > > +
> > > > +               int ret = cio2_.sensor()->applyRequestControls(request);
>
> Talking with Jacopo, he really dislikes passing the request to the
> sensor ;-( - While I dislike having to filter out the test pattern
> control (or CameraSensor specific controls) in the frameStart event for
> each pipeline handler.
>
> Not sure what the middle ground is yet. But it might just be renaming
> applyRequestControls() to applyTestPatternControls() or something that
> can't be suggested that it might be used by other controls. Perhaps
> that's closer to your original implementation anyway ...
>

I will ask Laurent about this today.

-Hiro
>
> > > > +               if (ret)
> > > > +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> > > > +       }
> > > > +
> > > > +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> > > > +       delayedCtrls_->applyControls(sequence);
> > > > +}
> > > > +
> > > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> > > >
> > > >  } /* namespace libcamera */
> > > > --
> > > > 2.33.1.1089.g2158813163f-goog
> > > >
Laurent Pinchart Nov. 22, 2021, 2:13 a.m. UTC | #5 
Hi Hiro,

On Wed, Nov 10, 2021 at 12:18:25PM +0900, Hirokazu Honda wrote:
> On Tue, Nov 9, 2021 at 8:54 PM Kieran Bingham wrote:
> > Quoting Hirokazu Honda (2021-11-09 06:39:43)
> > > On Tue, Nov 9, 2021 at 7:16 AM Kieran Bingham wrote:
> > > > Quoting Hirokazu Honda (2021-11-04 06:42:52)
> > > > > This enables ipu3 pipeline handler to apply the test pattern mode
> > > > > per frame.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > > > ---
> > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++--
> > > > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > index 63cb7f11..cb141ba9 100644
> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > @@ -59,6 +59,7 @@ public:
> > > > >         void statBufferReady(FrameBuffer *buffer);
> > > > >         void queuePendingRequests();
> > > > >         void cancelPendingRequests();
> > > > > +       void frameStart(uint32_t sequence);
> > > > >
> > > > >         CIO2Device cio2_;
> > > > >         ImgUDevice *imgu_;
> > > > > @@ -78,6 +79,7 @@ public:
> > > > >         std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> > > > >
> > > > >         std::queue<Request *> pendingRequests_;
> > > > > +       std::queue<Request *> processingRequests_;
> > > >
> > > > I'm a bit weary of having requests on multiple queues like this.
> > > > It might be better if we could add some documentation describing the
> > > > purpose of each queue, and what lifetime a request has on each one.
> > > >
> > > >
> > > > Perhaps a banner comment before processingRequests_ to describe briefly
> > > > that the requests will sit on this queue to synchronise setting controls
> > > > at frameStart time? I'm not sure what level will help - but it seems a
> > > > bit vague for 'processingRequests' and 'pendingRequests' otherwise.
> > > >
> > > >
> > > >
> > > > >
> > > > >         ControlInfoMap ipaControls_;
> > > > >
> > > > > @@ -569,6 +571,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > > > >         cio2->sensor()->sensorInfo(&sensorInfo);
> > > > >         data->cropRegion_ = sensorInfo.analogCrop;
> > > > >
> > > > > +       cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
> > > > > +
> > > >
> > > > This looks like something the CameraSensor should do at the end of initialisation. (CameraSensor::init())
> > > >
> > > >
> > > > >         /*
> > > > >          * Configure the H/V flip controls based on the combination of
> > > > >          * the sensor and user transform.
> > > > > @@ -797,11 +801,13 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > > >         int ret = 0;
> > > > >
> > > > >         data->cancelPendingRequests();
> > > > > +       data->processingRequests_ = {};
> > > >
> > > > We add requests to the processingRequests_ during
> > > > queuePendingRequests(), so maybe we should clear them during
> > > > cancelPendingRequests()?
> > > >
> > > > >         data->ipa_->stop();
> > > > >
> > > > >         ret |= data->imgu_->stop();
> > > > >         ret |= data->cio2_.stop();
> > > > > +
> > > > >         if (ret)
> > > > >                 LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> > > > >
> > > > > @@ -852,6 +858,8 @@ void IPU3CameraData::queuePendingRequests()
> > > > >
> > > > >                 info->rawBuffer = rawBuffer;
> > > > >
> > > > > +               processingRequests_.push(request);
> > > > > +
> > > > >                 ipa::ipu3::IPU3Event ev;
> > > > >                 ev.op = ipa::ipu3::EventProcessControls;
> > > > >                 ev.frame = info->id;
> > > > > @@ -1131,8 +1139,8 @@ int PipelineHandlerIPU3::registerCameras()
> > > > >                 data->delayedCtrls_ =
> > > > >                         std::make_unique<DelayedControls>(cio2->sensor()->device(),
> > > > >                                                           params);
> > > > > -               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > > > > -                                                &DelayedControls::applyControls);
> > > > > +               data->cio2_.frameStart().connect(data.get(),
> > > > > +                                                &IPU3CameraData::frameStart);
> > > >
> > > > I like having an explicit/clear frameStart event function here.
> > > >
> > >
> > > Sorry, I don't get this comment. Could you elaborate?
> >
> > I meant that I think this is good ;-)
> >
> > It makes an explicit call on the IPU3CameraData when there is a
> > frameStart event, rather than the short cut to delayed controls..
> >
> > > > >
> > > > >                 /* Convert the sensor rotation to a transformation */
> > > > >                 int32_t rotation = 0;
> > > > > @@ -1423,6 +1431,22 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > > > >         ipa_->processEvent(ev);
> > > > >  }
> > > > >
> > > > > +void IPU3CameraData::frameStart(uint32_t sequence)
> > > > > +{
> > > > > +       if (!processingRequests_.empty()) {
> > > > > +               /* Assumes applying the test pattern mode affects immediately. */
> > > >
> > > > That comment seems misplaced now.
> > > > Perhaps you could add:
> > > >
> > > >                   /*
> > > >                    * Handle controls which are to be set ready for the
> > > >                    * next frame to start.
> > > >                    */
> > > >
> > > > But I'm not sure of the race/sequencing here. I presume at the point we
> > > > have a frameStart event, any controls we apply will take effect for the
> > > > /next/ frame ? This frame has already started...
> > > >
> > > > Or do we expect to set a control here and have it take effect on this
> > > > frame? (That sounds way too racy to be possible).
> > > >
> > >
> > > That's good point.. It is difficult to set control in a proper time as
> > > we're not sure when the proper time is.
> > > We also need to think about the effect to the proceeding frames that
> > > are being processed.
> > >
> > > Thanks,
> > > -Hiro
> > > > We're going to need to be able to set other controls at this point,
> > > > perhaps from the IPA too, so I expect we'll see some further rework here
> > > > later.
> > > >
> > > > > +               Request *request = processingRequests_.front();
> > > > > +               processingRequests_.pop();
> > > > > +
> > > > > +               int ret = cio2_.sensor()->applyRequestControls(request);
> >
> > Talking with Jacopo, he really dislikes passing the request to the
> > sensor ;-( - While I dislike having to filter out the test pattern
> > control (or CameraSensor specific controls) in the frameStart event for
> > each pipeline handler.
> >
> > Not sure what the middle ground is yet. But it might just be renaming
> > applyRequestControls() to applyTestPatternControls() or something that
> > can't be suggested that it might be used by other controls. Perhaps
> > that's closer to your original implementation anyway ...
> 
> I will ask Laurent about this today.

I've replied to this in patch 2/3.

> > > > > +               if (ret)
> > > > > +                       LOG(IPU3, Error) << "Failed applying controls: " << ret;
> > > > > +       }
> > > > > +
> > > > > +       /* Controls that don't affect immediately are applied in delayedCtrls. */
> > > > > +       delayedCtrls_->applyControls(sequence);
> > > > > +}
> > > > > +
> > > > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> > > > >
> > > > >  } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 63cb7f11..cb141ba9 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -59,6 +59,7 @@  public:
 	void statBufferReady(FrameBuffer *buffer);
 	void queuePendingRequests();
 	void cancelPendingRequests();
+	void frameStart(uint32_t sequence);
 
 	CIO2Device cio2_;
 	ImgUDevice *imgu_;
@@ -78,6 +79,7 @@  public:
 	std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
 
 	std::queue<Request *> pendingRequests_;
+	std::queue<Request *> processingRequests_;
 
 	ControlInfoMap ipaControls_;
 
@@ -569,6 +571,8 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 	cio2->sensor()->sensorInfo(&sensorInfo);
 	data->cropRegion_ = sensorInfo.analogCrop;
 
+	cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff);
+
 	/*
 	 * Configure the H/V flip controls based on the combination of
 	 * the sensor and user transform.
@@ -797,11 +801,13 @@  void PipelineHandlerIPU3::stop(Camera *camera)
 	int ret = 0;
 
 	data->cancelPendingRequests();
+	data->processingRequests_ = {};
 
 	data->ipa_->stop();
 
 	ret |= data->imgu_->stop();
 	ret |= data->cio2_.stop();
+
 	if (ret)
 		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
 
@@ -852,6 +858,8 @@  void IPU3CameraData::queuePendingRequests()
 
 		info->rawBuffer = rawBuffer;
 
+		processingRequests_.push(request);
+
 		ipa::ipu3::IPU3Event ev;
 		ev.op = ipa::ipu3::EventProcessControls;
 		ev.frame = info->id;
@@ -1131,8 +1139,8 @@  int PipelineHandlerIPU3::registerCameras()
 		data->delayedCtrls_ =
 			std::make_unique<DelayedControls>(cio2->sensor()->device(),
 							  params);
-		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
-						 &DelayedControls::applyControls);
+		data->cio2_.frameStart().connect(data.get(),
+						 &IPU3CameraData::frameStart);
 
 		/* Convert the sensor rotation to a transformation */
 		int32_t rotation = 0;
@@ -1423,6 +1431,22 @@  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
 	ipa_->processEvent(ev);
 }
 
+void IPU3CameraData::frameStart(uint32_t sequence)
+{
+	if (!processingRequests_.empty()) {
+		/* Assumes applying the test pattern mode affects immediately. */
+		Request *request = processingRequests_.front();
+		processingRequests_.pop();
+
+		int ret = cio2_.sensor()->applyRequestControls(request);
+		if (ret)
+			LOG(IPU3, Error) << "Failed applying controls: " << ret;
+	}
+
+	/* Controls that don't affect immediately are applied in delayedCtrls. */
+	delayedCtrls_->applyControls(sequence);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
 
 } /* namespace libcamera */