Hi Ricardo,
On Mon, Aug 19, 2019 at 02:17:18PM +0200, Ricardo Ribalda Delgado wrote:
> 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 <ribalda@xxxxxxxxxx>
> ---
> 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(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index cd1ae016706f..a3a0086c96ff 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -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
>
> /* 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?
> + 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...
> 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);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..63de780398b8 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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;
> };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..13f0410df4c6 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -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
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2427bc4d8eba..21f4846dca0b 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -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.
> + __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,
> + */
leftovers ?
> + 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
> };
>
> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> --
> 2.23.0.rc1
>
Attachment:
signature.asc
Description: PGP signature
