[PATCH 1/2] tw686x: use a formula instead of two tables for div

[PATCH 1/2] tw686x: use a formula instead of two tables for div

[Ezequiel Garcia]

From: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Instead of using two tables to estimate the temporal decimation
factor, use a formula. This allows to get the closest fps, which
sounds better than the current tables.

Also, the current code doesn't store the real framerate.

This patch fixes the above issues.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
[Ezequiel:
  - introduce a TW686X_MAX_FPS macro for max_fps.
  - use hweight_long instead of open coding the set bits count.]
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
Mauro,

I'm resending your patch with a cosmetic modifications.
Let me know how it looks.

 drivers/media/pci/tw686x/tw686x-video.c | 95 +++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 34 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 0e839f617e03..3131f9305313 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -28,6 +28,7 @@
 #define TW686X_INPUTS_PER_CH		4
 #define TW686X_VIDEO_WIDTH		720
 #define TW686X_VIDEO_HEIGHT(id)		((id & V4L2_STD_525_60) ? 480 : 576)
+#define TW686X_MAX_FPS(id)		((id & V4L2_STD_525_60) ? 30 : 25)
 
 #define TW686X_MAX_SG_ENTRY_SIZE	4096
 #define TW686X_MAX_SG_DESC_COUNT	256 /* PAL 720x576 needs 203 4-KB pages */
@@ -369,53 +370,79 @@ const struct tw686x_dma_ops sg_dma_ops = {
 	.field		= V4L2_FIELD_SEQ_TB,
 };
 
-static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
+static const unsigned int fps_map[15] = {
+	/*
+	 * bit 31 enables selecting the field control register
+	 * bits 0-29 are a bitmask with fields that will be output.
+	 * For NTSC (and PAL-M, PAL-60), all 30 bits are used.
+	 * For other PAL standards, only the first 25 bits are used.
+	 */
+	0x00000000, /* output all fields */
+	0x80000006, /* 2 fps (60Hz), 2 fps (50Hz) */
+	0x80018006, /* 4 fps (60Hz), 4 fps (50Hz) */
+	0x80618006, /* 6 fps (60Hz), 6 fps (50Hz) */
+	0x81818186, /* 8 fps (60Hz), 8 fps (50Hz) */
+	0x86186186, /* 10 fps (60Hz), 8 fps (50Hz) */
+	0x86619866, /* 12 fps (60Hz), 10 fps (50Hz) */
+	0x86666666, /* 14 fps (60Hz), 12 fps (50Hz) */
+	0x9999999e, /* 16 fps (60Hz), 14 fps (50Hz) */
+	0x99e6799e, /* 18 fps (60Hz), 16 fps (50Hz) */
+	0x9e79e79e, /* 20 fps (60Hz), 16 fps (50Hz) */
+	0x9e7e7e7e, /* 22 fps (60Hz), 18 fps (50Hz) */
+	0x9fe7f9fe, /* 24 fps (60Hz), 20 fps (50Hz) */
+	0x9ffe7ffe, /* 26 fps (60Hz), 22 fps (50Hz) */
+	0x9ffffffe, /* 28 fps (60Hz), 24 fps (50Hz) */
+};
+
+static unsigned int tw686x_real_fps(unsigned int index, unsigned int max_fps)
 {
-	static const unsigned int map[15] = {
-		0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
-		0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
-		0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
-	};
-
-	static const unsigned int std_625_50[26] = {
-		0, 1, 1, 2,  3,  3,  4,  4,  5,  5,  6,  7,  7,
-		   8, 8, 9, 10, 10, 11, 11, 12, 13, 13, 14, 14, 0
-	};
-
-	static const unsigned int std_525_60[31] = {
-		0, 1, 1, 1, 2,  2,  3,  3,  4,  4,  5,  5,  6,  6, 7, 7,
-		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
-	};
+	unsigned long mask;
 
-	unsigned int i;
+	if (!index || index >= ARRAY_SIZE(fps_map))
+		return max_fps;
 
-	if (std & V4L2_STD_525_60) {
-		if (fps >= ARRAY_SIZE(std_525_60))
-			fps = 30;
-		i = std_525_60[fps];
-	} else {
-		if (fps >= ARRAY_SIZE(std_625_50))
-			fps = 25;
-		i = std_625_50[fps];
-	}
+	mask = GENMASK(max_fps - 1, 0);
+	return hweight_long(fps_map[index] & mask);
+}
+
+static unsigned int tw686x_fps_idx(unsigned int fps, unsigned int max_fps)
+{
+	unsigned int idx, real_fps;
+	int delta;
+
+	/* First guess */
+	idx = (12 + 15 * fps) / max_fps;
+
+	/* Minimal possible framerate is 2 frames per second */
+	if (!idx)
+		return 1;
+
+	/* Check if the difference is bigger than abs(1) and adjust */
+	real_fps = tw686x_real_fps(idx, max_fps);
+	delta = real_fps - fps;
+	if (delta < -1)
+		idx++;
+	else if (delta > 1)
+		idx--;
+
+	/* Max framerate */
+	if (idx >= 15)
+		return 0;
 
-	return map[i];
+	return idx;
 }
 
 static void tw686x_set_framerate(struct tw686x_video_channel *vc,
 				 unsigned int fps)
 {
-	unsigned int map;
+	unsigned int i;
 
 	if (vc->fps == fps)
 		return;
 
-	map = tw686x_fields_map(vc->video_standard, fps) << 1;
-	map |= map << 1;
-	if (map > 0)
-		map |= BIT(31);
-	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], map);
-	vc->fps = fps;
+	i = tw686x_fps_idx(fps, TW686X_MAX_FPS(vc->video_standard));
+	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
+	vc->fps = tw686x_real_fps(i, TW686X_MAX_FPS(vc->video_standard));
 }
 
 static const struct tw686x_format *format_by_fourcc(unsigned int fourcc)
