Re: [PATCH] v4l2: Support for multiple selections

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ricardo,

Sorry for the delayed review, but I'm finally catching up with my backlog.

I've got a number of comments regarding this patch. I'm ignoring the platform
driver patches for now until the core support is correct.

On 09/16/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda <ricardo.ribalda@xxxxxxxxx>
> 
> Extend v4l2 selection API to support multiple selection areas, this way
> sensors that support multiple readout areas can work with multiple areas
> of insterest.
> 
> The struct v4l2_selection and v4l2_subdev_selection has been extented
> with a new field rectangles. If it is value is different than zero the
> pr array is used instead of the r field.
> 
> A new structure v4l2_ext_rect has been defined, containing 4 reserved
> fields for future improvements, as suggested by Hans.
> 
> A new function in v4l2-comon (v4l2_selection_flat_struct) is in charge
> of converting a pr pointer with one item to a flatten struct. This
> function is used in all the old drivers that dont support multiple
> selections.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> ---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c      |  6 +++
>  drivers/media/platform/exynos4-is/fimc-capture.c |  6 +++
>  drivers/media/platform/exynos4-is/fimc-lite.c    |  6 +++
>  drivers/media/platform/s3c-camif/camif-capture.c |  6 +++
>  drivers/media/platform/s5p-jpeg/jpeg-core.c      |  3 ++
>  drivers/media/platform/s5p-tv/mixer_video.c      |  6 +++
>  drivers/media/platform/soc_camera/soc_camera.c   |  6 +++
>  drivers/media/v4l2-core/v4l2-common.c            | 13 ++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c             | 54 +++++++++++++++++++++---
>  include/media/v4l2-common.h                      |  2 +
>  include/uapi/linux/v4l2-subdev.h                 | 10 ++++-
>  include/uapi/linux/videodev2.h                   | 15 ++++++-
>  12 files changed, 122 insertions(+), 11 deletions(-)
> 

<snip>

> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index a95e5e2..cd20567 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -886,3 +886,16 @@ void v4l2_get_timestamp(struct timeval *tv)
>  	tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
> +
> +int v4l2_selection_flat_struct(struct v4l2_selection *s)
> +{
> +	if (s->rectangles == 0)
> +		return 0;
> +
> +	if (s->rectangles != 1)
> +		return -EINVAL;
> +
> +	s->r = s->pr[0].r;

This would overwrite the pr pointer. Not a good idea.

I would make a helper function like this:

int v4l2_selection_get_rect(struct v4l2_selection *s, struct v4l2_ext_rect *r)
{
	if (s->rectangles > 1)
		return -EINVAL;
	if (s->rectangles == 1) {
		*r = s->pr[0];
		return 0;
	}
	if (s->r.width < 0 || s->r.height < 0)
		return -EINVAL;
	r->left = s->r.left;
	r->top = s->r.top;
	r->width = s->r.width;
	r->height = s->r.height;
	memset(r->reserved, 0, sizeof(r->reserved));
	return 0;
}

See also my proposed v4l2_ext_rect definition below.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_selection_flat_struct);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..fe92f6b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -572,11 +572,22 @@ static void v4l_print_crop(const void *arg, bool write_only)
>  static void v4l_print_selection(const void *arg, bool write_only)
>  {
>  	const struct v4l2_selection *p = arg;
> +	int i;
>  
> -	pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> -		prt_names(p->type, v4l2_type_names),
> -		p->target, p->flags,
> -		p->r.width, p->r.height, p->r.left, p->r.top);
> +	if (p->rectangles == 0)
> +		pr_cont("type=%s, target=%d, flags=0x%x, wxh=%dx%d, x,y=%d,%d\n",
> +			prt_names(p->type, v4l2_type_names),
> +			p->target, p->flags,
> +			p->r.width, p->r.height, p->r.left, p->r.top);
> +	else{
> +		pr_cont("type=%s, target=%d, flags=0x%x rectangles=%d\n",
> +			prt_names(p->type, v4l2_type_names),
> +			p->target, p->flags, p->rectangles);
> +		for (i = 0; i < p->rectangles; i++)
> +			pr_cont("rectangle %d: wxh=%dx%d, x,y=%d,%d\n",
> +				i, p->r.width, p->r.height,
> +				p->r.left, p->r.top);
> +	}
>  }
>  
>  static void v4l_print_jpegcompression(const void *arg, bool write_only)
> @@ -1645,6 +1656,7 @@ static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
>  	struct v4l2_crop *p = arg;
>  	struct v4l2_selection s = {
>  		.type = p->type,
> +		.rectangles = 0,

No need for this. In initializers like this fields that aren't initialized
explicitly will be zeroed by the compiler.

>  	};
>  	int ret;
>  
> @@ -1673,6 +1685,7 @@ static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
>  	struct v4l2_selection s = {
>  		.type = p->type,
>  		.r = p->c,
> +		.rectangles = 0,
>  	};
>  
>  	if (ops->vidioc_s_crop)
> @@ -1692,7 +1705,10 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
>  	struct v4l2_cropcap *p = arg;
> -	struct v4l2_selection s = { .type = p->type };
> +	struct v4l2_selection s = {
> +		.type = p->type,
> +		.rectangles = 0,
> +	};
>  	int ret;
>  
>  	if (ops->vidioc_cropcap)
> @@ -1726,6 +1742,30 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops,
>  	return 0;
>  }
>  
> +static int v4l_s_selection(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_selection *s = arg;
> +
> +	if (s->rectangles &&
> +		!access_ok(VERIFY_READ, s->pr, s->rectangles * sizeof(*s->pr)))
> +		return -EFAULT;

No, this isn't necessary. Instead add support for the selection array to
check_array_args() in v4l2-ioctl.c. That's where this should be handled.
video_usercopy() will then do the copy_from/to_user for you and drivers don't
need to care about it.

Note that you also need to update v4l2-compat-ioctl32.c!

> +
> +	return ops->vidioc_s_selection(file, fh, s);
> +}
> +
> +static int v4l_g_selection(const struct v4l2_ioctl_ops *ops,
> +				struct file *file, void *fh, void *arg)
> +{
> +	struct v4l2_selection *s = arg;
> +
> +	if (s->rectangles &&
> +		!access_ok(VERIFY_WRITE, s->pr, s->rectangles * sizeof(*s->pr)))
> +		return -EFAULT;
> +
> +	return ops->vidioc_g_selection(file, fh, s);
> +}
> +
>  static int v4l_log_status(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -2018,8 +2058,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_FNC(VIDIOC_CROPCAP, v4l_cropcap, v4l_print_cropcap, INFO_FL_CLEAR(v4l2_cropcap, type)),
>  	IOCTL_INFO_FNC(VIDIOC_G_CROP, v4l_g_crop, v4l_print_crop, INFO_FL_CLEAR(v4l2_crop, type)),
>  	IOCTL_INFO_FNC(VIDIOC_S_CROP, v4l_s_crop, v4l_print_crop, INFO_FL_PRIO),
> -	IOCTL_INFO_STD(VIDIOC_G_SELECTION, vidioc_g_selection, v4l_print_selection, 0),
> -	IOCTL_INFO_STD(VIDIOC_S_SELECTION, vidioc_s_selection, v4l_print_selection, INFO_FL_PRIO),
> +	IOCTL_INFO_FNC(VIDIOC_G_SELECTION, v4l_g_selection, v4l_print_selection, 0),
> +	IOCTL_INFO_FNC(VIDIOC_S_SELECTION, v4l_s_selection, v4l_print_selection, INFO_FL_PRIO),
>  	IOCTL_INFO_STD(VIDIOC_G_JPEGCOMP, vidioc_g_jpegcomp, v4l_print_jpegcompression, 0),
>  	IOCTL_INFO_STD(VIDIOC_S_JPEGCOMP, vidioc_s_jpegcomp, v4l_print_jpegcompression, INFO_FL_PRIO),
>  	IOCTL_INFO_FNC(VIDIOC_QUERYSTD, v4l_querystd, v4l_print_std, 0),
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 015ff82..b0a3d2b 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -216,4 +216,6 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait);
>  
>  void v4l2_get_timestamp(struct timeval *tv);
>  
> +int v4l2_selection_flat_struct(struct v4l2_selection *s);
> +
>  #endif /* V4L2_COMMON_H_ */
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index a33c4da..b5ee08b 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -133,6 +133,8 @@ struct v4l2_subdev_frame_interval_enum {
>   *	    defined in v4l2-common.h; V4L2_SEL_TGT_* .
>   * @flags: constraint flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
>   * @r: coordinates of the selection window
> + * @pr:		array of rectangles containing the selection windows
> + * @rectangles:	Number of rectangles in pr structure. If zero, r is used instead
>   * @reserved: for future use, set to zero for now
>   *
>   * Hardware may use multiple helper windows to process a video stream.
> @@ -144,8 +146,12 @@ struct v4l2_subdev_selection {
>  	__u32 pad;
>  	__u32 target;
>  	__u32 flags;
> -	struct v4l2_rect r;
> -	__u32 reserved[8];
> +	union{

Add space after 'union'.

> +		struct v4l2_rect r;
> +		struct v4l2_ext_rect        *pr;
> +	};
> +	__u32 rectangles;
> +	__u32 reserved[7];
>  };

I suspect this should get the packed attribute. It's a good idea anyone to add that,
but we have to check that the sizeof(struct v4l2_subdev_selection) doesn't change
for both 32 and 64 bit compilations.

>  
>  struct v4l2_subdev_edid {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 95ef455..db8b1a5 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -211,6 +211,11 @@ struct v4l2_rect {
>  	__s32   height;
>  };
>  
> +struct v4l2_ext_rect {
> +	struct v4l2_rect r;
> +	__u32   reserved[4];
> +};

I actually would prefer this:

struct v4l2_ext_rect {
	__s32 left;
	__s32 top;
	__u32 width;
	__u32 height;
	__u32 reserved[4];
};

It has always annoyed me that width and height were signed, and this fixes that problem.

This is also why I was using v4l2_ext_rect in the selection helper function: that way
drivers do not need to check for negative width/height values.

> +
>  struct v4l2_fract {
>  	__u32   numerator;
>  	__u32   denominator;
> @@ -804,6 +809,8 @@ struct v4l2_crop {
>   *		defined in v4l2-common.h; V4L2_SEL_TGT_* .
>   * @flags:	constraints flags, defined in v4l2-common.h; V4L2_SEL_FLAG_*.
>   * @r:		coordinates of selection window
> + * @pr:		array of rectangles containing the selection windows
> + * @rectangles:	Number of rectangles in pr structure. If zero, r is used instead
>   * @reserved:	for future use, rounds structure size to 64 bytes, set to zero
>   *
>   * Hardware may use multiple helper windows to process a video stream.
> @@ -814,8 +821,12 @@ struct v4l2_selection {
>  	__u32			type;
>  	__u32			target;
>  	__u32                   flags;
> -	struct v4l2_rect        r;
> -	__u32                   reserved[9];
> +	union{

Add space after 'union'.

> +		struct v4l2_rect        r;
> +		struct v4l2_ext_rect        *pr;
> +	};
> +	__u32                   rectangles;
> +	__u32                   reserved[8];
>  };
>  
>  
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux