Re: [PATCH 1/3] media: add pixel_size control

From: Ricardo Ribalda Delgado
Date: Wed Aug 21 2019 - 12:35:46 EST

Next message: Will Deacon: "Re: [PATCH v10 09/23] iommu/io-pgtable-arm-v7s: Extend to support PA[33:32] for MediaTek"
Previous message: Nadav Amit: "Re: [PATCH] mm/balloon_compaction: suppress allocation warnings"
In reply to: Jacopo Mondi: "Re: [PATCH 1/3] media: add pixel_size control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jacopo

[sorry for the multiple send, working on a train and fighting with the
mail client and the 4g connection :S]

On Wed, 21 Aug 2019, 18:10 Jacopo Mondi, <jacopo@xxxxxxxxxx> wrote:
>
> 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

Please review v2. It has changed from pixel_size to unit size, which
is more clear.

>
>
> >
> > /* 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...

Sure. will do on v3

>
> > 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

Thanks , will fix in v3
>
> > /* 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.

Fixed on v2

>
> > + __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 ?
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

>
> > + 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!!

On Wed, Aug 21, 2019 at 6:10 PM Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
>
> 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
> >
-----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-----

Next message: Will Deacon: "Re: [PATCH v10 09/23] iommu/io-pgtable-arm-v7s: Extend to support PA[33:32] for MediaTek"
Previous message: Nadav Amit: "Re: [PATCH] mm/balloon_compaction: suppress allocation warnings"
In reply to: Jacopo Mondi: "Re: [PATCH 1/3] media: add pixel_size control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]