From: Ricardo Ribalda Delgado <ribalda@kernel.org>
To: Jacopo Mondi <jacopo@jmondi.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
linux-media <linux-media@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] media: add pixel_size control
Date: Wed, 21 Aug 2019 18:35:22 +0200 [thread overview]
Message-ID: <CAPybu_3bttSTcDgEuyR0G2gsQKe7gwwU2fu=eAH0Yj+FH98XJQ@mail.gmail.com> (raw)
In-Reply-To: <20190821161150.hcbxzxcjvbkoqyb3@uno.localdomain>
[-- Attachment #1: Type: text/plain, Size: 13739 bytes --]
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@jmondi.org> 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@kernel.org>
> > ---
> > 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@jmondi.org> 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@kernel.org>
> > ---
> > 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 #2: signature.asc --]
[-- Type: text/plain, Size: 849 bytes --]
-----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-----
prev parent reply other threads:[~2019-08-21 16:35 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-19 12:17 [PATCH 1/3] media: add pixel_size control Ricardo Ribalda Delgado
2019-08-19 12:17 ` [PATCH 2/3] Documentation: Describe V4L2_CID_PIXEL_SIZE Ricardo Ribalda Delgado
2019-08-19 13:42 ` Philipp Zabel
2019-08-19 13:44 ` Ricardo Ribalda Delgado
2019-08-19 12:17 ` [PATCH 3/3] media: imx214: Add new control with V4L2_CID_PIXEL_SIZE Ricardo Ribalda Delgado
2019-08-21 16:15 ` Jacopo Mondi
2019-08-21 16:31 ` Ricardo Ribalda Delgado
2019-08-22 6:58 ` Jacopo Mondi
2019-08-21 16:11 ` [PATCH 1/3] media: add pixel_size control Jacopo Mondi
2019-08-21 16:35 ` Ricardo Ribalda Delgado [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPybu_3bttSTcDgEuyR0G2gsQKe7gwwU2fu=eAH0Yj+FH98XJQ@mail.gmail.com' \
--to=ribalda@kernel.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo@jmondi.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).