[PATCH V2 0/2] media: video-i2c: additional support for Melexis MLX90640

[PATCH V2 0/2] media: video-i2c: additional support for Melexis MLX90640

[Seongyong Park]

Previous attempt at re-submitting the patch(set) seems to have failed, due to a mishap at threading.
Here is another attempt.

[PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Seongyong Park]

Current implementation calculates frame delay based on the time of
start of the loop. This inevitably causes the loop delay to be
slightly longer than the actual measurement period, thus skipping a frame
every now and then.

However, MLX90640 should ideally be working without a frame skip.
Each measurement step updates half of the pixels in the frame
(every other pixel in default "chess mode", and every other row
in "interleave mode"), while additional coefficient data (25th & 26th row)
updates every step. The compensational coefficient data only corresponds
with the pixels updated in the same step.

In short, if a frame is skipped, then half of a frame loses correction
information and becomes garbage data.

Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 0465832a4..2ccb08335 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
 	struct video_i2c_data *data = priv;
 	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
 				       data->frame_interval.denominator);
+	unsigned long end_jiffies = jiffies;

 	set_freezable();

 	do {
-		unsigned long start_jiffies = jiffies;
 		struct video_i2c_buffer *vid_cap_buf = NULL;
 		int schedule_delay;

+		end_jiffies += delay;
+
 		try_to_freeze();

 		spin_lock(&data->slock);
@@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
 				VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
 		}

-		schedule_delay = delay - (jiffies - start_jiffies);
-
-		if (time_after(jiffies, start_jiffies + delay))
-			schedule_delay = delay;
+		schedule_delay = end_jiffies - jiffies;
+		while (schedule_delay <= 0)
+			schedule_delay += delay;

 		schedule_timeout_interruptible(schedule_delay);
 	} while (!kthread_should_stop());
--
2.31.1

[PATCH 2/2] media: video-i2c: append register data on MLX90640's frame

[Seongyong Park]

On MLX90640, Each measurement step updates half of the pixels in the frame
(every other pixel in default "chess mode", and every other row
in "interleave mode"), while additional coefficient data (25th & 26th row)
updates every step. The compensational coefficient data only corresponds
with the pixels updated in the same step.

