media: add pixel_size control
Ricardo Ribalda Delgado
maintainerPATCH 1/3 media: add pixel_size control
This control returns the pixel size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of the sensor.
--- drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++ include/media/v4l2-ctrls.h | 2 ++ include/uapi/linux/v4l2-controls.h | 3 +++ include/uapi/linux/videodev2.h | 11 +++++++++++ 4 files changed, 27 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +11 -0
@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)case V4L2_CID_AUTO_FOCUS_RANGE :return "Auto Focus, Range" ;case V4L2_CID_PAN_SPEED :return "Pan, Speed" ;case V4L2_CID_TILT_SPEED :return "Tilt, Speed" ; +case V4L2_CID_PIXEL_SIZE :return "Pixel Size" ;/* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER : *type =V4L2_CTRL_TYPE_VP8_FRAME_HEADER ;break ; +case V4L2_CID_PIXEL_SIZE : + *type =V4L2_CTRL_TYPE_PIXEL_SIZE ; +break ;default : *type =V4L2_CTRL_TYPE_INTEGER ;break ;
@@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT :case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM :case V4L2_CID_RDS_RX_MUSIC_SPEECH : +case V4L2_CID_PIXEL_SIZE : *flags |=V4L2_CTRL_FLAG_READ_ONLY ;break ;case V4L2_CID_RF_TUNER_PLL_LOCK :
@@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_FWHT_PARAMS :break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : +break ; +case V4L2_CTRL_TYPE_H264_SPS :case V4L2_CTRL_TYPE_H264_PPS :case V4L2_CTRL_TYPE_H264_SCALING_MATRIX :
@@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,case V4L2_CTRL_TYPE_VP8_FRAME_HEADER : elem_size =sizeof (struct v4l2_ctrl_vp8_frame_header );break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : + elem_size =sizeof (struct v4l2_pixel_size ); +break ;default :if (type <V4L2_CTRL_COMPOUND_TYPES ) elem_size =sizeof (s32);
include/media/v4l2-ctrls.h +2 -0
@@ -50,6 +50,7 @@ struct poll_table_struct;* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. * @p_vp8_frame_header: Pointer to a VP8 frame header structure. +* @p_pixel_size: Pointer to a pixel_size value. * @p: Pointer to a compound value. */ union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; +struct v4l2_pixel_size *p_pixel_size;void *p; };
include/uapi/linux/v4l2-controls.h +3 -0
@@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {#define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32)#define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) +#define V4L2_CID_PIXEL_SIZE (V4L2_CID_CAMERA_CLASS_BASE+34) + +/* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
include/uapi/linux/videodev2.h +11 -0
@@ -422,6 +422,11 @@ struct v4l2_fract {__u32 denominator; }; +struct v4l2_pixel_size { +__u32 width; +__u32 height; +}; +/** * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP *
@@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {V4L2_CTRL_TYPE_U8 =0x0100 ,V4L2_CTRL_TYPE_U16 =0x0101 ,V4L2_CTRL_TYPE_U32 =0x0102 , +/* +* V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103, +* V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104, +* V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105, +*/ + V4L2_CTRL_TYPE_PIXEL_SIZE = 0x0106, };/* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
Signature · 1 line
2.23.0.rc1
Ricardo Ribalda Delgado
maintainerPATCH 2/3 Documentation: Describe V4L2_CID_PIXEL_SIZE
New control to pass to userspace the width/height of a pixel. Which is needed for 3D calibration and lens selection.
--- Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/media/uapi/v4l/ext-ctrls-camera.rst +6 -0
@@ -510,6 +510,12 @@ enum v4l2_scene_mode - value down. A value of zero stops the motion if one is in progress and has no effect otherwise. +``V4L2_CID_PIXEL_SIZE (struct)`` + This control returns the pixel size in nanometres. The struct provides + the width and the height in separated fields to take into consideration + asymmetric pixels and/or hardware binning. + This control is required for automatic calibration of the sensor. + .. [#f1] This control may be changed to a menu control in the future, if more options are required.
Signature · 1 line
2.23.0.rc1
Re: [PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE
Quoted 15 lines · Ricardo Ribalda Delgado
New control to pass to userspace the width/height of a pixel. Which is needed for 3D calibration and lens selection.
Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/media/uapi/v4l/ext-ctrls-camera.rst +0
-0@@ -510,6 +510,12 @@ enum v4l2_scene_mode - value down. A value of zero stops the motion if one is in progress and has no effect otherwise.
Documentation/media/uapi/v4l/ext-ctrls-camera.rst enum v4l2_scene_mode - · line 521
Quoted 10 lines
Documentation/media/uapi/v4l/ext-ctrls-camera.rst +6
-0+``V4L2_CID_PIXEL_SIZE (struct)`` + This control returns the pixel size in nanometres. The struct provides + the width and the height in separated fields to take into consideration + asymmetric pixels and/or hardware binning. + This control is required for automatic calibration of the sensor. + .. [#f1] This control may be changed to a menu control in the future, if more options are required.
I suppose this is a common term, but should it be mentioned that pixel size is the same as unit cell size, and not necessarily the size of the light sensitive area? Just in case the effective fill-factor is < 100%.
regards
Philipp
Ricardo Ribalda Delgado
maintainerRe: [PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE
Completely agree. Maybe we should call the control unit cell size?, in case in the future we need a control for the light sensitive area.
Best regards
Quoted 34 lines · Philipp Zabel
Quoted 15 lines · Ricardo Ribalda Delgado
New control to pass to userspace the width/height of a pixel. Which is needed for 3D calibration and lens selection.
Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 6 ++++++ 1 file changed, 6 insertions(+)Documentation/media/uapi/v4l/ext-ctrls-camera.rst +0
-0@@ -510,6 +510,12 @@ enum v4l2_scene_mode - value down. A value of zero stops the motion if one is in progress and has no effect otherwise.Documentation/media/uapi/v4l/ext-ctrls-camera.rst enum v4l2_scene_mode - · line 521
Quoted 10 lines
Documentation/media/uapi/v4l/ext-ctrls-camera.rst +6
-0+``V4L2_CID_PIXEL_SIZE (struct)`` + This control returns the pixel size in nanometres. The struct provides + the width and the height in separated fields to take into consideration + asymmetric pixels and/or hardware binning. + This control is required for automatic calibration of the sensor. + .. [#f1] This control may be changed to a menu control in the future, if more options are required.I suppose this is a common term, but should it be mentioned that pixel size is the same as unit cell size, and not necessarily the size of the light sensitive area? Just in case the effective fill-factor is < 100%.
regards
Philipp
Ricardo Ribalda Delgado
maintainerPATCH 3/3 media: imx214: Add new control with V4L2_CID_PIXEL_SIZE
In reply to Ricardo Ribalda DelgadoAccording to the product brief, the unit cell size is 1120 nanometers^2.
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
--- drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
drivers/media/i2c/imx214.c +23 -0
@@ -47,6 +47,7 @@ struct imx214 {struct v4l2_ctrl *pixel_rate;struct v4l2_ctrl *link_freq;struct v4l2_ctrl *exposure; +struct v4l2_ctrl *pixel_size;struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES ];
@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)return ret; } +static void pixel_size_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ + ptr.p_pixel_size->width =1120 ; + ptr.p_pixel_size->height =1120 ; +} + +static const struct v4l2_ctrl_type_ops pixel_size_ops = { + .init = pixel_size_init, +}; + +static struct v4l2_ctrl *new_pixel_size_ctrl (struct v4l2_ctrl_handler *handler) +{ +static struct v4l2_ctrl_config ctrl = { + .id =V4L2_CID_PIXEL_SIZE , + .type_ops = &pixel_size_ops, + }; + +return v4l2_ctrl_new_custom (handler, &ctrl,NULL ); +}static int imx214_probe (struct i2c_client *client) {struct device *dev = &client->dev;
@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)V4L2_CID_EXPOSURE , 0, 3184, 1, 0x0c70); + imx214->pixel_size =new_pixel_size_ctrl (&imx214->ctrls); + ret = imx214->ctrls.error;if (ret) { dev_err(&client->dev, "%s control init failed (%d)\n",
Signature · 1 line
2.23.0.rc1
Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE
Hi Ricardo,
Quoted 1 line · Ricardo Ribalda Delgado
According to the product brief, the unit cell size is 1120 nanometers^2.
Should this information come from DT ?
I'm asking as I've a series in review that adds an helper that collectes dt properties and register controls for them. It currently only supports the newly proposed camera location control, but there might be others like the rotation, for which we already have a DT property.
https://patchwork.kernel.org/project/linux-media/list/?series=160901
This new one is indeed an HW property of the sensor, I wonder if having it in the firmware interface would make any sense or not...
Thanks
j
Quoted 59 lines
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)drivers/media/i2c/imx214.c +23
-1@@ -47,6 +47,7 @@ struct imx214 {struct v4l2_ctrl *pixel_rate;struct v4l2_ctrl *link_freq;struct v4l2_ctrl *exposure; +struct v4l2_ctrl *pixel_size;struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES ];@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)return ret; } +static void pixel_size_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ + ptr.p_pixel_size->width =1120 ; + ptr.p_pixel_size->height =1120 ; +} + +static const struct v4l2_ctrl_type_ops pixel_size_ops = { + .init = pixel_size_init, +}; + +static struct v4l2_ctrl *new_pixel_size_ctrl (struct v4l2_ctrl_handler *handler) +{ +static struct v4l2_ctrl_config ctrl = { + .id =V4L2_CID_PIXEL_SIZE , + .type_ops = &pixel_size_ops, + }; + +return v4l2_ctrl_new_custom (handler, &ctrl,NULL ); +}static int imx214_probe (struct i2c_client *client) {struct device *dev = &client->dev;@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)V4L2_CID_EXPOSURE , 0, 3184, 1, 0x0c70); + imx214->pixel_size =new_pixel_size_ctrl (&imx214->ctrls); + ret = imx214->ctrls.error;if (ret) { dev_err(&client->dev, "%s control init failed (%d)\n", --2.23.0.rc1
Ricardo Ribalda Delgado
maintainerRe: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE
Hi Jacopo
Quoted 7 lines · Jacopo Mondi
Hi Ricardo,
Quoted 1 line · Ricardo Ribalda Delgado
According to the product brief, the unit cell size is 1120 nanometers^2.
Should this information come from DT ?
I do not think so. You cannot change this value and it needs to be defined also in sensors/cameras that might not have a DT, like a usb webcam.
It would be like adding to the DT the min/max exposure time...
But of course we can discuss it ;)
Best regards
Quoted 74 lines
I'm asking as I've a series in review that adds an helper that collectes dt properties and register controls for them. It currently only supports the newly proposed camera location control, but there might be others like the rotation, for which we already have a DT property.
https://patchwork.kernel.org/project/linux-media/list/?series=160901
This new one is indeed an HW property of the sensor, I wonder if having it in the firmware interface would make any sense or not...
Thanks
jQuoted 59 lines
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)drivers/media/i2c/imx214.c +23
-1@@ -47,6 +47,7 @@ struct imx214 {struct v4l2_ctrl *pixel_rate;struct v4l2_ctrl *link_freq;struct v4l2_ctrl *exposure; +struct v4l2_ctrl *pixel_size;struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES ];@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)return ret; } +static void pixel_size_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ + ptr.p_pixel_size->width =1120 ; + ptr.p_pixel_size->height =1120 ; +} + +static const struct v4l2_ctrl_type_ops pixel_size_ops = { + .init = pixel_size_init, +}; + +static struct v4l2_ctrl *new_pixel_size_ctrl (struct v4l2_ctrl_handler *handler) +{ +static struct v4l2_ctrl_config ctrl = { + .id =V4L2_CID_PIXEL_SIZE , + .type_ops = &pixel_size_ops, + }; + +return v4l2_ctrl_new_custom (handler, &ctrl,NULL ); +}static int imx214_probe (struct i2c_client *client) {struct device *dev = &client->dev;@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)V4L2_CID_EXPOSURE , 0, 3184, 1, 0x0c70); + imx214->pixel_size =new_pixel_size_ctrl (&imx214->ctrls); + ret = imx214->ctrls.error;if (ret) { dev_err(&client->dev, "%s control init failed (%d)\n", --2.23.0.rc1
Re: [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE
Hi Ricardo,
Quoted 7 lines · Ricardo Ribalda Delgado
Hi Jacopo
Quoted 3 lines · Jacopo Mondi
Hi Ricardo,
Quoted 8 lines
Quoted 4 lines · Ricardo Ribalda Delgado
Quoted 1 line
According to the product brief, the unit cell size is 1120 nanometers^2.
Should this information come from DT ?
I do not think so. You cannot change this value and it needs to be defined also in sensors/cameras that might not have a DT, like a usb webcam.
You're probably right. I got this thinking because the camera location/orientation are two read only parameters that come from DT, but their value depends on the design of the device where the camera is installed on, so they're configurable, while this and other physical properties are not, and it doesn't make much sense to have them in DT.
Thanks
j
Quoted 81 lines
It would be like adding to the DT the min/max exposure time...
But of course we can discuss it ;)
Best regards
Quoted 74 lines
I'm asking as I've a series in review that adds an helper that collectes dt properties and register controls for them. It currently only supports the newly proposed camera location control, but there might be others like the rotation, for which we already have a DT property.
https://patchwork.kernel.org/project/linux-media/list/?series=160901
This new one is indeed an HW property of the sensor, I wonder if having it in the firmware interface would make any sense or not...
Thanks
jQuoted 59 lines
https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf
Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)drivers/media/i2c/imx214.c +23
-1@@ -47,6 +47,7 @@ struct imx214 {struct v4l2_ctrl *pixel_rate;struct v4l2_ctrl *link_freq;struct v4l2_ctrl *exposure; +struct v4l2_ctrl *pixel_size;struct regulator_bulk_data supplies[IMX214_NUM_SUPPLIES ];@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)return ret; } +static void pixel_size_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ + ptr.p_pixel_size->width =1120 ; + ptr.p_pixel_size->height =1120 ; +} + +static const struct v4l2_ctrl_type_ops pixel_size_ops = { + .init = pixel_size_init, +}; + +static struct v4l2_ctrl *new_pixel_size_ctrl (struct v4l2_ctrl_handler *handler) +{ +static struct v4l2_ctrl_config ctrl = { + .id =V4L2_CID_PIXEL_SIZE , + .type_ops = &pixel_size_ops, + }; + +return v4l2_ctrl_new_custom (handler, &ctrl,NULL ); +}static int imx214_probe (struct i2c_client *client) {struct device *dev = &client->dev;@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)V4L2_CID_EXPOSURE , 0, 3184, 1, 0x0c70); + imx214->pixel_size =new_pixel_size_ctrl (&imx214->ctrls); + ret = imx214->ctrls.error;if (ret) { dev_err(&client->dev, "%s control init failed (%d)\n", --2.23.0.rc1
Hi Ricardo,
Quoted 18 lines · Ricardo Ribalda Delgado
This control returns the pixel size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of the sensor.Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++ include/media/v4l2-ctrls.h | 2 ++ include/uapi/linux/v4l2-controls.h | 3 +++ include/uapi/linux/videodev2.h | 11 +++++++++++ 4 files changed, 27 insertions(+)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0
drivers/media/v4l2-core/v4l2-ctrls.c const char *v4l2_ctrl_get_name(u32 id) · line 981
Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-0@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)case V4L2_CID_AUTO_FOCUS_RANGE :return "Auto Focus, Range" ;case V4L2_CID_PAN_SPEED :return "Pan, Speed" ;case V4L2_CID_TILT_SPEED :return "Tilt, Speed" ; +case V4L2_CID_PIXEL_SIZE :return "Pixel Size" ;
Is this a camera class control or an image source one ?
Also, isn't pixel size a bit too generic? I would somehow specify this is the physical pixel size
drivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1362
Quoted 9 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0/* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER : *type =V4L2_CTRL_TYPE_VP8_FRAME_HEADER ;break ; +case V4L2_CID_PIXEL_SIZE : + *type =V4L2_CTRL_TYPE_PIXEL_SIZE ;
Isn't this a read-only control?
drivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1431
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0+break ;default : *type =V4L2_CTRL_TYPE_INTEGER ;break ;@@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT :case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM :case V4L2_CID_RDS_RX_MUSIC_SPEECH : +case V4L2_CID_PIXEL_SIZE : *flags |=V4L2_CTRL_FLAG_READ_ONLY ;
Ah yes, you set flags here... I don't get why we have two switches here.. Anyway, I would set both type and flags in a single case, otherwise one should jump back and forth...
Quoted 47 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6
-0break ;case V4L2_CID_RF_TUNER_PLL_LOCK :@@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_FWHT_PARAMS :break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : +break ; +case V4L2_CTRL_TYPE_H264_SPS :case V4L2_CTRL_TYPE_H264_PPS :case V4L2_CTRL_TYPE_H264_SCALING_MATRIX :@@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,case V4L2_CTRL_TYPE_VP8_FRAME_HEADER : elem_size =sizeof (struct v4l2_ctrl_vp8_frame_header );break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : + elem_size =sizeof (struct v4l2_pixel_size ); +break ;default :if (type <V4L2_CTRL_COMPOUND_TYPES ) elem_size =sizeof (s32);include/media/v4l2-ctrls.h +2
-0@@ -50,6 +50,7 @@ struct poll_table_struct;* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. * @p_vp8_frame_header: Pointer to a VP8 frame header structure. +* @p_pixel_size: Pointer to a pixel_size value. * @p: Pointer to a compound value. */ union v4l2_ctrl_ptr {@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; +struct v4l2_pixel_size *p_pixel_size;void *p; };include/uapi/linux/v4l2-controls.h +0
-0
include/uapi/linux/v4l2-controls.h enum v4l2_auto_focus_range { · line 917
Quoted 6 lines
include/uapi/linux/v4l2-controls.h +3
-0@@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {#define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32)#define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) +#define V4L2_CID_PIXEL_SIZE (V4L2_CID_CAMERA_CLASS_BASE+34) + +
Double empty line
include/uapi/linux/videodev2.h struct v4l2_fract { · line 425
Quoted 12 lines
include/uapi/linux/v4l2-controls.h +0
-0/* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)include/uapi/linux/videodev2.h +1
-0@@ -422,6 +422,11 @@ struct v4l2_fract {__u32 denominator; }; +struct v4l2_pixel_size {
I wonder if instead of defining a 'pixel size' this shouldn't be a more generic v4l2_size or something similar. Or even a v4l2_rect with left=top=0.
Quoted 8 lines
include/uapi/linux/videodev2.h +4
-0+__u32 width; +__u32 height; +}; +/** * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP *
include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1730
Quoted 8 lines
include/uapi/linux/videodev2.h +5
-0@@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {V4L2_CTRL_TYPE_U8 =0x0100 ,V4L2_CTRL_TYPE_U16 =0x0101 ,V4L2_CTRL_TYPE_U32 =0x0102 , +/* +* V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103, +* V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104, +* V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105, + */
leftovers ?
include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1731
Quoted 1 line
include/uapi/linux/videodev2.h +1
-0+ V4L2_CTRL_TYPE_PIXEL_SIZE = 0x0106,
I don't see other compound controls with a custom payload adding their types here. What am I missing?
Thanks
j
Quoted 6 lines
include/uapi/linux/videodev2.h +0
-1};/* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ --2.23.0.rc1
Ricardo Ribalda Delgado
maintainerHi Jacopo
[sorry for the multiple send, working on a train and fighting with the mail client and the 4g connection :S]
Quoted 17 lines · On Wed, 21 Aug 2019, 18:10 Jacopo Mondi, <[email protected]> wrote:
Hi Ricardo,
Quoted 13 lines · Ricardo Ribalda Delgado
This control returns the pixel size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of the sensor.Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++ include/media/v4l2-ctrls.h | 2 ++ include/uapi/linux/v4l2-controls.h | 3 +++ include/uapi/linux/videodev2.h | 11 +++++++++++ 4 files changed, 27 insertions(+)
Quoted 13 lines
drivers/media/v4l2-core/v4l2-ctrls.c const char *v4l2_ctrl_get_name(u32 id) · line 981
Quoted 9 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-0@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)case V4L2_CID_AUTO_FOCUS_RANGE :return "Auto Focus, Range" ;case V4L2_CID_PAN_SPEED :return "Pan, Speed" ;case V4L2_CID_TILT_SPEED :return "Tilt, Speed" ; +case V4L2_CID_PIXEL_SIZE :return "Pixel Size" ;Is this a camera class control or an image source one ?
Also, isn't pixel size a bit too generic? I would somehow specify this is the physical pixel size
Please review v2. It has changed from pixel_size to unit size, which is more clear.
Quoted 14 lines
drivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1362
Quoted 11 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0/* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER : *type =V4L2_CTRL_TYPE_VP8_FRAME_HEADER ;break ; +case V4L2_CID_PIXEL_SIZE : + *type =V4L2_CTRL_TYPE_PIXEL_SIZE ;Isn't this a read-only control?
Quoted 14 lines
drivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1431
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0+break ;default : *type =V4L2_CTRL_TYPE_INTEGER ;break ;@@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT :case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM :case V4L2_CID_RDS_RX_MUSIC_SPEECH : +case V4L2_CID_PIXEL_SIZE : *flags |=V4L2_CTRL_FLAG_READ_ONLY ;Ah yes, you set flags here... I don't get why we have two switches here.. Anyway, I would set both type and flags in a single case, otherwise one should jump back and forth...
Sure. will do on v3
Quoted 51 lines
Quoted 51 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6
-0break ;case V4L2_CID_RF_TUNER_PLL_LOCK :@@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_FWHT_PARAMS :break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : +break ; +case V4L2_CTRL_TYPE_H264_SPS :case V4L2_CTRL_TYPE_H264_PPS :case V4L2_CTRL_TYPE_H264_SCALING_MATRIX :@@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,case V4L2_CTRL_TYPE_VP8_FRAME_HEADER : elem_size =sizeof (struct v4l2_ctrl_vp8_frame_header );break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : + elem_size =sizeof (struct v4l2_pixel_size ); +break ;default :if (type <V4L2_CTRL_COMPOUND_TYPES ) elem_size =sizeof (s32);include/media/v4l2-ctrls.h +2
-0@@ -50,6 +50,7 @@ struct poll_table_struct;* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. * @p_vp8_frame_header: Pointer to a VP8 frame header structure. +* @p_pixel_size: Pointer to a pixel_size value. * @p: Pointer to a compound value. */ union v4l2_ctrl_ptr {@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; +struct v4l2_pixel_size *p_pixel_size;void *p; };include/uapi/linux/v4l2-controls.h +0
-0@@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {#define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32)#define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33)
Quoted 5 lines
include/uapi/linux/v4l2-controls.h enum v4l2_auto_focus_range { · line 917
Quoted 3 lines
include/uapi/linux/v4l2-controls.h +3
-0+#define V4L2_CID_PIXEL_SIZE (V4L2_CID_CAMERA_CLASS_BASE+34) + +Double empty line
Thanks , will fix in v3
Quoted 12 lines
Quoted 12 lines
include/uapi/linux/v4l2-controls.h +0
-0/* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)include/uapi/linux/videodev2.h +0
-0@@ -422,6 +422,11 @@ struct v4l2_fract {__u32 denominator; };
Quoted 5 lines
include/uapi/linux/videodev2.h struct v4l2_fract { · line 425
Quoted 1 line
include/uapi/linux/videodev2.h +1
-0+struct v4l2_pixel_size {I wonder if instead of defining a 'pixel size' this shouldn't be a more generic
v4l2_sizeor something similar. Or even av4l2_rectwith left=top=0.
Fixed on v2
Quoted 11 lines
Quoted 11 lines
include/uapi/linux/videodev2.h +4
-0+__u32 width; +__u32 height; +}; +/** * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP * @@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {V4L2_CTRL_TYPE_U8 =0x0100 , V4L2_CTRL_TYPE_U16 = 0x0101,
Quoted 8 lines
include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1730
Quoted 6 lines
include/uapi/linux/videodev2.h +5
-0V4L2_CTRL_TYPE_U32 = 0x0102, +/* +* V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103, +* V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104, +* V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105, +*/ leftovers ?
They are defined on other .h. I thought it was nice to know why I did not use 0x0103
include/media/mpeg2-ctrls.h:#define V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS 0x0103 include/media/mpeg2-ctrls.h:#define V4L2_CTRL_TYPE_MPEG2_QUANTIZATION 0x0104 include/media/fwht-ctrls.h:#define V4L2_CTRL_TYPE_FWHT_PARAMS 0x0105
Quoted 5 lines
include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1731
Quoted 2 lines
include/uapi/linux/videodev2.h +1
-0+ V4L2_CTRL_TYPE_PIXEL_SIZE = 0x0106,I don't see other compound controls with a custom payload adding their types here. What am I missing?
@Hans could you clarify here what is the best practice?
Thanks!!
Quoted 150 lines · Jacopo Mondi
Hi Ricardo,
Quoted 18 lines · Ricardo Ribalda Delgado
This control returns the pixel size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of the sensor.Signed-off-by Ricardo Ribalda Delgado <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++ include/media/v4l2-ctrls.h | 2 ++ include/uapi/linux/v4l2-controls.h | 3 +++ include/uapi/linux/videodev2.h | 11 +++++++++++ 4 files changed, 27 insertions(+)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0drivers/media/v4l2-core/v4l2-ctrls.c const char *v4l2_ctrl_get_name(u32 id) · line 981
Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-0@@ -978,6 +978,7 @@ const char *v4l2_ctrl_get_name(u32 id)case V4L2_CID_AUTO_FOCUS_RANGE :return "Auto Focus, Range" ;case V4L2_CID_PAN_SPEED :return "Pan, Speed" ;case V4L2_CID_TILT_SPEED :return "Tilt, Speed" ; +case V4L2_CID_PIXEL_SIZE :return "Pixel Size" ;Is this a camera class control or an image source one ?
Also, isn't pixel size a bit too generic? I would somehow specify this is the physical pixel sizedrivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1362
Quoted 9 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0/* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1357,6 +1358,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER : *type =V4L2_CTRL_TYPE_VP8_FRAME_HEADER ;break ; +case V4L2_CID_PIXEL_SIZE : + *type =V4L2_CTRL_TYPE_PIXEL_SIZE ;Isn't this a read-only control?
drivers/media/v4l2-core/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 1431
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +2
-0+break ;default : *type =V4L2_CTRL_TYPE_INTEGER ;break ;@@ -1423,6 +1427,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,case V4L2_CID_RDS_RX_TRAFFIC_ANNOUNCEMENT :case V4L2_CID_RDS_RX_TRAFFIC_PROGRAM :case V4L2_CID_RDS_RX_MUSIC_SPEECH : +case V4L2_CID_PIXEL_SIZE : *flags |=V4L2_CTRL_FLAG_READ_ONLY ;Ah yes, you set flags here... I don't get why we have two switches here.. Anyway, I would set both type and flags in a single case, otherwise one should jump back and forth...
Quoted 47 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6
-0break ;case V4L2_CID_RF_TUNER_PLL_LOCK :@@ -1705,6 +1710,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_FWHT_PARAMS :break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : +break ; +case V4L2_CTRL_TYPE_H264_SPS :case V4L2_CTRL_TYPE_H264_PPS :case V4L2_CTRL_TYPE_H264_SCALING_MATRIX :@@ -2403,6 +2411,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,case V4L2_CTRL_TYPE_VP8_FRAME_HEADER : elem_size =sizeof (struct v4l2_ctrl_vp8_frame_header );break ; +case V4L2_CTRL_TYPE_PIXEL_SIZE : + elem_size =sizeof (struct v4l2_pixel_size ); +break ;default :if (type <V4L2_CTRL_COMPOUND_TYPES ) elem_size =sizeof (s32);include/media/v4l2-ctrls.h +2
-0@@ -50,6 +50,7 @@ struct poll_table_struct;* @p_h264_slice_params: Pointer to a struct v4l2_ctrl_h264_slice_params. * @p_h264_decode_params: Pointer to a struct v4l2_ctrl_h264_decode_params. * @p_vp8_frame_header: Pointer to a VP8 frame header structure. +* @p_pixel_size: Pointer to a pixel_size value. * @p: Pointer to a compound value. */ union v4l2_ctrl_ptr {@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header; +struct v4l2_pixel_size *p_pixel_size;void *p; };include/uapi/linux/v4l2-controls.h +0
-0include/uapi/linux/v4l2-controls.h enum v4l2_auto_focus_range { · line 917
Quoted 6 lines
include/uapi/linux/v4l2-controls.h +3
-0@@ -912,6 +912,9 @@ enum v4l2_auto_focus_range {#define V4L2_CID_PAN_SPEED (V4L2_CID_CAMERA_CLASS_BASE+32)#define V4L2_CID_TILT_SPEED (V4L2_CID_CAMERA_CLASS_BASE+33) +#define V4L2_CID_PIXEL_SIZE (V4L2_CID_CAMERA_CLASS_BASE+34) + +Double empty line
include/uapi/linux/videodev2.h struct v4l2_fract { · line 425
Quoted 12 lines
include/uapi/linux/v4l2-controls.h +0
-0/* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)include/uapi/linux/videodev2.h +1
-0@@ -422,6 +422,11 @@ struct v4l2_fract {__u32 denominator; }; +struct v4l2_pixel_size {I wonder if instead of defining a 'pixel size' this shouldn't be a more generic
v4l2_sizeor something similar. Or even av4l2_rectwith left=top=0.Quoted 8 lines
include/uapi/linux/videodev2.h +4
-0+__u32 width; +__u32 height; +}; +/** * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP * include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1730
Quoted 8 lines
include/uapi/linux/videodev2.h +5
-0@@ -1718,6 +1723,12 @@ enum v4l2_ctrl_type {V4L2_CTRL_TYPE_U8 =0x0100 ,V4L2_CTRL_TYPE_U16 =0x0101 ,V4L2_CTRL_TYPE_U32 =0x0102 , +/* +* V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103, +* V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104, +* V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105, + */leftovers ?
Quoted 14 lines
include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1731
Quoted 1 line
include/uapi/linux/videodev2.h +1
-0+ V4L2_CTRL_TYPE_PIXEL_SIZE = 0x0106,I don't see other compound controls with a custom payload adding their types here. What am I missing?
Thanks
jQuoted 6 lines
include/uapi/linux/videodev2.h +0
-1};/* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */ --2.23.0.rc1
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAl1dbUYACgkQcjQGjxah VjzMSg//T3Z0Pn/MM4rl0d9E08+UhovwymM92g/k2giBnTTDmLyZSanh4wJ5z95z CcySqDqSdsgk4oe7BSMR3+kaK/O0M9o7wkzNUIzpgT0H2zPIkhLo4rkYN97W2XCj eVIcUkdK0YSRpJLUYXP4PyTYMNhwwmP8ugCtfnhdMnVP5ly5H/jHcH9PVlBUVpaq qrZVRmfzX7N6jt5L3QE7EMXnH0OouQuZRE7c0RqcRYkFwtsyDtu7Wc9cfAxQGKW2 Udkx387YTsLGTvh3TgVyiR28fonYxfo48lj8YPM49O53stwsqgMeg667UJ6HPzko RY6QUwbz9Yo5ThnMhfIRqIP88wPTYyd+gls8Hs6PJ+LZx5AGYavfBvtWI15NuRSc +VKrN9s2pqrO4SJsZvtxR0P4he6SeGPNDjozLv2rgtrJDlWstPKe97fwoRWBhFwd xb8dHGD6LD5X6bDiJNWFNs5gMti3lMH92CfXC36S2OktV40EVAXWQCryQWohH3cm hvFbS8odj2AvXP+1ogErP5QgZe4iI+kDct1kUxt1TjE3iWjbvIENpjgTU9WUvM70 VY6YcW58J1vk+DZHORiEbB5lA/8cDeoWYSVTROKq9FPrfGxOybwzLzcVfo5N37QO JaPhMfm1TLiecZV2w8KdpdfHsEJF3QEGDy9pBztLV/B8FCw7xA0= =+EdP
-----END PGP SIGNATURE-----