diff mbox series

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

Message ID 20210516110902.784-1-euphoriccatface@gmail.com
State Superseded
Headers show
Series [1/2] media: video-i2c: frame delay based on last frame's end time | expand

Commit Message

Seongyong Park May 16, 2021, 11:09 a.m. UTC 
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(-)

--
2.31.1

Comments

Matt Ranostay May 16, 2021, 8:48 p.m. UTC | #1 
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
>
Seongyong Park May 17, 2021, 1:39 a.m. UTC | #2 
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

> >
Matt Ranostay May 17, 2021, 11:40 p.m. UTC | #3 
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

> > >
diff mbox series

Patch

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());