| Message ID | 1379336058-31178-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.ribalda@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.ribalda@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hans As allways thank you very much for your comments. On Mon, Sep 30, 2013 at 12:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > 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.ribalda@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.ribalda@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. That was exactly the point. The helper function convert the multi_selection, ext_rect to the legacy struct. This way the drivers needed almost no modification, just a call to the helper at the beginning of the handler. Otherwise we need your get_rect helper, and then a set_rect helper at every exit. If you think this is the way, then lets do it. Right now there are not too many drivers that supports selection, so it is right time to make such a decisions. > > 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. Just one comment. On the bmp standard a negative height means that the image is upside down. With multiple selections it could be a nice thing to allow such a behavious, so one selection can be inverted (if all are inverted, I guess vflip is more correct). > > 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 Thank you very much Regards
On 09/30/2013 01:17 PM, Ricardo Ribalda Delgado wrote: > Hello Hans > > As allways thank you very much for your comments. > > On Mon, Sep 30, 2013 at 12:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> 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.ribalda@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.ribalda@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. > > That was exactly the point. The helper function convert the > multi_selection, ext_rect to the legacy struct. This way the drivers > needed almost no modification, just a call to the helper at the > beginning of the handler. That doesn't work: G and S_SELECTION are IOWR, so the driver can modify the rectangles and those will have to be passed back to userspace. So you cannot just change the contents of struct v4l2_selection. > > Otherwise we need your get_rect helper, and then a set_rect helper at > every exit. > > If you think this is the way, then lets do it. Right now there are not > too many drivers that supports selection, so it is right time to make > such a decisions. > >> >> 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. > > Just one comment. On the bmp standard a negative height means that the > image is upside down. With multiple selections it could be a nice > thing to allow such a behavious, so one selection can be inverted (if > all are inverted, I guess vflip is more correct). Negative width/height values are completely out of spec. No driver supports that, and as you say, we have vflip/hflip for mirroring. Regards, Hans > >> >> 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]; >>> }; >>> >>> >>> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hans I have just posted a new patch that only takes care of the core. Thanks! On Mon, Sep 30, 2013 at 2:11 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 09/30/2013 01:17 PM, Ricardo Ribalda Delgado wrote: >> Hello Hans >> >> As allways thank you very much for your comments. >> >> On Mon, Sep 30, 2013 at 12:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> 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.ribalda@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.ribalda@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. >> >> That was exactly the point. The helper function convert the >> multi_selection, ext_rect to the legacy struct. This way the drivers >> needed almost no modification, just a call to the helper at the >> beginning of the handler. > > That doesn't work: G and S_SELECTION are IOWR, so the driver can modify the > rectangles and those will have to be passed back to userspace. So you cannot > just change the contents of struct v4l2_selection. > >> >> Otherwise we need your get_rect helper, and then a set_rect helper at >> every exit. >> >> If you think this is the way, then lets do it. Right now there are not >> too many drivers that supports selection, so it is right time to make >> such a decisions. >> >>> >>> 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. >> >> Just one comment. On the bmp standard a negative height means that the >> image is upside down. With multiple selections it could be a nice >> thing to allow such a behavious, so one selection can be inverted (if >> all are inverted, I guess vflip is more correct). > > Negative width/height values are completely out of spec. No driver supports that, and > as you say, we have vflip/hflip for mirroring. > > Regards, > > Hans > >> >>> >>> 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]; >>>> }; >>>> >>>> >>>> >
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c index 40a73f7..599a907 100644 --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c @@ -452,6 +452,9 @@ static int gsc_m2m_g_selection(struct file *file, void *fh, (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + frame = ctx_get_frame(ctx, s->type); if (IS_ERR(frame)) return PTR_ERR(frame); @@ -495,6 +498,9 @@ static int gsc_m2m_s_selection(struct file *file, void *fh, (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + ret = gsc_try_crop(ctx, &cr); if (ret) return ret; diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index fb27ff7..357ac81 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -1283,6 +1283,9 @@ static int fimc_cap_g_selection(struct file *file, void *fh, if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + switch (s->target) { case V4L2_SEL_TGT_COMPOSE_DEFAULT: case V4L2_SEL_TGT_COMPOSE_BOUNDS: @@ -1333,6 +1336,9 @@ static int fimc_cap_s_selection(struct file *file, void *fh, if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + if (s->target == V4L2_SEL_TGT_COMPOSE) f = &ctx->d_frame; else if (s->target == V4L2_SEL_TGT_CROP) diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index 08fbfed..b895318 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -915,6 +915,9 @@ static int fimc_lite_g_selection(struct file *file, void *fh, if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + switch (sel->target) { case V4L2_SEL_TGT_COMPOSE_BOUNDS: case V4L2_SEL_TGT_COMPOSE_DEFAULT: @@ -944,6 +947,9 @@ static int fimc_lite_s_selection(struct file *file, void *fh, sel->target != V4L2_SEL_TGT_COMPOSE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + fimc_lite_try_compose(fimc, &rect); if ((sel->flags & V4L2_SEL_FLAG_LE) && diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c index 40b298a..951dce4 100644 --- a/drivers/media/platform/s3c-camif/camif-capture.c +++ b/drivers/media/platform/s3c-camif/camif-capture.c @@ -1016,6 +1016,9 @@ static int s3c_camif_g_selection(struct file *file, void *priv, if (sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + switch (sel->target) { case V4L2_SEL_TGT_COMPOSE_BOUNDS: case V4L2_SEL_TGT_COMPOSE_DEFAULT: @@ -1057,6 +1060,9 @@ static int s3c_camif_s_selection(struct file *file, void *priv, sel->target != V4L2_SEL_TGT_COMPOSE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + __camif_try_compose(camif, vp, &rect); sel->r = rect; diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c index 15d2396..6f46869 100644 --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c @@ -848,6 +848,9 @@ static int s5p_jpeg_g_selection(struct file *file, void *priv, s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + /* For JPEG blob active == default == bounds */ switch (s->target) { case V4L2_SEL_TGT_CROP: diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c index 641b1f0..52a8de9 100644 --- a/drivers/media/platform/s5p-tv/mixer_video.c +++ b/drivers/media/platform/s5p-tv/mixer_video.c @@ -368,6 +368,9 @@ static int mxr_g_selection(struct file *file, void *fh, s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + switch (s->target) { case V4L2_SEL_TGT_CROP: s->r.left = geo->src.x_offset; @@ -436,6 +439,9 @@ static int mxr_s_selection(struct file *file, void *fh, s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + switch (s->target) { /* ignore read-only targets */ case V4L2_SEL_TGT_CROP_DEFAULT: diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index 2dd0e52..c84f4e3 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -1061,6 +1061,9 @@ static int soc_camera_g_selection(struct file *file, void *fh, if (!ici->ops->get_selection) return -ENOTTY; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + return ici->ops->get_selection(icd, s); } @@ -1077,6 +1080,9 @@ static int soc_camera_s_selection(struct file *file, void *fh, s->target != V4L2_SEL_TGT_CROP)) return -EINVAL; + if (v4l2_selection_flat_struct(s)) + return -EINVAL; + if (s->target == V4L2_SEL_TGT_COMPOSE) { /* No output size change during a running capture! */ if (is_streaming(ici, icd) && 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; + 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, }; 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; + + 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{ + struct v4l2_rect r; + struct v4l2_ext_rect *pr; + }; + __u32 rectangles; + __u32 reserved[7]; }; 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]; +}; + 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{ + struct v4l2_rect r; + struct v4l2_ext_rect *pr; + }; + __u32 rectangles; + __u32 reserved[8]; };