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

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

[Seongyong Park]

Thanks to Matt Ranostay and Mauro Carvalho Chehab (mchehab) for guidance

Seongyong Park (2):
  media: video-i2c: more precise intervals between frames
  media: video-i2c: append register data on MLX90640's frame

 drivers/media/i2c/video-i2c.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH v3 1/2] media: video-i2c: more precise intervals between frames

[Seongyong Park]

MLX90640 should ideally be working without a frame skip.
In short, if a frame is skipped, then half of a frame loses correction
information, having no way to retrieve its original compensation.

This patch improves the timing in three ways:

1) Replaced schedule_timeout_interruptible() to usleep_range()
The former "only ensures that it will sleep for at least
schedule_delay (if not interrupted)", as pointed out by mchehab.
As a result, the frame rate could lag behind than the actual capability
of the hardware
(Raspberry Pi would show a few Hz slower than set value)

2) Calculation based on us, not jiffies
Jiffies usually has resolution of 100Hz, and possibly even cruder.
MLX90640 can go up to 64Hz frame rate, which does not make sense to
calculate the interval with aforementioned resolution.

3) Interval calculation based on the last frame's end time
Using the start time of the current frame will probably make tiny bit
of drift every time. This made more sense when I didn't realize 1),
but it still makes sense without adding virtually any complexity,
so this stays in.

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

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 0465832a4..64ba96329 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -443,14 +443,15 @@ static void buffer_queue(struct vb2_buffer *vb)
 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);
+	u32 delay = mult_frac(1000000UL, data->frame_interval.numerator,
+			       data->frame_interval.denominator);
+	s64 end_us = ktime_to_us(ktime_get());
 
 	set_freezable();
 
 	do {
-		unsigned long start_jiffies = jiffies;
 		struct video_i2c_buffer *vid_cap_buf = NULL;
+		s64 current_us;
 		int schedule_delay;
 
 		try_to_freeze();
@@ -477,12 +478,14 @@ 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_timeout_interruptible(schedule_delay);
+		end_us += delay;
+		current_us = ktime_to_us(ktime_get());
+		if (current_us < end_us) {
+			schedule_delay = end_us - current_us;
+			usleep_range(schedule_delay * 3/4, schedule_delay);
+		} else {
+			end_us = current_us;
+		}
 	} while (!kthread_should_stop());
 
 	return 0;
-- 
2.31.1

Re: [PATCH v3 1/2] media: video-i2c: more precise intervals between frames

[Matt Ranostay]

On Tue, Jun 8, 2021 at 8:25 AM Seongyong Park <euphoriccatface@gmail.com> wrote:
>
> MLX90640 should ideally be working without a frame skip.
> In short, if a frame is skipped, then half of a frame loses correction
> information, having no way to retrieve its original compensation.
>
> This patch improves the timing in three ways:
>
> 1) Replaced schedule_timeout_interruptible() to usleep_range()
> The former "only ensures that it will sleep for at least
> schedule_delay (if not interrupted)", as pointed out by mchehab.
> As a result, the frame rate could lag behind than the actual capability
> of the hardware
> (Raspberry Pi would show a few Hz slower than set value)
>
> 2) Calculation based on us, not jiffies
> Jiffies usually has resolution of 100Hz, and possibly even cruder.
> MLX90640 can go up to 64Hz frame rate, which does not make sense to
> calculate the interval with aforementioned resolution.
>
> 3) Interval calculation based on the last frame's end time
> Using the start time of the current frame will probably make tiny bit
> of drift every time. This made more sense when I didn't realize 1),
> but it still makes sense without adding virtually any complexity,
> so this stays in.
>
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 0465832a4..64ba96329 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -443,14 +443,15 @@ static void buffer_queue(struct vb2_buffer *vb)
>  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);
> +       u32 delay = mult_frac(1000000UL, data->frame_interval.numerator,
> +                              data->frame_interval.denominator);
> +       s64 end_us = ktime_to_us(ktime_get());
>
>         set_freezable();
>
>         do {
> -               unsigned long start_jiffies = jiffies;
>                 struct video_i2c_buffer *vid_cap_buf = NULL;
> +               s64 current_us;
>                 int schedule_delay;
>
>                 try_to_freeze();
> @@ -477,12 +478,14 @@ 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_timeout_interruptible(schedule_delay);
> +               end_us += delay;
> +               current_us = ktime_to_us(ktime_get());
> +               if (current_us < end_us) {
> +                       schedule_delay = end_us - current_us;
> +                       usleep_range(schedule_delay * 3/4, schedule_delay);

Looks fine to me.. not sure I like the 3/4 but the only way to do it
without a floating point of 0.75 which
is evil and would kick off a compiler warning anyway.

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

> +               } else {
> +                       end_us = current_us;
> +               }
>         } while (!kthread_should_stop());
>
>         return 0;
> --
> 2.31.1
>

