[libcamera-devel,v2,0/7] Raspberry Pi: ipa::init() restructuring
mbox series

Message ID 20210323122739.680560-1-naush@raspberrypi.com
Headers show
Series
  • Raspberry Pi: ipa::init() restructuring
Related show

Message

Naushir Patuck March 23, 2021, 12:27 p.m. UTC 
Hi,

Here is version 2 of this series.  I have applied all the changes suggested by
Laurent, except for the following in patch 2/7:

> > +     /*
> > +      * Setup our delayed control writer with the sensor default
> > +      * gain and exposure delays. Mark VBLANK for priority write.
> > +      */
> > +     std::unordered_map<uint32_t, DelayedControls::ControlParams>
> params = {
> > +             { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false
> } },
> > +             { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false }
> },
> > +             { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }
> > +     };
> > +     data->delayedCtrls_ =
> std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(),
> params);
> > +     data->sensorMetadata_ = sensorConfig.sensorMetadata;
> > +
>
> As this code deals with RPiCameraData only, could you move it to
> RPiCameraData::loadIPA() ? That way you won't have to prefix everything
> with data->, and you'll avoid passing sensorConfig to loadIPA().

As explained in the original reply, I put this block here instead of loadIPA()
as the DelayedControls helper needs the device node already opened in order to
validate the controls that are passed in via params.  If I were to move this
to loadIPA(),  it would split the opening of unicam_[Unicam::Image] and
unicam_[Unicam::Embedded] into two separate locations, which I thought
might be undesirable.

Regards,
Naush

Naushir Patuck (7):
  ipa: Add sensor model string to IPASettings
  pipeline: ipa: raspberrypi: Open the CamHelper on ipa::init()
  pipeline: raspberrypi: Conditionally open the embedded data node
  ipa: raspberrypi: Move the controller initialise to ipa::init()
  ipa: raspberrypi: Remove unused member variable
  ipa: raspberrypi: Rationalise parameters to ipa::start()
  ipa: raspberrypi: Rationalise parameters to ipa::configure()

 include/libcamera/ipa/core.mojom              |   8 ++
 include/libcamera/ipa/raspberrypi.mojom       |  27 ++--
 src/ipa/raspberrypi/raspberrypi.cpp           | 110 +++++++---------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |   3 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 124 +++++++-----------
 src/libcamera/pipeline/vimc/vimc.cpp          |   2 +-
 test/ipa/ipa_interface_test.cpp               |   2 +-
 7 files changed, 116 insertions(+), 160 deletions(-)