Only way to know which "subpage" was updated on the last step is to read
"status register" on address 0x8000. Without this data,
compensation calculation may be able to detect which sets of pixels have
been updated, but it will have to make assumptions when frame skip happens,
and there is no way to do it correctly when the host simply cannot
keep up with refresh rate.

Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 2ccb08335..ca3a1c504 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -74,7 +74,7 @@ static const struct v4l2_fmtdesc mlx90640_format = {

 static const struct v4l2_frmsize_discrete mlx90640_size = {
 	.width = 32,
-	.height = 26, /* 24 lines of pixel data + 2 lines of processing data */
+	.height = 27, /* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
 };

 static const struct regmap_config amg88xx_regmap_config = {
@@ -168,8 +168,11 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)

 static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
 {
-	return regmap_bulk_read(data->regmap, 0x400, buf,
-				data->chip->buffer_size);
+	int ret = regmap_bulk_read(data->regmap, 0x400, buf,
+				   data->chip->buffer_size - 64);
+	if (ret) return ret;
+	return regmap_bulk_read(data->regmap, 0x8000, buf + 1664,
+				64);
 }

 static int amg88xx_setup(struct video_i2c_data *data)
@@ -375,7 +378,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.format		= &mlx90640_format,
 		.frame_intervals	= mlx90640_frame_intervals,
 		.num_frame_intervals	= ARRAY_SIZE(mlx90640_frame_intervals),
-		.buffer_size	= 1664,
+		.buffer_size	= 1728,
 		.bpp		= 16,
 		.regmap_config	= &mlx90640_regmap_config,
 		.nvmem_config	= &mlx90640_nvram_config,
--
2.31.1

Re: [PATCH 2/2] media: video-i2c: append register data on MLX90640's frame

[Matt Ranostay]

On Sun, May 16, 2021 at 4:09 AM Seongyong Park
<euphoriccatface@gmail.com> wrote:
>
> On MLX90640, Each measurement step updates half of the pixels in the frame
> (every other pixel in default "chess mode", and every other row
> in "interleave mode"), while additional coefficient data (25th & 26th row)
> updates every step. The compensational coefficient data only corresponds
> with the pixels updated in the same step.
>
> Only way to know which "subpage" was updated on the last step is to read
> "status register" on address 0x8000. Without this data,
> compensation calculation may be able to detect which sets of pixels have
> been updated, but it will have to make assumptions when frame skip happens,
> and there is no way to do it correctly when the host simply cannot
> keep up with refresh rate.
>
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 2ccb08335..ca3a1c504 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -74,7 +74,7 @@ static const struct v4l2_fmtdesc mlx90640_format = {
>
>  static const struct v4l2_frmsize_discrete mlx90640_size = {
>         .width = 32,
> -       .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
> +       .height = 27, /* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
>  };
>
>  static const struct regmap_config amg88xx_regmap_config = {
> @@ -168,8 +168,11 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
>
>  static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
>  {
> -       return regmap_bulk_read(data->regmap, 0x400, buf,
> -                               data->chip->buffer_size);
> +       int ret = regmap_bulk_read(data->regmap, 0x400, buf,
> +                                  data->chip->buffer_size - 64);
> +       if (ret) return ret;
> +       return regmap_bulk_read(data->regmap, 0x8000, buf + 1664,
> +                               64);

Rather than "buf + 1664" it would be more portable and clear if you
use "buf + (data->chip->buffer_size - 64)"

Also isn't the 1 line of registers only 32-bytes not 64-bytes?

- Matt

>  }
>
>  static int amg88xx_setup(struct video_i2c_data *data)
> @@ -375,7 +378,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
>                 .format         = &mlx90640_format,
>                 .frame_intervals        = mlx90640_frame_intervals,
>                 .num_frame_intervals    = ARRAY_SIZE(mlx90640_frame_intervals),
> -               .buffer_size    = 1664,
> +               .buffer_size    = 1728,
>                 .bpp            = 16,
>                 .regmap_config  = &mlx90640_regmap_config,
>                 .nvmem_config   = &mlx90640_nvram_config,
> --
> 2.31.1
>

Re: [PATCH 2/2] media: video-i2c: append register data on MLX90640's frame

[Seongyong Park]

2021년 5월 17일 (월) 오전 5:49, Matt Ranostay <matt.ranostay@konsulko.com>님이 작성:
>
> On Sun, May 16, 2021 at 4:09 AM Seongyong Park
> <euphoriccatface@gmail.com> wrote:
> >
> > On MLX90640, Each measurement step updates half of the pixels in the frame
> > (every other pixel in default "chess mode", and every other row
> > in "interleave mode"), while additional coefficient data (25th & 26th row)
> > updates every step. The compensational coefficient data only corresponds
> > with the pixels updated in the same step.
> >
> > Only way to know which "subpage" was updated on the last step is to read
> > "status register" on address 0x8000. Without this data,
> > compensation calculation may be able to detect which sets of pixels have
> > been updated, but it will have to make assumptions when frame skip happens,
> > and there is no way to do it correctly when the host simply cannot
> > keep up with refresh rate.
> >
> > Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 2ccb08335..ca3a1c504 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -74,7 +74,7 @@ static const struct v4l2_fmtdesc mlx90640_format = {
> >
> >  static const struct v4l2_frmsize_discrete mlx90640_size = {
> >         .width = 32,
> > -       .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
> > +       .height = 27, /* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
> >  };
> >
> >  static const struct regmap_config amg88xx_regmap_config = {
> > @@ -168,8 +168,11 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
> >
> >  static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
> >  {
> > -       return regmap_bulk_read(data->regmap, 0x400, buf,
> > -                               data->chip->buffer_size);
> > +       int ret = regmap_bulk_read(data->regmap, 0x400, buf,
> > +                                  data->chip->buffer_size - 64);
> > +       if (ret) return ret;
> > +       return regmap_bulk_read(data->regmap, 0x8000, buf + 1664,
> > +                               64);
>
> Rather than "buf + 1664" it would be more portable and clear if you
> use "buf + (data->chip->buffer_size - 64)"

Okay, I'll send another patch later to change it.

> Also isn't the 1 line of registers only 32-bytes not 64-bytes?

In terms of the V4L2 frame, a row is 64-bytes because width is 32 and
each pixel is 16-bit.
This was the main reason to decide the appendage to be 64-bytes.

For MLX90640, the address 0x8010 is mentioned in the datasheet, and
actual output appears that up to 0x8013 is valid
For reference, an output from my sensor shows:
0x8000: 0900 a200 9f69 0000 6120 0500 2003 e003 2817 4f8e 8701 8d04
0000 8119 0000 0000
0x8010: 33be 8000 4d02 0000 ffff ffff ffff ffff ffff ffff ffff ffff
ffff ffff ffff ffff

- Seongyong

>
> - Matt
>
> >  }
> >
> >  static int amg88xx_setup(struct video_i2c_data *data)
> > @@ -375,7 +378,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
> >                 .format         = &mlx90640_format,
> >                 .frame_intervals        = mlx90640_frame_intervals,
> >                 .num_frame_intervals    = ARRAY_SIZE(mlx90640_frame_intervals),
> > -               .buffer_size    = 1664,
> > +               .buffer_size    = 1728,
> >                 .bpp            = 16,
> >                 .regmap_config  = &mlx90640_regmap_config,
> >                 .nvmem_config   = &mlx90640_nvram_config,
> > --
> > 2.31.1
> >

Re: [PATCH 2/2] media: video-i2c: append register data on MLX90640's frame

[Matt Ranostay]

On Sun, May 16, 2021 at 6:40 PM Seongyong Park
<euphoriccatface@gmail.com> wrote:
>
> 2021년 5월 17일 (월) 오전 5:49, Matt Ranostay <matt.ranostay@konsulko.com>님이 작성:
> >
> > On Sun, May 16, 2021 at 4:09 AM Seongyong Park
> > <euphoriccatface@gmail.com> wrote:
> > >
> > > On MLX90640, Each measurement step updates half of the pixels in the frame
> > > (every other pixel in default "chess mode", and every other row
> > > in "interleave mode"), while additional coefficient data (25th & 26th row)
> > > updates every step. The compensational coefficient data only corresponds
> > > with the pixels updated in the same step.
> > >
> > > Only way to know which "subpage" was updated on the last step is to read
> > > "status register" on address 0x8000. Without this data,
> > > compensation calculation may be able to detect which sets of pixels have
> > > been updated, but it will have to make assumptions when frame skip happens,
> > > and there is no way to do it correctly when the host simply cannot
> > > keep up with refresh rate.
> > >
> > > Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> > > ---
> > >  drivers/media/i2c/video-i2c.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > > index 2ccb08335..ca3a1c504 100644
> > > --- a/drivers/media/i2c/video-i2c.c
> > > +++ b/drivers/media/i2c/video-i2c.c
> > > @@ -74,7 +74,7 @@ static const struct v4l2_fmtdesc mlx90640_format = {
> > >
> > >  static const struct v4l2_frmsize_discrete mlx90640_size = {
> > >         .width = 32,
> > > -       .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
> > > +       .height = 27, /* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
> > >  };
> > >
> > >  static const struct regmap_config amg88xx_regmap_config = {
> > > @@ -168,8 +168,11 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
> > >
> > >  static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
> > >  {
> > > -       return regmap_bulk_read(data->regmap, 0x400, buf,
> > > -                               data->chip->buffer_size);
> > > +       int ret = regmap_bulk_read(data->regmap, 0x400, buf,
> > > +                                  data->chip->buffer_size - 64);
> > > +       if (ret) return ret;
> > > +       return regmap_bulk_read(data->regmap, 0x8000, buf + 1664,
> > > +                               64);
> >
> > Rather than "buf + 1664" it would be more portable and clear if you
> > use "buf + (data->chip->buffer_size - 64)"
>
> Okay, I'll send another patch later to change it.
>
> > Also isn't the 1 line of registers only 32-bytes not 64-bytes?
>
> In terms of the V4L2 frame, a row is 64-bytes because width is 32 and
> each pixel is 16-bit.
> This was the main reason to decide the appendage to be 64-bytes.
>

Ok, forgot this sensor has 12-bit data padded to 16-bits.

- Matt

> For MLX90640, the address 0x8010 is mentioned in the datasheet, and
> actual output appears that up to 0x8013 is valid
> For reference, an output from my sensor shows:
> 0x8000: 0900 a200 9f69 0000 6120 0500 2003 e003 2817 4f8e 8701 8d04
> 0000 8119 0000 0000
> 0x8010: 33be 8000 4d02 0000 ffff ffff ffff ffff ffff ffff ffff ffff
> ffff ffff ffff ffff
>
> - Seongyong
>
> >
> > - Matt
> >
> > >  }
> > >
> > >  static int amg88xx_setup(struct video_i2c_data *data)
> > > @@ -375,7 +378,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
> > >                 .format         = &mlx90640_format,
> > >                 .frame_intervals        = mlx90640_frame_intervals,
> > >                 .num_frame_intervals    = ARRAY_SIZE(mlx90640_frame_intervals),
> > > -               .buffer_size    = 1664,
> > > +               .buffer_size    = 1728,
> > >                 .bpp            = 16,
> > >                 .regmap_config  = &mlx90640_regmap_config,
> > >                 .nvmem_config   = &mlx90640_nvram_config,
> > > --
> > > 2.31.1
> > >

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Matt Ranostay]

On Sun, May 16, 2021 at 4:09 AM Seongyong Park
<euphoriccatface@gmail.com> wrote:
>
> Current implementation calculates frame delay based on the time of
> start of the loop. This inevitably causes the loop delay to be
> slightly longer than the actual measurement period, thus skipping a frame
> every now and then.
>
> However, MLX90640 should ideally be working without a frame skip.
> Each measurement step updates half of the pixels in the frame
> (every other pixel in default "chess mode", and every other row
> in "interleave mode"), while additional coefficient data (25th & 26th row)
> updates every step. The compensational coefficient data only corresponds
> with the pixels updated in the same step.
>
> In short, if a frame is skipped, then half of a frame loses correction
> information and becomes garbage data.

This changeset makes sense to me. Guess I got lucky with the timing on my board.

Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>

>
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 0465832a4..2ccb08335 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
>         struct video_i2c_data *data = priv;
>         unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
>                                        data->frame_interval.denominator);
> +       unsigned long end_jiffies = jiffies;
>
>         set_freezable();
>
>         do {
> -               unsigned long start_jiffies = jiffies;
>                 struct video_i2c_buffer *vid_cap_buf = NULL;
>                 int schedule_delay;
>
> +               end_jiffies += delay;
> +
>                 try_to_freeze();
>
>                 spin_lock(&data->slock);
> @@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
>                                 VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>                 }
>
> -               schedule_delay = delay - (jiffies - start_jiffies);
> -
> -               if (time_after(jiffies, start_jiffies + delay))
> -                       schedule_delay = delay;
> +               schedule_delay = end_jiffies - jiffies;
> +               while (schedule_delay <= 0)
> +                       schedule_delay += delay;
>
>                 schedule_timeout_interruptible(schedule_delay);
>         } while (!kthread_should_stop());
> --
> 2.31.1
>

[PATCH V2 2/2] media: video-i2c: append register data on MLX90640's frame

[Seongyong Park]

On MLX90640, Each measurement step updates half of the pixels in the frame
(every other pixel in default "chess mode", and every other row
in "interleave mode"), while additional coefficient data (25th & 26th row)
updates every step. The compensational coefficient data only corresponds
with the pixels updated in the same step.

Only way to know which "subpage" was updated on the last step is to read
"status register" on address 0x8000. Without this data,
compensation calculation may be able to detect which sets of pixels have
been updated, but it will have to make assumptions when frame skip happens,
and there is no way to do it correctly when the host simply cannot
keep up with refresh rate.

Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 2ccb08335..f2313b446 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -74,7 +74,8 @@ static const struct v4l2_fmtdesc mlx90640_format = {
 
 static const struct v4l2_frmsize_discrete mlx90640_size = {
 	.width = 32,
-	.height = 26, /* 24 lines of pixel data + 2 lines of processing data */
+	.height = 27,
+	/* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
 };
 
 static const struct regmap_config amg88xx_regmap_config = {
@@ -168,8 +169,12 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 
 static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
 {
-	return regmap_bulk_read(data->regmap, 0x400, buf,
-				data->chip->buffer_size);
+	int ret = regmap_bulk_read(data->regmap, 0x400, buf,
+				   data->chip->buffer_size - 64);
+	if (ret)
+		return ret;
+	return regmap_bulk_read(data->regmap, 0x8000, buf + (data->chip->buffer_size - 64),
+				64);
 }
 
 static int amg88xx_setup(struct video_i2c_data *data)
@@ -375,7 +380,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.format		= &mlx90640_format,
 		.frame_intervals	= mlx90640_frame_intervals,
 		.num_frame_intervals	= ARRAY_SIZE(mlx90640_frame_intervals),
-		.buffer_size	= 1664,
+		.buffer_size	= 1728,
 		.bpp		= 16,
 		.regmap_config	= &mlx90640_regmap_config,
 		.nvmem_config	= &mlx90640_nvram_config,
-- 
2.31.1

[PATCH V2 2/2] media: video-i2c: append register data on MLX90640's frame

[Seongyong Park]

On MLX90640, Each measurement step updates half of the pixels in the frame
(every other pixel in default "chess mode", and every other row
in "interleave mode"), while additional coefficient data (25th & 26th row)
updates every step. The compensational coefficient data only corresponds
with the pixels updated in the same step.

Only way to know which "subpage" was updated on the last step is to read
"status register" on address 0x8000. Without this data,
compensation calculation may be able to detect which sets of pixels have
been updated, but it will have to make assumptions when frame skip happens,
and there is no way to do it correctly when the host simply cannot
keep up with refresh rate.

Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 2ccb08335..f2313b446 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -74,7 +74,8 @@ static const struct v4l2_fmtdesc mlx90640_format = {
 
 static const struct v4l2_frmsize_discrete mlx90640_size = {
 	.width = 32,
-	.height = 26, /* 24 lines of pixel data + 2 lines of processing data */
+	.height = 27,
+	/* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
 };
 
 static const struct regmap_config amg88xx_regmap_config = {
@@ -168,8 +169,12 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 
 static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
 {
-	return regmap_bulk_read(data->regmap, 0x400, buf,
-				data->chip->buffer_size);
+	int ret = regmap_bulk_read(data->regmap, 0x400, buf,
+				   data->chip->buffer_size - 64);
+	if (ret)
+		return ret;
+	return regmap_bulk_read(data->regmap, 0x8000, buf + (data->chip->buffer_size - 64),
+				64);
 }
 
 static int amg88xx_setup(struct video_i2c_data *data)
@@ -375,7 +380,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.format		= &mlx90640_format,
 		.frame_intervals	= mlx90640_frame_intervals,
 		.num_frame_intervals	= ARRAY_SIZE(mlx90640_frame_intervals),
-		.buffer_size	= 1664,
+		.buffer_size	= 1728,
 		.bpp		= 16,
 		.regmap_config	= &mlx90640_regmap_config,
 		.nvmem_config	= &mlx90640_nvram_config,
-- 
2.31.1

[PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Seongyong Park]

Current implementation calculates frame delay based on the time of
start of the loop. This inevitably causes the loop delay to be
slightly longer than the actual measurement period, thus skipping a frame
every now and then.

However, MLX90640 should ideally be working without a frame skip.
Each measurement step updates half of the pixels in the frame
(every other pixel in default "chess mode", and every other row
in "interleave mode"), while additional coefficient data (25th & 26th row)
updates every step. The compensational coefficient data only corresponds
with the pixels updated in the same step.

In short, if a frame is skipped, then half of a frame loses correction
information and becomes garbage data.

Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 0465832a4..2ccb08335 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
 	struct video_i2c_data *data = priv;
 	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
 				       data->frame_interval.denominator);
+	unsigned long end_jiffies = jiffies;
 
 	set_freezable();
 
 	do {
-		unsigned long start_jiffies = jiffies;
 		struct video_i2c_buffer *vid_cap_buf = NULL;
 		int schedule_delay;
 
+		end_jiffies += delay;
+
 		try_to_freeze();
 
 		spin_lock(&data->slock);
@@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
 				VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
 		}
 
-		schedule_delay = delay - (jiffies - start_jiffies);
-
-		if (time_after(jiffies, start_jiffies + delay))
-			schedule_delay = delay;
+		schedule_delay = end_jiffies - jiffies;
+		while (schedule_delay <= 0)
+			schedule_delay += delay;
 
 		schedule_timeout_interruptible(schedule_delay);
 	} while (!kthread_should_stop());
-- 
2.31.1

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Mauro Carvalho Chehab]

Em Sat,  5 Jun 2021 20:54:56 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> Current implementation calculates frame delay based on the time of
> start of the loop. This inevitably causes the loop delay to be
> slightly longer than the actual measurement period, thus skipping a frame
> every now and then.
> 
> However, MLX90640 should ideally be working without a frame skip.
> Each measurement step updates half of the pixels in the frame
> (every other pixel in default "chess mode", and every other row
> in "interleave mode"), while additional coefficient data (25th & 26th row)
> updates every step. The compensational coefficient data only corresponds
> with the pixels updated in the same step.
> 
> In short, if a frame is skipped, then half of a frame loses correction
> information and becomes garbage data.
> 
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 0465832a4..2ccb08335 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
>  	struct video_i2c_data *data = priv;
>  	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
>  				       data->frame_interval.denominator);
> +	unsigned long end_jiffies = jiffies;
>  
>  	set_freezable();
>  
>  	do {
> -		unsigned long start_jiffies = jiffies;
>  		struct video_i2c_buffer *vid_cap_buf = NULL;
>  		int schedule_delay;
>  
> +		end_jiffies += delay;
> +
>  		try_to_freeze();
>  
>  		spin_lock(&data->slock);
> @@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
>  				VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>  		}
>  
> -		schedule_delay = delay - (jiffies - start_jiffies);
> -
> -		if (time_after(jiffies, start_jiffies + delay))
> -			schedule_delay = delay;

> +		schedule_delay = end_jiffies - jiffies;
> +		while (schedule_delay <= 0)
> +			schedule_delay += delay;

Huh? Why do you need a loop for that? If you want to make it positive,
you could just do:

	if (schedule_delay < 0)
		schedule_delay = delay; /* or something else */

but this won't solve the issue, as this is basically equivalent
to the logic you removed.

>  
>  		schedule_timeout_interruptible(schedule_delay);

This is probably what's causing troubles, as this only ensures
that it will sleep for at least schedule_delay (if not
interrupted).

If you want to give a dead line to schedule, you should
likely be doing, instead:

		usleep_range(delay, delay + delta);

this would tell the realtime clock that there's a dead line of
(schedule_delay + delta) microseconds.

You'll need to change the delay to be in microseconds too,
e. g., I guess that something similar to this:


    static int video_i2c_thread_vid_cap(void *priv)
    {
        struct video_i2c_data *data = priv;
	u64 delay;

	delay = div64_u64(1000000ULL * data->frame_interval.numerator,
			  data->frame_interval.denominator);

	set_freezable();

        do {
...
		usleep_range(delay * 3 / 4, delay);
	} while (!kthread_should_stop());

	return 0;
    }

would give you what you want.

Regards,
Mauro

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Mauro Carvalho Chehab]

Em Sat, 5 Jun 2021 16:00:28 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Em Sat,  5 Jun 2021 20:54:56 +0900
> Seongyong Park <euphoriccatface@gmail.com> escreveu:
> 
> > Current implementation calculates frame delay based on the time of
> > start of the loop. This inevitably causes the loop delay to be
> > slightly longer than the actual measurement period, thus skipping a frame
> > every now and then.
> > 
> > However, MLX90640 should ideally be working without a frame skip.
> > Each measurement step updates half of the pixels in the frame
> > (every other pixel in default "chess mode", and every other row
> > in "interleave mode"), while additional coefficient data (25th & 26th row)
> > updates every step. The compensational coefficient data only corresponds
> > with the pixels updated in the same step.
> > 
> > In short, if a frame is skipped, then half of a frame loses correction
> > information and becomes garbage data.
> > 
> > Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 0465832a4..2ccb08335 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
> >  	struct video_i2c_data *data = priv;
> >  	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
> >  				       data->frame_interval.denominator);
> > +	unsigned long end_jiffies = jiffies;
> >  
> >  	set_freezable();
> >  
> >  	do {
> > -		unsigned long start_jiffies = jiffies;
> >  		struct video_i2c_buffer *vid_cap_buf = NULL;
> >  		int schedule_delay;
> >  
> > +		end_jiffies += delay;
> > +
> >  		try_to_freeze();
> >  
> >  		spin_lock(&data->slock);
> > @@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
> >  				VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> >  		}
> >  
> > -		schedule_delay = delay - (jiffies - start_jiffies);
> > -
> > -		if (time_after(jiffies, start_jiffies + delay))
> > -			schedule_delay = delay;  
> 
> > +		schedule_delay = end_jiffies - jiffies;
> > +		while (schedule_delay <= 0)
> > +			schedule_delay += delay;  
> 
> Huh? Why do you need a loop for that? If you want to make it positive,
> you could just do:
> 
> 	if (schedule_delay < 0)
> 		schedule_delay = delay; /* or something else */
> 
> but this won't solve the issue, as this is basically equivalent
> to the logic you removed.
> 
> >  
> >  		schedule_timeout_interruptible(schedule_delay);  
> 
> This is probably what's causing troubles, as this only ensures
> that it will sleep for at least schedule_delay (if not
> interrupted).
> 
> If you want to give a dead line to schedule, you should
> likely be doing, instead:
> 
> 		usleep_range(delay, delay + delta);
> 
> this would tell the realtime clock that there's a dead line of
> (schedule_delay + delta) microseconds.
> 
> You'll need to change the delay to be in microseconds too,
> e. g., I guess that something similar to this:
> 
> 
>     static int video_i2c_thread_vid_cap(void *priv)
>     {
>         struct video_i2c_data *data = priv;
> 	u64 delay;
> 
> 	delay = div64_u64(1000000ULL * data->frame_interval.numerator,
> 			  data->frame_interval.denominator);
> 
> 	set_freezable();
> 
>         do {
> ...
> 		usleep_range(delay * 3 / 4, delay);
> 	} while (!kthread_should_stop());
> 
> 	return 0;
>     }
> 
> would give you what you want.

