Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: video-i2c: add mlx90640 subpage data to output
@ 2019-08-11  7:10 Matt Ranostay
  2019-08-12 13:05 ` Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Matt Ranostay @ 2019-08-11  7:10 UTC (permalink / raw)
  To: linux-media; +Cc: Matt Ranostay

Add current subpage data via the status register to the video
frame in the last word of data, which seems to be unused
undocumented reserved data.

Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/media/i2c/video-i2c.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 078141712c88..8bc7b228ba40 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -168,8 +168,16 @@ 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);
+	int size = data->chip->bpp / 8;
+
+	if (ret)
+		return ret;
+
+	/* read status register, which contains subpage that is read */
+	return regmap_bulk_read(data->regmap, 0x8000,
+				&buf[data->chip->buffer_size - size], size);
 }
 
 static int amg88xx_setup(struct video_i2c_data *data)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: video-i2c: add mlx90640 subpage data to output
  2019-08-11  7:10 [PATCH] media: video-i2c: add mlx90640 subpage data to output Matt Ranostay
@ 2019-08-12 13:05 ` Hans Verkuil
  2019-08-15  6:23   ` Matt Ranostay
  2019-08-21  5:59   ` Matt Ranostay
  0 siblings, 2 replies; 6+ messages in thread
From: Hans Verkuil @ 2019-08-12 13:05 UTC (permalink / raw)
  To: Matt Ranostay, linux-media

Hi Matt,

On 8/11/19 9:10 AM, Matt Ranostay wrote:
> Add current subpage data via the status register to the video
> frame in the last word of data, which seems to be unused
> undocumented reserved data.

I don't really understand from this description what is going on
here.

mlx90640_xfer() reads the buffer data from the i2c device, but
that data is split over two different addresses? Or does
0x8000 contain something else with a different meaning compared
to what is read from 0x400?

> 
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/media/i2c/video-i2c.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 078141712c88..8bc7b228ba40 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -168,8 +168,16 @@ 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);

Shouldn't this be data->chip->buffer_size - size, since the last
'size' bytes will be overwritten anyway?

> +	int size = data->chip->bpp / 8;
> +
> +	if (ret)
> +		return ret;
> +
> +	/* read status register, which contains subpage that is read */
> +	return regmap_bulk_read(data->regmap, 0x8000,
> +				&buf[data->chip->buffer_size - size], size);
>  }
>  
>  static int amg88xx_setup(struct video_i2c_data *data)
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: video-i2c: add mlx90640 subpage data to output
  2019-08-12 13:05 ` Hans Verkuil
@ 2019-08-15  6:23   ` Matt Ranostay
  2019-08-16  7:57     ` Hans Verkuil
  2019-08-21  5:59   ` Matt Ranostay
  1 sibling, 1 reply; 6+ messages in thread
From: Matt Ranostay @ 2019-08-15  6:23 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

On Mon, Aug 12, 2019 at 6:05 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Matt,
>
> On 8/11/19 9:10 AM, Matt Ranostay wrote:
> > Add current subpage data via the status register to the video
> > frame in the last word of data, which seems to be unused
> > undocumented reserved data.
>
> I don't really understand from this description what is going on
> here.

Probably can make it more verbose if requested.

But simple overview is that all the frame data is from 0x400 read but
there is a some status register data that is from 0x8000
that needs to be read.

So mostly from the latter read (which can have an unlikely race
condition and be incorrect) is to confirm which sub frame that is
being read which can be one or two. You need the previous subpage
magic values to process that respective frame temperature data.

>
> mlx90640_xfer() reads the buffer data from the i2c device, but
> that data is split over two different addresses? Or does
> 0x8000 contain something else with a different meaning compared
> to what is read from 0x400?
>
> >
> > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 078141712c88..8bc7b228ba40 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -168,8 +168,16 @@ 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);
>
> Shouldn't this be data->chip->buffer_size - size, since the last
> 'size' bytes will be overwritten anyway?

Yes that is true but don't see in datasheet that you can just not read
the last two bytes of the transaction.
Probably totally fine to not do so but who knows.

