libcamera: rkisp1: Mark VBLANK as priority

24948 diff mbox series

The DelayedControls class works around a limitation of the V4L2 controls
API by assigning to controls that modify the limits of other controls a
'priority' flag.

'Priority' controls are written to the device before others to make sure
the limits of dependent controls are correctly updated.

A typical example of a priority control is VBLANK, whose value changes the
limits of the EXPOSURE control. This doesn't apply to a specific hardware
platform but to all V4L2 sensors.

The RkISP1 pipeline handler doesn't mark VBLANK as a priority control, an
issue which might result in the exposure being clamped to an outdated frame
length.

Fix the rkisp1 pipeline by marking VBLANK as a priority control.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
A simpler version of "libcamera: delayed_controls: Make VBLANK priority
by default" as automatically setting VBLANK would save pipeline handlers
from doing it, but if anyone has to support HBLANK, in example, it has
to modify DelayedControls, not sure what's better, so I'm sending both.
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: b1f09c013a01a82c739f0e30b71fd8d000ef5655
change-id: 20251102-rkisp1-vblank-ba2084646ba7

Best regards,

Hi Jacopo,

Thank you for addressing this so quickly.

On Sun, Nov 02, 2025 at 08:54:33PM +0100, Jacopo Mondi wrote:
> The DelayedControls class works around a limitation of the V4L2 controls
> API by assigning to controls that modify the limits of other controls a
> 'priority' flag.
> 
> 'Priority' controls are written to the device before others to make sure
> the limits of dependent controls are correctly updated.
> 
> A typical example of a priority control is VBLANK, whose value changes the
> limits of the EXPOSURE control. This doesn't apply to a specific hardware
> platform but to all V4L2 sensors.
> 
> The RkISP1 pipeline handler doesn't mark VBLANK as a priority control, an
> issue which might result in the exposure being clamped to an outdated frame
> length.
> 
> Fix the rkisp1 pipeline by marking VBLANK as a priority control.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> A simpler version of "libcamera: delayed_controls: Make VBLANK priority
> by default" as automatically setting VBLANK would save pipeline handlers
> from doing it, but if anyone has to support HBLANK, in example, it has
> to modify DelayedControls, not sure what's better, so I'm sending both.

I think I like this one better for the time being as it's less invasive.
We know we need to rework DelayedControls in the future.

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ecd13831539fdf5cb79da2ea4b33a353514328ae..d7195b6c484d091857a91de709e0e060810c7c32 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1348,7 +1348,7 @@ 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, { delays.vblankDelay, false } },
> +		{ V4L2_CID_VBLANK, { delays.vblankDelay, true } },
>  	};
>  
>  	data->delayedCtrls_ =
> 
> ---
> base-commit: b1f09c013a01a82c739f0e30b71fd8d000ef5655
> change-id: 20251102-rkisp1-vblank-ba2084646ba7

Hi

2025. 11. 02. 20:54 keltezéssel, Jacopo Mondi írta:
> The DelayedControls class works around a limitation of the V4L2 controls
> API by assigning to controls that modify the limits of other controls a
> 'priority' flag.
> 
> 'Priority' controls are written to the device before others to make sure
> the limits of dependent controls are correctly updated.
> 
> A typical example of a priority control is VBLANK, whose value changes the
> limits of the EXPOSURE control. This doesn't apply to a specific hardware
> platform but to all V4L2 sensors.
> 
> The RkISP1 pipeline handler doesn't mark VBLANK as a priority control, an
> issue which might result in the exposure being clamped to an outdated frame
> length.
> 
> Fix the rkisp1 pipeline by marking VBLANK as a priority control.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> A simpler version of "libcamera: delayed_controls: Make VBLANK priority
> by default" as automatically setting VBLANK would save pipeline handlers
> from doing it, but if anyone has to support HBLANK, in example, it has
> to modify DelayedControls, not sure what's better, so I'm sending both.
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ecd13831539fdf5cb79da2ea4b33a353514328ae..d7195b6c484d091857a91de709e0e060810c7c32 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1348,7 +1348,7 @@ 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, { delays.vblankDelay, false } },
> +		{ V4L2_CID_VBLANK, { delays.vblankDelay, true } },

Why was it not marked as "priority-write"? Was it simply an oversight in
f72c76eb6e06a41d2aaff8c8c4002dea21a7774d ("rkisp1: Honor the FrameDurationLimits control") ?

In any case, it appears OK to me.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   	};
>   
>   	data->delayedCtrls_ =
> 
> ---
> base-commit: b1f09c013a01a82c739f0e30b71fd8d000ef5655
> change-id: 20251102-rkisp1-vblank-ba2084646ba7
> 
> Best regards,