-- 
2.9.0

[PATCH 2/2] tw686x: Support VIDIOC_{S,G}_PARM ioctls

[Ezequiel Garcia]

Now that the frame rate can be properly set, this commit adds support
for S_PARM and G_PARM.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/media/pci/tw686x/tw686x-video.c | 46 ++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 3131f9305313..40b5b835d452 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -437,9 +437,6 @@ static void tw686x_set_framerate(struct tw686x_video_channel *vc,
 {
 	unsigned int i;
 
-	if (vc->fps == fps)
-		return;
-
 	i = tw686x_fps_idx(fps, TW686X_MAX_FPS(vc->video_standard));
 	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
 	vc->fps = tw686x_real_fps(i, TW686X_MAX_FPS(vc->video_standard));
@@ -843,6 +840,12 @@ static int tw686x_s_std(struct file *file, void *priv, v4l2_std_id id)
 	ret = tw686x_g_fmt_vid_cap(file, priv, &f);
 	if (!ret)
 		tw686x_s_fmt_vid_cap(file, priv, &f);
+
+	/*
+	 * Frame decimation depends on the chosen standard,
+	 * so reset it to the current value.
+	 */
+	tw686x_set_framerate(vc, vc->fps);
 	return 0;
 }
 
@@ -912,6 +915,40 @@ static int tw686x_g_std(struct file *file, void *priv, v4l2_std_id *id)
 	return 0;
 }
 
