[PATCH 1/2] [media] vivi: Kill BUFFER_TIMEOUT macro

[PATCH 1/2] [media] vivi: Kill BUFFER_TIMEOUT macro

[Kirill Smelkov]

Usage of BUFFER_TIMEOUT has gone in 2008 in 78718e5d (V4L/DVB (7492):
vivi: Simplify the vivi driver and avoid deadlocks), but the macro
remains. Say goodbye to it.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index b366b05..3e6902a 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -39,7 +39,6 @@
 /* Wake up at about 30 fps */
 #define WAKE_NUMERATOR 30
 #define WAKE_DENOMINATOR 1001
-#define BUFFER_TIMEOUT     msecs_to_jiffies(500)  /* 0.5 seconds */
 
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
-- 
1.8.0.rc3.331.g5b9a629

[PATCH 2/2] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

I was testing my video-over-ethernet subsystem today, and vivi seemed to
be perfect video source for testing when one don't have lots of capture
boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
while in my country we usually use PAL (25 fps). That's why the patch.
Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 3e6902a..48325f4 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,10 +36,6 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
-
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -58,6 +54,11 @@ static unsigned n_devs = 1;
 module_param(n_devs, uint, 0644);
 MODULE_PARM_DESC(n_devs, "number of video devices to create");
 
+static struct v4l2_fract fps = { 30000, 1001 }; /* ~ 30 fps by default */
+static unsigned __fps[2], __nfps;
+module_param_array_named(fps, __fps, uint, &__nfps, 0644);
+MODULE_PARM_DESC(fps, "frames per second as ratio (e.g. 30000,1001 or 25,1)");
+
 static unsigned debug;
 module_param(debug, uint, 0644);
 MODULE_PARM_DESC(debug, "activates debug info");
@@ -661,7 +662,7 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 }
 
 #define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+	((frames * fps.denominator * 1000) / fps.numerator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -1376,6 +1377,13 @@ static int __init vivi_init(void)
 	if (n_devs <= 0)
 		n_devs = 1;
 
+	if (__nfps > 0) {
+		fps.numerator   = __fps[0];
+		fps.denominator = (__nfps > 1) ? __fps[1] : 1;
+	}
+	if (fps.numerator <= 0)
+		fps.numerator = 1;
+
 	for (i = 0; i < n_devs; i++) {
 		ret = vivi_create_instance(i);
 		if (ret) {
-- 
1.8.0.rc3.331.g5b9a629

Re: [PATCH 2/2] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> I was testing my video-over-ethernet subsystem today, and vivi seemed to
> be perfect video source for testing when one don't have lots of capture
> boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> while in my country we usually use PAL (25 fps). That's why the patch.
> Thanks.

Rather than introducing a module option, it's much nicer if you can
implement enum_frameintervals and g/s_parm. This can be made quite flexible
allowing you to also support 50/59.94/60 fps.

Regards,

	Hans

> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 3e6902a..48325f4 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -58,6 +54,11 @@ static unsigned n_devs = 1;
>  module_param(n_devs, uint, 0644);
>  MODULE_PARM_DESC(n_devs, "number of video devices to create");
>  
> +static struct v4l2_fract fps = { 30000, 1001 }; /* ~ 30 fps by default */
> +static unsigned __fps[2], __nfps;
> +module_param_array_named(fps, __fps, uint, &__nfps, 0644);
> +MODULE_PARM_DESC(fps, "frames per second as ratio (e.g. 30000,1001 or 25,1)");
> +
>  static unsigned debug;
>  module_param(debug, uint, 0644);
>  MODULE_PARM_DESC(debug, "activates debug info");
> @@ -661,7 +662,7 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  }
>  
>  #define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +	((frames * fps.denominator * 1000) / fps.numerator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -1376,6 +1377,13 @@ static int __init vivi_init(void)
>  	if (n_devs <= 0)
>  		n_devs = 1;
>  
> +	if (__nfps > 0) {
> +		fps.numerator   = __fps[0];
> +		fps.denominator = (__nfps > 1) ? __fps[1] : 1;
> +	}
> +	if (fps.numerator <= 0)
> +		fps.numerator = 1;
> +
>  	for (i = 0; i < n_devs; i++) {
>  		ret = vivi_create_instance(i);
>  		if (ret) {
> 

Re: [PATCH 2/2] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > be perfect video source for testing when one don't have lots of capture
> > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > while in my country we usually use PAL (25 fps). That's why the patch.
> > Thanks.
> 
> Rather than introducing a module option, it's much nicer if you can
> implement enum_frameintervals and g/s_parm. This can be made quite flexible
> allowing you to also support 50/59.94/60 fps.

Thanks for feedback. I've reworked the patch for FPS to be set via
->{g,s}_parm(), and yes now it is more flexble, because one can set
different FPS on different vivi devices. Only I don't know V4L2 ioctls
details well enough and various drivers do things differently. The patch
is below. Is it ok?

Thanks,
Kirill


---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 22 Oct 2012 17:25:24 +0400
Subject: [PATCH v2] [media] vivi: Teach it to tune FPS

I was testing my video-over-ethernet subsystem today, and vivi seemed to
be perfect video source for testing when one don't have lots of capture
boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
while in my country we usually use PAL (25 fps).

That's why here is this patch with ->enum_frameintervals and
->{g,s}_parm implemented as suggested by Hans Verkuil. Not sure I've
done ->g_parm right -- some drivers clear parm memory before setting
fields, some don't. As at is at least it works for me (tested via
v4l2-ctl -P / -p <fps>).

Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 50 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

V2:

    - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
      by Hans Verkuil.

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 3e6902a..c0855a5 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,10 +36,6 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
-
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -232,6 +228,7 @@ struct vivi_dev {
 
 	/* video capture */
 	struct vivi_fmt            *fmt;
+	struct v4l2_fract          timeperframe;
 	unsigned int               width, height;
 	struct vb2_queue	   vb_vidq;
 	unsigned int		   field_count;
@@ -660,8 +657,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
 
-#define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+#define frames_to_ms(dev, frames)				\
+	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -677,7 +674,8 @@ static void vivi_sleep(struct vivi_dev *dev)
 		goto stop_task;
 
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(1));
+	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
+
 
 	vivi_thread_tick(dev);
 
@@ -1049,6 +1047,39 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	return 0;
 }
 
+/* timeperframe is arbitrary and continous */
+static int vidioc_enum_frameintervals(struct file *file, void *priv,
+					     struct v4l2_frmivalenum *fival)
+{
+	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+	return 0;
+}
+
+static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	return 0;
+}
+
+static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	dev->timeperframe.numerator	= parm->parm.capture.timeperframe.numerator;
+	dev->timeperframe.denominator	= parm->parm.capture.timeperframe.denominator ?: 1;
+
+	return 0;
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1207,6 +1238,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
 	.vidioc_s_input       = vidioc_s_input,
+	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+	.vidioc_g_parm        = vidioc_g_parm,
+	.vidioc_s_parm        = vidioc_s_parm,
 	.vidioc_streamon      = vb2_ioctl_streamon,
 	.vidioc_streamoff     = vb2_ioctl_streamoff,
 	.vidioc_log_status    = v4l2_ctrl_log_status,
@@ -1265,6 +1299,8 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
+	dev->timeperframe.numerator = 1001;
+	dev->timeperframe.denominator = 30000;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;
-- 
1.8.0.rc3.331.g5b9a629

Re: [PATCH 2/2] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Mon, Oct 22, 2012 at 09:01:39PM +0400, Kirill Smelkov wrote:
> On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > be perfect video source for testing when one don't have lots of capture
> > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > Thanks.
> > 
> > Rather than introducing a module option, it's much nicer if you can
> > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > allowing you to also support 50/59.94/60 fps.
> 
> Thanks for feedback. I've reworked the patch for FPS to be set via
> ->{g,s}_parm(), and yes now it is more flexble, because one can set

By the way, here is what I've found while working on the abovementioned
patch:

---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 22 Oct 2012 21:14:01 +0400
Subject: [PATCH] v4l2: Fix typo in struct v4l2_captureparm description

