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
