[PATCH] [RFC] libcamera: pipeline: Add h/v blanking delays

[Paul Elder]

Quoting Kieran Bingham (2025-05-14 00:02:23)
> Quoting Paul Elder (2025-05-13 20:07:21)
> > Quoting Kieran Bingham (2025-05-13 19:43:12)
> > > The HBLANK and VBLANK control delays are not being accounted for in all
> > > of the libipa platforms. Update their delayed controls instantiation
> > > accordingly.
> > 
> > I thought hblank was a generally a read-only control?
> 
> Generally perhaps - but not always - it can be a read-only or a
> writeable control for devices that can set it. So we should consider it
> when possible. It will give a better configuration range on exposure and
> frame duration if we have sensors with hblank controls.
> 
> But yes, some sensors are read only here.
> 
> > > In particular, update the RKISP1 VBLANK control delay to be marked as a
> > > priority control as well as setting the correct delay to align with the
> > > existing Raspberry Pi implementation.
> > 
> > Given that the priorityWrite docs says 
> > 
> >  * Typically set for the \a V4L2_CID_VBLANK control so that the device driver
> >  * does not reject \a V4L2_CID_EXPOSURE control values that may be outside of
> >  * the existing vertical blanking specified bounds, but are within the new
> >  * blanking bounds.
> > 
> > I suppose we do want this.
> > 
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > 
> > > Marking this as RFC because I haven't tested this or explored it yet -
> > > but as I was reviewing other parts of the code base - I noted that these
> > > are inconsistent across pipeline handlers - so lets figure out why and
> > > make sure all the pipeline handlers operate in the same way where
> > > possible.
> > > 
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp         | 2 ++
> > >  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 2 ++
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-
> > >  src/libcamera/pipeline/simple/simple.cpp     | 2 ++
> > >  4 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index e31e3879dcc9..4cec22b01940 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1082,6 +1082,8 @@ int PipelineHandlerIPU3::registerCameras()
> > >                 std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > >                         { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> > >                         { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
> > > +                       { V4L2_CID_HBLANK, { delays.hblankDelay, false } },
> > > +                       { V4L2_CID_VBLANK, { delays.vblankDelay, true } },
> > >                 };
> > >  
> > >                 data->delayedCtrls_ =
> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > index a05e11fccf8d..7d052263b34d 100644
> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > @@ -1607,6 +1607,8 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)
> > >                 std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > >                         { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> > >                         { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
> > > +                       { V4L2_CID_HBLANK, { delays.hblankDelay, false } },
> > > +                       { V4L2_CID_VBLANK, { delays.vblankDelay, true } },
> > >                 };
> > >  
> > >                 data->delayedCtrls_ =
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 52633fe3cb85..6c5b81b9a2ba 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -1325,7 +1325,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >         std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > >                 { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> > >                 { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
> > > -               { V4L2_CID_VBLANK, { 1, false } },
> > 
> > (This probably won't apply upstream as it was recently fixed to
> > { V4L2_CID_VBLANK, { delays.vblankDelay, false } }, )
> > 
> 
> Ooops - I hadn't realised I wasn't on a mainline branch...
> 
> So I'll rebase and update - but seems like it is the right thing to do
> for each pipeline ... which makes me wonder if we should just put a
> helper somewhere common (V4L2Device given it's V4L2Specific? or pipeline
> handler base, or common utils ?) that can parse the delayed controls
> delays in the same way for each...

Given that it's guaranteed to be duplicated between all IPAs, it might
indeed be a good idea to move it somewhere else common. Athough it's
slightly not the focus of this patch, might we also want to do the same
with exposure and gain?


Paul

> 
> > 
> > > +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },
> > > +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } },
> > >         };
> > >  
> > >         data->delayedCtrls_ =
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index efb07051b175..fc88efc7a716 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -546,6 +546,8 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > >         std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > >                 { V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
> > >                 { V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
> > > +               { V4L2_CID_HBLANK, { delays.hblankDelay, false } },
> > > +               { V4L2_CID_VBLANK, { delays.vblankDelay, true } },
> > >         };
> > >         delayedCtrls_ = std::make_unique<DelayedControls>(sensor_->device(), params);
> > >  
> > > -- 
> > > 2.49.0
> > >