[v1,2/4] libcamera: v4l2_device: Map `BITMASK` type to `Unsigned32`
diff mbox series

Message ID 20260325092659.79453-2-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1,1/4] libcamera: v4l2_device: Ensure consistent control types
Related show

Commit Message

Barnabás Pőcze March 25, 2026, 9:26 a.m. UTC 
A bitmask is conceptually an unsigned integer, as implied by the documentation:

  The maximum value is interpreted as a __u32,
  allowing the use of bit 31 in the bitmask.

so map it to a 32-bit unsigned integer. Adjust the rkisp1 pipeline handler
accordingly.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---
 src/libcamera/v4l2_device.cpp            | 3 ++-
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi March 25, 2026, 4:43 p.m. UTC | #1 
Hi Barnabás

On Wed, Mar 25, 2026 at 10:26:57AM +0100, Barnabás Pőcze wrote:
> A bitmask is conceptually an unsigned integer, as implied by the documentation:
>
>   The maximum value is interpreted as a __u32,
>   allowing the use of bit 31 in the bitmask.
>
> so map it to a 32-bit unsigned integer. Adjust the rkisp1 pipeline handler
> accordingly.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---
>  src/libcamera/v4l2_device.cpp            | 3 ++-
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 320a4dc5a..c14b57322 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1479,9 +1479,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
>  		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
>  		if (!list.empty())
> -			supportedBlocks = static_cast<uint32_t>(
> -				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
> -					.get<int32_t>());
> +			supportedBlocks = list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS).get<uint32_t>();

This is not related to the below changes right ?

I mean, the cast wasn't necessary even without the below changes ?


>  	} else {
>  		LOG(RkISP1, Error)
>  			<< "Failed to query supported params blocks. Falling back to defaults.";
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index e0099cb7a..db1eb70e4 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -572,6 +572,7 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>  		return ControlTypeUnsigned16;
>
>  	case V4L2_CTRL_TYPE_U32:
> +	case V4L2_CTRL_TYPE_BITMASK:
>  		return ControlTypeUnsigned32;
>
>  	case V4L2_CTRL_TYPE_INTEGER:
> @@ -582,7 +583,6 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>
>  	case V4L2_CTRL_TYPE_MENU:
>  	case V4L2_CTRL_TYPE_BUTTON:
> -	case V4L2_CTRL_TYPE_BITMASK:

Becaue bitmask was deflected to the U32 case already, wasn't it ?

>  	case V4L2_CTRL_TYPE_INTEGER_MENU:
>  		/*
>  		 * More precise types may be needed, for now use a 32-bit
> @@ -636,6 +636,7 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
>  				   static_cast<uint16_t>(ctrl.default_value));
>
>  	case V4L2_CTRL_TYPE_U32:
> +	case V4L2_CTRL_TYPE_BITMASK:
>  		return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
>  				   static_cast<uint32_t>(ctrl.maximum),
>  				   static_cast<uint32_t>(ctrl.default_value));

Anyway, I don't have objections on making this more precise

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> --
> 2.53.0
>
Barnabás Pőcze March 27, 2026, 7:47 a.m. UTC | #2 
2026. 03. 25. 17:43 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Mar 25, 2026 at 10:26:57AM +0100, Barnabás Pőcze wrote:
>> A bitmask is conceptually an unsigned integer, as implied by the documentation:
>>
>>    The maximum value is interpreted as a __u32,
>>    allowing the use of bit 31 in the bitmask.
>>
>> so map it to a 32-bit unsigned integer. Adjust the rkisp1 pipeline handler
>> accordingly.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---
>>   src/libcamera/v4l2_device.cpp            | 3 ++-
>>   2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 320a4dc5a..c14b57322 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -1479,9 +1479,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>>   	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
>>   		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
>>   		if (!list.empty())
>> -			supportedBlocks = static_cast<uint32_t>(
>> -				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
>> -					.get<int32_t>());
>> +			supportedBlocks = list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS).get<uint32_t>();
> 
> This is not related to the below changes right ?
> 
> I mean, the cast wasn't necessary even without the below changes ?

The above part is necessary in this patch. `RKISP1_CID_SUPPORTED_PARAMS_BLOCKS` is a bitmask,
but previously it was queried as `int32_t` (and then cast to `uint32_t`). Now that bitmasks
are `uint32_t`, the `get()` call template parameter needs to be adjusted (making the cast
unnecessary).


> 
> 
>>   	} else {
>>   		LOG(RkISP1, Error)
>>   			<< "Failed to query supported params blocks. Falling back to defaults.";
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index e0099cb7a..db1eb70e4 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -572,6 +572,7 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>>   		return ControlTypeUnsigned16;
>>
>>   	case V4L2_CTRL_TYPE_U32:
>> +	case V4L2_CTRL_TYPE_BITMASK:
>>   		return ControlTypeUnsigned32;
>>
>>   	case V4L2_CTRL_TYPE_INTEGER:
>> @@ -582,7 +583,6 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>>
>>   	case V4L2_CTRL_TYPE_MENU:
>>   	case V4L2_CTRL_TYPE_BUTTON:
>> -	case V4L2_CTRL_TYPE_BITMASK:
> 
> Becaue bitmask was deflected to the U32 case already, wasn't it ?

If I understand you correctly, then "yes".


> 
>>   	case V4L2_CTRL_TYPE_INTEGER_MENU:
>>   		/*
>>   		 * More precise types may be needed, for now use a 32-bit
>> @@ -636,6 +636,7 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
>>   				   static_cast<uint16_t>(ctrl.default_value));
>>
>>   	case V4L2_CTRL_TYPE_U32:
>> +	case V4L2_CTRL_TYPE_BITMASK:
>>   		return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
>>   				   static_cast<uint32_t>(ctrl.maximum),
>>   				   static_cast<uint32_t>(ctrl.default_value));
> 
> Anyway, I don't have objections on making this more precise
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
>> --
>> 2.53.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 320a4dc5a..c14b57322 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1479,9 +1479,7 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	if (controls.find(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS) != controls.end()) {
 		auto list = param_->getControls({ { RKISP1_CID_SUPPORTED_PARAMS_BLOCKS } });
 		if (!list.empty())
-			supportedBlocks = static_cast<uint32_t>(
-				list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS)
-					.get<int32_t>());
+			supportedBlocks = list.get(RKISP1_CID_SUPPORTED_PARAMS_BLOCKS).get<uint32_t>();
 	} else {
 		LOG(RkISP1, Error)
 			<< "Failed to query supported params blocks. Falling back to defaults.";
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index e0099cb7a..db1eb70e4 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -572,6 +572,7 @@  ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
 		return ControlTypeUnsigned16;
 
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_BITMASK:
 		return ControlTypeUnsigned32;
 
 	case V4L2_CTRL_TYPE_INTEGER:
@@ -582,7 +583,6 @@  ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
 
 	case V4L2_CTRL_TYPE_MENU:
 	case V4L2_CTRL_TYPE_BUTTON:
-	case V4L2_CTRL_TYPE_BITMASK:
 	case V4L2_CTRL_TYPE_INTEGER_MENU:
 		/*
 		 * More precise types may be needed, for now use a 32-bit
@@ -636,6 +636,7 @@  std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
 				   static_cast<uint16_t>(ctrl.default_value));
 
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_BITMASK:
 		return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
 				   static_cast<uint32_t>(ctrl.maximum),
 				   static_cast<uint32_t>(ctrl.default_value));