Judging from what drivers do and from my experience temeperframe
fraction is set in seconds - look e.g. here

    static int bttv_g_parm(struct file *file, void *f,
                                    struct v4l2_streamparm *parm)
    {
            struct bttv_fh *fh = f;
            struct bttv *btv = fh->btv;

            v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id,
                                        &parm->parm.capture.timeperframe);

    ...

    void v4l2_video_std_frame_period(int id, struct v4l2_fract *frameperiod)
    {
            if (id & V4L2_STD_525_60) {
                    frameperiod->numerator = 1001;
                    frameperiod->denominator = 30000;
            } else {
                    frameperiod->numerator = 1;
                    frameperiod->denominator = 25;
            }

and also v4l2-ctl in userspace decodes this as seconds:

    if (doioctl(fd, VIDIOC_G_PARM, &parm, "VIDIOC_G_PARM") == 0) {
            const struct v4l2_fract &tf = parm.parm.capture.timeperframe;

            ...

            printf("\tFrames per second: %.3f (%d/%d)\n",
                            (1.0 * tf.denominator) / tf.numerator,
                            tf.denominator, tf.numerator);

The typo was there from day 1 - added in 2002 in e028b61b ([PATCH]
add v4l2 api)(*)

(*) found in history tree
    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 include/uapi/linux/videodev2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 57bfa59..2fff7ff 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -726,29 +726,29 @@ struct v4l2_window {
 	__u32			field;	 /* enum v4l2_field */
 	__u32			chromakey;
 	struct v4l2_clip	__user *clips;
 	__u32			clipcount;
 	void			__user *bitmap;
 	__u8                    global_alpha;
 };
 
 /*
  *	C A P T U R E   P A R A M E T E R S
  */
 struct v4l2_captureparm {
 	__u32		   capability;	  /*  Supported modes */
 	__u32		   capturemode;	  /*  Current mode */
-	struct v4l2_fract  timeperframe;  /*  Time per frame in .1us units */
+	struct v4l2_fract  timeperframe;  /*  Time per frame in seconds */
 	__u32		   extendedmode;  /*  Driver-specific extensions */
 	__u32              readbuffers;   /*  # of buffers for read */
 	__u32		   reserved[4];
 };
 
 /*  Flags for 'capability' and 'capturemode' fields */
 #define V4L2_MODE_HIGHQUALITY	0x0001	/*  High quality imaging mode */
 #define V4L2_CAP_TIMEPERFRAME	0x1000	/*  timeperframe field is supported */
 
 struct v4l2_outputparm {
 	__u32		   capability;	 /*  Supported modes */
 	__u32		   outputmode;	 /*  Current mode */
 	struct v4l2_fract  timeperframe; /*  Time per frame in seconds */
 	__u32		   extendedmode; /*  Driver-specific extensions */
-- 
1.8.0.rc3.331.g5b9a629

Re: [PATCH 2/2] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Mon October 22 2012 19:29:01 Kirill Smelkov wrote:
> On Mon, Oct 22, 2012 at 09:01:39PM +0400, Kirill Smelkov wrote:
> > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > be perfect video source for testing when one don't have lots of capture
> > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > Thanks.
> > > 
> > > Rather than introducing a module option, it's much nicer if you can
> > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > allowing you to also support 50/59.94/60 fps.
> > 
> > Thanks for feedback. I've reworked the patch for FPS to be set via
> > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> 
> By the way, here is what I've found while working on the abovementioned
> patch:
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Mon, 22 Oct 2012 21:14:01 +0400
> Subject: [PATCH] v4l2: Fix typo in struct v4l2_captureparm description
> 
> Judging from what drivers do and from my experience temeperframe
> fraction is set in seconds - look e.g. here
> 
>     static int bttv_g_parm(struct file *file, void *f,
>                                     struct v4l2_streamparm *parm)
>     {
>             struct bttv_fh *fh = f;
>             struct bttv *btv = fh->btv;
> 
>             v4l2_video_std_frame_period(bttv_tvnorms[btv->tvnorm].v4l2_id,
>                                         &parm->parm.capture.timeperframe);
> 
>     ...
> 
>     void v4l2_video_std_frame_period(int id, struct v4l2_fract *frameperiod)
>     {
>             if (id & V4L2_STD_525_60) {
>                     frameperiod->numerator = 1001;
>                     frameperiod->denominator = 30000;
>             } else {
>                     frameperiod->numerator = 1;
>                     frameperiod->denominator = 25;
>             }
> 
> and also v4l2-ctl in userspace decodes this as seconds:
> 
>     if (doioctl(fd, VIDIOC_G_PARM, &parm, "VIDIOC_G_PARM") == 0) {
>             const struct v4l2_fract &tf = parm.parm.capture.timeperframe;
> 
>             ...
> 
>             printf("\tFrames per second: %.3f (%d/%d)\n",
>                             (1.0 * tf.denominator) / tf.numerator,
>                             tf.denominator, tf.numerator);
> 
> The typo was there from day 1 - added in 2002 in e028b61b ([PATCH]
> add v4l2 api)(*)
> 
> (*) found in history tree
>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>

Good catch. Luckily the V4L2 spec is correct, it is just that single comment
that's wrong.

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  include/uapi/linux/videodev2.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 57bfa59..2fff7ff 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -726,29 +726,29 @@ struct v4l2_window {
>  	__u32			field;	 /* enum v4l2_field */
>  	__u32			chromakey;
>  	struct v4l2_clip	__user *clips;
>  	__u32			clipcount;
>  	void			__user *bitmap;
>  	__u8                    global_alpha;
>  };
>  
>  /*
>   *	C A P T U R E   P A R A M E T E R S
>   */
>  struct v4l2_captureparm {
>  	__u32		   capability;	  /*  Supported modes */
>  	__u32		   capturemode;	  /*  Current mode */
> -	struct v4l2_fract  timeperframe;  /*  Time per frame in .1us units */
> +	struct v4l2_fract  timeperframe;  /*  Time per frame in seconds */
>  	__u32		   extendedmode;  /*  Driver-specific extensions */
>  	__u32              readbuffers;   /*  # of buffers for read */
>  	__u32		   reserved[4];
>  };
>  
>  /*  Flags for 'capability' and 'capturemode' fields */
>  #define V4L2_MODE_HIGHQUALITY	0x0001	/*  High quality imaging mode */
>  #define V4L2_CAP_TIMEPERFRAME	0x1000	/*  timeperframe field is supported */
>  
>  struct v4l2_outputparm {
>  	__u32		   capability;	 /*  Supported modes */
>  	__u32		   outputmode;	 /*  Current mode */
>  	struct v4l2_fract  timeperframe; /*  Time per frame in seconds */
>  	__u32		   extendedmode; /*  Driver-specific extensions */
> 

Re: [PATCH 2/2] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > be perfect video source for testing when one don't have lots of capture
> > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > Thanks.
> > 
> > Rather than introducing a module option, it's much nicer if you can
> > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > allowing you to also support 50/59.94/60 fps.
> 
> Thanks for feedback. I've reworked the patch for FPS to be set via
> ->{g,s}_parm(), and yes now it is more flexble, because one can set
> different FPS on different vivi devices. Only I don't know V4L2 ioctls
> details well enough and various drivers do things differently. The patch
> is below. Is it ok?

Close, but it's not quite there.

You should run the v4l2-compliance tool from the v4l-utils.git repository
(take the master branch). That will report any errors in your implementation.

In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
for that). I would set the nominal framerate whenever the denominator == 0.

For vidioc_enum_frameintervals you need to check the IN fields and fill in the
stepwise struct.

Regards,

	Hans

> 
> Thanks,
> Kirill
> 
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Mon, 22 Oct 2012 17:25:24 +0400
> Subject: [PATCH v2] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem today, and vivi seemed to
> be perfect video source for testing when one don't have lots of capture
> boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> while in my country we usually use PAL (25 fps).
> 
> That's why here is this patch with ->enum_frameintervals and
> ->{g,s}_parm implemented as suggested by Hans Verkuil. Not sure I've
> done ->g_parm right -- some drivers clear parm memory before setting
> fields, some don't. As at is at least it works for me (tested via
> v4l2-ctl -P / -p <fps>).
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 50 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 7 deletions(-)
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 3e6902a..c0855a5 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -232,6 +228,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -660,8 +657,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -677,7 +674,8 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
> +
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1049,6 +1047,39 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	return 0;
> +}
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	dev->timeperframe.numerator	= parm->parm.capture.timeperframe.numerator;
> +	dev->timeperframe.denominator	= parm->parm.capture.timeperframe.denominator ?: 1;
> +
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1207,6 +1238,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1265,6 +1299,8 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe.numerator = 1001;
> +	dev->timeperframe.denominator = 30000;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> 

[PATCH v3] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > be perfect video source for testing when one don't have lots of capture
> > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > Thanks.
> > > 
> > > Rather than introducing a module option, it's much nicer if you can
> > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > allowing you to also support 50/59.94/60 fps.
> > 
> > Thanks for feedback. I've reworked the patch for FPS to be set via
> > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > details well enough and various drivers do things differently. The patch
> > is below. Is it ok?
> 
> Close, but it's not quite there.
> 
> You should run the v4l2-compliance tool from the v4l-utils.git repository
> (take the master branch). That will report any errors in your implementation.
> 
> In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> for that). I would set the nominal framerate whenever the denominator == 0.
> 
> For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> stepwise struct.

Thanks for pointers and info about v4l2-compliance handy-tool. I've
tried to correct all the issues you mentioned above and here is the
patch.

(Only requirement to set stepwise.step to 1.0 for
 V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
 that's what the V4L2 spec requires...)

Thanks,
Kirill

---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Tue, 23 Oct 2012 16:56:59 +0400
Subject: [PATCH v3] [media] vivi: Teach it to tune FPS

I was testing my video-over-ethernet subsystem yesterday, and vivi
seemed to be perfect video source for testing when one don't have lots
of capture boards and cameras. Only its framerate was hardcoded to
NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
needed that to precisely simulate bandwidth.

That's why here is this patch with ->enum_frameintervals() and
->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.

Regarding newly introduced __get_format(u32 pixelformat) I decided not
to convert original get_format() to operate on fourcc codes, since >= 3
places in driver need to deal with v4l2_format and otherwise it won't be
handy.

Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 9 deletions(-)

V3:
    - corrected issues with V4L2 spec compliance as pointed by Hans
      Verkuil.

V2:

    - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
      by Hans Verkuil.


diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 3e6902a..3adea58 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,10 +36,6 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
-
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
 /* Global font descriptor */
 static const u8 *font8x16;
 
+/* default to NTSC timeperframe */
+static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
+
 #define dprintk(dev, level, fmt, arg...) \
 	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
 
@@ -150,14 +149,14 @@ static struct vivi_fmt formats[] = {
 	},
 };
 
-static struct vivi_fmt *get_format(struct v4l2_format *f)
+static struct vivi_fmt *__get_format(u32 pixelformat)
 {
 	struct vivi_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < ARRAY_SIZE(formats); k++) {
 		fmt = &formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == pixelformat)
 			break;
 	}
 
@@ -167,6 +166,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
 	return &formats[k];
 }
 
+static struct vivi_fmt *get_format(struct v4l2_format *f)
+{
+	return __get_format(f->fmt.pix.pixelformat);
+}
+
 /* buffer for one video frame */
 struct vivi_buffer {
 	/* common v4l buffer stuff -- must be first */
@@ -232,6 +236,7 @@ struct vivi_dev {
 
 	/* video capture */
 	struct vivi_fmt            *fmt;
+	struct v4l2_fract          timeperframe;
 	unsigned int               width, height;
 	struct vb2_queue	   vb_vidq;
 	unsigned int		   field_count;
@@ -660,8 +665,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
 
-#define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+#define frames_to_ms(dev, frames)				\
+	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -677,7 +682,7 @@ static void vivi_sleep(struct vivi_dev *dev)
 		goto stop_task;
 
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(1));
+	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
 
 	vivi_thread_tick(dev);
 
@@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	return 0;
 }
 
+/* timeperframe is arbitrary and continous */
+static int vidioc_enum_frameintervals(struct file *file, void *priv,
+					     struct v4l2_frmivalenum *fival)
+{
+	struct vivi_fmt *fmt;
+
+	if (fival->index)
+		return -EINVAL;
+
+	fmt = __get_format(fival->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	/* regarding width width & height - we support any */
+
+	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+
+	/* fill in stepwise as required by V4L2 spec, i.e.
+	 *
+	 * min <= (step = 1.0) <= max
+	 */
+	fival->stepwise.step = (struct v4l2_fract) {1, 1};
+	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
+	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
+
+	return 0;
+}
+
+static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
+static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
+		parm->parm.capture.timeperframe :
+		TPF_DEFAULT;	/* {*, 0} resets timing */
+
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1207,6 +1269,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
 	.vidioc_s_input       = vidioc_s_input,
+	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+	.vidioc_g_parm        = vidioc_g_parm,
+	.vidioc_s_parm        = vidioc_s_parm,
 	.vidioc_streamon      = vb2_ioctl_streamon,
 	.vidioc_streamoff     = vb2_ioctl_streamoff,
 	.vidioc_log_status    = v4l2_ctrl_log_status,
@@ -1265,6 +1330,7 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
+	dev->timeperframe = TPF_DEFAULT;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;
-- 
1.8.0.rc3.331.g5b9a629

Re: [PATCH v3] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Tue, Oct 23, 2012 at 05:35:21PM +0400, Kirill Smelkov wrote:
> On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > be perfect video source for testing when one don't have lots of capture
> > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > Thanks.
> > > > 
> > > > Rather than introducing a module option, it's much nicer if you can
> > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > allowing you to also support 50/59.94/60 fps.
> > > 
> > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > details well enough and various drivers do things differently. The patch
> > > is below. Is it ok?
> > 
> > Close, but it's not quite there.
> > 
> > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > (take the master branch). That will report any errors in your implementation.
> > 
> > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > for that). I would set the nominal framerate whenever the denominator == 0.
> > 
> > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > stepwise struct.
> 
> Thanks for pointers and info about v4l2-compliance handy-tool. I've
> tried to correct all the issues you mentioned above and here is the
> patch.
> 
> (Only requirement to set stepwise.step to 1.0 for
>  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
>  that's what the V4L2 spec requires...)
> 
> Thanks,
> Kirill
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 23 Oct 2012 16:56:59 +0400
> Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem yesterday, and vivi
> seemed to be perfect video source for testing when one don't have lots
> of capture boards and cameras. Only its framerate was hardcoded to
> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> needed that to precisely simulate bandwidth.
> 
> That's why here is this patch with ->enum_frameintervals() and
> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> 
> Regarding newly introduced __get_format(u32 pixelformat) I decided not
> to convert original get_format() to operate on fourcc codes, since >= 3
> places in driver need to deal with v4l2_format and otherwise it won't be
> handy.
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 9 deletions(-)
> 
> V3:
>     - corrected issues with V4L2 spec compliance as pointed by Hans
>       Verkuil.
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 3e6902a..3adea58 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> +/* default to NTSC timeperframe */
> +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> +
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>  
> @@ -150,14 +149,14 @@ static struct vivi_fmt formats[] = {
>  	},
>  };
>  
> -static struct vivi_fmt *get_format(struct v4l2_format *f)
> +static struct vivi_fmt *__get_format(u32 pixelformat)
>  {
>  	struct vivi_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -167,6 +166,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +static struct vivi_fmt *get_format(struct v4l2_format *f)
> +{
> +	return __get_format(f->fmt.pix.pixelformat);
> +}
> +
>  /* buffer for one video frame */
>  struct vivi_buffer {
>  	/* common v4l buffer stuff -- must be first */
> @@ -232,6 +236,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -660,8 +665,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -677,7 +682,7 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	struct vivi_fmt *fmt;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = __get_format(fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* regarding width width & height - we support any */
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +
> +	/* fill in stepwise as required by V4L2 spec, i.e.
> +	 *
> +	 * min <= (step = 1.0) <= max
> +	 */
> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> +		parm->parm.capture.timeperframe :
> +		TPF_DEFAULT;	/* {*, 0} resets timing */
> +
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1207,6 +1269,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1265,6 +1330,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe = TPF_DEFAULT;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> -- 
> 1.8.0.rc3.331.g5b9a629

Ping. Is maybe something wrong with it?

Re: [PATCH v3] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

Thanks for the ping, I forgot about this patch...

On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
> On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > be perfect video source for testing when one don't have lots of capture
> > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > Thanks.
> > > > 
> > > > Rather than introducing a module option, it's much nicer if you can
> > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > allowing you to also support 50/59.94/60 fps.
> > > 
> > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > details well enough and various drivers do things differently. The patch
> > > is below. Is it ok?
> > 
> > Close, but it's not quite there.
> > 
> > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > (take the master branch). That will report any errors in your implementation.
> > 
> > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > for that). I would set the nominal framerate whenever the denominator == 0.
> > 
> > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > stepwise struct.
> 
> Thanks for pointers and info about v4l2-compliance handy-tool. I've
> tried to correct all the issues you mentioned above and here is the
> patch.
> 
> (Only requirement to set stepwise.step to 1.0 for
>  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
>  that's what the V4L2 spec requires...)
> 
> Thanks,
> Kirill
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 23 Oct 2012 16:56:59 +0400
> Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem yesterday, and vivi
> seemed to be perfect video source for testing when one don't have lots
> of capture boards and cameras. Only its framerate was hardcoded to
> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> needed that to precisely simulate bandwidth.
> 
> That's why here is this patch with ->enum_frameintervals() and
> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> 
> Regarding newly introduced __get_format(u32 pixelformat) I decided not
> to convert original get_format() to operate on fourcc codes, since >= 3
> places in driver need to deal with v4l2_format and otherwise it won't be
> handy.
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 9 deletions(-)
> 
> V3:
>     - corrected issues with V4L2 spec compliance as pointed by Hans
>       Verkuil.
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 3e6902a..3adea58 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> +/* default to NTSC timeperframe */
> +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};

Keep the name lower case: tpf_default. Upper case is for defines only.

> +
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>  
> @@ -150,14 +149,14 @@ static struct vivi_fmt formats[] = {
>  	},
>  };
>  
> -static struct vivi_fmt *get_format(struct v4l2_format *f)
> +static struct vivi_fmt *__get_format(u32 pixelformat)
>  {
>  	struct vivi_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -167,6 +166,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +static struct vivi_fmt *get_format(struct v4l2_format *f)
> +{
> +	return __get_format(f->fmt.pix.pixelformat);
> +}
> +
>  /* buffer for one video frame */
>  struct vivi_buffer {
>  	/* common v4l buffer stuff -- must be first */
> @@ -232,6 +236,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -660,8 +665,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -677,7 +682,7 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	struct vivi_fmt *fmt;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = __get_format(fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* regarding width width & height - we support any */
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +
> +	/* fill in stepwise as required by V4L2 spec, i.e.
> +	 *
> +	 * min <= (step = 1.0) <= max
> +	 */
> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};

Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
framerates? {2, 1} is 2 fps, which is a bit low :-)

> +
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?

This should check that the fps is between the min and max values as reported by
vidioc_enum_frameintervals(). Fall back to the default if the values are out of
range.

> +		parm->parm.capture.timeperframe :
> +		TPF_DEFAULT;	/* {*, 0} resets timing */
> +
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1207,6 +1269,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1265,6 +1330,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe = TPF_DEFAULT;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> 

Regards,

	Hans

Re: [PATCH v3] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 02, 2012 at 03:42:21PM +0100, Hans Verkuil wrote:
> Thanks for the ping, I forgot about this patch...

Thanks for the reply. Have to run now - will try to rework it in several
days.

Thanks again,
Kirill


> On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
> > On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > > be perfect video source for testing when one don't have lots of capture
> > > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > > Thanks.
> > > > > 
> > > > > Rather than introducing a module option, it's much nicer if you can
> > > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > > allowing you to also support 50/59.94/60 fps.
> > > > 
> > > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > > details well enough and various drivers do things differently. The patch
> > > > is below. Is it ok?
> > > 
> > > Close, but it's not quite there.
> > > 
> > > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > > (take the master branch). That will report any errors in your implementation.
> > > 
> > > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > > for that). I would set the nominal framerate whenever the denominator == 0.
> > > 
> > > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > > stepwise struct.
> > 
> > Thanks for pointers and info about v4l2-compliance handy-tool. I've
> > tried to correct all the issues you mentioned above and here is the
> > patch.
> > 
> > (Only requirement to set stepwise.step to 1.0 for
> >  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
> >  that's what the V4L2 spec requires...)
> > 
> > Thanks,
> > Kirill
> > 
> > ---- 8< ----
> > From: Kirill Smelkov <kirr@mns.spb.ru>
> > Date: Tue, 23 Oct 2012 16:56:59 +0400
> > Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> > 
> > I was testing my video-over-ethernet subsystem yesterday, and vivi
> > seemed to be perfect video source for testing when one don't have lots
> > of capture boards and cameras. Only its framerate was hardcoded to
> > NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> > needed that to precisely simulate bandwidth.
> > 
> > That's why here is this patch with ->enum_frameintervals() and
> > ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> > v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> > 
> > Regarding newly introduced __get_format(u32 pixelformat) I decided not
> > to convert original get_format() to operate on fourcc codes, since >= 3
> > places in driver need to deal with v4l2_format and otherwise it won't be
> > handy.
> > 
> > Thanks.
> > 
> > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > ---
> >  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> > 
> > V3:
> >     - corrected issues with V4L2 spec compliance as pointed by Hans
> >       Verkuil.
> > 
> > V2:
> > 
> >     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
> >       by Hans Verkuil.
> > 
> > 
> > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > index 3e6902a..3adea58 100644
> > --- a/drivers/media/platform/vivi.c
> > +++ b/drivers/media/platform/vivi.c
> > @@ -36,10 +36,6 @@
> >  
> >  #define VIVI_MODULE_NAME "vivi"
> >  
> > -/* Wake up at about 30 fps */
> > -#define WAKE_NUMERATOR 30
> > -#define WAKE_DENOMINATOR 1001
> > -
> >  #define MAX_WIDTH 1920
> >  #define MAX_HEIGHT 1200
> >  
> > @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >  /* Global font descriptor */
> >  static const u8 *font8x16;
> >  
> > +/* default to NTSC timeperframe */
> > +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> 
> Keep the name lower case: tpf_default. Upper case is for defines only.
> 
> > +
> >  #define dprintk(dev, level, fmt, arg...) \
> >  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
> >  
> > @@ -150,14 +149,14 @@ static struct vivi_fmt formats[] = {
> >  	},
> >  };
> >  
> > -static struct vivi_fmt *get_format(struct v4l2_format *f)
> > +static struct vivi_fmt *__get_format(u32 pixelformat)
> >  {
> >  	struct vivi_fmt *fmt;
> >  	unsigned int k;
> >  
> >  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> >  		fmt = &formats[k];
> > -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> > +		if (fmt->fourcc == pixelformat)
> >  			break;
> >  	}
> >  
> > @@ -167,6 +166,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
> >  	return &formats[k];
> >  }
> >  
> > +static struct vivi_fmt *get_format(struct v4l2_format *f)
> > +{
> > +	return __get_format(f->fmt.pix.pixelformat);
> > +}
> > +
> >  /* buffer for one video frame */
> >  struct vivi_buffer {
> >  	/* common v4l buffer stuff -- must be first */
> > @@ -232,6 +236,7 @@ struct vivi_dev {
> >  
> >  	/* video capture */
> >  	struct vivi_fmt            *fmt;
> > +	struct v4l2_fract          timeperframe;
> >  	unsigned int               width, height;
> >  	struct vb2_queue	   vb_vidq;
> >  	unsigned int		   field_count;
> > @@ -660,8 +665,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
> >  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
> >  }
> >  
> > -#define frames_to_ms(frames)					\
> > -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> > +#define frames_to_ms(dev, frames)				\
> > +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
> >  
> >  static void vivi_sleep(struct vivi_dev *dev)
> >  {
> > @@ -677,7 +682,7 @@ static void vivi_sleep(struct vivi_dev *dev)
> >  		goto stop_task;
> >  
> >  	/* Calculate time to wake up */
> > -	timeout = msecs_to_jiffies(frames_to_ms(1));
> > +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
> >  
> >  	vivi_thread_tick(dev);
> >  
> > @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> >  	return 0;
> >  }
> >  
> > +/* timeperframe is arbitrary and continous */
> > +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> > +					     struct v4l2_frmivalenum *fival)
> > +{
> > +	struct vivi_fmt *fmt;
> > +
> > +	if (fival->index)
> > +		return -EINVAL;
> > +
> > +	fmt = __get_format(fival->pixel_format);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	/* regarding width width & height - we support any */
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > +
> > +	/* fill in stepwise as required by V4L2 spec, i.e.
> > +	 *
> > +	 * min <= (step = 1.0) <= max
> > +	 */
> > +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> > +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> > +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
> 
> Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
> framerates? {2, 1} is 2 fps, which is a bit low :-)
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > +	parm->parm.capture.readbuffers  = 1;
> > +	return 0;
> > +}
> > +
> > +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> 
> This should check that the fps is between the min and max values as reported by
> vidioc_enum_frameintervals(). Fall back to the default if the values are out of
> range.
> 
> > +		parm->parm.capture.timeperframe :
> > +		TPF_DEFAULT;	/* {*, 0} resets timing */
> > +
> > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > +	parm->parm.capture.readbuffers  = 1;
> > +	return 0;
> > +}
> > +
> >  /* --- controls ---------------------------------------------- */
> >  
> >  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -1207,6 +1269,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
> >  	.vidioc_enum_input    = vidioc_enum_input,
> >  	.vidioc_g_input       = vidioc_g_input,
> >  	.vidioc_s_input       = vidioc_s_input,
> > +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> > +	.vidioc_g_parm        = vidioc_g_parm,
> > +	.vidioc_s_parm        = vidioc_s_parm,
> >  	.vidioc_streamon      = vb2_ioctl_streamon,
> >  	.vidioc_streamoff     = vb2_ioctl_streamoff,
> >  	.vidioc_log_status    = v4l2_ctrl_log_status,
> > @@ -1265,6 +1330,7 @@ static int __init vivi_create_instance(int inst)
> >  		goto free_dev;
> >  
> >  	dev->fmt = &formats[0];
> > +	dev->timeperframe = TPF_DEFAULT;
> >  	dev->width = 640;
> >  	dev->height = 480;
> >  	dev->pixelsize = dev->fmt->depth / 8;

