ACK/Cmnt: [PULL v7][SRU][Jammy] Support Intel IPU6 MIPI camera on Alder Lake platforms

[You-Sheng Yang]

On Wed, May 25, 2022 at 9:30 PM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> On Wed, May 25, 2022 at 06:38:44PM +0800, You-Sheng Yang wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1955383
> >
> > [Impact]
> >
> > To support Intel IPU6 MIPI camera on Alder Lake platforms.
> >
> > [Fix]
> >
> > Initial support for Intel IPU6 MIPI camera on Tiger Lake platforms has
> > been addressed by bug 1921345 and 1939539. They are backported from
> > https://github.com/intel/ipu6-drivers.
> >
> > Further works to enable IPU6 camera on Alder Lake platforms depend on a
> > few more fixes from same ipu6-drivers repository, as well as an extra
> > https://github.com/intel/ivsc-driver for Intel Vision Sensing
> > Controller(IVSC).
> >
> > [Test Case]
> >
> > This depends on an integral of enablement components inclusive of the
> > kernel drivers that are being proposed, firmware, updates for the
> > userspace camera hardware abstration layer library and a gstreamer
> > element as what we have for Tiger Lake platforms.
> >
> > Due to previous regression on another TGL-based SKU, reboot stress
> > tests were also enrolled on following platforms/SKUs:
> >
> >   * TGL/ov01a1s sensor, SKU 2
> >   * TGL/ov01a1s sensor, SKU 3
> >   * TGL/hm11b1 sensor
> >   * Tributo
> >   * AndrewsMLK
> >
> > [Where problems could occur]
> >
> > It's confirmed Intel IPU6 MIPI camera doesn't support suspend at
> > streaming.
> >
> > It's also a known issue that we don't have support on IPU6 TGL
> > platforms as this initial kernel 5.15 porting. This is being worked
> > on by the vendor.
> >
> > [Other Info]
> >
> > V7: Drop previous Acked-By, Signed-off-by from commit messages.
> >
> > V6: Added additional commits for the stability issue that regressed in
> >     Ubuntu-5.15.0-28.29 and reverted in Ubuntu-5.15.0-29.30.
> >
> > V5: correct wrong branch/repo generated in previous rev
> >
> > V4: drop obsoleted cflags manipulation and fix UBSAN warnings
> >
> > V3: build only on amd64 and add config annotations
> >
> > V2: no change rebase only.
>
> I've looked quickly at all the code, of course it's quite difficult to
> review the code itself, but I didn't spot any obvious errors. I see
> that there's a lot of importing stuff and dropping what we don't need,
> I'm wondering if it's be better to squash some of these commits
> together into a single initial import SAUCE patch, but maybe this way is
> better to keep track of the upstream commits and figure out from where
> we started.
>
> A few comments below.
>
> 1) In the first set of commits:
>
>  1f69ec679b19 UBUNTU: SAUCE: IPU driver release WW14
>  7d3cff6e365e UBUNTU: SAUCE: IPU driver release WW04
>  a5733bc898ab UBUNTU: SAUCE: IPU driver release WW52
>  db472c127487 UBUNTU: SAUCE: IPU driver release WW48 with MCU
>  8d6d4af24f5b UBUNTU: SAUCE: IPU driver release WW48
>  07e7d9202f9b UBUNTU: SAUCE: intel ipu drivers first release
>
> it would be nice to know what kind of backport activity was required, I see
> that you pretty much imported the initial code from
> github.com/intel/ipu6-drivers, what kind of adjustment you had to do to
> apply this code to the jammy kernel? Just a few context adjustments or
> you had to fix some ABI changes?

The upstream repo is a partial kernel tree that contains only
ipu6/ivsc drivers. It, along with ivsc-driver, finally gained the
ability to build inlinely as a DKMS source tree. So, these backports
were for accommodating Makefile/Kconfig contents to our kernel source
tree.

> 2) I think I've asked this before, but who's going to maintain this? Do
> we need to actively keep track of github.com/intel/ipu6-drivers and
> apply stable fixes in the future?

Two parts. From oem-5.17 and on, we're to use the DKMS format instead.
Bugs for tracking this are so far:

  * https://launchpad.net/bugs/1972106: import ivsc-driver to Ubuntu archive
  * https://launchpad.net/bugs/1972109: import ipu6-drivers

So there will not be ipu6/ivsc drivers in the kernel tree starting
from 5.17, but it's also true there might still be fixes specialized
for IPU6 to be backported into generic/oem kernel trees, such as
https://bugs.launchpad.net/bugs/1958004.

With https://bugs.launchpad.net/bugs/1969434 made its way into Jammy,
we may reconsider doing the same thing as oem-5.17 -- use dkms instead
and remove in kernel IPU6 drivers.

And, yes, even if we switched to DKMS method, we'll still have to
monitor the changes from upstream repo as long as we would like to
continue supporting IPU6 cameras in oem projects, except it's now
easier because the upstream repos have been converted to DKMS source
trees already. No backporting to generic/oem kernel trees will then be
required.

>  Is there any plan to merge this upstream?

Intel's plan was to upstream their IPU7 (not IPU6) driver on 2024, and
Dell and Canonical have raised the request to upstream IPU6 driver as
well explicitly.

> 3) Which commits have been added to address the regressions of
> Ubuntu-5.15.0-28.29? Just curious to see what was the problem and maybe
> check for similar patterns in the code.

The problem was that the last 10 commits from this pull request break
IPU6 on TGL platforms, but we need 5.15 kernel for ADL platforms only,
for now. On the first time these drivers were proposed for 5.15, I
skipped the last 10 commits so that proposed changeset were identical
to those in oem-5.14 (with a few fixes for 5.15 of course), but it
turned out to hang one of the TGL MIPI camera platform while loading
ljca driver from ivsc-driver repo. So it's a choice between hanging
one of the TGL platforms or failing to enable the camera of all TGL
platforms. We now pick the latter, and Intel is working on identifying
the root cause before we will migrate oem-5.14 to hwe-5.15 a couple of
months later.

> Apart than that the code seems pretty well self-contained, even if it
> huge and I'm still worried about maintenance / security fixes, but
> overall I think we can apply it, so:
>
> Acked-by: Andrea Righi <andrea.righi at canonical.com>



-- 
Regards,
You-Sheng Yang