[libcamera-devel] [PATCH] rkisp1: Add support for sensor test pattern control

[Laurent Pinchart]

Hi Kieran,

On Thu, Oct 20, 2022 at 09:52:28AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-10-20 00:25:33)
> > On Tue, Oct 18, 2022 at 05:34:38PM +0900, Paul Elder via libcamera-devel wrote:
> > > Add support for the TestPatternMode control.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp                |  7 +++++
> > 
> > For IPU3, we implements TPG support in the pipeline handler, without
> > involving the IPA module (see commit acf8d028edda "libcamera: pipeline:
> > ipu3: Apply a requested test pattern mode"). Is there a reason to do
> > differently here ?
> 
> I actually suspect we might be better going through the IPA, as I recall
> someone reported that if we enable the TPG on IPU3, then the AE/AGC can
> end up breaking... So we might need to let the IPA know that the gains
> applied will not correlate to the statistics of the frame received.

I think the IPA module needs to be informed, yes (hopefully no sensor
will reflect the exposure time and analog gain settings in their test
pattern output, so we'll be able to take the TPG into account in
algorithms and have consistent behaviour across all sensors).

> It would also mean we can align how we handle per-frame control for
> 'which' frame to apply the test-pattern on, along with other sensor
> controls.

I'm not sure I want to allow the TPG to be controllable on a per-frame
basis though. That would make it quite complicated for AGC. If the TPG
settings have to be constant for the whole capture session, we would be
able to simply disable AGC.

> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 +++++++++++++++++++++++-
> > >  2 files changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 3f5c1a58..1aac8884 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -183,6 +183,13 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > >  
> > >       /* Return the controls handled by the IPA. */
> > >       ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > > +
> > > +     auto tpgInfo = sensorControls.find(V4L2_CID_TEST_PATTERN);
> > > +     if (tpgInfo != sensorControls.end()) {
> > > +             ctrlMap.emplace(&controls::draft::TestPatternMode,
> > > +                             ControlInfo(tpgInfo->second.values()));
> > > +     }
> > > +
> > >       *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > >  
> > >       return 0;
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 455ee2a0..42803266 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -93,6 +93,8 @@ public:
> > >       PipelineHandlerRkISP1 *pipe();
> > >       int loadIPA(unsigned int hwRevision);
> > >  
> > > +     int setSensorTestPattern(const ControlList *controls);
> > > +
> > >       Stream mainPathStream_;
> > >       Stream selfPathStream_;
> > >       std::unique_ptr<CameraSensor> sensor_;
> > > @@ -104,6 +106,8 @@ public:
> > >       RkISP1MainPath *mainPath_;
> > >       RkISP1SelfPath *selfPath_;
> > >  
> > > +     controls::draft::TestPatternModeEnum testPatternMode_;
> > > +
> > >       std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
> > >  
> > >  private:
> > > @@ -357,6 +361,30 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > >       return 0;
> > >  }
> > >  
> > > +int RkISP1CameraData::setSensorTestPattern(const ControlList *controls)
> > > +{
> > > +     if (!controls)
> > > +             return 0;
> > > +
> > > +     const auto &testPattern = controls->get(controls::draft::TestPatternMode);
> > > +
> > > +     if (!testPattern)
> > > +             return 0;
> > > +
> > > +     if (testPattern && *testPattern == testPatternMode_)
> > > +             return 0;
> > > +
> > > +     testPatternMode_ = static_cast<controls::draft::TestPatternModeEnum>(*testPattern);
> > > +
> > > +     int ret = sensor_->setTestPatternMode(testPatternMode_);
> > > +     if (ret && !sensor_->testPatternModes().empty())
> > > +             return ret;
> > > +
> > > +     LOG(RkISP1, Debug) << "Set test pattern to " << static_cast<int>(testPatternMode_);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  void RkISP1CameraData::paramFilled(unsigned int frame)
> > >  {
> > >       PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
> > > @@ -820,11 +848,15 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> > >       return 0;
> > >  }
> > >  
> > > -int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> > > +int PipelineHandlerRkISP1::start(Camera *camera, const ControlList *controls)
> > >  {
> > >       RkISP1CameraData *data = cameraData(camera);
> > >       int ret;
> > >  
> > > +     ret = data->setSensorTestPattern(controls);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       /* Allocate buffers for internal pipeline usage. */
> > >       ret = allocateBuffers(camera);
> > >       if (ret)
> > > @@ -923,6 +955,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> > >  {
> > >       RkISP1CameraData *data = cameraData(camera);
> > >  
> > > +     int ret = data->setSensorTestPattern(&request->controls());
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
> > >       if (!info)
> > >               return -ENOENT;

-- 
Regards,

Laurent Pinchart