Re: [RFC PATCH 0/7] Intel IPU7 PCI and input system device drivers

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

Hi Sakari,

On Fri, Feb 21, 2025 at 12:55:09PM +0000, Sakari Ailus wrote:
> On Fri, Feb 21, 2025 at 03:52:45PM +0800, bingbu.cao@intel.com wrote:
> > From: Bingbu Cao <bingbu.cao@intel.com>
> > 
> > This patch series adds a driver for Intel IPU7 input system.
> > IPU7 is the seventh generation of Imaging Processing Unit, it is a
> > PCI device which can be found in some Intel Client Platforms such as
> > Intel Lunar Lake and Panther Lake, etc.
> > User can use IPU7 to capture images from MIPI camera sensors, support
> > both MIPI CSI DPHY and CPHY.
> > 
> > IPU7 input system driver uses MC and V4L2 sub-device APIs besides V4L2.
> > 
> >  - ipu7.c: the IPU7 PCI device driver probe, basic hardware
> >    configuration and initialization
> >  - ipu7-bus: Initialise and register the auxiliary device for IS
> >  - ipu7-buttress: Hardware buttress interfaces to allow driver to
> >    program to enable the power, firmware authentication and interrupt
> >    handling.
> >  - ipu7-mmu and ipu7-dma: IPU MMU table setup and IPU DMA mapping APIs
> >  - ipu7-cpd and ipu7-boot: firmware format parse and firmware boot flow
> >  - abi: a series of ABI headers used by driver and firmware
> >  - ipu7-syscom: syscom communication interfaces used by driver to queue
> >    commands to firmware
> >  - ipu7-isys: main IPU7 input system driver, it registers CSI2 as v4l2
> >    sub-devices and the capture as video devices.
> 
> Given that much of the driver code is the same than in the IPU6 driver (in
> particular split into devices, DMA, MMU and V4L2 / MC API related code), we
> should share at least that instead of looking to maintain two copies of it
> separately. The IPU7 driver probably has had a different starting point for
> development than the current IPU6 driver so there are a fair amount of
> differences in that code now but it is largely not due to differences in
> the hardware itself, for instance, the lack of metadata or multiple virtual
> channel support you listed below.

Merging the two drivers in one seems a good idea to me, with lots of
upsides, and very few downsides, if any. If there are strong technical
reasons why this can't be done we should discuss them.

