| Message ID | 1380623614-26265-1-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ping? Any comment about this version? Shall I post a new one with the modified device drivers? Thanks! On Tue, Oct 1, 2013 at 12:33 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > 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. > > Two helper functiona are also added, to help the drivers that support > a single selection. > > This Patch ONLY adds the modification to the core. Once it is agreed, a > new version including changes on the drivers that handle the selection > api will come. > > This patch includes all the comments by Hans Verkuil. > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > v3: > -Changes on compat-ioctl32 > -Remove checks on v4l2_selection_set_rect > > drivers/media/v4l2-core/v4l2-common.c | 36 +++++++++++++++ > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63 +++++++++++++++++++++++++++ > drivers/media/v4l2-core/v4l2-ioctl.c | 37 +++++++++++++--- > include/media/v4l2-common.h | 6 +++ > include/uapi/linux/v4l2-subdev.h | 10 ++++- > include/uapi/linux/videodev2.h | 21 +++++++-- > 6 files changed, 162 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index a95e5e2..f60a2ce 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -886,3 +886,39 @@ 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_get_rect(const 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; > +} > +EXPORT_SYMBOL_GPL(v4l2_selection_get_rect); > + > +void v4l2_selection_set_rect(struct v4l2_selection *s, > + const struct v4l2_ext_rect *r) > +{ > + if (s->rectangles == 0) { > + s->r.left = r->left; > + s->r.top = r->top; > + s->r.width = r->width; > + s->r.height = r->height; > + return; > + } > + s->pr[0] = *r; > + s->rectangles = 1; > + return; > +} > +EXPORT_SYMBOL_GPL(v4l2_selection_set_rect); > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 8f7a6a4..36ed3c3 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde > return 0; > } > > +struct v4l2_selection32 { > + __u32 type; > + __u32 target; > + __u32 flags; > + union { > + struct v4l2_rect r; > + compat_uptr_t *pr; > + }; > + __u32 rectangles; > + __u32 reserved[8]; > +} __packed; > + > +static int get_v4l2_selection32(struct v4l2_selection *kp, > + struct v4l2_selection32 __user *up) > +{ > + if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32)) > + || (copy_from_user(kp, up, sizeof(*kp)))) > + return -EFAULT; > + > + if (kp->rectangles) { > + compat_uptr_t usr_ptr; > + if (get_user(usr_ptr, &up->pr)) > + return -EFAULT; > + kp->pr = compat_ptr(usr_ptr); > + } > + > + return 0; > + > +} > + > +static int put_v4l2_selection32(struct v4l2_selection *kp, > + struct v4l2_selection32 __user *up) > +{ > + compat_uptr_t usr_ptr = 0; > + > + if ((kp->rectangles) && get_user(usr_ptr, &up->pr)) > + return -EFAULT; > + > + if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32)) > + || (copy_to_user(kp, up, sizeof(*kp)))) > + return -EFAULT; > + > + if (kp->rectangles) > + put_user(usr_ptr, &up->pr); > + > + return 0; > + > +} > > #define VIDIOC_G_FMT32 _IOWR('V', 4, struct v4l2_format32) > #define VIDIOC_S_FMT32 _IOWR('V', 5, struct v4l2_format32) > @@ -796,6 +844,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde > #define VIDIOC_DQEVENT32 _IOR ('V', 89, struct v4l2_event32) > #define VIDIOC_CREATE_BUFS32 _IOWR('V', 92, struct v4l2_create_buffers32) > #define VIDIOC_PREPARE_BUF32 _IOWR('V', 93, struct v4l2_buffer32) > +#define VIDIOC_G_SELECTION32 _IOWR('V', 94, struct v4l2_selection32) > +#define VIDIOC_S_SELECTION32 _IOWR('V', 95, struct v4l2_selection32) > > #define VIDIOC_OVERLAY32 _IOW ('V', 14, s32) > #define VIDIOC_STREAMON32 _IOW ('V', 18, s32) > @@ -817,6 +867,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > struct v4l2_event v2ev; > struct v4l2_create_buffers v2crt; > struct v4l2_subdev_edid v2edid; > + struct v4l2_selection v2sel; > unsigned long vx; > int vi; > } karg; > @@ -851,6 +902,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break; > case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break; > case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break; > + case VIDIOC_G_SELECTION32: cmd = VIDIOC_G_SELECTION; break; > + case VIDIOC_S_SELECTION32: cmd = VIDIOC_S_SELECTION; break; > } > > switch (cmd) { > @@ -922,6 +975,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_DQEVENT: > compatible_arg = 0; > break; > + case VIDIOC_G_SELECTION: > + case VIDIOC_S_SELECTION: > + err = get_v4l2_selection32(&karg.v2sel, up); > + compatible_arg = 0; > + break; > } > if (err) > return err; > @@ -994,6 +1052,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > case VIDIOC_ENUMINPUT: > err = put_v4l2_input32(&karg.v2i, up); > break; > + > + case VIDIOC_G_SELECTION: > + case VIDIOC_S_SELECTION: > + err = put_v4l2_selection32(&karg.v2sel, up); > + break; > } > return err; > } > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 68e6b5e..aa1c2a4 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) > @@ -1692,7 +1703,9 @@ 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, > + }; > int ret; > > if (ops->vidioc_cropcap) > @@ -2253,6 +2266,20 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > } > break; > } > + > + case VIDIOC_G_SELECTION: > + case VIDIOC_S_SELECTION: { > + struct v4l2_selection *s = parg; > + > + if (s->rectangles) { > + *user_ptr = (void __user *)s->pr; > + *kernel_ptr = (void *)&s->pr; > + *array_size = sizeof(struct v4l2_ext_rect) > + * s->rectangles; > + ret = 1; > + } > + break; > + } > } > > return ret; > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > index 015ff82..417ab82 100644 > --- a/include/media/v4l2-common.h > +++ b/include/media/v4l2-common.h > @@ -216,4 +216,10 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait); > > void v4l2_get_timestamp(struct timeval *tv); > > +int v4l2_selection_get_rect(const struct v4l2_selection *s, > + struct v4l2_ext_rect *r); > + > +void v4l2_selection_set_rect(struct v4l2_selection *s, > + const struct v4l2_ext_rect *r); > + > #endif /* V4L2_COMMON_H_ */ > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index a33c4da..c02a886 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..a4a7902 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -211,6 +211,14 @@ struct v4l2_rect { > __s32 height; > }; > > +struct v4l2_ext_rect { > + __s32 left; > + __s32 top; > + __u32 width; > + __u32 height; > + __u32 reserved[4]; > +}; > + > struct v4l2_fract { > __u32 numerator; > __u32 denominator; > @@ -804,6 +812,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,10 +824,13 @@ 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]; > +} __packed; > > /* > * A N A L O G V I D E O S T A N D A R D > -- > 1.8.4.rc3 >
Hi Ricardo, I am the designer of selection API. I hope I can help you a little. I think that there are two issues mixed in 'Mulitple selections' topic. Firstly, you described that you program a piece of hardware that is capable of selecting 8 areas for scanning. Now you are looking for userspace API to support such a feature. The feature of posting multiple rectangle was proposed in this RFC. Secondly, You introduced struct v4l2_ext_rect which is a future-proof version of v4l2_rect. I think that both issues should be solved in two separate patchsets. Ad 1. The selection of multiple scanning areas is a very driver-specific feature, isn't it? I think that you do not need to introduce any abstract interface. What would be other applications of the proposed interface? Do you know other drivers that may need it? Sakari mentioned introduction of private targets for selections. I like this idea. Just define: #define V4L2_SEL_TGT_PRIVATE 0x80000000 All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. Generic applications must not use them. Non-generic application must check out the driver of video node before using selections from private set. If some target becomes more useful and accepted by more then one driver then it can be moved to generic API. The good thing about private target is that enums from different drivers can collide so the target space is not going to be trashed. But how to deal with multiple rectangles? I have an auxiliary question. Do you have to set all rectangles at once? can you set up them one by one? Anyway, first try to define something like this: #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) ... -- OR-- parametrized macros similar to one below: #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) The application could setup all scanout areas one-by-one. By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. The height of all consecutive area would be 0. This means disabling them effectively. The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive rectangle by shifting them down or resetting them to height 0. Notice that as long as targets are driver specific you are free do define interaction between the targets. I hope that proposed solution is satisfactory. BTW. I think that the HW trick you described is not cropping. By cropping you select which part of sensor area is going to be processed into compose rectangle in a buffer. So technicaly you still insert the whole sensor area into the buffer. Only part of the buffer is actually updated. So there is no cropping (cropping area is equal to the whole array). Notice, that every 'cropping' area goes to different part of a buffer. So you would need also muliple targets for composing (!!!) and very long chapter in V4L2 doc describing interactions between muliple-rectangle crops and compositions. Good luck !!! :). Currently it is a hell to understand and define interaction between single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. I strongly recommend to use private selections. It will be much simpler to define, implement, and modify if needed. BTW2. I think that the mulitple scanout areas can be modelled using media device API. Sakari may know how to do this. Ad 2. Extended rectangles. It is a good idea because v4l2_rect lacks any place for extensions. But before adding it to V4L2 API, I would like to know the examples of actual applications. Please, point drivers that actually need it. Other thing worth mentioning is reservation of few bits from v4l2_selection::flags to describe the type of data used for rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid introducting v4l2_selection::rectangles field. I hope you find this comments useful. Regards, Tomasz Stanislawski -- 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 Tomasz Sorry for the late reply, but I have been offline the last week due to the conference. On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > Hi Ricardo, > I am the designer of selection API. I hope I can help you a little. > I think that there are two issues mixed in 'Mulitple selections' topic. > > Firstly, you described that you program a piece of hardware that is > capable of selecting 8 areas for scanning. Now you > are looking for userspace API to support such a feature. > The feature of posting multiple rectangle was proposed in this RFC. > > Secondly, You introduced struct v4l2_ext_rect which is a future-proof > version of v4l2_rect. > > > I think that both issues should be solved in two separate patchsets. > > Ad 1. > The selection of multiple scanning areas is a very driver-specific > feature, isn't it? I think that you do not need to introduce any abstract > interface. What would be other applications of the proposed interface? It is not driver specific. There are many sensors out there that supports multiple window of interest, but today we are ignoring them just because we dont have an api. The main application would be industrial imaging, where less data to read means more fps and therefore the system can run faster. From my field I can tell you that it is a hard requirement for computer vision. And it is a feature that we cannot model through v4l2 controls. > Do you know other drivers that may need it? Sakari mentioned introduction > of private targets for selections. I like this idea. Just define: > > #define V4L2_SEL_TGT_PRIVATE 0x80000000 > > All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. > Generic applications must not use them. Non-generic application > must check out the driver of video node before using selections > from private set. If some target becomes more useful and accepted > by more then one driver then it can be moved to generic API. > The good thing about private target is that enums from different > drivers can collide so the target space is not going to be trashed. > If you read the previous RFCs you will see that the approach you are mentioning has been rejected. The main issue is that you cannot set atomically all the rectangles. Lets say that the configuration formed by rectangle A, B and C is legal, but the configuration A and B is not allowed by the sensor. How could you set the rectangles one by one? > I have an auxiliary question. Do you have to set all rectangles > at once? can you set up them one by one? Also if you tell the driver what exact configuration you will need, it will provide you with the closest possible confuration, that cannot be done if you provide rectangle by rectangle. > But how to deal with multiple rectangles? Multiple rectangles is a desired feature, please take a look to the presentation on the workshop. > > Anyway, first try to define something like this: > > #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE > #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) > #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) > > #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) > ... > > -- OR-- parametrized macros similar to one below: > > #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) > > The application could setup all scanout areas one-by-one. > By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. > The height of all consecutive area would be 0. This means disabling > them effectively. Lets say rectangle A + B + C +D is legal, and A +B is also legal. You are in ABCD and you want to go to AB. How can you do it? If yo dissable C or D, the configuration is ilegal and therefor the driver will return -EINVAL. So once you are in ABCD you cannot go back... > > The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive > rectangle by shifting them down or resetting them to height 0. > Notice that as long as targets are driver specific you are free do define > interaction between the targets. > > I hope that proposed solution is satisfactory. As stated before, please follow the previous comments on the rfc, specially the ones from Hans. > > BTW. I think that the HW trick you described is not cropping. > By cropping you select which part of sensor area is going > to be processed into compose rectangle in a buffer. You are selecting part of the sensor, therefore you are cropping the image. > > So technicaly you still insert the whole sensor area into the buffer. Only the lines/columns are read into the buffer. > Only part of the buffer is actually updated. So there is no cropping > (cropping area is equal to the whole array). > > Notice, that every 'cropping' area goes to different part of a buffer. > So you would need also muliple targets for composing (!!!) and very long > chapter in V4L2 doc describing interactions between muliple-rectangle > crops and compositions. Good luck !!! :). It is not that difficult to describe. You need the same ammount of rectangles in cropping and in compossing. Rectangle X in cropping will be mapped to rectangle X in compose. > Currently it is a hell to understand and define interaction between > single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. On m2m devices we are only lacking a s_fmt on the first buffer, as we have discussed on the workshop. I think we only lack a good reference model in vivi. > > I strongly recommend to use private selections. > It will be much simpler to define, implement, and modify if needed. I think the private selections will lead to specific applications for specific drivers and we cannot support all the configurations with them. Also there is no way for an app to enumerate that capability. > > BTW2. I think that the mulitple scanout areas can be modelled using > media device API. Sakari may know how to do this. The areas are not read from the sensor. Therefore they can only be proccessed on the sensor subdevice. > > > Ad 2. Extended rectangles. > It is a good idea because v4l2_rect lacks any place for extensions. > But before adding it to V4L2 API, I would like to know the examples > of actual applications. Please, point drivers that actually need it. I have no need for it, but now that we are extending the selection API it would be the perfect moment to add them. They could describe properties of the sensor, like tracking ids. > > Other thing worth mentioning is reservation of few bits from > v4l2_selection::flags to describe the type of data used for > rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid > introducting v4l2_selection::rectangles field. > > I hope you find this comments useful. Regards > > Regards, > Tomasz Stanislawski > >
Hi Tomasz, (Please also see the notes from the Media summit Hans posted.) Tomasz Stanislawski wrote: > Hi Ricardo, > I am the designer of selection API. I hope I can help you a little. > I think that there are two issues mixed in 'Mulitple selections' topic. > > Firstly, you described that you program a piece of hardware that is > capable of selecting 8 areas for scanning. Now you > are looking for userspace API to support such a feature. > The feature of posting multiple rectangle was proposed in this RFC. > > Secondly, You introduced struct v4l2_ext_rect which is a future-proof > version of v4l2_rect. > > > I think that both issues should be solved in two separate patchsets. > > Ad 1. > The selection of multiple scanning areas is a very driver-specific > feature, isn't it? I think that you do not need to introduce any abstract > interface. What would be other applications of the proposed interface? > Do you know other drivers that may need it? Sakari mentioned introduction > of private targets for selections. I like this idea. Just define: > > #define V4L2_SEL_TGT_PRIVATE 0x80000000 I think a private target would definitely make sense if this was functionality that was present on a single sensor or two --- that's what I actually proposed for this originally. But as far as I understand, it is more common but just not present on sensors used in mobile devices. So standardising this makes sense. > All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. > Generic applications must not use them. Non-generic application > must check out the driver of video node before using selections > from private set. If some target becomes more useful and accepted > by more then one driver then it can be moved to generic API. > The good thing about private target is that enums from different > drivers can collide so the target space is not going to be trashed. > > But how to deal with multiple rectangles? > I have an auxiliary question. Do you have to set all rectangles > at once? can you set up them one by one? > > Anyway, first try to define something like this: > > #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE > #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) > #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) > > #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) > ... > > -- OR-- parametrized macros similar to one below: > > #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) > > The application could setup all scanout areas one-by-one. > By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. > The height of all consecutive area would be 0. This means disabling > them effectively. > > The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive > rectangle by shifting them down or resetting them to height 0. > Notice that as long as targets are driver specific you are free do define > interaction between the targets. > > I hope that proposed solution is satisfactory. > > BTW. I think that the HW trick you described is not cropping. > By cropping you select which part of sensor area is going > to be processed into compose rectangle in a buffer. > > So technicaly you still insert the whole sensor area into the buffer. > Only part of the buffer is actually updated. So there is no cropping > (cropping area is equal to the whole array). > > Notice, that every 'cropping' area goes to different part of a buffer. > So you would need also muliple targets for composing (!!!) and very long > chapter in V4L2 doc describing interactions between muliple-rectangle > crops and compositions. Good luck !!! :). Multiple crop rectangles shouldn't make that more difficult than it was since for the next step the image size is still a rectangle (it is just composed of several smaller ones). Drivers supporting this will suffer, though, but others shouldn't need to care. The documentation must thus also be changed to say that the crop rectangles must together form a rectangle if multiple rectangle support is added. I reckon Ricardo is looking forward to using this on V4L2 interface, but I think support should also be added to the V4L2 sub-device interface. > Currently it is a hell to understand and define interaction between > single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. > > I strongly recommend to use private selections. > It will be much simpler to define, implement, and modify if needed. > > BTW2. I think that the mulitple scanout areas can be modelled using > media device API. Sakari may know how to do this. The media device plays no part in subsystem specific matters such as formats. This is relevant in the scope of V4L2 only, IMHO.
Hi Ricardo, Sorry for a late reply. I've been 'offline' for the last two weeks. Please refer to the comments below. On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote: > Hello Tomasz > > Sorry for the late reply, but I have been offline the last week due to > the conference. > > > On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski > <t.stanislaws@samsung.com> wrote: >> Hi Ricardo, >> I am the designer of selection API. I hope I can help you a little. >> I think that there are two issues mixed in 'Mulitple selections' topic. >> >> Firstly, you described that you program a piece of hardware that is >> capable of selecting 8 areas for scanning. Now you >> are looking for userspace API to support such a feature. >> The feature of posting multiple rectangle was proposed in this RFC. >> >> Secondly, You introduced struct v4l2_ext_rect which is a future-proof >> version of v4l2_rect. >> >> >> I think that both issues should be solved in two separate patchsets. >> >> Ad 1. >> The selection of multiple scanning areas is a very driver-specific >> feature, isn't it? I think that you do not need to introduce any abstract >> interface. What would be other applications of the proposed interface? > > It is not driver specific. There are many sensors out there that > supports multiple window of interest, but today we are ignoring them > just because we dont have an api. > > The main application would be industrial imaging, where less data to > read means more fps and therefore the system can run faster. > > From my field I can tell you that it is a hard requirement for > computer vision. And it is a feature that we cannot model through v4l2 > controls. > > OK. So there is no need to implement this feature as a driver-specific API. It can go automatically to generic API. >> Do you know other drivers that may need it? Sakari mentioned introduction >> of private targets for selections. I like this idea. Just define: >> >> #define V4L2_SEL_TGT_PRIVATE 0x80000000 >> >> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. >> Generic applications must not use them. Non-generic application >> must check out the driver of video node before using selections >> from private set. If some target becomes more useful and accepted >> by more then one driver then it can be moved to generic API. >> The good thing about private target is that enums from different >> drivers can collide so the target space is not going to be trashed. >> > > If you read the previous RFCs you will see that the approach you are > mentioning has been rejected. > > The main issue is that you cannot set atomically all the rectangles. > Lets say that the configuration formed by rectangle A, B and C is > legal, but the configuration A and B is not allowed by the sensor. How > could you set the rectangles one by one? > As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. So activation of rectangle B (setting height to non-zero value) will enable rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) may disable rectangle C automatically. I do not follow what is the problem here? Hmm. I think that the real problem is much different. Not how to set up rectangles atomically but rather how do anything non-trivial atomically in V4L2. It would be very nice to have crop/compose and format configured at the same time. However, current version of V4L2 API does not support that. Setting multiple crop rectangles may help a bit for only this little case. But how would like to set multicrop rectangles and multicompose rectangle atomically. How to define which crop rectangle refers to which to which compose rectangle. What to do if one would like to change only 3rd crop rectangle? Introduce rectangle id into v4l2_ext_rect? Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? Is it really scalable? Multirectangle targets may seam to be a solution but I think it is not. I think that atomic transactions are what is really needed in V4L2. Other ideas like try-context from subdev API may also help. It will be nice to have something like VIDIOC_BEGIN VIDIOC_S_SELECTION (target = CROP) VIDIOC_S_SELECTION (target = COMPOSE) VIDIOC_S_FMT VIDIOC_S_CTRL VIDIOC_COMMIT and call a bunch of VIDIOC_G_* to see what really was applied. >> I have an auxiliary question. Do you have to set all rectangles >> at once? can you set up them one by one? > > Also if you tell the driver what exact configuration you will need, it > will provide you with the closest possible confuration, that cannot be s/cannot be done/may not be 'doable' > done if you provide rectangle by rectangle. > >> But how to deal with multiple rectangles? > > Multiple rectangles is a desired feature, please take a look to the > presentation on the workshop. > I agree that it may be useful. I just think that multirectangle selections are needed to add support for such a feature. >> >> Anyway, first try to define something like this: >> >> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >> >> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >> ... >> >> -- OR-- parametrized macros similar to one below: >> >> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >> >> The application could setup all scanout areas one-by-one. >> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >> The height of all consecutive area would be 0. This means disabling >> them effectively. > > Lets say rectangle A + B + C +D is legal, and A +B is also legal. You > are in ABCD and you want to go to AB. How can you do it? Just set C to have height 0. It will disable D automatically. BTW. How are you going to emulate S_CROP by selection API if you must use at least two rectangles (A + B) ? I suggest to use SCANOUT target in your first implementation. Notice that as long you use something different from CROP and COMPOSE, you may define interactions and dependencies any way you want, like - if change of scanout area can affect composing area - interaction with format and frame size - check if scanout area can overlap, multi crops should be allowed to overlap See if it will be sufficient. See what kind of problems you encouter. > > If yo dissable C or D, the configuration is ilegal and therefor the > driver will return -EINVAL. So once you are in ABCD you cannot go > back... > The driver must not return EINVAL as long as it is possible to adjust values passed by user to some valid configuration. If configuration is fixed then VIDIOC_S_SELECTION works effectively as VIDIOC_G_SELECTION. > >> >> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >> rectangle by shifting them down or resetting them to height 0. >> Notice that as long as targets are driver specific you are free do define >> interaction between the targets. >> >> I hope that proposed solution is satisfactory. > > As stated before, please follow the previous comments on the rfc, > specially the ones from Hans. > >> >> BTW. I think that the HW trick you described is not cropping. >> By cropping you select which part of sensor area is going >> to be processed into compose rectangle in a buffer. > > You are selecting part of the sensor, therefore you are cropping the image. > Yes, it is the same as long as you use only one rectangle. If using more then 1 the situation may become more complex. Currently a pair of composing and crop rectangles are used to setup scaling. You have to introduce a new definition of scaling factor if multirect crops are introduced. Moreover what happens if flipping or rotation is applied? Currently only the content of the rectangle is rotated/flipped. If you use multi rectangle then content of each rectangle must be rotated/flipped separatelly. Are you sure that your hardware can do this? What about layout of composing rectangle inside output buffer? Should it reflect the layout of crop rectangle or all of them should be independent. Of course you can always say that everything is driver dependent :). Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. >> >> So technicaly you still insert the whole sensor area into the buffer. > > Only the lines/columns are read into the buffer. > Yes, but where into a buffer? Does you HW support setting destination for every crop rectangle? >> Only part of the buffer is actually updated. So there is no cropping >> (cropping area is equal to the whole array). >> >> Notice, that every 'cropping' area goes to different part of a buffer. >> So you would need also muliple targets for composing (!!!) and very long >> chapter in V4L2 doc describing interactions between muliple-rectangle >> crops and compositions. Good luck !!! :). > > It is not that difficult to describe. > > You need the same ammount of rectangles in cropping and in compossing. > Rectangle X in cropping will be mapped to rectangle X in compose. > > >> Currently it is a hell to understand and define interaction between >> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. > > On m2m devices we are only lacking a s_fmt on the first buffer, as we > have discussed on the workshop. I think we only lack a good reference > model in vivi. > > I was not present on the workshop. Could you provide more details? >> >> I strongly recommend to use private selections. >> It will be much simpler to define, implement, and modify if needed. > > I think the private selections will lead to specific applications for > specific drivers and we cannot support all the configurations with > them. Also there is no way for an app to enumerate that capability. call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? EINVAL means that it is not supported. > >> >> BTW2. I think that the mulitple scanout areas can be modelled using >> media device API. Sakari may know how to do this. > > The areas are not read from the sensor. Therefore they can only be > proccessed on the sensor subdevice. > >> >> >> Ad 2. Extended rectangles. >> It is a good idea because v4l2_rect lacks any place for extensions. >> But before adding it to V4L2 API, I would like to know the examples >> of actual applications. Please, point drivers that actually need it. > > I have no need for it, but now that we are extending the selection API > it would be the perfect moment to add them. The perfect moment for adding something is when it is needed. The bad idea is preventing something from being added too early. Regards, Tomasz Stanislawski > > They could describe properties of the sensor, like tracking ids. > >> >> Other thing worth mentioning is reservation of few bits from >> v4l2_selection::flags to describe the type of data used for >> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid >> introducting v4l2_selection::rectangles field. >> >> I hope you find this comments useful. > > > Regards > >> >> Regards, >> Tomasz Stanislawski >> >> > > > -- 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
Hi Tomasz, On 11/12/13 15:54, Tomasz Stanislawski wrote: > Hi Ricardo, > Sorry for a late reply. I've been 'offline' for the last two weeks. > Please refer to the comments below. > > On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote: >> Hello Tomasz >> >> Sorry for the late reply, but I have been offline the last week due to >> the conference. >> >> >> On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> Hi Ricardo, >>> I am the designer of selection API. I hope I can help you a little. >>> I think that there are two issues mixed in 'Mulitple selections' topic. >>> >>> Firstly, you described that you program a piece of hardware that is >>> capable of selecting 8 areas for scanning. Now you >>> are looking for userspace API to support such a feature. >>> The feature of posting multiple rectangle was proposed in this RFC. >>> >>> Secondly, You introduced struct v4l2_ext_rect which is a future-proof >>> version of v4l2_rect. >>> >>> >>> I think that both issues should be solved in two separate patchsets. >>> >>> Ad 1. >>> The selection of multiple scanning areas is a very driver-specific >>> feature, isn't it? I think that you do not need to introduce any abstract >>> interface. What would be other applications of the proposed interface? >> >> It is not driver specific. There are many sensors out there that >> supports multiple window of interest, but today we are ignoring them >> just because we dont have an api. >> >> The main application would be industrial imaging, where less data to >> read means more fps and therefore the system can run faster. >> >> From my field I can tell you that it is a hard requirement for >> computer vision. And it is a feature that we cannot model through v4l2 >> controls. >> >> > > OK. So there is no need to implement this feature as a driver-specific API. > It can go automatically to generic API. > >>> Do you know other drivers that may need it? Sakari mentioned introduction >>> of private targets for selections. I like this idea. Just define: >>> >>> #define V4L2_SEL_TGT_PRIVATE 0x80000000 >>> >>> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. >>> Generic applications must not use them. Non-generic application >>> must check out the driver of video node before using selections >>> from private set. If some target becomes more useful and accepted >>> by more then one driver then it can be moved to generic API. >>> The good thing about private target is that enums from different >>> drivers can collide so the target space is not going to be trashed. >>> >> >> If you read the previous RFCs you will see that the approach you are >> mentioning has been rejected. >> >> The main issue is that you cannot set atomically all the rectangles. >> Lets say that the configuration formed by rectangle A, B and C is >> legal, but the configuration A and B is not allowed by the sensor. How >> could you set the rectangles one by one? >> > > As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. > So activation of rectangle B (setting height to non-zero value) will enable > rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) > may disable rectangle C automatically. I do not follow what is the problem here? The problem would be in a situation like this: ...... .AA.B. ...... --> AAB .C.DD. CDD ...... A-D are the rectangles you want to select. They are cropped as shown on the left and composed as shown on the right. From what Ricardo told me the resulting composed image typically must be a proper rectangle without padding anywhere. Trying to add rectangles one at a time breaks down when adding C because the composition result is no longer a 'proper' rectangle. I don't see how you can set something like that up one rectangle at a time. It makes much more sense to set everything up at once. BTW, what probably wasn't clear from Ricardo's explanation is that for every crop rectangle you must have a corresponding compose rectangle so that you know where to DMA it to. Your next question will be that it is a real problem that you can't set crop and compose simultaneously, and you are right about that. Read on for that... :-) > Hmm. I think that the real problem is much different. > Not how to set up rectangles atomically but rather > how do anything non-trivial atomically in V4L2. > > It would be very nice to have crop/compose and format configured at the same time. > However, current version of V4L2 API does not support that. > > Setting multiple crop rectangles may help a bit for only this little case. > But how would like to set multicrop rectangles and multicompose rectangle atomically. Why can't we extend the selection API a bit? How about this: #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 struct v4l2_selection { __u32 type; __u32 target; __u32 flags; union { struct v4l2_rect r; struct v4l2_ext_rect *pr; }; __u32 flags2; union { struct v4l2_rect r2; struct v4l2_ext_rect *pr2; }; __u32 rectangles; __u32 reserved[3]; }; If the target is CROP_COMPOSE then flags & r define the crop rectangle, and flags2 & r2 define the compose rectangle. That way you can set them both atomically. I would propose that the interaction with S_FMT is as follows: if S_FMT defines a rectangle < then the current compose rectangle, then the composition (and optionally crop) rectangle is reset. If it is >=, then all is fine. If a new compose rectangle doesn't fit in the S_FMT rectangle then it should adapt the S_FMT rectangle to make it fit (as long as HW constraints are obeyed of course). This sequence will obey the rules of V4L2 as well: the last operation takes precedence over older operations. So setting S_FMT allow the driver to change cropping/composing to get as close to the desired format as possible. > How to define which crop rectangle refers to which to which compose rectangle. By setting the crop and compose selections at the same time and of the same size you can map each crop selection to a compose selection. It's all atomic as well. > > What to do if one would like to change only 3rd crop rectangle? > > Introduce rectangle id into v4l2_ext_rect? > Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? > Is it really scalable? Why not? > > Multirectangle targets may seam to be a solution but I think it is not. > > I think that atomic transactions are what is really needed in V4L2. > Other ideas like try-context from subdev API may also help. > > It will be nice to have something like > > VIDIOC_BEGIN > VIDIOC_S_SELECTION (target = CROP) > VIDIOC_S_SELECTION (target = COMPOSE) > VIDIOC_S_FMT > VIDIOC_S_CTRL > VIDIOC_COMMIT I don't think S_FMT is needed here: it's something you set up at the beginning and don't touch afterwards. Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to solving these atomicity problems? Another nice feature that can be added to the selection API is to add a field to refer to a frame sequence number or a v4l2_buffer index (the latter is probably better): this would make it easy to apply an atomic crop/compose change to easily implement things like digital zoom or moving windows around. We could do something similar for controls. This would also solve the problem of assigning a per-buffer (or per-frame) configuration, something that libcamera2/3 needs. > > and call a bunch of VIDIOC_G_* to see what really was applied. > >>> I have an auxiliary question. Do you have to set all rectangles >>> at once? can you set up them one by one? >> >> Also if you tell the driver what exact configuration you will need, it >> will provide you with the closest possible confuration, that cannot be > > s/cannot be done/may not be 'doable' > >> done if you provide rectangle by rectangle. >> >>> But how to deal with multiple rectangles? >> >> Multiple rectangles is a desired feature, please take a look to the >> presentation on the workshop. >> > > I agree that it may be useful. I just think that multirectangle selections > are needed to add support for such a feature. I don't follow, isn't that what this proposal adds? > >>> >>> Anyway, first try to define something like this: >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>> ... >>> >>> -- OR-- parametrized macros similar to one below: >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>> >>> The application could setup all scanout areas one-by-one. >>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>> The height of all consecutive area would be 0. This means disabling >>> them effectively. >> >> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >> are in ABCD and you want to go to AB. How can you do it? > > Just set C to have height 0. It will disable D automatically. It's the other direction that's the problem: how to go from A + B to A + B + C + D if ABC is illegal. > BTW. How are you going to emulate S_CROP by selection API > if you must use at least two rectangles (A + B) ? S_CROP will always just set one rectangle A. So if you had multiple rectangles in your selection then after S_CROP you'll have only one. I'm assuming that the hw will always support a single crop rectangle. If not (extraordinarily unlikely), then S_CROP should just return an error. > I suggest to use SCANOUT target in your first implementation. > Notice that as long you use something different from CROP and COMPOSE, > you may define interactions and dependencies any way you want, like > - if change of scanout area can affect composing area > - interaction with format and frame size > - check if scanout area can overlap, multi crops should be allowed to overlap I see no reason for this. A CROP_COMPOSE target does all you need. Note that to set up N rectangles where N differs from the current rectangle count you do need to use CROP_COMPOSE. You can't setup CROP and COMPOSE rectangles independently since having N crop rectangles and M compose rectangles makes no sense. If you don't change the number of rectangles, then you can still use CROP and COMPOSE as is. > > See if it will be sufficient. > See what kind of problems you encouter. > >> >> If yo dissable C or D, the configuration is ilegal and therefor the >> driver will return -EINVAL. So once you are in ABCD you cannot go >> back... >> > > The driver must not return EINVAL as long as it is possible to > adjust values passed by user to some valid configuration. > If configuration is fixed then VIDIOC_S_SELECTION works > effectively as VIDIOC_G_SELECTION. > > >> >>> >>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >>> rectangle by shifting them down or resetting them to height 0. >>> Notice that as long as targets are driver specific you are free do define >>> interaction between the targets. >>> >>> I hope that proposed solution is satisfactory. >> >> As stated before, please follow the previous comments on the rfc, >> specially the ones from Hans. >> >>> >>> BTW. I think that the HW trick you described is not cropping. >>> By cropping you select which part of sensor area is going >>> to be processed into compose rectangle in a buffer. >> >> You are selecting part of the sensor, therefore you are cropping the image. >> > > Yes, it is the same as long as you use only one rectangle. > > If using more then 1 the situation may become more complex. > Currently a pair of composing and crop rectangles are used to setup scaling. > > You have to introduce a new definition of scaling factor if multirect crops are introduced. > > Moreover what happens if flipping or rotation is applied? > Currently only the content of the rectangle is rotated/flipped. > If you use multi rectangle then content of each rectangle must be > rotated/flipped separatelly. Are you sure that your hardware can do this? > > What about layout of composing rectangle inside output buffer? > Should it reflect the layout of crop rectangle or all of them should be > independent. > > Of course you can always say that everything is driver dependent :). > > Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. I disagree. But it requires (and I hadn't realized that before) that you pass all the crop and compose rectangles atomically. > >>> >>> So technicaly you still insert the whole sensor area into the buffer. >> >> Only the lines/columns are read into the buffer. >> > > Yes, but where into a buffer? > Does you HW support setting destination for every crop rectangle? From what Ricardo told us during the meeting I got the impression that it is basically just skipping the lines and columns that are not wanted. So ...AAA..BB..C... just ends up as AAABBC .D....EEEEE..... DEEEEE > >>> Only part of the buffer is actually updated. So there is no cropping >>> (cropping area is equal to the whole array). >>> >>> Notice, that every 'cropping' area goes to different part of a buffer. >>> So you would need also muliple targets for composing (!!!) and very long >>> chapter in V4L2 doc describing interactions between muliple-rectangle >>> crops and compositions. Good luck !!! :). >> >> It is not that difficult to describe. >> >> You need the same ammount of rectangles in cropping and in compossing. >> Rectangle X in cropping will be mapped to rectangle X in compose. >> >> >>> Currently it is a hell to understand and define interaction between >>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. >> >> On m2m devices we are only lacking a s_fmt on the first buffer, as we >> have discussed on the workshop. I think we only lack a good reference >> model in vivi. >> >> > > I was not present on the workshop. Could you provide more details? > >>> >>> I strongly recommend to use private selections. >>> It will be much simpler to define, implement, and modify if needed. >> >> I think the private selections will lead to specific applications for >> specific drivers and we cannot support all the configurations with >> them. Also there is no way for an app to enumerate that capability. > > call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? > EINVAL means that it is not supported. > >> >>> >>> BTW2. I think that the mulitple scanout areas can be modelled using >>> media device API. Sakari may know how to do this. >> >> The areas are not read from the sensor. Therefore they can only be >> proccessed on the sensor subdevice. >> >>> >>> >>> Ad 2. Extended rectangles. >>> It is a good idea because v4l2_rect lacks any place for extensions. >>> But before adding it to V4L2 API, I would like to know the examples >>> of actual applications. Please, point drivers that actually need it. >> >> I have no need for it, but now that we are extending the selection API >> it would be the perfect moment to add them. > > The perfect moment for adding something is when it is needed. We need to add support for multiple rectangles, so we added a pointer. > The bad idea is preventing something from being added too early. But if we make it a pointer to v4l2_rect, then we can *never* add additional information to each rectangle. Whenever we add new APIs or features we try to allow for future extensions (with mixed success...), so now is the time to do so for v4l2_rect. Something like this: struct v4l2_sel_rect { struct v4l2_rect r; __u32 reserved[4]; }; would be fine by me as well. It's an additional indirection, but it also makes it easier to go from a v4l2_rect to a v4l2_sel_rect. Regards, Hans > > Regards, > Tomasz Stanislawski > >> >> They could describe properties of the sensor, like tracking ids. >> >>> >>> Other thing worth mentioning is reservation of few bits from >>> v4l2_selection::flags to describe the type of data used for >>> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid >>> introducting v4l2_selection::rectangles field. >>> >>> I hope you find this comments useful. >> >> >> Regards >> >>> >>> Regards, >>> Tomasz Stanislawski >>> >>> >> >> >> > > -- > 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 > -- 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
Hi Hans, On 11/14/2013 11:18 AM, Hans Verkuil wrote: > Hi Tomasz, > > On 11/12/13 15:54, Tomasz Stanislawski wrote: >> Hi Ricardo, >> Sorry for a late reply. I've been 'offline' for the last two weeks. >> Please refer to the comments below. >> [snip] >> >> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >> So activation of rectangle B (setting height to non-zero value) will enable >> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >> may disable rectangle C automatically. I do not follow what is the problem here? > > The problem would be in a situation like this: > > ...... > .AA.B. > ...... --> AAB > .C.DD. CDD > ...... > > A-D are the rectangles you want to select. They are cropped as shown on the > left and composed as shown on the right. > >>From what Ricardo told me the resulting composed image typically must be > a proper rectangle without padding anywhere. > > Trying to add rectangles one at a time breaks down when adding C because > the composition result is no longer a 'proper' rectangle. I don't see how > you can set something like that up one rectangle at a time. I see the issue but I think that it is not a big problem. Activating C forms a non-proper rectangle with A and B. Therefore, driver must force enabling D to form a proper rectangle again. I mean that instead of enlarging C to sum of width of A and B, the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced to inform application which part of a buffers if going to be modified with some undefined value. I see nothing against setting a padded rectangle for C to a rectangle that covers C and D or even the whole ABCD rectangle. I think it could be a great application for PADDED target. The application could easily detect which part of a buffer are affected. Even applications prepared to work with single crop devices would still work after enabling multi crop mode. The setup of rectangles my look like this. ************************************************ S_SELECTION(CROP0 = A) crop compose ---------------------- ...... .AA... ...... --> AA.. ...... .... ...... .... G_SELECTION(COMPOSE0) AA.. .... .... G_SELECTION(COMPOSE0_PADDED) AA.. .... .... ************************************************ S_SELECTION(CROP1 = B) ...... .AA.B. ...... --> AAB. ...... .... ...... .... G_SELECTION(COMPOSE0) AA.. .... .... G_SELECTION(COMPOSE0_PADDED) AAA. .... .... G_SELECTION(COMPOSE1) ..B. .... .... G_SELECTION(COMPOSE1_PADDED) BBB. .... .... ************************************************ S_SELECTION(CROP2 = C) - D is activated implicitly ...... .AA.B. .C.DD. --> AAB. ...... CDD. ...... .... G_SELECTION(COMPOSE0_PADDED) AAA. AAA. .... G_SELECTION(COMPOSE2) .... C... .... G_SELECTION(COMPOSE2_PADDED) CCC. CCC. .... G_SELECTION(COMPOSE3) .... .DD. .... G_SELECTION(COMPOSE3_PADDED) DDD. DDD. .... One may argue that all this logic is unnecessary after adding support for multirect selections. So, I kindly ask what should happen if someone call S_SELECTION (in multirect mode) passing THREE rectangles A, B, and C (not D) ? The driver must adjust rectangles to some valid value. So it can increase width of C or implicitly activate D or disable C. I think that the best solution is activating D because it allows to set size of C to the value closest to requested one. Therefore logic for implicit activation of D should be implemented anyway. > > It makes much more sense to set everything up at once. I agree that it is better to set everything at once. But I strongly believe that transactions are the proper way to achieve that. Not multirectangle selections. It obfuscates API. It only pretends to fix a problem with applying a part of configuration atomically. > > BTW, what probably wasn't clear from Ricardo's explanation is that for every > crop rectangle you must have a corresponding compose rectangle so that you > know where to DMA it to. > > Your next question will be that it is a real problem that you can't set > crop and compose simultaneously, and you are right about that. Read on for > that... :-) > >> Hmm. I think that the real problem is much different. >> Not how to set up rectangles atomically but rather >> how do anything non-trivial atomically in V4L2. >> >> It would be very nice to have crop/compose and format configured at the same time. >> However, current version of V4L2 API does not support that. >> >> Setting multiple crop rectangles may help a bit for only this little case. >> But how would like to set multicrop rectangles and multicompose rectangle atomically. > > Why can't we extend the selection API a bit? How about this: > > #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 > > struct v4l2_selection { > __u32 type; > __u32 target; > __u32 flags; > union { > struct v4l2_rect r; > struct v4l2_ext_rect *pr; > }; > __u32 flags2; > union { > struct v4l2_rect r2; > struct v4l2_ext_rect *pr2; > }; > __u32 rectangles; > __u32 reserved[3]; > }; > > If the target is CROP_COMPOSE then flags & r define the crop rectangle, and > flags2 & r2 define the compose rectangle. That way you can set them both > atomically. I do not like this idea because: - mix crop and composing generating semi-related cropcompose - obfuscates the API even more - wastes a lot of reserved fields - what if someone would like to use separate 'flags' for each rectangle ... this could be a nice feature anyway :) This remains me the proposition from early days of selection API. http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945 Refer to point 4 where v4l2_crop2 was mentioned. > > I would propose that the interaction with S_FMT is as follows: if S_FMT > defines a rectangle < then the current compose rectangle, then the > composition (and optionally crop) rectangle is reset. If it is >=, then > all is fine. There is a issue here. Long time ago it was stated that S_FMT should result in configuration where data are processed (scaled) into the 'whole picture'. It means that a COMPOSE rectangle should be reset after very S_FMT. > > If a new compose rectangle doesn't fit in the S_FMT rectangle then it should > adapt the S_FMT rectangle to make it fit (as long as HW constraints are > obeyed of course). What happens if REQBUFS was called between S_FMT and S_SELECTION? > > This sequence will obey the rules of V4L2 as well: the last operation takes > precedence over older operations. So setting S_FMT allow the driver to > change cropping/composing to get as close to the desired format as possible. > >> How to define which crop rectangle refers to which to which compose rectangle. > > By setting the crop and compose selections at the same time and of the same > size you can map each crop selection to a compose selection. It's all atomic > as well. > I think that adding an id field to v4l2_selection might be a good alternative to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ... It will add support for all multirectangle features as cost of sacrifice of single reserved field. Moreover id field might be very useful for other selection targets like FOCUS or FACE. Other idea might to using some bits of 'flags' field to carry rectangle id. >> >> What to do if one would like to change only 3rd crop rectangle? >> >> Introduce rectangle id into v4l2_ext_rect? >> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >> Is it really scalable? > > Why not? > Because it is not scalable. An application has to use 2 syscall instead of 1. Unnecessary copying has to be performed. >> >> Multirectangle targets may seam to be a solution but I think it is not. >> >> I think that atomic transactions are what is really needed in V4L2. >> Other ideas like try-context from subdev API may also help. >> >> It will be nice to have something like >> >> VIDIOC_BEGIN >> VIDIOC_S_SELECTION (target = CROP) >> VIDIOC_S_SELECTION (target = COMPOSE) >> VIDIOC_S_FMT >> VIDIOC_S_CTRL >> VIDIOC_COMMIT > > I don't think S_FMT is needed here: it's something you set up at the > beginning and don't touch afterwards. One could say the same about compose, crop, ctrls. The problem are the interactions between those objects. What is dependent on what. What can change what. How to reach a valid state in all cases not preventing some valid configurations from being unreachable. > > Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to > solving these atomicity problems? > > Another nice feature that can be added to the selection API is to > add a field to refer to a frame sequence number or a v4l2_buffer > index (the latter is probably better): this would make it easy to > apply an atomic crop/compose change to easily implement things like > digital zoom or moving windows around. We could do something similar > for controls. > > This would also solve the problem of assigning a per-buffer (or per-frame) > configuration, something that libcamera2/3 needs. > Introduce transactions with frame sequence numbers. I proposed this in Cambridge in 2012. >> >> and call a bunch of VIDIOC_G_* to see what really was applied. >> >>>> I have an auxiliary question. Do you have to set all rectangles >>>> at once? can you set up them one by one? >>> >>> Also if you tell the driver what exact configuration you will need, it >>> will provide you with the closest possible confuration, that cannot be >> >> s/cannot be done/may not be 'doable' >> >>> done if you provide rectangle by rectangle. >>> >>>> But how to deal with multiple rectangles? >>> >>> Multiple rectangles is a desired feature, please take a look to the >>> presentation on the workshop. >>> >> >> I agree that it may be useful. I just think that multirectangle selections >> are needed to add support for such a feature. > > I don't follow, isn't that what this proposal adds? > s/are needed/are not needed/ Sorry for confusion. >> >>>> >>>> Anyway, first try to define something like this: >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>> ... >>>> >>>> -- OR-- parametrized macros similar to one below: >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>> >>>> The application could setup all scanout areas one-by-one. >>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>> The height of all consecutive area would be 0. This means disabling >>>> them effectively. >>> >>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>> are in ABCD and you want to go to AB. How can you do it? >> >> Just set C to have height 0. It will disable D automatically. > > It's the other direction that's the problem: how to go from A + B to > A + B + C + D if ABC is illegal. > >> BTW. How are you going to emulate S_CROP by selection API >> if you must use at least two rectangles (A + B) ? > > S_CROP will always just set one rectangle A. So if you had multiple > rectangles in your selection then after S_CROP you'll have only one. So S_CROP should change only A. What happens if someone already set device to work with 4 rectangles. Then calls S_CROP that modifies A in such a way that ABCD no longer forms a proper rectangle. What should happen? - ignore S_CROP, return previous settings, or - modify B,C,D to form a proper rectangle, or - disable B,C,D, or ? > > I'm assuming that the hw will always support a single crop rectangle. > If not (extraordinarily unlikely), then S_CROP should just return an > error. I agree that it is highly unlikely. I am just trying to figure out what should happen when calling S_CROP or single-rectangle S_SELECTION after calling multirect S_SELECTION. > >> I suggest to use SCANOUT target in your first implementation. >> Notice that as long you use something different from CROP and COMPOSE, >> you may define interactions and dependencies any way you want, like >> - if change of scanout area can affect composing area >> - interaction with format and frame size >> - check if scanout area can overlap, multi crops should be allowed to overlap > > I see no reason for this. A CROP_COMPOSE target does all you need. I think that CROP and COMPOSE is all you need. > > Note that to set up N rectangles where N differs from the current > rectangle count you do need to use CROP_COMPOSE. You can't setup CROP > and COMPOSE rectangles independently since having N crop rectangles > and M compose rectangles makes no sense. If you don't change the > number of rectangles, then you can still use CROP and COMPOSE as is. > I am afraid that you are trying to kill too many birds with one stone. I think it is a very good moment to use bazooka. Transactions enhanced with frame numbers will handle a lot of issues with V4L2. Moreover it will allow to support some nice effects like: - multiple exposures during HDR photography - continuous online changes preview windows - flash at a specific frame - change format during video streaming to grab a high-resolution picture - support multi rectangle cropping and composing :) >> [snip] >>> I have no need for it, but now that we are extending the selection API >>> it would be the perfect moment to add them. >> >> The perfect moment for adding something is when it is needed. > > We need to add support for multiple rectangles, so we added a pointer. > >> The bad idea is preventing something from being added too early. > > But if we make it a pointer to v4l2_rect, then we can *never* add > additional information to each rectangle. Whenever we add new APIs > or features we try to allow for future extensions (with mixed > success...), so now is the time to do so for v4l2_rect. > > Something like this: > > struct v4l2_sel_rect { > struct v4l2_rect r; > __u32 reserved[4]; > }; > > would be fine by me as well. It's an additional indirection, but it > also makes it easier to go from a v4l2_rect to a v4l2_sel_rect. We can always migrate to extended rectangle by modifying the structure to struct v4l2_selection { __u32 type; __u32 target; __u32 flags; union { struct v4l2_rect r; struct v4l2_ext_rect rext; }; __u32 reserved[X]; }; and adding V4L2_SEL_FLAG_EXT_RECT flag. > > Regards, > > Hans > >> Regards, Tomasz Stanislawski -- 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 Tomasz Now is my time to say sorry for the delay, but i have been in holidays and then I had a pile of work waiting on my desk :). On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > Hi Ricardo, > Sorry for a late reply. I've been 'offline' for the last two weeks. > Please refer to the comments below. > > On 10/28/2013 11:46 PM, Ricardo Ribalda Delgado wrote: >> Hello Tomasz >> >> Sorry for the late reply, but I have been offline the last week due to >> the conference. >> >> >> On Thu, Oct 24, 2013 at 12:31 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> Hi Ricardo, >>> I am the designer of selection API. I hope I can help you a little. >>> I think that there are two issues mixed in 'Mulitple selections' topic. >>> >>> Firstly, you described that you program a piece of hardware that is >>> capable of selecting 8 areas for scanning. Now you >>> are looking for userspace API to support such a feature. >>> The feature of posting multiple rectangle was proposed in this RFC. >>> >>> Secondly, You introduced struct v4l2_ext_rect which is a future-proof >>> version of v4l2_rect. >>> >>> >>> I think that both issues should be solved in two separate patchsets. >>> >>> Ad 1. >>> The selection of multiple scanning areas is a very driver-specific >>> feature, isn't it? I think that you do not need to introduce any abstract >>> interface. What would be other applications of the proposed interface? >> >> It is not driver specific. There are many sensors out there that >> supports multiple window of interest, but today we are ignoring them >> just because we dont have an api. >> >> The main application would be industrial imaging, where less data to >> read means more fps and therefore the system can run faster. >> >> From my field I can tell you that it is a hard requirement for >> computer vision. And it is a feature that we cannot model through v4l2 >> controls. >> >> > > OK. So there is no need to implement this feature as a driver-specific API. > It can go automatically to generic API. > >>> Do you know other drivers that may need it? Sakari mentioned introduction >>> of private targets for selections. I like this idea. Just define: >>> >>> #define V4L2_SEL_TGT_PRIVATE 0x80000000 >>> >>> All targets that are >= V4L2_SEL_TGT_PRIVATE are driver-specific. >>> Generic applications must not use them. Non-generic application >>> must check out the driver of video node before using selections >>> from private set. If some target becomes more useful and accepted >>> by more then one driver then it can be moved to generic API. >>> The good thing about private target is that enums from different >>> drivers can collide so the target space is not going to be trashed. >>> >> >> If you read the previous RFCs you will see that the approach you are >> mentioning has been rejected. >> >> The main issue is that you cannot set atomically all the rectangles. >> Lets say that the configuration formed by rectangle A, B and C is >> legal, but the configuration A and B is not allowed by the sensor. How >> could you set the rectangles one by one? >> > > As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. > So activation of rectangle B (setting height to non-zero value) will enable > rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) > may disable rectangle C automatically. I do not follow what is the problem here? > Lets say you want a configuration composed by 3 rectangles ABC and there are no pair of rectangles with a legal configuration. You cannot do step by step configuration. Also lets say that your sensor requires that the total size of the image is 700 lines on 3 rectanges and 500 on 4. You cannot do this configuration step by step. > Hmm. I think that the real problem is much different. > Not how to set up rectangles atomically but rather > how do anything non-trivial atomically in V4L2. > > It would be very nice to have crop/compose and format configured at the same time. > However, current version of V4L2 API does not support that. > > Setting multiple crop rectangles may help a bit for only this little case. > But how would like to set multicrop rectangles and multicompose rectangle atomically. > How to define which crop rectangle refers to which to which compose rectangle. The number of retangles in crop are the same number of rectables in the compose. Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on.... > > What to do if one would like to change only 3rd crop rectangle? You send the whole configuration. The same as today when the user only wants to change the pixel format. He still have to send the size. > > Introduce rectangle id into v4l2_ext_rect? > Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? > Is it really scalable? Why it is not scalable?It is much more scalable than 8 ioctls to set 8 rectangles. > > Multirectangle targets may seam to be a solution but I think it is not. > > I think that atomic transactions are what is really needed in V4L2. > Other ideas like try-context from subdev API may also help. > > It will be nice to have something like > > VIDIOC_BEGIN > VIDIOC_S_SELECTION (target = CROP) > VIDIOC_S_SELECTION (target = COMPOSE) > VIDIOC_S_FMT > VIDIOC_S_CTRL > VIDIOC_COMMIT > > and call a bunch of VIDIOC_G_* to see what really was applied. > This will trigger other isues. What we do if 2 programs starts two transactions at the same time. We will keep a transaction array with ALL the configurations? And what happens if there are 100? >>> I have an auxiliary question. Do you have to set all rectangles >>> at once? can you set up them one by one? >> >> Also if you tell the driver what exact configuration you will need, it >> will provide you with the closest possible confuration, that cannot be > > s/cannot be done/may not be 'doable' cannot be done. The driver cannot guess which rectangles will the user set in the future. > >> done if you provide rectangle by rectangle. >> >>> But how to deal with multiple rectangles? >> >> Multiple rectangles is a desired feature, please take a look to the >> presentation on the workshop. >> > > I agree that it may be useful. I just think that multirectangle selections > are needed to add support for such a feature. > >>> >>> Anyway, first try to define something like this: >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>> ... >>> >>> -- OR-- parametrized macros similar to one below: >>> >>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>> >>> The application could setup all scanout areas one-by-one. >>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>> The height of all consecutive area would be 0. This means disabling >>> them effectively. >> >> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >> are in ABCD and you want to go to AB. How can you do it? > > Just set C to have height 0. It will disable D automatically. And what if I have ABCDE and I want to go to ABE? As I said before the causistic of the sensors is inmense. Why limit ourselves? > > BTW. How are you going to emulate S_CROP by selection API > if you must use at least two rectangles (A + B) ? Simple, you dont. > > I suggest to use SCANOUT target in your first implementation. > Notice that as long you use something different from CROP and COMPOSE, > you may define interactions and dependencies any way you want, like > - if change of scanout area can affect composing area > - interaction with format and frame size > - check if scanout area can overlap, multi crops should be allowed to overlap > > See if it will be sufficient. > See what kind of problems you encouter. On the process of the RFC I have used in our system all the implementations. So far the only that has worked 100% is the solution where the user present all the rectangles at once to the driver. > >> >> If yo dissable C or D, the configuration is ilegal and therefor the >> driver will return -EINVAL. So once you are in ABCD you cannot go >> back... >> > > The driver must not return EINVAL as long as it is possible to > adjust values passed by user to some valid configuration. > If configuration is fixed then VIDIOC_S_SELECTION works > effectively as VIDIOC_G_SELECTION. > > >> >>> >>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >>> rectangle by shifting them down or resetting them to height 0. >>> Notice that as long as targets are driver specific you are free do define >>> interaction between the targets. >>> >>> I hope that proposed solution is satisfactory. >> >> As stated before, please follow the previous comments on the rfc, >> specially the ones from Hans. >> >>> >>> BTW. I think that the HW trick you described is not cropping. >>> By cropping you select which part of sensor area is going >>> to be processed into compose rectangle in a buffer. >> >> You are selecting part of the sensor, therefore you are cropping the image. >> > > Yes, it is the same as long as you use only one rectangle. > > If using more then 1 the situation may become more complex. > Currently a pair of composing and crop rectangles are used to setup scaling. > > You have to introduce a new definition of scaling factor if multirect crops are introduced. The number of rectangles must be the same on the croping and on the composing. The scaling factor is defined by the relation between compose[x]/crop[x] > > Moreover what happens if flipping or rotation is applied? Flipping is applied to the whole sensor. > Currently only the content of the rectangle is rotated/flipped. > If you use multi rectangle then content of each rectangle must be > rotated/flipped separatelly. Are you sure that your hardware can do this? In our hardware there is a mapping core that can map any pixel in any location. So yes :) > > What about layout of composing rectangle inside output buffer? > Should it reflect the layout of crop rectangle or all of them should be > independent. > > Of course you can always say that everything is driver dependent :). > > Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. I dont see why. When we defined the API we didn't consider the case of multiples WOI, now we fix this. > >>> >>> So technicaly you still insert the whole sensor area into the buffer. >> >> Only the lines/columns are read into the buffer. >> > > Yes, but where into a buffer? > Does you HW support setting destination for every crop rectangle? The dma supports up to 256 destinations. > >>> Only part of the buffer is actually updated. So there is no cropping >>> (cropping area is equal to the whole array). >>> >>> Notice, that every 'cropping' area goes to different part of a buffer. >>> So you would need also muliple targets for composing (!!!) and very long >>> chapter in V4L2 doc describing interactions between muliple-rectangle >>> crops and compositions. Good luck !!! :). >> >> It is not that difficult to describe. >> >> You need the same ammount of rectangles in cropping and in compossing. >> Rectangle X in cropping will be mapped to rectangle X in compose. >> >> >>> Currently it is a hell to understand and define interaction between >>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. >> >> On m2m devices we are only lacking a s_fmt on the first buffer, as we >> have discussed on the workshop. I think we only lack a good reference >> model in vivi. >> >> > > I was not present on the workshop. Could you provide more details? > >>> >>> I strongly recommend to use private selections. >>> It will be much simpler to define, implement, and modify if needed. >> >> I think the private selections will lead to specific applications for >> specific drivers and we cannot support all the configurations with >> them. Also there is no way for an app to enumerate that capability. > > call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? > EINVAL means that it is not supported. And then we will have X zillions of SCANOUTS, that is really the Pandora box :) > >> >>> >>> BTW2. I think that the mulitple scanout areas can be modelled using >>> media device API. Sakari may know how to do this. >> >> The areas are not read from the sensor. Therefore they can only be >> proccessed on the sensor subdevice. >> >>> >>> >>> Ad 2. Extended rectangles. >>> It is a good idea because v4l2_rect lacks any place for extensions. >>> But before adding it to V4L2 API, I would like to know the examples >>> of actual applications. Please, point drivers that actually need it. >> >> I have no need for it, but now that we are extending the selection API >> it would be the perfect moment to add them. > > The perfect moment for adding something is when it is needed. If we add the extended rectangles. Why using a structure that we cannot extend in the future? > > The bad idea is preventing something from being added too early. > > Regards, > Tomasz Stanislawski > >> >> They could describe properties of the sensor, like tracking ids. >> >>> >>> Other thing worth mentioning is reservation of few bits from >>> v4l2_selection::flags to describe the type of data used for >>> rectangle, v4l2_rect or v4l2_ext_rect. This way one can avoid >>> introducting v4l2_selection::rectangles field. >>> >>> I hope you find this comments useful. >> >> >> Regards >> >>> >>> Regards, >>> Tomasz Stanislawski >>> >>> >> >> >> > Regards
Hello Tomasz and Hans On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > Hi Hans, > > On 11/14/2013 11:18 AM, Hans Verkuil wrote: >> Hi Tomasz, >> >> On 11/12/13 15:54, Tomasz Stanislawski wrote: >>> Hi Ricardo, >>> Sorry for a late reply. I've been 'offline' for the last two weeks. >>> Please refer to the comments below. >>> > > [snip] > >>> >>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >>> So activation of rectangle B (setting height to non-zero value) will enable >>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >>> may disable rectangle C automatically. I do not follow what is the problem here? >> >> The problem would be in a situation like this: >> >> ...... >> .AA.B. >> ...... --> AAB >> .C.DD. CDD >> ...... >> >> A-D are the rectangles you want to select. They are cropped as shown on the >> left and composed as shown on the right. >> >>>From what Ricardo told me the resulting composed image typically must be >> a proper rectangle without padding anywhere. >> >> Trying to add rectangles one at a time breaks down when adding C because >> the composition result is no longer a 'proper' rectangle. I don't see how >> you can set something like that up one rectangle at a time. > > I see the issue but I think that it is not a big problem. > Activating C forms a non-proper rectangle with A and B. > Therefore, driver must force enabling D to form a proper rectangle again. > > I mean that instead of enlarging C to sum of width of A and B, > the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. I think this will lead to X different drivers with X different behaviours. > > The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced > to inform application which part of a buffers if going to be modified > with some undefined value. > > I see nothing against setting a padded rectangle for C to a rectangle that > covers C and D or even the whole ABCD rectangle. > I think it could be a great application for PADDED target. > The application could easily detect which part of a buffer are affected. > > Even applications prepared to work with single crop devices > would still work after enabling multi crop mode. > > The setup of rectangles my look like this. > > ************************************************ > > S_SELECTION(CROP0 = A) > > crop compose > ---------------------- > ...... > .AA... > ...... --> AA.. > ...... .... > ...... .... > > G_SELECTION(COMPOSE0) > AA.. > .... > .... > > > G_SELECTION(COMPOSE0_PADDED) > AA.. > .... > .... > > ************************************************ > > S_SELECTION(CROP1 = B) > ...... > .AA.B. > ...... --> AAB. > ...... .... > ...... .... > > G_SELECTION(COMPOSE0) > AA.. > .... > .... > > G_SELECTION(COMPOSE0_PADDED) > AAA. > .... > .... > > G_SELECTION(COMPOSE1) > ..B. > .... > .... > > G_SELECTION(COMPOSE1_PADDED) > BBB. > .... > .... > > > ************************************************ > > S_SELECTION(CROP2 = C) - D is activated implicitly > ...... > .AA.B. > .C.DD. --> AAB. > ...... CDD. > ...... .... > > G_SELECTION(COMPOSE0_PADDED) > > AAA. > AAA. > .... > > G_SELECTION(COMPOSE2) > .... > C... > .... > > > G_SELECTION(COMPOSE2_PADDED) > CCC. > CCC. > .... > > G_SELECTION(COMPOSE3) > .... > .DD. > .... > > > G_SELECTION(COMPOSE3_PADDED) > DDD. > DDD. > .... > > One may argue that all this logic is unnecessary after adding support > for multirect selections. > So, I kindly ask what should happen if someone call S_SELECTION > (in multirect mode) passing THREE rectangles A, B, and C (not D) ? > > The driver must adjust rectangles to some valid value. So it can > increase width of C or implicitly activate D or disable C. > I think that the best solution is activating D because > it allows to set size of C to the value closest to requested one. > Therefore logic for implicit activation of D should be implemented anyway. You are assuming that the user will send you the rectangles in an order that "makes sense", but it is not always the case. On the other hand if you have all the rectangles, you can ALWAYS make the better decisition. > >> >> It makes much more sense to set everything up at once. > > I agree that it is better to set everything at once. > But I strongly believe that transactions are the proper way to achieve that. > > Not multirectangle selections. > As I said before the transaction is something easier said than made. What happens if multiple users starts a transaction? What happen if in a transaction of 10 itmes, item 7 needs a readjusment by the driver? The selection API cannot cover a type of selection, therefore it should be fixed. Especially when it is something as easy as the propossed RFC. > It obfuscates API. It only pretends to fix a problem with applying > a part of configuration atomically. > >> >> BTW, what probably wasn't clear from Ricardo's explanation is that for every >> crop rectangle you must have a corresponding compose rectangle so that you >> know where to DMA it to. >> >> Your next question will be that it is a real problem that you can't set >> crop and compose simultaneously, and you are right about that. Read on for >> that... :-) >> >>> Hmm. I think that the real problem is much different. >>> Not how to set up rectangles atomically but rather >>> how do anything non-trivial atomically in V4L2. >>> >>> It would be very nice to have crop/compose and format configured at the same time. >>> However, current version of V4L2 API does not support that. >>> >>> Setting multiple crop rectangles may help a bit for only this little case. >>> But how would like to set multicrop rectangles and multicompose rectangle atomically. >> >> Why can't we extend the selection API a bit? How about this: >> >> #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 >> >> struct v4l2_selection { >> __u32 type; >> __u32 target; >> __u32 flags; >> union { >> struct v4l2_rect r; >> struct v4l2_ext_rect *pr; >> }; >> __u32 flags2; >> union { >> struct v4l2_rect r2; >> struct v4l2_ext_rect *pr2; >> }; >> __u32 rectangles; >> __u32 reserved[3]; >> }; >> >> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and >> flags2 & r2 define the compose rectangle. That way you can set them both >> atomically. > > I do not like this idea because: > - mix crop and composing generating semi-related cropcompose Now looking with some perspective the selection API, dont you think that the crop and compose are related enough to be set at the same time? > - obfuscates the API even more Agreed, but we can find better naming. > - wastes a lot of reserved fields Only 2 > - what if someone would like to use separate 'flags' for each rectangle > ... this could be a nice feature anyway :) Then he uses the ext_rect :P > > This remains me the proposition from early days of selection API. > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945 > > Refer to point 4 where v4l2_crop2 was mentioned. > >> >> I would propose that the interaction with S_FMT is as follows: if S_FMT >> defines a rectangle < then the current compose rectangle, then the >> composition (and optionally crop) rectangle is reset. If it is >=, then >> all is fine. > > There is a issue here. Long time ago it was stated that S_FMT > should result in configuration where data are processed (scaled) into > the 'whole picture'. > > It means that a COMPOSE rectangle should be reset after very S_FMT. > >> >> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should >> adapt the S_FMT rectangle to make it fit (as long as HW constraints are >> obeyed of course). > > What happens if REQBUFS was called between S_FMT and S_SELECTION? > >> >> This sequence will obey the rules of V4L2 as well: the last operation takes >> precedence over older operations. So setting S_FMT allow the driver to >> change cropping/composing to get as close to the desired format as possible. >> >>> How to define which crop rectangle refers to which to which compose rectangle. >> >> By setting the crop and compose selections at the same time and of the same >> size you can map each crop selection to a compose selection. It's all atomic >> as well. >> > > I think that adding an id field to v4l2_selection might be a good alternative > to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ... > > It will add support for all multirectangle features as cost of > sacrifice of single reserved field. Moreover id field might be > very useful for other selection targets like FOCUS or FACE. > > Other idea might to using some bits of 'flags' field to carry rectangle id. > >>> >>> What to do if one would like to change only 3rd crop rectangle? >>> >>> Introduce rectangle id into v4l2_ext_rect? >>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >>> Is it really scalable? >> >> Why not? >> > > Because it is not scalable. > An application has to use 2 syscall instead of 1. > Unnecessary copying has to be performed. And setting 8 rectangles will mean 8 iocts instead of 2. The user can also keep a copy of what has been set and there will only be 1 ioctl. > >>> >>> Multirectangle targets may seam to be a solution but I think it is not. >>> >>> I think that atomic transactions are what is really needed in V4L2. >>> Other ideas like try-context from subdev API may also help. >>> >>> It will be nice to have something like >>> >>> VIDIOC_BEGIN >>> VIDIOC_S_SELECTION (target = CROP) >>> VIDIOC_S_SELECTION (target = COMPOSE) >>> VIDIOC_S_FMT >>> VIDIOC_S_CTRL >>> VIDIOC_COMMIT >> >> I don't think S_FMT is needed here: it's something you set up at the >> beginning and don't touch afterwards. > > One could say the same about compose, crop, ctrls. > The problem are the interactions between those objects. > What is dependent on what. What can change what. > How to reach a valid state in all cases not preventing > some valid configurations from being unreachable. > >> >> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to >> solving these atomicity problems? >> >> Another nice feature that can be added to the selection API is to >> add a field to refer to a frame sequence number or a v4l2_buffer >> index (the latter is probably better): this would make it easy to >> apply an atomic crop/compose change to easily implement things like >> digital zoom or moving windows around. We could do something similar >> for controls. >> >> This would also solve the problem of assigning a per-buffer (or per-frame) >> configuration, something that libcamera2/3 needs. >> > > Introduce transactions with frame sequence numbers. > I proposed this in Cambridge in 2012. > >>> >>> and call a bunch of VIDIOC_G_* to see what really was applied. >>> >>>>> I have an auxiliary question. Do you have to set all rectangles >>>>> at once? can you set up them one by one? >>>> >>>> Also if you tell the driver what exact configuration you will need, it >>>> will provide you with the closest possible confuration, that cannot be >>> >>> s/cannot be done/may not be 'doable' >>> >>>> done if you provide rectangle by rectangle. >>>> >>>>> But how to deal with multiple rectangles? >>>> >>>> Multiple rectangles is a desired feature, please take a look to the >>>> presentation on the workshop. >>>> >>> >>> I agree that it may be useful. I just think that multirectangle selections >>> are needed to add support for such a feature. >> >> I don't follow, isn't that what this proposal adds? >> > > s/are needed/are not needed/ > > Sorry for confusion. > >>> >>>>> >>>>> Anyway, first try to define something like this: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>>> ... >>>>> >>>>> -- OR-- parametrized macros similar to one below: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>>> >>>>> The application could setup all scanout areas one-by-one. >>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>>> The height of all consecutive area would be 0. This means disabling >>>>> them effectively. >>>> >>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>>> are in ABCD and you want to go to AB. How can you do it? >>> >>> Just set C to have height 0. It will disable D automatically. >> >> It's the other direction that's the problem: how to go from A + B to >> A + B + C + D if ABC is illegal. >> >>> BTW. How are you going to emulate S_CROP by selection API >>> if you must use at least two rectangles (A + B) ? >> >> S_CROP will always just set one rectangle A. So if you had multiple >> rectangles in your selection then after S_CROP you'll have only one. > > So S_CROP should change only A. > What happens if someone already set device to work with 4 rectangles. > Then calls S_CROP that modifies A in such a way that ABCD no longer forms > a proper rectangle. > > What should happen? > - ignore S_CROP, return previous settings, or > - modify B,C,D to form a proper rectangle, or > - disable B,C,D, or ? It will be the same as setting a multicrop with 1 rectangle. > >> >> I'm assuming that the hw will always support a single crop rectangle. >> If not (extraordinarily unlikely), then S_CROP should just return an >> error. > > I agree that it is highly unlikely. I am just trying to figure out > what should happen when calling S_CROP or single-rectangle S_SELECTION > after calling multirect S_SELECTION. The sensor is configured with 1 rectangle. > >> >>> I suggest to use SCANOUT target in your first implementation. >>> Notice that as long you use something different from CROP and COMPOSE, >>> you may define interactions and dependencies any way you want, like >>> - if change of scanout area can affect composing area >>> - interaction with format and frame size >>> - check if scanout area can overlap, multi crops should be allowed to overlap >> >> I see no reason for this. A CROP_COMPOSE target does all you need. > > I think that CROP and COMPOSE is all you need. > >> >> Note that to set up N rectangles where N differs from the current >> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP >> and COMPOSE rectangles independently since having N crop rectangles >> and M compose rectangles makes no sense. If you don't change the >> number of rectangles, then you can still use CROP and COMPOSE as is. >> > > I am afraid that you are trying to kill too many birds with one stone. > I think it is a very good moment to use bazooka. > > Transactions enhanced with frame numbers will handle a lot of issues with V4L2. > > Moreover it will allow to support some nice effects like: > - multiple exposures during HDR photography > - continuous online changes preview windows > - flash at a specific frame > - change format during video streaming to grab a high-resolution picture > - support multi rectangle cropping and composing :) Even with transactions I humble believe that we should have a multirectangle call. :) > >>> > > [snip] > > >>>> I have no need for it, but now that we are extending the selection API >>>> it would be the perfect moment to add them. >>> >>> The perfect moment for adding something is when it is needed. >> >> We need to add support for multiple rectangles, so we added a pointer. >> >>> The bad idea is preventing something from being added too early. >> >> But if we make it a pointer to v4l2_rect, then we can *never* add >> additional information to each rectangle. Whenever we add new APIs >> or features we try to allow for future extensions (with mixed >> success...), so now is the time to do so for v4l2_rect. >> >> Something like this: >> >> struct v4l2_sel_rect { >> struct v4l2_rect r; >> __u32 reserved[4]; >> }; >> >> would be fine by me as well. It's an additional indirection, but it >> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect. > > We can always migrate to extended rectangle by modifying the structure to > > struct v4l2_selection { > __u32 type; > __u32 target; > __u32 flags; > union { > struct v4l2_rect r; > struct v4l2_ext_rect rext; > }; > __u32 reserved[X]; > }; > > and adding V4L2_SEL_FLAG_EXT_RECT flag. Dont you think this is a bit confusing? > >> >> Regards, >> >> Hans >> >>> > > Regards, > Tomasz Stanislawski > Regards
Hi Ricardo, Please refer to the comments below. On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: > Hello Tomasz > > Now is my time to say sorry for the delay, but i have been in holidays > and then I had a pile of work waiting on my desk :). > No problem. > > > On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski > <t.stanislaws@samsung.com> wrote: >> Hi Ricardo, >> Sorry for a late reply. I've been 'offline' for the last two weeks. >> Please refer to the comments below. [snip] >> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >> So activation of rectangle B (setting height to non-zero value) will enable >> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >> may disable rectangle C automatically. I do not follow what is the problem here? >> > > Lets say you want a configuration composed by 3 rectangles ABC and > there are no pair of rectangles with a legal configuration. You cannot > do step by step configuration. > > Also lets say that your sensor requires that the total size of the > image is 700 lines on 3 rectanges and 500 on 4. You cannot do this > configuration step by step. > > Please, give more details. I do not think that situation is that problematic. Is it even a practical limitation of hardware? >> Hmm. I think that the real problem is much different. >> Not how to set up rectangles atomically but rather >> how do anything non-trivial atomically in V4L2. >> >> It would be very nice to have crop/compose and format configured at the same time. >> However, current version of V4L2 API does not support that. >> >> Setting multiple crop rectangles may help a bit for only this little case. >> But how would like to set multicrop rectangles and multicompose rectangle atomically. >> How to define which crop rectangle refers to which to which compose rectangle. > > The number of retangles in crop are the same number of rectables in the compose. > > Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on.... > I mean something different. While using multirectangle selection, one still must use two VIDIOC_S_SELECTION calls. One to set crop rectangles, second one to set compose rectangles. So you cannot set crop and compose atomically, anyway. Hans proposed some extension to support atomic setup of both properties. However I think that it is a little overengineered. I still does not solve problems with flipping and rotations, which may have a huge impact on mulitrect cropping/composing limitations. >> >> What to do if one would like to change only 3rd crop rectangle? > > You send the whole configuration. The same as today when the user only > wants to change the pixel format. He still have to send the size. > >> >> Introduce rectangle id into v4l2_ext_rect? >> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >> Is it really scalable? > > Why it is not scalable?It is much more scalable than 8 ioctls to set > 8 rectangles. > > One has to copy_from_user/copy_to_user an arbitrary amount of data. One cannot say "I would like to change rectangle number 5". Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. When using multirect selections a kernel must copy all data to user (completely needlessly) during G_SELECTION. next, the kernel must load all rectangles back to kernel space (only one rectangle is actually needed to be copied). It is quite probable that copying 8 rectangle will NOT cause any performance issues. But copying 256 rectangles in both directions might cause an observable slowdown. >> >> Multirectangle targets may seam to be a solution but I think it is not. >> >> I think that atomic transactions are what is really needed in V4L2. >> Other ideas like try-context from subdev API may also help. >> >> It will be nice to have something like >> >> VIDIOC_BEGIN >> VIDIOC_S_SELECTION (target = CROP) >> VIDIOC_S_SELECTION (target = COMPOSE) >> VIDIOC_S_FMT >> VIDIOC_S_CTRL >> VIDIOC_COMMIT >> >> and call a bunch of VIDIOC_G_* to see what really was applied. >> > > This will trigger other isues. What we do if 2 programs starts two > transactions at the same time. We will keep a transaction array with > ALL the configurations? And what happens if there are 100? > > There are multiple ways to solve the problem. One options would be making VIDIOC_BEGIN a blocking operation so only one thread can access the critical section. Other way would be creation of temporary configuration for each file handle and applying it after VIDIOC_COMMIT. The VIDIOC_COMMIT would he handled in a driver under a mutex. There is a huge space for inventions here. >>>> I have an auxiliary question. Do you have to set all rectangles >>>> at once? can you set up them one by one? >>> >>> Also if you tell the driver what exact configuration you will need, it >>> will provide you with the closest possible confuration, that cannot be >> >> s/cannot be done/may not be 'doable' > > cannot be done. The driver cannot guess which rectangles will the user > set in the future. > >> >>> done if you provide rectangle by rectangle. >>> >>>> But how to deal with multiple rectangles? >>> >>> Multiple rectangles is a desired feature, please take a look to the >>> presentation on the workshop. >>> >> >> I agree that it may be useful. I just think that multirectangle selections >> are needed to add support for such a feature. >> >>>> >>>> Anyway, first try to define something like this: >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>> ... >>>> >>>> -- OR-- parametrized macros similar to one below: >>>> >>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>> >>>> The application could setup all scanout areas one-by-one. >>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>> The height of all consecutive area would be 0. This means disabling >>>> them effectively. >>> >>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>> are in ABCD and you want to go to AB. How can you do it? >> >> Just set C to have height 0. It will disable D automatically. > > And what if I have ABCDE and I want to go to ABE? > > As I said before the causistic of the sensors is inmense. Why limit ourselves? > Please provide some example with detailed description. >> >> BTW. How are you going to emulate S_CROP by selection API >> if you must use at least two rectangles (A + B) ? > > > Simple, you dont. > Sorry but I do not follow. Driver must setup a configuration that is the most similar to user's request. It is not allowed to return EINVAL. The selections are allowed to return -ERANGE if rectangle is not consistent with constraints. However, using constraints is optional, so applications that uses constraints expects that ioctl might fail. If HW supports crop the it should allow user to set crop. Support for one rectangle can be emulated using 2 rectangles. So I see no reason to give up support for single rectangle crop. >> >> I suggest to use SCANOUT target in your first implementation. >> Notice that as long you use something different from CROP and COMPOSE, >> you may define interactions and dependencies any way you want, like >> - if change of scanout area can affect composing area >> - interaction with format and frame size >> - check if scanout area can overlap, multi crops should be allowed to overlap >> >> See if it will be sufficient. >> See what kind of problems you encouter. > > On the process of the RFC I have used in our system all the > implementations. So far the only that has worked 100% is the solution > where the user present all the rectangles at once to the driver. > What were the problems with "the other" implementations? Maybe they could be fixed/improved to support the multirect feature without those huge API extensions. Please provide more practical details. I mean detailed HW limitations, and application needs. >> >>> >>> If yo dissable C or D, the configuration is ilegal and therefor the >>> driver will return -EINVAL. So once you are in ABCD you cannot go >>> back... >>> >> >> The driver must not return EINVAL as long as it is possible to >> adjust values passed by user to some valid configuration. >> If configuration is fixed then VIDIOC_S_SELECTION works >> effectively as VIDIOC_G_SELECTION. >> >> >>> >>>> >>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >>>> rectangle by shifting them down or resetting them to height 0. >>>> Notice that as long as targets are driver specific you are free do define >>>> interaction between the targets. >>>> >>>> I hope that proposed solution is satisfactory. >>> >>> As stated before, please follow the previous comments on the rfc, >>> specially the ones from Hans. >>> >>>> >>>> BTW. I think that the HW trick you described is not cropping. >>>> By cropping you select which part of sensor area is going >>>> to be processed into compose rectangle in a buffer. >>> >>> You are selecting part of the sensor, therefore you are cropping the image. >>> >> >> Yes, it is the same as long as you use only one rectangle. >> >> If using more then 1 the situation may become more complex. >> Currently a pair of composing and crop rectangles are used to setup scaling. >> >> You have to introduce a new definition of scaling factor if multirect crops are introduced. > > The number of rectangles must be the same on the croping and on the > composing. The scaling factor is defined by the relation between > compose[x]/crop[x] > > What happens if scaling factor must be the same for all rectangles due to HW limitations? How to choose the proper scaling factor is crop and compose rectangles are passed in separate ioctls? >> >> Moreover what happens if flipping or rotation is applied? > > Flipping is applied to the whole sensor. > >> Currently only the content of the rectangle is rotated/flipped. >> If you use multi rectangle then content of each rectangle must be >> rotated/flipped separatelly. Are you sure that your hardware can do this? > > In our hardware there is a mapping core that can map any pixel in any > location. So yes :) > > So your HW is so sophisticated that using single rectangle selections (with small tweaks) might be good enough to configure your hardware. Please provide more details. >> >> What about layout of composing rectangle inside output buffer? >> Should it reflect the layout of crop rectangle or all of them should be >> independent. >> >> Of course you can always say that everything is driver dependent :). >> >> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. > > I dont see why. When we defined the API we didn't consider the case of > multiples WOI, now we fix this. > Adding multiple instances of crop rectangle is not an issue in selection API. This API was designed to allow new targets (or ids per target) to be added. The real problem is passing all rectangles at one time. Using mutlirect selection may help a bit in your special case at a cost of API obfuscation. It still will not solve problems with interactions between format/crop/compose/flipping/rotation. Again. I think that transactions are the solution to many of those problems. I would be much more generic solutions than all hypothetical extensions to selections. >> >>>> >>>> So technicaly you still insert the whole sensor area into the buffer. >>> >>> Only the lines/columns are read into the buffer. >>> >> >> Yes, but where into a buffer? >> Does you HW support setting destination for every crop rectangle? > > The dma supports up to 256 destinations. > Nice. So may encounter performance issues while trying to change one rectangle using S_SELECTION :). >> >>>> Only part of the buffer is actually updated. So there is no cropping >>>> (cropping area is equal to the whole array). >>>> >>>> Notice, that every 'cropping' area goes to different part of a buffer. >>>> So you would need also muliple targets for composing (!!!) and very long >>>> chapter in V4L2 doc describing interactions between muliple-rectangle >>>> crops and compositions. Good luck !!! :). >>> >>> It is not that difficult to describe. >>> >>> You need the same ammount of rectangles in cropping and in compossing. >>> Rectangle X in cropping will be mapped to rectangle X in compose. >>> >>> >>>> Currently it is a hell to understand and define interaction between >>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. >>> >>> On m2m devices we are only lacking a s_fmt on the first buffer, as we >>> have discussed on the workshop. I think we only lack a good reference >>> model in vivi. >>> >>> >> >> I was not present on the workshop. Could you provide more details? >> >>>> >>>> I strongly recommend to use private selections. >>>> It will be much simpler to define, implement, and modify if needed. >>> >>> I think the private selections will lead to specific applications for >>> specific drivers and we cannot support all the configurations with >>> them. Also there is no way for an app to enumerate that capability. >> >> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? >> EINVAL means that it is not supported. > > And then we will have X zillions of SCANOUTS, that is really the Pandora box :) > > Not exactly. I preferred to introduce a bunch of SCANOUT targets to avoid wasting reserved fields in struct v4l2_selection. There is a plenty of unused bits in v4l2_selection::target. However, it is still ok to introduce v4l2_selection::index field. The API is still pretty nice. I preferred to use new target called SCANOUTs to avoid mixing this multirectangle feature is crop/compose interactions. This fear might be considered was little premature (or even paranoid). I just wanted to open this "Pandora's Box" in same safer and less demanding environment. First trying to implement everything using some non-conflicting hw-specific target called SCANOUT. If it'd worked than one would make it an alias to CROP in a newer version of the kernel. Making it a part of main API will force you to support its functionality (even erroneous) in all future versions of kernel with no possibility to change it. >> >>> >>>> >>>> BTW2. I think that the mulitple scanout areas can be modelled using >>>> media device API. Sakari may know how to do this. >>> >>> The areas are not read from the sensor. Therefore they can only be >>> proccessed on the sensor subdevice. >>> >>>> >>>> >>>> Ad 2. Extended rectangles. >>>> It is a good idea because v4l2_rect lacks any place for extensions. >>>> But before adding it to V4L2 API, I would like to know the examples >>>> of actual applications. Please, point drivers that actually need it. >>> >>> I have no need for it, but now that we are extending the selection API >>> it would be the perfect moment to add them. >> >> The perfect moment for adding something is when it is needed. > > If we add the extended rectangles. Why using a structure that we > cannot extend in the future? > Basically, because it is good enough. Introducting extended rectangles breaks symmetry between singlerect and multirect selections. v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc. Why not enhancing them with this new extended rectangle? Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy". Why setting number of rectangles changes the structure type. Please, first find a real application that really cannot be implemented without introducing extended rectangle before extending the API. The extended rectangles are an orthogonal feature to multirect selections and can be added later. Regards, Tomasz Stanislawski -- 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
Hi Sylwester, On 12/19/2013 12:09 PM, Tomasz Stanislawski wrote: > Hi Ricardo, > Please refer to the comments below. > > On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: >> Hello Tomasz >> >> Now is my time to say sorry for the delay, but i have been in holidays >> and then I had a pile of work waiting on my desk :). >> > > No problem. > >> >> >> On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> Hi Ricardo, >>> Sorry for a late reply. I've been 'offline' for the last two weeks. >>> Please refer to the comments below. > > [snip] > >>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >>> So activation of rectangle B (setting height to non-zero value) will enable >>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >>> may disable rectangle C automatically. I do not follow what is the problem here? >>> >> >> Lets say you want a configuration composed by 3 rectangles ABC and >> there are no pair of rectangles with a legal configuration. You cannot >> do step by step configuration. >> >> Also lets say that your sensor requires that the total size of the >> image is 700 lines on 3 rectanges and 500 on 4. You cannot do this >> configuration step by step. >> >> > > Please, give more details. I do not think that situation is that problematic. > Is it even a practical limitation of hardware? > >>> Hmm. I think that the real problem is much different. >>> Not how to set up rectangles atomically but rather >>> how do anything non-trivial atomically in V4L2. >>> >>> It would be very nice to have crop/compose and format configured at the same time. >>> However, current version of V4L2 API does not support that. >>> >>> Setting multiple crop rectangles may help a bit for only this little case. >>> But how would like to set multicrop rectangles and multicompose rectangle atomically. >>> How to define which crop rectangle refers to which to which compose rectangle. >> >> The number of retangles in crop are the same number of rectables in the compose. >> >> Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on.... >> > > I mean something different. > While using multirectangle selection, one still must use two > VIDIOC_S_SELECTION calls. One to set crop rectangles, > second one to set compose rectangles. > So you cannot set crop and compose atomically, anyway. > > Hans proposed some extension to support atomic setup > of both properties. However I think that it is a little overengineered. I disagree. I implemented it in vivi and it turned out to be quite easy. For the record: I'm talking about this RFC: http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822 > I still does not solve problems with flipping and rotations, which may > have a huge impact on mulitrect cropping/composing limitations. My proposal will make that much easier as well since flipping, rotating, cropping and composing are all controls/properties that can be set atomically (a control cluster). So drivers can create a single function that can handle all the relationships in one place, and applications can set all of these with one VIDIOC_S_EXT_CTRLS call. > >>> >>> What to do if one would like to change only 3rd crop rectangle? >> >> You send the whole configuration. The same as today when the user only >> wants to change the pixel format. He still have to send the size. >> >>> >>> Introduce rectangle id into v4l2_ext_rect? >>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >>> Is it really scalable? >> >> Why it is not scalable?It is much more scalable than 8 ioctls to set >> 8 rectangles. >> >> > > One has to copy_from_user/copy_to_user an arbitrary amount of data. > One cannot say "I would like to change rectangle number 5". > Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. > > When using multirect selections a kernel must copy > all data to user (completely needlessly) during G_SELECTION. > next, the kernel must load all rectangles back to kernel space > (only one rectangle is actually needed to be copied). > > It is quite probable that copying 8 rectangle will NOT cause any > performance issues. But copying 256 rectangles in both directions > might cause an observable slowdown. Adding matrix support is part of my proposal (i.e. allowing for 2D-arrays of properties/controls. An array is just a 1xN matrix). Originally I thought it would be a good idea to prefix the matrix with a struct v4l2_rect where the application could set which sub-matrix it wants to get/set. E.g. if the full 2D array is 10x20, then you could just obtain a 2x2 matrix at position 5x4. While that works, I've decided to drop that. It is quite cumbersome and I am not convinced of its usefulness. Should it be needed in the future, then it can be added (a flag would need to be added in struct v4l2_ext_control to tell the framework that the matrix is preceeded by a struct v4l2_rect). But before I would do that I would have to see a real use-case where this is a clear performance issue. Adding support for this would increase the code and add a performance penalty for the non-matrix controls/properties. > >>> >>> Multirectangle targets may seam to be a solution but I think it is not. >>> >>> I think that atomic transactions are what is really needed in V4L2. >>> Other ideas like try-context from subdev API may also help. >>> >>> It will be nice to have something like >>> >>> VIDIOC_BEGIN >>> VIDIOC_S_SELECTION (target = CROP) >>> VIDIOC_S_SELECTION (target = COMPOSE) >>> VIDIOC_S_FMT >>> VIDIOC_S_CTRL >>> VIDIOC_COMMIT >>> >>> and call a bunch of VIDIOC_G_* to see what really was applied. >>> >> >> This will trigger other isues. What we do if 2 programs starts two >> transactions at the same time. We will keep a transaction array with >> ALL the configurations? And what happens if there are 100? >> >> > > There are multiple ways to solve the problem. > One options would be making VIDIOC_BEGIN a blocking operation > so only one thread can access the critical section. > > Other way would be creation of temporary configuration for > each file handle and applying it after VIDIOC_COMMIT. > The VIDIOC_COMMIT would he handled in a driver under a mutex. > > There is a huge space for inventions here. Using the control framework for this turns out to be very nice. All the support for atomic configuration is already there. Implementing begin/commit ioctls would be very hard, and it would also be difficult to tie into the configuration store concept. Not to mention that it would basically duplicate what the control framework already does w.r.t. handling atomicity. I actually looked at what would be needed to do something like that, but I realized very quickly that in reality the control framework did almost everything I needed, and that it was much easier to add the missing pieces there then to try and invent something completely new. I hope to post a patch series for review before Christmas (fingers crossed). Personally I am quite happy with it because it adds powerful new features with a minimum amount of API (only one new ioctl: VIDIOC_QUERY_EXT_CTRL) and code churn. Regards, Hans > >>>>> I have an auxiliary question. Do you have to set all rectangles >>>>> at once? can you set up them one by one? >>>> >>>> Also if you tell the driver what exact configuration you will need, it >>>> will provide you with the closest possible confuration, that cannot be >>> >>> s/cannot be done/may not be 'doable' >> >> cannot be done. The driver cannot guess which rectangles will the user >> set in the future. >> >>> >>>> done if you provide rectangle by rectangle. >>>> >>>>> But how to deal with multiple rectangles? >>>> >>>> Multiple rectangles is a desired feature, please take a look to the >>>> presentation on the workshop. >>>> >>> >>> I agree that it may be useful. I just think that multirectangle selections >>> are needed to add support for such a feature. >>> >>>>> >>>>> Anyway, first try to define something like this: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>>> ... >>>>> >>>>> -- OR-- parametrized macros similar to one below: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>>> >>>>> The application could setup all scanout areas one-by-one. >>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>>> The height of all consecutive area would be 0. This means disabling >>>>> them effectively. >>>> >>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>>> are in ABCD and you want to go to AB. How can you do it? >>> >>> Just set C to have height 0. It will disable D automatically. >> >> And what if I have ABCDE and I want to go to ABE? >> >> As I said before the causistic of the sensors is inmense. Why limit ourselves? >> > > Please provide some example with detailed description. > >>> >>> BTW. How are you going to emulate S_CROP by selection API >>> if you must use at least two rectangles (A + B) ? >> >> >> Simple, you dont. >> > > Sorry but I do not follow. > Driver must setup a configuration that is the most similar to user's request. > It is not allowed to return EINVAL. The selections are allowed to > return -ERANGE if rectangle is not consistent with constraints. > However, using constraints is optional, so applications that uses > constraints expects that ioctl might fail. > > If HW supports crop the it should allow user to set crop. > Support for one rectangle can be emulated using 2 rectangles. > So I see no reason to give up support for single rectangle crop. > >>> >>> I suggest to use SCANOUT target in your first implementation. >>> Notice that as long you use something different from CROP and COMPOSE, >>> you may define interactions and dependencies any way you want, like >>> - if change of scanout area can affect composing area >>> - interaction with format and frame size >>> - check if scanout area can overlap, multi crops should be allowed to overlap >>> >>> See if it will be sufficient. >>> See what kind of problems you encouter. >> >> On the process of the RFC I have used in our system all the >> implementations. So far the only that has worked 100% is the solution >> where the user present all the rectangles at once to the driver. >> > > What were the problems with "the other" implementations? > Maybe they could be fixed/improved to support > the multirect feature without those huge API extensions. > Please provide more practical details. > I mean detailed HW limitations, and application needs. > >>> >>>> >>>> If yo dissable C or D, the configuration is ilegal and therefor the >>>> driver will return -EINVAL. So once you are in ABCD you cannot go >>>> back... >>>> >>> >>> The driver must not return EINVAL as long as it is possible to >>> adjust values passed by user to some valid configuration. >>> If configuration is fixed then VIDIOC_S_SELECTION works >>> effectively as VIDIOC_G_SELECTION. >>> >>> >>>> >>>>> >>>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >>>>> rectangle by shifting them down or resetting them to height 0. >>>>> Notice that as long as targets are driver specific you are free do define >>>>> interaction between the targets. >>>>> >>>>> I hope that proposed solution is satisfactory. >>>> >>>> As stated before, please follow the previous comments on the rfc, >>>> specially the ones from Hans. >>>> >>>>> >>>>> BTW. I think that the HW trick you described is not cropping. >>>>> By cropping you select which part of sensor area is going >>>>> to be processed into compose rectangle in a buffer. >>>> >>>> You are selecting part of the sensor, therefore you are cropping the image. >>>> >>> >>> Yes, it is the same as long as you use only one rectangle. >>> >>> If using more then 1 the situation may become more complex. >>> Currently a pair of composing and crop rectangles are used to setup scaling. >>> >>> You have to introduce a new definition of scaling factor if multirect crops are introduced. >> >> The number of rectangles must be the same on the croping and on the >> composing. The scaling factor is defined by the relation between >> compose[x]/crop[x] >> >> > > What happens if scaling factor must be the same for all rectangles due to HW limitations? > How to choose the proper scaling factor is crop and compose rectangles > are passed in separate ioctls? > >>> >>> Moreover what happens if flipping or rotation is applied? >> >> Flipping is applied to the whole sensor. >> >>> Currently only the content of the rectangle is rotated/flipped. >>> If you use multi rectangle then content of each rectangle must be >>> rotated/flipped separatelly. Are you sure that your hardware can do this? >> >> In our hardware there is a mapping core that can map any pixel in any >> location. So yes :) >> >> > > So your HW is so sophisticated that using single rectangle selections > (with small tweaks) might be good enough to configure your hardware. > Please provide more details. > >>> >>> What about layout of composing rectangle inside output buffer? >>> Should it reflect the layout of crop rectangle or all of them should be >>> independent. >>> >>> Of course you can always say that everything is driver dependent :). >>> >>> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. >> >> I dont see why. When we defined the API we didn't consider the case of >> multiples WOI, now we fix this. >> > > Adding multiple instances of crop rectangle is not an issue in selection API. > This API was designed to allow new targets (or ids per target) to be added. > > The real problem is passing all rectangles at one time. > Using mutlirect selection may help a bit in your special case > at a cost of API obfuscation. > It still will not solve problems with interactions between > format/crop/compose/flipping/rotation. > > Again. I think that transactions are the solution to many of those problems. > I would be much more generic solutions than all hypothetical extensions > to selections. > >>> >>>>> >>>>> So technicaly you still insert the whole sensor area into the buffer. >>>> >>>> Only the lines/columns are read into the buffer. >>>> >>> >>> Yes, but where into a buffer? >>> Does you HW support setting destination for every crop rectangle? >> >> The dma supports up to 256 destinations. >> > > Nice. So may encounter performance issues while trying to change > one rectangle using S_SELECTION :). > >>> >>>>> Only part of the buffer is actually updated. So there is no cropping >>>>> (cropping area is equal to the whole array). >>>>> >>>>> Notice, that every 'cropping' area goes to different part of a buffer. >>>>> So you would need also muliple targets for composing (!!!) and very long >>>>> chapter in V4L2 doc describing interactions between muliple-rectangle >>>>> crops and compositions. Good luck !!! :). >>>> >>>> It is not that difficult to describe. >>>> >>>> You need the same ammount of rectangles in cropping and in compossing. >>>> Rectangle X in cropping will be mapped to rectangle X in compose. >>>> >>>> >>>>> Currently it is a hell to understand and define interaction between >>>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. >>>> >>>> On m2m devices we are only lacking a s_fmt on the first buffer, as we >>>> have discussed on the workshop. I think we only lack a good reference >>>> model in vivi. >>>> >>>> >>> >>> I was not present on the workshop. Could you provide more details? >>> >>>>> >>>>> I strongly recommend to use private selections. >>>>> It will be much simpler to define, implement, and modify if needed. >>>> >>>> I think the private selections will lead to specific applications for >>>> specific drivers and we cannot support all the configurations with >>>> them. Also there is no way for an app to enumerate that capability. >>> >>> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? >>> EINVAL means that it is not supported. >> >> And then we will have X zillions of SCANOUTS, that is really the Pandora box :) >> >> > > Not exactly. I preferred to introduce a bunch of SCANOUT targets to > avoid wasting reserved fields in struct v4l2_selection. There is > a plenty of unused bits in v4l2_selection::target. However, it is still > ok to introduce v4l2_selection::index field. The API is still pretty nice. > > I preferred to use new target called SCANOUTs to avoid mixing > this multirectangle feature is crop/compose interactions. > This fear might be considered was little premature (or even paranoid). > > I just wanted to open this "Pandora's Box" in same safer > and less demanding environment. > First trying to implement everything using some > non-conflicting hw-specific target called SCANOUT. > If it'd worked than one would make it an alias to CROP in > a newer version of the kernel. > Making it a part of main API will force you to support > its functionality (even erroneous) in all future versions of kernel > with no possibility to change it. > >>> >>>> >>>>> >>>>> BTW2. I think that the mulitple scanout areas can be modelled using >>>>> media device API. Sakari may know how to do this. >>>> >>>> The areas are not read from the sensor. Therefore they can only be >>>> proccessed on the sensor subdevice. >>>> >>>>> >>>>> >>>>> Ad 2. Extended rectangles. >>>>> It is a good idea because v4l2_rect lacks any place for extensions. >>>>> But before adding it to V4L2 API, I would like to know the examples >>>>> of actual applications. Please, point drivers that actually need it. >>>> >>>> I have no need for it, but now that we are extending the selection API >>>> it would be the perfect moment to add them. >>> >>> The perfect moment for adding something is when it is needed. >> >> If we add the extended rectangles. Why using a structure that we >> cannot extend in the future? >> > > Basically, because it is good enough. > Introducting extended rectangles breaks symmetry between > singlerect and multirect selections. > v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc. > Why not enhancing them with this new extended rectangle? > Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy". > Why setting number of rectangles changes the structure type. > > Please, first find a real application that really cannot be implemented > without introducing extended rectangle before extending the API. > > The extended rectangles are an orthogonal feature to multirect selections > and can be added later. > > Regards, > > Tomasz Stanislawski > -- 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 Tomasz Thanks for your comments. On Thu, Dec 19, 2013 at 12:09 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > Hi Ricardo, > Please refer to the comments below. > > On 12/10/2013 09:37 AM, Ricardo Ribalda Delgado wrote: >> Hello Tomasz >> >> Now is my time to say sorry for the delay, but i have been in holidays >> and then I had a pile of work waiting on my desk :). >> > > No problem. > >> >> >> On Tue, Nov 12, 2013 at 3:54 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> Hi Ricardo, >>> Sorry for a late reply. I've been 'offline' for the last two weeks. >>> Please refer to the comments below. > > [snip] > >>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >>> So activation of rectangle B (setting height to non-zero value) will enable >>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >>> may disable rectangle C automatically. I do not follow what is the problem here? >>> >> >> Lets say you want a configuration composed by 3 rectangles ABC and >> there are no pair of rectangles with a legal configuration. You cannot >> do step by step configuration. >> >> Also lets say that your sensor requires that the total size of the >> image is 700 lines on 3 rectanges and 500 on 4. You cannot do this >> configuration step by step. >> >> > > Please, give more details. I do not think that situation is that problematic. > Is it even a practical limitation of hardware? We have an fpga camera that uses X cycles to read a sensor. That X is FIXED. Lets say it is 1000 lines time The sensor is read linearly from a programmable address A. for a lines You can program also address B for b lines Jumping from the area A to the area B needs j lines time. a+b+j must be equal to 1000. On the multiselect, the user sets A and B, the constrains are verified and the system is set. On your proposal Area A is set, but because the size is <1000, the driver changes the size to 1000, probably also changing the initial line a. Area B is set, the driver needs to resize the area A too, but the initial line a (what the user wants) has been replaced by a driver chossen a This is a very simple example with 2 areas, extrapolate it to 8 areas. > >>> Hmm. I think that the real problem is much different. >>> Not how to set up rectangles atomically but rather >>> how do anything non-trivial atomically in V4L2. >>> >>> It would be very nice to have crop/compose and format configured at the same time. >>> However, current version of V4L2 API does not support that. >>> >>> Setting multiple crop rectangles may help a bit for only this little case. >>> But how would like to set multicrop rectangles and multicompose rectangle atomically. >>> How to define which crop rectangle refers to which to which compose rectangle. >> >> The number of retangles in crop are the same number of rectables in the compose. >> >> Crop[0] corresponds to compose[0], crop[1]to compose[1] and so on.... >> > > I mean something different. > While using multirectangle selection, one still must use two > VIDIOC_S_SELECTION calls. One to set crop rectangles, > second one to set compose rectangles. > So you cannot set crop and compose atomically, anyway. No, but I can set all the crop rectangles at once. The driver then finds out a compatible configuration of composing. The user replaces that configuration (if needed) with a new compose. Exactly the same as today with one rectangle.. > > Hans proposed some extension to support atomic setup > of both properties. However I think that it is a little overengineered. > > I still does not solve problems with flipping and rotations, which may > have a huge impact on mulitrect cropping/composing limitations. Flip and rotation is affected to whole sensor not to the specific rectangles. > >>> >>> What to do if one would like to change only 3rd crop rectangle? >> >> You send the whole configuration. The same as today when the user only >> wants to change the pixel format. He still have to send the size. >> >>> >>> Introduce rectangle id into v4l2_ext_rect? >>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >>> Is it really scalable? >> >> Why it is not scalable?It is much more scalable than 8 ioctls to set >> 8 rectangles. >> >> > > One has to copy_from_user/copy_to_user an arbitrary amount of data. > One cannot say "I would like to change rectangle number 5". > Using single S_SELECTION for 1 rectangle is only one ioctl need to be called. If the user have saved the output of the previous s_selection he can change a specific rectangle with one s_selection call On the other hand he can even change all the rectangles with one s_selection, on your example the user has to make N s_selection calls. > > When using multirect selections a kernel must copy > all data to user (completely needlessly) during G_SELECTION. > next, the kernel must load all rectangles back to kernel space > (only one rectangle is actually needed to be copied). > > It is quite probable that copying 8 rectangle will NOT cause any > performance issues. But copying 256 rectangles in both directions > might cause an observable slowdown. 256 small ioctls are at least one order of mangniture slower that 1 ioctl with 256 times more memory send. Also bare in mind that the most probable case takes 2 or 3 rectangles, not 256. > >>> >>> Multirectangle targets may seam to be a solution but I think it is not. >>> >>> I think that atomic transactions are what is really needed in V4L2. >>> Other ideas like try-context from subdev API may also help. >>> >>> It will be nice to have something like >>> >>> VIDIOC_BEGIN >>> VIDIOC_S_SELECTION (target = CROP) >>> VIDIOC_S_SELECTION (target = COMPOSE) >>> VIDIOC_S_FMT >>> VIDIOC_S_CTRL >>> VIDIOC_COMMIT >>> >>> and call a bunch of VIDIOC_G_* to see what really was applied. >>> >> >> This will trigger other isues. What we do if 2 programs starts two >> transactions at the same time. We will keep a transaction array with >> ALL the configurations? And what happens if there are 100? >> >> > > There are multiple ways to solve the problem. > One options would be making VIDIOC_BEGIN a blocking operation > so only one thread can access the critical section. So, an app opens /dev/video0 , calls vidioc_begin, and no other user can use it. > > Other way would be creation of temporary configuration for > each file handle and applying it after VIDIOC_COMMIT. > The VIDIOC_COMMIT would he handled in a driver under a mutex. So a driver has to anyway save a multirectangle structure in his memory. The app will have to implement common algorithtms like rectangle collide, rectangle reorder, max line counting.... IMHO: Even with transaction a multirectangle set is a good idea. > > There is a huge space for inventions here. > >>>>> I have an auxiliary question. Do you have to set all rectangles >>>>> at once? can you set up them one by one? >>>> >>>> Also if you tell the driver what exact configuration you will need, it >>>> will provide you with the closest possible confuration, that cannot be >>> >>> s/cannot be done/may not be 'doable' >> >> cannot be done. The driver cannot guess which rectangles will the user >> set in the future. >> >>> >>>> done if you provide rectangle by rectangle. >>>> >>>>> But how to deal with multiple rectangles? >>>> >>>> Multiple rectangles is a desired feature, please take a look to the >>>> presentation on the workshop. >>>> >>> >>> I agree that it may be useful. I just think that multirectangle selections >>> are needed to add support for such a feature. >>> >>>>> >>>>> Anyway, first try to define something like this: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>>> ... >>>>> >>>>> -- OR-- parametrized macros similar to one below: >>>>> >>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>>> >>>>> The application could setup all scanout areas one-by-one. >>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>>> The height of all consecutive area would be 0. This means disabling >>>>> them effectively. >>>> >>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>>> are in ABCD and you want to go to AB. How can you do it? >>> >>> Just set C to have height 0. It will disable D automatically. >> >> And what if I have ABCDE and I want to go to ABE? >> >> As I said before the causistic of the sensors is inmense. Why limit ourselves? >> > > Please provide some example with detailed description. We gave 3 rectangles A: 0-10 B: 20-30 C: 40-50 we want a setup with A: 0-10 C: 40-50 If I remove B with your proposal also C is removed, so I have to create it again. Also take a look to the very first example, there are sensors that require that the total amount of lines to be X > >>> >>> BTW. How are you going to emulate S_CROP by selection API >>> if you must use at least two rectangles (A + B) ? >> >> >> Simple, you dont. >> > > Sorry but I do not follow. > Driver must setup a configuration that is the most similar to user's request. > It is not allowed to return EINVAL. The selections are allowed to > return -ERANGE if rectangle is not consistent with constraints. > However, using constraints is optional, so applications that uses > constraints expects that ioctl might fail. > > If HW supports crop the it should allow user to set crop. > Support for one rectangle can be emulated using 2 rectangles. > So I see no reason to give up support for single rectangle crop. > And what if the rectangless cannot be contiguous? I think that if the HW requires more than 1 rectangle S/G_CROP must not be implemented in the driver. >>> >>> I suggest to use SCANOUT target in your first implementation. >>> Notice that as long you use something different from CROP and COMPOSE, >>> you may define interactions and dependencies any way you want, like >>> - if change of scanout area can affect composing area >>> - interaction with format and frame size >>> - check if scanout area can overlap, multi crops should be allowed to overlap >>> >>> See if it will be sufficient. >>> See what kind of problems you encouter. >> >> On the process of the RFC I have used in our system all the >> implementations. So far the only that has worked 100% is the solution >> where the user present all the rectangles at once to the driver. >> > > What were the problems with "the other" implementations? > Maybe they could be fixed/improved to support > the multirect feature without those huge API extensions. > Please provide more practical details. > I mean detailed HW limitations, and application needs. It is mainly the first example. The driver can take more educated guesses of what the user needs if it the user gives as much information as possible. There were some setups what could not be achieved by setting the rectangles one by one > >>> >>>> >>>> If yo dissable C or D, the configuration is ilegal and therefor the >>>> driver will return -EINVAL. So once you are in ABCD you cannot go >>>> back... >>>> >>> >>> The driver must not return EINVAL as long as it is possible to >>> adjust values passed by user to some valid configuration. >>> If configuration is fixed then VIDIOC_S_SELECTION works >>> effectively as VIDIOC_G_SELECTION. >>> >>> >>>> >>>>> >>>>> The change of V4L2_SEL_TGT_XXX_SCANOUT0 would influence all consequtive >>>>> rectangle by shifting them down or resetting them to height 0. >>>>> Notice that as long as targets are driver specific you are free do define >>>>> interaction between the targets. >>>>> >>>>> I hope that proposed solution is satisfactory. >>>> >>>> As stated before, please follow the previous comments on the rfc, >>>> specially the ones from Hans. >>>> >>>>> >>>>> BTW. I think that the HW trick you described is not cropping. >>>>> By cropping you select which part of sensor area is going >>>>> to be processed into compose rectangle in a buffer. >>>> >>>> You are selecting part of the sensor, therefore you are cropping the image. >>>> >>> >>> Yes, it is the same as long as you use only one rectangle. >>> >>> If using more then 1 the situation may become more complex. >>> Currently a pair of composing and crop rectangles are used to setup scaling. >>> >>> You have to introduce a new definition of scaling factor if multirect crops are introduced. >> >> The number of rectangles must be the same on the croping and on the >> composing. The scaling factor is defined by the relation between >> compose[x]/crop[x] >> >> > > What happens if scaling factor must be the same for all rectangles due to HW limitations? > How to choose the proper scaling factor is crop and compose rectangles > are passed in separate ioctls? How do we solve that today? Why it is different? > >>> >>> Moreover what happens if flipping or rotation is applied? >> >> Flipping is applied to the whole sensor. >> >>> Currently only the content of the rectangle is rotated/flipped. >>> If you use multi rectangle then content of each rectangle must be >>> rotated/flipped separatelly. Are you sure that your hardware can do this? >> >> In our hardware there is a mapping core that can map any pixel in any >> location. So yes :) >> >> > > So your HW is so sophisticated that using single rectangle selections > (with small tweaks) might be good enough to configure your hardware. > Please provide more details. We have a working window A. Every pixel (x,y) can be remapped to any pixel in the range (0,y-A) (MAX_X,y+A). So we still need multirectangles. Also, the core if after the sensor, and the best benefit is gained if the data is NOT read from the sensor. > >>> >>> What about layout of composing rectangle inside output buffer? >>> Should it reflect the layout of crop rectangle or all of them should be >>> independent. >>> >>> Of course you can always say that everything is driver dependent :). >>> >>> Anyway, adding multirectangle selections for crop/compose is openning Pandora's Box. >> >> I dont see why. When we defined the API we didn't consider the case of >> multiples WOI, now we fix this. >> > > Adding multiple instances of crop rectangle is not an issue in selection API. > This API was designed to allow new targets (or ids per target) to be added. > > The real problem is passing all rectangles at one time. > Using mutlirect selection may help a bit in your special case > at a cost of API obfuscation. > It still will not solve problems with interactions between > format/crop/compose/flipping/rotation. > > Again. I think that transactions are the solution to many of those problems. > I would be much more generic solutions than all hypothetical extensions > to selections. > >>> >>>>> >>>>> So technicaly you still insert the whole sensor area into the buffer. >>>> >>>> Only the lines/columns are read into the buffer. >>>> >>> >>> Yes, but where into a buffer? >>> Does you HW support setting destination for every crop rectangle? >> >> The dma supports up to 256 destinations. >> > > Nice. So may encounter performance issues while trying to change > one rectangle using S_SELECTION :). Less that trying to change all the rectangles at once :P > >>> >>>>> Only part of the buffer is actually updated. So there is no cropping >>>>> (cropping area is equal to the whole array). >>>>> >>>>> Notice, that every 'cropping' area goes to different part of a buffer. >>>>> So you would need also muliple targets for composing (!!!) and very long >>>>> chapter in V4L2 doc describing interactions between muliple-rectangle >>>>> crops and compositions. Good luck !!! :). >>>> >>>> It is not that difficult to describe. >>>> >>>> You need the same ammount of rectangles in cropping and in compossing. >>>> Rectangle X in cropping will be mapped to rectangle X in compose. >>>> >>>> >>>>> Currently it is a hell to understand and define interaction between >>>>> single crop, and compose and unfamous VIDIOC_S_FMT and m2m madness. >>>> >>>> On m2m devices we are only lacking a s_fmt on the first buffer, as we >>>> have discussed on the workshop. I think we only lack a good reference >>>> model in vivi. >>>> >>>> >>> >>> I was not present on the workshop. Could you provide more details? >>> >>>>> >>>>> I strongly recommend to use private selections. >>>>> It will be much simpler to define, implement, and modify if needed. >>>> >>>> I think the private selections will lead to specific applications for >>>> specific drivers and we cannot support all the configurations with >>>> them. Also there is no way for an app to enumerate that capability. >>> >>> call VIDIOC_G_SELECTION to check if a given SCANOUT is supported? >>> EINVAL means that it is not supported. >> >> And then we will have X zillions of SCANOUTS, that is really the Pandora box :) >> >> > > Not exactly. I preferred to introduce a bunch of SCANOUT targets to > avoid wasting reserved fields in struct v4l2_selection. There is > a plenty of unused bits in v4l2_selection::target. However, it is still > ok to introduce v4l2_selection::index field. The API is still pretty nice. > > I preferred to use new target called SCANOUTs to avoid mixing > this multirectangle feature is crop/compose interactions. > This fear might be considered was little premature (or even paranoid). > > I just wanted to open this "Pandora's Box" in same safer > and less demanding environment. > First trying to implement everything using some > non-conflicting hw-specific target called SCANOUT. > If it'd worked than one would make it an alias to CROP in > a newer version of the kernel. > Making it a part of main API will force you to support > its functionality (even erroneous) in all future versions of kernel > with no possibility to change it. > >>> >>>> >>>>> >>>>> BTW2. I think that the mulitple scanout areas can be modelled using >>>>> media device API. Sakari may know how to do this. >>>> >>>> The areas are not read from the sensor. Therefore they can only be >>>> proccessed on the sensor subdevice. >>>> >>>>> >>>>> >>>>> Ad 2. Extended rectangles. >>>>> It is a good idea because v4l2_rect lacks any place for extensions. >>>>> But before adding it to V4L2 API, I would like to know the examples >>>>> of actual applications. Please, point drivers that actually need it. >>>> >>>> I have no need for it, but now that we are extending the selection API >>>> it would be the perfect moment to add them. >>> >>> The perfect moment for adding something is when it is needed. >> >> If we add the extended rectangles. Why using a structure that we >> cannot extend in the future? >> > > Basically, because it is good enough. > Introducting extended rectangles breaks symmetry between > singlerect and multirect selections. > v4l2_rect is used in other structures like v4l2_clip, v4l2_crop, v4l2_window, etc. > Why not enhancing them with this new extended rectangle? > Moreover, it violates "Rule of Least Surprise" from "Unix Philosophy". > Why setting number of rectangles changes the structure type. > > Please, first find a real application that really cannot be implemented > without introducing extended rectangle before extending the API. > > The extended rectangles are an orthogonal feature to multirect selections > and can be added later. > > Regards, > > Tomasz Stanislawski > I am ok with not adding the ext_rect, but I really think we need the multirect, even with transactions. The multiple rectangle via id uses the same amount of reserved fields but requires more ioctl calls. The multiple rectangle will be more deterministic that setting one rectangle at a time, because the driver has to take decisions for every rectangle set. With multiple rectangle we can create a bunch of function helpers that can be used by the device driver, instead of reinventing the wheel every-time. Multirectangle is just the equivalent of multi control set/get. Regards!
On 12/19/2013 12:45 PM, Hans Verkuil wrote:
> Hi Sylwester,
That should be Tomasz, of course :-)
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
Hi Ricardo, On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote: > Hello Tomasz and Hans > > On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski > <t.stanislaws@samsung.com> wrote: >> Hi Hans, >> >> On 11/14/2013 11:18 AM, Hans Verkuil wrote: >>> Hi Tomasz, >>> >>> On 11/12/13 15:54, Tomasz Stanislawski wrote: >>>> Hi Ricardo, >>>> Sorry for a late reply. I've been 'offline' for the last two weeks. >>>> Please refer to the comments below. >>>> >> >> [snip] >> >>>> >>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >>>> So activation of rectangle B (setting height to non-zero value) will enable >>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >>>> may disable rectangle C automatically. I do not follow what is the problem here? >>> >>> The problem would be in a situation like this: >>> >>> ...... >>> .AA.B. >>> ...... --> AAB >>> .C.DD. CDD >>> ...... >>> >>> A-D are the rectangles you want to select. They are cropped as shown on the >>> left and composed as shown on the right. >>> >>> >From what Ricardo told me the resulting composed image typically must be >>> a proper rectangle without padding anywhere. >>> >>> Trying to add rectangles one at a time breaks down when adding C because >>> the composition result is no longer a 'proper' rectangle. I don't see how >>> you can set something like that up one rectangle at a time. >> >> I see the issue but I think that it is not a big problem. >> Activating C forms a non-proper rectangle with A and B. >> Therefore, driver must force enabling D to form a proper rectangle again. >> >> I mean that instead of enlarging C to sum of width of A and B, >> the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. > > I think this will lead to X different drivers with X different behaviours. > I sadly have to agree. All drivers behave differently. But, is it really an issue? As I understand a developer is responsible to assure that future versions of driver works the same (in practical context) when used by old applications. The driver is allowed to interpret/adjust user calls. This includes even ignoring them. >> >> The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced >> to inform application which part of a buffers if going to be modified >> with some undefined value. >> >> I see nothing against setting a padded rectangle for C to a rectangle that >> covers C and D or even the whole ABCD rectangle. >> I think it could be a great application for PADDED target. >> The application could easily detect which part of a buffer are affected. >> >> Even applications prepared to work with single crop devices >> would still work after enabling multi crop mode. >> >> The setup of rectangles my look like this. >> >> ************************************************ >> >> S_SELECTION(CROP0 = A) >> >> crop compose >> ---------------------- >> ...... >> .AA... >> ...... --> AA.. >> ...... .... >> ...... .... >> >> G_SELECTION(COMPOSE0) >> AA.. >> .... >> .... >> >> >> G_SELECTION(COMPOSE0_PADDED) >> AA.. >> .... >> .... >> >> ************************************************ >> >> S_SELECTION(CROP1 = B) >> ...... >> .AA.B. >> ...... --> AAB. >> ...... .... >> ...... .... >> >> G_SELECTION(COMPOSE0) >> AA.. >> .... >> .... >> >> G_SELECTION(COMPOSE0_PADDED) >> AAA. >> .... >> .... >> >> G_SELECTION(COMPOSE1) >> ..B. >> .... >> .... >> >> G_SELECTION(COMPOSE1_PADDED) >> BBB. >> .... >> .... >> >> >> ************************************************ >> >> S_SELECTION(CROP2 = C) - D is activated implicitly >> ...... >> .AA.B. >> .C.DD. --> AAB. >> ...... CDD. >> ...... .... >> >> G_SELECTION(COMPOSE0_PADDED) >> >> AAA. >> AAA. >> .... >> >> G_SELECTION(COMPOSE2) >> .... >> C... >> .... >> >> >> G_SELECTION(COMPOSE2_PADDED) >> CCC. >> CCC. >> .... >> >> G_SELECTION(COMPOSE3) >> .... >> .DD. >> .... >> >> >> G_SELECTION(COMPOSE3_PADDED) >> DDD. >> DDD. >> .... >> >> One may argue that all this logic is unnecessary after adding support >> for multirect selections. >> So, I kindly ask what should happen if someone call S_SELECTION >> (in multirect mode) passing THREE rectangles A, B, and C (not D) ? >> >> The driver must adjust rectangles to some valid value. So it can >> increase width of C or implicitly activate D or disable C. >> I think that the best solution is activating D because >> it allows to set size of C to the value closest to requested one. >> Therefore logic for implicit activation of D should be implemented anyway. > > You are assuming that the user will send you the rectangles in an > order that "makes sense", but it is not always the case. On the other > hand if you have all the rectangles, you can ALWAYS make the better > decisition. > Yes. I assume that a user will send rectangles in the right order. It stated that changing order of V4L2 operations may result is different configuration. So I do not see a problem that a specific configuration can be achieved on specific hardware only if operations are performed in specific order. > On the other > hand if you have all the rectangles, you can ALWAYS make the better > decisition. I agree completely. To make a best decision you must have "all rectangles". It means both cropping/composing and format which is also dependent of S_CROP (see V4L2 doc). Moreover, having controls like rotation and flipping may be helpful. So passing only crop rectangles is only a part of information needed to make a best decision. Therefore, multirectangle selections are only a partial solution to the problem of configuration. It is a workaround for the specific feature of some hardware. Therefore, before merging multirect selections you must PROVE that the existing solutions are not satisfactory in your case. That you truly cannot use singlerect selections to configure your piece of hardware (not some abstract HW). You cannot say that it is impossible because you do not know how to do it. I just want to avoid loosing time and obfuscating the API for a feature that will not be even sufficient for future needs. >> >>> >>> It makes much more sense to set everything up at once. >> >> I agree that it is better to set everything at once. >> But I strongly believe that transactions are the proper way to achieve that. >> >> Not multirectangle selections. >> > > As I said before the transaction is something easier said than made. > What happens if multiple users starts a transaction? What happen if in > a transaction of 10 itmes, item 7 needs a readjusment by the driver? > It is a very good question. I have some ideas about transactions. First, that there should some form of temporary configuration buffer. This buffer could be singular. In such a case VIDIOC_BEGIN would be a blocking ioctl. Or every handle/thread could have its own configuration buffer. The buffer would be processed at VIDIOC_COMMIT. The values would be adjusted against HW limitations, current configuration and the buffer itself. Finally, all properties would be applied to HW. As I said in previous email. There is a huge space for ideas. > The selection API cannot cover a type of selection, therefore it > should be fixed. Especially when it is something as easy as the > propossed RFC. No. It is by no means easy. It just pretends to be easy. There is a very complex logic hidden behind this extension. Just add v4l2_selection::index if you want to avoid utilizing bits from v4l2_selection::target. Index is nice because 0 would refer to first and usually only one crop rectangle. Those are relatively simple to add with few lines of patch. You can even use CROP target instead of SCANOUT. You will responsible for fighting with nightmare of backward compatibility and messy business logic hidden in driver code. I am strongly against passing all rectangles at the same time. The simultaneous configuration should be a separate part of V4L2 API. > > >> It obfuscates API. It only pretends to fix a problem with applying >> a part of configuration atomically. >> >>> >>> BTW, what probably wasn't clear from Ricardo's explanation is that for every >>> crop rectangle you must have a corresponding compose rectangle so that you >>> know where to DMA it to. >>> >>> Your next question will be that it is a real problem that you can't set >>> crop and compose simultaneously, and you are right about that. Read on for >>> that... :-) >>> >>>> Hmm. I think that the real problem is much different. >>>> Not how to set up rectangles atomically but rather >>>> how do anything non-trivial atomically in V4L2. >>>> >>>> It would be very nice to have crop/compose and format configured at the same time. >>>> However, current version of V4L2 API does not support that. >>>> >>>> Setting multiple crop rectangles may help a bit for only this little case. >>>> But how would like to set multicrop rectangles and multicompose rectangle atomically. >>> >>> Why can't we extend the selection API a bit? How about this: >>> >>> #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 >>> >>> struct v4l2_selection { >>> __u32 type; >>> __u32 target; >>> __u32 flags; >>> union { >>> struct v4l2_rect r; >>> struct v4l2_ext_rect *pr; >>> }; >>> __u32 flags2; >>> union { >>> struct v4l2_rect r2; >>> struct v4l2_ext_rect *pr2; >>> }; >>> __u32 rectangles; >>> __u32 reserved[3]; >>> }; >>> >>> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and >>> flags2 & r2 define the compose rectangle. That way you can set them both >>> atomically. >> >> I do not like this idea because: >> - mix crop and composing generating semi-related cropcompose > Now looking with some perspective the selection API, dont you think > that the crop and compose are related enough to be set at the same > time? > Technically everything in V4L2 is more or less related. I expect that in future we may see Frankenstein's monster that sets up multirectangle crops, compose, format and some important controls using single ioctl with a huge struct. Probably, this would sufficient ... for some time. I am simply tired of all those "almost solutions". The API should be modular. It should be made of simple parts that do one thing well. >> - obfuscates the API even more > Agreed, but we can find better naming. > >> - wastes a lot of reserved fields > Only 2 There are new fields named flags2 (1 word), second union (4 words), rectangles (1 word). So 6 extra words are wasted. > >> - what if someone would like to use separate 'flags' for each rectangle >> ... this could be a nice feature anyway :) > Then he uses the ext_rect :P > Or use only single rectangle selections. >> >> This remains me the proposition from early days of selection API. >> >> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945 >> >> Refer to point 4 where v4l2_crop2 was mentioned. >> >>> >>> I would propose that the interaction with S_FMT is as follows: if S_FMT >>> defines a rectangle < then the current compose rectangle, then the >>> composition (and optionally crop) rectangle is reset. If it is >=, then >>> all is fine. >> >> There is a issue here. Long time ago it was stated that S_FMT >> should result in configuration where data are processed (scaled) into >> the 'whole picture'. >> >> It means that a COMPOSE rectangle should be reset after very S_FMT. >> >>> >>> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should >>> adapt the S_FMT rectangle to make it fit (as long as HW constraints are >>> obeyed of course). >> >> What happens if REQBUFS was called between S_FMT and S_SELECTION? >> >>> >>> This sequence will obey the rules of V4L2 as well: the last operation takes >>> precedence over older operations. So setting S_FMT allow the driver to >>> change cropping/composing to get as close to the desired format as possible. >>> >>>> How to define which crop rectangle refers to which to which compose rectangle. >>> >>> By setting the crop and compose selections at the same time and of the same >>> size you can map each crop selection to a compose selection. It's all atomic >>> as well. >>> >> >> I think that adding an id field to v4l2_selection might be a good alternative >> to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ... >> >> It will add support for all multirectangle features as cost of >> sacrifice of single reserved field. Moreover id field might be >> very useful for other selection targets like FOCUS or FACE. >> >> Other idea might to using some bits of 'flags' field to carry rectangle id. >> >>>> >>>> What to do if one would like to change only 3rd crop rectangle? >>>> >>>> Introduce rectangle id into v4l2_ext_rect? >>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >>>> Is it really scalable? >>> >>> Why not? >>> >> >> Because it is not scalable. >> An application has to use 2 syscall instead of 1. >> Unnecessary copying has to be performed. > > And setting 8 rectangles will mean 8 iocts instead of 2. The very similar amount of data is copied in both cases. sizeof(v4l2_selection) + N * sizeof (v4l2_ext_rect) OR N * sizeof(v4l2_selection) > > The user can also keep a copy of what has been set and there will only > be 1 ioctl. > Notice that user must update this copy every time S_SELECTION is called. Moreover it must always use this array whenever calling S_SELECTION. If one use single-rectangle selections there is no need to keep all those bookkeeping data. Nor assure its consistency. >> >>>> >>>> Multirectangle targets may seam to be a solution but I think it is not. >>>> >>>> I think that atomic transactions are what is really needed in V4L2. >>>> Other ideas like try-context from subdev API may also help. >>>> >>>> It will be nice to have something like >>>> >>>> VIDIOC_BEGIN >>>> VIDIOC_S_SELECTION (target = CROP) >>>> VIDIOC_S_SELECTION (target = COMPOSE) >>>> VIDIOC_S_FMT >>>> VIDIOC_S_CTRL >>>> VIDIOC_COMMIT >>> >>> I don't think S_FMT is needed here: it's something you set up at the >>> beginning and don't touch afterwards. >> >> One could say the same about compose, crop, ctrls. >> The problem are the interactions between those objects. >> What is dependent on what. What can change what. >> How to reach a valid state in all cases not preventing >> some valid configurations from being unreachable. >> >>> >>> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to >>> solving these atomicity problems? >>> >>> Another nice feature that can be added to the selection API is to >>> add a field to refer to a frame sequence number or a v4l2_buffer >>> index (the latter is probably better): this would make it easy to >>> apply an atomic crop/compose change to easily implement things like >>> digital zoom or moving windows around. We could do something similar >>> for controls. >>> >>> This would also solve the problem of assigning a per-buffer (or per-frame) >>> configuration, something that libcamera2/3 needs. >>> >> >> Introduce transactions with frame sequence numbers. >> I proposed this in Cambridge in 2012. >> >>>> >>>> and call a bunch of VIDIOC_G_* to see what really was applied. >>>> >>>>>> I have an auxiliary question. Do you have to set all rectangles >>>>>> at once? can you set up them one by one? >>>>> >>>>> Also if you tell the driver what exact configuration you will need, it >>>>> will provide you with the closest possible confuration, that cannot be >>>> >>>> s/cannot be done/may not be 'doable' >>>> >>>>> done if you provide rectangle by rectangle. >>>>> >>>>>> But how to deal with multiple rectangles? >>>>> >>>>> Multiple rectangles is a desired feature, please take a look to the >>>>> presentation on the workshop. >>>>> >>>> >>>> I agree that it may be useful. I just think that multirectangle selections >>>> are needed to add support for such a feature. >>> >>> I don't follow, isn't that what this proposal adds? >>> >> >> s/are needed/are not needed/ >> >> Sorry for confusion. >> >>>> >>>>>> >>>>>> Anyway, first try to define something like this: >>>>>> >>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>>>> >>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>>>> ... >>>>>> >>>>>> -- OR-- parametrized macros similar to one below: >>>>>> >>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>>>> >>>>>> The application could setup all scanout areas one-by-one. >>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>>>> The height of all consecutive area would be 0. This means disabling >>>>>> them effectively. >>>>> >>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>>>> are in ABCD and you want to go to AB. How can you do it? >>>> >>>> Just set C to have height 0. It will disable D automatically. >>> >>> It's the other direction that's the problem: how to go from A + B to >>> A + B + C + D if ABC is illegal. >>> >>>> BTW. How are you going to emulate S_CROP by selection API >>>> if you must use at least two rectangles (A + B) ? >>> >>> S_CROP will always just set one rectangle A. So if you had multiple >>> rectangles in your selection then after S_CROP you'll have only one. >> >> So S_CROP should change only A. >> What happens if someone already set device to work with 4 rectangles. >> Then calls S_CROP that modifies A in such a way that ABCD no longer forms >> a proper rectangle. >> >> What should happen? >> - ignore S_CROP, return previous settings, or >> - modify B,C,D to form a proper rectangle, or >> - disable B,C,D, or ? > > It will be the same as setting a multicrop with 1 rectangle. > It not the answer to my question. I expect that your answer refers to "disable B,C,D" variant, doesn't it? >> >>> >>> I'm assuming that the hw will always support a single crop rectangle. >>> If not (extraordinarily unlikely), then S_CROP should just return an >>> error. >> >> I agree that it is highly unlikely. I am just trying to figure out >> what should happen when calling S_CROP or single-rectangle S_SELECTION >> after calling multirect S_SELECTION. > > The sensor is configured with 1 rectangle. > >> >>> >>>> I suggest to use SCANOUT target in your first implementation. >>>> Notice that as long you use something different from CROP and COMPOSE, >>>> you may define interactions and dependencies any way you want, like >>>> - if change of scanout area can affect composing area >>>> - interaction with format and frame size >>>> - check if scanout area can overlap, multi crops should be allowed to overlap >>> >>> I see no reason for this. A CROP_COMPOSE target does all you need. >> >> I think that CROP and COMPOSE is all you need. >> >>> >>> Note that to set up N rectangles where N differs from the current >>> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP >>> and COMPOSE rectangles independently since having N crop rectangles >>> and M compose rectangles makes no sense. If you don't change the >>> number of rectangles, then you can still use CROP and COMPOSE as is. >>> >> >> I am afraid that you are trying to kill too many birds with one stone. >> I think it is a very good moment to use bazooka. >> >> Transactions enhanced with frame numbers will handle a lot of issues with V4L2. >> >> Moreover it will allow to support some nice effects like: >> - multiple exposures during HDR photography >> - continuous online changes preview windows >> - flash at a specific frame >> - change format during video streaming to grab a high-resolution picture >> - support multi rectangle cropping and composing :) > > Even with transactions I humble believe that we should have a > multirectangle call. :) > > I humbly believe that single rectangle selections are sufficient. Please, try to convince me that I am wrong. If I was sure that this multirect feature cannot be implemented using single-rect selections I will support this extension. Even if this feature is not future-proof as mentioned earlier. We may let transactions to be a part of V4L3 :). By now, I am still not convinced. >> >>>> >> >> [snip] >> >> >>>>> I have no need for it, but now that we are extending the selection API >>>>> it would be the perfect moment to add them. >>>> >>>> The perfect moment for adding something is when it is needed. >>> >>> We need to add support for multiple rectangles, so we added a pointer. >>> >>>> The bad idea is preventing something from being added too early. >>> >>> But if we make it a pointer to v4l2_rect, then we can *never* add >>> additional information to each rectangle. Whenever we add new APIs >>> or features we try to allow for future extensions (with mixed >>> success...), so now is the time to do so for v4l2_rect. >>> >>> Something like this: >>> >>> struct v4l2_sel_rect { >>> struct v4l2_rect r; >>> __u32 reserved[4]; >>> }; >>> >>> would be fine by me as well. It's an additional indirection, but it >>> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect. >> >> We can always migrate to extended rectangle by modifying the structure to >> >> struct v4l2_selection { >> __u32 type; >> __u32 target; >> __u32 flags; >> union { >> struct v4l2_rect r; >> struct v4l2_ext_rect rext; >> }; >> __u32 reserved[X]; >> }; >> >> and adding V4L2_SEL_FLAG_EXT_RECT flag. > > Dont you think this is a bit confusing? > > > Why? It is much less confusing then union type dependent on value of v4l2_selection::rectangles. You can use pointer to struct v4l2_ext_rect instead of composition. Then only 1 bit could be used to support extended rectangles. Regards, Tomasz Stanislawski -- 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 Tomasz On Thu, Dec 19, 2013 at 1:34 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > Hi Ricardo, > > On 12/10/2013 10:46 AM, Ricardo Ribalda Delgado wrote: >> Hello Tomasz and Hans >> >> On Thu, Nov 14, 2013 at 4:40 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> Hi Hans, >>> >>> On 11/14/2013 11:18 AM, Hans Verkuil wrote: >>>> Hi Tomasz, >>>> >>>> On 11/12/13 15:54, Tomasz Stanislawski wrote: >>>>> Hi Ricardo, >>>>> Sorry for a late reply. I've been 'offline' for the last two weeks. >>>>> Please refer to the comments below. >>>>> >>> >>> [snip] >>> >>>>> >>>>> As I said. Changes of rectangle n may trigger changes in rectangle n+1 and so on. >>>>> So activation of rectangle B (setting height to non-zero value) will enable >>>>> rectangle C with some default size. Moreover disabling rectangle B (setting height to 0) >>>>> may disable rectangle C automatically. I do not follow what is the problem here? >>>> >>>> The problem would be in a situation like this: >>>> >>>> ...... >>>> .AA.B. >>>> ...... --> AAB >>>> .C.DD. CDD >>>> ...... >>>> >>>> A-D are the rectangles you want to select. They are cropped as shown on the >>>> left and composed as shown on the right. >>>> >>>> >From what Ricardo told me the resulting composed image typically must be >>>> a proper rectangle without padding anywhere. >>>> >>>> Trying to add rectangles one at a time breaks down when adding C because >>>> the composition result is no longer a 'proper' rectangle. I don't see how >>>> you can set something like that up one rectangle at a time. >>> >>> I see the issue but I think that it is not a big problem. >>> Activating C forms a non-proper rectangle with A and B. >>> Therefore, driver must force enabling D to form a proper rectangle again. >>> >>> I mean that instead of enlarging C to sum of width of A and B, >>> the driver can implicitly activate D to ensure that A,B,C,D form a proper rectangle. >> >> I think this will lead to X different drivers with X different behaviours. >> > > I sadly have to agree. All drivers behave differently. > But, is it really an issue? Well, I would say that we have APIs to abstract HW from SW. I expect the same behaviour on every mouse I connect on my computer :) > > As I understand a developer is responsible to assure that > future versions of driver works the same (in practical context) > when used by old applications. > > The driver is allowed to interpret/adjust user calls. > This includes even ignoring them. > >>> >>> The special target called V4L2_SEL_TGT_COMPOSE_PADDED was introduced >>> to inform application which part of a buffers if going to be modified >>> with some undefined value. >>> >>> I see nothing against setting a padded rectangle for C to a rectangle that >>> covers C and D or even the whole ABCD rectangle. >>> I think it could be a great application for PADDED target. >>> The application could easily detect which part of a buffer are affected. >>> >>> Even applications prepared to work with single crop devices >>> would still work after enabling multi crop mode. >>> >>> The setup of rectangles my look like this. >>> >>> ************************************************ >>> >>> S_SELECTION(CROP0 = A) >>> >>> crop compose >>> ---------------------- >>> ...... >>> .AA... >>> ...... --> AA.. >>> ...... .... >>> ...... .... >>> >>> G_SELECTION(COMPOSE0) >>> AA.. >>> .... >>> .... >>> >>> >>> G_SELECTION(COMPOSE0_PADDED) >>> AA.. >>> .... >>> .... >>> >>> ************************************************ >>> >>> S_SELECTION(CROP1 = B) >>> ...... >>> .AA.B. >>> ...... --> AAB. >>> ...... .... >>> ...... .... >>> >>> G_SELECTION(COMPOSE0) >>> AA.. >>> .... >>> .... >>> >>> G_SELECTION(COMPOSE0_PADDED) >>> AAA. >>> .... >>> .... >>> >>> G_SELECTION(COMPOSE1) >>> ..B. >>> .... >>> .... >>> >>> G_SELECTION(COMPOSE1_PADDED) >>> BBB. >>> .... >>> .... >>> >>> >>> ************************************************ >>> >>> S_SELECTION(CROP2 = C) - D is activated implicitly >>> ...... >>> .AA.B. >>> .C.DD. --> AAB. >>> ...... CDD. >>> ...... .... >>> >>> G_SELECTION(COMPOSE0_PADDED) >>> >>> AAA. >>> AAA. >>> .... >>> >>> G_SELECTION(COMPOSE2) >>> .... >>> C... >>> .... >>> >>> >>> G_SELECTION(COMPOSE2_PADDED) >>> CCC. >>> CCC. >>> .... >>> >>> G_SELECTION(COMPOSE3) >>> .... >>> .DD. >>> .... >>> >>> >>> G_SELECTION(COMPOSE3_PADDED) >>> DDD. >>> DDD. >>> .... >>> >>> One may argue that all this logic is unnecessary after adding support >>> for multirect selections. >>> So, I kindly ask what should happen if someone call S_SELECTION >>> (in multirect mode) passing THREE rectangles A, B, and C (not D) ? >>> >>> The driver must adjust rectangles to some valid value. So it can >>> increase width of C or implicitly activate D or disable C. >>> I think that the best solution is activating D because >>> it allows to set size of C to the value closest to requested one. >>> Therefore logic for implicit activation of D should be implemented anyway. >> >> You are assuming that the user will send you the rectangles in an >> order that "makes sense", but it is not always the case. On the other >> hand if you have all the rectangles, you can ALWAYS make the better >> decisition. >> > > Yes. I assume that a user will send rectangles in the right order. > It stated that changing order of V4L2 operations may result > is different configuration. So I do not see a problem that > a specific configuration can be achieved on specific hardware > only if operations are performed in specific order. What is the right order? Why the software should know anything about the hardware to procure a proper image? > >> On the other >> hand if you have all the rectangles, you can ALWAYS make the better >> decisition. > > I agree completely. > To make a best decision you must have "all rectangles". It means both > cropping/composing and format which is also dependent of S_CROP (see V4L2 doc). > Moreover, having controls like rotation and flipping may be helpful. > > So passing only crop rectangles is only a part of information needed to make > a best decision. Therefore, multirectangle selections are only a partial solution > to the problem of configuration. It is a workaround for the specific feature > of some hardware. Therefore, before merging multirect selections you must PROVE > that the existing solutions are not satisfactory in your case. > That you truly cannot use singlerect selections to configure > your piece of hardware (not some abstract HW). > You cannot say that it is impossible because you do not know how to do it. Please take a look to the example from my previous mail, which is a real camera. If the total size depends of the number of rectangles I can not see how to do it right. > > I just want to avoid loosing time and obfuscating the API for a feature > that will not be even sufficient for future needs. > >>> >>>> >>>> It makes much more sense to set everything up at once. >>> >>> I agree that it is better to set everything at once. >>> But I strongly believe that transactions are the proper way to achieve that. >>> >>> Not multirectangle selections. >>> >> >> As I said before the transaction is something easier said than made. >> What happens if multiple users starts a transaction? What happen if in >> a transaction of 10 itmes, item 7 needs a readjusment by the driver? >> > > It is a very good question. > I have some ideas about transactions. > First, that there should some form of temporary configuration buffer. > This buffer could be singular. In such a case VIDIOC_BEGIN would > be a blocking ioctl. > Or every handle/thread could have its own configuration buffer. > The buffer would be processed at VIDIOC_COMMIT. > The values would be adjusted against HW limitations, current configuration > and the buffer itself. > Finally, all properties would be applied to HW. > > As I said in previous email. There is a huge space for ideas. But even with transactions we need a way to define multiple rectangles. If we provide common api we will be able to reuse helper functions and have similar behaviour on all the drivers. > >> The selection API cannot cover a type of selection, therefore it >> should be fixed. Especially when it is something as easy as the >> propossed RFC. > > No. It is by no means easy. It just pretends to be easy. > There is a very complex logic hidden behind this extension. Where is the complexity? > > Just add v4l2_selection::index if you want to avoid utilizing bits > from v4l2_selection::target. Index is nice because 0 would > refer to first and usually only one crop rectangle. > Those are relatively simple to add with few lines of patch. The current rfc is only 150 lines of code, including doc. > > You can even use CROP target instead of SCANOUT. > You will responsible for fighting with nightmare of > backward compatibility and messy business logic hidden > in driver code. > > I am strongly against passing all rectangles at the same time. > The simultaneous configuration should be a separate part of > V4L2 API. What I want to explain is that we are not setting rectangle0, rectangle1, rectangle2... we are setting the readout configuration which is atomic. It would be the same as separate the gain in gain_bit0, gain_bit1, gain_bit2... > >> >> >>> It obfuscates API. It only pretends to fix a problem with applying >>> a part of configuration atomically. >>> >>>> >>>> BTW, what probably wasn't clear from Ricardo's explanation is that for every >>>> crop rectangle you must have a corresponding compose rectangle so that you >>>> know where to DMA it to. >>>> >>>> Your next question will be that it is a real problem that you can't set >>>> crop and compose simultaneously, and you are right about that. Read on for >>>> that... :-) >>>> >>>>> Hmm. I think that the real problem is much different. >>>>> Not how to set up rectangles atomically but rather >>>>> how do anything non-trivial atomically in V4L2. >>>>> >>>>> It would be very nice to have crop/compose and format configured at the same time. >>>>> However, current version of V4L2 API does not support that. >>>>> >>>>> Setting multiple crop rectangles may help a bit for only this little case. >>>>> But how would like to set multicrop rectangles and multicompose rectangle atomically. >>>> >>>> Why can't we extend the selection API a bit? How about this: >>>> >>>> #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 >>>> >>>> struct v4l2_selection { >>>> __u32 type; >>>> __u32 target; >>>> __u32 flags; >>>> union { >>>> struct v4l2_rect r; >>>> struct v4l2_ext_rect *pr; >>>> }; >>>> __u32 flags2; >>>> union { >>>> struct v4l2_rect r2; >>>> struct v4l2_ext_rect *pr2; >>>> }; >>>> __u32 rectangles; >>>> __u32 reserved[3]; >>>> }; >>>> >>>> If the target is CROP_COMPOSE then flags & r define the crop rectangle, and >>>> flags2 & r2 define the compose rectangle. That way you can set them both >>>> atomically. >>> >>> I do not like this idea because: >>> - mix crop and composing generating semi-related cropcompose >> Now looking with some perspective the selection API, dont you think >> that the crop and compose are related enough to be set at the same >> time? >> > > Technically everything in V4L2 is more or less related. > > I expect that in future we may see Frankenstein's monster that > sets up multirectangle crops, compose, format and some important controls > using single ioctl with a huge struct. > Probably, this would sufficient ... for some time. > > I am simply tired of all those "almost solutions". > The API should be modular. It should be made of simple > parts that do one thing well. Agree, but every ioctl must do something complete. > >>> - obfuscates the API even more >> Agreed, but we can find better naming. >> >>> - wastes a lot of reserved fields >> Only 2 > > There are new fields named flags2 (1 word), second union (4 words), > rectangles (1 word). So 6 extra words are wasted. > >> >>> - what if someone would like to use separate 'flags' for each rectangle >>> ... this could be a nice feature anyway :) >> Then he uses the ext_rect :P >> > > Or use only single rectangle selections. > >>> >>> This remains me the proposition from early days of selection API. >>> >>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/28945 >>> >>> Refer to point 4 where v4l2_crop2 was mentioned. >>> >>>> >>>> I would propose that the interaction with S_FMT is as follows: if S_FMT >>>> defines a rectangle < then the current compose rectangle, then the >>>> composition (and optionally crop) rectangle is reset. If it is >=, then >>>> all is fine. >>> >>> There is a issue here. Long time ago it was stated that S_FMT >>> should result in configuration where data are processed (scaled) into >>> the 'whole picture'. >>> >>> It means that a COMPOSE rectangle should be reset after very S_FMT. >>> >>>> >>>> If a new compose rectangle doesn't fit in the S_FMT rectangle then it should >>>> adapt the S_FMT rectangle to make it fit (as long as HW constraints are >>>> obeyed of course). >>> >>> What happens if REQBUFS was called between S_FMT and S_SELECTION? >>> >>>> >>>> This sequence will obey the rules of V4L2 as well: the last operation takes >>>> precedence over older operations. So setting S_FMT allow the driver to >>>> change cropping/composing to get as close to the desired format as possible. >>>> >>>>> How to define which crop rectangle refers to which to which compose rectangle. >>>> >>>> By setting the crop and compose selections at the same time and of the same >>>> size you can map each crop selection to a compose selection. It's all atomic >>>> as well. >>>> >>> >>> I think that adding an id field to v4l2_selection might be a good alternative >>> to introduction of numbered targets like V4L2_SEL_TGT_COMPOSE0, 1, ... >>> >>> It will add support for all multirectangle features as cost of >>> sacrifice of single reserved field. Moreover id field might be >>> very useful for other selection targets like FOCUS or FACE. >>> >>> Other idea might to using some bits of 'flags' field to carry rectangle id. >>> >>>>> >>>>> What to do if one would like to change only 3rd crop rectangle? >>>>> >>>>> Introduce rectangle id into v4l2_ext_rect? >>>>> Call VIDIOC_G_SELECTION to get all rectangles, change one and apply VIDIOC_S_SELECTION? >>>>> Is it really scalable? >>>> >>>> Why not? >>>> >>> >>> Because it is not scalable. >>> An application has to use 2 syscall instead of 1. >>> Unnecessary copying has to be performed. >> >> And setting 8 rectangles will mean 8 iocts instead of 2. > > The very similar amount of data is copied in both cases. > > sizeof(v4l2_selection) + N * sizeof (v4l2_ext_rect) > OR > N * sizeof(v4l2_selection) But 8 times more iocts, and ioctl N can change a rectangle set in ioct N-1. So in order to set up my readout I will probably have to 1) set rectangle 0 2) set retangle 1 3) get rectangle 0, to make sure it is still ok 4) set rectangle 2 5) get rectangle 1 6) get rectangle 0 .... > >> >> The user can also keep a copy of what has been set and there will only >> be 1 ioctl. >> > > Notice that user must update this copy every time S_SELECTION is called. > Moreover it must always use this array whenever calling S_SELECTION. > > If one use single-rectangle selections there is no need to keep all > those bookkeeping data. Nor assure its consistency. It is in your example where the user needs to keep a whole copy to avoid N ioctsl. In the propossed api if the user doesnt want to keep a copy he only needs 1 ioclt. > >>> >>>>> >>>>> Multirectangle targets may seam to be a solution but I think it is not. >>>>> >>>>> I think that atomic transactions are what is really needed in V4L2. >>>>> Other ideas like try-context from subdev API may also help. >>>>> >>>>> It will be nice to have something like >>>>> >>>>> VIDIOC_BEGIN >>>>> VIDIOC_S_SELECTION (target = CROP) >>>>> VIDIOC_S_SELECTION (target = COMPOSE) >>>>> VIDIOC_S_FMT >>>>> VIDIOC_S_CTRL >>>>> VIDIOC_COMMIT >>>> >>>> I don't think S_FMT is needed here: it's something you set up at the >>>> beginning and don't touch afterwards. >>> >>> One could say the same about compose, crop, ctrls. >>> The problem are the interactions between those objects. >>> What is dependent on what. What can change what. >>> How to reach a valid state in all cases not preventing >>> some valid configurations from being unreachable. >>> >>>> >>>> Wouldn't VIDIOC_S_SELECTION (target = CROP_COMPOSE) go a long way to >>>> solving these atomicity problems? >>>> >>>> Another nice feature that can be added to the selection API is to >>>> add a field to refer to a frame sequence number or a v4l2_buffer >>>> index (the latter is probably better): this would make it easy to >>>> apply an atomic crop/compose change to easily implement things like >>>> digital zoom or moving windows around. We could do something similar >>>> for controls. >>>> >>>> This would also solve the problem of assigning a per-buffer (or per-frame) >>>> configuration, something that libcamera2/3 needs. >>>> >>> >>> Introduce transactions with frame sequence numbers. >>> I proposed this in Cambridge in 2012. >>> >>>>> >>>>> and call a bunch of VIDIOC_G_* to see what really was applied. >>>>> >>>>>>> I have an auxiliary question. Do you have to set all rectangles >>>>>>> at once? can you set up them one by one? >>>>>> >>>>>> Also if you tell the driver what exact configuration you will need, it >>>>>> will provide you with the closest possible confuration, that cannot be >>>>> >>>>> s/cannot be done/may not be 'doable' >>>>> >>>>>> done if you provide rectangle by rectangle. >>>>>> >>>>>>> But how to deal with multiple rectangles? >>>>>> >>>>>> Multiple rectangles is a desired feature, please take a look to the >>>>>> presentation on the workshop. >>>>>> >>>>> >>>>> I agree that it may be useful. I just think that multirectangle selections >>>>> are needed to add support for such a feature. >>>> >>>> I don't follow, isn't that what this proposal adds? >>>> >>> >>> s/are needed/are not needed/ >>> >>> Sorry for confusion. >>> >>>>> >>>>>>> >>>>>>> Anyway, first try to define something like this: >>>>>>> >>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 V4L2_SEL_TGT_PRIVATE >>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_DEFAULT (V4L2_SEL_TGT_XXX_SCANOUT0 + 1) >>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0_BOUNDS (V4L2_SEL_TGT_XXX_SCANOUT0 + 2) >>>>>>> >>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT0 (V4L2_SEL_TGT_PRIVATE + 16) >>>>>>> ... >>>>>>> >>>>>>> -- OR-- parametrized macros similar to one below: >>>>>>> >>>>>>> #define V4L2_SEL_TGT_XXX_SCANOUT(n) (V4L2_SEL_TGT_PRIVATE + 16 * (n)) >>>>>>> >>>>>>> The application could setup all scanout areas one-by-one. >>>>>>> By default V4L2_SEL_TGT_XXX_SCANOUT0 would be equal to the whole array. >>>>>>> The height of all consecutive area would be 0. This means disabling >>>>>>> them effectively. >>>>>> >>>>>> Lets say rectangle A + B + C +D is legal, and A +B is also legal. You >>>>>> are in ABCD and you want to go to AB. How can you do it? >>>>> >>>>> Just set C to have height 0. It will disable D automatically. >>>> >>>> It's the other direction that's the problem: how to go from A + B to >>>> A + B + C + D if ABC is illegal. >>>> >>>>> BTW. How are you going to emulate S_CROP by selection API >>>>> if you must use at least two rectangles (A + B) ? >>>> >>>> S_CROP will always just set one rectangle A. So if you had multiple >>>> rectangles in your selection then after S_CROP you'll have only one. >>> >>> So S_CROP should change only A. >>> What happens if someone already set device to work with 4 rectangles. >>> Then calls S_CROP that modifies A in such a way that ABCD no longer forms >>> a proper rectangle. >>> >>> What should happen? >>> - ignore S_CROP, return previous settings, or >>> - modify B,C,D to form a proper rectangle, or >>> - disable B,C,D, or ? >> >> It will be the same as setting a multicrop with 1 rectangle. >> > > It not the answer to my question. > I expect that your answer refers to "disable B,C,D" variant, doesn't it? s_crop should be the equivalent of s_selection with n_rect=0 > >>> >>>> >>>> I'm assuming that the hw will always support a single crop rectangle. >>>> If not (extraordinarily unlikely), then S_CROP should just return an >>>> error. >>> >>> I agree that it is highly unlikely. I am just trying to figure out >>> what should happen when calling S_CROP or single-rectangle S_SELECTION >>> after calling multirect S_SELECTION. >> >> The sensor is configured with 1 rectangle. >> >>> >>>> >>>>> I suggest to use SCANOUT target in your first implementation. >>>>> Notice that as long you use something different from CROP and COMPOSE, >>>>> you may define interactions and dependencies any way you want, like >>>>> - if change of scanout area can affect composing area >>>>> - interaction with format and frame size >>>>> - check if scanout area can overlap, multi crops should be allowed to overlap >>>> >>>> I see no reason for this. A CROP_COMPOSE target does all you need. >>> >>> I think that CROP and COMPOSE is all you need. >>> >>>> >>>> Note that to set up N rectangles where N differs from the current >>>> rectangle count you do need to use CROP_COMPOSE. You can't setup CROP >>>> and COMPOSE rectangles independently since having N crop rectangles >>>> and M compose rectangles makes no sense. If you don't change the >>>> number of rectangles, then you can still use CROP and COMPOSE as is. >>>> >>> >>> I am afraid that you are trying to kill too many birds with one stone. >>> I think it is a very good moment to use bazooka. >>> >>> Transactions enhanced with frame numbers will handle a lot of issues with V4L2. >>> >>> Moreover it will allow to support some nice effects like: >>> - multiple exposures during HDR photography >>> - continuous online changes preview windows >>> - flash at a specific frame >>> - change format during video streaming to grab a high-resolution picture >>> - support multi rectangle cropping and composing :) >> >> Even with transactions I humble believe that we should have a >> multirectangle call. :) >> >> > > I humbly believe that single rectangle selections are sufficient. > Please, try to convince me that I am wrong. > If I was sure that this multirect feature cannot be implemented > using single-rect selections I will support this extension. > Even if this feature is not future-proof as mentioned earlier. -Configurations where the number of lines depends on the number of rectangles. -Configurations where the valid start of rectangles depends on the number of rectangles > > We may let transactions to be a part of V4L3 :). > > By now, I am still not convinced. > >>> >>>>> >>> >>> [snip] >>> >>> >>>>>> I have no need for it, but now that we are extending the selection API >>>>>> it would be the perfect moment to add them. >>>>> >>>>> The perfect moment for adding something is when it is needed. >>>> >>>> We need to add support for multiple rectangles, so we added a pointer. >>>> >>>>> The bad idea is preventing something from being added too early. >>>> >>>> But if we make it a pointer to v4l2_rect, then we can *never* add >>>> additional information to each rectangle. Whenever we add new APIs >>>> or features we try to allow for future extensions (with mixed >>>> success...), so now is the time to do so for v4l2_rect. >>>> >>>> Something like this: >>>> >>>> struct v4l2_sel_rect { >>>> struct v4l2_rect r; >>>> __u32 reserved[4]; >>>> }; >>>> >>>> would be fine by me as well. It's an additional indirection, but it >>>> also makes it easier to go from a v4l2_rect to a v4l2_sel_rect. >>> >>> We can always migrate to extended rectangle by modifying the structure to >>> >>> struct v4l2_selection { >>> __u32 type; >>> __u32 target; >>> __u32 flags; >>> union { >>> struct v4l2_rect r; >>> struct v4l2_ext_rect rext; >>> }; >>> __u32 reserved[X]; >>> }; >>> >>> and adding V4L2_SEL_FLAG_EXT_RECT flag. >> >> Dont you think this is a bit confusing? >> >> >> > > Why? It is much less confusing then union type > dependent on value of v4l2_selection::rectangles. > > You can use pointer to struct v4l2_ext_rect > instead of composition. Then only 1 bit could > be used to support extended rectangles. > > Regards, > Tomasz Stanislawski Best Regards
Hi Hans,
We've misunderstood. When I was saying 'overengineered'
I did not mean your RFC.
I was taking about this:
#define V4L2_SEL_TGT_CROP_COMPOSE 0x0200
struct v4l2_selection {
__u32 type;
__u32 target;
__u32 flags;
union {
struct v4l2_rect r;
struct v4l2_ext_rect *pr;
};
__u32 flags2;
union {
struct v4l2_rect r2;
struct v4l2_ext_rect *pr2;
};
__u32 rectangles;
__u32 reserved[3];
};
This structure looks scary to me :).
>
> I disagree. I implemented it in vivi and it turned out to be quite easy.
>
> For the record: I'm talking about this RFC:
>
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822
>
>> I still does not solve problems with flipping and rotations, which may
>> have a huge impact on mulitrect cropping/composing limitations.
>
> My proposal will make that much easier as well since flipping, rotating,
> cropping and composing are all controls/properties that can be set
> atomically (a control cluster). So drivers can create a single function
> that can handle all the relationships in one place, and applications can
> set all of these with one VIDIOC_S_EXT_CTRLS call.
>
I think that your idea is quite good. Solve atomic configuration
in a different part of API (control cluster), not by making
properties larger.
As I said, there are multiple way to handle atomic configuration.
Using control API is one of them. Quite nice BTW :)
Regards,
Tomasz Stanislawski
--
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
On 12/19/2013 02:30 PM, Tomasz Stanislawski wrote: > Hi Hans, > We've misunderstood. When I was saying 'overengineered' > I did not mean your RFC. > I was taking about this: > > #define V4L2_SEL_TGT_CROP_COMPOSE 0x0200 > > struct v4l2_selection { > __u32 type; > __u32 target; > __u32 flags; > union { > struct v4l2_rect r; > struct v4l2_ext_rect *pr; > }; > __u32 flags2; > union { > struct v4l2_rect r2; > struct v4l2_ext_rect *pr2; > }; > __u32 rectangles; > __u32 reserved[3]; > }; > > This structure looks scary to me :). Ah, yes. Implementing this as properties works much better. And multi-selection can be done simply by making an array of crop and compose rectangles. Regards, Hans > >> >> I disagree. I implemented it in vivi and it turned out to be quite easy. >> >> For the record: I'm talking about this RFC: >> >> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/71822 >> >>> I still does not solve problems with flipping and rotations, which may >>> have a huge impact on mulitrect cropping/composing limitations. >> >> My proposal will make that much easier as well since flipping, rotating, >> cropping and composing are all controls/properties that can be set >> atomically (a control cluster). So drivers can create a single function >> that can handle all the relationships in one place, and applications can >> set all of these with one VIDIOC_S_EXT_CTRLS call. >> > > I think that your idea is quite good. Solve atomic configuration > in a different part of API (control cluster), not by making > properties larger. > > As I said, there are multiple way to handle atomic configuration. > Using control API is one of them. Quite nice BTW :) > > Regards, > Tomasz Stanislawski > > -- 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
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index a95e5e2..f60a2ce 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -886,3 +886,39 @@ 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_get_rect(const 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; +} +EXPORT_SYMBOL_GPL(v4l2_selection_get_rect); + +void v4l2_selection_set_rect(struct v4l2_selection *s, + const struct v4l2_ext_rect *r) +{ + if (s->rectangles == 0) { + s->r.left = r->left; + s->r.top = r->top; + s->r.width = r->width; + s->r.height = r->height; + return; + } + s->pr[0] = *r; + s->rectangles = 1; + return; +} +EXPORT_SYMBOL_GPL(v4l2_selection_set_rect); diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..36ed3c3 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -777,6 +777,54 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde return 0; } +struct v4l2_selection32 { + __u32 type; + __u32 target; + __u32 flags; + union { + struct v4l2_rect r; + compat_uptr_t *pr; + }; + __u32 rectangles; + __u32 reserved[8]; +} __packed; + +static int get_v4l2_selection32(struct v4l2_selection *kp, + struct v4l2_selection32 __user *up) +{ + if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_selection32)) + || (copy_from_user(kp, up, sizeof(*kp)))) + return -EFAULT; + + if (kp->rectangles) { + compat_uptr_t usr_ptr; + if (get_user(usr_ptr, &up->pr)) + return -EFAULT; + kp->pr = compat_ptr(usr_ptr); + } + + return 0; + +} + +static int put_v4l2_selection32(struct v4l2_selection *kp, + struct v4l2_selection32 __user *up) +{ + compat_uptr_t usr_ptr = 0; + + if ((kp->rectangles) && get_user(usr_ptr, &up->pr)) + return -EFAULT; + + if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_selection32)) + || (copy_to_user(kp, up, sizeof(*kp)))) + return -EFAULT; + + if (kp->rectangles) + put_user(usr_ptr, &up->pr); + + return 0; + +} #define VIDIOC_G_FMT32 _IOWR('V', 4, struct v4l2_format32) #define VIDIOC_S_FMT32 _IOWR('V', 5, struct v4l2_format32) @@ -796,6 +844,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde #define VIDIOC_DQEVENT32 _IOR ('V', 89, struct v4l2_event32) #define VIDIOC_CREATE_BUFS32 _IOWR('V', 92, struct v4l2_create_buffers32) #define VIDIOC_PREPARE_BUF32 _IOWR('V', 93, struct v4l2_buffer32) +#define VIDIOC_G_SELECTION32 _IOWR('V', 94, struct v4l2_selection32) +#define VIDIOC_S_SELECTION32 _IOWR('V', 95, struct v4l2_selection32) #define VIDIOC_OVERLAY32 _IOW ('V', 14, s32) #define VIDIOC_STREAMON32 _IOW ('V', 18, s32) @@ -817,6 +867,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar struct v4l2_event v2ev; struct v4l2_create_buffers v2crt; struct v4l2_subdev_edid v2edid; + struct v4l2_selection v2sel; unsigned long vx; int vi; } karg; @@ -851,6 +902,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break; case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break; case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break; + case VIDIOC_G_SELECTION32: cmd = VIDIOC_G_SELECTION; break; + case VIDIOC_S_SELECTION32: cmd = VIDIOC_S_SELECTION; break; } switch (cmd) { @@ -922,6 +975,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar case VIDIOC_DQEVENT: compatible_arg = 0; break; + case VIDIOC_G_SELECTION: + case VIDIOC_S_SELECTION: + err = get_v4l2_selection32(&karg.v2sel, up); + compatible_arg = 0; + break; } if (err) return err; @@ -994,6 +1052,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar case VIDIOC_ENUMINPUT: err = put_v4l2_input32(&karg.v2i, up); break; + + case VIDIOC_G_SELECTION: + case VIDIOC_S_SELECTION: + err = put_v4l2_selection32(&karg.v2sel, up); + break; } return err; } diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 68e6b5e..aa1c2a4 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) @@ -1692,7 +1703,9 @@ 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, + }; int ret; if (ops->vidioc_cropcap) @@ -2253,6 +2266,20 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, } break; } + + case VIDIOC_G_SELECTION: + case VIDIOC_S_SELECTION: { + struct v4l2_selection *s = parg; + + if (s->rectangles) { + *user_ptr = (void __user *)s->pr; + *kernel_ptr = (void *)&s->pr; + *array_size = sizeof(struct v4l2_ext_rect) + * s->rectangles; + ret = 1; + } + break; + } } return ret; diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 015ff82..417ab82 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -216,4 +216,10 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait); void v4l2_get_timestamp(struct timeval *tv); +int v4l2_selection_get_rect(const struct v4l2_selection *s, + struct v4l2_ext_rect *r); + +void v4l2_selection_set_rect(struct v4l2_selection *s, + const struct v4l2_ext_rect *r); + #endif /* V4L2_COMMON_H_ */ diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index a33c4da..c02a886 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..a4a7902 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -211,6 +211,14 @@ struct v4l2_rect { __s32 height; }; +struct v4l2_ext_rect { + __s32 left; + __s32 top; + __u32 width; + __u32 height; + __u32 reserved[4]; +}; + struct v4l2_fract { __u32 numerator; __u32 denominator; @@ -804,6 +812,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,10 +824,13 @@ 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]; +} __packed; /* * A N A L O G V I D E O S T A N D A R D
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. Two helper functiona are also added, to help the drivers that support a single selection. This Patch ONLY adds the modification to the core. Once it is agreed, a new version including changes on the drivers that handle the selection api will come. This patch includes all the comments by Hans Verkuil. Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- v3: -Changes on compat-ioctl32 -Remove checks on v4l2_selection_set_rect drivers/media/v4l2-core/v4l2-common.c | 36 +++++++++++++++ drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 63 +++++++++++++++++++++++++++ drivers/media/v4l2-core/v4l2-ioctl.c | 37 +++++++++++++--- include/media/v4l2-common.h | 6 +++ include/uapi/linux/v4l2-subdev.h | 10 ++++- include/uapi/linux/videodev2.h | 21 +++++++-- 6 files changed, 162 insertions(+), 11 deletions(-)