[v3] tw686x: use a formula instead of two tables for div
diff mbox

Message ID 527358bfde61acf7ac1a6e5bfc737f5645ba05a7.1461770668.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab April 27, 2016, 3:27 p.m. UTC
Instead of using two tables to estimate the temporal decimation
factor, use a formula. This allows to get the closest fps, with
sounds better than the current tables.

Compile-tested only.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

[media] tw686x: cleanup the fps estimation code

There are some issues with the old code:
	1) it uses two static tables;
	2) some values for 50Hz standards are wrong;
	3) it doesn't store the real framerate.

This patch fixes the above issues.

Compile-tested only.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

-

v3: Patch v2 were actually a diff patch against PATCH v1. Fold the two patches in one.

PS.: With this patch, it should be easy to add support for
VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the
real frame rate, with should be used when returning from those
functions.

---
 drivers/media/pci/tw686x/tw686x-video.c | 110 +++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 37 deletions(-)

Comments

Ezequiel Garcia June 27, 2016, 3:45 p.m. UTC | #1
Hi Mauro,

Thanks a lot for the patch.

On 27 April 2016 at 12:27, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> Instead of using two tables to estimate the temporal decimation
> factor, use a formula. This allows to get the closest fps, with
> sounds better than the current tables.
>
> Compile-tested only.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> [media] tw686x: cleanup the fps estimation code
>
> There are some issues with the old code:
>         1) it uses two static tables;
>         2) some values for 50Hz standards are wrong;
>         3) it doesn't store the real framerate.
>
> This patch fixes the above issues.
>
> Compile-tested only.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> -
>
> v3: Patch v2 were actually a diff patch against PATCH v1. Fold the two patches in one.
>
> PS.: With this patch, it should be easy to add support for
> VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the
> real frame rate, with should be used when returning from those
> functions.
>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 110 +++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 253e10823ba3..b247a7b4ddd8 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -43,53 +43,89 @@ static const struct tw686x_format formats[] = {
>         }
>  };
>
> -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.
> +        */

I ran a few tests and it worked perfectly fine for 60Hz standards.
For 50Hz standards, or at least for PAL-Nc, it didn't
work so well, and the real FPS was too different from the requested
one. I need to look into it some more.

> +       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) */

Why this particular selection of fps values and bits set in each case?
Is it arbitrary?

> +};
> +
> +static unsigned int tw686x_real_fps(unsigned int index, unsigned int max_fps)
> +{
> +       unsigned int i, bits, c = 0;
> +
> +       if (!index || index >= ARRAY_SIZE(fps_map))
> +               return max_fps;
> +
> +       bits = fps_map[index];
> +       for (i = 0; i < max_fps; i++)
> +               if ((1 << i) & bits)
> +                       c++;
> +

We can use hweight_long here to count the number of bits set.
If you are OK with it, I can rework the patch and submit a new version.
Mauro Carvalho Chehab June 27, 2016, 5:38 p.m. UTC | #2
Em Mon, 27 Jun 2016 12:45:38 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:

> Hi Mauro,
> 
> Thanks a lot for the patch.
> 
> On 27 April 2016 at 12:27, Mauro Carvalho Chehab
> <mchehab@osg.samsung.com> wrote:
> > Instead of using two tables to estimate the temporal decimation
> > factor, use a formula. This allows to get the closest fps, with
> > sounds better than the current tables.
> >
> > Compile-tested only.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >
> > [media] tw686x: cleanup the fps estimation code
> >
> > There are some issues with the old code:
> >         1) it uses two static tables;
> >         2) some values for 50Hz standards are wrong;
> >         3) it doesn't store the real framerate.
> >
> > This patch fixes the above issues.
> >
> > Compile-tested only.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >
> > -
> >
> > v3: Patch v2 were actually a diff patch against PATCH v1. Fold the two patches in one.
> >
> > PS.: With this patch, it should be easy to add support for
> > VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the
> > real frame rate, with should be used when returning from those
> > functions.
> >
> > ---
> >  drivers/media/pci/tw686x/tw686x-video.c | 110 +++++++++++++++++++++-----------
> >  1 file changed, 73 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> > index 253e10823ba3..b247a7b4ddd8 100644
> > --- a/drivers/media/pci/tw686x/tw686x-video.c
> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
> > @@ -43,53 +43,89 @@ static const struct tw686x_format formats[] = {
> >         }
> >  };
> >
> > -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.
> > +        */  
> 
> I ran a few tests and it worked perfectly fine for 60Hz standards.

Good!

> For 50Hz standards, or at least for PAL-Nc, it didn't
> work so well, and the real FPS was too different from the requested
> one. I need to look into it some more.

I would be expecting a maximum difference a little bigger than 1 Hz.

> > +       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) */  
> 
> Why this particular selection of fps values and bits set in each case?
> Is it arbitrary?

No. This is the same table that was already in the code:

static const unsigned int map[15] = {
                0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
                0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
                0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
};

Except that the calculus that used to be there to set bit 31 to 1
on everything except map[0] and the code that makes it set two
FPS at the same time were pre-calculated, e. g. I run this code
locally to generate the new table:

	map = tw686x_fields_map(vc->video_standard, fps) << 1;
	map |= map << 1;
	if (map > 0)
		map |= BIT(31);

There, bit 31 = 0 disables the frame filtering. bit 31 = 1 enables it.

Each bit at the 0-29 bitrange means one of the 30 frames received.
If equal to 1, the frame is sent; if equal to 0, it is not sent.

So, the first value, for example: 0x80000006 has 2 consecutive
bits set, so the mean frame rate would be 2Hz. The next value there
is 0x0x80018006, with has 4 bits selected, so mean fps is 4Hz, and
so on.

As the original table always sets 2 consecutive frames at the same
time, I suspect that this is a requirement to avoid interlacing
issues.

So, the code I used to generate the table always allocate 2 consecutive
bits each time.

I suspect that such the table was built assuming that there are 30 bits,
but, on 50Hz, only 25 bits are used. So, it is sub-optimal for 50 Hz.
It means that the frames are not equally spaced.

If you want, I can construct a table for 50Hz that would produce better
results.

> 
> > +};
> > +
> > +static unsigned int tw686x_real_fps(unsigned int index, unsigned int max_fps)
> > +{
> > +       unsigned int i, bits, c = 0;
> > +
> > +       if (!index || index >= ARRAY_SIZE(fps_map))
> > +               return max_fps;
> > +
> > +       bits = fps_map[index];
> > +       for (i = 0; i < max_fps; i++)
> > +               if ((1 << i) & bits)
> > +                       c++;
> > +  
> 
> We can use hweight_long here to count the number of bits set.
> If you are OK with it, I can rework the patch and submit a new version.
Ezequiel Garcia June 29, 2016, 12:11 a.m. UTC | #3
On 27 June 2016 at 14:38, Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:
> Em Mon, 27 Jun 2016 12:45:38 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
>
>> Hi Mauro,
>>
>> Thanks a lot for the patch.
>>
>> On 27 April 2016 at 12:27, Mauro Carvalho Chehab
>> <mchehab@osg.samsung.com> wrote:
>> > Instead of using two tables to estimate the temporal decimation
>> > factor, use a formula. This allows to get the closest fps, with
>> > sounds better than the current tables.
>> >
>> > Compile-tested only.
>> >
>> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>> >
>> > [media] tw686x: cleanup the fps estimation code
>> >
>> > There are some issues with the old code:
>> >         1) it uses two static tables;
>> >         2) some values for 50Hz standards are wrong;
>> >         3) it doesn't store the real framerate.
>> >
>> > This patch fixes the above issues.
>> >
>> > Compile-tested only.
>> >
>> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>> >
>> > -
>> >
>> > v3: Patch v2 were actually a diff patch against PATCH v1. Fold the two patches in one.
>> >
>> > PS.: With this patch, it should be easy to add support for
>> > VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the
>> > real frame rate, with should be used when returning from those
>> > functions.
>> >
>> > ---
>> >  drivers/media/pci/tw686x/tw686x-video.c | 110 +++++++++++++++++++++-----------
>> >  1 file changed, 73 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
>> > index 253e10823ba3..b247a7b4ddd8 100644
>> > --- a/drivers/media/pci/tw686x/tw686x-video.c
>> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
>> > @@ -43,53 +43,89 @@ static const struct tw686x_format formats[] = {
>> >         }
>> >  };
>> >
>> > -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.
>> > +        */
>>
>> I ran a few tests and it worked perfectly fine for 60Hz standards.
>
> Good!
>
>> For 50Hz standards, or at least for PAL-Nc, it didn't
>> work so well, and the real FPS was too different from the requested
>> one. I need to look into it some more.
>
> I would be expecting a maximum difference a little bigger than 1 Hz.
>
>> > +       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) */
>>
>> Why this particular selection of fps values and bits set in each case?
>> Is it arbitrary?
>
> No. This is the same table that was already in the code:
>
> static const unsigned int map[15] = {
>                 0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
>                 0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
>                 0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
> };
>
> Except that the calculus that used to be there to set bit 31 to 1
> on everything except map[0] and the code that makes it set two
> FPS at the same time were pre-calculated, e. g. I run this code
> locally to generate the new table:
>
>         map = tw686x_fields_map(vc->video_standard, fps) << 1;
>         map |= map << 1;
>         if (map > 0)
>                 map |= BIT(31);
>
> There, bit 31 = 0 disables the frame filtering. bit 31 = 1 enables it.
>
> Each bit at the 0-29 bitrange means one of the 30 frames received.
> If equal to 1, the frame is sent; if equal to 0, it is not sent.
>
> So, the first value, for example: 0x80000006 has 2 consecutive
> bits set, so the mean frame rate would be 2Hz. The next value there
> is 0x0x80018006, with has 4 bits selected, so mean fps is 4Hz, and
> so on.
>
> As the original table always sets 2 consecutive frames at the same
> time, I suspect that this is a requirement to avoid interlacing
> issues.
>
> So, the code I used to generate the table always allocate 2 consecutive
> bits each time.
>
> I suspect that such the table was built assuming that there are 30 bits,
> but, on 50Hz, only 25 bits are used. So, it is sub-optimal for 50 Hz.
> It means that the frames are not equally spaced.
>
> If you want, I can construct a table for 50Hz that would produce better
> results.
>

Actually, it's working fine on both 50Hz and 60Hz standards. I tested
PAL-Nc using my modifications, and I had an small error in the
hweight_long usage. My bad!

So... now I think it's working properly, for 50Hz the requested FPS matches
the actual FPS, and for 60Hz there's a slight difference:

* 24 FPS gives 23.50 FPS
* 22 FPS gives 21.66 FPS
* 20 FPS gives 20 FPS
* 10 FPS gives 10 FPS
* 6 FPS give 5 FPS
* 2 FPS gives 1.66 FPS

Output FPS values are those measured by qv4l2.

Patch
diff mbox

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 253e10823ba3..b247a7b4ddd8 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -43,53 +43,89 @@  static const struct tw686x_format formats[] = {
 	}
 };
 
-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)
+{
+	unsigned int i, bits, c = 0;
+
+	if (!index || index >= ARRAY_SIZE(fps_map))
+		return max_fps;
+
+	bits = fps_map[index];
+	for (i = 0; i < max_fps; i++)
+		if ((1 << i) & bits)
+			c++;
+
+	return c;
+}
+
+static unsigned int tw686x_fps_idx(unsigned int fps, 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 int i;
-
-	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];
-	}
-
-	return map[i];
+	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 idx;
 }
 
 static void tw686x_set_framerate(struct tw686x_video_channel *vc,
 				 unsigned int fps)
 {
-	unsigned int map;
+	unsigned int i, max_fps;
 
 	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;
+	if (vc->video_standard & V4L2_STD_525_60)
+		max_fps = 30;
+	else
+		max_fps = 25;
+
+	i = tw686x_fps_idx(fps, max_fps);
+
+	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
+	vc->fps = tw686x_real_fps(i, max_fps);
 }
 
 static const struct tw686x_format *format_by_fourcc(unsigned int fourcc)