- Matt

>
> > +     int size = data->chip->bpp / 8;
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* read status register, which contains subpage that is read */
> > +     return regmap_bulk_read(data->regmap, 0x8000,
> > +                             &buf[data->chip->buffer_size - size], size);
> >  }
> >
> >  static int amg88xx_setup(struct video_i2c_data *data)
> >
>
> Regards,
>
>         Hans

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: video-i2c: add mlx90640 subpage data to output
  2019-08-15  6:23   ` Matt Ranostay
@ 2019-08-16  7:57     ` Hans Verkuil
  0 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2019-08-16  7:57 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: linux-media

On 8/15/19 8:23 AM, Matt Ranostay wrote:
> On Mon, Aug 12, 2019 at 6:05 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Matt,
>>
>> On 8/11/19 9:10 AM, Matt Ranostay wrote:
>>> Add current subpage data via the status register to the video
>>> frame in the last word of data, which seems to be unused
>>> undocumented reserved data.
>>
>> I don't really understand from this description what is going on
>> here.
> 
> Probably can make it more verbose if requested.
> 
> But simple overview is that all the frame data is from 0x400 read but
> there is a some status register data that is from 0x8000
> that needs to be read.
> 
> So mostly from the latter read (which can have an unlikely race
> condition and be incorrect) is to confirm which sub frame that is
> being read which can be one or two. You need the previous subpage
> magic values to process that respective frame temperature data.

But 1) you are overwriting the last pixel value of the frame with this
status value and 2) it is undocumented so how would applications handle
this?

As far as I can tell you are just replacing the last 'pixel' in the
buffer with a magic value without explaining what it is or what userspace
should do with it. And exposing low-level register information to userspace
is almost always wrong.

So: what is this status value? How should it be used?

Regards,

	Hans

> 
>>
>> mlx90640_xfer() reads the buffer data from the i2c device, but
>> that data is split over two different addresses? Or does
>> 0x8000 contain something else with a different meaning compared
>> to what is read from 0x400?
>>
>>>
>>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
>>> ---
>>>  drivers/media/i2c/video-i2c.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
>>> index 078141712c88..8bc7b228ba40 100644
>>> --- a/drivers/media/i2c/video-i2c.c
>>> +++ b/drivers/media/i2c/video-i2c.c
>>> @@ -168,8 +168,16 @@ 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);
>>
>> Shouldn't this be data->chip->buffer_size - size, since the last
>> 'size' bytes will be overwritten anyway?
> 
> Yes that is true but don't see in datasheet that you can just not read
> the last two bytes of the transaction.
> Probably totally fine to not do so but who knows.
> 
> - Matt
> 
>>
>>> +     int size = data->chip->bpp / 8;
>>> +
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* read status register, which contains subpage that is read */
>>> +     return regmap_bulk_read(data->regmap, 0x8000,
>>> +                             &buf[data->chip->buffer_size - size], size);
>>>  }
>>>
>>>  static int amg88xx_setup(struct video_i2c_data *data)
>>>
>>
>> Regards,
>>
>>         Hans


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: video-i2c: add mlx90640 subpage data to output
  2019-08-12 13:05 ` Hans Verkuil
  2019-08-15  6:23   ` Matt Ranostay
@ 2019-08-21  5:59   ` Matt Ranostay
  2019-08-21  8:14     ` Hans Verkuil
  1 sibling, 1 reply; 6+ messages in thread
From: Matt Ranostay @ 2019-08-21  5:59 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

On Mon, Aug 12, 2019 at 6:05 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Matt,
>
> On 8/11/19 9:10 AM, Matt Ranostay wrote:
> > Add current subpage data via the status register to the video
> > frame in the last word of data, which seems to be unused
> > undocumented reserved data.
>
> I don't really understand from this description what is going on
> here.
>
> mlx90640_xfer() reads the buffer data from the i2c device, but
> that data is split over two different addresses? Or does
> 0x8000 contain something else with a different meaning compared
> to what is read from 0x400?

So the status register contains the bit on if subpage one or two is in
the reading, so in reality a 8 fps reading in v4l2
space is only a 4 fps processed images.

This is important for userspace processing because it needs to know it
has a subpage one and two to process the thermal data.
Can't do it with subpage two and one in that order per the algorithms,
and also since it is polling it is possible a frame will be dropped.

- Matt

>
> >
> > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 078141712c88..8bc7b228ba40 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -168,8 +168,16 @@ 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);
>
> Shouldn't this be data->chip->buffer_size - size, since the last
> 'size' bytes will be overwritten anyway?
>
> > +     int size = data->chip->bpp / 8;
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* read status register, which contains subpage that is read */
> > +     return regmap_bulk_read(data->regmap, 0x8000,
> > +                             &buf[data->chip->buffer_size - size], size);
> >  }
> >
> >  static int amg88xx_setup(struct video_i2c_data *data)
> >
>
> Regards,
>
>         Hans

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] media: video-i2c: add mlx90640 subpage data to output
  2019-08-21  5:59   ` Matt Ranostay
@ 2019-08-21  8:14     ` Hans Verkuil
  0 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2019-08-21  8:14 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: linux-media

On 8/21/19 7:59 AM, Matt Ranostay wrote:
> On Mon, Aug 12, 2019 at 6:05 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Matt,
>>
>> On 8/11/19 9:10 AM, Matt Ranostay wrote:
>>> Add current subpage data via the status register to the video
>>> frame in the last word of data, which seems to be unused
>>> undocumented reserved data.
>>
>> I don't really understand from this description what is going on
>> here.
>>
>> mlx90640_xfer() reads the buffer data from the i2c device, but
>> that data is split over two different addresses? Or does
>> 0x8000 contain something else with a different meaning compared
>> to what is read from 0x400?
> 
> So the status register contains the bit on if subpage one or two is in
> the reading, so in reality a 8 fps reading in v4l2
> space is only a 4 fps processed images.

Isn't it the job of the driver to ensure both subpages are read in the
correct order before returning the full frame? It makes no sense to
off-load this to userspace, this definitely is a job for the driver.

Also, you report V4L2_PIX_FMT_Y16_BE as the format, but it clearly
isn't luma data at all, it is a specialized format which includes
metadata as well. This needs a new pixel format, just for this device.
Luckily the datasheet is public, so you can refer to it, but you can't
just use Y16_BE here.

There is a similar issue with V4L2_PIX_FMT_Y12 for the other device.
It is a much simpler format, but it still needs its own pixelformat.
If nothing else, these values are signed, not unsigned.

Regards,

	Hans

> 
> This is important for userspace processing because it needs to know it
> has a subpage one and two to process the thermal data.
> Can't do it with subpage two and one in that order per the algorithms,
> and also since it is polling it is possible a frame will be dropped.
> 
> - Matt
> 
>>
>>>
>>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
>>> ---
>>>  drivers/media/i2c/video-i2c.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
>>> index 078141712c88..8bc7b228ba40 100644
>>> --- a/drivers/media/i2c/video-i2c.c
>>> +++ b/drivers/media/i2c/video-i2c.c
>>> @@ -168,8 +168,16 @@ 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);
>>
>> Shouldn't this be data->chip->buffer_size - size, since the last
>> 'size' bytes will be overwritten anyway?
>>
>>> +     int size = data->chip->bpp / 8;
>>> +
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* read status register, which contains subpage that is read */
>>> +     return regmap_bulk_read(data->regmap, 0x8000,
>>> +                             &buf[data->chip->buffer_size - size], size);
>>>  }
>>>
>>>  static int amg88xx_setup(struct video_i2c_data *data)
>>>
>>
>> Regards,
>>
>>         Hans


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-11  7:10 [PATCH] media: video-i2c: add mlx90640 subpage data to output Matt Ranostay
2019-08-12 13:05 ` Hans Verkuil
2019-08-15  6:23   ` Matt Ranostay
2019-08-16  7:57     ` Hans Verkuil
2019-08-21  5:59   ` Matt Ranostay
2019-08-21  8:14     ` Hans Verkuil

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox