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@xxxxxx>님이 작성:
>
> 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@xxxxxxxxx>
> > >> ---
> > >> 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
