2021-10-30 10:54:29

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 00/17] various fixes for atomisp to make it work

Em Sat, 30 Oct 2021 18:50:14 +0900
Tsuchiya Yuto <[email protected]> escreveu:

> On Thu, 2021-10-28 at 11:58 +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Oct 2021 13:32:29 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
> >
> > > <Fixed Cc list>
> > >
> > > On Mon, 2021-10-18 at 01:19 +0900, Tsuchiya Yuto wrote:
> > > > [...]
> > > > ## taking a picture with atomisp
> > > >
> > > > Note that to try to take a picture, please also apply at least the
> > > > this RFC patch ("[BUG][RFC] media: atomisp: pci: assume run_mode is
> > > > PREVIEW") I'll send as almost a BUG report later.
> > > >
> > > > You need to use firmware version irci_stable_candrpv_0415_20150521_0458,
> > > > which is available from the intel-aero [1]
> > >
> > > Just in case, the hash (as well as version) of firmware which I
> > > downloaded from intel-aero and I use to capture is the following:
> > >
> > > $ sha256sum /lib/firmware/shisp_2401a0_v21.bin
> > > e89359f4e4934c410c83d525e283f34c5fcce9cb5caa75ad8a32d66d3842d95c /lib/firmware/shisp_2401a0_v21.bin
> > >
> > > $ strings /lib/firmware/shisp_2401a0_v21.bin | grep 2015
> > > irci_stable_candrpv_0415_20150521_0458
>
> Also note that the firmware file from the intel-aero only supports
> hw_revision 2401a0_v21 as the filename implies. So, if someone have
> Bay Trail (ISP2400) device to test, you need to get a firmware file (from
> somewhere like Android installation/image as the initial commit of atomisp
> mentions) made for version irci_stable_candrpv_0415_20150521_0458 and
> hw_revision 2400b0_v21 then place it under /lib/firmware

Yeah, understood.

> > > > The atomisp (ipu2), like the ipu3, needs userspace support. The libcamera
> > > > has now decent ipu3 support but does not have atomisp support yet.
> > > >
> > > > I found some userspace tools for atomisp that run on Linux:
> > > >
> > > > - capturev4l2 from intel-aero/sample-apps
> > > > (https://github.com/intel-aero/sample-apps/tree/master/capturev4l2)
> > > > - hd-camera from intel-aero/sample-apps
> > > > (https://github.com/intel-aero/sample-apps/tree/master/hd-camera)
> > > > - intel/nvt
> > > > (https://github.com/intel/nvt)
> > > >
> > > > It looks like the nvt is the most feature-rich, like exposure and white
> > > > balance. Note that current upstreamed atomisp dropped 32-bit support.
> > > > So, you need to build it with `-m64` (change it in Makefile). Here is
> > > > the example of usage I use on mipad2:
> > > >
> > > > $ ./v4l2n -o [email protected] \
> > > > --device /dev/video2 \
> > > > --input 0 \
> > > > --exposure=30000,30000,30000,30000 \
> > > > --parm type=1,capturemode=CI_MODE_PREVIEW \
> > > > --fmt type=1,width=1920,height=1080,pixelformat=NV12 \
> > > > --reqbufs count=2,memory=USERPTR \
> > > > --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
> > > > --capture=2 \
> > > >
> > > > ./raw2pnm -x1920 -y1080 -fNV12 testimage_001.raw testimage_001.pnm
> > > > feh *.pnm # open the converted image
> > > > rm testimage*
> >
> > Great! that worked for me too on Asus T101HA (CHT). I had to tweak the
> > resolution, as ov2680 sensor has a max of 1616x1216 30fps. I had
> > to use a number smaller than that, though (1600x1200).
>
> Ah, glad to hear that!
>
> > I guess the next step is to make a generic app to also work on it.
>
> It's great if we can eventually add atomisp support to the libcamera.
> I think this is the easiest way to support generic apps (I mean, like
> cheese). Some ipu3 cameras already work on cheese with libcamera.
> I don't have any knowledge about userspace support though.

There are a lot more to be done in order to make it ready for libcamera
(if ever).

See, libcamera assumes that the device exports its internal pipelines
via the media controller. The atomisp code setups such pipelines
internally, exposing a "normal" [1] V4L2 interface.

[1] Well, it misses several V4L2 ioctl implementations that currently
causes generic applications to fail, and mis-implement others,
but the idea behind the driver is to fully control the driver via
/dev/video? nodes, without requiring the media controller API.

Converting atomisp into a media-controller driver will require a
major rework. I suspect that it is a lot easier to make it work with
normal V4L2 applications by fixing the ioctl implementation than
to port it to MC.

Yet, in order to be able to move it from staging, we'll need to convert
it into an MC-controlled driver.

What I'm saying is that, IMHO, we should:

1. Fix the ioctls in order to allow a normal app to use it. I'm
already doing some work on this sense. We should ensure that the
driver will pass v4l2-compliance tests on this step;

2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
register address differences between ISP2400 and ISP2401;

3. Cleanup the driver code, removing the abstraction layers inside it;

4. Migrate the sensor drivers out of staging (or re-using existing
drivers);

5. Remove the logic which sets up pipelines inside it, moving it to
libcamera and implement MC support;

6. Move it out of staging.

This is easily said than done, as steps 2-6 are very complex and will
require lots of work. Also, both ISP2400 and 2401 should be tested
while doing some of those major reworks, in order to avoid breakages.

Btw, v4l2grab app (at v4l-utils) already works. This is a very simple
app, written to allow stream testing. It doesn't do anything fancy,
like trying to enumerate the formats, and it needs to be set to a
resolution lower than the one announced by the sensor, probably due
to some bug at the COPY pipeline settings at atomisp driver.

qv4l2, for instance, causes a driver OOPS when it calls G/S_PRIORITY
ioctls.

Regards,
Mauro


2021-10-30 11:05:12

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 00/17] various fixes for atomisp to make it work

On Sat, Oct 30, 2021 at 1:49 PM Mauro Carvalho Chehab
<[email protected]> wrote:
> Em Sat, 30 Oct 2021 18:50:14 +0900
> Tsuchiya Yuto <[email protected]> escreveu:

...

> What I'm saying is that, IMHO, we should:
>
> 1. Fix the ioctls in order to allow a normal app to use it. I'm
> already doing some work on this sense. We should ensure that the
> driver will pass v4l2-compliance tests on this step;
>
> 2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
> register address differences between ISP2400 and ISP2401;
>
> 3. Cleanup the driver code, removing the abstraction layers inside it;
>
> 4. Migrate the sensor drivers out of staging (or re-using existing
> drivers);
>
> 5. Remove the logic which sets up pipelines inside it, moving it to
> libcamera and implement MC support;
>
> 6. Move it out of staging.
>
> This is easily said than done, as steps 2-6 are very complex and will
> require lots of work. Also, both ISP2400 and 2401 should be tested
> while doing some of those major reworks, in order to avoid breakages.

This is a great roadmap nevertheless!
However, we missed one important step here, i.e. persuade Intel to
clarify license of the firmware (for distirbution) and make sure we
have it in Linux firmware project, so it won't get lost.


> Btw, v4l2grab app (at v4l-utils) already works. This is a very simple
> app, written to allow stream testing. It doesn't do anything fancy,
> like trying to enumerate the formats, and it needs to be set to a
> resolution lower than the one announced by the sensor, probably due
> to some bug at the COPY pipeline settings at atomisp driver.
>
> qv4l2, for instance, causes a driver OOPS when it calls G/S_PRIORITY
> ioctls.


--
With Best Regards,
Andy Shevchenko

2021-10-30 18:34:54

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 00/17] various fixes for atomisp to make it work

Em Sat, 30 Oct 2021 14:01:01 +0300
Andy Shevchenko <[email protected]> escreveu:

> On Sat, Oct 30, 2021 at 1:49 PM Mauro Carvalho Chehab
> <[email protected]> wrote:
> > Em Sat, 30 Oct 2021 18:50:14 +0900
> > Tsuchiya Yuto <[email protected]> escreveu:
>
> ...
>
> > What I'm saying is that, IMHO, we should:
> >
> > 1. Fix the ioctls in order to allow a normal app to use it. I'm
> > already doing some work on this sense. We should ensure that the
> > driver will pass v4l2-compliance tests on this step;
> >
> > 2. remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
> > register address differences between ISP2400 and ISP2401;
> >
> > 3. Cleanup the driver code, removing the abstraction layers inside it;
> >
> > 4. Migrate the sensor drivers out of staging (or re-using existing
> > drivers);
> >
> > 5. Remove the logic which sets up pipelines inside it, moving it to
> > libcamera and implement MC support;
> >
> > 6. Move it out of staging.
> >
> > This is easily said than done, as steps 2-6 are very complex and will
> > require lots of work. Also, both ISP2400 and 2401 should be tested
> > while doing some of those major reworks, in order to avoid breakages.
>
> This is a great roadmap nevertheless!

> However, we missed one important step here, i.e. persuade Intel to
> clarify license of the firmware (for distirbution) and make sure we
> have it in Linux firmware project, so it won't get lost.

Heh, true!

I suspect that the firmare for ISP2401, used on Intel Aero, is probably
easier to make it available than the firmware for ISP2400.

At least looking at the github repository for Intel Aero, it says at the
package metadata that its content is under GPL:

https://github.com/intel-aero/meta-intel-aero-base/blob/master/LICENSE

The firmware itself is there, at:

https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin


The Yocto's meta package for the firmware blob is at:

https://github.com/intel-aero/packages/tree/master/firmware-atomisp

Also says, on its copyright's notice:

https://github.com/intel-aero/packages/blob/master/firmware-atomisp/debian/copyright

that:

Source: https://github.com/intel-aero/meta-intel-aero-base

Files: *
Copyright: 2016 Intel Corporation
License: GPL-2
/usr/share/common-licenses/GPL-2

Granted, it doesn't make sense to say that a firmware blob is under
GPL. My reading is that the original intend was to allow GPL software
to use/distribute such firmware, but yeah, someone at Intel should
provide the community an explicit authorization to allow distros to
start packaging such firmware.

Regards,
Mauro