diff mbox series

[v5,2/4] media: v4l: ctrls: Add a control for HDR mode

Message ID 20220831090118.104057-3-benjamin.mugnier@foss.st.com (mailing list archive)
State New, archived
Headers show
Series media: Add ST VGXY61 camera sensor driver | expand

Commit Message

Benjamin Mugnier Aug. 31, 2022, 9:01 a.m. UTC 
Add V4L2_CID_HDR_MODE as a menu item control to set the HDR mode of the
sensor, and its documentation.
Menu items are not standardized as they differ for each sensors.

Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
---
 .../userspace-api/media/v4l/ext-ctrls-camera.rst          | 8 ++++++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 2 ++
 include/uapi/linux/v4l2-controls.h                        | 2 ++
 3 files changed, 12 insertions(+)

Comments

Nicolas Dufresne Sept. 12, 2022, 3:51 p.m. UTC | #1 
Hi Benjamin,

Le mercredi 31 août 2022 à 11:01 +0200, Benjamin Mugnier a écrit :
> Add V4L2_CID_HDR_MODE as a menu item control to set the HDR mode of the
> sensor, and its documentation.
> Menu items are not standardized as they differ for each sensors.
> 
> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-camera.rst          | 8 ++++++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 2 ++
>  include/uapi/linux/v4l2-controls.h                        | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..0ee09ff250e7 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,11 @@ enum v4l2_scene_mode -
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.
> +
> +``V4L2_CID_HDR_MODE (menu)``

Perhaps try to make this more sensor specific in it name ?
V4L2_CID_HDR_SENSOR_MODE ?

> +    Change the sensor HDR mode. A HDR picture is obtained by merging two
> +    captures of the same scene using two different exposure periods. HDR mode
> +    describes the way these two captures are merged in the sensor.
> +
> +    As modes differ for each sensor, menu items are not standardized by this
> +    control and are left to the programmer.

I do have concern about this approach. Can you clarify ?