[PATCH v4] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 02, 2012 at 03:42:21PM +0100, Hans Verkuil wrote:
> Thanks for the ping, I forgot about this patch...

Thanks for feedback and for waiting. I'm here again...


> On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
> > On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > > be perfect video source for testing when one don't have lots of capture
> > > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > > Thanks.
> > > > > 
> > > > > Rather than introducing a module option, it's much nicer if you can
> > > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > > allowing you to also support 50/59.94/60 fps.
> > > > 
> > > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > > details well enough and various drivers do things differently. The patch
> > > > is below. Is it ok?
> > > 
> > > Close, but it's not quite there.
> > > 
> > > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > > (take the master branch). That will report any errors in your implementation.
> > > 
> > > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > > for that). I would set the nominal framerate whenever the denominator == 0.
> > > 
> > > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > > stepwise struct.
> > 
> > Thanks for pointers and info about v4l2-compliance handy-tool. I've
> > tried to correct all the issues you mentioned above and here is the
> > patch.
> > 
> > (Only requirement to set stepwise.step to 1.0 for
> >  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
> >  that's what the V4L2 spec requires...)
> > 
> > Thanks,
> > Kirill
> > 
> > ---- 8< ----
> > From: Kirill Smelkov <kirr@mns.spb.ru>
> > Date: Tue, 23 Oct 2012 16:56:59 +0400
> > Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> > 
> > I was testing my video-over-ethernet subsystem yesterday, and vivi
> > seemed to be perfect video source for testing when one don't have lots
> > of capture boards and cameras. Only its framerate was hardcoded to
> > NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> > needed that to precisely simulate bandwidth.
> > 
> > That's why here is this patch with ->enum_frameintervals() and
> > ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> > v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> > 
> > Regarding newly introduced __get_format(u32 pixelformat) I decided not
> > to convert original get_format() to operate on fourcc codes, since >= 3
> > places in driver need to deal with v4l2_format and otherwise it won't be
> > handy.
> > 
> > Thanks.
> > 
> > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > ---
> >  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> > 
> > V3:
> >     - corrected issues with V4L2 spec compliance as pointed by Hans
> >       Verkuil.
> > 
> > V2:
> > 
> >     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
> >       by Hans Verkuil.
> > 
> > 
> > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > index 3e6902a..3adea58 100644
> > --- a/drivers/media/platform/vivi.c
> > +++ b/drivers/media/platform/vivi.c
> > @@ -36,10 +36,6 @@
> >  
> >  #define VIVI_MODULE_NAME "vivi"
> >  
> > -/* Wake up at about 30 fps */
> > -#define WAKE_NUMERATOR 30
> > -#define WAKE_DENOMINATOR 1001
> > -
> >  #define MAX_WIDTH 1920
> >  #define MAX_HEIGHT 1200
> >  
> > @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >  /* Global font descriptor */
> >  static const u8 *font8x16;
> >  
> > +/* default to NTSC timeperframe */
> > +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> 
> Keep the name lower case: tpf_default. Upper case is for defines only.

ok

[...]

> > @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> >  	return 0;
> >  }
> >  
> > +/* timeperframe is arbitrary and continous */
> > +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> > +					     struct v4l2_frmivalenum *fival)
> > +{
> > +	struct vivi_fmt *fmt;
> > +
> > +	if (fival->index)
> > +		return -EINVAL;
> > +
> > +	fmt = __get_format(fival->pixel_format);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	/* regarding width width & height - we support any */
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > +
> > +	/* fill in stepwise as required by V4L2 spec, i.e.
> > +	 *
> > +	 * min <= (step = 1.0) <= max
> > +	 */
> > +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> > +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> > +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
> 
> Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
> framerates? {2, 1} is 2 fps, which is a bit low :-)

 [ see below ... ]

> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > +	parm->parm.capture.readbuffers  = 1;
> > +	return 0;
> > +}
> > +
> > +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> 
> This should check that the fps is between the min and max values as reported by
> vidioc_enum_frameintervals(). Fall back to the default if the values are out of
> range.

{2, 1} is 0.5 fps and {120, 1} is 1/120 fps :) but anyway, why should we
set that artificial limits here?

Thanks for catching the actual bug, but I propose we set min=1/infty and
max=infty so that the user chooses which tpf/fps he/she needs, be it
9000 fps or 1 frame per hour. And imho it's better to clip the value,
than to fallback to default (but we still reset it if */0 is coming from
userspace).

Said all that, here is the interdiff and updated patch.

Thanks,
Kirill

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 37d0af8..5d1b374 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
 /* Global font descriptor */
 static const u8 *font8x16;
 
-/* default to NTSC timeperframe */
-static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
+/* timeperframe: min/max and default */
+static const struct v4l2_fract
+	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
+	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
+	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
 
 #define dprintk(dev, level, fmt, arg...) \
 	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
@@ -1098,17 +1101,14 @@ static int vidioc_enum_frameintervals(struct file *file, void *priv,
 	if (!fmt)
 		return -EINVAL;
 
-	/* regarding width width & height - we support any */
+	/* regarding width & height - we support any */
 
 	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
 
-	/* fill in stepwise as required by V4L2 spec, i.e.
-	 *
-	 * min <= (step = 1.0) <= max
-	 */
+	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
+	fival->stepwise.min  = tpf_min;
+	fival->stepwise.max  = tpf_max;
 	fival->stepwise.step = (struct v4l2_fract) {1, 1};
-	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
-	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
 
 	return 0;
 }
@@ -1126,18 +1126,26 @@ static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *
 	return 0;
 }
 
+#define FRACT_CMP(a, OP, b)	\
+	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
+
 static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
 {
 	struct vivi_dev *dev = video_drvdata(file);
+	struct v4l2_fract tpf;
 
 	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
 
-	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
-		parm->parm.capture.timeperframe :
-		TPF_DEFAULT;	/* {*, 0} resets timing */
+	tpf = parm->parm.capture.timeperframe;
 
-	parm->parm.capture.timeperframe = dev->timeperframe;
+	/* tpf: {*, 0} resets timing; clip to [min, max]*/
+	tpf = tpf.denominator ? tpf : tpf_default;
+	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+
+	dev->timeperframe = tpf;
+	parm->parm.capture.timeperframe = tpf;
 	parm->parm.capture.readbuffers  = 1;
 	return 0;
 }
@@ -1361,7 +1369,7 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
-	dev->timeperframe = TPF_DEFAULT;
+	dev->timeperframe = tpf_default;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;



---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Tue, 23 Oct 2012 16:56:59 +0400
Subject: [PATCH v4] [media] vivi: Teach it to tune FPS

I was testing my video-over-ethernet subsystem recently, and vivi
seemed to be perfect video source for testing when one don't have lots
of capture boards and cameras. Only its framerate was hardcoded to
NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
needed that to precisely simulate bandwidth.

That's why here is this patch with ->enum_frameintervals() and
->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.

Regarding newly introduced __get_format(u32 pixelformat) I decided not
to convert original get_format() to operate on fourcc codes, since >= 3
places in driver need to deal with v4l2_format and otherwise it won't be
handy.

Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 92 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 83 insertions(+), 9 deletions(-)

V4:
    - corrected fival->stepwise setting and added its check to s_parm();
      also cosmetics - all as per Hans Verkuil review.

V3:
    - corrected issues with V4L2 spec compliance as pointed by Hans
      Verkuil.

V2:

    - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
      by Hans Verkuil.


diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 6e6dd25..5d1b374 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,10 +36,6 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
-
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -69,6 +65,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
 /* Global font descriptor */
 static const u8 *font8x16;
 
+/* timeperframe: min/max and default */
+static const struct v4l2_fract
+	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
+	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
+	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
+
 #define dprintk(dev, level, fmt, arg...) \
 	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
 
@@ -150,14 +152,14 @@ static struct vivi_fmt formats[] = {
 	},
 };
 
-static struct vivi_fmt *get_format(struct v4l2_format *f)
+static struct vivi_fmt *__get_format(u32 pixelformat)
 {
 	struct vivi_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < ARRAY_SIZE(formats); k++) {
 		fmt = &formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == pixelformat)
 			break;
 	}
 
@@ -167,6 +169,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
 	return &formats[k];
 }
 
+static struct vivi_fmt *get_format(struct v4l2_format *f)
+{
+	return __get_format(f->fmt.pix.pixelformat);
+}
+
 /* buffer for one video frame */
 struct vivi_buffer {
 	/* common v4l buffer stuff -- must be first */
@@ -232,6 +239,7 @@ struct vivi_dev {
 
 	/* video capture */
 	struct vivi_fmt            *fmt;
+	struct v4l2_fract          timeperframe;
 	unsigned int               width, height;
 	struct vb2_queue	   vb_vidq;
 	unsigned int		   field_count;
@@ -691,8 +699,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
 
-#define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+#define frames_to_ms(dev, frames)				\
+	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -708,7 +716,7 @@ static void vivi_sleep(struct vivi_dev *dev)
 		goto stop_task;
 
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(1));
+	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
 
 	vivi_thread_tick(dev);
 
@@ -1080,6 +1088,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	return 0;
 }
 
