[RFC] libcamera: pipeline: Add h/v blanking delays
diff mbox series

Message ID 20250513174312.106676-1-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • [RFC] libcamera: pipeline: Add h/v blanking delays
Related show

Commit Message

Kieran Bingham May 13, 2025, 5:43 p.m. UTC 
The HBLANK and VBLANK control delays are not being accounted for in all
of the libipa platforms. Update their delayed controls instantiation
accordingly.

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.

Signed-off-by: Kieran Bingham <kieran.bingham@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(-)

Comments

Paul Elder May 13, 2025, 7:07 p.m. UTC | #1 
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?

> 
> 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@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 } }, )


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
>
Kieran Bingham May 13, 2025, 10:02 p.m. UTC | #2 
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@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...

> 
> 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
> >

Patch
diff mbox series 

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 } },
+		{ 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);