[PATCH v3 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 64ba96329..2b50a76f3 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

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

[Matt Ranostay]

On Tue, Jun 8, 2021 at 8:25 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 | 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 64ba96329..2b50a76f3 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 */

Guess you hit the 80 character line here and checkpatch.pl complained
.. But should all be one line since it is
much more clear on one line.

>  };
>
>  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,

Minus nitpick above looks good to me. You can keep the acked-by if
that is only change

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


>                 .bpp            = 16,
>                 .regmap_config  = &mlx90640_regmap_config,
>                 .nvmem_config   = &mlx90640_nvram_config,
> --
> 2.31.1
>

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

[Seongyong Park]

2021년 6월 10일 (목) 오전 8:13, Seongyong Park <euphoriccatface@gmail.com>님이 작성:
>
> 2021년 6월 9일 (수) 오후 4:14, Matt Ranostay <matt.ranostay@konsulko.com>님이 작성:
> >
> > On Tue, Jun 8, 2021 at 8:25 AM Seongyong Park <euphoriccatface@gmail.com> wrote:
> >
> > > -       .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 */
> >
> > Guess you hit the 80 character line here and checkpatch.pl complained
> > .. But should all be one line since it is
> > much more clear on one line.
> >
> > >  };
> > >
> > >  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,
> >
> > Minus nitpick above looks good to me. You can keep the acked-by if
> > that is only change
> >
> > Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
> >
> Yes, other than your suggestion, indeed that is the only change I made
> for this commit between v1 and v2 (and v3 is the same as v2.)
> and that is because of the checkpatch.pl indeed :)
>
> Thanks,
> Seongyong Park

Re-sending this mail mainly because I have made a direct reply, rather
than a reply to all.
Sorry about that.

Matt, while we're at it, if I happen to make another revision of this patchset,
would you find it looking okay to make a line break after the second parameter?
The odd arrangement was partly because I wanted to make a line break
after the same number of parameters.

The lines will look like this:
+       int ret = regmap_bulk_read(data->regmap, 0x400,
+                                  buf, data->chip->buffer_size - 64);
...
+       return regmap_bulk_read(data->regmap, 0x8000,
+                               buf + (data->chip->buffer_size - 64), 64);

Thanks,
- Seongyong Park

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

[Matt Ranostay]

On Fri, Jun 11, 2021 at 10:55 PM Seongyong Park
<euphoriccatface@gmail.com> wrote:
>
> 2021년 6월 10일 (목) 오전 8:13, Seongyong Park <euphoriccatface@gmail.com>님이 작성:
> >
> > 2021년 6월 9일 (수) 오후 4:14, Matt Ranostay <matt.ranostay@konsulko.com>님이 작성:
> > >
> > > On Tue, Jun 8, 2021 at 8:25 AM Seongyong Park <euphoriccatface@gmail.com> wrote:
> > >
> > > > -       .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 */
> > >
> > > Guess you hit the 80 character line here and checkpatch.pl complained
> > > .. But should all be one line since it is
> > > much more clear on one line.
> > >
> > > >  };
> > > >
> > > >  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,
> > >
> > > Minus nitpick above looks good to me. You can keep the acked-by if
> > > that is only change
> > >
> > > Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > >
> > Yes, other than your suggestion, indeed that is the only change I made
> > for this commit between v1 and v2 (and v3 is the same as v2.)
> > and that is because of the checkpatch.pl indeed :)
> >
> > Thanks,
> > Seongyong Park
>
> Re-sending this mail mainly because I have made a direct reply, rather
> than a reply to all.
> Sorry about that.
>
> Matt, while we're at it, if I happen to make another revision of this patchset,
> would you find it looking okay to make a line break after the second parameter?
> The odd arrangement was partly because I wanted to make a line break
> after the same number of parameters.
>
> The lines will look like this:
> +       int ret = regmap_bulk_read(data->regmap, 0x400,
> +                                  buf, data->chip->buffer_size - 64);
> ...
> +       return regmap_bulk_read(data->regmap, 0x8000,
> +                               buf + (data->chip->buffer_size - 64), 64);
>

