[libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add control of sensor vblanking

[Jacopo Mondi]

Hi Naush,

On Tue, Jan 19, 2021 at 07:45:54PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>

[snip]

> > > >
> > > >
> > > > Sorry for the many questions. The only thing that worries me is the
> > > > ControlInfo intialization, but that's not something you should address
> > > > at the moment. Other than that with the metadata and default value
> > > > assignement clarified, you can retain my tag.
> > > >
> > >
> > > Not a problem.  Good to ensure things are right by everyone.  Please let
> > me
> > > know your thoughts on my reply, there may be more to discuss before
> > > submitting is.
> >
> > Yeah, good to be on the same page but sorry for having asked the same
> > questions again.
> >
> > Let's get this merged soon so we can close all points highlighted here
> > in follow-up patches..
> >
> >
> Thanks!  So would you be ok if I leave this v12 series as-is and pending
> feedback comments from Laurent, merge this version if he raises no other
> issues?  I note that there is a minor nit on standardising the ordering of
> member variables  in the base and derived classes, but I will fix that up
> in my next series.  Hope that is ok.

Yeah, it's ok no worries, no need to resend

I hope we can this in quickly, as I need this control as well to be
able to report to the Camera HAL the frame duration limits.

Thanks
  j


>
> Regards,
> Naush
>
>
>
> Thanks
> >    j
> >
> > >
> > > Regards,
> > > Naush
> > >
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > > +
> >  libcameraMetadata_.set(controls::FrameDurations,
> > > > > +                                            {
> > > > static_cast<int64_t>(minFrameDuration_),
> > > > > +
> > > > static_cast<int64_t>(maxFrameDuration_) });
> > > > > +                     break;
> > > > > +             }
> > > > > +
> > > > >               default:
> > > > >                       LOG(IPARPI, Warning)
> > > > >                               << "Ctrl " << controls::controls.at
> > > > (ctrl.first)->name()
> > > > > @@ -962,15 +992,27 @@ void IPARPi::applyAWB(const struct AwbStatus
> > > > *awbStatus, ControlList &ctrls)
> > > > >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList
> > > > &ctrls)
> > > > >  {
> > > > >       int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > > > > -     int32_t exposureLines =
> > > > helper_->ExposureLines(agcStatus->shutter_time);
> > > > >
> > > > > -     LOG(IPARPI, Debug) << "Applying AGC Exposure: " <<
> > > > agcStatus->shutter_time
> > > > > -                        << " (Shutter lines: " << exposureLines <<
> > ")
> > > > Gain: "
> > > > > +     /* GetVBlanking might clip exposure time to the fps limits. */
> > > > > +     double exposure = agcStatus->shutter_time;
> > > > > +     int32_t vblanking = helper_->GetVBlanking(exposure,
> > > > minFrameDuration_,
> > > > > +                                               maxFrameDuration_);
> > > > > +     int32_t exposureLines = helper_->ExposureLines(exposure);
> > > > > +
> > > > > +     LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > > > > +                        << " (Shutter lines: " << exposureLines <<
> > ",
> > > > AGC requested "
> > > > > +                        << agcStatus->shutter_time << ") Gain: "
> > > > >                          << agcStatus->analogue_gain << " (Gain
> > Code: "
> > > > >                          << gainCode << ")";
> > > > >
> > > > > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > > > > +     /*
> > > > > +      * Due to the behavior of V4L2, the current value of VBLANK
> > could
> > > > clip the
> > > > > +      * exposure time without us knowing. The next time though this
> > > > function should
> > > > > +      * clip exposure correctly.
> > > > > +      */
> > > > > +     ctrls.set(V4L2_CID_VBLANK, vblanking);
> > > > >       ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> > > > > +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > > > >  }
> > > > >
> > > > >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList
> > > > &ctrls)
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index 7a5f5881b9b3..252cab64023e 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const
> > > > CameraConfiguration *config)
> > > > >               if (!staggeredCtrl_) {
> > > > >
> >  staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > > > >                                           { { V4L2_CID_ANALOGUE_GAIN,
> > > > result.data[resultIdx++] },
> > > > > -                                           { V4L2_CID_EXPOSURE,
> > > > result.data[resultIdx++] } });
> > > > > +                                           { V4L2_CID_EXPOSURE,
> > > > result.data[resultIdx++] },
> > > > > +                                           { V4L2_CID_VBLANK,
> > > > result.data[resultIdx++] } });
> > > > >                       sensorMetadata_ = result.data[resultIdx++];
> > > > >               }
> > > > >       }
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> >