Re: [PATCH v3 00/15] Intel IPU6 and IPU6 input system drivers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jan 16, 2024 at 05:57:14PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 1/16/24 17:54, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Tue, Jan 16, 2024 at 05:12:50PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 1/11/24 07:55, bingbu.cao@intel.com wrote:
> >>> From: Bingbu Cao <bingbu.cao@intel.com>
> >>>
> >>> This patch series adds a driver for Intel IPU6 input system.
> >>> IPU6 is the sixth generation of Imaging Processing Unit, it is a PCI
> >>> device which can be found in some Intel Client Platforms. User can use
> >>> IPU6 to capture images from MIPI camera sensors.
> >>>
> >>> IPU6 has its own firmware which exposes ABIs to driver, and communicates
> >>> with CSE to do firmware authentication. IPU6 has its MMU hardware, so
> >>> the driver sets up a page table to allow IPU6 DMA to access the system
> >>> memory.
> >>>
> >>> IPU6 input system driver uses MC and V4L2 sub-device APIs besides V4L2.
> >>> ---
> >>> v2 -> v3:
> >>>   - Add line-based metadata capture support
> >>>   - Fix header files inclusion issues
> >>>   - Fix the CSI2 timing calculation
> >>>   - Fix crash when remove module during streaming
> >>>   - Remove some unused code
> >>>   - Use cross-referencing links in documentation
> >>>   - Update Makefile to use ":=" for objects
> >>>   - Fix several bugs and coding style issues
> >>
> >> So I've given this version a try on a Lenovo X1 yoga gen 8 with ov2740
> >> sensor using the ongoing libcamera SoftISP work + this small patch
> >> to enable the SoftISP on IPU6 :
> >>
> >> https://github.com/jwrdegoede/libcamera/commit/3172f3703cf7076390fbf86c3b43e388c2422b31
> >>
> >> things work fine when using patch 1-15 + 17 on top of 6.7, note
> >> I'm skipping patch 16 ("media: ipu6/isys: support line-based
> >> metadata capture support")" here!
> >>
> >> However when I instead apply the whole series on top of:
> >> https://git.linuxtv.org/sailus/media_tree.git/log/?h=metadata
> >>
> >> Then things stop working, with the following errors
> >> (I added extra error logging to figure out in which syscall
> >> resetRoutingTable() fails and made libcamera ignore the routing
> >> errors):
> >>
> >> [2:02:04.466310686] [8943] ERROR SimplePipeline simple.cpp:1443 GetRouting() failed -25
> >> [2:02:04.466315975] [8943] ERROR SimplePipeline simple.cpp:1574 Failed to reset routes for /dev/v4l-subdev1: Inappropriate ioctl for device
> >> [2:02:04.466366331] [8943] ERROR SimplePipeline simple.cpp:1443 GetRouting() failed -25
> >> [2:02:04.466370025] [8943] ERROR SimplePipeline simple.cpp:1574 Failed to reset routes for /dev/v4l-subdev4: Inappropriate ioctl for device
> >>  	[2:03:32.334708887] [8929]  INFO Camera camera.cpp:1183 configuring streams: (0) 1924x1088-BGR888
> >> [2:03:32.335129023] [8943] ERROR SimplePipeline simple.cpp:1205 Unable to configure capture in 1932x1092-BA10 (got 0x0-@...)
> >> Failed to configure camera.
> >>
> >> I was sorta assuming that the new metadata-stream support would
> >> be backwards compatible for userspace without support for this,
> >> so I think this is a bug ?
> > 
> > That's the intention when it comes to the kernel APIs indeed.
> > 
> > I wonder how the simple pipeline handler works with this, does it try to
> > configure both streams all the way from the internal source pad to the
> > video node? This will certainly fail without metadata support in the ISYS
> > driver. Just guessing the cause though. An extra stream from the source pad
> > won't fail pipeline validation.
> > 
> > I should be able to set up a system to test this, too.
> 
> I did include Bingbu's patch to add metadata to the isys driver for my
> testing (and I also tried reverting just that patch). I think the issue
> might be that libcamera already has some streams awareness based on
> an older streams patch-set.

Correct. It uses the G_ROUTING and S_ROUTING ioctls, and those have
chenged in the latest kernel patch series.

The simple pipeline handler doesn't configure routing as such, it gets
the default routes with G_ROUTING(TRY) if the device advertises routing
support, and calls S_ROUTING(ACTIVE) to reset the routes to the default.
It then uses the routes to walk the pipeline, but doesn't change them.

> I'll retest when Laurent's branch for this is ready.

-- 
Regards,

Laurent Pinchart