+/* timeperframe is arbitrary and continous */
+static int vidioc_enum_frameintervals(struct file *file, void *priv,
+					     struct v4l2_frmivalenum *fival)
+{
+	struct vivi_fmt *fmt;
+
+	if (fival->index)
+		return -EINVAL;
+
+	fmt = __get_format(fival->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	/* regarding width & height - we support any */
+
+	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+
+	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
+	fival->stepwise.min  = tpf_min;
+	fival->stepwise.max  = tpf_max;
+	fival->stepwise.step = (struct v4l2_fract) {1, 1};
+
+	return 0;
+}
+
+static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
+#define FRACT_CMP(a, OP, b)	\
+	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
+
+static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+	struct v4l2_fract tpf;
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	tpf = parm->parm.capture.timeperframe;
+
+	/* tpf: {*, 0} resets timing; clip to [min, max]*/
+	tpf = tpf.denominator ? tpf : tpf_default;
+	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+
+	dev->timeperframe = tpf;
+	parm->parm.capture.timeperframe = tpf;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1238,6 +1308,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
 	.vidioc_s_input       = vidioc_s_input,
+	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+	.vidioc_g_parm        = vidioc_g_parm,
+	.vidioc_s_parm        = vidioc_s_parm,
 	.vidioc_streamon      = vb2_ioctl_streamon,
 	.vidioc_streamoff     = vb2_ioctl_streamoff,
 	.vidioc_log_status    = v4l2_ctrl_log_status,
@@ -1296,6 +1369,7 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
+	dev->timeperframe = tpf_default;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;
-- 
1.8.0.337.g4cdca5d

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Wed, Nov 07, 2012 at 03:30:01PM +0400, Kirill Smelkov wrote:
> On Fri, Nov 02, 2012 at 03:42:21PM +0100, Hans Verkuil wrote:
> > Thanks for the ping, I forgot about this patch...
> 
> Thanks for feedback and for waiting. I'm here again...
> 
> 
> > On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
> > > On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > > > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > > > be perfect video source for testing when one don't have lots of capture
> > > > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > > > Thanks.
> > > > > > 
> > > > > > Rather than introducing a module option, it's much nicer if you can
> > > > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > > > allowing you to also support 50/59.94/60 fps.
> > > > > 
> > > > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > > > details well enough and various drivers do things differently. The patch
> > > > > is below. Is it ok?
> > > > 
> > > > Close, but it's not quite there.
> > > > 
> > > > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > > > (take the master branch). That will report any errors in your implementation.
> > > > 
> > > > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > > > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > > > for that). I would set the nominal framerate whenever the denominator == 0.
> > > > 
> > > > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > > > stepwise struct.
> > > 
> > > Thanks for pointers and info about v4l2-compliance handy-tool. I've
> > > tried to correct all the issues you mentioned above and here is the
> > > patch.
> > > 
> > > (Only requirement to set stepwise.step to 1.0 for
> > >  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
> > >  that's what the V4L2 spec requires...)
> > > 
> > > Thanks,
> > > Kirill
> > > 
> > > ---- 8< ----
> > > From: Kirill Smelkov <kirr@mns.spb.ru>
> > > Date: Tue, 23 Oct 2012 16:56:59 +0400
> > > Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> > > 
> > > I was testing my video-over-ethernet subsystem yesterday, and vivi
> > > seemed to be perfect video source for testing when one don't have lots
> > > of capture boards and cameras. Only its framerate was hardcoded to
> > > NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> > > needed that to precisely simulate bandwidth.
> > > 
> > > That's why here is this patch with ->enum_frameintervals() and
> > > ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> > > v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> > > 
> > > Regarding newly introduced __get_format(u32 pixelformat) I decided not
> > > to convert original get_format() to operate on fourcc codes, since >= 3
> > > places in driver need to deal with v4l2_format and otherwise it won't be
> > > handy.
> > > 
> > > Thanks.
> > > 
> > > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > > ---
> > >  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 75 insertions(+), 9 deletions(-)
> > > 
> > > V3:
> > >     - corrected issues with V4L2 spec compliance as pointed by Hans
> > >       Verkuil.
> > > 
> > > V2:
> > > 
> > >     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
> > >       by Hans Verkuil.
> > > 
> > > 
> > > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > index 3e6902a..3adea58 100644
> > > --- a/drivers/media/platform/vivi.c
> > > +++ b/drivers/media/platform/vivi.c
> > > @@ -36,10 +36,6 @@
> > >  
> > >  #define VIVI_MODULE_NAME "vivi"
> > >  
> > > -/* Wake up at about 30 fps */
> > > -#define WAKE_NUMERATOR 30
> > > -#define WAKE_DENOMINATOR 1001
> > > -
> > >  #define MAX_WIDTH 1920
> > >  #define MAX_HEIGHT 1200
> > >  
> > > @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > >  /* Global font descriptor */
> > >  static const u8 *font8x16;
> > >  
> > > +/* default to NTSC timeperframe */
> > > +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > 
> > Keep the name lower case: tpf_default. Upper case is for defines only.
> 
> ok
> 
> [...]
> 
> > > @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> > >  	return 0;
> > >  }
> > >  
> > > +/* timeperframe is arbitrary and continous */
> > > +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> > > +					     struct v4l2_frmivalenum *fival)
> > > +{
> > > +	struct vivi_fmt *fmt;
> > > +
> > > +	if (fival->index)
> > > +		return -EINVAL;
> > > +
> > > +	fmt = __get_format(fival->pixel_format);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +
> > > +	/* regarding width width & height - we support any */
> > > +
> > > +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > > +
> > > +	/* fill in stepwise as required by V4L2 spec, i.e.
> > > +	 *
> > > +	 * min <= (step = 1.0) <= max
> > > +	 */
> > > +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> > > +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> > > +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
> > 
> > Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
> > framerates? {2, 1} is 2 fps, which is a bit low :-)
> 
>  [ see below ... ]
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > > +{
> > > +	struct vivi_dev *dev = video_drvdata(file);
> > > +
> > > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		return -EINVAL;
> > > +
> > > +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> > > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > > +	parm->parm.capture.readbuffers  = 1;
> > > +	return 0;
> > > +}
> > > +
> > > +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > > +{
> > > +	struct vivi_dev *dev = video_drvdata(file);
> > > +
> > > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		return -EINVAL;
> > > +
> > > +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> > 
> > This should check that the fps is between the min and max values as reported by
> > vidioc_enum_frameintervals(). Fall back to the default if the values are out of
> > range.
> 
> {2, 1} is 0.5 fps and {120, 1} is 1/120 fps :) but anyway, why should we
> set that artificial limits here?
> 
> Thanks for catching the actual bug, but I propose we set min=1/infty and
> max=infty so that the user chooses which tpf/fps he/she needs, be it
> 9000 fps or 1 frame per hour. And imho it's better to clip the value,
> than to fallback to default (but we still reset it if */0 is coming from
> userspace).
> 
> Said all that, here is the interdiff and updated patch.
> 
> Thanks,
> Kirill
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 37d0af8..5d1b374 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> -/* default to NTSC timeperframe */
> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> +/* timeperframe: min/max and default */
> +static const struct v4l2_fract
> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> +	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
>  
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
> @@ -1098,17 +1101,14 @@ static int vidioc_enum_frameintervals(struct file *file, void *priv,
>  	if (!fmt)
>  		return -EINVAL;
>  
> -	/* regarding width width & height - we support any */
> +	/* regarding width & height - we support any */
>  
>  	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>  
> -	/* fill in stepwise as required by V4L2 spec, i.e.
> -	 *
> -	 * min <= (step = 1.0) <= max
> -	 */
> +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> +	fival->stepwise.min  = tpf_min;
> +	fival->stepwise.max  = tpf_max;
>  	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> -	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> -	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
>  
>  	return 0;
>  }
> @@ -1126,18 +1126,26 @@ static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *
>  	return 0;
>  }
>  
> +#define FRACT_CMP(a, OP, b)	\
> +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> +
>  static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
>  {
>  	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fract tpf;
>  
>  	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
> -	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> -		parm->parm.capture.timeperframe :
> -		TPF_DEFAULT;	/* {*, 0} resets timing */
> +	tpf = parm->parm.capture.timeperframe;
>  
> -	parm->parm.capture.timeperframe = dev->timeperframe;
> +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> +	tpf = tpf.denominator ? tpf : tpf_default;
> +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> +
> +	dev->timeperframe = tpf;
> +	parm->parm.capture.timeperframe = tpf;
>  	parm->parm.capture.readbuffers  = 1;
>  	return 0;
>  }
> @@ -1361,7 +1369,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> -	dev->timeperframe = TPF_DEFAULT;
> +	dev->timeperframe = tpf_default;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> 
> 
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 23 Oct 2012 16:56:59 +0400
> Subject: [PATCH v4] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem recently, and vivi
> seemed to be perfect video source for testing when one don't have lots
> of capture boards and cameras. Only its framerate was hardcoded to
> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> needed that to precisely simulate bandwidth.
> 
> That's why here is this patch with ->enum_frameintervals() and
> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> 
> Regarding newly introduced __get_format(u32 pixelformat) I decided not
> to convert original get_format() to operate on fourcc codes, since >= 3
> places in driver need to deal with v4l2_format and otherwise it won't be
> handy.
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 92 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 83 insertions(+), 9 deletions(-)
> 
> V4:
>     - corrected fival->stepwise setting and added its check to s_parm();
>       also cosmetics - all as per Hans Verkuil review.
> 
> V3:
>     - corrected issues with V4L2 spec compliance as pointed by Hans
>       Verkuil.
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 6e6dd25..5d1b374 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -69,6 +65,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> +/* timeperframe: min/max and default */
> +static const struct v4l2_fract
> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> +	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
> +
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>  
> @@ -150,14 +152,14 @@ static struct vivi_fmt formats[] = {
>  	},
>  };
>  
> -static struct vivi_fmt *get_format(struct v4l2_format *f)
> +static struct vivi_fmt *__get_format(u32 pixelformat)
>  {
>  	struct vivi_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -167,6 +169,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +static struct vivi_fmt *get_format(struct v4l2_format *f)
> +{
> +	return __get_format(f->fmt.pix.pixelformat);
> +}
> +
>  /* buffer for one video frame */
>  struct vivi_buffer {
>  	/* common v4l buffer stuff -- must be first */
> @@ -232,6 +239,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -691,8 +699,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -708,7 +716,7 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1080,6 +1088,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	struct vivi_fmt *fmt;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = __get_format(fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* regarding width & height - we support any */
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +
> +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> +	fival->stepwise.min  = tpf_min;
> +	fival->stepwise.max  = tpf_max;
> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
> +#define FRACT_CMP(a, OP, b)	\
> +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fract tpf;
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	tpf = parm->parm.capture.timeperframe;
> +
> +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> +	tpf = tpf.denominator ? tpf : tpf_default;
> +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> +
> +	dev->timeperframe = tpf;
> +	parm->parm.capture.timeperframe = tpf;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1238,6 +1308,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1296,6 +1369,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe = tpf_default;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;

Ping. Is maybe something stupid on my side?

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Mon 12 November 2012 09:12:58 Kirill Smelkov wrote:

> Ping. Is maybe something stupid on my side?
> 

No, I've been abroad and haven't had time to look at it. I want to do that
this week. Ping me again if you haven't heard from me by Saturday.

Regards,

	Hans

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Mon, Nov 12, 2012 at 10:46:26AM +0100, Hans Verkuil wrote:
> On Mon 12 November 2012 09:12:58 Kirill Smelkov wrote:
> 
> > Ping. Is maybe something stupid on my side?
> > 
> 
> No, I've been abroad and haven't had time to look at it. I want to do that
> this week. Ping me again if you haven't heard from me by Saturday.

Thanks for feedback. ok.

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> On Fri, Nov 02, 2012 at 03:42:21PM +0100, Hans Verkuil wrote:
> > Thanks for the ping, I forgot about this patch...
> 
> Thanks for feedback and for waiting. I'm here again...
> 
> 
> > On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
> > > On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
> > > > On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
> > > > > On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
> > > > > > On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
> > > > > > > I was testing my video-over-ethernet subsystem today, and vivi seemed to
> > > > > > > be perfect video source for testing when one don't have lots of capture
> > > > > > > boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
> > > > > > > while in my country we usually use PAL (25 fps). That's why the patch.
> > > > > > > Thanks.
> > > > > > 
> > > > > > Rather than introducing a module option, it's much nicer if you can
> > > > > > implement enum_frameintervals and g/s_parm. This can be made quite flexible
> > > > > > allowing you to also support 50/59.94/60 fps.
> > > > > 
> > > > > Thanks for feedback. I've reworked the patch for FPS to be set via
> > > > > ->{g,s}_parm(), and yes now it is more flexble, because one can set
> > > > > different FPS on different vivi devices. Only I don't know V4L2 ioctls
> > > > > details well enough and various drivers do things differently. The patch
> > > > > is below. Is it ok?
> > > > 
> > > > Close, but it's not quite there.
> > > > 
> > > > You should run the v4l2-compliance tool from the v4l-utils.git repository
> > > > (take the master branch). That will report any errors in your implementation.
> > > > 
> > > > In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
> > > > equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
> > > > for that). I would set the nominal framerate whenever the denominator == 0.
> > > > 
> > > > For vidioc_enum_frameintervals you need to check the IN fields and fill in the
> > > > stepwise struct.
> > > 
> > > Thanks for pointers and info about v4l2-compliance handy-tool. I've
> > > tried to correct all the issues you mentioned above and here is the
> > > patch.
> > > 
> > > (Only requirement to set stepwise.step to 1.0 for
> > >  V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
> > >  that's what the V4L2 spec requires...)
> > > 
> > > Thanks,
> > > Kirill
> > > 
> > > ---- 8< ----
> > > From: Kirill Smelkov <kirr@mns.spb.ru>
> > > Date: Tue, 23 Oct 2012 16:56:59 +0400
> > > Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
> > > 
> > > I was testing my video-over-ethernet subsystem yesterday, and vivi
> > > seemed to be perfect video source for testing when one don't have lots
> > > of capture boards and cameras. Only its framerate was hardcoded to
> > > NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> > > needed that to precisely simulate bandwidth.
> > > 
> > > That's why here is this patch with ->enum_frameintervals() and
> > > ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> > > v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> > > 
> > > Regarding newly introduced __get_format(u32 pixelformat) I decided not
> > > to convert original get_format() to operate on fourcc codes, since >= 3
> > > places in driver need to deal with v4l2_format and otherwise it won't be
> > > handy.
> > > 
> > > Thanks.
> > > 
> > > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > > ---
> > >  drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 75 insertions(+), 9 deletions(-)
> > > 
> > > V3:
> > >     - corrected issues with V4L2 spec compliance as pointed by Hans
> > >       Verkuil.
> > > 
> > > V2:
> > > 
> > >     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
> > >       by Hans Verkuil.
> > > 
> > > 
> > > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > index 3e6902a..3adea58 100644
> > > --- a/drivers/media/platform/vivi.c
> > > +++ b/drivers/media/platform/vivi.c
> > > @@ -36,10 +36,6 @@
> > >  
> > >  #define VIVI_MODULE_NAME "vivi"
> > >  
> > > -/* Wake up at about 30 fps */
> > > -#define WAKE_NUMERATOR 30
> > > -#define WAKE_DENOMINATOR 1001
> > > -
> > >  #define MAX_WIDTH 1920
> > >  #define MAX_HEIGHT 1200
> > >  
> > > @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > >  /* Global font descriptor */
> > >  static const u8 *font8x16;
> > >  
> > > +/* default to NTSC timeperframe */
> > > +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > 
> > Keep the name lower case: tpf_default. Upper case is for defines only.
> 
> ok
> 
> [...]
> 
> > > @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> > >  	return 0;
> > >  }
> > >  
> > > +/* timeperframe is arbitrary and continous */
> > > +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> > > +					     struct v4l2_frmivalenum *fival)
> > > +{
> > > +	struct vivi_fmt *fmt;
> > > +
> > > +	if (fival->index)
> > > +		return -EINVAL;
> > > +
> > > +	fmt = __get_format(fival->pixel_format);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +
> > > +	/* regarding width width & height - we support any */
> > > +
> > > +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > > +
> > > +	/* fill in stepwise as required by V4L2 spec, i.e.
> > > +	 *
> > > +	 * min <= (step = 1.0) <= max
> > > +	 */
> > > +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> > > +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> > > +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
> > 
> > Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
> > framerates? {2, 1} is 2 fps, which is a bit low :-)
> 
>  [ see below ... ]
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > > +{
> > > +	struct vivi_dev *dev = video_drvdata(file);
> > > +
> > > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		return -EINVAL;
> > > +
> > > +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> > > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > > +	parm->parm.capture.readbuffers  = 1;
> > > +	return 0;
> > > +}
> > > +
> > > +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > > +{
> > > +	struct vivi_dev *dev = video_drvdata(file);
> > > +
> > > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > > +		return -EINVAL;
> > > +
> > > +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> > 
> > This should check that the fps is between the min and max values as reported by
> > vidioc_enum_frameintervals(). Fall back to the default if the values are out of
> > range.
> 
> {2, 1} is 0.5 fps and {120, 1} is 1/120 fps :) but anyway, why should we
> set that artificial limits here?
> 
> Thanks for catching the actual bug, but I propose we set min=1/infty and
> max=infty so that the user chooses which tpf/fps he/she needs, be it
> 9000 fps or 1 frame per hour. And imho it's better to clip the value,
> than to fallback to default (but we still reset it if */0 is coming from
> userspace).
> 
> Said all that, here is the interdiff and updated patch.
> 
> Thanks,
> Kirill
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 37d0af8..5d1b374 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> -/* default to NTSC timeperframe */
> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> +/* timeperframe: min/max and default */
> +static const struct v4l2_fract
> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */

I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
might hit application errors when they manipulate these values. The shortest time
between frames is 1 ms anyway.

It's the only comment I have, it looks good otherwise.

Regards,

	Hans

> +	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
>  
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
> @@ -1098,17 +1101,14 @@ static int vidioc_enum_frameintervals(struct file *file, void *priv,
>  	if (!fmt)
>  		return -EINVAL;
>  
> -	/* regarding width width & height - we support any */
> +	/* regarding width & height - we support any */
>  
>  	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>  
> -	/* fill in stepwise as required by V4L2 spec, i.e.
> -	 *
> -	 * min <= (step = 1.0) <= max
> -	 */
> +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> +	fival->stepwise.min  = tpf_min;
> +	fival->stepwise.max  = tpf_max;
>  	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> -	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
> -	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
>  
>  	return 0;
>  }
> @@ -1126,18 +1126,26 @@ static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *
>  	return 0;
>  }
>  
> +#define FRACT_CMP(a, OP, b)	\
> +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> +
>  static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
>  {
>  	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fract tpf;
>  
>  	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		return -EINVAL;
>  
> -	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
> -		parm->parm.capture.timeperframe :
> -		TPF_DEFAULT;	/* {*, 0} resets timing */
> +	tpf = parm->parm.capture.timeperframe;
>  
> -	parm->parm.capture.timeperframe = dev->timeperframe;
> +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> +	tpf = tpf.denominator ? tpf : tpf_default;
> +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> +
> +	dev->timeperframe = tpf;
> +	parm->parm.capture.timeperframe = tpf;
>  	parm->parm.capture.readbuffers  = 1;
>  	return 0;
>  }
> @@ -1361,7 +1369,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> -	dev->timeperframe = TPF_DEFAULT;
> +	dev->timeperframe = tpf_default;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> 
> 
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 23 Oct 2012 16:56:59 +0400
> Subject: [PATCH v4] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem recently, and vivi
> seemed to be perfect video source for testing when one don't have lots
> of capture boards and cameras. Only its framerate was hardcoded to
> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> needed that to precisely simulate bandwidth.
> 
> That's why here is this patch with ->enum_frameintervals() and
> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> 
> Regarding newly introduced __get_format(u32 pixelformat) I decided not
> to convert original get_format() to operate on fourcc codes, since >= 3
> places in driver need to deal with v4l2_format and otherwise it won't be
> handy.
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 92 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 83 insertions(+), 9 deletions(-)
> 
> V4:
>     - corrected fival->stepwise setting and added its check to s_parm();
>       also cosmetics - all as per Hans Verkuil review.
> 
> V3:
>     - corrected issues with V4L2 spec compliance as pointed by Hans
>       Verkuil.
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 6e6dd25..5d1b374 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,10 +36,6 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> -
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>  
> @@ -69,6 +65,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> +/* timeperframe: min/max and default */
> +static const struct v4l2_fract
> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> +	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
> +
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>  
> @@ -150,14 +152,14 @@ static struct vivi_fmt formats[] = {
>  	},
>  };
>  
> -static struct vivi_fmt *get_format(struct v4l2_format *f)
> +static struct vivi_fmt *__get_format(u32 pixelformat)
>  {
>  	struct vivi_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -167,6 +169,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +static struct vivi_fmt *get_format(struct v4l2_format *f)
> +{
> +	return __get_format(f->fmt.pix.pixelformat);
> +}
> +
>  /* buffer for one video frame */
>  struct vivi_buffer {
>  	/* common v4l buffer stuff -- must be first */
> @@ -232,6 +239,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -691,8 +699,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -708,7 +716,7 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1080,6 +1088,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	struct vivi_fmt *fmt;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = __get_format(fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* regarding width & height - we support any */
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +
> +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> +	fival->stepwise.min  = tpf_min;
> +	fival->stepwise.max  = tpf_max;
> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
> +#define FRACT_CMP(a, OP, b)	\
> +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fract tpf;
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	tpf = parm->parm.capture.timeperframe;
> +
> +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> +	tpf = tpf.denominator ? tpf : tpf_default;
> +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> +
> +	dev->timeperframe = tpf;
> +	parm->parm.capture.timeperframe = tpf;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1238,6 +1308,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1296,6 +1369,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe = tpf_default;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;
> 

Re: [PATCH v5] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 16, 2012 at 02:38:09PM +0100, Hans Verkuil wrote:
> On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
[...]

> > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > index 37d0af8..5d1b374 100644
> > --- a/drivers/media/platform/vivi.c
> > +++ b/drivers/media/platform/vivi.c
> > @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >  /* Global font descriptor */
> >  static const u8 *font8x16;
> >  
> > -/* default to NTSC timeperframe */
> > -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > +/* timeperframe: min/max and default */
> > +static const struct v4l2_fract
> > +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> > +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> 
> I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> might hit application errors when they manipulate these values. The shortest time
> between frames is 1 ms anyway.
> 
> It's the only comment I have, it looks good otherwise.

Thanks, Let's then merge it with 1/1000 - 1000/1 limit. Ok?

Thanks again,
Kirill


---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Tue, 23 Oct 2012 16:56:59 +0400
Subject: [PATCH v5] [media] vivi: Teach it to tune FPS

I was testing my video-over-ethernet subsystem recently, and vivi
seemed to be perfect video source for testing when one don't have lots
of capture boards and cameras. Only its framerate was hardcoded to
NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
needed that to precisely simulate bandwidth.

That's why here is this patch with ->enum_frameintervals() and
->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.

Regarding newly introduced __get_format(u32 pixelformat) I decided not
to convert original get_format() to operate on fourcc codes, since >= 3
places in driver need to deal with v4l2_format and otherwise it won't be
handy.

Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 92 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 83 insertions(+), 9 deletions(-)

V5:
    - changed  1/infty - infty/1  limits to  1/1000 - 1000/1, to avoid
      hitting aplication errors when they try to manipulato those
      values, as suggested by Hans Verkuil.

V4:
    - corrected fival->stepwise setting and added its check to s_parm();
      also cosmetics - all as per Hans Verkuil review.

V3:
    - corrected issues with V4L2 spec compliance as pointed by Hans
      Verkuil.

V2:

    - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
      by Hans Verkuil.

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 6e6dd25..793d7ee 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,10 +36,6 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
-
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -69,6 +65,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
 /* Global font descriptor */
 static const u8 *font8x16;
 
+/* timeperframe: min/max and default */
+static const struct v4l2_fract
+	tpf_min     = {.numerator = 1,		.denominator = 1000},	/* ~1/infty */
+	tpf_max     = {.numerator = 1000,	.denominator = 1},	/* ~infty */
+	tpf_default = {.numerator = 1001,	.denominator = 30000};	/* NTSC */
+
 #define dprintk(dev, level, fmt, arg...) \
 	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
 
@@ -150,14 +152,14 @@ static struct vivi_fmt formats[] = {
 	},
 };
 
-static struct vivi_fmt *get_format(struct v4l2_format *f)
+static struct vivi_fmt *__get_format(u32 pixelformat)
 {
 	struct vivi_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < ARRAY_SIZE(formats); k++) {
 		fmt = &formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == pixelformat)
 			break;
 	}
 
@@ -167,6 +169,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
 	return &formats[k];
 }
 