> > TODO list:
> >  1. ABI headers cleanup (currently a copy from firmware as it is
> >     still in development and updating regularly, need cleanup to unifiy
> >     the headers)
> >  2. add metadata capture support
> >  3. add multiple virtual-channels capture support
> >  4. remove unsecure mode code as secure mode is force to enabled in
> >     product
> > 
> > Bingbu Cao (7):
> >   media: Rename the IPU PCI device table header and add IPU7 PCI IDs
> >   media: ipu7: add Intel IPU7 PCI device driver
> >   media: ipu7: add IPU7 DMA APIs and MMU mapping
> >   media: ipu7: add firmware parse, syscom interface and boot sequence
> >   media: ipu7: add IPU7 firmware ABI headers
> >   media: ipu7: add IPU7 input system device driver
> >   media: ipu7: add Makefile and Kconfig for IPU7
> > 
> >  drivers/media/pci/intel/Kconfig               |    1 +
> >  drivers/media/pci/intel/Makefile              |    1 +
> >  drivers/media/pci/intel/ipu6/ipu6.c           |    2 +-
> >  drivers/media/pci/intel/ipu7/Kconfig          |   18 +
> >  drivers/media/pci/intel/ipu7/Makefile         |   23 +
> >  .../pci/intel/ipu7/abi/ipu7_fw_boot_abi.h     |  164 +
> >  .../pci/intel/ipu7/abi/ipu7_fw_common_abi.h   |  243 ++
> >  .../pci/intel/ipu7/abi/ipu7_fw_config_abi.h   |   19 +
> >  .../intel/ipu7/abi/ipu7_fw_insys_config_abi.h |   19 +
> >  .../pci/intel/ipu7/abi/ipu7_fw_isys_abi.h     |  495 +++
> >  .../pci/intel/ipu7/abi/ipu7_fw_msg_abi.h      |  509 +++
> >  .../intel/ipu7/abi/ipu7_fw_psys_config_abi.h  |   22 +
> >  .../pci/intel/ipu7/abi/ipu7_fw_syscom_abi.h   |   60 +
> >  drivers/media/pci/intel/ipu7/ipu7-boot.c      |  434 +++
> >  drivers/media/pci/intel/ipu7/ipu7-boot.h      |   21 +
> >  drivers/media/pci/intel/ipu7/ipu7-bus.c       |  158 +
> >  drivers/media/pci/intel/ipu7/ipu7-bus.h       |   69 +
> >  .../media/pci/intel/ipu7/ipu7-buttress-regs.h |  465 +++
> >  drivers/media/pci/intel/ipu7/ipu7-buttress.c  | 1187 +++++++
> >  drivers/media/pci/intel/ipu7/ipu7-buttress.h  |   84 +
> >  drivers/media/pci/intel/ipu7/ipu7-cpd.c       |  230 ++
> >  drivers/media/pci/intel/ipu7/ipu7-cpd.h       |   62 +
> >  drivers/media/pci/intel/ipu7/ipu7-dma.c       |  509 +++
> >  drivers/media/pci/intel/ipu7/ipu7-dma.h       |   50 +
> >  drivers/media/pci/intel/ipu7/ipu7-fw-isys.c   |  300 ++
> >  drivers/media/pci/intel/ipu7/ipu7-fw-isys.h   |   39 +
> >  .../media/pci/intel/ipu7/ipu7-isys-csi-phy.c  | 1051 +++++++
> >  .../media/pci/intel/ipu7/ipu7-isys-csi-phy.h  |   16 +
> >  .../pci/intel/ipu7/ipu7-isys-csi2-regs.h      | 1197 +++++++
> >  drivers/media/pci/intel/ipu7/ipu7-isys-csi2.c |  565 ++++
> >  drivers/media/pci/intel/ipu7/ipu7-isys-csi2.h |   73 +
> >  .../media/pci/intel/ipu7/ipu7-isys-queue.c    |  836 +++++
> >  .../media/pci/intel/ipu7/ipu7-isys-queue.h    |   79 +
> >  .../media/pci/intel/ipu7/ipu7-isys-subdev.c   |  381 +++
> >  .../media/pci/intel/ipu7/ipu7-isys-subdev.h   |   55 +
> >  .../media/pci/intel/ipu7/ipu7-isys-video.c    | 1130 +++++++
> >  .../media/pci/intel/ipu7/ipu7-isys-video.h    |  117 +
> >  drivers/media/pci/intel/ipu7/ipu7-isys.c      | 1170 +++++++
> >  drivers/media/pci/intel/ipu7/ipu7-isys.h      |  150 +
> >  drivers/media/pci/intel/ipu7/ipu7-mmu.c       |  853 +++++
> >  drivers/media/pci/intel/ipu7/ipu7-mmu.h       |  414 +++
> >  .../media/pci/intel/ipu7/ipu7-platform-regs.h |  146 +
> >  drivers/media/pci/intel/ipu7/ipu7-syscom.c    |   79 +
> >  drivers/media/pci/intel/ipu7/ipu7-syscom.h    |   36 +
> >  drivers/media/pci/intel/ipu7/ipu7.c           | 2791 +++++++++++++++++
> >  drivers/media/pci/intel/ipu7/ipu7.h           |  244 ++
> >  drivers/media/pci/intel/ivsc/mei_csi.c        |    2 +-
> >  .../{ipu6-pci-table.h => ipu-pci-table.h}     |   14 +-
> >  48 files changed, 16578 insertions(+), 5 deletions(-)
> >  create mode 100644 drivers/media/pci/intel/ipu7/Kconfig
> >  create mode 100644 drivers/media/pci/intel/ipu7/Makefile
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_boot_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_common_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_config_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_insys_config_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_isys_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_msg_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_psys_config_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/abi/ipu7_fw_syscom_abi.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-boot.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-boot.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-bus.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-bus.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-buttress-regs.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-buttress.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-buttress.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-cpd.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-cpd.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-dma.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-dma.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-fw-isys.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-fw-isys.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-csi-phy.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-csi-phy.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-csi2-regs.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-csi2.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-csi2.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-queue.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-queue.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-subdev.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-subdev.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-video.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys-video.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-isys.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-mmu.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-mmu.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-platform-regs.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-syscom.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7-syscom.h
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7.c
> >  create mode 100644 drivers/media/pci/intel/ipu7/ipu7.h
> >  rename include/media/{ipu6-pci-table.h => ipu-pci-table.h} (68%)

-- 
Regards,

Laurent Pinchart