The actual the logic should be more complex, as it needs to
dynamically adjust the delay based on how much frames were streamed
and how much time it was elapsed, but the basically idea is that
you would need to use:

	usleep_range(min_delay_us, max_delay_us);

instead of:

	schedule_timeout_interruptible(schedule_delay);

in order to tell the realtime clock about a dead line for
sleeping.

Thanks,
Mauro

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Seongyong Park]

2021년 6월 5일 (토) 오후 11:53, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
> you would need to use:
>
>         usleep_range(min_delay_us, max_delay_us);
>
> instead of:
>
>         schedule_timeout_interruptible(schedule_delay);
>
> in order to tell the realtime clock about a dead line for
> sleeping.
>
> Thanks,
> Mauro

Okay, I have tried `usleep_range()` instead, and it indeed shows
improvement in the frame rate.
Now it's basically the same as before my patch, except for
`jiffies_to_usecs()` and then `usleep_range()`.

...
         int schedule_delay;
+        uint64_t schedule_delay_us;

        try_to_freeze();
...
        if (time_after(jiffies, start_jiffies + delay))
            schedule_delay = delay;

-        schedule_timeout_interruptible(schedule_delay);
+        schedule_delay_us = jiffies_to_usecs(schedule_delay);
+        usleep_range(schedule_delay_us * 3/4, schedule_delay_us);
     } while (!kthread_should_stop());

     return 0;
...

I decided to keep the `if (...) schedule_delay = delay;` part.
The concern was that my RPi Zero was having quite a bit of constant
drift, like showing 3FPS when set to 4FPS, 6FPS when 8FPS, 10FPS when
16FPS, and so on.
Now that I've confirmed the timing's good enough (usually ~0.5 FPS
faster than the frame rate given), there's no need for me to bother
anymore.

I'll send another patchset if it doesn't look too bad.

Thank you very much.
Seongyong Park

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Mauro Carvalho Chehab]

Em Sun, 6 Jun 2021 16:20:53 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> 2021년 6월 5일 (토) 오후 11:53, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
> > you would need to use:
> >
> >         usleep_range(min_delay_us, max_delay_us);
> >
> > instead of:
> >
> >         schedule_timeout_interruptible(schedule_delay);
> >
> > in order to tell the realtime clock about a dead line for
> > sleeping.
> >
> > Thanks,
> > Mauro  
> 
> Okay, I have tried `usleep_range()` instead, and it indeed shows
> improvement in the frame rate.
> Now it's basically the same as before my patch, except for
> `jiffies_to_usecs()` and then `usleep_range()`.
> 
> ...
>          int schedule_delay;
> +        uint64_t schedule_delay_us;
> 
>         try_to_freeze();
> ...
>         if (time_after(jiffies, start_jiffies + delay))
>             schedule_delay = delay;
> 
> -        schedule_timeout_interruptible(schedule_delay);
> +        schedule_delay_us = jiffies_to_usecs(schedule_delay);
> +        usleep_range(schedule_delay_us * 3/4, schedule_delay_us);
>      } while (!kthread_should_stop());
> 
>      return 0;
> ...
> 
> I decided to keep the `if (...) schedule_delay = delay;` part.

Yeah, you would need something like that.

> The concern was that my RPi Zero was having quite a bit of constant
> drift, like showing 3FPS when set to 4FPS, 6FPS when 8FPS, 10FPS when
> 16FPS, and so on.
> Now that I've confirmed the timing's good enough (usually ~0.5 FPS
> faster than the frame rate given), there's no need for me to bother
> anymore.

In other to avoid the drift, the logic needs to calculate the delay based
on something like this (pseudo-C) code:

	start_jiffies = jiffies;
	frame_count = 0;
	i = 0;
	do {
		i++;
		delay = jiffies - (start_jiffies * i * <interval>);
...
	}

The actual logic should probably avoid multiplying stuff there, as this
could go sideways easily due to overflows.

Perhaps something like this would work better, keeping a more precise
average fps rate:

	next_jiffies = jiffies + delay;
	do {
...
		schedule_delay_us = jiffies_to_usecs(next_jiffies - jiffies);
		usleep_range(schedule_delay_us * 3/4, schedule_delay_us);		...
		next_jiffies += delay;
	}

> 
> I'll send another patchset if it doesn't look too bad.
> 
> Thank you very much.
> Seongyong Park



Thanks,
Mauro

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Seongyong Park]

`2021년 6월 6일 (일) 오후 8:00, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
>
>
> Perhaps something like this would work better, keeping a more precise
> average fps rate:
>
>         next_jiffies = jiffies + delay;
>         do {
> ...
>                 schedule_delay_us = jiffies_to_usecs(next_jiffies - jiffies);
>                 usleep_range(schedule_delay_us * 3/4, schedule_delay_us);               ...
>                 next_jiffies += delay;
>         }
>
> >
> > I'll send another patchset if it doesn't look too bad.
> >
> > Thank you very much.
> > Seongyong Park
>
>
>
> Thanks,
> Mauro

For a few hours, I couldn't get the module to make precise timing.
It would always be a few tenths of FPS higher than I set, regardless
of how I construct the calculation.
And then it hit me, maybe jiffies is not granular enough - and of
course, resolution of jiffies turns out to be 100Hz :P

e.g. If you set it 16FPS, the count of jiffies to sleep results 100 / 16 = 6
The sleep interval ends up being 60ms, resulting in 16.666... FPS.
This discrepancy gets quite big if you set it to 64 FPS, resulting in
a single jiffies count, i.e. 100Hz.
(Though you will need to either skip some data, or set I2C baud rate
beyond the sensor's spec
in order to realistically even achieve 64 FPS, at least on an RPi Zero
it seems.)

So, I basically changed the time source from `jiffies` to
`ktime_to_us(ktime_get())`.
The timing is definitely better now :)

One last question please, before creating another patchset.
>                 usleep_range(schedule_delay_us * 3/4, schedule_delay_us);
Is it okay to make it essentially 0 microseconds sleep here, by
setting `schedule_delay_us` to 0?
It's going to be like,
```
if (current_us > end_us) {
    end_us = current_us;
}
schedule_delay_us = end_us - current_us;
```

Regards,
Seongyong Park

Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Mauro Carvalho Chehab]

Em Mon, 7 Jun 2021 00:06:36 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> `2021년 6월 6일 (일) 오후 8:00, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
> >
> >
> > Perhaps something like this would work better, keeping a more precise
> > average fps rate:
> >
> >         next_jiffies = jiffies + delay;
> >         do {
> > ...
> >                 schedule_delay_us = jiffies_to_usecs(next_jiffies - jiffies);
> >                 usleep_range(schedule_delay_us * 3/4, schedule_delay_us);               ...
> >                 next_jiffies += delay;
> >         }
> >  
> > >
> > > I'll send another patchset if it doesn't look too bad.
> > >
> > > Thank you very much.
> > > Seongyong Park  
> >
> >
> >
> > Thanks,
> > Mauro  
> 
> For a few hours, I couldn't get the module to make precise timing.
> It would always be a few tenths of FPS higher than I set, regardless
> of how I construct the calculation.
> And then it hit me, maybe jiffies is not granular enough - and of
> course, resolution of jiffies turns out to be 100Hz :P

