[PULL V3][SRU][Jammy] Support Intel IPU6 MIPI camera on Alder Lake platforms

You-Sheng Yang vicamo.yang at canonical.com
Thu Mar 24 15:01:56 UTC 2022


On Thu, Mar 24, 2022 at 8:43 PM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> Overall, this is quite a big patch... If we decide to accept this we
> need to maintain it for many years. Good thing is that it's mostly
> self-contained, it doesn't touch other subsystems in the kernel and the
> code seems to be written pretty well.
>
> I have a few concerns, comments below.
>
> On Mon, Mar 14, 2022 at 10:36:30PM +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.
> >
> > [Where problems could occur]
> >
> > It's confirmed Intel IPU6 MIPI camera doesn't support suspend at
> > streaming.
> >
> > On Jammy, while intel_iommu is turned on in bug 1951440, it breaks
> > current IPU6 driver implementation and can be worked-around with
> > intel_iommu=off temporarily. A follow-up bug 1958004 has been filed.
> >
> > Besides, since UBSAN is also on since Ubuntu 5.15 kernels, multiple
> > warnings have been detected and filed as bug 1958006. No function
> > impact observed so far.
>
> UBSAN warnings can be problematic, in LP: #1958006 I see some
> array-index-out-of-bounds, shift-out-of-bouds, .. these are scary, they
> might be false positives, but if they're not they may introduce nasty
> security-related bugs.
>
> Do you think it's possible to get them fixed before applying this patch
> set?

While these warnings are rooted in values read from hw registers, I've
notified the vendor about our serious concern and demand a quick fix.

> >
> > [Other Info]
> >
> > Before this, we did not submit IPU6 driver for generic kernel due to
> > the lack of a solid commitment from the vendor to upstream this
> > before kernel camera API is out. There is still no such commitment,
> > but during the process we've been able to address version migration
> > issues and we would continue to do so. We're getting to have the
> > confidence that it's possible to maintain and share the support for
> > generic Ubuntu users.
> >
> > Fixes for oem-5.14 have been merged, so here we nominate for Jammy
> > only. This patchset is almost identical to that for oem-5.14 except
> > one additioanl fix for 5.15 kernel API change in "UBUNTU: SAUCE: Fix
> > build error for kernel 5.15".
> >
> > V2: no change rebase only.
> > V3: build only on amd64 and add config annotations
>
> ...
>
> > diff --git a/drivers/media/pci/intel/Makefile b/drivers/media/pci/intel/Makefile
> > index 0b4236c4db49..608acd574921 100644
> > --- a/drivers/media/pci/intel/Makefile
> > +++ b/drivers/media/pci/intel/Makefile
> > @@ -3,4 +3,13 @@
> >  # Makefile for the IPU3 cio2 and ImGU drivers
> >  #
> >
> > -obj-y        += ipu3/
> > +# force check the compile warning to make sure zero warnings
> > +# note we may have build issue when gcc upgraded.
> > +subdir-ccflags-y := -Wall -Wextra
> > +subdir-ccflags-y += $(call cc-disable-warning, unused-parameter)
> > +subdir-ccflags-y += $(call cc-disable-warning, implicit-fallthrough)
> > +subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
> > +subdir-ccflags-$(CONFIG_VIDEO_INTEL_IPU_WERROR) += -Werror
>
> Hm.. this looks bad. Why do we need to hide these warnings?

I've tested this should be unnecessary and no other warning is dumped
except to unused functions. I can add an additional commit to remove
these cflags overrides.

> For the rest of the code it's pretty hard to do a full review, it's
> really a lot, I'll take a quick look later and send more comments if I
> spot any other potential issues.
>
> For now I think the most critical issue that I see is about the UBSAN
> warnings.
>
> Thanks,
> -Andrea



-- 
Regards,
You-Sheng Yang



More information about the kernel-team mailing list