+static struct vivi_fmt *get_format(struct v4l2_format *f)
+{
+	return __get_format(f->fmt.pix.pixelformat);
+}
+
 /* buffer for one video frame */
 struct vivi_buffer {
 	/* common v4l buffer stuff -- must be first */
@@ -232,6 +239,7 @@ struct vivi_dev {
 
 	/* video capture */
 	struct vivi_fmt            *fmt;
+	struct v4l2_fract          timeperframe;
 	unsigned int               width, height;
 	struct vb2_queue	   vb_vidq;
 	unsigned int		   field_count;
@@ -691,8 +699,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
 
-#define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+#define frames_to_ms(dev, frames)				\
+	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -708,7 +716,7 @@ static void vivi_sleep(struct vivi_dev *dev)
 		goto stop_task;
 
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(1));
+	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
 
 	vivi_thread_tick(dev);
 
@@ -1080,6 +1088,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	return 0;
 }
 
+/* timeperframe is arbitrary and continous */
+static int vidioc_enum_frameintervals(struct file *file, void *priv,
+					     struct v4l2_frmivalenum *fival)
+{
+	struct vivi_fmt *fmt;
+
+	if (fival->index)
+		return -EINVAL;
+
+	fmt = __get_format(fival->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	/* regarding width & height - we support any */
+
+	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+
+	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
+	fival->stepwise.min  = tpf_min;
+	fival->stepwise.max  = tpf_max;
+	fival->stepwise.step = (struct v4l2_fract) {1, 1};
+
+	return 0;
+}
+
+static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
+#define FRACT_CMP(a, OP, b)	\
+	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
+
+static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+	struct v4l2_fract tpf;
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	tpf = parm->parm.capture.timeperframe;
+
+	/* tpf: {*, 0} resets timing; clip to [min, max]*/
+	tpf = tpf.denominator ? tpf : tpf_default;
+	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+
+	dev->timeperframe = tpf;
+	parm->parm.capture.timeperframe = tpf;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1238,6 +1308,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
 	.vidioc_s_input       = vidioc_s_input,
+	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+	.vidioc_g_parm        = vidioc_g_parm,
+	.vidioc_s_parm        = vidioc_s_parm,
 	.vidioc_streamon      = vb2_ioctl_streamon,
 	.vidioc_streamoff     = vb2_ioctl_streamoff,
 	.vidioc_log_status    = v4l2_ctrl_log_status,
@@ -1296,6 +1369,7 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
+	dev->timeperframe = tpf_default;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;
-- 
1.8.0.289.g7a667bc

Re: [PATCH v5] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Fri November 16 2012 15:48:41 Kirill Smelkov wrote:
> On Fri, Nov 16, 2012 at 02:38:09PM +0100, Hans Verkuil wrote:
> > On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> [...]
> 
> > > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > index 37d0af8..5d1b374 100644
> > > --- a/drivers/media/platform/vivi.c
> > > +++ b/drivers/media/platform/vivi.c
> > > @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > >  /* Global font descriptor */
> > >  static const u8 *font8x16;
> > >  
> > > -/* default to NTSC timeperframe */
> > > -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > > +/* timeperframe: min/max and default */
> > > +static const struct v4l2_fract
> > > +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> > > +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> > 
> > I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> > 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> > might hit application errors when they manipulate these values. The shortest time
> > between frames is 1 ms anyway.
> > 
> > It's the only comment I have, it looks good otherwise.
> 
> Thanks, Let's then merge it with 1/1000 - 1000/1 limit. Ok?

OK.

	Hans

Re: [PATCH v5] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 16, 2012 at 03:51:23PM +0100, Hans Verkuil wrote:
> On Fri November 16 2012 15:48:41 Kirill Smelkov wrote:
> > On Fri, Nov 16, 2012 at 02:38:09PM +0100, Hans Verkuil wrote:
> > > On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> > [...]
> > 
> > > > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > > index 37d0af8..5d1b374 100644
> > > > --- a/drivers/media/platform/vivi.c
> > > > +++ b/drivers/media/platform/vivi.c
> > > > @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > > >  /* Global font descriptor */
> > > >  static const u8 *font8x16;
> > > >  
> > > > -/* default to NTSC timeperframe */
> > > > -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > > > +/* timeperframe: min/max and default */
> > > > +static const struct v4l2_fract
> > > > +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> > > > +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> > > 
> > > I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> > > 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> > > might hit application errors when they manipulate these values. The shortest time
> > > between frames is 1 ms anyway.
> > > 
> > > It's the only comment I have, it looks good otherwise.
> > 
> > Thanks, Let's then merge it with 1/1000 - 1000/1 limit. Ok?
> 
> OK.

Thanks.

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Mauro Carvalho Chehab]

Em 16-11-2012 11:38, Hans Verkuil escreveu:
> On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
>> On Fri, Nov 02, 2012 at 03:42:21PM +0100, Hans Verkuil wrote:
>>> Thanks for the ping, I forgot about this patch...
>>
>> Thanks for feedback and for waiting. I'm here again...
>>
>>
>>> On Tue October 23 2012 15:35:21 Kirill Smelkov wrote:
>>>> On Tue, Oct 23, 2012 at 08:40:04AM +0200, Hans Verkuil wrote:
>>>>> On Mon October 22 2012 19:01:40 Kirill Smelkov wrote:
>>>>>> On Mon, Oct 22, 2012 at 04:16:14PM +0200, Hans Verkuil wrote:
>>>>>>> On Mon October 22 2012 15:54:44 Kirill Smelkov wrote:
>>>>>>>> I was testing my video-over-ethernet subsystem today, and vivi seemed to
>>>>>>>> be perfect video source for testing when one don't have lots of capture
>>>>>>>> boards and cameras. Only its framerate was hardcoded to NTSC's 30fps,
>>>>>>>> while in my country we usually use PAL (25 fps). That's why the patch.
>>>>>>>> Thanks.
>>>>>>>
>>>>>>> Rather than introducing a module option, it's much nicer if you can
>>>>>>> implement enum_frameintervals and g/s_parm. This can be made quite flexible
>>>>>>> allowing you to also support 50/59.94/60 fps.
>>>>>>
>>>>>> Thanks for feedback. I've reworked the patch for FPS to be set via
>>>>>> ->{g,s}_parm(), and yes now it is more flexble, because one can set
>>>>>> different FPS on different vivi devices. Only I don't know V4L2 ioctls
>>>>>> details well enough and various drivers do things differently. The patch
>>>>>> is below. Is it ok?
>>>>>
>>>>> Close, but it's not quite there.
>>>>>
>>>>> You should run the v4l2-compliance tool from the v4l-utils.git repository
>>>>> (take the master branch). That will report any errors in your implementation.
>>>>>
>>>>> In this case g/s_parm doesn't set readbuffers (set it to 1) and if timeperframe
>>>>> equals { 0, 0 }, then you should get a nominal framerate (let's stick to 29.97
>>>>> for that). I would set the nominal framerate whenever the denominator == 0.
>>>>>
>>>>> For vidioc_enum_frameintervals you need to check the IN fields and fill in the
>>>>> stepwise struct.
>>>>
>>>> Thanks for pointers and info about v4l2-compliance handy-tool. I've
>>>> tried to correct all the issues you mentioned above and here is the
>>>> patch.
>>>>
>>>> (Only requirement to set stepwise.step to 1.0 for
>>>>   V4L2_FRMIVAL_TYPE_CONTINUOUS seems a bit illogical to me, but anyway,
>>>>   that's what the V4L2 spec requires...)
>>>>
>>>> Thanks,
>>>> Kirill
>>>>
>>>> ---- 8< ----
>>>> From: Kirill Smelkov <kirr@mns.spb.ru>
>>>> Date: Tue, 23 Oct 2012 16:56:59 +0400
>>>> Subject: [PATCH v3] [media] vivi: Teach it to tune FPS
>>>>
>>>> I was testing my video-over-ethernet subsystem yesterday, and vivi
>>>> seemed to be perfect video source for testing when one don't have lots
>>>> of capture boards and cameras. Only its framerate was hardcoded to
>>>> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
>>>> needed that to precisely simulate bandwidth.
>>>>
>>>> That's why here is this patch with ->enum_frameintervals() and
>>>> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
>>>> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
>>>>
>>>> Regarding newly introduced __get_format(u32 pixelformat) I decided not
>>>> to convert original get_format() to operate on fourcc codes, since >= 3
>>>> places in driver need to deal with v4l2_format and otherwise it won't be
>>>> handy.
>>>>
>>>> Thanks.
>>>>
>>>> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
>>>> ---
>>>>   drivers/media/platform/vivi.c | 84 ++++++++++++++++++++++++++++++++++++++-----
>>>>   1 file changed, 75 insertions(+), 9 deletions(-)
>>>>
>>>> V3:
>>>>      - corrected issues with V4L2 spec compliance as pointed by Hans
>>>>        Verkuil.
>>>>
>>>> V2:
>>>>
>>>>      - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>>>>        by Hans Verkuil.
>>>>
>>>>
>>>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>>>> index 3e6902a..3adea58 100644
>>>> --- a/drivers/media/platform/vivi.c
>>>> +++ b/drivers/media/platform/vivi.c
>>>> @@ -36,10 +36,6 @@
>>>>
>>>>   #define VIVI_MODULE_NAME "vivi"
>>>>
>>>> -/* Wake up at about 30 fps */
>>>> -#define WAKE_NUMERATOR 30
>>>> -#define WAKE_DENOMINATOR 1001
>>>> -
>>>>   #define MAX_WIDTH 1920
>>>>   #define MAX_HEIGHT 1200
>>>>
>>>> @@ -69,6 +65,9 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>>>>   /* Global font descriptor */
>>>>   static const u8 *font8x16;
>>>>
>>>> +/* default to NTSC timeperframe */
>>>> +static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
>>>
>>> Keep the name lower case: tpf_default. Upper case is for defines only.
>>
>> ok
>>
>> [...]
>>
>>>> @@ -1049,6 +1054,63 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>>>>   	return 0;
>>>>   }
>>>>
>>>> +/* timeperframe is arbitrary and continous */
>>>> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
>>>> +					     struct v4l2_frmivalenum *fival)
>>>> +{
>>>> +	struct vivi_fmt *fmt;
>>>> +
>>>> +	if (fival->index)
>>>> +		return -EINVAL;
>>>> +
>>>> +	fmt = __get_format(fival->pixel_format);
>>>> +	if (!fmt)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* regarding width width & height - we support any */
>>>> +
>>>> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>>>> +
>>>> +	/* fill in stepwise as required by V4L2 spec, i.e.
>>>> +	 *
>>>> +	 * min <= (step = 1.0) <= max
>>>> +	 */
>>>> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
>>>> +	fival->stepwise.min  = (struct v4l2_fract) {1, 1};
>>>> +	fival->stepwise.max  = (struct v4l2_fract) {2, 1};
>>>
>>> Shouldn't max for {60, 1} or perhaps even {120, 1} if you want to be able to test 120 Hz
>>> framerates? {2, 1} is 2 fps, which is a bit low :-)
>>
>>   [ see below ... ]
>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
>>>> +{
>>>> +	struct vivi_dev *dev = video_drvdata(file);
>>>> +
>>>> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> +		return -EINVAL;
>>>> +
>>>> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
>>>> +	parm->parm.capture.timeperframe = dev->timeperframe;
>>>> +	parm->parm.capture.readbuffers  = 1;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
>>>> +{
>>>> +	struct vivi_dev *dev = video_drvdata(file);
>>>> +
>>>> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> +		return -EINVAL;
>>>> +
>>>> +	dev->timeperframe = parm->parm.capture.timeperframe.denominator ?
>>>
>>> This should check that the fps is between the min and max values as reported by
>>> vidioc_enum_frameintervals(). Fall back to the default if the values are out of
>>> range.
>>
>> {2, 1} is 0.5 fps and {120, 1} is 1/120 fps :) but anyway, why should we
>> set that artificial limits here?
>>
>> Thanks for catching the actual bug, but I propose we set min=1/infty and
>> max=infty so that the user chooses which tpf/fps he/she needs, be it
>> 9000 fps or 1 frame per hour. And imho it's better to clip the value,
>> than to fallback to default (but we still reset it if */0 is coming from
>> userspace).
>>
>> Said all that, here is the interdiff and updated patch.
>>
>> Thanks,
>> Kirill
>>
>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>> index 37d0af8..5d1b374 100644
>> --- a/drivers/media/platform/vivi.c
>> +++ b/drivers/media/platform/vivi.c
>> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>>   /* Global font descriptor */
>>   static const u8 *font8x16;
>>
>> -/* default to NTSC timeperframe */
>> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
>> +/* timeperframe: min/max and default */
>> +static const struct v4l2_fract
>> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
>> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
>
> I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> might hit application errors when they manipulate these values. The shortest time
> between frames is 1 ms anyway.
>
> It's the only comment I have, it looks good otherwise.

As those will be a arbitrary values, I suggest to declare a macro for it at the
beginning of vivi.c file, with some comment explaining the rationale of the choose,
and what else needs to be changed, if this changes (e. g. less than 1ms would require
changing the image generation logic, to avoid producing frames with equal content).

Regards,
Mauro

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
> Em 16-11-2012 11:38, Hans Verkuil escreveu:
> >On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
[...]

> >>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> >>index 37d0af8..5d1b374 100644
> >>--- a/drivers/media/platform/vivi.c
> >>+++ b/drivers/media/platform/vivi.c
> >>@@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >>  /* Global font descriptor */
> >>  static const u8 *font8x16;
> >>
> >>-/* default to NTSC timeperframe */
> >>-static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> >>+/* timeperframe: min/max and default */
> >>+static const struct v4l2_fract
> >>+	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> >>+	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> >
> >I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> >1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> >might hit application errors when they manipulate these values. The shortest time
> >between frames is 1 ms anyway.
> >
> >It's the only comment I have, it looks good otherwise.
> 
> As those will be a arbitrary values, I suggest to declare a macro for it at the
> beginning of vivi.c file, with some comment explaining the rationale of the choose,
> and what else needs to be changed, if this changes (e. g. less than 1ms would require
> changing the image generation logic, to avoid producing frames with equal content).

Maybe something like this? (please note, I'm not a good text writer. If
this needs adjustment please help me shape the text up)


diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 5d1b374..45b8a81 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,6 +36,18 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
+/* Maximum allowed frame rate
+ *
+ * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
+ *
+ * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
+ * might hit application errors when they manipulate these values.
+ *
+ * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
+ * producing frames with equal content.
+ */
+#define FPS_MAX 1000
+
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
 
@@ -67,8 +79,8 @@ static const u8 *font8x16;
 
 /* timeperframe: min/max and default */
 static const struct v4l2_fract
-	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
-	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
+	tpf_min     = {.numerator = 1,		.denominator = FPS_MAX},   /* ~1/infty */
+	tpf_max     = {.numerator = FPS_MAX,	.denominator = 1},         /* ~infty */
 	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
 
 #define dprintk(dev, level, fmt, arg...) \

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Mauro Carvalho Chehab]

Em 17-11-2012 08:45, Kirill Smelkov escreveu:
> On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
>> Em 16-11-2012 11:38, Hans Verkuil escreveu:
>>> On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> [...]
>
>>>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>>>> index 37d0af8..5d1b374 100644
>>>> --- a/drivers/media/platform/vivi.c
>>>> +++ b/drivers/media/platform/vivi.c
>>>> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>>>>   /* Global font descriptor */
>>>>   static const u8 *font8x16;
>>>>
>>>> -/* default to NTSC timeperframe */
>>>> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
>>>> +/* timeperframe: min/max and default */
>>>> +static const struct v4l2_fract
>>>> +	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
>>>> +	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
>>>
>>> I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
>>> 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
>>> might hit application errors when they manipulate these values. The shortest time
>>> between frames is 1 ms anyway.
>>>
>>> It's the only comment I have, it looks good otherwise.
>>
>> As those will be a arbitrary values, I suggest to declare a macro for it at the
>> beginning of vivi.c file, with some comment explaining the rationale of the choose,
>> and what else needs to be changed, if this changes (e. g. less than 1ms would require
>> changing the image generation logic, to avoid producing frames with equal content).
>
> Maybe something like this? (please note, I'm not a good text writer. If
> this needs adjustment please help me shape the text up)
>
>
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 5d1b374..45b8a81 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,6 +36,18 @@
>
>   #define VIVI_MODULE_NAME "vivi"
>
> +/* Maximum allowed frame rate
> + *
> + * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> + *
> + * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> + * might hit application errors when they manipulate these values.
> + *
> + * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> + * producing frames with equal content.
> + */
> +#define FPS_MAX 1000
> +
>   #define MAX_WIDTH 1920
>   #define MAX_HEIGHT 1200
>
> @@ -67,8 +79,8 @@ static const u8 *font8x16;
>
>   /* timeperframe: min/max and default */
>   static const struct v4l2_fract
> -	tpf_min     = {.numerator = 1,		.denominator = UINT_MAX},  /* 1/infty */
> -	tpf_max     = {.numerator = UINT_MAX,	.denominator = 1},         /* infty */
> +	tpf_min     = {.numerator = 1,		.denominator = FPS_MAX},   /* ~1/infty */
> +	tpf_max     = {.numerator = FPS_MAX,	.denominator = 1},         /* ~infty */
>   	tpf_default = {.numerator = 1001,	.denominator = 30000};     /* NTSC */
>
>   #define dprintk(dev, level, fmt, arg...) \

seems OK to me.

Regards,
Mauro

>

Re: [PATCH v4] [media] vivi: Teach it to tune FPS

[Mauro Carvalho Chehab]

Em 18-11-2012 07:24, Mauro Carvalho Chehab escreveu:
> Em 17-11-2012 08:45, Kirill Smelkov escreveu:
>> On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
>>> Em 16-11-2012 11:38, Hans Verkuil escreveu:
>>>> On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
>> [...]
>>
>>>>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>>>>> index 37d0af8..5d1b374 100644
>>>>> --- a/drivers/media/platform/vivi.c
>>>>> +++ b/drivers/media/platform/vivi.c
>>>>> @@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>>>>>   /* Global font descriptor */
>>>>>   static const u8 *font8x16;
>>>>>
>>>>> -/* default to NTSC timeperframe */
>>>>> -static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
>>>>> +/* timeperframe: min/max and default */
>>>>> +static const struct v4l2_fract
>>>>> +    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
>>>>> +    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
>>>>
>>>> I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
>>>> 1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
>>>> might hit application errors when they manipulate these values. The shortest time
>>>> between frames is 1 ms anyway.
>>>>
>>>> It's the only comment I have, it looks good otherwise.
>>>
>>> As those will be a arbitrary values, I suggest to declare a macro for it at the
>>> beginning of vivi.c file, with some comment explaining the rationale of the choose,
>>> and what else needs to be changed, if this changes (e. g. less than 1ms would require
>>> changing the image generation logic, to avoid producing frames with equal content).
>>
>> Maybe something like this? (please note, I'm not a good text writer. If
>> this needs adjustment please help me shape the text up)
>>
>>
>> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
>> index 5d1b374..45b8a81 100644
>> --- a/drivers/media/platform/vivi.c
>> +++ b/drivers/media/platform/vivi.c
>> @@ -36,6 +36,18 @@
>>
>>   #define VIVI_MODULE_NAME "vivi"
>>
>> +/* Maximum allowed frame rate
>> + *
>> + * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
>> + *
>> + * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
>> + * might hit application errors when they manipulate these values.
>> + *
>> + * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
>> + * producing frames with equal content.
>> + */
>> +#define FPS_MAX 1000
>> +
>>   #define MAX_WIDTH 1920
>>   #define MAX_HEIGHT 1200
>>
>> @@ -67,8 +79,8 @@ static const u8 *font8x16;
>>
>>   /* timeperframe: min/max and default */
>>   static const struct v4l2_fract
>> -    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
>> -    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
>> +    tpf_min     = {.numerator = 1,        .denominator = FPS_MAX},   /* ~1/infty */
>> +    tpf_max     = {.numerator = FPS_MAX,    .denominator = 1},         /* ~infty */

Was too fast answering it... The comments there should also be dropped, as it doesn't
range anymore to infty.

>>       tpf_default = {.numerator = 1001,    .denominator = 30000};     /* NTSC */
>>
>>   #define dprintk(dev, level, fmt, arg...) \
>
> seems OK to me.
>
> Regards,
> Mauro
>
>>
>

Re: [PATCH v6] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Sun, Nov 18, 2012 at 07:25:38AM -0200, Mauro Carvalho Chehab wrote:
> Em 18-11-2012 07:24, Mauro Carvalho Chehab escreveu:
> >Em 17-11-2012 08:45, Kirill Smelkov escreveu:
> >>On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
> >>>Em 16-11-2012 11:38, Hans Verkuil escreveu:
> >>>>On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> >>[...]
> >>
> >>>>>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> >>>>>index 37d0af8..5d1b374 100644
> >>>>>--- a/drivers/media/platform/vivi.c
> >>>>>+++ b/drivers/media/platform/vivi.c
> >>>>>@@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >>>>>  /* Global font descriptor */
> >>>>>  static const u8 *font8x16;
> >>>>>
> >>>>>-/* default to NTSC timeperframe */
> >>>>>-static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> >>>>>+/* timeperframe: min/max and default */
> >>>>>+static const struct v4l2_fract
> >>>>>+    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> >>>>>+    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> >>>>
> >>>>I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> >>>>1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> >>>>might hit application errors when they manipulate these values. The shortest time
> >>>>between frames is 1 ms anyway.
> >>>>
> >>>>It's the only comment I have, it looks good otherwise.
> >>>
> >>>As those will be a arbitrary values, I suggest to declare a macro for it at the
> >>>beginning of vivi.c file, with some comment explaining the rationale of the choose,
> >>>and what else needs to be changed, if this changes (e. g. less than 1ms would require
> >>>changing the image generation logic, to avoid producing frames with equal content).
> >>
> >>Maybe something like this? (please note, I'm not a good text writer. If
> >>this needs adjustment please help me shape the text up)
> >>
> >>
> >>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> >>index 5d1b374..45b8a81 100644
> >>--- a/drivers/media/platform/vivi.c
> >>+++ b/drivers/media/platform/vivi.c
> >>@@ -36,6 +36,18 @@
> >>
> >>  #define VIVI_MODULE_NAME "vivi"
> >>
> >>+/* Maximum allowed frame rate
> >>+ *
> >>+ * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> >>+ *
> >>+ * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> >>+ * might hit application errors when they manipulate these values.
> >>+ *
> >>+ * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> >>+ * producing frames with equal content.
> >>+ */
> >>+#define FPS_MAX 1000
> >>+
> >>  #define MAX_WIDTH 1920
> >>  #define MAX_HEIGHT 1200
> >>
> >>@@ -67,8 +79,8 @@ static const u8 *font8x16;
> >>
> >>  /* timeperframe: min/max and default */
> >>  static const struct v4l2_fract
> >>-    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> >>-    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> >>+    tpf_min     = {.numerator = 1,        .denominator = FPS_MAX},   /* ~1/infty */
> >>+    tpf_max     = {.numerator = FPS_MAX,    .denominator = 1},         /* ~infty */
> 
> Was too fast answering it... The comments there should also be dropped, as it doesn't
> range anymore to infty.

Ok, agree, let's drop those ~infty comments and be done with it.


---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Tue, 23 Oct 2012 16:56:59 +0400
Subject: [PATCH v6] [media] vivi: Teach it to tune FPS

I was testing my video-over-ethernet subsystem recently, and vivi
seemed to be perfect video source for testing when one don't have lots
of capture boards and cameras. Only its framerate was hardcoded to
NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
needed that to precisely simulate bandwidth.

That's why here is this patch with ->enum_frameintervals() and
->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.

Regarding newly introduced __get_format(u32 pixelformat) I decided not
to convert original get_format() to operate on fourcc codes, since >= 3
places in driver need to deal with v4l2_format and otherwise it won't be
handy.

Thanks.

Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/media/platform/vivi.c | 102 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 94 insertions(+), 8 deletions(-)

V6:
    - Moved TPF/FPS limit to beginning of vivi.c and added comment there
      why that limit is used, to avoid overlows, as suggested by Mauro
      Carvalho Chehab.

V5:
    - changed  1/infty - infty/1  limits to  1/1000 - 1000/1, to avoid
      hitting aplication errors when they try to manipulato those
      values, as suggested by Hans Verkuil.

V4:
    - corrected fival->stepwise setting and added its check to s_parm();
      also cosmetics - all as per Hans Verkuil review.

V3:
    - corrected issues with V4L2 spec compliance as pointed by Hans
      Verkuil.

V2:

    - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
      by Hans Verkuil.


diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 6e6dd25..9ca920e 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -36,9 +36,17 @@
 
 #define VIVI_MODULE_NAME "vivi"
 
-/* Wake up at about 30 fps */
-#define WAKE_NUMERATOR 30
-#define WAKE_DENOMINATOR 1001
+/* Maximum allowed frame rate
+ *
+ * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
+ *
+ * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
+ * might hit application errors when they manipulate these values.
+ *
+ * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
+ * producing frames with equal content.
+ */
+#define FPS_MAX 1000
 
 #define MAX_WIDTH 1920
 #define MAX_HEIGHT 1200
@@ -69,6 +77,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
 /* Global font descriptor */
 static const u8 *font8x16;
 
+/* timeperframe: min/max and default */
+static const struct v4l2_fract
+	tpf_min     = {.numerator = 1,		.denominator = FPS_MAX},
+	tpf_max     = {.numerator = FPS_MAX,	.denominator = 1},
+	tpf_default = {.numerator = 1001,	.denominator = 30000};	/* NTSC */
+
 #define dprintk(dev, level, fmt, arg...) \
 	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
 
@@ -150,14 +164,14 @@ static struct vivi_fmt formats[] = {
 	},
 };
 
-static struct vivi_fmt *get_format(struct v4l2_format *f)
+static struct vivi_fmt *__get_format(u32 pixelformat)
 {
 	struct vivi_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < ARRAY_SIZE(formats); k++) {
 		fmt = &formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == pixelformat)
 			break;
 	}
 
@@ -167,6 +181,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
 	return &formats[k];
 }
 