It is actually worse than that, as it depends on CONFIG_HZ, which is
set by the one who built the Kernel. So, on other machines, it could
be higher (like 1200) or lower (24 is one of the options on mips) ;-)

> e.g. If you set it 16FPS, the count of jiffies to sleep results 100 / 16 = 6
> The sleep interval ends up being 60ms, resulting in 16.666... FPS.

That's basically why I suggested you to count the time from the start
of streaming, instead of just waiting for a fixed delay every time.

> This discrepancy gets quite big if you set it to 64 FPS, resulting in
> a single jiffies count, i.e. 100Hz.
> (Though you will need to either skip some data, or set I2C baud rate
> beyond the sensor's spec
> in order to realistically even achieve 64 FPS, at least on an RPi Zero
> it seems.)

The issue is probably not RPi zero, but the low speeds of I2C bus,
and the speed of the sensor. 

> So, I basically changed the time source from `jiffies` to
> `ktime_to_us(ktime_get())`.
> The timing is definitely better now :)

This helps but it probably use a lot more CPU cycles than reading
jiffies.

> One last question please, before creating another patchset.
> >                 usleep_range(schedule_delay_us * 3/4, schedule_delay_us);  
> Is it okay to make it essentially 0 microseconds sleep here, by
> setting `schedule_delay_us` to 0?
> It's going to be like,
> ```
> if (current_us > end_us) {
>     end_us = current_us;
> }
> schedule_delay_us = end_us - current_us;
> ```

Not sure, but you could instead revert the logic, like:

if (current_us <= end_us)
	usleep_range();

Thanks,
Mauro