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

[Hans de Goede <hdegoede@redhat.com>]

<Added some more people to the Cc who are also definitely interested in this>

Hi Tsuchiya,

On 10/17/21 18:19, Tsuchiya Yuto wrote:
> Hi all,
> 
> This patch series contains fixes for atomisp to work (again). Tested on
> Microsoft Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with
> v5.15-rc5. Both are Cherry Trail (ISP2401) devices.
> 
> I'm still not used to Linux patch sending flow. Sorry in advance
> if there is some weirdness :-) but I did my best.

You actually did pretty good AFAICT.

Also thank you so much for working on this. Finally getting working
atomisp2 support is awesome!

I actually have been working on and off on this myself
(even did a bit of work on it this weekend). My plan was to:

1. Find android (5.1) x86 kernel sources which I can build from
source and get a working Android (on a device with Android pre-installed)
with a kernel build from source -> Done

2. Get a regular Linux distro to boot with the kernel from 1. -> Done
   (normal Linux v4l2 utils do not give a picture, might be the need
    to set preview mode)

3. Add ioctl debugging to the kernel from 1. capture a trace to see what
Android userspace is doing -> Done .  See here for an example log:

https://fedorapeople.org/~jwrdegoede/atomisp-logs-t116/

4. Write a userspace utility mimicking Android userspace, I started on
this this weekend

5. Once I've 4. working the plan was a bit vague, the idea was to see if
I could use that to quickly get the mainline staging version to work; or
alternatively forward port the working Android kernel sources to
mainline from scratch.

But it looks like 4 and 5 will no longer be necessary, which is awesome,
thank you so much!

I'll try to look into this series in more detail; and try to get thing
working on one of my own devices when I can make some time for this.









> 
> I'll send another series that contains RFC patches later named ("bug
> reports for atomisp to make it work"). To try to capture images, applying
> those RFC patches are also needed.
> 
> The following 1st-7th patches are fixes for the upstreamed atomisp:
> 
>     media: atomisp: pci: add missing media_device_cleanup() in
>       atomisp_unregister_entities()
>     media: atomisp: pci: fix punit_ddr_dvfs_enable() argument for
>       mrfld_power up case
>     media: atomisp: pci: fix inverted logic in buffers_needed()
>     media: atomisp: pci: do not use err var when checking port validity
>       for ISP2400
>     media: atomisp: pci: fix inverted error check for
>       ia_css_mipi_is_source_port_valid()
>     media: atomisp: pci: use IA_CSS_ERROR() for error messages in
>       sh_css_mipi.c
>     media: atomisp: pci: fix ifdefs in sh_css.c
> 
> The following 8th-13th patches partially reverts driver version back
> to irci_stable_candrpv_0415_20150521_0458:
> 
>     media: atomisp: pci: make fw ver
>       irci_stable_candrpv_0415_20150521_0458 work - 1/5
>     media: atomisp: pci: make fw ver
>       irci_stable_candrpv_0415_20150521_0458 work - 2/5
>     media: atomisp: pci: make fw ver
>       irci_stable_candrpv_0415_20150521_0458 work - 3/5
>     media: atomisp: pci: make fw ver
>       irci_stable_candrpv_0415_20150521_0458 work - 4/5
>     media: atomisp: pci: make fw ver
>       irci_stable_candrpv_0415_20150521_0458 work - 5/5
>     media: atomisp: pci: release_version is now
>       irci_stable_candrpv_0415_20150521_0458
> 
> One of the issues on the upstreamed atomisp is, the driver is a result
> of the following two different versions of driver merged by tools using
> `ifdef ISP2401`:
> 
>     - ISP2400: irci_stable_candrpv_0415_20150521_0458
>     - ISP2401: irci_master_20150911_0724
> 
> and we don't have such firmware made for irci_master_20150911_0724.

Right this is something which I also noticed (but I did not do anything with /
about yet)

> I confirmed that the driver version irci_stable_candrpv_0415_20150521_0458
> works well on the intel-aero version atomisp for ISP2401, too.

"irci_stable_candrpv_0415_20150521_0458" is also the version of the atomisp
firmware shipped in CHT tablets which come with Android pre-installed. So
I agree that this is the version which we should go for.


> Here is
> my port, if someone is interested [2]:
> 
> So, eventually, such ISP version tests caused by just the driver version
> difference can be removed (not just being unified but removed).

Ack.

> 
> That said, it may take longer time until we remove such tests. So, for
> now I thought it may be better to focus on just making atomisp work by
> partially reverting the incompatible things for the firmware version
> irci_stable_candrpv_0415_20150521_0458.

Ack.

<snip>

>   ## about (a lot of) ISP2401 ifdef tests
> 
> When porting intel-aero version atomisp to mainline, I thought almost
> all the `ifdef ISP2401` things are the result of two different driver
> version merged by tools.
> 
> To confirm that, I tried removing ifdefs for the initial commit of
> upstreamed atomisp [1]. And I can successfully take a picture there on
> surface3.
> 
> Currently, I can remove ifdefs up to commit bd674b5a413c ("media: atomisp:
> cleanup ifdefs from ia_css_debug.c") [2] which is before 641c2292bf19 ("media:
> atomisp: get rid of version-dependent globals"). Up to there, I stopped
> and realized it may take some time to remove ifdefs for current atomisp.
> So, instead of removing ifdefs, I partially reverted incompatible parts
> in this series for now.
> 
> The ifdefs for the real hardware difference is like the following which
> were removed or integrated into `ifdef ISP2401` on commit 641c2292bf19
> ("media: atomisp: get rid of version-dependent globals") and bd674b5a413c
> ("media: atomisp: cleanup ifdefs from ia_css_debug.c"):
> 
>   - HAS_NO_INPUT_FORMATTER
>   - USE_INPUT_SYSTEM_VERSION_2
>   - USE_INPUT_SYSTEM_VERSION_2401
>   ...
> 
> I need to elaborate on this ifdef thing later (and I'll do later), but
> for now, let's focus on make it just work...

Just focusing on making it work sounds good to me. I also have quite a few
Bay Trail devices, so I would love to also see the 2400 version working.

But one step at a time lets focus on CHT / the 2401 for now.

> For devices which use intel_pmic_bytcrc driver, you need to add i2c
> address. I sent RFC patch earlier named ("ACPI / PMIC: Add i2c address
> to intel_pmic_bytcrc driver").

I'll reply to that patch in its own thread (it needs some work,
but we should be able to get something ready for upstream easily).

> Also, sensor drivers are not upstream. Take a look at my working tree
> if someone is interested [1].
> 
> I made world-facing camera (OV8835) work, which the driver is from the
> old Android kernel tree. Unfortunately, the user-facing camera (AR0330)
> is not working yet; the output is completely black. I'm not sure why,
> maybe the sensor power issue (atomisp_gmin_platform) or sensor driver
> issue, which the driver is from non-atomisp driver.
> 
> [1] https://github.com/kitakar5525/linux-kernel/commits/mainline+upst_atomisp
> 
>   ## for mipad2 (and whiskey cove pmic based devices)
> 
> For devices which equip whiskey cove PMIC, you need to add non-upstream
> regulator driver [1].
> 
> [1] work done by jekhor, which seems to be from intel-aero or old
>     Android kernel
>     https://github.com/jekhor/yogabook-linux-kernel/commit/11c05b365fb2eeb4fced5aa66b362c511be32a34
>     ("intel_soc_pmic_chtwc: Add regulator driver and definition for VPROG1B")

Interesting I recently bought a 2nd hand mipad2 myself too. I still need
to put Linux on there. I'm definitely motivated to do that now :)

Regards,

Hans