+static struct vivi_fmt *get_format(struct v4l2_format *f)
+{
+	return __get_format(f->fmt.pix.pixelformat);
+}
+
 /* buffer for one video frame */
 struct vivi_buffer {
 	/* common v4l buffer stuff -- must be first */
@@ -232,6 +251,7 @@ struct vivi_dev {
 
 	/* video capture */
 	struct vivi_fmt            *fmt;
+	struct v4l2_fract          timeperframe;
 	unsigned int               width, height;
 	struct vb2_queue	   vb_vidq;
 	unsigned int		   field_count;
@@ -691,8 +711,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
 
-#define frames_to_ms(frames)					\
-	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
+#define frames_to_ms(dev, frames)				\
+	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
 
 static void vivi_sleep(struct vivi_dev *dev)
 {
@@ -708,7 +728,7 @@ static void vivi_sleep(struct vivi_dev *dev)
 		goto stop_task;
 
 	/* Calculate time to wake up */
-	timeout = msecs_to_jiffies(frames_to_ms(1));
+	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
 
 	vivi_thread_tick(dev);
 
@@ -1080,6 +1100,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	return 0;
 }
 
+/* timeperframe is arbitrary and continous */
+static int vidioc_enum_frameintervals(struct file *file, void *priv,
+					     struct v4l2_frmivalenum *fival)
+{
+	struct vivi_fmt *fmt;
+
+	if (fival->index)
+		return -EINVAL;
+
+	fmt = __get_format(fival->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	/* regarding width & height - we support any */
+
+	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+
+	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
+	fival->stepwise.min  = tpf_min;
+	fival->stepwise.max  = tpf_max;
+	fival->stepwise.step = (struct v4l2_fract) {1, 1};
+
+	return 0;
+}
+
+static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
+	parm->parm.capture.timeperframe = dev->timeperframe;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
+#define FRACT_CMP(a, OP, b)	\
+	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
+
+static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+	struct v4l2_fract tpf;
+
+	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	tpf = parm->parm.capture.timeperframe;
+
+	/* tpf: {*, 0} resets timing; clip to [min, max]*/
+	tpf = tpf.denominator ? tpf : tpf_default;
+	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+
+	dev->timeperframe = tpf;
+	parm->parm.capture.timeperframe = tpf;
+	parm->parm.capture.readbuffers  = 1;
+	return 0;
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1238,6 +1320,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
 	.vidioc_s_input       = vidioc_s_input,
+	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
+	.vidioc_g_parm        = vidioc_g_parm,
+	.vidioc_s_parm        = vidioc_s_parm,
 	.vidioc_streamon      = vb2_ioctl_streamon,
 	.vidioc_streamoff     = vb2_ioctl_streamoff,
 	.vidioc_log_status    = v4l2_ctrl_log_status,
@@ -1296,6 +1381,7 @@ static int __init vivi_create_instance(int inst)
 		goto free_dev;
 
 	dev->fmt = &formats[0];
+	dev->timeperframe = tpf_default;
 	dev->width = 640;
 	dev->height = 480;
 	dev->pixelsize = dev->fmt->depth / 8;
-- 
1.8.0.289.g7a667bc

Re: [PATCH v6] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Mon, Nov 19, 2012 at 09:52:36AM +0400, Kirill Smelkov wrote:
> On Sun, Nov 18, 2012 at 07:25:38AM -0200, Mauro Carvalho Chehab wrote:
> > Em 18-11-2012 07:24, Mauro Carvalho Chehab escreveu:
> > >Em 17-11-2012 08:45, Kirill Smelkov escreveu:
> > >>On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
> > >>>Em 16-11-2012 11:38, Hans Verkuil escreveu:
> > >>>>On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> > >>[...]
> > >>
> > >>>>>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > >>>>>index 37d0af8..5d1b374 100644
> > >>>>>--- a/drivers/media/platform/vivi.c
> > >>>>>+++ b/drivers/media/platform/vivi.c
> > >>>>>@@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > >>>>>  /* Global font descriptor */
> > >>>>>  static const u8 *font8x16;
> > >>>>>
> > >>>>>-/* default to NTSC timeperframe */
> > >>>>>-static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > >>>>>+/* timeperframe: min/max and default */
> > >>>>>+static const struct v4l2_fract
> > >>>>>+    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> > >>>>>+    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> > >>>>
> > >>>>I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> > >>>>1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> > >>>>might hit application errors when they manipulate these values. The shortest time
> > >>>>between frames is 1 ms anyway.
> > >>>>
> > >>>>It's the only comment I have, it looks good otherwise.
> > >>>
> > >>>As those will be a arbitrary values, I suggest to declare a macro for it at the
> > >>>beginning of vivi.c file, with some comment explaining the rationale of the choose,
> > >>>and what else needs to be changed, if this changes (e. g. less than 1ms would require
> > >>>changing the image generation logic, to avoid producing frames with equal content).
> > >>
> > >>Maybe something like this? (please note, I'm not a good text writer. If
> > >>this needs adjustment please help me shape the text up)
> > >>
> > >>
> > >>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > >>index 5d1b374..45b8a81 100644
> > >>--- a/drivers/media/platform/vivi.c
> > >>+++ b/drivers/media/platform/vivi.c
> > >>@@ -36,6 +36,18 @@
> > >>
> > >>  #define VIVI_MODULE_NAME "vivi"
> > >>
> > >>+/* Maximum allowed frame rate
> > >>+ *
> > >>+ * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> > >>+ *
> > >>+ * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> > >>+ * might hit application errors when they manipulate these values.
> > >>+ *
> > >>+ * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> > >>+ * producing frames with equal content.
> > >>+ */
> > >>+#define FPS_MAX 1000
> > >>+
> > >>  #define MAX_WIDTH 1920
> > >>  #define MAX_HEIGHT 1200
> > >>
> > >>@@ -67,8 +79,8 @@ static const u8 *font8x16;
> > >>
> > >>  /* timeperframe: min/max and default */
> > >>  static const struct v4l2_fract
> > >>-    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> > >>-    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> > >>+    tpf_min     = {.numerator = 1,        .denominator = FPS_MAX},   /* ~1/infty */
> > >>+    tpf_max     = {.numerator = FPS_MAX,    .denominator = 1},         /* ~infty */
> > 
> > Was too fast answering it... The comments there should also be dropped, as it doesn't
> > range anymore to infty.
> 
> Ok, agree, let's drop those ~infty comments and be done with it.
> 
> 
> ---- 8< ----
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 23 Oct 2012 16:56:59 +0400
> Subject: [PATCH v6] [media] vivi: Teach it to tune FPS
> 
> I was testing my video-over-ethernet subsystem recently, and vivi
> seemed to be perfect video source for testing when one don't have lots
> of capture boards and cameras. Only its framerate was hardcoded to
> NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> needed that to precisely simulate bandwidth.
> 
> That's why here is this patch with ->enum_frameintervals() and
> ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> 
> Regarding newly introduced __get_format(u32 pixelformat) I decided not
> to convert original get_format() to operate on fourcc codes, since >= 3
> places in driver need to deal with v4l2_format and otherwise it won't be
> handy.
> 
> Thanks.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/media/platform/vivi.c | 102 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 94 insertions(+), 8 deletions(-)
> 
> V6:
>     - Moved TPF/FPS limit to beginning of vivi.c and added comment there
>       why that limit is used, to avoid overlows, as suggested by Mauro
>       Carvalho Chehab.
> 
> V5:
>     - changed  1/infty - infty/1  limits to  1/1000 - 1000/1, to avoid
>       hitting aplication errors when they try to manipulato those
>       values, as suggested by Hans Verkuil.
> 
> V4:
>     - corrected fival->stepwise setting and added its check to s_parm();
>       also cosmetics - all as per Hans Verkuil review.
> 
> V3:
>     - corrected issues with V4L2 spec compliance as pointed by Hans
>       Verkuil.
> 
> V2:
> 
>     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
>       by Hans Verkuil.
> 
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index 6e6dd25..9ca920e 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -36,9 +36,17 @@
>  
>  #define VIVI_MODULE_NAME "vivi"
>  
> -/* Wake up at about 30 fps */
> -#define WAKE_NUMERATOR 30
> -#define WAKE_DENOMINATOR 1001
> +/* Maximum allowed frame rate
> + *
> + * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> + *
> + * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> + * might hit application errors when they manipulate these values.
> + *
> + * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> + * producing frames with equal content.
> + */
> +#define FPS_MAX 1000
>  
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
> @@ -69,6 +77,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
>  /* Global font descriptor */
>  static const u8 *font8x16;
>  
> +/* timeperframe: min/max and default */
> +static const struct v4l2_fract
> +	tpf_min     = {.numerator = 1,		.denominator = FPS_MAX},
> +	tpf_max     = {.numerator = FPS_MAX,	.denominator = 1},
> +	tpf_default = {.numerator = 1001,	.denominator = 30000};	/* NTSC */
> +
>  #define dprintk(dev, level, fmt, arg...) \
>  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>  
> @@ -150,14 +164,14 @@ static struct vivi_fmt formats[] = {
>  	},
>  };
>  
> -static struct vivi_fmt *get_format(struct v4l2_format *f)
> +static struct vivi_fmt *__get_format(u32 pixelformat)
>  {
>  	struct vivi_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -167,6 +181,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
>  	return &formats[k];
>  }
>  
> +static struct vivi_fmt *get_format(struct v4l2_format *f)
> +{
> +	return __get_format(f->fmt.pix.pixelformat);
> +}
> +
>  /* buffer for one video frame */
>  struct vivi_buffer {
>  	/* common v4l buffer stuff -- must be first */
> @@ -232,6 +251,7 @@ struct vivi_dev {
>  
>  	/* video capture */
>  	struct vivi_fmt            *fmt;
> +	struct v4l2_fract          timeperframe;
>  	unsigned int               width, height;
>  	struct vb2_queue	   vb_vidq;
>  	unsigned int		   field_count;
> @@ -691,8 +711,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
>  
> -#define frames_to_ms(frames)					\
> -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> +#define frames_to_ms(dev, frames)				\
> +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
>  
>  static void vivi_sleep(struct vivi_dev *dev)
>  {
> @@ -708,7 +728,7 @@ static void vivi_sleep(struct vivi_dev *dev)
>  		goto stop_task;
>  
>  	/* Calculate time to wake up */
> -	timeout = msecs_to_jiffies(frames_to_ms(1));
> +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
>  
>  	vivi_thread_tick(dev);
>  
> @@ -1080,6 +1100,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  	return 0;
>  }
>  
> +/* timeperframe is arbitrary and continous */
> +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> +					     struct v4l2_frmivalenum *fival)
> +{
> +	struct vivi_fmt *fmt;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = __get_format(fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* regarding width & height - we support any */
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> +
> +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> +	fival->stepwise.min  = tpf_min;
> +	fival->stepwise.max  = tpf_max;
> +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> +	parm->parm.capture.timeperframe = dev->timeperframe;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
> +#define FRACT_CMP(a, OP, b)	\
> +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> +
> +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> +{
> +	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fract tpf;
> +
> +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	tpf = parm->parm.capture.timeperframe;
> +
> +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> +	tpf = tpf.denominator ? tpf : tpf_default;
> +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> +
> +	dev->timeperframe = tpf;
> +	parm->parm.capture.timeperframe = tpf;
> +	parm->parm.capture.readbuffers  = 1;
> +	return 0;
> +}
> +
>  /* --- controls ---------------------------------------------- */
>  
>  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1238,6 +1320,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_enum_input    = vidioc_enum_input,
>  	.vidioc_g_input       = vidioc_g_input,
>  	.vidioc_s_input       = vidioc_s_input,
> +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> +	.vidioc_g_parm        = vidioc_g_parm,
> +	.vidioc_s_parm        = vidioc_s_parm,
>  	.vidioc_streamon      = vb2_ioctl_streamon,
>  	.vidioc_streamoff     = vb2_ioctl_streamoff,
>  	.vidioc_log_status    = v4l2_ctrl_log_status,
> @@ -1296,6 +1381,7 @@ static int __init vivi_create_instance(int inst)
>  		goto free_dev;
>  
>  	dev->fmt = &formats[0];
> +	dev->timeperframe = tpf_default;
>  	dev->width = 640;
>  	dev->height = 480;
>  	dev->pixelsize = dev->fmt->depth / 8;


Mauro, Hans, what's the state of this patch? Just a ping, I know you are
under load.

Thanks,
Kirill


P.S. Hans, if this is ok for you, would you please add your ack?

Re: [PATCH v6] [media] vivi: Teach it to tune FPS

[Hans Verkuil]

On Fri November 30 2012 12:02:14 Kirill Smelkov wrote:
> On Mon, Nov 19, 2012 at 09:52:36AM +0400, Kirill Smelkov wrote:
> > On Sun, Nov 18, 2012 at 07:25:38AM -0200, Mauro Carvalho Chehab wrote:
> > > Em 18-11-2012 07:24, Mauro Carvalho Chehab escreveu:
> > > >Em 17-11-2012 08:45, Kirill Smelkov escreveu:
> > > >>On Fri, Nov 16, 2012 at 01:46:58PM -0200, Mauro Carvalho Chehab wrote:
> > > >>>Em 16-11-2012 11:38, Hans Verkuil escreveu:
> > > >>>>On Wed November 7 2012 12:30:01 Kirill Smelkov wrote:
> > > >>[...]
> > > >>
> > > >>>>>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > >>>>>index 37d0af8..5d1b374 100644
> > > >>>>>--- a/drivers/media/platform/vivi.c
> > > >>>>>+++ b/drivers/media/platform/vivi.c
> > > >>>>>@@ -65,8 +65,11 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> > > >>>>>  /* Global font descriptor */
> > > >>>>>  static const u8 *font8x16;
> > > >>>>>
> > > >>>>>-/* default to NTSC timeperframe */
> > > >>>>>-static const struct v4l2_fract TPF_DEFAULT = {.numerator = 1001, .denominator = 30000};
> > > >>>>>+/* timeperframe: min/max and default */
> > > >>>>>+static const struct v4l2_fract
> > > >>>>>+    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> > > >>>>>+    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> > > >>>>
> > > >>>>I understand your reasoning here, but I wouldn't go with UINT_MAX here. Something like
> > > >>>>1/1000 tpf (or 1 ms) up to 86400/1 tpf (or once a day). With UINT_MAX I am afraid we
> > > >>>>might hit application errors when they manipulate these values. The shortest time
> > > >>>>between frames is 1 ms anyway.
> > > >>>>
> > > >>>>It's the only comment I have, it looks good otherwise.
> > > >>>
> > > >>>As those will be a arbitrary values, I suggest to declare a macro for it at the
> > > >>>beginning of vivi.c file, with some comment explaining the rationale of the choose,
> > > >>>and what else needs to be changed, if this changes (e. g. less than 1ms would require
> > > >>>changing the image generation logic, to avoid producing frames with equal content).
> > > >>
> > > >>Maybe something like this? (please note, I'm not a good text writer. If
> > > >>this needs adjustment please help me shape the text up)
> > > >>
> > > >>
> > > >>diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > > >>index 5d1b374..45b8a81 100644
> > > >>--- a/drivers/media/platform/vivi.c
> > > >>+++ b/drivers/media/platform/vivi.c
> > > >>@@ -36,6 +36,18 @@
> > > >>
> > > >>  #define VIVI_MODULE_NAME "vivi"
> > > >>
> > > >>+/* Maximum allowed frame rate
> > > >>+ *
> > > >>+ * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> > > >>+ *
> > > >>+ * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> > > >>+ * might hit application errors when they manipulate these values.
> > > >>+ *
> > > >>+ * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> > > >>+ * producing frames with equal content.
> > > >>+ */
> > > >>+#define FPS_MAX 1000
> > > >>+
> > > >>  #define MAX_WIDTH 1920
> > > >>  #define MAX_HEIGHT 1200
> > > >>
> > > >>@@ -67,8 +79,8 @@ static const u8 *font8x16;
> > > >>
> > > >>  /* timeperframe: min/max and default */
> > > >>  static const struct v4l2_fract
> > > >>-    tpf_min     = {.numerator = 1,        .denominator = UINT_MAX},  /* 1/infty */
> > > >>-    tpf_max     = {.numerator = UINT_MAX,    .denominator = 1},         /* infty */
> > > >>+    tpf_min     = {.numerator = 1,        .denominator = FPS_MAX},   /* ~1/infty */
> > > >>+    tpf_max     = {.numerator = FPS_MAX,    .denominator = 1},         /* ~infty */
> > > 
> > > Was too fast answering it... The comments there should also be dropped, as it doesn't
> > > range anymore to infty.
> > 
> > Ok, agree, let's drop those ~infty comments and be done with it.
> > 
> > 
> > ---- 8< ----
> > From: Kirill Smelkov <kirr@mns.spb.ru>
> > Date: Tue, 23 Oct 2012 16:56:59 +0400
> > Subject: [PATCH v6] [media] vivi: Teach it to tune FPS
> > 
> > I was testing my video-over-ethernet subsystem recently, and vivi
> > seemed to be perfect video source for testing when one don't have lots
> > of capture boards and cameras. Only its framerate was hardcoded to
> > NTSC's 30fps, while in my country we usually use PAL (25 fps) and I
> > needed that to precisely simulate bandwidth.
> > 
> > That's why here is this patch with ->enum_frameintervals() and
> > ->{g,s}_parm() implemented as suggested by Hans Verkuil which passes
> > v4l2-compliance and manual testing through v4l2-ctl -P / -p <fps>.
> > 
> > Regarding newly introduced __get_format(u32 pixelformat) I decided not
> > to convert original get_format() to operate on fourcc codes, since >= 3
> > places in driver need to deal with v4l2_format and otherwise it won't be
> > handy.
> > 
> > Thanks.
> > 
> > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > ---
> >  drivers/media/platform/vivi.c | 102 ++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 94 insertions(+), 8 deletions(-)
> > 
> > V6:
> >     - Moved TPF/FPS limit to beginning of vivi.c and added comment there
> >       why that limit is used, to avoid overlows, as suggested by Mauro
> >       Carvalho Chehab.
> > 
> > V5:
> >     - changed  1/infty - infty/1  limits to  1/1000 - 1000/1, to avoid
> >       hitting aplication errors when they try to manipulato those
> >       values, as suggested by Hans Verkuil.
> > 
> > V4:
> >     - corrected fival->stepwise setting and added its check to s_parm();
> >       also cosmetics - all as per Hans Verkuil review.
> > 
> > V3:
> >     - corrected issues with V4L2 spec compliance as pointed by Hans
> >       Verkuil.
> > 
> > V2:
> > 
> >     - reworked FPS setting from module param to via ->{g,s}_parm() as suggested
> >       by Hans Verkuil.
> > 
> > 
> > diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> > index 6e6dd25..9ca920e 100644
> > --- a/drivers/media/platform/vivi.c
> > +++ b/drivers/media/platform/vivi.c
> > @@ -36,9 +36,17 @@
> >  
> >  #define VIVI_MODULE_NAME "vivi"
> >  
> > -/* Wake up at about 30 fps */
> > -#define WAKE_NUMERATOR 30
> > -#define WAKE_DENOMINATOR 1001
> > +/* Maximum allowed frame rate
> > + *
> > + * Vivi will allow setting timeperframe in [1/FPS_MAX - FPS_MAX/1] range.
> > + *
> > + * Ideally FPS_MAX should be infinity, i.e. practically UINT_MAX, but that
> > + * might hit application errors when they manipulate these values.
> > + *
> > + * Besides, for tpf < 1ms image-generation logic should be changed, to avoid
> > + * producing frames with equal content.
> > + */
> > +#define FPS_MAX 1000
> >  
> >  #define MAX_WIDTH 1920
> >  #define MAX_HEIGHT 1200
> > @@ -69,6 +77,12 @@ MODULE_PARM_DESC(vid_limit, "capture memory limit in megabytes");
> >  /* Global font descriptor */
> >  static const u8 *font8x16;
> >  
> > +/* timeperframe: min/max and default */
> > +static const struct v4l2_fract
> > +	tpf_min     = {.numerator = 1,		.denominator = FPS_MAX},
> > +	tpf_max     = {.numerator = FPS_MAX,	.denominator = 1},
> > +	tpf_default = {.numerator = 1001,	.denominator = 30000};	/* NTSC */
> > +
> >  #define dprintk(dev, level, fmt, arg...) \
> >  	v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
> >  
> > @@ -150,14 +164,14 @@ static struct vivi_fmt formats[] = {
> >  	},
> >  };
> >  
> > -static struct vivi_fmt *get_format(struct v4l2_format *f)
> > +static struct vivi_fmt *__get_format(u32 pixelformat)
> >  {
> >  	struct vivi_fmt *fmt;
> >  	unsigned int k;
> >  
> >  	for (k = 0; k < ARRAY_SIZE(formats); k++) {
> >  		fmt = &formats[k];
> > -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> > +		if (fmt->fourcc == pixelformat)
> >  			break;
> >  	}
> >  
> > @@ -167,6 +181,11 @@ static struct vivi_fmt *get_format(struct v4l2_format *f)
> >  	return &formats[k];
> >  }
> >  
> > +static struct vivi_fmt *get_format(struct v4l2_format *f)
> > +{
> > +	return __get_format(f->fmt.pix.pixelformat);
> > +}
> > +
> >  /* buffer for one video frame */
> >  struct vivi_buffer {
> >  	/* common v4l buffer stuff -- must be first */
> > @@ -232,6 +251,7 @@ struct vivi_dev {
> >  
> >  	/* video capture */
> >  	struct vivi_fmt            *fmt;
> > +	struct v4l2_fract          timeperframe;
> >  	unsigned int               width, height;
> >  	struct vb2_queue	   vb_vidq;
> >  	unsigned int		   field_count;
> > @@ -691,8 +711,8 @@ static void vivi_thread_tick(struct vivi_dev *dev)
> >  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
> >  }
> >  
> > -#define frames_to_ms(frames)					\
> > -	((frames * WAKE_NUMERATOR * 1000) / WAKE_DENOMINATOR)
> > +#define frames_to_ms(dev, frames)				\
> > +	((frames * dev->timeperframe.numerator * 1000) / dev->timeperframe.denominator)
> >  
> >  static void vivi_sleep(struct vivi_dev *dev)
> >  {
> > @@ -708,7 +728,7 @@ static void vivi_sleep(struct vivi_dev *dev)
> >  		goto stop_task;
> >  
> >  	/* Calculate time to wake up */
> > -	timeout = msecs_to_jiffies(frames_to_ms(1));
> > +	timeout = msecs_to_jiffies(frames_to_ms(dev, 1));
> >  
> >  	vivi_thread_tick(dev);
> >  
> > @@ -1080,6 +1100,68 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
> >  	return 0;
> >  }
> >  
> > +/* timeperframe is arbitrary and continous */
> > +static int vidioc_enum_frameintervals(struct file *file, void *priv,
> > +					     struct v4l2_frmivalenum *fival)
> > +{
> > +	struct vivi_fmt *fmt;
> > +
> > +	if (fival->index)
> > +		return -EINVAL;
> > +
> > +	fmt = __get_format(fival->pixel_format);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	/* regarding width & height - we support any */
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
> > +
> > +	/* fill in stepwise (step=1.0 is requred by V4L2 spec) */
> > +	fival->stepwise.min  = tpf_min;
> > +	fival->stepwise.max  = tpf_max;
> > +	fival->stepwise.step = (struct v4l2_fract) {1, 1};
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_g_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	parm->parm.capture.capability   = V4L2_CAP_TIMEPERFRAME;
> > +	parm->parm.capture.timeperframe = dev->timeperframe;
> > +	parm->parm.capture.readbuffers  = 1;
> > +	return 0;
> > +}
> > +
> > +#define FRACT_CMP(a, OP, b)	\
> > +	( (u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator )
> > +
> > +static int vidioc_s_parm(struct file *file, void *priv, struct v4l2_streamparm *parm)
> > +{
> > +	struct vivi_dev *dev = video_drvdata(file);
> > +	struct v4l2_fract tpf;
> > +
> > +	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> > +
> > +	tpf = parm->parm.capture.timeperframe;
> > +
> > +	/* tpf: {*, 0} resets timing; clip to [min, max]*/
> > +	tpf = tpf.denominator ? tpf : tpf_default;
> > +	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
> > +	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
> > +
> > +	dev->timeperframe = tpf;
> > +	parm->parm.capture.timeperframe = tpf;
> > +	parm->parm.capture.readbuffers  = 1;
> > +	return 0;
> > +}
> > +
> >  /* --- controls ---------------------------------------------- */
> >  
> >  static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -1238,6 +1320,9 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
> >  	.vidioc_enum_input    = vidioc_enum_input,
> >  	.vidioc_g_input       = vidioc_g_input,
> >  	.vidioc_s_input       = vidioc_s_input,
> > +	.vidioc_enum_frameintervals = vidioc_enum_frameintervals,
> > +	.vidioc_g_parm        = vidioc_g_parm,
> > +	.vidioc_s_parm        = vidioc_s_parm,
> >  	.vidioc_streamon      = vb2_ioctl_streamon,
> >  	.vidioc_streamoff     = vb2_ioctl_streamoff,
> >  	.vidioc_log_status    = v4l2_ctrl_log_status,
> > @@ -1296,6 +1381,7 @@ static int __init vivi_create_instance(int inst)
> >  		goto free_dev;
> >  
> >  	dev->fmt = &formats[0];
> > +	dev->timeperframe = tpf_default;
> >  	dev->width = 640;
> >  	dev->height = 480;
> >  	dev->pixelsize = dev->fmt->depth / 8;
> 
> 
> Mauro, Hans, what's the state of this patch? Just a ping, I know you are
> under load.
> 
> Thanks,
> Kirill
> 
> 
> P.S. Hans, if this is ok for you, would you please add your ack?
> 

Looks good to me!

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks,

	Hans

Re: [PATCH v6] [media] vivi: Teach it to tune FPS

[Kirill Smelkov]

On Fri, Nov 30, 2012 at 12:10:59PM +0100, Hans Verkuil wrote:
> On Fri November 30 2012 12:02:14 Kirill Smelkov wrote:

[...]
> > P.S. Hans, if this is ok for you, would you please add your ack?
> > 
> 
> Looks good to me!
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

Re: [PATCH 1/2] [media] vivi: Kill BUFFER_TIMEOUT macro

[Hans Verkuil]

On Mon October 22 2012 15:54:43 Kirill Smelkov wrote:
> Usage of BUFFER_TIMEOUT has gone in 2008 in 78718e5d (V4L/DVB (7492):
> vivi: Simplify the vivi driver and avoid deadlocks), but the macro
> remains. Say goodbye to it.
> 
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/platform/vivi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
> index b366b05..3e6902a 100644
> --- a/drivers/media/platform/vivi.c
> +++ b/drivers/media/platform/vivi.c
> @@ -39,7 +39,6 @@
>  /* Wake up at about 30 fps */
>  #define WAKE_NUMERATOR 30
>  #define WAKE_DENOMINATOR 1001
> -#define BUFFER_TIMEOUT     msecs_to_jiffies(500)  /* 0.5 seconds */
>  
>  #define MAX_WIDTH 1920
>  #define MAX_HEIGHT 1200
>