+static int tw686x_g_parm(struct file *file, void *priv,
+			 struct v4l2_streamparm *sp)
+{
+	struct tw686x_video_channel *vc = video_drvdata(file);
+	struct v4l2_captureparm *cp = &sp->parm.capture;
+
+	if (sp->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+	sp->parm.capture.readbuffers = 3;
+
+	cp->capability = V4L2_CAP_TIMEPERFRAME;
+	cp->timeperframe.numerator = 1;
+	cp->timeperframe.denominator = vc->fps;
+	return 0;
+}
+
+static int tw686x_s_parm(struct file *file, void *priv,
+			 struct v4l2_streamparm *sp)
+{
+	struct tw686x_video_channel *vc = video_drvdata(file);
+	struct v4l2_captureparm *cp = &sp->parm.capture;
+	unsigned int denominator = cp->timeperframe.denominator;
+	unsigned int numerator = cp->timeperframe.numerator;
+	unsigned int fps;
+
+	if (vb2_is_busy(&vc->vidq))
+		return -EBUSY;
+
+	fps = (!numerator || !denominator) ? 0 : denominator / numerator;
+	if (vc->fps != fps)
+		tw686x_set_framerate(vc, fps);
+	return tw686x_g_parm(file, priv, sp);
+}
+
 static int tw686x_enum_fmt_vid_cap(struct file *file, void *priv,
 				   struct v4l2_fmtdesc *f)
 {
@@ -998,6 +1035,9 @@ static const struct v4l2_ioctl_ops tw686x_video_ioctl_ops = {
 	.vidioc_g_std			= tw686x_g_std,
 	.vidioc_s_std			= tw686x_s_std,
 
+	.vidioc_g_parm			= tw686x_g_parm,
+	.vidioc_s_parm			= tw686x_s_parm,
+
 	.vidioc_enum_input		= tw686x_enum_input,
 	.vidioc_g_input			= tw686x_g_input,
 	.vidioc_s_input			= tw686x_s_input,
-- 
2.9.0

Re: [PATCH 2/2] tw686x: Support VIDIOC_{S,G}_PARM ioctls

[Hans Verkuil]

On 06/29/2016 04:17 AM, Ezequiel Garcia wrote:
> Now that the frame rate can be properly set, this commit adds support
> for S_PARM and G_PARM.

As mentioned in our irc discussion you also need to add enum_framesize/intervals.
Otherwise applications won't know what values to use with s_parm.

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 46 ++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 3131f9305313..40b5b835d452 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -437,9 +437,6 @@ static void tw686x_set_framerate(struct tw686x_video_channel *vc,
>  {
>  	unsigned int i;
>  
> -	if (vc->fps == fps)
> -		return;
> -
>  	i = tw686x_fps_idx(fps, TW686X_MAX_FPS(vc->video_standard));
>  	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
>  	vc->fps = tw686x_real_fps(i, TW686X_MAX_FPS(vc->video_standard));
> @@ -843,6 +840,12 @@ static int tw686x_s_std(struct file *file, void *priv, v4l2_std_id id)
>  	ret = tw686x_g_fmt_vid_cap(file, priv, &f);
>  	if (!ret)
>  		tw686x_s_fmt_vid_cap(file, priv, &f);
> +
> +	/*
> +	 * Frame decimation depends on the chosen standard,
> +	 * so reset it to the current value.
> +	 */
> +	tw686x_set_framerate(vc, vc->fps);
>  	return 0;
>  }
>  
> @@ -912,6 +915,40 @@ static int tw686x_g_std(struct file *file, void *priv, v4l2_std_id *id)
>  	return 0;
>  }
>  
> +static int tw686x_g_parm(struct file *file, void *priv,
> +			 struct v4l2_streamparm *sp)
> +{
> +	struct tw686x_video_channel *vc = video_drvdata(file);
> +	struct v4l2_captureparm *cp = &sp->parm.capture;
> +
> +	if (sp->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;

You need this check in s_parm as well.

> +	sp->parm.capture.readbuffers = 3;
> +
> +	cp->capability = V4L2_CAP_TIMEPERFRAME;
> +	cp->timeperframe.numerator = 1;
> +	cp->timeperframe.denominator = vc->fps;
> +	return 0;
> +}
> +
> +static int tw686x_s_parm(struct file *file, void *priv,
> +			 struct v4l2_streamparm *sp)
> +{
> +	struct tw686x_video_channel *vc = video_drvdata(file);
> +	struct v4l2_captureparm *cp = &sp->parm.capture;
> +	unsigned int denominator = cp->timeperframe.denominator;
> +	unsigned int numerator = cp->timeperframe.numerator;
> +	unsigned int fps;
> +
> +	if (vb2_is_busy(&vc->vidq))
> +		return -EBUSY;
> +
> +	fps = (!numerator || !denominator) ? 0 : denominator / numerator;
> +	if (vc->fps != fps)
> +		tw686x_set_framerate(vc, fps);
> +	return tw686x_g_parm(file, priv, sp);
> +}
> +
>  static int tw686x_enum_fmt_vid_cap(struct file *file, void *priv,
>  				   struct v4l2_fmtdesc *f)
>  {
> @@ -998,6 +1035,9 @@ static const struct v4l2_ioctl_ops tw686x_video_ioctl_ops = {
>  	.vidioc_g_std			= tw686x_g_std,
>  	.vidioc_s_std			= tw686x_s_std,
>  
> +	.vidioc_g_parm			= tw686x_g_parm,
> +	.vidioc_s_parm			= tw686x_s_parm,
> +
>  	.vidioc_enum_input		= tw686x_enum_input,
>  	.vidioc_g_input			= tw686x_g_input,
>  	.vidioc_s_input			= tw686x_s_input,
> 

Regards,

	Hans

Re: [PATCH 2/2] tw686x: Support VIDIOC_{S,G}_PARM ioctls

[Ezequiel Garcia]

On 01 Jul 12:46 PM, Hans Verkuil wrote:
> On 06/29/2016 04:17 AM, Ezequiel Garcia wrote:
> > Now that the frame rate can be properly set, this commit adds support
> > for S_PARM and G_PARM.
> 
> As mentioned in our irc discussion you also need to add enum_framesize/intervals.
> Otherwise applications won't know what values to use with s_parm.
> 

Sure, I'll add those as follow-up patches.

> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> >  drivers/media/pci/tw686x/tw686x-video.c | 46 ++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> > index 3131f9305313..40b5b835d452 100644
> > --- a/drivers/media/pci/tw686x/tw686x-video.c
> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
> > @@ -437,9 +437,6 @@ static void tw686x_set_framerate(struct tw686x_video_channel *vc,
> >  {
> >  	unsigned int i;
> >  
> > -	if (vc->fps == fps)
> > -		return;
> > -
> >  	i = tw686x_fps_idx(fps, TW686X_MAX_FPS(vc->video_standard));
> >  	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
> >  	vc->fps = tw686x_real_fps(i, TW686X_MAX_FPS(vc->video_standard));
> > @@ -843,6 +840,12 @@ static int tw686x_s_std(struct file *file, void *priv, v4l2_std_id id)
> >  	ret = tw686x_g_fmt_vid_cap(file, priv, &f);
> >  	if (!ret)
> >  		tw686x_s_fmt_vid_cap(file, priv, &f);
> > +
> > +	/*
> > +	 * Frame decimation depends on the chosen standard,
> > +	 * so reset it to the current value.
> > +	 */
> > +	tw686x_set_framerate(vc, vc->fps);
> >  	return 0;
> >  }
> >  
> > @@ -912,6 +915,40 @@ static int tw686x_g_std(struct file *file, void *priv, v4l2_std_id *id)
> >  	return 0;
> >  }
> >  
> > +static int tw686x_g_parm(struct file *file, void *priv,
> > +			 struct v4l2_streamparm *sp)
> > +{
> > +	struct tw686x_video_channel *vc = video_drvdata(file);
> > +	struct v4l2_captureparm *cp = &sp->parm.capture;
> > +
> > +	if (sp->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> 
> You need this check in s_parm as well.
> 

Will do.

> > +	sp->parm.capture.readbuffers = 3;
> > +
> > +	cp->capability = V4L2_CAP_TIMEPERFRAME;
> > +	cp->timeperframe.numerator = 1;
> > +	cp->timeperframe.denominator = vc->fps;
> > +	return 0;
> > +}
> > +
> > +static int tw686x_s_parm(struct file *file, void *priv,
> > +			 struct v4l2_streamparm *sp)
> > +{
> > +	struct tw686x_video_channel *vc = video_drvdata(file);
> > +	struct v4l2_captureparm *cp = &sp->parm.capture;
> > +	unsigned int denominator = cp->timeperframe.denominator;
> > +	unsigned int numerator = cp->timeperframe.numerator;
> > +	unsigned int fps;
> > +
> > +	if (vb2_is_busy(&vc->vidq))
> > +		return -EBUSY;
> > +
> > +	fps = (!numerator || !denominator) ? 0 : denominator / numerator;
> > +	if (vc->fps != fps)
> > +		tw686x_set_framerate(vc, fps);
> > +	return tw686x_g_parm(file, priv, sp);
> > +}
> > +
> >  static int tw686x_enum_fmt_vid_cap(struct file *file, void *priv,
> >  				   struct v4l2_fmtdesc *f)
> >  {
> > @@ -998,6 +1035,9 @@ static const struct v4l2_ioctl_ops tw686x_video_ioctl_ops = {
> >  	.vidioc_g_std			= tw686x_g_std,
> >  	.vidioc_s_std			= tw686x_s_std,
> >  
> > +	.vidioc_g_parm			= tw686x_g_parm,
> > +	.vidioc_s_parm			= tw686x_s_parm,
> > +
> >  	.vidioc_enum_input		= tw686x_enum_input,
> >  	.vidioc_g_input			= tw686x_g_input,
> >  	.vidioc_s_input			= tw686x_s_input,
> > 

Thanks,
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar