atomisp current issues

atomisp current issues

[Mauro Carvalho Chehab]

Hi,

From what I've seen so far, those are the main issues with regards to V4L2 API,
in order to allow a generic V4L2 application to work with it.

MMAP support
============

Despite having some MMAP code on it, the current implementation is broken. 
Fixing it is not trivial, as it would require fixing the HMM support on it, 
which does several tricks.

The best would be to replace it by something simpler. If this is similar
enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
videodev2 + IPU3 HMM code.

As this is not trivial, I'm postponing such task. If someone has enough
time, it would be great to have this fixed.

From my side, I opted to add support for USERPTR on camorama:

	https://github.com/alessio/camorama

As this is something I wanted to do anyway, and it allowed me to cleanup
several things in camorama's code.

Support for USERPTR is not autodetected. So, this should be selected
via a command line parameter. Here (Fedora), I installed the build
dependencies on my test device with:

	$ sudo dnf builddep camorama
	$ sudo dnf install gnome-common  # for gnome-autogen.sh

Testing it with atomisp would be:

	$ git clone https://github.com/alessio/camorama
	$ cd camorama && ./autogen.sh
	$ make && ./src/camorama -d /dev/video2  --debug -U -D

In time:
--------

 Camorama currently doesn't work due to some other API bugs. See below.


VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT issues
===============================================

The implementation for VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT are not
properly implemented. It has several issues.

The main one is related to padding. Based on the comments inside the code,
the ISP 2 seems to require padding to work, both vertically an horizontally.

Those are controlled by two modprobe parameters: pad_h and pad_w.
The default for both is 16.

There are some other padding logic inside the function which sets the
video formats at atomisp_cmd: atomisp_set_fmt(). This function is quite
complex, being big and it calls several other atomisp kAPIs.

It basically queries the sensor, and then it mounts a pipeline that
will have the sensor + ISP blocks. Those ISP blocks convert the format
from Bayer into YUYV or RGB and scale down the resolution in order to
match the request.

It also adjusts the padding. The padding code there is very complex,
as it not only uses pad_h/pad_w, having things like:

	if (!atomisp_subdev_format_conversion(asd, source_pad)) {
		padding_w = 0;
		padding_h = 0;
	} else if (IS_BYT) {
		padding_w = 12;
		padding_h = 12;
	}
	...
	atomisp_get_dis_envelop(asd, f->fmt.pix.width, f->fmt.pix.height,
				&dvs_env_w, &dvs_env_h);
	...
	/*
	 * set format info to sensor
	 * In continuous mode, resolution is set only if it is higher than
	 * existing value. This because preview pipe will be configured after
	 * capture pipe and usually has lower resolution than capture pipe.
	 */
	if (!asd->continuous_mode->val ||
	    isp_sink_fmt->width < (f->fmt.pix.width + padding_w + dvs_env_w) ||
	    isp_sink_fmt->height < (f->fmt.pix.height + padding_h +
				    dvs_env_h)) {

Due to that, the sensors are set to have those extra 16 columns/lines.
Those extra data are consumed internally at the driver, so the output
buffer won't contain those.

Yet, the buffer size to be allocated by userspace on USERPTR mode is not just
width x height x bpp, but it may need extra space for such pads and/or other 
things.

In practice, when VIDIOC_S_FMT asks for a 1600x1200 resolution, it
actually sets 1616x1216 at the sensor (at the pad source), but the
pad sink provides the requested resolution: 1600x1200.

However, the resolution returned by VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT
is not always the sink resolution. Sometimes, it returns the sensor format.

Fixing it has been challenging.

-

Another related issue is that, when a resolution bigger than the maximum
resolution is requested, the driver doesn't return 1600x1200, but, instead,
a smaller one.

On other words, setting to YUYV 1600x1200 gives:

	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1600,height=1200 -d /dev/video2 --verbose
	VIDIOC_QUERYCAP: ok
	VIDIOC_G_FMT: ok
	VIDIOC_S_FMT: ok
	Format Video Capture:
		Width/Height      : 1600/1200
		Pixel Format      : 'YUYV' (YUYV 4:2:2)
		Field             : None
		Bytes per Line    : 3200
		Size Image        : 3842048
		Colorspace        : Rec. 709
		Transfer Function : Rec. 709
		YCbCr/HSV Encoding: Rec. 709
		Quantization      : Default (maps to Limited Range)
		Flags             : 

Setting to a higher resolution (which is a method that some apps use to test
for the max resolution, when VIDIOC_ENUM_FRAMESIZES is not available or
broken) produces:

	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=10000,height=10000 -d /dev/video2 --verbose
	VIDIOC_QUERYCAP: ok
	VIDIOC_G_FMT: ok
	VIDIOC_S_FMT: ok
	Format Video Capture:
		Width/Height      : 1600/900
		Pixel Format      : 'YUYV' (YUYV 4:2:2)
		Field             : None
		Bytes per Line    : 3200
		Size Image        : 2883584
		Colorspace        : Rec. 709
		Transfer Function : Rec. 709
		YCbCr/HSV Encoding: Rec. 709
		Quantization      : Default (maps to Limited Range)
		Flags             : 

Trying to set to the sensor resolution is even worse, as it returns EINVAL:

	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1616,height=1216 -d /dev/video2 --verbose
	VIDIOC_QUERYCAP: ok
	VIDIOC_G_FMT: ok
	VIDIOC_S_FMT: failed: Invalid argument

The logs for such case are:

	[ 4799.594724] atomisp-isp2 0000:00:03.0: can't create streams
	[ 4799.594757] atomisp-isp2 0000:00:03.0: __get_frame_info 1616x1216 (padded to 0) returned -22
	[ 4799.594781] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22

Video devices
=============

Currently, 10 video? devices are created:

	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
	/dev/video3:	Name             : ATOMISP ISP VIDEO output
	/dev/video4:	Name             : ATOMISP ISP ACC
	/dev/video5:	Name             : ATOMISP ISP MEMORY input
	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
	/dev/video9:	Name             : ATOMISP ISP VIDEO output
	/dev/video10:	Name             : ATOMISP ISP ACC

That seems to be written to satisfy some Android-based app, but we don't
really need all of those.

I'm thinking to comment out the part of the code which creates all of those,
keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
of those.

Comments?

Regards,
Mauro

Re: atomisp current issues

[Andy Shevchenko]

On Wed, Nov 3, 2021 at 3:54 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:

...

> Currently, 10 video? devices are created:
>
>         $ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
>         /dev/video0:    Name             : ATOMISP ISP CAPTURE output
>         /dev/video1:    Name             : ATOMISP ISP VIEWFINDER output
>         /dev/video2:    Name             : ATOMISP ISP PREVIEW output
>         /dev/video3:    Name             : ATOMISP ISP VIDEO output
>         /dev/video4:    Name             : ATOMISP ISP ACC
>         /dev/video5:    Name             : ATOMISP ISP MEMORY input
>         /dev/video6:    Name             : ATOMISP ISP CAPTURE output
>         /dev/video7:    Name             : ATOMISP ISP VIEWFINDER output
>         /dev/video8:    Name             : ATOMISP ISP PREVIEW output
>         /dev/video9:    Name             : ATOMISP ISP VIDEO output
>         /dev/video10:   Name             : ATOMISP ISP ACC
>
> That seems to be written to satisfy some Android-based app, but we don't
> really need all of those.
>
> I'm thinking to comment out the part of the code which creates all of those,
> keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> of those.

Are they using the same microprograms (named routines) in the firmware?

-- 
With Best Regards,
Andy Shevchenko

Re: atomisp current issues

[Hans Verkuil]

On 03/11/2021 14:54, Mauro Carvalho Chehab wrote:
> Hi,
> 
> From what I've seen so far, those are the main issues with regards to V4L2 API,
> in order to allow a generic V4L2 application to work with it.
> 
> MMAP support
> ============
> 
> Despite having some MMAP code on it, the current implementation is broken. 
> Fixing it is not trivial, as it would require fixing the HMM support on it, 
> which does several tricks.
> 
> The best would be to replace it by something simpler. If this is similar
> enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> videodev2 + IPU3 HMM code.
> 
> As this is not trivial, I'm postponing such task. If someone has enough
> time, it would be great to have this fixed.
> 
> From my side, I opted to add support for USERPTR on camorama:
> 
> 	https://github.com/alessio/camorama
> 
> As this is something I wanted to do anyway, and it allowed me to cleanup
> several things in camorama's code.
> 
> Support for USERPTR is not autodetected. So, this should be selected

You can autodetect this: the capabilities field returned by VIDIOC_REQBUFS
or VIDIOC_CREATE_BUFS will indicate support for this. This works with any
vb2-based driver.

Just thought I should mention this...

Regards,

	Hans

> via a command line parameter. Here (Fedora), I installed the build
> dependencies on my test device with:
> 
> 	$ sudo dnf builddep camorama
> 	$ sudo dnf install gnome-common  # for gnome-autogen.sh
> 
> Testing it with atomisp would be:
> 
> 	$ git clone https://github.com/alessio/camorama
> 	$ cd camorama && ./autogen.sh
> 	$ make && ./src/camorama -d /dev/video2  --debug -U -D
> 
> In time:
> --------
> 
>  Camorama currently doesn't work due to some other API bugs. See below.
> 
> 
> VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT issues
> ===============================================
> 
> The implementation for VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT are not
> properly implemented. It has several issues.
> 
> The main one is related to padding. Based on the comments inside the code,
> the ISP 2 seems to require padding to work, both vertically an horizontally.
> 
> Those are controlled by two modprobe parameters: pad_h and pad_w.
> The default for both is 16.
> 
> There are some other padding logic inside the function which sets the
> video formats at atomisp_cmd: atomisp_set_fmt(). This function is quite
> complex, being big and it calls several other atomisp kAPIs.
> 
> It basically queries the sensor, and then it mounts a pipeline that
> will have the sensor + ISP blocks. Those ISP blocks convert the format
> from Bayer into YUYV or RGB and scale down the resolution in order to
> match the request.
> 
> It also adjusts the padding. The padding code there is very complex,
> as it not only uses pad_h/pad_w, having things like:
> 
> 	if (!atomisp_subdev_format_conversion(asd, source_pad)) {
> 		padding_w = 0;
> 		padding_h = 0;
> 	} else if (IS_BYT) {
> 		padding_w = 12;
> 		padding_h = 12;
> 	}
> 	...
> 	atomisp_get_dis_envelop(asd, f->fmt.pix.width, f->fmt.pix.height,
> 				&dvs_env_w, &dvs_env_h);
> 	...
> 	/*
> 	 * set format info to sensor
> 	 * In continuous mode, resolution is set only if it is higher than
> 	 * existing value. This because preview pipe will be configured after
> 	 * capture pipe and usually has lower resolution than capture pipe.
> 	 */
> 	if (!asd->continuous_mode->val ||
> 	    isp_sink_fmt->width < (f->fmt.pix.width + padding_w + dvs_env_w) ||
> 	    isp_sink_fmt->height < (f->fmt.pix.height + padding_h +
> 				    dvs_env_h)) {
> 
> Due to that, the sensors are set to have those extra 16 columns/lines.
> Those extra data are consumed internally at the driver, so the output
> buffer won't contain those.
> 
> Yet, the buffer size to be allocated by userspace on USERPTR mode is not just
> width x height x bpp, but it may need extra space for such pads and/or other 
> things.
> 
> In practice, when VIDIOC_S_FMT asks for a 1600x1200 resolution, it
> actually sets 1616x1216 at the sensor (at the pad source), but the
> pad sink provides the requested resolution: 1600x1200.
> 
> However, the resolution returned by VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT
> is not always the sink resolution. Sometimes, it returns the sensor format.
> 
> Fixing it has been challenging.
> 
> -
> 
> Another related issue is that, when a resolution bigger than the maximum
> resolution is requested, the driver doesn't return 1600x1200, but, instead,
> a smaller one.
> 
> On other words, setting to YUYV 1600x1200 gives:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1600,height=1200 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: ok
> 	Format Video Capture:
> 		Width/Height      : 1600/1200
> 		Pixel Format      : 'YUYV' (YUYV 4:2:2)
> 		Field             : None
> 		Bytes per Line    : 3200
> 		Size Image        : 3842048
> 		Colorspace        : Rec. 709
> 		Transfer Function : Rec. 709
> 		YCbCr/HSV Encoding: Rec. 709
> 		Quantization      : Default (maps to Limited Range)
> 		Flags             : 
> 
> Setting to a higher resolution (which is a method that some apps use to test
> for the max resolution, when VIDIOC_ENUM_FRAMESIZES is not available or
> broken) produces:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=10000,height=10000 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: ok
> 	Format Video Capture:
> 		Width/Height      : 1600/900
> 		Pixel Format      : 'YUYV' (YUYV 4:2:2)
> 		Field             : None
> 		Bytes per Line    : 3200
> 		Size Image        : 2883584
> 		Colorspace        : Rec. 709
> 		Transfer Function : Rec. 709
> 		YCbCr/HSV Encoding: Rec. 709
> 		Quantization      : Default (maps to Limited Range)
> 		Flags             : 
> 
> Trying to set to the sensor resolution is even worse, as it returns EINVAL:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1616,height=1216 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: failed: Invalid argument
> 
> The logs for such case are:
> 
> 	[ 4799.594724] atomisp-isp2 0000:00:03.0: can't create streams
> 	[ 4799.594757] atomisp-isp2 0000:00:03.0: __get_frame_info 1616x1216 (padded to 0) returned -22
> 	[ 4799.594781] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22
> 
> Video devices
> =============
> 
> Currently, 10 video? devices are created:
> 
> 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> 	/dev/video4:	Name             : ATOMISP ISP ACC
> 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> 	/dev/video10:	Name             : ATOMISP ISP ACC
> 
> That seems to be written to satisfy some Android-based app, but we don't
> really need all of those.
> 
> I'm thinking to comment out the part of the code which creates all of those,
> keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> of those.
> 
> Comments?
> 
> Regards,
> Mauro
> 

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Wed, 3 Nov 2021 15:41:05 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 03/11/2021 14:54, Mauro Carvalho Chehab wrote:
> > Hi,
> > 
> > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > in order to allow a generic V4L2 application to work with it.
> > 
> > MMAP support
> > ============
> > 
> > Despite having some MMAP code on it, the current implementation is broken. 
> > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > which does several tricks.
> > 
> > The best would be to replace it by something simpler. If this is similar
> > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > videodev2 + IPU3 HMM code.
> > 
> > As this is not trivial, I'm postponing such task. If someone has enough
> > time, it would be great to have this fixed.
> > 
> > From my side, I opted to add support for USERPTR on camorama:
> > 
> > 	https://github.com/alessio/camorama
> > 
> > As this is something I wanted to do anyway, and it allowed me to cleanup
> > several things in camorama's code.
> > 
> > Support for USERPTR is not autodetected. So, this should be selected  
> 
> You can autodetect this: the capabilities field returned by VIDIOC_REQBUFS
> or VIDIOC_CREATE_BUFS will indicate support for this. This works with any
> vb2-based driver.
> 
> Just thought I should mention this...

Yeah, surely the app could try it, but:

1. As libv4l doesn't support USERPTR, such detection should happen
   early inside camorama code;

2. Atomisp does have support for MMAP, but it is broken.
   (this is the most relevant reason)

Ok, we could change it to return -ENOIOCTLCMD for mmap, and add
a basic logic at camorama that would try to call REQBUFS in order
to verify if -ENOIOCTLCMD is returned.

Perhaps one more item to our todo list, if nobody fixes MMAP before
that.

Regards,
Mauro

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Wed, 3 Nov 2021 16:34:39 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:

> On Wed, Nov 3, 2021 at 3:54 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> 
> ...
> 
> > Currently, 10 video? devices are created:
> >
> >         $ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> >         /dev/video0:    Name             : ATOMISP ISP CAPTURE output
> >         /dev/video1:    Name             : ATOMISP ISP VIEWFINDER output
> >         /dev/video2:    Name             : ATOMISP ISP PREVIEW output
> >         /dev/video3:    Name             : ATOMISP ISP VIDEO output
> >         /dev/video4:    Name             : ATOMISP ISP ACC
> >         /dev/video5:    Name             : ATOMISP ISP MEMORY input
> >         /dev/video6:    Name             : ATOMISP ISP CAPTURE output
> >         /dev/video7:    Name             : ATOMISP ISP VIEWFINDER output
> >         /dev/video8:    Name             : ATOMISP ISP PREVIEW output
> >         /dev/video9:    Name             : ATOMISP ISP VIDEO output
> >         /dev/video10:   Name             : ATOMISP ISP ACC
> >
> > That seems to be written to satisfy some Android-based app, but we don't
> > really need all of those.
> >
> > I'm thinking to comment out the part of the code which creates all of those,
> > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > of those.  
> 
> Are they using the same microprograms (named routines) in the firmware?

More or less. There's a "run_mode" parameter that actually controls it.

Right now, one of the patches fixed it to work only in preview mode.

So, we're testing only the microprograms that are associated to it.

Regards,
Mauro

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Wed, 3 Nov 2021 16:54:24 +0000
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Wed, 3 Nov 2021 15:41:05 +0100
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
> > On 03/11/2021 14:54, Mauro Carvalho Chehab wrote:  
> > > Hi,
> > > 
> > > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > > in order to allow a generic V4L2 application to work with it.
> > > 
> > > MMAP support
> > > ============
> > > 
> > > Despite having some MMAP code on it, the current implementation is broken. 
> > > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > > which does several tricks.
> > > 
> > > The best would be to replace it by something simpler. If this is similar
> > > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > > videodev2 + IPU3 HMM code.
> > > 
> > > As this is not trivial, I'm postponing such task. If someone has enough
> > > time, it would be great to have this fixed.
> > > 
> > > From my side, I opted to add support for USERPTR on camorama:
> > > 
> > > 	https://github.com/alessio/camorama
> > > 
> > > As this is something I wanted to do anyway, and it allowed me to cleanup
> > > several things in camorama's code.
> > > 
> > > Support for USERPTR is not autodetected. So, this should be selected    
> > 
> > You can autodetect this: the capabilities field returned by VIDIOC_REQBUFS
> > or VIDIOC_CREATE_BUFS will indicate support for this. This works with any
> > vb2-based driver.
> > 
> > Just thought I should mention this...  
> 
> Yeah, surely the app could try it, but:
> 
> 1. As libv4l doesn't support USERPTR, such detection should happen
>    early inside camorama code;

I ended adding auto-detection support for USERPTR inside camorama,
for completeness.

The "-U" command line option remains, so one could use it to force USERPTR
mode.

As the way I implemented it is that camorama checks if REQBUFS doesn't
return any error, it means that it will automatically fallback to USERPTR
with atomisp driver (while MMAP support is not fixed there).

So, once I fix the issues with S_FMT/G_FMT, camorama will likely work
out of the box with it.

Regards,
Mauro

Re: atomisp current issues

[Laurent Pinchart]

Hi Mauro,

On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:
> Hi,
> 
> From what I've seen so far, those are the main issues with regards to V4L2 API,
> in order to allow a generic V4L2 application to work with it.
> 
> MMAP support
> ============
> 
> Despite having some MMAP code on it, the current implementation is broken. 
> Fixing it is not trivial, as it would require fixing the HMM support on it, 
> which does several tricks.
> 
> The best would be to replace it by something simpler. If this is similar
> enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> videodev2 + IPU3 HMM code.
> 
> As this is not trivial, I'm postponing such task. If someone has enough
> time, it would be great to have this fixed.
> 
> From my side, I opted to add support for USERPTR on camorama:
> 
> 	https://github.com/alessio/camorama

We should *really* phase out USERPTR support. Worst case you may support
DMABUF only if MMAP is problematic, but I don't really see why it could
be easy to map an imported buffer and difficult to map a buffer
allocated by the driver. videobuf2 should be used.

> As this is something I wanted to do anyway, and it allowed me to cleanup
> several things in camorama's code.
> 
> Support for USERPTR is not autodetected. So, this should be selected
> via a command line parameter. Here (Fedora), I installed the build
> dependencies on my test device with:
> 
> 	$ sudo dnf builddep camorama
> 	$ sudo dnf install gnome-common  # for gnome-autogen.sh
> 
> Testing it with atomisp would be:
> 
> 	$ git clone https://github.com/alessio/camorama
> 	$ cd camorama && ./autogen.sh
> 	$ make && ./src/camorama -d /dev/video2  --debug -U -D
> 
> In time:
> --------
> 
>  Camorama currently doesn't work due to some other API bugs. See below.
> 
> 
> VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT issues
> ===============================================
> 
> The implementation for VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT are not
> properly implemented. It has several issues.
> 
> The main one is related to padding. Based on the comments inside the code,
> the ISP 2 seems to require padding to work, both vertically an horizontally.
> 
> Those are controlled by two modprobe parameters: pad_h and pad_w.
> The default for both is 16.
> 
> There are some other padding logic inside the function which sets the
> video formats at atomisp_cmd: atomisp_set_fmt(). This function is quite
> complex, being big and it calls several other atomisp kAPIs.
> 
> It basically queries the sensor, and then it mounts a pipeline that
> will have the sensor + ISP blocks. Those ISP blocks convert the format
> from Bayer into YUYV or RGB and scale down the resolution in order to
> match the request.
> 
> It also adjusts the padding. The padding code there is very complex,
> as it not only uses pad_h/pad_w, having things like:
> 
> 	if (!atomisp_subdev_format_conversion(asd, source_pad)) {
> 		padding_w = 0;
> 		padding_h = 0;
> 	} else if (IS_BYT) {
> 		padding_w = 12;
> 		padding_h = 12;
> 	}
> 	...
> 	atomisp_get_dis_envelop(asd, f->fmt.pix.width, f->fmt.pix.height,
> 				&dvs_env_w, &dvs_env_h);
> 	...
> 	/*
> 	 * set format info to sensor
> 	 * In continuous mode, resolution is set only if it is higher than
> 	 * existing value. This because preview pipe will be configured after
> 	 * capture pipe and usually has lower resolution than capture pipe.
> 	 */
> 	if (!asd->continuous_mode->val ||
> 	    isp_sink_fmt->width < (f->fmt.pix.width + padding_w + dvs_env_w) ||
> 	    isp_sink_fmt->height < (f->fmt.pix.height + padding_h +
> 				    dvs_env_h)) {
> 
> Due to that, the sensors are set to have those extra 16 columns/lines.
> Those extra data are consumed internally at the driver, so the output
> buffer won't contain those.
> 
> Yet, the buffer size to be allocated by userspace on USERPTR mode is not just
> width x height x bpp, but it may need extra space for such pads and/or other 
> things.
> 
> In practice, when VIDIOC_S_FMT asks for a 1600x1200 resolution, it
> actually sets 1616x1216 at the sensor (at the pad source), but the
> pad sink provides the requested resolution: 1600x1200.
> 
> However, the resolution returned by VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT
> is not always the sink resolution. Sometimes, it returns the sensor format.
> 
> Fixing it has been challenging.
> 
> -
> 
> Another related issue is that, when a resolution bigger than the maximum
> resolution is requested, the driver doesn't return 1600x1200, but, instead,
> a smaller one.
> 
> On other words, setting to YUYV 1600x1200 gives:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1600,height=1200 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: ok
> 	Format Video Capture:
> 		Width/Height      : 1600/1200
> 		Pixel Format      : 'YUYV' (YUYV 4:2:2)
> 		Field             : None
> 		Bytes per Line    : 3200
> 		Size Image        : 3842048
> 		Colorspace        : Rec. 709
> 		Transfer Function : Rec. 709
> 		YCbCr/HSV Encoding: Rec. 709
> 		Quantization      : Default (maps to Limited Range)
> 		Flags             : 
> 
> Setting to a higher resolution (which is a method that some apps use to test
> for the max resolution, when VIDIOC_ENUM_FRAMESIZES is not available or
> broken) produces:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=10000,height=10000 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: ok
> 	Format Video Capture:
> 		Width/Height      : 1600/900
> 		Pixel Format      : 'YUYV' (YUYV 4:2:2)
> 		Field             : None
> 		Bytes per Line    : 3200
> 		Size Image        : 2883584
> 		Colorspace        : Rec. 709
> 		Transfer Function : Rec. 709
> 		YCbCr/HSV Encoding: Rec. 709
> 		Quantization      : Default (maps to Limited Range)
> 		Flags             : 
> 
> Trying to set to the sensor resolution is even worse, as it returns EINVAL:
> 
> 	$ v4l2-ctl --set-fmt-video pixelformat=YUYV,width=1616,height=1216 -d /dev/video2 --verbose
> 	VIDIOC_QUERYCAP: ok
> 	VIDIOC_G_FMT: ok
> 	VIDIOC_S_FMT: failed: Invalid argument
> 
> The logs for such case are:
> 
> 	[ 4799.594724] atomisp-isp2 0000:00:03.0: can't create streams
> 	[ 4799.594757] atomisp-isp2 0000:00:03.0: __get_frame_info 1616x1216 (padded to 0) returned -22
> 	[ 4799.594781] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22
> 
> Video devices
> =============
> 
> Currently, 10 video? devices are created:
> 
> 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> 	/dev/video4:	Name             : ATOMISP ISP ACC
> 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> 	/dev/video10:	Name             : ATOMISP ISP ACC
> 
> That seems to be written to satisfy some Android-based app, but we don't
> really need all of those.
> 
> I'm thinking to comment out the part of the code which creates all of those,
> keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> of those.

Why is that ? Being able to capture multiple streams in different
resolutions is important for lots of applications, the viewfinder
resolution is often different than the video streaming and/or still
capture resolution. Scaling after capture is often expensive (and there
are memory bandwidth and power constraints to take into account too). A
single-stream device may be better than nothing, but it's time to move
to the 21st century.

-- 
Regards,

Laurent Pinchart

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Thu, 4 Nov 2021 11:53:55 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:
> > Hi,
> > 
> > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > in order to allow a generic V4L2 application to work with it.
> > 
> > MMAP support
> > ============
> > 
> > Despite having some MMAP code on it, the current implementation is broken. 
> > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > which does several tricks.
> > 
> > The best would be to replace it by something simpler. If this is similar
> > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > videodev2 + IPU3 HMM code.
> > 
> > As this is not trivial, I'm postponing such task. If someone has enough
> > time, it would be great to have this fixed.
> > 
> > From my side, I opted to add support for USERPTR on camorama:
> > 
> > 	https://github.com/alessio/camorama  
> 
> We should *really* phase out USERPTR support. 

I'm not a big fan of userptr, buy why do we phase it out?

> Worst case you may support
> DMABUF only if MMAP is problematic, but I don't really see why it could
> be easy to map an imported buffer and difficult to map a buffer
> allocated by the driver. videobuf2 should be used.

Yeah, atomisp should be migrated to VB2, and such migration is listed at
its TODO file. However, this is a complex task, as its memory management
code is very complex. Maybe we could try to use the ISP3 code on it,
replacing the current HMM logic, but not sure if the implementation there 
would be compatible.

In any case, the current priority is to make the driver to work, fixing 
the V4L2 API implementation, which has several issues.

...

> > Video devices
> > =============
> > 
> > Currently, 10 video? devices are created:
> > 
> > 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> > 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> > 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> > 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> > 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> > 	/dev/video4:	Name             : ATOMISP ISP ACC
> > 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> > 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> > 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> > 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> > 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> > 	/dev/video10:	Name             : ATOMISP ISP ACC
> > 
> > That seems to be written to satisfy some Android-based app, but we don't
> > really need all of those.
> > 
> > I'm thinking to comment out the part of the code which creates all of those,
> > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > of those.  
> 
> Why is that ? Being able to capture multiple streams in different
> resolutions is important for lots of applications, the viewfinder
> resolution is often different than the video streaming and/or still
> capture resolution. Scaling after capture is often expensive (and there
> are memory bandwidth and power constraints to take into account too). A
> single-stream device may be better than nothing, but it's time to move
> to the 21st century.

True, but having multiple videonodes at this moment is not helping,
specially since only one of such modes (PREVIEW) is actually working at
the moment.

So, this is more a strategy to help focusing on making this work
properly, and not a statement that those modules would be dropped.

I'd say that the "final" version of atomisp - once it gets 
fixed, cleaned up and started being MC-controlled - should support
all such features, and have the pipelines setup via libcamera.

Regards,
Mauro

Re: atomisp current issues

[Mauro Carvalho Chehab]

As an update on that:

Em Wed, 3 Nov 2021 13:54:18 +0000
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Hi,
> 
> From what I've seen so far, those are the main issues with regards to V4L2 API,
> in order to allow a generic V4L2 application to work with it.
> 
> MMAP support

This issue still needs to be addressed...

> From my side, I opted to add support for USERPTR on camorama:
> 
> 	https://github.com/alessio/camorama

...

> In time:
> --------
> 
>  Camorama currently doesn't work due to some other API bugs. See below.

But now camorama has gained support for autodetecting problems with
MMAP support.

At least for Asus T101HA, camorama is now working with atomisp.

> VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT issues
> ===============================================

I addressed those issues. The implementation is not 100%, but it is good
enough for camorama to start working.

> 
> Video devices
> =============
> 
> Currently, 10 video? devices are created:

I didn't touch it. So, camorama should be called the first time with:

	camorama -d /dev/video2

in order for it to work. As camorama memorizes the last used camera,
it will pick /dev/video2 if called later without any parameters.

Regards,
Mauro

Re: atomisp current issues

[Laurent Pinchart]

Hi Mauro,

On Thu, Nov 04, 2021 at 10:50:51AM +0000, Mauro Carvalho Chehab wrote:
> Em Thu, 4 Nov 2021 11:53:55 +0200 Laurent Pinchart escreveu:
> > On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:
> > > Hi,
> > > 
> > > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > > in order to allow a generic V4L2 application to work with it.
> > > 
> > > MMAP support
> > > ============
> > > 
> > > Despite having some MMAP code on it, the current implementation is broken. 
> > > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > > which does several tricks.
> > > 
> > > The best would be to replace it by something simpler. If this is similar
> > > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > > videodev2 + IPU3 HMM code.
> > > 
> > > As this is not trivial, I'm postponing such task. If someone has enough
> > > time, it would be great to have this fixed.
> > > 
> > > From my side, I opted to add support for USERPTR on camorama:
> > > 
> > > 	https://github.com/alessio/camorama  
> > 
> > We should *really* phase out USERPTR support. 
> 
> I'm not a big fan of userptr, buy why do we phase it out?

Because USERPTR is broken by design. It gives a false promise to
userspace that a user pointer can be DMA'ed to, and this isn't generally
true. Even if buffer are carefully allocated to be compatible with the
device requirements, there are corner cases that prevent making a
mechanism based on get_user_pages() a first class citizen. In any case,
USERPTR makes life more difficult for the kernel.

There may be some use cases for which USERPTR could be an appropriate
solution, but now that we have DMABUF (and of course MMAP), I see no
reason to continue supporting USERPTR forever, and certainly not adding
new users.

> > Worst case you may support
> > DMABUF only if MMAP is problematic, but I don't really see why it could
> > be easy to map an imported buffer and difficult to map a buffer
> > allocated by the driver. videobuf2 should be used.
> 
> Yeah, atomisp should be migrated to VB2, and such migration is listed at
> its TODO file. However, this is a complex task, as its memory management
> code is very complex.

Have a look at GPU memory management, and you'll find the atomisp driver
very simple in comparison :-)

I'm also pretty sure that drivers/staging/media/atomisp/pci/hmm/ could
be rewritten to use more of the existing kernel frameworks.

> Maybe we could try to use the ISP3 code on it,
> replacing the current HMM logic, but not sure if the implementation there 
> would be compatible.

I'd be surprised if the IPU3 was compatible.

> In any case, the current priority is to make the driver to work, fixing 
> the V4L2 API implementation, which has several issues.
> 
> ...
> 
> > > Video devices
> > > =============
> > > 
> > > Currently, 10 video? devices are created:
> > > 
> > > 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> > > 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> > > 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> > > 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> > > 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> > > 	/dev/video4:	Name             : ATOMISP ISP ACC
> > > 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> > > 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> > > 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> > > 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> > > 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> > > 	/dev/video10:	Name             : ATOMISP ISP ACC
> > > 
> > > That seems to be written to satisfy some Android-based app, but we don't
> > > really need all of those.
> > > 
> > > I'm thinking to comment out the part of the code which creates all of those,
> > > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > > of those.  
> > 
> > Why is that ? Being able to capture multiple streams in different
> > resolutions is important for lots of applications, the viewfinder
> > resolution is often different than the video streaming and/or still
> > capture resolution. Scaling after capture is often expensive (and there
> > are memory bandwidth and power constraints to take into account too). A
> > single-stream device may be better than nothing, but it's time to move
> > to the 21st century.
> 
> True, but having multiple videonodes at this moment is not helping,
> specially since only one of such modes (PREVIEW) is actually working at
> the moment.
> 
> So, this is more a strategy to help focusing on making this work
> properly, and not a statement that those modules would be dropped.
> 
> I'd say that the "final" version of atomisp - once it gets 
> fixed, cleaned up and started being MC-controlled - should support
> all such features, and have the pipelines setup via libcamera.

I have no issue with phasing development (I have few issues with the
atomisp driver in general actually, as it's in staging), but the goal
should be kept in mind to make sure development goes in the right
direction.

-- 
Regards,

Laurent Pinchart

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Thu, 4 Nov 2021 14:47:14 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Thu, Nov 04, 2021 at 10:50:51AM +0000, Mauro Carvalho Chehab wrote:
> > Em Thu, 4 Nov 2021 11:53:55 +0200 Laurent Pinchart escreveu:  
> > > On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:  
> > > > Hi,
> > > > 
> > > > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > > > in order to allow a generic V4L2 application to work with it.
> > > > 
> > > > MMAP support
> > > > ============
> > > > 
> > > > Despite having some MMAP code on it, the current implementation is broken. 
> > > > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > > > which does several tricks.
> > > > 
> > > > The best would be to replace it by something simpler. If this is similar
> > > > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > > > videodev2 + IPU3 HMM code.
> > > > 
> > > > As this is not trivial, I'm postponing such task. If someone has enough
> > > > time, it would be great to have this fixed.
> > > > 
> > > > From my side, I opted to add support for USERPTR on camorama:
> > > > 
> > > > 	https://github.com/alessio/camorama    
> > > 
> > > We should *really* phase out USERPTR support.   
> > 
> > I'm not a big fan of userptr, buy why do we phase it out?  
> 
> Because USERPTR is broken by design. It gives a false promise to
> userspace that a user pointer can be DMA'ed to, and this isn't generally
> true. 

Actually, the only promise USERPTR gives is that the driver should
be able to store streaming data on it (usually done via DMA from
the V4L2 card to the memory).

It never promised - or was designed - to be used on embedded
devices and allow DMA transfers to GPU. See, the original design
is from 1999 and it was focused only on x86 archs ;-)

The thing is that it was an alternative to FBUF before DMABUF
was added.

But yeah, newer embedded device drivers should not enable it,
using instead VB2 and DMABUF.

Perhaps we should document it better at:
	https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/userp.html#userp

warning developers to use DMABUF when the goal is to share DMA buffers
with other drivers.

> Even if buffer are carefully allocated to be compatible with the
> device requirements, there are corner cases that prevent making a
> mechanism based on get_user_pages() a first class citizen.

Drivers that can't warrant that should disable support for it ;-)

> In any case,
> USERPTR makes life more difficult for the kernel.
> 
> There may be some use cases for which USERPTR could be an appropriate
> solution, but now that we have DMABUF (and of course MMAP), I see no
> reason to continue supporting USERPTR forever, and certainly not adding
> new users.

As we need to support this forever, IMO, it doesn't make sense to deny
patches adding new users for it - yet it makes sense to recommend not to,
specially on drivers whose usage would be on embedded systems.

> > > Worst case you may support
> > > DMABUF only if MMAP is problematic, but I don't really see why it could
> > > be easy to map an imported buffer and difficult to map a buffer
> > > allocated by the driver. videobuf2 should be used.  
> > 
> > Yeah, atomisp should be migrated to VB2, and such migration is listed at
> > its TODO file. However, this is a complex task, as its memory management
> > code is very complex.  
> 
> Have a look at GPU memory management, and you'll find the atomisp driver
> very simple in comparison :-)

Yeah, there are lots of complex thing at GPU mm. Yet, I don't see too much 
levels of abstraction there... the biggest issue on atomisp is that there
are usually two or three levels of abstraction between the actual 
implementation and the callers. It also has some hacks in the middle of code
that seems to be due to some special devices for Android.

> I'm also pretty sure that drivers/staging/media/atomisp/pci/hmm/ could
> be rewritten to use more of the existing kernel frameworks.

Yes, I guess so. Again, the problem is that the glue also require
changes and cleanups. It also requires migration from VB1 to VB2.

> > Maybe we could try to use the ISP3 code on it,
> > replacing the current HMM logic, but not sure if the implementation there 
> > would be compatible.  
> 
> I'd be surprised if the IPU3 was compatible.

If IPU3 started as an upgrade from ISP2, then maybe it is partially
compatible.

Anyway, further studies and tests are required.

> > In any case, the current priority is to make the driver to work, fixing 
> > the V4L2 API implementation, which has several issues.
> > 
> > ...
> >   
> > > > Video devices
> > > > =============
> > > > 
> > > > Currently, 10 video? devices are created:
> > > > 
> > > > 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> > > > 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> > > > 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> > > > 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> > > > 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> > > > 	/dev/video4:	Name             : ATOMISP ISP ACC
> > > > 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> > > > 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> > > > 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> > > > 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> > > > 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> > > > 	/dev/video10:	Name             : ATOMISP ISP ACC
> > > > 
> > > > That seems to be written to satisfy some Android-based app, but we don't
> > > > really need all of those.
> > > > 
> > > > I'm thinking to comment out the part of the code which creates all of those,
> > > > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > > > of those.    
> > > 
> > > Why is that ? Being able to capture multiple streams in different
> > > resolutions is important for lots of applications, the viewfinder
> > > resolution is often different than the video streaming and/or still
> > > capture resolution. Scaling after capture is often expensive (and there
> > > are memory bandwidth and power constraints to take into account too). A
> > > single-stream device may be better than nothing, but it's time to move
> > > to the 21st century.  
> > 
> > True, but having multiple videonodes at this moment is not helping,
> > specially since only one of such modes (PREVIEW) is actually working at
> > the moment.
> > 
> > So, this is more a strategy to help focusing on making this work
> > properly, and not a statement that those modules would be dropped.
> > 
> > I'd say that the "final" version of atomisp - once it gets 
> > fixed, cleaned up and started being MC-controlled - should support
> > all such features, and have the pipelines setup via libcamera.  
> 
> I have no issue with phasing development (I have few issues with the
> atomisp driver in general actually, as it's in staging), but the goal
> should be kept in mind to make sure development goes in the right
> direction.

Yeah, surely it needs to progress at the right direction. We're
just too far of having it ready to be ported to MC and libcamera...
there are too many issues to be solved.

Regards,
Mauro

Re: atomisp current issues

[Laurent Pinchart]

Hi Mauro,

On Fri, Nov 05, 2021 at 06:55:26PM +0000, Mauro Carvalho Chehab wrote:
> Em Thu, 4 Nov 2021 14:47:14 +0200 Laurent Pinchart escreveu:
> > On Thu, Nov 04, 2021 at 10:50:51AM +0000, Mauro Carvalho Chehab wrote:
> > > Em Thu, 4 Nov 2021 11:53:55 +0200 Laurent Pinchart escreveu:  
> > > > On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:  
> > > > > Hi,
> > > > > 
> > > > > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > > > > in order to allow a generic V4L2 application to work with it.
> > > > > 
> > > > > MMAP support
> > > > > ============
> > > > > 
> > > > > Despite having some MMAP code on it, the current implementation is broken. 
> > > > > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > > > > which does several tricks.
> > > > > 
> > > > > The best would be to replace it by something simpler. If this is similar
> > > > > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > > > > videodev2 + IPU3 HMM code.
> > > > > 
> > > > > As this is not trivial, I'm postponing such task. If someone has enough
> > > > > time, it would be great to have this fixed.
> > > > > 
> > > > > From my side, I opted to add support for USERPTR on camorama:
> > > > > 
> > > > > 	https://github.com/alessio/camorama    
> > > > 
> > > > We should *really* phase out USERPTR support.   
> > > 
> > > I'm not a big fan of userptr, buy why do we phase it out?  
> > 
> > Because USERPTR is broken by design. It gives a false promise to
> > userspace that a user pointer can be DMA'ed to, and this isn't generally
> > true. 
> 
> Actually, the only promise USERPTR gives is that the driver should
> be able to store streaming data on it (usually done via DMA from
> the V4L2 card to the memory).

Yes, and that promise is broken. It doesn't work universally (across
architectures, or across devices).

> It never promised - or was designed - to be used on embedded
> devices and allow DMA transfers to GPU. See, the original design
> is from 1999 and it was focused only on x86 archs ;-)

Embedded isn't relevant in this context anymore. x86 hasn't been limited
to the desktop space for a long time, and neither is ARM embedded-only.

> The thing is that it was an alternative to FBUF before DMABUF
> was added.
> 
> But yeah, newer embedded device drivers should not enable it,
> using instead VB2 and DMABUF.
> 
> Perhaps we should document it better at:
> 	https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/userp.html#userp
> 
> warning developers to use DMABUF when the goal is to share DMA buffers
> with other drivers.
> 
> > Even if buffer are carefully allocated to be compatible with the
> > device requirements, there are corner cases that prevent making a
> > mechanism based on get_user_pages() a first class citizen.
> 
> Drivers that can't warrant that should disable support for it ;-)

With USERPTR implemented through videobuf2, I doubt most driver authors
are aware of the USERPTR corner cases and limitations.

> > In any case,
> > USERPTR makes life more difficult for the kernel.
> > 
> > There may be some use cases for which USERPTR could be an appropriate
> > solution, but now that we have DMABUF (and of course MMAP), I see no
> > reason to continue supporting USERPTR forever, and certainly not adding
> > new users.
> 
> As we need to support this forever, IMO, it doesn't make sense to deny
> patches adding new users for it - yet it makes sense to recommend not to,
> specially on drivers whose usage would be on embedded systems.

Why should we accept it for new drivers, what use cases does it enable
that can't be supported through MMAP and DMABUF ?

> > > > Worst case you may support
> > > > DMABUF only if MMAP is problematic, but I don't really see why it could
> > > > be easy to map an imported buffer and difficult to map a buffer
> > > > allocated by the driver. videobuf2 should be used.  
> > > 
> > > Yeah, atomisp should be migrated to VB2, and such migration is listed at
> > > its TODO file. However, this is a complex task, as its memory management
> > > code is very complex.  
> > 
> > Have a look at GPU memory management, and you'll find the atomisp driver
> > very simple in comparison :-)
> 
> Yeah, there are lots of complex thing at GPU mm. Yet, I don't see too much 
> levels of abstraction there... the biggest issue on atomisp is that there
> are usually two or three levels of abstraction between the actual 
> implementation and the callers. It also has some hacks in the middle of code
> that seems to be due to some special devices for Android.

Isn't it just a matter of dropping the abstraction layers ?

> > I'm also pretty sure that drivers/staging/media/atomisp/pci/hmm/ could
> > be rewritten to use more of the existing kernel frameworks.
> 
> Yes, I guess so. Again, the problem is that the glue also require
> changes and cleanups. It also requires migration from VB1 to VB2.
> 
> > > Maybe we could try to use the ISP3 code on it,
> > > replacing the current HMM logic, but not sure if the implementation there 
> > > would be compatible.  
> > 
> > I'd be surprised if the IPU3 was compatible.
> 
> If IPU3 started as an upgrade from ISP2, then maybe it is partially
> compatible.
> 
> Anyway, further studies and tests are required.
> 
> > > In any case, the current priority is to make the driver to work, fixing 
> > > the V4L2 API implementation, which has several issues.
> > > 
> > > ...
> > >   
> > > > > Video devices
> > > > > =============
> > > > > 
> > > > > Currently, 10 video? devices are created:
> > > > > 
> > > > > 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> > > > > 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> > > > > 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> > > > > 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> > > > > 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> > > > > 	/dev/video4:	Name             : ATOMISP ISP ACC
> > > > > 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> > > > > 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> > > > > 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> > > > > 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> > > > > 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> > > > > 	/dev/video10:	Name             : ATOMISP ISP ACC
> > > > > 
> > > > > That seems to be written to satisfy some Android-based app, but we don't
> > > > > really need all of those.
> > > > > 
> > > > > I'm thinking to comment out the part of the code which creates all of those,
> > > > > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > > > > of those.    
> > > > 
> > > > Why is that ? Being able to capture multiple streams in different
> > > > resolutions is important for lots of applications, the viewfinder
> > > > resolution is often different than the video streaming and/or still
> > > > capture resolution. Scaling after capture is often expensive (and there
> > > > are memory bandwidth and power constraints to take into account too). A
> > > > single-stream device may be better than nothing, but it's time to move
> > > > to the 21st century.  
> > > 
> > > True, but having multiple videonodes at this moment is not helping,
> > > specially since only one of such modes (PREVIEW) is actually working at
> > > the moment.
> > > 
> > > So, this is more a strategy to help focusing on making this work
> > > properly, and not a statement that those modules would be dropped.
> > > 
> > > I'd say that the "final" version of atomisp - once it gets 
> > > fixed, cleaned up and started being MC-controlled - should support
> > > all such features, and have the pipelines setup via libcamera.  
> > 
> > I have no issue with phasing development (I have few issues with the
> > atomisp driver in general actually, as it's in staging), but the goal
> > should be kept in mind to make sure development goes in the right
> > direction.
> 
> Yeah, surely it needs to progress at the right direction. We're
> just too far of having it ready to be ported to MC and libcamera...
> there are too many issues to be solved.

I'm not even sure porting it to MC makes sense, the firmware seems to
expose a too high level of abstraction. I could be wrong though.

-- 
Regards,

Laurent Pinchart

Re: atomisp current issues

[Mauro Carvalho Chehab]

Em Sat, 6 Nov 2021 01:13:59 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Fri, Nov 05, 2021 at 06:55:26PM +0000, Mauro Carvalho Chehab wrote:
> > Em Thu, 4 Nov 2021 14:47:14 +0200 Laurent Pinchart escreveu:  
> > > On Thu, Nov 04, 2021 at 10:50:51AM +0000, Mauro Carvalho Chehab wrote:  
> > > > Em Thu, 4 Nov 2021 11:53:55 +0200 Laurent Pinchart escreveu:    
> > > > > On Wed, Nov 03, 2021 at 01:54:18PM +0000, Mauro Carvalho Chehab wrote:    
> > > > > > Hi,
> > > > > > 
> > > > > > From what I've seen so far, those are the main issues with regards to V4L2 API,
> > > > > > in order to allow a generic V4L2 application to work with it.
> > > > > > 
> > > > > > MMAP support
> > > > > > ============
> > > > > > 
> > > > > > Despite having some MMAP code on it, the current implementation is broken. 
> > > > > > Fixing it is not trivial, as it would require fixing the HMM support on it, 
> > > > > > which does several tricks.
> > > > > > 
> > > > > > The best would be to replace it by something simpler. If this is similar
> > > > > > enough to IPU3, perhaps one idea would be to replace the HMM code on it by 
> > > > > > videodev2 + IPU3 HMM code.
> > > > > > 
> > > > > > As this is not trivial, I'm postponing such task. If someone has enough
> > > > > > time, it would be great to have this fixed.
> > > > > > 
> > > > > > From my side, I opted to add support for USERPTR on camorama:
> > > > > > 
> > > > > > 	https://github.com/alessio/camorama      
> > > > > 
> > > > > We should *really* phase out USERPTR support.     
> > > > 
> > > > I'm not a big fan of userptr, buy why do we phase it out?    
> > > 
> > > Because USERPTR is broken by design. It gives a false promise to
> > > userspace that a user pointer can be DMA'ed to, and this isn't generally
> > > true.   
> > 
> > Actually, the only promise USERPTR gives is that the driver should
> > be able to store streaming data on it (usually done via DMA from
> > the V4L2 card to the memory).  
> 
> Yes, and that promise is broken. It doesn't work universally (across
> architectures, or across devices).
> 
> > It never promised - or was designed - to be used on embedded
> > devices and allow DMA transfers to GPU. See, the original design
> > is from 1999 and it was focused only on x86 archs ;-)  
> 
> Embedded isn't relevant in this context anymore. x86 hasn't been limited
> to the desktop space for a long time, and neither is ARM embedded-only.

I see your point. Back on the old days, VB1 had some VMA code that would 
reallocate the physical memory associated with an userptr, in order to 
warrant that it would fit the driver's requirement (basically aiming
devices that won't support DMA scatter/gather).

So, yeah, it was always harder for the Kernel to support USERPTR.

> 
> > The thing is that it was an alternative to FBUF before DMABUF
> > was added.
> > 
> > But yeah, newer embedded device drivers should not enable it,
> > using instead VB2 and DMABUF.
> > 
> > Perhaps we should document it better at:
> > 	https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/userp.html#userp
> > 
> > warning developers to use DMABUF when the goal is to share DMA buffers
> > with other drivers.
> >   
> > > Even if buffer are carefully allocated to be compatible with the
> > > device requirements, there are corner cases that prevent making a
> > > mechanism based on get_user_pages() a first class citizen.  
> > 
> > Drivers that can't warrant that should disable support for it ;-)  
> 
> With USERPTR implemented through videobuf2, I doubt most driver authors
> are aware of the USERPTR corner cases and limitations.

I guess the first step to deprecate this feature would be to write some
text at the uAPI Documentation explaining the USERPTR corner
cases/limitations and recommend userspace apps to not to rely on it.

> > > In any case,
> > > USERPTR makes life more difficult for the kernel.
> > > 
> > > There may be some use cases for which USERPTR could be an appropriate
> > > solution, but now that we have DMABUF (and of course MMAP), I see no
> > > reason to continue supporting USERPTR forever, and certainly not adding
> > > new users.  
> > 
> > As we need to support this forever, IMO, it doesn't make sense to deny
> > patches adding new users for it - yet it makes sense to recommend not to,
> > specially on drivers whose usage would be on embedded systems.  
> 
> Why should we accept it for new drivers, what use cases does it enable
> that can't be supported through MMAP and DMABUF ?

I can't think on any special reason, except for compatibility with some
userspace app that might only work with USERPTR. I don't know any
open source app that supports USERPTR but doesn't support MMAP those
days, but I won't doubt they exist.

Anyway, if we're willing to do that, the first step would be to
write a Documentation patch for driver API, documenting VB2
I/O flags and recommending not to set VB2_USERPTR on new drivers,
explaining why it is a bad idea to use it.

Feel free to write such patch.

> 
> > > > > Worst case you may support
> > > > > DMABUF only if MMAP is problematic, but I don't really see why it could
> > > > > be easy to map an imported buffer and difficult to map a buffer
> > > > > allocated by the driver. videobuf2 should be used.    
> > > > 
> > > > Yeah, atomisp should be migrated to VB2, and such migration is listed at
> > > > its TODO file. However, this is a complex task, as its memory management
> > > > code is very complex.    
> > > 
> > > Have a look at GPU memory management, and you'll find the atomisp driver
> > > very simple in comparison :-)  
> > 
> > Yeah, there are lots of complex thing at GPU mm. Yet, I don't see too much 
> > levels of abstraction there... the biggest issue on atomisp is that there
> > are usually two or three levels of abstraction between the actual 
> > implementation and the callers. It also has some hacks in the middle of code
> > that seems to be due to some special devices for Android.  
> 
> Isn't it just a matter of dropping the abstraction layers ?

Easier said than done.

> > > I'm also pretty sure that drivers/staging/media/atomisp/pci/hmm/ could
> > > be rewritten to use more of the existing kernel frameworks.  
> > 
> > Yes, I guess so. Again, the problem is that the glue also require
> > changes and cleanups. It also requires migration from VB1 to VB2.
> >   
> > > > Maybe we could try to use the ISP3 code on it,
> > > > replacing the current HMM logic, but not sure if the implementation there 
> > > > would be compatible.    
> > > 
> > > I'd be surprised if the IPU3 was compatible.  
> > 
> > If IPU3 started as an upgrade from ISP2, then maybe it is partially
> > compatible.
> > 
> > Anyway, further studies and tests are required.
> >   
> > > > In any case, the current priority is to make the driver to work, fixing 
> > > > the V4L2 API implementation, which has several issues.
> > > > 
> > > > ...
> > > >     
> > > > > > Video devices
> > > > > > =============
> > > > > > 
> > > > > > Currently, 10 video? devices are created:
> > > > > > 
> > > > > > 	$ for i in $(ls /dev/video*|sort -k2 -to -n); do echo -n $i:; v4l2-ctl -D -d $i|grep Name; done
> > > > > > 	/dev/video0:	Name             : ATOMISP ISP CAPTURE output
> > > > > > 	/dev/video1:	Name             : ATOMISP ISP VIEWFINDER output
> > > > > > 	/dev/video2:	Name             : ATOMISP ISP PREVIEW output
> > > > > > 	/dev/video3:	Name             : ATOMISP ISP VIDEO output
> > > > > > 	/dev/video4:	Name             : ATOMISP ISP ACC
> > > > > > 	/dev/video5:	Name             : ATOMISP ISP MEMORY input
> > > > > > 	/dev/video6:	Name             : ATOMISP ISP CAPTURE output
> > > > > > 	/dev/video7:	Name             : ATOMISP ISP VIEWFINDER output
> > > > > > 	/dev/video8:	Name             : ATOMISP ISP PREVIEW output
> > > > > > 	/dev/video9:	Name             : ATOMISP ISP VIDEO output
> > > > > > 	/dev/video10:	Name             : ATOMISP ISP ACC
> > > > > > 
> > > > > > That seems to be written to satisfy some Android-based app, but we don't
> > > > > > really need all of those.
> > > > > > 
> > > > > > I'm thinking to comment out the part of the code which creates all of those,
> > > > > > keeping just "ATOMISP ISP PREVIEW output", as I don't think we need all
> > > > > > of those.      
> > > > > 
> > > > > Why is that ? Being able to capture multiple streams in different
> > > > > resolutions is important for lots of applications, the viewfinder
> > > > > resolution is often different than the video streaming and/or still
> > > > > capture resolution. Scaling after capture is often expensive (and there
> > > > > are memory bandwidth and power constraints to take into account too). A
> > > > > single-stream device may be better than nothing, but it's time to move
> > > > > to the 21st century.    
> > > > 
> > > > True, but having multiple videonodes at this moment is not helping,
> > > > specially since only one of such modes (PREVIEW) is actually working at
> > > > the moment.
> > > > 
> > > > So, this is more a strategy to help focusing on making this work
> > > > properly, and not a statement that those modules would be dropped.
> > > > 
> > > > I'd say that the "final" version of atomisp - once it gets 
> > > > fixed, cleaned up and started being MC-controlled - should support
> > > > all such features, and have the pipelines setup via libcamera.    
> > > 
> > > I have no issue with phasing development (I have few issues with the
> > > atomisp driver in general actually, as it's in staging), but the goal
> > > should be kept in mind to make sure development goes in the right
> > > direction.  
> > 
> > Yeah, surely it needs to progress at the right direction. We're
> > just too far of having it ready to be ported to MC and libcamera...
> > there are too many issues to be solved.  
> 
> I'm not even sure porting it to MC makes sense, the firmware seems to
> expose a too high level of abstraction. I could be wrong though.

I see your point. Yeah, ISP2 seems very different than other MC-based
devices, as the firmware provides ~60 different programs (the driver refers
to them as binaries) so, you can't just add/remove a processing block.
You need to stick with whatever is there.

So, for instance, this:

	$ v4l2grab -f YUYV -x 1600 -y 1200 -d /dev/video2 -n 1 -u

Makes the driver to use two binaries:

	[  145.386407] atomisp-isp2 0000:00:03.0: Using binary isp_preview_var_isp2 (id 22), type 0, mode 1, continuous true
	[  145.386616] atomisp-isp2 0000:00:03.0: Using binary isp_vf_pp_opt (id 3), type 0, mode 9, continuous true

Internally, this binary combination will do, at least:

	Bayer->YUV conversion
	Scaler
	Cropping

The driver has a complex logic (with lots of layers) to try to select the
right binaries. Based on the comments there, those are auto-generated together
with the firmware file itself. So, when a "copy-mode" binary is added at the
firmware file, the code generator also adds (via two or three abstraction layers)
the code that would allow selecting and configuring such binary.

It can still be mapped via MC, in the sense that you would have:

	- source nodes - one for each sensor
	- sink nodes
	- several binary blocks that do a mix of things

If it would make sense to use MC to setup such pipelines and/or moving the
binary selection logic to libcamera is a separate matter.

IMO, we should first cleanup and drop the abstraction layers, having a
better understanding about the binary selection logic before start
thinking about moving pipeline setup to userspace.

Regards,
Mauro

-

As a reference, those are the binaries inside the
candrpv_0415_20150521_0458 firmware file for ISP2401:

[   19.289518] atomisp-isp2 0000:00:03.0: binary #0  type SP: sp
[   19.289714] atomisp-isp2 0000:00:03.0: binary #1  type ISP (Normal), binary id is  0: isp_copy_var
[   19.289726] atomisp-isp2 0000:00:03.0: binary #2  type ISP (Normal), binary id is  2: isp_vf_pp_full
[   19.289733] atomisp-isp2 0000:00:03.0: binary #3  type ISP (Normal), binary id is  3: isp_vf_pp_opt
[   19.289739] atomisp-isp2 0000:00:03.0: binary #4  type ISP (Normal), binary id is 60: isp_capture_pp_var_bli
[   19.289749] atomisp-isp2 0000:00:03.0: binary #5  type ISP (Normal), binary id is 61: isp_capture_pp_ldc
[   19.289755] atomisp-isp2 0000:00:03.0: binary #6  type ISP (Normal), binary id is  5: isp_capture_pp_var
[   19.289762] atomisp-isp2 0000:00:03.0: binary #7  type ISP (Normal), binary id is  4: isp_yuv_scale_var
[   19.289768] atomisp-isp2 0000:00:03.0: binary #8  type ISP (Normal), binary id is  6: isp_preisp_var
[   19.289776] atomisp-isp2 0000:00:03.0: binary #9  type ISP (Normal), binary id is  7: isp_preisp_var_isp2
[   19.289782] atomisp-isp2 0000:00:03.0: binary #10 type ISP (Normal), binary id is 58: isp_pre_de_var_isp2
[   19.289789] atomisp-isp2 0000:00:03.0: binary #11 type ISP (Normal), binary id is  8: isp_gdc_var
[   19.289795] atomisp-isp2 0000:00:03.0: binary #12 type ISP (Normal), binary id is 11: isp_anr_var
[   19.289801] atomisp-isp2 0000:00:03.0: binary #13 type ISP (Normal), binary id is 12: isp_anr_var_isp2
[   19.289807] atomisp-isp2 0000:00:03.0: binary #14 type ISP (Normal), binary id is  9: isp_postisp_var
[   19.289813] atomisp-isp2 0000:00:03.0: binary #15 type ISP (Normal), binary id is 10: isp_postisp_var_isp2
[   19.289819] atomisp-isp2 0000:00:03.0: binary #16 type ISP (Normal), binary id is 15: isp_preview_dec
[   19.289825] atomisp-isp2 0000:00:03.0: binary #17 type ISP (Normal), binary id is 16: isp_preview_cont_bds125_isp2
[   19.289831] atomisp-isp2 0000:00:03.0: binary #18 type ISP (Normal), binary id is 17: isp_preview_cont_dpc_bds150_isp2
[   19.289847] atomisp-isp2 0000:00:03.0: binary #19 type ISP (Normal), binary id is 19: isp_preview_cont_dpc_bds200_isp2
[   19.289854] atomisp-isp2 0000:00:03.0: binary #20 type ISP (Normal), binary id is 18: isp_preview_cont_bds150_isp2
[   19.289860] atomisp-isp2 0000:00:03.0: binary #21 type ISP (Normal), binary id is 20: isp_preview_cont_bds200_isp2
[   19.289867] atomisp-isp2 0000:00:03.0: binary #22 type ISP (Normal), binary id is 21: isp_preview_var
[   19.289873] atomisp-isp2 0000:00:03.0: binary #23 type ISP (Normal), binary id is 22: isp_preview_var_isp2
[   19.289879] atomisp-isp2 0000:00:03.0: binary #24 type ISP (Normal), binary id is 24: isp_primary_var
[   19.289885] atomisp-isp2 0000:00:03.0: binary #25 type ISP (Normal), binary id is 25: isp_primary_var_isp2
[   19.289891] atomisp-isp2 0000:00:03.0: binary #26 type ISP (Normal), binary id is 26: isp_primary_small
[   19.289897] atomisp-isp2 0000:00:03.0: binary #27 type ISP (Normal), binary id is 27: isp_primary_striped
[   19.289903] atomisp-isp2 0000:00:03.0: binary #28 type ISP (Normal), binary id is 28: isp_primary_striped_isp2
[   19.289909] atomisp-isp2 0000:00:03.0: binary #29 type ISP (Normal), binary id is 29: isp_primary_8mp
[   19.289915] atomisp-isp2 0000:00:03.0: binary #30 type ISP (Normal), binary id is 30: isp_primary_14mp
[   19.289921] atomisp-isp2 0000:00:03.0: binary #31 type ISP (Normal), binary id is 31: isp_primary_16mp
[   19.289926] atomisp-isp2 0000:00:03.0: binary #32 type ISP (Normal), binary id is 33: isp_primary_isp261_stage0
[   19.289932] atomisp-isp2 0000:00:03.0: binary #33 type ISP (Normal), binary id is 34: isp_primary_isp261_stage1
[   19.289938] atomisp-isp2 0000:00:03.0: binary #34 type ISP (Normal), binary id is 35: isp_primary_isp261_stage2
[   19.289947] atomisp-isp2 0000:00:03.0: binary #35 type ISP (Normal), binary id is 36: isp_primary_isp261_stage3
[   19.289954] atomisp-isp2 0000:00:03.0: binary #36 type ISP (Normal), binary id is 37: isp_primary_isp261_stage4
[   19.289960] atomisp-isp2 0000:00:03.0: binary #37 type ISP (Normal), binary id is 38: isp_primary_isp261_stage5
[   19.289966] atomisp-isp2 0000:00:03.0: binary #38 type ISP (Normal), binary id is 42: isp_video_dz
[   19.289971] atomisp-isp2 0000:00:03.0: binary #39 type ISP (Normal), binary id is 44: isp_video_high
[   19.289978] atomisp-isp2 0000:00:03.0: binary #40 type ISP (Normal), binary id is 45: isp_video_nodz
[   19.289984] atomisp-isp2 0000:00:03.0: binary #41 type ISP (Normal), binary id is 46: isp_video_cont_multibds_isp2_min
[   19.289990] atomisp-isp2 0000:00:03.0: binary #42 type ISP (Normal), binary id is 47: isp_video_cont_bds_300_600_isp2_min
[   19.290029] atomisp-isp2 0000:00:03.0: binary #43 type ISP (Normal), binary id is 48: isp_video_cont_dpc_bds150_isp2_min
[   19.290037] atomisp-isp2 0000:00:03.0: binary #44 type ISP (Normal), binary id is 50: isp_video_cont_dpc_bds200_isp2_min
[   19.290044] atomisp-isp2 0000:00:03.0: binary #45 type ISP (Normal), binary id is 49: isp_video_cont_bds150_isp2_min
[   19.290050] atomisp-isp2 0000:00:03.0: binary #46 type ISP (Normal), binary id is 51: isp_video_cont_bds200_isp2_min
[   19.290057] atomisp-isp2 0000:00:03.0: binary #47 type ISP (Normal), binary id is 52: isp_video_cont_nobds_isp2_min
[   19.290063] atomisp-isp2 0000:00:03.0: binary #48 type ISP (Normal), binary id is 53: isp_video_dz_isp2_min
[   19.290070] atomisp-isp2 0000:00:03.0: binary #49 type ISP (Normal), binary id is 54: isp_video_dz_isp2
[   19.290076] atomisp-isp2 0000:00:03.0: binary #50 type ISP (Normal), binary id is 55: isp_video_lp_isp2