[libcamera-devel] [PATCH v2] ipa: rpi: imx708: Fix mode switch drop frame count

[Naushir Patuck]

On Thu, 6 Jul 2023 at 13:53, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Thu, Jul 06, 2023 at 01:19:29PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2023-07-06 13:09:41)
> > > On Thu, Jul 06, 2023 at 12:20:58PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > > Quoting Naushir Patuck via libcamera-devel (2023-06-27 14:05:53)
> > > > > The imx708 must drop a single frame on startup - but only when in HDR
> > > > > mode. Non-HDR modes do not need to drop frames. Fix the logic in
> > > > > hideFramesModeSwitch() which currently unconditionally advertises to
> > > > > drop one frame.
> > > > >
> > > > > Unfortunately there is no clear way to tell if the sensor is in the HDR
> > > > > mode. So for now, look the resolution and framerate to deduce this.
> > > > >
> > > > > Additionally ensure we override hideFramesStartup() and return the same
> > > > > number as hideFramesModeSwitch().
> > > > >
> > > > > Bug: https://github.com/raspberrypi/libcamera-apps/issues/524
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > ---
> > > > > Changes since v1:
> > > > > - Fix a typo in the comparison statement.
> > > > > ---
> > > > >  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 25 +++++++++++++++++++-
> > > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > index 641ba18f4b9d..9bc0272dd4c1 100644
> > > > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> > > > > @@ -21,6 +21,8 @@ using namespace RPiController;
> > > > >  using namespace libcamera;
> > > > >  using libcamera::utils::Duration;
> > > > >
> > > > > +using namespace std::literals::chrono_literals;
> > > > > +
> > > > >  namespace libcamera {
> > > > >  LOG_DECLARE_CATEGORY(IPARPI)
> > > > >  }
> > > > > @@ -56,7 +58,8 @@ public:
> > > > >                        int &vblankDelay, int &hblankDelay) const override;
> > > > >         bool sensorEmbeddedDataPresent() const override;
> > > > >         double getModeSensitivity(const CameraMode &mode) const override;
> > > > > -       unsigned int hideFramesModeSwitch() const override { return 1; } // seems to be required for HDR
> > > > > +       unsigned int hideFramesModeSwitch() const override;
> > > > > +       unsigned int hideFramesStartup() const;
> > > > >
> > > > >  private:
> > > > >         /*
> > > > > @@ -225,6 +228,26 @@ double CamHelperImx708::getModeSensitivity(const CameraMode &mode) const
> > > > >         return (mode.width > 2304) ? 1.0 : 2.0;
> > > > >  }
> > > > >
> > > > > +unsigned int CamHelperImx708::hideFramesModeSwitch() const
> > > > > +{
> > > > > +       /*
> > > > > +        * We need to drop the first startup frame in HDR mode.
> > > > > +        * Unfortunately the only way to currently determine if the sensor is in
> > > > > +        * the HDR mode is to match with the resolution and framerate - the HDR
> > > > > +        * mode only runs upto 30fps.
> > > >
> > > > Perhaps we should add a
> > > >
> > > >        * \todo: Revisit this when the HDR mode is handled by
> > > >        * libcamera.
> > >
> > > How will this issue be handled then ?
> >
> > I don't know, that would be the action of the todo - but are you saying
> > "No don't ever bother revisiting this?"
>
> It was a question for Naush or David, I'd like to know if we have any
> idea on how we could handle this in the future, or if it's completely
> unknown.

IMO sensor HDR would need to become part of the V4L2 API.

This might mean HDR is signalled as either a new pixel format (not
very neat), or a control value (mostly like today, also not very
neat).

In any hypothetical rework of the kernel sensor API, HDR modes ought
to become a part of the format/mode information, possibly as a
decoupled/modifier field?

Regards,
Naush



>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > > +        */
> > > > > +       if (mode_.width == 2304 && mode_.height == 1296 &&
> > > > > +           mode_.minFrameDuration > 1.0s / 32)
> > > > > +               return 1;
> > > > > +       else
> > > > > +               return 0;
> > > > > +}
> > > > > +
> > > > > +unsigned int CamHelperImx708::hideFramesStartup() const
> > > > > +{
> > > > > +       return hideFramesModeSwitch();
> > > > > +}
> > > > > +
> > > > >  void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
> > > > >                                        Metadata &metadata) const
> > > > >  {
>
> --
> Regards,
>
> Laurent Pinchart