Hi Ricardo,
Sorry for the delayed review, but I'm finally catching up with my backlog.
I've got a number of comments regarding this patch. I'm ignoring the platform
driver patches for now until the core support is correct.
On 09/16/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda <ricardo.riba...@gmail.com>
>
> Extend v4l2 selection API to support multiple selection areas, this way
> sensors that support multiple readout areas can work with multiple areas
> of insterest.
>
> The struct v4l2_selection and v4l2_subdev_selection has been extented
> with a new field rectangles. If it is value is different than zero the
> pr array is used instead of the r field.
>
> A new structure v4l2_ext_rect has been defined, containing 4 reserved
> fields for future improvements, as suggested by Hans.
>
> A new function in v4l2-comon (v4l2_selection_flat_struct) is in charge
> of converting a pr pointer with one item to a flatten struct. This
> function is used in all the old drivers that dont support multiple
> selections.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.riba...@gmail.com>
> ---
> drivers/media/platform/exynos-gsc/gsc-m2m.c | 6 +++
> drivers/media/platform/exynos4-is/fimc-capture.c | 6 +++
> drivers/media/platform/exynos4-is/fimc-lite.c | 6 +++
> drivers/media/platform/s3c-camif/camif-capture.c | 6 +++
> drivers/media/platform/s5p-jpeg/jpeg-core.c | 3 ++
> drivers/media/platform/s5p-tv/mixer_video.c | 6 +++
> drivers/media/platform/soc_camera/soc_camera.c | 6 +++
> drivers/media/v4l2-core/v4l2-common.c | 13 ++++++
> drivers/media/v4l2-core/v4l2-ioctl.c | 54
> +++++++++++++++++++++---
> include/media/v4l2-common.h | 2 +
> include/uapi/linux/v4l2-subdev.h | 10 ++++-
> include/uapi/linux/videodev2.h | 15 ++++++-
> 12 files changed, 122 insertions(+), 11 deletions(-)
>
<snip>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c
> index a95e5e2..cd20567 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -886,3 +886,16 @@ void v4l2_get_timestamp(struct timeval *tv)
> tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> }
> EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> +
> +int v4l2_selection_flat_struct(struct v4l2_selection *s)
> +{
> + if (s->rectangles == 0)
> + return 0;
> +
> + if (s->rectangles != 1)
> + return -EINVAL;
> +
> + s->r = s->pr[0].r;
This would overwrite the pr pointer. Not a good idea.
I would make a helper function like this:
int v4l2_selection_get_rect(struct v4l2_selection *s, struct v4l2_ext_rect *r)
{
if (s->rectangles > 1)
return -EINVAL;
if (s->rectangles == 1) {
*r = s->pr[0];
return 0;
}
if (s->r.width < 0 || s->r.height < 0)
return -EINVAL;
r->left = s->r.left;
r->top = s->r.top;
r->width = s->r.width;
r->height = s->r.height;
memset(r->reserved, 0, sizeof(r->reserved));
return 0;
}
See also my proposed v4l2_ext_rect definition below.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_selection_flat_struct);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..fe92f6b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -572,11 +572,22 @@ static void v4l_print_crop(const void *arg, bool
> write_only)
> static void v4l_print_selection(const void *arg, bool write_only)
> {
> const struct v4l2_selection *p = arg;
> + int i;
>
> - pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> - prt_names(p->type, v4l2_type_names),
> - p->target, p->flags,
> - p->r.width, p->r.height, p->r.left, p->r.top);
> + if (p->rectangles == 0)
> + pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d,
> x,y=%d,%d\n",
> + prt_names(p->type, v4l2_type_names),
> + p->target, p->flags,
> + p->r.width, p->r.height, p->r.left, p->r.top);
> + else{
> + pr_cont("type=%s, target=%d, flags=0x%x rectangles=%d\n",
> + prt_names(p->type, v4l2_type_names),
> + p->target, p->flags, p->rectangles);
> + for (i = 0; i < p->rectangles; i++)
> + pr_cont("rectangle %d: wxh=%dx%d, x,y=%d,%d\n",
> + i, p->r.width, p->r.height,
> + p->r.left, p->r.top);
> + }
> }
>
> static void v4l_print_jpegcompression(const void *arg, bool write_only)
> @@ -1645,6 +1656,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> struct v4l2_crop *p = arg;
> struct v4l2_selection s = {
> .type = p->type,
> + .rectangles = 0,
No need for this. In initializers like this fields that aren't initialized
explicitly will be zeroed by the compiler.
> };
> int ret;
>
> @@ -1673,6 +1685,7 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
> struct v4l2_selection s = {
> .type = p->type,
> .r = p->c,
> + .rectangles = 0,
> };
>
> if (ops->vidioc_s_crop)
> @@ -1692,7 +1705,10 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops
> *ops,
> struct file *file, void *fh, void *arg)
> {
> struct v4l2_cropcap *p = arg;
> - struct v4l2_selection s = { .type = p->type };
> + struct v4l2_selection s = {
> + .type = p->type,
> + .rectangles = 0,
> + };
> int ret;
>
> if (ops->vidioc_cropcap)
> @@ -1726,6 +1742,30 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops
> *ops,
> return 0;
> }
>
> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_selection *s = arg;
> +
> + if (s->rectangles &&
> + !access_ok(VERIFY_READ, s->pr, s->rectangles * sizeof(*s->pr)))
> + return -EFAULT;
No, this isn't necessary. Instead add support for the selection array to
check_array_args() in v4l2-ioctl.c. That's where this should be handled.
video_usercopy() will then do the copy_from/to_user for you and drivers don't
need to care about it.
Note that you also need to update v4l2-compat-ioctl32.c!
> +
> + return ops->vidioc_s_selection(file, fh, s);
> +}
> +
> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
> + struct file *file, void *fh, void *arg)
> +{
> + struct v4l2_selection *s = arg;
> +
> + if (s->rectangles &&
> + !access_ok(VERIFY_WRITE, s->pr, s->rectangles * sizeof(*s->pr)))
> + return -EFAULT;
> +
> + return ops->vidioc_g_selection(file, fh, s);
> +}
> +
> static int v4l_log_status(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> @@ -2018,8 +2058,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
> IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap,
> INFO_FL_CLEAR(v4l2_cropcap, type)),
> IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop,
> INFO_FL_CLEAR(v4l2_crop, type)),
> IOCTL_INFO_FNC(VIDIOC_S_CROP, v4l_s_crop, v4l_print_crop, INFO_FL_PRIO),
> - IOCTL_INFO_STD(VIDIOC_G_SELECTION, vidioc_g_selection,
> v4l_print_selection, 0),
> - IOCTL_INFO_STD(VIDIOC_S_SELECTION, vidioc_s_selection,
> v4l_print_selection, INFO_FL_PRIO),
> + IOCTL_INFO_FNC(VIDIOC_G_SELECTION, v4l_g_selection,
> v4l_print_selection, 0),
> + IOCTL_INFO_FNC(VIDIOC_S_SELECTION, v4l_s_selection,
> v4l_print_selection, INFO_FL_PRIO),
> IOCTL_INFO_STD(VIDIOC_G_JPEGCOMP, vidioc_g_jpegcomp,
> v4l_print_jpegcompression, 0),
> IOCTL_INFO_STD(VIDIOC_S_JPEGCOMP, vidioc_s_jpegcomp,
> v4l_print_jpegcompression, INFO_FL_PRIO),
> IOCTL_INFO_FNC(VIDIOC_QUERYSTD, v4l_querystd, v4l_print_std, 0),
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 015ff82..b0a3d2b 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -216,4 +216,6 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8
> hor_landscape, u8 vert_portrait);
>
> void v4l2_get_timestamp(struct timeval *tv);
>
> +int v4l2_selection_flat_struct(struct v4l2_selection *s);
> +
> #endif /* V4L2_COMMON_H_ */
> diff --git a/include/uapi/linux/v4l2-subdev.h
> b/include/uapi/linux/v4l2-subdev.h
> index a33c4da..b5ee08b 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -133,6 +133,8 @@ struct v4l2_subdev_frame_interval_enum {
> * defined in v4l2-common.h; V4L2_SEL_TGT_* .
> * @flags: constraint flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
> * @r: coordinates of the selection window
> + * @pr: array of rectangles containing the selection windows
> + * @rectangles: Number of rectangles in pr structure. If zero, r is
> used instead
> * @reserved: for future use, set to zero for now
> *
> * Hardware may use multiple helper windows to process a video stream.
> @@ -144,8 +146,12 @@ struct v4l2_subdev_selection {
> __u32 pad;
> __u32 target;
> __u32 flags;
> - struct v4l2_rect r;
> - __u32 reserved[8];
> + union{
Add space after 'union'.
> + struct v4l2_rect r;
> + struct v4l2_ext_rect *pr;
> + };
> + __u32 rectangles;
> + __u32 reserved[7];
> };
I suspect this should get the packed attribute. It's a good idea anyone to add
that,
but we have to check that the sizeof(struct v4l2_subdev_selection) doesn't
change
for both 32 and 64 bit compilations.
>
> struct v4l2_subdev_edid {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 95ef455..db8b1a5 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -211,6 +211,11 @@ struct v4l2_rect {
> __s32 height;
> };
>
> +struct v4l2_ext_rect {
> + struct v4l2_rect r;
> + __u32 reserved[4];
> +};
I actually would prefer this:
struct v4l2_ext_rect {
__s32 left;
__s32 top;
__u32 width;
__u32 height;
__u32 reserved[4];
};
It has always annoyed me that width and height were signed, and this fixes that
problem.
This is also why I was using v4l2_ext_rect in the selection helper function:
that way
drivers do not need to check for negative width/height values.
> +
> struct v4l2_fract {
> __u32 numerator;
> __u32 denominator;
> @@ -804,6 +809,8 @@ struct v4l2_crop {
> * defined in v4l2-common.h; V4L2_SEL_TGT_* .
> * @flags: constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
> * @r: coordinates of selection window
> + * @pr: array of rectangles containing the selection windows
> + * @rectangles: Number of rectangles in pr structure. If zero, r is
> used instead
> * @reserved: for future use, rounds structure size to 64 bytes, set
> to zero
> *
> * Hardware may use multiple helper windows to process a video stream.
> @@ -814,8 +821,12 @@ struct v4l2_selection {
> __u32 type;
> __u32 target;
> __u32 flags;
> - struct v4l2_rect r;
> - __u32 reserved[9];
> + union{
Add space after 'union'.
> + struct v4l2_rect r;
> + struct v4l2_ext_rect *pr;
> + };
> + __u32 rectangles;
> + __u32 reserved[8];
> };
>
>
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