Yeah this is fine by me. Keep the signed off if this is the  only change.

- Matt

> Thanks,
> - Seongyong Park

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

[Sakari Ailus]

Hi Seongyong,

On Wed, Jun 09, 2021 at 12:24:51AM +0900, Seongyong Park 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 | 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 64ba96329..2b50a76f3 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 */

This device should actually use a multi-plane format as the data isn't
Y16_BE as the driver declares. That said, the format would be device
specific and using one would require specific support from applications.

I might just declare it produces fewer lines while an application that
knows the device could access the extra data close to the end of the
buffer.

An alternative would be to use that multi-plane format and keep driver
support for the plain Y16_BE as well. But I think this would be a bit
heavy-weight solution.

I wonder what Hans thinks.

>  };
>  
>  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),

Please wrap before 80 (unless there's a reason to do otherwise).

> +				64);
>  }
>  

-- 
Kind regards,

Sakari Ailus

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

[Hans Verkuil]

On 05/08/2021 16:55, Sakari Ailus wrote:
> Hi Seongyong,
> 
> On Wed, Jun 09, 2021 at 12:24:51AM +0900, Seongyong Park 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 | 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 64ba96329..2b50a76f3 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 */
> 
> This device should actually use a multi-plane format as the data isn't
> Y16_BE as the driver declares. That said, the format would be device
> specific and using one would require specific support from applications.
> 
> I might just declare it produces fewer lines while an application that
> knows the device could access the extra data close to the end of the
> buffer.
> 
> An alternative would be to use that multi-plane format and keep driver
> support for the plain Y16_BE as well. But I think this would be a bit
> heavy-weight solution.
> 
> I wonder what Hans thinks.

I think it is a bit overkill as well.

Wouldn't it be better to just add a new pixel format for this device, and
document it properly? I would keep the existing Y16_BE as it was to avoid
breaking userspace (since adding the extra line breaks the ABI IMHO), and
add a new format that properly documents how to parse the contents of the
buffer, which includes the last line with the status register.

Sorry for the late reply, it looks like an attempt was made to CC me, but
the email address was wrong, so I never received it. I only noticed this
when I started to review this patch.

Regards,

	Hans

> 
>>  };
>>  
>>  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),
> 
> Please wrap before 80 (unless there's a reason to do otherwise).
> 
>> +				64);
>>  }
>>  
> 

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

[Sakari Ailus]

Hi Hans, Seongyong,

On Mon, Sep 13, 2021 at 10:57:04AM +0200, Hans Verkuil wrote:
> On 05/08/2021 16:55, Sakari Ailus wrote:
> > Hi Seongyong,
> > 
> > On Wed, Jun 09, 2021 at 12:24:51AM +0900, Seongyong Park 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 | 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 64ba96329..2b50a76f3 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 */
> > 
> > This device should actually use a multi-plane format as the data isn't
> > Y16_BE as the driver declares. That said, the format would be device
> > specific and using one would require specific support from applications.
> > 
> > I might just declare it produces fewer lines while an application that
> > knows the device could access the extra data close to the end of the
> > buffer.
> > 
> > An alternative would be to use that multi-plane format and keep driver
> > support for the plain Y16_BE as well. But I think this would be a bit
> > heavy-weight solution.
> > 
> > I wonder what Hans thinks.
> 
> I think it is a bit overkill as well.
> 
> Wouldn't it be better to just add a new pixel format for this device, and
> document it properly? I would keep the existing Y16_BE as it was to avoid
> breaking userspace (since adding the extra line breaks the ABI IMHO), and
> add a new format that properly documents how to parse the contents of the
> buffer, which includes the last line with the status register.
> 
> Sorry for the late reply, it looks like an attempt was made to CC me, but
> the email address was wrong, so I never received it. I only noticed this
> when I started to review this patch.

Yeah, my mistake. I thought I had an alias for you but apparently not. I do
now.

I think that'd be fine, albeit equally heavy-weight. But also correct.
Documenting what's there is also important.

-- 
Regards,

Sakari Ailus

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

[Seongyong Park]

Thank you to everyone who gave some feedback.
I finally have found the time to get back to this. Sorry for the delay.

I tried gathering what I'd need to do if I wanted to add the suggested
new pixel format.
Since I'm learning a ton with this project on my own, I'd like some
help to figure things out.
Much appreciated if someone could confirm whether this looks right:

1. Modify any part in video-i2c that assume capability / stream
parameter / buffer type of *VIDEO_CAPTURE only
For example, the following part of `video_i2c_g_parm()`
```
if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
    return -EINVAL;
```

2. Make it possible for the videobuf2 queue to be configured for
BUF_TYPE_VIDEO_CAPTURE_MPLANE as well
Since the `vb2_queue_init()` is currently called inside
`video_i2c_probe()`, I assume the setup and initialization will have
to be moved to another place. Maybe into `start_streaming()`.

3. Add an entry to `videodev2.h`
I assume so because, with quick grep-ing, I couldn't find any pixel
format definitions under `drivers/media` but one
(drivers/media/usb/msi2500)
The entry would be named like V4L2_PIX_FMT_MLX9064X (MLX90641 seems to
have similar format)

4. Add a flag in `video_i2c_data` for the multiplane output, and add
appropriate code to fill in the second plane

...and then the rest will be adjusting the code, like getting the size
of data correct and so on.

Thanks in advance.
Seongyong Park

2021년 9월 13일 (월) 오후 7:05, Sakari Ailus <sakari.ailus@iki.fi>님이 작성:

>
> Hi Hans, Seongyong,
>
> On Mon, Sep 13, 2021 at 10:57:04AM +0200, Hans Verkuil wrote:
> > On 05/08/2021 16:55, Sakari Ailus wrote:
> > > Hi Seongyong,
> > >
> > > On Wed, Jun 09, 2021 at 12:24:51AM +0900, Seongyong Park 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 | 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 64ba96329..2b50a76f3 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 */
> > >
> > > This device should actually use a multi-plane format as the data isn't
> > > Y16_BE as the driver declares. That said, the format would be device
> > > specific and using one would require specific support from applications.
> > >
> > > I might just declare it produces fewer lines while an application that
> > > knows the device could access the extra data close to the end of the
> > > buffer.
> > >
> > > An alternative would be to use that multi-plane format and keep driver
> > > support for the plain Y16_BE as well. But I think this would be a bit
> > > heavy-weight solution.
> > >
> > > I wonder what Hans thinks.
> >
> > I think it is a bit overkill as well.
> >
> > Wouldn't it be better to just add a new pixel format for this device, and
> > document it properly? I would keep the existing Y16_BE as it was to avoid
> > breaking userspace (since adding the extra line breaks the ABI IMHO), and
> > add a new format that properly documents how to parse the contents of the
> > buffer, which includes the last line with the status register.
> >
> > Sorry for the late reply, it looks like an attempt was made to CC me, but
> > the email address was wrong, so I never received it. I only noticed this
> > when I started to review this patch.
>
> Yeah, my mistake. I thought I had an alias for you but apparently not. I do
> now.
>
> I think that'd be fine, albeit equally heavy-weight. But also correct.
> Documenting what's there is also important.
>
> --
> Regards,
>
> Sakari Ailus

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

[Matt Ranostay]

On Tue, Jun 8, 2021 at 8:25 AM Seongyong Park <euphoriccatface@gmail.com> wrote:
>
> Thanks to Matt Ranostay and Mauro Carvalho Chehab (mchehab) for guidance
>

Just one nitpick on future revisions. It is helpful to mention the
changes from from v1 to v2 makes it easier
for a quicker review.

- Matt

> Seongyong Park (2):
>   media: video-i2c: more precise intervals between frames
>   media: video-i2c: append register data on MLX90640's frame
>
>  drivers/media/i2c/video-i2c.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> --
> 2.31.1
>