Hello Hans
Thanks for your comments
I have just posted a new version.
Regards!
On Tue, Oct 1, 2013 at 11:13 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On Tue 1 October 2013 10:26:56 Ricardo Ribalda Delgado 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.
>>
>> struct v4l2_selection has the same size on 32 and 64 bits, therefore I
>> dont think that any change on v4l2-compat-ioctl32 is needed.
>
> Yes, you do. The pr pointer is 32-bit for a 32-bit application and the
> compat code is needed to convert it to a 64-bit pointer. The struct as a
> whole may have the same size, but the pointer in the union definitely has
> a different size.
>
>>
>> ricardo@neopili:/tmp$ gcc kk.c -m32
>> ricardo@neopili:/tmp$ ./a.out
>> Size of v4l2_selection=64
>> ricardo@neopili:/tmp$ gcc kk.c
>> ricardo@neopili:/tmp$ ./a.out
>> Size of v4l2_selection=64
>>
>> This patch includes all the comments by Hans Verkuil.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-common.c | 39 +++++++++++++++++++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-ioctl.c | 37 ++++++++++++++++++++++++++++-----
>> include/media/v4l2-common.h | 4 ++++
>> include/uapi/linux/v4l2-subdev.h | 10 +++++++--
>> include/uapi/linux/videodev2.h | 21 +++++++++++++++----
>> 5 files changed, 100 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index a95e5e2..007ab32 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -886,3 +886,42 @@ 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(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);
>> +
>> +int v4l2_selection_set_rect(struct v4l2_ext_rect *r, struct v4l2_selection *s)
>> +{
>> + if (s->rectangles == 0) {
>> + if (s->r.width > UINT_MAX || s->r.height > UINT_MAX)
>> + return -EINVAL;
>
> This makes no sense. You probably mean r->width > INT_MAX, but I would just
> skip this whole test: it is the driver that will set these values, and you
> can assume that it is correct. You can never rely on what you get from userspace,
> but I think this is reasonably in this case to assume that the rectangle is
> a reasonable size.
>
> That also means that this can be a void function.
>
>> + s->r.left = r->left;
>> + s->r.top = r->top;
>> + s->r.width = r->width;
>> + s->r.height = r->height;
>> + return 0;
>> + }
>> + if (s->rectangles > 1) {
>> + s->pr[0] = *r;
>> + s->rectangles = 1;
>> + return 0;
>> + }
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_selection_set_rect);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 68e6b5e..29f7cf1 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..dc96861 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -216,4 +216,8 @@ 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(struct v4l2_selection *s, struct v4l2_ext_rect *r);
>> +
>> +int v4l2_selection_set_rect(struct v4l2_ext_rect *r, struct v4l2_selection *s);
>
> Swap the arguments to be consistent with get_rect. The v4l2_ext_rect argument can
> be const as well since you don't want set_rect to modify *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
>>
>
> Regards,
>
> Hans
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