regards,
Nicolas

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index e22921e7ea61..0854de1ef1a5 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1043,6 +1043,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> +	case V4L2_CID_HDR_MODE:			return "HDR mode";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1370,6 +1371,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_STATELESS_H264_START_CODE:
>  	case V4L2_CID_CAMERA_ORIENTATION:
>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE:
> +	case V4L2_CID_HDR_MODE:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
>  	case V4L2_CID_LINK_FREQ:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 5f46bf4a570c..5dfd38b09768 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1013,6 +1013,8 @@ enum v4l2_auto_focus_range {
>  
>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>  
> +#define V4L2_CID_HDR_MODE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
Benjamin Mugnier Sept. 15, 2022, 7:29 a.m. UTC | #2 
Hi Nicolas,

Thank you for your review.

On 9/12/22 17:51, Nicolas Dufresne wrote:
> Hi Benjamin,
> 
> Le mercredi 31 août 2022 à 11:01 +0200, Benjamin Mugnier a écrit :
>> Add V4L2_CID_HDR_MODE as a menu item control to set the HDR mode of the
>> sensor, and its documentation.
>> Menu items are not standardized as they differ for each sensors.
>>
>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>> ---
>>  .../userspace-api/media/v4l/ext-ctrls-camera.rst          | 8 ++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 2 ++
>>  include/uapi/linux/v4l2-controls.h                        | 2 ++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 4c5061aa9cd4..0ee09ff250e7 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -661,3 +661,11 @@ enum v4l2_scene_mode -
>>  .. [#f1]
>>     This control may be changed to a menu control in the future, if more
>>     options are required.
>> +
>> +``V4L2_CID_HDR_MODE (menu)``
> 
> Perhaps try to make this more sensor specific in it name ?
> V4L2_CID_HDR_SENSOR_MODE ?
> 

No problem, queued for v6. Thank you.

>> +    Change the sensor HDR mode. A HDR picture is obtained by merging two
>> +    captures of the same scene using two different exposure periods. HDR mode
>> +    describes the way these two captures are merged in the sensor.
>> +
>> +    As modes differ for each sensor, menu items are not standardized by this
>> +    control and are left to the programmer.
> 
> I do have concern about this approach. Can you clarify ?
> 

Sure! This is the implementation of Sakari's excellent idea from v3 which you can read here:
https://lore.kernel.org/linux-media/YwNcUpsw1psudCOC@paasikivi.fi.intel.com/

The control name is standardized as a menu type, but values of the menu or not. The driver developer has to provide them by himself in its driver when adding the control. This is pretty versatile as hdr modes differ from one sensor to another (at least for my cases).
For the vgxy61 I defined these hdr modes in the driver file:

+static const char * const vgxy61_hdr_mode_menu[] = {
+	"HDR linearize",
+	"HDR substraction",
+	"No HDR",
+};

and added these elements to the V4L2_CID_HDR_MODE control with a std_menu_items in the vgxy61_init_controls function:

+	v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE,
+				     ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0,
+				     VGXY61_NO_HDR, vgxy61_hdr_mode_menu);

I hope this is clearer.
This implementation can of course be discussed. Tell me what you think.


Regards,

Benjamin

> regards,
> Nicolas
> 
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index e22921e7ea61..0854de1ef1a5 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1043,6 +1043,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
>>  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
>>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>> +	case V4L2_CID_HDR_MODE:			return "HDR mode";
>>  
>>  	/* FM Radio Modulator controls */
>>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1370,6 +1371,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_STATELESS_H264_START_CODE:
>>  	case V4L2_CID_CAMERA_ORIENTATION:
>>  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE:
>> +	case V4L2_CID_HDR_MODE:
>>  		*type = V4L2_CTRL_TYPE_MENU;
>>  		break;
>>  	case V4L2_CID_LINK_FREQ:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 5f46bf4a570c..5dfd38b09768 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1013,6 +1013,8 @@ enum v4l2_auto_focus_range {
>>  
>>  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
>>  
>> +#define V4L2_CID_HDR_MODE			(V4L2_CID_CAMERA_CLASS_BASE+36)
>> +
>>  /* FM Modulator class control IDs */
>>  
>>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
>
Nicolas Dufresne Oct. 18, 2022, 3:15 p.m. UTC | #3 
Hi,

Le jeudi 15 septembre 2022 à 09:29 +0200, Benjamin MUGNIER a écrit :
> Hi Nicolas,
> 
> Thank you for your review.
> 
> On 9/12/22 17:51, Nicolas Dufresne wrote:
> > Hi Benjamin,
> > 
> > Le mercredi 31 août 2022 à 11:01 +0200, Benjamin Mugnier a écrit :
> > > Add V4L2_CID_HDR_MODE as a menu item control to set the HDR mode of the
> > > sensor, and its documentation.
> > > Menu items are not standardized as they differ for each sensors.
> > > 
> > > Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com>
> > > ---
> > >  .../userspace-api/media/v4l/ext-ctrls-camera.rst          | 8 ++++++++
> > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 2 ++
> > >  include/uapi/linux/v4l2-controls.h                        | 2 ++
> > >  3 files changed, 12 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > index 4c5061aa9cd4..0ee09ff250e7 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > @@ -661,3 +661,11 @@ enum v4l2_scene_mode -
> > >  .. [#f1]
> > >     This control may be changed to a menu control in the future, if more
> > >     options are required.
> > > +
> > > +``V4L2_CID_HDR_MODE (menu)``
> > 
> > Perhaps try to make this more sensor specific in it name ?
> > V4L2_CID_HDR_SENSOR_MODE ?
> > 
> 
> No problem, queued for v6. Thank you.
> 
> > > +    Change the sensor HDR mode. A HDR picture is obtained by merging two
> > > +    captures of the same scene using two different exposure periods. HDR mode
> > > +    describes the way these two captures are merged in the sensor.
> > > +
> > > +    As modes differ for each sensor, menu items are not standardized by this
> > > +    control and are left to the programmer.
> > 
> > I do have concern about this approach. Can you clarify ?
> > 
> 
> Sure! This is the implementation of Sakari's excellent idea from v3 which you can read here:
> https://lore.kernel.org/linux-media/YwNcUpsw1psudCOC@paasikivi.fi.intel.com/
> 
> The control name is standardized as a menu type, but values of the menu or not. The driver developer has to provide them by himself in its driver when adding the control. This is pretty versatile as hdr modes differ from one sensor to another (at least for my cases).
> For the vgxy61 I defined these hdr modes in the driver file:
> 
> +static const char * const vgxy61_hdr_mode_menu[] = {
> +	"HDR linearize",
> +	"HDR substraction",
> +	"No HDR",
> +};
> 
> and added these elements to the V4L2_CID_HDR_MODE control with a std_menu_items in the vgxy61_init_controls function:
> 
> +	v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE,
> +				     ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0,
> +				     VGXY61_NO_HDR, vgxy61_hdr_mode_menu);
> 
> I hope this is clearer.
> This implementation can of course be discussed. Tell me what you think.

Just food for the mind, this is going to be HW specific. If we go that way, I'd
like to adopt a rule similar to DRM, that for these type of controls there must
be an Open Source userland users of it. I'm sure you'd have no issue with that.

The downside is that we no longer have a place to extend on the HW specific
documentation/knowledge, which often endup lost. How is that approach better
then having multiple vendor controls ? (Perhaps there is family of censors that
could possibly share some of the controls and we'd have a common place to
document the exact meaning of the menu items).

> 
> 
> Regards,
> 
> Benjamin
> 
> > regards,
> > Nicolas
> > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index e22921e7ea61..0854de1ef1a5 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1043,6 +1043,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
> > >  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
> > >  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> > > +	case V4L2_CID_HDR_MODE:			return "HDR mode";
> > >  
> > >  	/* FM Radio Modulator controls */
> > >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > @@ -1370,6 +1371,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >  	case V4L2_CID_STATELESS_H264_START_CODE:
> > >  	case V4L2_CID_CAMERA_ORIENTATION:
> > >  	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE:
> > > +	case V4L2_CID_HDR_MODE:
> > >  		*type = V4L2_CTRL_TYPE_MENU;
> > >  		break;
> > >  	case V4L2_CID_LINK_FREQ:
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 5f46bf4a570c..5dfd38b09768 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1013,6 +1013,8 @@ enum v4l2_auto_focus_range {
> > >  
> > >  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
> > >  
> > > +#define V4L2_CID_HDR_MODE			(V4L2_CID_CAMERA_CLASS_BASE+36)
> > > +
> > >  /* FM Modulator class control IDs */
> > >  
> > >  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> >
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 4c5061aa9cd4..0ee09ff250e7 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -661,3 +661,11 @@  enum v4l2_scene_mode -
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
+
+``V4L2_CID_HDR_MODE (menu)``
+    Change the sensor HDR mode. A HDR picture is obtained by merging two
+    captures of the same scene using two different exposure periods. HDR mode
+    describes the way these two captures are merged in the sensor.
+
+    As modes differ for each sensor, menu items are not standardized by this
+    control and are left to the programmer.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index e22921e7ea61..0854de1ef1a5 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1043,6 +1043,7 @@  const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
 	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
 	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
+	case V4L2_CID_HDR_MODE:			return "HDR mode";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1370,6 +1371,7 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_STATELESS_H264_START_CODE:
 	case V4L2_CID_CAMERA_ORIENTATION:
 	case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE:
+	case V4L2_CID_HDR_MODE:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
 	case V4L2_CID_LINK_FREQ:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 5f46bf4a570c..5dfd38b09768 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1013,6 +1013,8 @@  enum v4l2_auto_focus_range {
 
 #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
 
+#define V4L2_CID_HDR_MODE			(V4L2_CID_CAMERA_CLASS_BASE+36)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)