mbox series

[00/15] Intel IPU6 and IPU6 input system drivers

Message ID 20230727071558.1148653-1-bingbu.cao@intel.com
Headers show
Series Intel IPU6 and IPU6 input system drivers | expand

Message

Cao, Bingbu July 27, 2023, 7:15 a.m. UTC
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.
---

RFC -> v1:
  - Add multiplexed streams support
  - Use auxiliary bus to register IPU6 devices
  - Add IPU6 hardware and driver overview documentation
  - Updata IPU6 admin-guide documentation
  - Update number of source pads and video nodes to support
    multiplexed streams

Bingbu Cao (15):
  media: intel/ipu6: add Intel IPU6 PCI device driver
  media: intel/ipu6: add IPU auxiliary devices
  media: intel/ipu6: add IPU6 buttress interface driver
  media: intel/ipu6: CPD parsing for get firmware components
  media: intel/ipu6: add IPU6 DMA mapping API and MMU table
  media: intel/ipu6: add syscom interfaces between firmware and driver
  media: intel/ipu6: input system ABI between firmware and driver
  media: intel/ipu6: add IPU6 CSI2 receiver v4l2 sub-device
  media: intel/ipu6: add the CSI2 DPHY implementation
  media: intel/ipu6: add input system driver
  media: intel/ipu6: input system video capture nodes
  media: add Kconfig and Makefile for IPU6
  MAINTAINERS: add maintainers for Intel IPU6 input system driver
  Documentation: add Intel IPU6 ISYS driver admin-guide doc
  Documentation: add documentation of Intel IPU6 driver and hardware
    overview

 Documentation/admin-guide/media/ipu6-isys.rst |  138 ++
 .../admin-guide/media/ipu6_isys_graph.svg     |  338 +++++
 .../admin-guide/media/ipu6_isys_multi.svg     | 1124 ++++++++++++++
 .../admin-guide/media/v4l-drivers.rst         |    1 +
 .../driver-api/media/drivers/index.rst        |    1 +
 .../driver-api/media/drivers/ipu6.rst         |  205 +++
 MAINTAINERS                                   |   10 +
 drivers/media/pci/intel/Kconfig               |    3 +-
 drivers/media/pci/intel/Makefile              |    1 +
 drivers/media/pci/intel/ipu6/Kconfig          |   15 +
 drivers/media/pci/intel/ipu6/Makefile         |   23 +
 drivers/media/pci/intel/ipu6/ipu6-bus.c       |  164 ++
 drivers/media/pci/intel/ipu6/ipu6-bus.h       |   58 +
 drivers/media/pci/intel/ipu6/ipu6-buttress.c  |  915 +++++++++++
 drivers/media/pci/intel/ipu6/ipu6-buttress.h  |  109 ++
 drivers/media/pci/intel/ipu6/ipu6-cpd.c       |  360 +++++
 drivers/media/pci/intel/ipu6/ipu6-cpd.h       |  102 ++
 drivers/media/pci/intel/ipu6/ipu6-dma.c       |  497 ++++++
 drivers/media/pci/intel/ipu6/ipu6-dma.h       |   19 +
 drivers/media/pci/intel/ipu6/ipu6-fw-com.c    |  418 +++++
 drivers/media/pci/intel/ipu6/ipu6-fw-com.h    |   47 +
 drivers/media/pci/intel/ipu6/ipu6-fw-isys.c   |  563 +++++++
 drivers/media/pci/intel/ipu6/ipu6-fw-isys.h   |  572 +++++++
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c |  656 ++++++++
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h |   81 +
 .../media/pci/intel/ipu6/ipu6-isys-dwc-phy.c  |  551 +++++++
 .../media/pci/intel/ipu6/ipu6-isys-jsl-phy.c  |  246 +++
 .../media/pci/intel/ipu6/ipu6-isys-mcd-phy.c  |  736 +++++++++
 drivers/media/pci/intel/ipu6/ipu6-isys-phy.h  |   24 +
 .../media/pci/intel/ipu6/ipu6-isys-queue.c    |  864 +++++++++++
 .../media/pci/intel/ipu6/ipu6-isys-queue.h    |   97 ++
 .../media/pci/intel/ipu6/ipu6-isys-subdev.c   |  378 +++++
 .../media/pci/intel/ipu6/ipu6-isys-subdev.h   |   58 +
 .../media/pci/intel/ipu6/ipu6-isys-video.c    | 1237 +++++++++++++++
 .../media/pci/intel/ipu6/ipu6-isys-video.h    |  133 ++
 drivers/media/pci/intel/ipu6/ipu6-isys.c      | 1348 +++++++++++++++++
 drivers/media/pci/intel/ipu6/ipu6-isys.h      |  188 +++
 drivers/media/pci/intel/ipu6/ipu6-mmu.c       |  833 ++++++++++
 drivers/media/pci/intel/ipu6/ipu6-mmu.h       |   65 +
 .../intel/ipu6/ipu6-platform-buttress-regs.h  |  231 +++
 .../intel/ipu6/ipu6-platform-isys-csi2-reg.h  |  187 +++
 .../media/pci/intel/ipu6/ipu6-platform-regs.h |  177 +++
 drivers/media/pci/intel/ipu6/ipu6-platform.h  |   31 +
 drivers/media/pci/intel/ipu6/ipu6.c           |  982 ++++++++++++
 drivers/media/pci/intel/ipu6/ipu6.h           |  347 +++++
 45 files changed, 15132 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/admin-guide/media/ipu6-isys.rst
 create mode 100644 Documentation/admin-guide/media/ipu6_isys_graph.svg
 create mode 100644 Documentation/admin-guide/media/ipu6_isys_multi.svg
 create mode 100644 Documentation/driver-api/media/drivers/ipu6.rst
 create mode 100644 drivers/media/pci/intel/ipu6/Kconfig
 create mode 100644 drivers/media/pci/intel/ipu6/Makefile
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-bus.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-bus.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-buttress.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-buttress.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-cpd.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-cpd.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-dma.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-dma.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-com.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-com.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-isys.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-fw-isys.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-dwc-phy.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-jsl-phy.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-mcd-phy.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-phy.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-video.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-video.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-mmu.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-mmu.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-buttress-regs.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-isys-csi2-reg.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform-regs.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6-platform.h
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6.c
 create mode 100644 drivers/media/pci/intel/ipu6/ipu6.h

Comments

Andy Shevchenko July 27, 2023, 10:19 a.m. UTC | #1
On Thu, Jul 27, 2023 at 03:15:56PM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> Update MAINTAINERS file for Intel IPU6 input system driver.

> +INTEL IPU6 INPUT SYSTEM DRIVER

In both cases the word "input" is confusing.
We have INPUT subsystem, how does this relate to it?
Claus Stovgaard Aug. 20, 2023, 3:09 p.m. UTC | #2
On Thu, 2023-07-27 at 15:15 +0800, 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.
> 
> 

Hello Bingbu.

First thanks for your work in upstreaming the IPU6 isys driver, and the
updates with v1 of the patch series.

I am trying to test it on a Dell XPS 9320 (0AF3) laptop

First - The patch series does not apply cleanly on linus 6.5-rc6, nor
the linux-media master.

For v6.5-rc6 I have an issue with

Patch failed at 0012 media: add Kconfig and Makefile for IPU6
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: drivers/media/pci/intel/Kconfig: does not exist in index
error: patch failed: drivers/media/pci/intel/Makefile:4
error: drivers/media/pci/intel/Makefile: patch does not apply

For linux media it fails after commit 
https://git.linuxtv.org/media_tree.git/commit/?id=dd61c2a380037166517214957790a1486ae5d348
media: mediatek: vcodec: Consider vdecsys presence in reg range check

As next commit is
https://git.linuxtv.org/media_tree.git/commit/?id=bda8953e8c3e7ecbbf6cb1be11790496300e3961
media: v4l: async: Drop v4l2_async_nf_parse_fwnode_endpoints()

It fails on the v4l parts, and of cause the newer commits regarding
v4l: async in the linux-media master branch. So the IPU6 patch series
need a refresh to fit the linux-media.

I did a custom branch from linus tag v6.5-rc5 with the commits from
linux-media up to the "Drop v4l2_async_nf_parse_fwnode_endpoints()" and
then applied the IPU6 patches on top.
https://github.com/frosteyes/linux/tree/fe/v6.5-rc5/media_test

With this I am able to load the IPU6 modules, but I have problems with
the sensor.

The sensor module is loaded - named ov01a10 but the probe function is
not run - as far as I can see

Also in /sys/kernel/debug/v4l2-async/pending_async_subdevices I have it
as pending

ipu6:
 [fwnode] dev=nil, node=\_SB.PC00.LNK1

Looking at the /sys/bus/acpi/devices I can see the sensor device with a
status of 15 (cat OVTI01A0\:00/status)

Will continue investigating, but I would like any input in getting the
driver up an running and testing on this Dell laptop. I think it should
be very close to working.

Regards
Claus Stovgaaard
Bingbu Cao Aug. 21, 2023, 6:22 a.m. UTC | #3
Claus,


On 8/21/23 11:14 AM, Bingbu Cao wrote:
> Hi, Claus,
> 
> Thanks for your mail.
> 
> On 8/20/23 11:09 PM, Claus Stovgaard wrote:
>> On Thu, 2023-07-27 at 15:15 +0800, 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.
>>>
>>>
>>
>> Hello Bingbu.
>>
>> First thanks for your work in upstreaming the IPU6 isys driver, and the
>> updates with v1 of the patch series.
>>
>> I am trying to test it on a Dell XPS 9320 (0AF3) laptop
>>
>> First - The patch series does not apply cleanly on linus 6.5-rc6, nor
>> the linux-media master.
> 
> I think it is caused by some media changes was queued after I send
> this patch - such as ipu-bridge, ivsc, v4l2-async, etc. So it
> needs some rebase work.
> 
>>
>> For v6.5-rc6 I have an issue with
>>
>> Patch failed at 0012 media: add Kconfig and Makefile for IPU6
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
>> error: drivers/media/pci/intel/Kconfig: does not exist in index
>> error: patch failed: drivers/media/pci/intel/Makefile:4
>> error: drivers/media/pci/intel/Makefile: patch does not apply
>>
>> For linux media it fails after commit 
>> https://git.linuxtv.org/media_tree.git/commit/?id=dd61c2a380037166517214957790a1486ae5d348
>> media: mediatek: vcodec: Consider vdecsys presence in reg range check
>>
>> As next commit is
>> https://git.linuxtv.org/media_tree.git/commit/?id=bda8953e8c3e7ecbbf6cb1be11790496300e3961
>> media: v4l: async: Drop v4l2_async_nf_parse_fwnode_endpoints()
>>
>> It fails on the v4l parts, and of cause the newer commits regarding
>> v4l: async in the linux-media master branch. So the IPU6 patch series
>> need a refresh to fit the linux-media.
>>
>> I did a custom branch from linus tag v6.5-rc5 with the commits from
>> linux-media up to the "Drop v4l2_async_nf_parse_fwnode_endpoints()" and
>> then applied the IPU6 patches on top.
>> https://github.com/frosteyes/linux/tree/fe/v6.5-rc5/media_test
>>
>> With this I am able to load the IPU6 modules, but I have problems with
>> the sensor.
>>
>> The sensor module is loaded - named ov01a10 but the probe function is
>> not run - as far as I can see
>>
>> Also in /sys/kernel/debug/v4l2-async/pending_async_subdevices I have it
>> as pending
>>
>> ipu6:
>>  [fwnode] dev=nil, node=\_SB.PC00.LNK1
>>
>> Looking at the /sys/bus/acpi/devices I can see the sensor device with a
>> status of 15 (cat OVTI01A0\:00/status)
>>
>> Will continue investigating, but I would like any input in getting the
>> driver up an running and testing on this Dell laptop. I think it should
>> be very close to working.
> 
> Do you any failure log for ov01a10?
> 
> For Dell XPS 9320, the camera sensor module has a dependency on Intel
> IVSC driver, so please make sure you have the latest ivsc driver.
> I remember they are already in media tree.
> 
> I will check again with latest IVSC driver, feel free to mail me or
> Wentong Wu meanwhile if you have any problems for camera sensor and
> IVSC.

I see that the ivsc driver has not been in master branch. Before that,
could you try several hack to check whether camera can work on master?

https://github.com/bingbucao/linux/commits/ipu_dev

7ebff51284d9 media: ov01a10: hack ivsc to make camera can work
01cc9f3d1b61 i2c: ljca: Call acpi_dev_clear_dependencies()
92e5d122e105 vsc: Defer firmware loading to avoid long probing time
5f5d5f0df06b driver: ivsc: add intel ivsc driver
0f4819dec533 Revert "gpio: Add support for Intel LJCA USB GPIO driver"

> 
>>
>> Regards
>> Claus Stovgaaard
>>
>
Laurent Pinchart Aug. 21, 2023, 12:19 p.m. UTC | #4
Hi Claus,

On Mon, Aug 21, 2023 at 12:07:59PM +0200, Claus Stovgaard wrote:
> On Mon, 2023-08-21 at 08:55 +0200, Claus Stovgaard wrote:
> > On Mon, 2023-08-21 at 14:22 +0800, Bingbu Cao wrote:
> > > 
> > > Claus,
> > > 
> > > 
> > > On 8/21/23 11:14 AM, Bingbu Cao wrote:
> > > 
> > > 
> > > I see that the ivsc driver has not been in master branch. Before
> > > that,
> > > could you try several hack to check whether camera can work on
> > > master?
> > > 
> > > https://github.com/bingbucao/linux/commits/ipu_dev
> > > 
> > > 7ebff51284d9 media: ov01a10: hack ivsc to make camera can work
> > > 01cc9f3d1b61 i2c: ljca: Call acpi_dev_clear_dependencies()
> > > 92e5d122e105 vsc: Defer firmware loading to avoid long probing time
> > > 5f5d5f0df06b driver: ivsc: add intel ivsc driver
> > > 0f4819dec533 Revert "gpio: Add support for Intel LJCA USB GPIO
> > > driver"
> > 
> > Thanks for your quick reply.
> > 
> > I was missing understanding of ivsc when I wrote the mail yesterday.
> > Got some basic understanding yesterday after I wrote, and big thanks
> > for confirming it, and also thanks for your ipu_dev branch. Has just
> > cloned it, and is building as I write.
> > 
> > Just fyi, I was trying to hack something together yesterday, and got
> > further, but not yet working.
> > 
> > My hack was to combine the out-of-tree ivsc drivers and firmware from
> > 
> > * https://github.com/intel/ivsc-firmware.git
> > * https://github.com/intel/ivsc-driver.git
> > 
> > Though noticed that I need some changes to the sensor driver so was
> > also building all the drivers from ipu6-drivers (with minor changes
> > to
> > get_pages) as out-of-tree modules.
> > 
> > * https://github.com/intel/ipu6-drivers.git 
> > 
> > Here I used everything beside media/pci/*.ko files. I could see the
> > sensor and got further, but was missing the last.
> > 
> > Looking forward to try your branch. Looks much cleaner, and would be
> > nice to get working :)
> 
> I got it to work on Dell XPS 9320.

I'm glad to hear this ! Even if PSYS support will be needed to make the
IPU6 truly usable, it is a very nice step in the right direction.

Would you be interested in adding initial support for the IPU6 in
libcamera ? :-) Given that only the ISYS is currently available, and
given the simplicity of the hardware, it may be as easy as a single line
addition.

> With some minor changes compared to your guide in Documentation/admin-
> guide/media/ipu6-isys.rst
> 
> [root@xps-1 ]# uname -a
> Linux xps-1 6.5.0-rc7-g7ebff51284d9 #1 SMP PREEMPT_DYNAMIC Mon Aug 21
> 09:02:20 CEST 2023 x86_64 GNU/Linux
> 
> [root@xps-1 ]# media-ctl -d /dev/media0 -p | tail -n10
> 
> - entity 2149: ov01a10 16-0036 (1 pad, 1 link)
>                type V4L2 subdev subtype Sensor flags 0
>                device node name /dev/v4l-subdev4
>         pad0: Source
>                 [fmt:SBGGR10_1X10/1280x800 field:none colorspace:raw
>                  crop.bounds:(0,0)/1296x816
>                  crop:(8,8)/1280x800]
>                 -> "Intel IPU6 CSI2 2":0 []
> 
> So i2c is 16-0036 - and we use it for setup like your guide.
> 
> export MDEV=/dev/media0
> 
> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
> 2\":0[1]"
> 
> media-ctl -d $MDEV -V "\"ov01a10 17-0036\":0 [fmt:SBGGR10/1280x800]"
> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":0 [fmt:SBGGR10/1280x800]"
> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":1 [fmt:SBGGR10/1280x800]"
> 
> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
> 2\":0[1]"
> media-ctl -d $MDEV -l "\"Intel IPU6 CSI2 2\":1 ->\"Intel IPU6 ISYS
> Capture 0\":0[5]"
> 
> Though yavta does not work in the way as described in the guide.
> 
> [root@xps-1 ]# yavta --data-prefix -u -c10 -n5 -I -s 1280x800 --
> file=/tmp/frame-#.bin -f SBGGR10 /dev/video0
> Device /dev/video0 opened.
> Device `ipu6' on `PCI:0000:00:05.0' (driver 'isys') supports video,
> capture, with mplanes.
> Video format set: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>  * Stride 2560, buffer size 2050560
> Video format: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>  * Stride 2560, buffer size 2050560
> Unable to request buffers: Invalid argument (22).
> 
> 
> So I changed to use v4l2-ctl
> 
> [root@xps-1 ]# v4l2-ctl -d /dev/video0 --set-fmt-video
> width=1280,height=800,pixelformat=BG10 --stream-mmap --stream-count=1 -
> -stream-to=frame.bin
> 
> With this I created raw data in BG10 format, and later used a small
> python script with numpy and opencv to look at the data.
> 
> #!/usr/bin/env python3
> # Demosaicing Bayer Raw image
> 
> import cv2
> import numpy as np
> 
> width = 1280
> height = 800
> 
> with open("frame.bin", "rb") as rawimg:
>     # Read the bayer data
>     data = np.fromfile(rawimg, np.uint16, width * height)
>     bayer = np.reshape(data, (height, width))
> 
>     # Just a offset gain to be able to see something
>     for x in range(0, len(bayer)):
>         for y in range(0, len(bayer[0])):
>             bayer[x, y] = (bayer[x,y] << 8)
> 
>     rgb = cv2.cvtColor(bayer, cv2.COLOR_BayerBGGR2RGB)
> 
>     cv2.imshow('rgb', rgb)
>     cv2.waitKey()
>     cv2.destroyAllWindows()
> 
> 
> Thanks for the help, and now we know what is needed to make it work on
> top of yesterdays rc7
Claus Stovgaard Aug. 22, 2023, 12:52 p.m. UTC | #5
On Mon, 2023-08-21 at 15:19 +0300, Laurent Pinchart wrote:
> Hi Claus,
> 
> On Mon, Aug 21, 2023 at 12:07:59PM +0200, Claus Stovgaard wrote:
> > On Mon, 2023-08-21 at 08:55 +0200, Claus Stovgaard wrote:
> > > 
> > > Looking forward to try your branch. Looks much cleaner, and would
> > > be
> > > nice to get working :)
> > 
> > I got it to work on Dell XPS 9320.
> 
> I'm glad to hear this ! Even if PSYS support will be needed to make
> the
> IPU6 truly usable, it is a very nice step in the right direction.
> 
> Would you be interested in adding initial support for the IPU6 in
> libcamera ? :-) Given that only the ISYS is currently available, and
> given the simplicity of the hardware, it may be as easy as a single
> line
> addition.
> 
> 

Hi Laurent.

Thanks for your offer - it might come in handy to have libcamera
support, but I don't need it right now.

My use case is a bit special. I am working as Embedded Engineer for
Ambu A/S, where we have 2 display units, named aView2 and aBox2, for
single use endoscopy.

https://youtu.be/eDcSrHxzZ70?t=14

Those units is based on the intel Apollo Lake with IPU4, where only the
isys part of IPU4 is used, as a FPGA in front of the Apollo Lake is
used for image processing. So the image stream is sent to the Apollo
Lake as RGB data - and is using the IPU4 isys as DMA. E.g. like below.

scope -> FPGA -> tc358748 -> IPU4-> memory

We need to support this for newer kernels, then provided from intel
(4.14 / 4.19) and looking at the code, it seems like a better option to
base it on this IPU6 isys driver and extend it to cover IPU4 isys also.

So we are being inspired by the provided 4.14 / 4.19 kernel, and then
work on the IPU6 codebase.

Our current status is that my coworker has the Buttress to load the
firmware on IPU4, and we will continue work from there.

My end goal would be that an upstream vanilla kernel is able to support
the isys part of IPU4, and the complete IPU6.

Regards
Claus
Laurent Pinchart Aug. 22, 2023, 2:22 p.m. UTC | #6
Hi Claus,

On Tue, Aug 22, 2023 at 02:52:35PM +0200, claus.stovgaard@gmail.com wrote:
> On Mon, 2023-08-21 at 15:19 +0300, Laurent Pinchart wrote:
> > On Mon, Aug 21, 2023 at 12:07:59PM +0200, Claus Stovgaard wrote:
> > > On Mon, 2023-08-21 at 08:55 +0200, Claus Stovgaard wrote:
> > > > 
> > > > Looking forward to try your branch. Looks much cleaner, and would be
> > > > nice to get working :)
> > > 
> > > I got it to work on Dell XPS 9320.
> > 
> > I'm glad to hear this ! Even if PSYS support will be needed to make the
> > IPU6 truly usable, it is a very nice step in the right direction.
> > 
> > Would you be interested in adding initial support for the IPU6 in
> > libcamera ? :-) Given that only the ISYS is currently available, and
> > given the simplicity of the hardware, it may be as easy as a single line
> > addition.
> 
> Hi Laurent.
> 
> Thanks for your offer - it might come in handy to have libcamera
> support, but I don't need it right now.
> 
> My use case is a bit special. I am working as Embedded Engineer for
> Ambu A/S, where we have 2 display units, named aView2 and aBox2, for
> single use endoscopy.
> 
> https://youtu.be/eDcSrHxzZ70?t=14
> 
> Those units is based on the intel Apollo Lake with IPU4, where only the
> isys part of IPU4 is used, as a FPGA in front of the Apollo Lake is
> used for image processing. So the image stream is sent to the Apollo
> Lake as RGB data - and is using the IPU4 isys as DMA. E.g. like below.
> 
> scope -> FPGA -> tc358748 -> IPU4-> memory

Out of curiosity, is this because the image processing requirements are
very device-specific, or was that done to work around the fact that the
IPU4 doesn't provide a good ISP driver ?

> We need to support this for newer kernels, then provided from intel
> (4.14 / 4.19)

*OUCH*. Seriously ?? :-(

> and looking at the code, it seems like a better option to
> base it on this IPU6 isys driver and extend it to cover IPU4 isys also.
> 
> So we are being inspired by the provided 4.14 / 4.19 kernel, and then
> work on the IPU6 codebase.
> 
> Our current status is that my coworker has the Buttress to load the
> firmware on IPU4, and we will continue work from there.
> 
> My end goal would be that an upstream vanilla kernel is able to support
> the isys part of IPU4, and the complete IPU6.

It would be very nice to have an upstream driver for the IPU4 CSI-2
receivers indeed. Looking forward to seeing one :-)
Claus Stovgaard Aug. 24, 2023, 8:19 p.m. UTC | #7
On Tue, 2023-08-22 at 11:05 +0800, Bingbu Cao wrote:
> 
> Claus,
> 
> On 8/21/23 6:07 PM, Claus Stovgaard wrote:
> > Bingbu
> > 
> > Though yavta does not work in the way as described in the guide.
> > 
> > [root@xps-1 ]# yavta --data-prefix -u -c10 -n5 -I -s 1280x800 --
> > file=/tmp/frame-#.bin -f SBGGR10 /dev/video0
> > Device /dev/video0 opened.
> > Device `ipu6' on `PCI:0000:00:05.0' (driver 'isys') supports video,
> > capture, with mplanes.
> > Video format set: SBGGR10 (30314742) 1280x800 field none, 1 planes:
> >  * Stride 2560, buffer size 2050560
> > Video format: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
> >  * Stride 2560, buffer size 2050560
> > Unable to request buffers: Invalid argument (22).
> 
> Firstly, thanks for your work. I just noticed that we remove the
> userptr buffer support before, that means yavta '-u' will not be
> supported. So I think you can try to remove '-u' to see whether it
> can work. I will update the documentation in next version.

Good catch. Removing the -u makes it work.

clst@emb-xps-1:~$ yavta --data-prefix -c10 -n5 -I -s 1280x800 --
file=/tmp/frame-#.bin -f SBGGR10 /dev/video0
Device /dev/video0 opened.
Device `ipu6' on `PCI:0000:00:05.0' (driver 'isys') supports video,
capture, with mplanes.
Video format set: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
 * Stride 2560, buffer size 2050560
Video format: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
 * Stride 2560, buffer size 2050560
5 buffers requested.
length: 1 offset: 1815302016 timestamp type/source: mono/EoF
Buffer 0/0 mapped at address 0x7fa9b040b000.
...
Warning: bytes used 2048000 != image size 2050560 for plane 0
0 (0) [-] none 0 2048000 B 183.555047 183.571463 59.641 fps ts mono/EoF
Warning: bytes used 2048000 != image size 2050560 for plane 0
1 (1) [-] none 1 2048000 B 183.571675 183.588100 60.140 fps ts mono/EoF
...
Captured 10 frames in 0.182985 seconds (54.649099 fps, 0.000000 B/s).
5 buffers released.


> For Dell XPS 9320, we still have some work to make IPU work with
> Intel VSC(upstreaming). My current hack on github is not offical.
> But it can help people on 9320 to verify the camera before
> everything ready. :)
> 


Yes. Big thanks for the work. I guess that after next merge window, the
IPU6 patches can be rebased on top of Intel VSC sent upstream, and also
changed to fit the changes in the V4L2 api.

Regards
Claus
Claus Stovgaard Aug. 24, 2023, 8:35 p.m. UTC | #8
On Tue, 2023-08-22 at 17:22 +0300, Laurent Pinchart wrote:
> Hi Claus,
> 
> On Tue, Aug 22, 2023 at 02:52:35PM +0200,
> claus.stovgaard@gmail.com wrote:
> > 
> > Hi Laurent.
> > 
> > Thanks for your offer - it might come in handy to have libcamera
> > support, but I don't need it right now.
> > 
> > My use case is a bit special. I am working as Embedded Engineer for
> > Ambu A/S, where we have 2 display units, named aView2 and aBox2,
> > for
> > single use endoscopy.
> > 
> > https://youtu.be/eDcSrHxzZ70?t=14
> > 
> > Those units is based on the intel Apollo Lake with IPU4, where only
> > the
> > isys part of IPU4 is used, as a FPGA in front of the Apollo Lake is
> > used for image processing. So the image stream is sent to the
> > Apollo
> > Lake as RGB data - and is using the IPU4 isys as DMA. E.g. like
> > below.
> > 
> > scope -> FPGA -> tc358748 -> IPU4-> memory
> 
> Out of curiosity, is this because the image processing requirements
> are
> very device-specific, or was that done to work around the fact that
> the
> IPU4 doesn't provide a good ISP driver ?

The hardware was created with this architecture before I started at
Ambu, so I don't know the details around the original design process.
Though I would say device-specific , because we are used for medical
procedures you want to have a mitigation for any failure.
The display is also connected to the FPGA, so when it is powered on,
before the Apollo Lake starts, a basic video pipeline is already
running. E.g. instant on, and showing video from the scopes.
If a code error happens on the Apollo Lake and the watchdog kicks it,
it always fall back to this FPGA view.
This FPGA view is of cause missing features and information present
when the complete system is running. Features like patient information,
video recording, voice recording, printing, data export etc. but the
FPGA view makes sure the doctor always is able to see what happens when
the scope is inserted in the body, even if any bug is hit in the
software.
So think device-specific risk mitigation.


> 
> > We need to support this for newer kernels, then provided from intel
> > (4.14 / 4.19)
> 
> *OUCH*. Seriously ?? :-(

Ohh yes.. So I think we have plenty of work for quite some time...
But a good option for learning this part of the kernel.

> 
> > and looking at the code, it seems like a better option to
> > base it on this IPU6 isys driver and extend it to cover IPU4 isys
> > also.
> > 
> > So we are being inspired by the provided 4.14 / 4.19 kernel, and
> > then
> > work on the IPU6 codebase.
> > 
> > Our current status is that my coworker has the Buttress to load the
> > firmware on IPU4, and we will continue work from there.
> > 
> > My end goal would be that an upstream vanilla kernel is able to
> > support
> > the isys part of IPU4, and the complete IPU6.
> 
> It would be very nice to have an upstream driver for the IPU4 CSI-2
> receivers indeed. Looking forward to seeing one :-)
> 

We will do our very best.

Regards
Claus
Hans de Goede Aug. 31, 2023, 9:24 p.m. UTC | #9
Hi Bingbu, Claus,

On 8/21/23 12:07, Claus Stovgaard wrote:
> Bingbu
> 
> On Mon, 2023-08-21 at 08:55 +0200, Claus Stovgaard wrote:
>> Bingbu
>>
>> On Mon, 2023-08-21 at 14:22 +0800, Bingbu Cao wrote:
>>>
>>> Claus,
>>>
>>>
>>> On 8/21/23 11:14 AM, Bingbu Cao wrote:
>>>
>>>
>>> I see that the ivsc driver has not been in master branch. Before
>>> that,
>>> could you try several hack to check whether camera can work on
>>> master?
>>>
>>> https://github.com/bingbucao/linux/commits/ipu_dev
>>>
>>> 7ebff51284d9 media: ov01a10: hack ivsc to make camera can work
>>> 01cc9f3d1b61 i2c: ljca: Call acpi_dev_clear_dependencies()
>>> 92e5d122e105 vsc: Defer firmware loading to avoid long probing time
>>> 5f5d5f0df06b driver: ivsc: add intel ivsc driver
>>> 0f4819dec533 Revert "gpio: Add support for Intel LJCA USB GPIO
>>> driver"
>>
>> Thanks for your quick reply.
>>
>> I was missing understanding of ivsc when I wrote the mail yesterday.
>> Got some basic understanding yesterday after I wrote, and big thanks
>> for confirming it, and also thanks for your ipu_dev branch. Has just
>> cloned it, and is building as I write.
>>
>> Just fyi, I was trying to hack something together yesterday, and got
>> further, but not yet working.
>>
>> My hack was to combine the out-of-tree ivsc drivers and firmware from
>>
>> * https://github.com/intel/ivsc-firmware.git
>> * https://github.com/intel/ivsc-driver.git
>>
>> Though noticed that I need some changes to the sensor driver so was
>> also building all the drivers from ipu6-drivers (with minor changes
>> to
>> get_pages) as out-of-tree modules.
>>
>> * https://github.com/intel/ipu6-drivers.git 
>>
>> Here I used everything beside media/pci/*.ko files. I could see the
>> sensor and got further, but was missing the last.
>>
>> Looking forward to try your branch. Looks much cleaner, and would be
>> nice to get working :)
>>
> 
> I got it to work on Dell XPS 9320.
> With some minor changes compared to your guide in Documentation/admin-
> guide/media/ipu6-isys.rst
> 
> [root@xps-1 ]# uname -a
> Linux xps-1 6.5.0-rc7-g7ebff51284d9 #1 SMP PREEMPT_DYNAMIC Mon Aug 21
> 09:02:20 CEST 2023 x86_64 GNU/Linux
> 
> [root@xps-1 ]# media-ctl -d /dev/media0 -p | tail -n10
> 
> - entity 2149: ov01a10 16-0036 (1 pad, 1 link)
>                type V4L2 subdev subtype Sensor flags 0
>                device node name /dev/v4l-subdev4
>         pad0: Source
>                 [fmt:SBGGR10_1X10/1280x800 field:none colorspace:raw
>                  crop.bounds:(0,0)/1296x816
>                  crop:(8,8)/1280x800]
>                 -> "Intel IPU6 CSI2 2":0 []
> 
> So i2c is 16-0036 - and we use it for setup like your guide.
> 
> export MDEV=/dev/media0
> 
> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
> 2\":0[1]"
> 
> media-ctl -d $MDEV -V "\"ov01a10 17-0036\":0 [fmt:SBGGR10/1280x800]"
> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":0 [fmt:SBGGR10/1280x800]"
> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":1 [fmt:SBGGR10/1280x800]"
> 
> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
> 2\":0[1]"
> media-ctl -d $MDEV -l "\"Intel IPU6 CSI2 2\":1 ->\"Intel IPU6 ISYS
> Capture 0\":0[5]"
> 
> Though yavta does not work in the way as described in the guide.
> 
> [root@xps-1 ]# yavta --data-prefix -u -c10 -n5 -I -s 1280x800 --
> file=/tmp/frame-#.bin -f SBGGR10 /dev/video0
> Device /dev/video0 opened.
> Device `ipu6' on `PCI:0000:00:05.0' (driver 'isys') supports video,
> capture, with mplanes.
> Video format set: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>  * Stride 2560, buffer size 2050560
> Video format: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>  * Stride 2560, buffer size 2050560
> Unable to request buffers: Invalid argument (22).
> 
> 
> So I changed to use v4l2-ctl
> 
> [root@xps-1 ]# v4l2-ctl -d /dev/video0 --set-fmt-video
> width=1280,height=800,pixelformat=BG10 --stream-mmap --stream-count=1 -
> -stream-to=frame.bin
> 
> With this I created raw data in BG10 format, and later used a small
> python script with numpy and opencv to look at the data.
> 
> #!/usr/bin/env python3
> # Demosaicing Bayer Raw image
> 
> import cv2
> import numpy as np
> 
> width = 1280
> height = 800
> 
> with open("frame.bin", "rb") as rawimg:
>     # Read the bayer data
>     data = np.fromfile(rawimg, np.uint16, width * height)
>     bayer = np.reshape(data, (height, width))
> 
>     # Just a offset gain to be able to see something
>     for x in range(0, len(bayer)):
>         for y in range(0, len(bayer[0])):
>             bayer[x, y] = (bayer[x,y] << 8)
> 
>     rgb = cv2.cvtColor(bayer, cv2.COLOR_BayerBGGR2RGB)
> 
>     cv2.imshow('rgb', rgb)
>     cv2.waitKey()
>     cv2.destroyAllWindows()
> 
> 
> Thanks for the help, and now we know what is needed to make it work on
> top of yesterdays rc7


Bingbu, thank you for the series. Claus, thank you for the python
test-script.

I've just given this a test-run on top of a recent checkout
of media-staging/master, so on top of the drivers/media
changes headed for 6.6 .

And with the attached changes + the ov2740 changes from
the github ipu6-drevers repo I got this working on
a lenovo thinkpad x1 yoga with an ov2740 driver.

I've attached the necessary changes to adjust the new ipu6
code for the v4l2-async changes which are queued up for
kernel 6.6 .

Regards,

Hans
Hans de Goede Sept. 2, 2023, 2:54 p.m. UTC | #10
Hi All,

On 8/31/23 23:24, Hans de Goede wrote:
> Hi Bingbu, Claus,
> 
> On 8/21/23 12:07, Claus Stovgaard wrote:
>> Bingbu
>>
>> On Mon, 2023-08-21 at 08:55 +0200, Claus Stovgaard wrote:
>>> Bingbu
>>>
>>> On Mon, 2023-08-21 at 14:22 +0800, Bingbu Cao wrote:
>>>>
>>>> Claus,
>>>>
>>>>
>>>> On 8/21/23 11:14 AM, Bingbu Cao wrote:
>>>>
>>>>
>>>> I see that the ivsc driver has not been in master branch. Before
>>>> that,
>>>> could you try several hack to check whether camera can work on
>>>> master?
>>>>
>>>> https://github.com/bingbucao/linux/commits/ipu_dev
>>>>
>>>> 7ebff51284d9 media: ov01a10: hack ivsc to make camera can work
>>>> 01cc9f3d1b61 i2c: ljca: Call acpi_dev_clear_dependencies()
>>>> 92e5d122e105 vsc: Defer firmware loading to avoid long probing time
>>>> 5f5d5f0df06b driver: ivsc: add intel ivsc driver
>>>> 0f4819dec533 Revert "gpio: Add support for Intel LJCA USB GPIO
>>>> driver"
>>>
>>> Thanks for your quick reply.
>>>
>>> I was missing understanding of ivsc when I wrote the mail yesterday.
>>> Got some basic understanding yesterday after I wrote, and big thanks
>>> for confirming it, and also thanks for your ipu_dev branch. Has just
>>> cloned it, and is building as I write.
>>>
>>> Just fyi, I was trying to hack something together yesterday, and got
>>> further, but not yet working.
>>>
>>> My hack was to combine the out-of-tree ivsc drivers and firmware from
>>>
>>> * https://github.com/intel/ivsc-firmware.git
>>> * https://github.com/intel/ivsc-driver.git
>>>
>>> Though noticed that I need some changes to the sensor driver so was
>>> also building all the drivers from ipu6-drivers (with minor changes
>>> to
>>> get_pages) as out-of-tree modules.
>>>
>>> * https://github.com/intel/ipu6-drivers.git 
>>>
>>> Here I used everything beside media/pci/*.ko files. I could see the
>>> sensor and got further, but was missing the last.
>>>
>>> Looking forward to try your branch. Looks much cleaner, and would be
>>> nice to get working :)
>>>
>>
>> I got it to work on Dell XPS 9320.
>> With some minor changes compared to your guide in Documentation/admin-
>> guide/media/ipu6-isys.rst
>>
>> [root@xps-1 ]# uname -a
>> Linux xps-1 6.5.0-rc7-g7ebff51284d9 #1 SMP PREEMPT_DYNAMIC Mon Aug 21
>> 09:02:20 CEST 2023 x86_64 GNU/Linux
>>
>> [root@xps-1 ]# media-ctl -d /dev/media0 -p | tail -n10
>>
>> - entity 2149: ov01a10 16-0036 (1 pad, 1 link)
>>                type V4L2 subdev subtype Sensor flags 0
>>                device node name /dev/v4l-subdev4
>>         pad0: Source
>>                 [fmt:SBGGR10_1X10/1280x800 field:none colorspace:raw
>>                  crop.bounds:(0,0)/1296x816
>>                  crop:(8,8)/1280x800]
>>                 -> "Intel IPU6 CSI2 2":0 []
>>
>> So i2c is 16-0036 - and we use it for setup like your guide.
>>
>> export MDEV=/dev/media0
>>
>> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
>> 2\":0[1]"
>>
>> media-ctl -d $MDEV -V "\"ov01a10 17-0036\":0 [fmt:SBGGR10/1280x800]"
>> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":0 [fmt:SBGGR10/1280x800]"
>> media-ctl -d $MDEV -V "\"Intel IPU6 CSI2 2\":1 [fmt:SBGGR10/1280x800]"
>>
>> media-ctl -d $MDEV -l "\"ov01a10 17-0036\":0 -> \"Intel IPU6 CSI2
>> 2\":0[1]"
>> media-ctl -d $MDEV -l "\"Intel IPU6 CSI2 2\":1 ->\"Intel IPU6 ISYS
>> Capture 0\":0[5]"
>>
>> Though yavta does not work in the way as described in the guide.
>>
>> [root@xps-1 ]# yavta --data-prefix -u -c10 -n5 -I -s 1280x800 --
>> file=/tmp/frame-#.bin -f SBGGR10 /dev/video0
>> Device /dev/video0 opened.
>> Device `ipu6' on `PCI:0000:00:05.0' (driver 'isys') supports video,
>> capture, with mplanes.
>> Video format set: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>>  * Stride 2560, buffer size 2050560
>> Video format: SBGGR10 (30314742) 1280x800 field none, 1 planes: 
>>  * Stride 2560, buffer size 2050560
>> Unable to request buffers: Invalid argument (22).
>>
>>
>> So I changed to use v4l2-ctl
>>
>> [root@xps-1 ]# v4l2-ctl -d /dev/video0 --set-fmt-video
>> width=1280,height=800,pixelformat=BG10 --stream-mmap --stream-count=1 -
>> -stream-to=frame.bin
>>
>> With this I created raw data in BG10 format, and later used a small
>> python script with numpy and opencv to look at the data.
>>
>> #!/usr/bin/env python3
>> # Demosaicing Bayer Raw image
>>
>> import cv2
>> import numpy as np
>>
>> width = 1280
>> height = 800
>>
>> with open("frame.bin", "rb") as rawimg:
>>     # Read the bayer data
>>     data = np.fromfile(rawimg, np.uint16, width * height)
>>     bayer = np.reshape(data, (height, width))
>>
>>     # Just a offset gain to be able to see something
>>     for x in range(0, len(bayer)):
>>         for y in range(0, len(bayer[0])):
>>             bayer[x, y] = (bayer[x,y] << 8)
>>
>>     rgb = cv2.cvtColor(bayer, cv2.COLOR_BayerBGGR2RGB)
>>
>>     cv2.imshow('rgb', rgb)
>>     cv2.waitKey()
>>     cv2.destroyAllWindows()
>>
>>
>> Thanks for the help, and now we know what is needed to make it work on
>> top of yesterdays rc7
> 
> 
> Bingbu, thank you for the series. Claus, thank you for the python
> test-script.
> 
> I've just given this a test-run on top of a recent checkout
> of media-staging/master, so on top of the drivers/media
> changes headed for 6.6 .
> 
> And with the attached changes + the ov2740 changes from
> the github ipu6-drevers repo I got this working on
> a lenovo thinkpad x1 yoga with an ov2740 driver.
> 
> I've attached the necessary changes to adjust the new ipu6
> code for the v4l2-async changes which are queued up for
> kernel 6.6 .

Attached is one more patch which fixes an oops when
using lockdep.

With both patches applied this is:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans
Hans de Goede Sept. 3, 2023, 2:32 p.m. UTC | #11
Hi All,

On 9/2/23 16:54, Hans de Goede wrote:
> Attached is one more patch which fixes an oops when
> using lockdep.
> 
> With both patches applied this is:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

I withdraw my Tested-by. After a fresh install the driver crashed,
messing up the entire machine, due to the firmware not being
installed.

On missing firmware the driver should simply exit cleanly, not
take down the entire machine.

Here is a backtrace of the crash:

[   12.549799] intel-ipu6 0000:00:05.0: cpd file name: intel/ipu6ep_fw.bin
[   12.551859] intel-ipu6 0000:00:05.0: Direct firmware load for intel/ipu6ep_fw.bin failed with error -2
[   12.551876] intel-ipu6 0000:00:05.0: Requesting signed firmware failed
[   12.551880] intel-ipu6: probe of 0000:00:05.0 failed with error -2
[   12.552112] BUG: kernel NULL pointer dereference, address: 0000000000000490
[   12.552116] #PF: supervisor read access in kernel mode
[   12.552118] #PF: error_code(0x0000) - not-present page
[   12.552119] PGD 0 P4D 0 
[   12.552121] Oops: 0000 [#1] PREEMPT SMP NOPTI
[   12.552124] CPU: 2 PID: 692 Comm: (udev-worker) Not tainted 6.5.0+ #1
[   12.552126] Hardware name: LENOVO 21CEZ9Q3US/21CEZ9Q3US, BIOS N3AET72W (1.37 ) 03/02/2023
[   12.552127] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
[   12.552137] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
[   12.552138] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
[   12.552140] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
[   12.552142] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
[   12.552143] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
[   12.552144] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
[   12.552145] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
[   12.552146] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
[   12.552148] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.552150] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
[   12.552152] PKRU: 55555554
[   12.552153] Call Trace:
[   12.552155]  <TASK>
[   12.552157]  ? __die+0x1f/0x70
[   12.552162]  ? page_fault_oops+0x13d/0x480
[   12.552167]  ? do_user_addr_fault+0x65/0x830
[   12.552170]  ? exc_page_fault+0x36/0x200
[   12.552174]  ? exc_page_fault+0x7b/0x200
[   12.552176]  ? asm_exc_page_fault+0x22/0x30
[   12.552180]  ? ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
[   12.552186]  ? _raw_spin_unlock_irqrestore+0x35/0x60
[   12.552190]  free_irq+0x256/0x3a0
[   12.552194]  devres_release_all+0xa5/0xe0
[   12.552200]  device_unbind_cleanup+0xe/0x70
[   12.552203]  really_probe+0x145/0x3e0
[   12.552206]  ? __pfx___driver_attach+0x10/0x10
[   12.552209]  __driver_probe_device+0x78/0x160
[   12.552212]  driver_probe_device+0x1f/0x90
[   12.552215]  __driver_attach+0xd2/0x1c0
[   12.552218]  bus_for_each_dev+0x63/0xa0
[   12.552221]  bus_add_driver+0x115/0x210
[   12.552223]  driver_register+0x55/0x100
[   12.552226]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
[   12.552234]  ipu6_init+0x20/0xff0 [intel_ipu6]
[   12.552241]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
[   12.552247]  do_one_initcall+0x5a/0x360
[   12.552251]  ? rcu_is_watching+0xd/0x40
[   12.552254]  ? kmalloc_trace+0xa5/0xb0
[   12.552258]  do_init_module+0x60/0x230
[   12.552261]  init_module_from_file+0x75/0xa0
[   12.552265]  idempotent_init_module+0xf9/0x270
[   12.552268]  ? subflow_v6_conn_request+0xc0/0x120
[   12.552273]  __x64_sys_finit_module+0x5a/0xb0
[   12.552276]  do_syscall_64+0x59/0x90
[   12.552279]  ? do_syscall_64+0x68/0x90
[   12.552281]  ? lockdep_hardirqs_on+0x7d/0x100
[   12.552283]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[   12.552286] RIP: 0033:0x7fd5bff2cb5d
[   12.552288] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 7b 92 0c 00 f7 d8 64 89 01 48
[   12.552289] RSP: 002b:00007ffc50c03b38 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   12.552291] RAX: ffffffffffffffda RBX: 000055e540797f00 RCX: 00007fd5bff2cb5d
[   12.552292] RDX: 0000000000000000 RSI: 00007fd5c052607d RDI: 000000000000000c
[   12.552293] RBP: 00007ffc50c03bf0 R08: 0000000000000000 R09: 00007ffc50c03b80
[   12.552294] R10: 000000000000000c R11: 0000000000000246 R12: 00007fd5c052607d
[   12.552296] R13: 0000000000020000 R14: 000055e540797030 R15: 000055e540796290
[   12.552301]  </TASK>
[   12.552302] Modules linked in: v4l2_async(+) processor_thermal_rfim industrialio_triggered_buffer ecdh_generic(+) processor_thermal_mbox kfifo_buf videodev snd processor_thermal_rapl intel_skl_int3472_tps68470 intel_ipu6(+) industrialio thunderbolt(+) soundcore tps68470_regulator rfkill mc intel_rapl_common ipu_bridge intel_hid int3403_thermal(+) idma64(+) soc_button_array intel_vsec igen6_edac int340x_thermal_zone clk_tps68470 joydev intel_skl_int3472_discrete sparse_keymap int3400_thermal acpi_thermal_rel acpi_pad acpi_tad loop zram hid_sensor_hub intel_ishtp_hid i915 i2c_algo_bit crct10dif_pclmul drm_buddy crc32_pclmul ttm crc32c_intel drm_display_helper intel_ish_ipc video nvme ghash_clmulni_intel ucsi_acpi wacom hid_multitouch sha512_ssse3 typec_ucsi nvme_core intel_ishtp cec typec i2c_hid_acpi i2c_hid wmi pinctrl_tigerlake serio_raw ip6_tables ip_tables fuse
[   12.552348] CR2: 0000000000000490
[   12.552351] ---[ end trace 0000000000000000 ]---
[   12.552352] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
[   12.552361] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
[   12.552363] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
[   12.552365] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
[   12.552366] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
[   12.552367] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
[   12.552368] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
[   12.552369] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
[   12.552370] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
[   12.552371] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.552372] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
[   12.552373] PKRU: 55555554

Please fix this for the next version. Reproducing this is easy, just remove the firmware file from under /lib/firmware/intel .

Regards,

Hans
Hans de Goede Sept. 4, 2023, 7:35 a.m. UTC | #12
Hi,

On 9/4/23 05:13, Cao, Bingbu wrote:
> Hans,
> 
> Thanks for your test and report.
> 
> I have made some changes locally which will support the latest
> v4l2-async APIs, I will also add the fix for this issue and merge
> them in v3.

Sounds good, thank you.

Regards,

Hans



> 
> 
> ------------------------------------------------------------------------
> BRs,  
> Bingbu Cao 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Sunday, September 3, 2023 10:33 PM
>> To: Claus Stovgaard <claus.stovgaard@gmail.com>; Bingbu Cao
>> <bingbu.cao@linux.intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; linux-
>> media@vger.kernel.org; sakari.ailus@linux.intel.com;
>> laurent.pinchart@ideasonboard.com
>> Cc: ilpo.jarvinen@linux.intel.com; tfiga@chromium.org;
>> senozhatsky@chromium.org; andriy.shevchenko@linux.intel.com;
>> tomi.valkeinen@ideasonboard.com; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
>> Wang, Hongju <hongju.wang@intel.com>
>> Subject: Re: [PATCH 00/15] Intel IPU6 and IPU6 input system drivers
>>
>> Hi All,
>>
>> On 9/2/23 16:54, Hans de Goede wrote:
>>> Attached is one more patch which fixes an oops when using lockdep.
>>>
>>> With both patches applied this is:
>>>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> I withdraw my Tested-by. After a fresh install the driver crashed,
>> messing up the entire machine, due to the firmware not being installed.
>>
>> On missing firmware the driver should simply exit cleanly, not take down
>> the entire machine.
>>
>> Here is a backtrace of the crash:
>>
>> [   12.549799] intel-ipu6 0000:00:05.0: cpd file name:
>> intel/ipu6ep_fw.bin
>> [   12.551859] intel-ipu6 0000:00:05.0: Direct firmware load for
>> intel/ipu6ep_fw.bin failed with error -2
>> [   12.551876] intel-ipu6 0000:00:05.0: Requesting signed firmware failed
>> [   12.551880] intel-ipu6: probe of 0000:00:05.0 failed with error -2
>> [   12.552112] BUG: kernel NULL pointer dereference, address:
>> 0000000000000490
>> [   12.552116] #PF: supervisor read access in kernel mode
>> [   12.552118] #PF: error_code(0x0000) - not-present page
>> [   12.552119] PGD 0 P4D 0
>> [   12.552121] Oops: 0000 [#1] PREEMPT SMP NOPTI
>> [   12.552124] CPU: 2 PID: 692 Comm: (udev-worker) Not tainted 6.5.0+ #1
>> [   12.552126] Hardware name: LENOVO 21CEZ9Q3US/21CEZ9Q3US, BIOS N3AET72W
>> (1.37 ) 03/02/2023
>> [   12.552127] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552137] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00
>> 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc
>> 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>> [   12.552138] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>> [   12.552140] RAX: ffffb59289000000 RBX: 0000000000000040 RCX:
>> 0000000000000001
>> [   12.552142] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI:
>> 0000000000000000
>> [   12.552143] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09:
>> 0000000000000001
>> [   12.552144] R10: 0000000000000001 R11: 0000000000000001 R12:
>> 0000000000000064
>> [   12.552145] R13: 0000000000000000 R14: ffff90a2c5b79400 R15:
>> 0000000000000000
>> [   12.552146] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000)
>> knlGS:0000000000000000
>> [   12.552148] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   12.552150] CR2: 0000000000000490 CR3: 000000010c28c000 CR4:
>> 0000000000750ee0
>> [   12.552152] PKRU: 55555554
>> [   12.552153] Call Trace:
>> [   12.552155]  <TASK>
>> [   12.552157]  ? __die+0x1f/0x70
>> [   12.552162]  ? page_fault_oops+0x13d/0x480
>> [   12.552167]  ? do_user_addr_fault+0x65/0x830
>> [   12.552170]  ? exc_page_fault+0x36/0x200
>> [   12.552174]  ? exc_page_fault+0x7b/0x200
>> [   12.552176]  ? asm_exc_page_fault+0x22/0x30
>> [   12.552180]  ? ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552186]  ? _raw_spin_unlock_irqrestore+0x35/0x60
>> [   12.552190]  free_irq+0x256/0x3a0
>> [   12.552194]  devres_release_all+0xa5/0xe0
>> [   12.552200]  device_unbind_cleanup+0xe/0x70
>> [   12.552203]  really_probe+0x145/0x3e0
>> [   12.552206]  ? __pfx___driver_attach+0x10/0x10
>> [   12.552209]  __driver_probe_device+0x78/0x160
>> [   12.552212]  driver_probe_device+0x1f/0x90
>> [   12.552215]  __driver_attach+0xd2/0x1c0
>> [   12.552218]  bus_for_each_dev+0x63/0xa0
>> [   12.552221]  bus_add_driver+0x115/0x210
>> [   12.552223]  driver_register+0x55/0x100
>> [   12.552226]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>> [   12.552234]  ipu6_init+0x20/0xff0 [intel_ipu6]
>> [   12.552241]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>> [   12.552247]  do_one_initcall+0x5a/0x360
>> [   12.552251]  ? rcu_is_watching+0xd/0x40
>> [   12.552254]  ? kmalloc_trace+0xa5/0xb0
>> [   12.552258]  do_init_module+0x60/0x230
>> [   12.552261]  init_module_from_file+0x75/0xa0
>> [   12.552265]  idempotent_init_module+0xf9/0x270
>> [   12.552268]  ? subflow_v6_conn_request+0xc0/0x120
>> [   12.552273]  __x64_sys_finit_module+0x5a/0xb0
>> [   12.552276]  do_syscall_64+0x59/0x90
>> [   12.552279]  ? do_syscall_64+0x68/0x90
>> [   12.552281]  ? lockdep_hardirqs_on+0x7d/0x100
>> [   12.552283]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>> [   12.552286] RIP: 0033:0x7fd5bff2cb5d
>> [   12.552288] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa
>> 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f
>> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 7b 92 0c 00 f7 d8 64 89 01 48
>> [   12.552289] RSP: 002b:00007ffc50c03b38 EFLAGS: 00000246 ORIG_RAX:
>> 0000000000000139
>> [   12.552291] RAX: ffffffffffffffda RBX: 000055e540797f00 RCX:
>> 00007fd5bff2cb5d
>> [   12.552292] RDX: 0000000000000000 RSI: 00007fd5c052607d RDI:
>> 000000000000000c
>> [   12.552293] RBP: 00007ffc50c03bf0 R08: 0000000000000000 R09:
>> 00007ffc50c03b80
>> [   12.552294] R10: 000000000000000c R11: 0000000000000246 R12:
>> 00007fd5c052607d
>> [   12.552296] R13: 0000000000020000 R14: 000055e540797030 R15:
>> 000055e540796290
>> [   12.552301]  </TASK>
>> [   12.552302] Modules linked in: v4l2_async(+) processor_thermal_rfim
>> industrialio_triggered_buffer ecdh_generic(+) processor_thermal_mbox
>> kfifo_buf videodev snd processor_thermal_rapl intel_skl_int3472_tps68470
>> intel_ipu6(+) industrialio thunderbolt(+) soundcore tps68470_regulator
>> rfkill mc intel_rapl_common ipu_bridge intel_hid int3403_thermal(+)
>> idma64(+) soc_button_array intel_vsec igen6_edac int340x_thermal_zone
>> clk_tps68470 joydev intel_skl_int3472_discrete sparse_keymap
>> int3400_thermal acpi_thermal_rel acpi_pad acpi_tad loop zram
>> hid_sensor_hub intel_ishtp_hid i915 i2c_algo_bit crct10dif_pclmul
>> drm_buddy crc32_pclmul ttm crc32c_intel drm_display_helper intel_ish_ipc
>> video nvme ghash_clmulni_intel ucsi_acpi wacom hid_multitouch
>> sha512_ssse3 typec_ucsi nvme_core intel_ishtp cec typec i2c_hid_acpi
>> i2c_hid wmi pinctrl_tigerlake serio_raw ip6_tables ip_tables fuse
>> [   12.552348] CR2: 0000000000000490
>> [   12.552351] ---[ end trace 0000000000000000 ]---
>> [   12.552352] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552361] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00
>> 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc
>> 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>> [   12.552363] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>> [   12.552365] RAX: ffffb59289000000 RBX: 0000000000000040 RCX:
>> 0000000000000001
>> [   12.552366] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI:
>> 0000000000000000
>> [   12.552367] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09:
>> 0000000000000001
>> [   12.552368] R10: 0000000000000001 R11: 0000000000000001 R12:
>> 0000000000000064
>> [   12.552369] R13: 0000000000000000 R14: ffff90a2c5b79400 R15:
>> 0000000000000000
>> [   12.552370] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000)
>> knlGS:0000000000000000
>> [   12.552371] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   12.552372] CR2: 0000000000000490 CR3: 000000010c28c000 CR4:
>> 0000000000750ee0
>> [   12.552373] PKRU: 55555554
>>
>> Please fix this for the next version. Reproducing this is easy, just
>> remove the firmware file from under /lib/firmware/intel .
>>
>> Regards,
>>
>> Hans
>>
>
Andy Shevchenko Sept. 4, 2023, 9:16 a.m. UTC | #13
On Mon, Sep 04, 2023 at 02:12:56PM +0800, Bingbu Cao wrote:
> On 9/3/23 10:32 PM, Hans de Goede wrote:

...

> Unfortunately, I did not reproduce this problem on my machine. The interrupt
> should not be triggered until buttress authentication if need. 
> So could you help try to move the devm_request_threaded_irq() block before

Maybe it's DEBUG_SHIRQ what is missing?
You always should use that for any of the code under development.
Hans de Goede Sept. 19, 2023, 10:23 a.m. UTC | #14
Hi,

On 9/4/23 08:12, Bingbu Cao wrote:
> Hans,
> 
> On 9/3/23 10:32 PM, Hans de Goede wrote:
>> Hi All,
>>
>> On 9/2/23 16:54, Hans de Goede wrote:
>>> Attached is one more patch which fixes an oops when
>>> using lockdep.
>>>
>>> With both patches applied this is:
>>>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> I withdraw my Tested-by. After a fresh install the driver crashed,
>> messing up the entire machine, due to the firmware not being
>> installed.
>>
>> On missing firmware the driver should simply exit cleanly, not
>> take down the entire machine.
>>
>> Here is a backtrace of the crash:
>>
>> [   12.549799] intel-ipu6 0000:00:05.0: cpd file name: intel/ipu6ep_fw.bin
>> [   12.551859] intel-ipu6 0000:00:05.0: Direct firmware load for intel/ipu6ep_fw.bin failed with error -2
>> [   12.551876] intel-ipu6 0000:00:05.0: Requesting signed firmware failed
>> [   12.551880] intel-ipu6: probe of 0000:00:05.0 failed with error -2
>> [   12.552112] BUG: kernel NULL pointer dereference, address: 0000000000000490
>> [   12.552116] #PF: supervisor read access in kernel mode
>> [   12.552118] #PF: error_code(0x0000) - not-present page
>> [   12.552119] PGD 0 P4D 0 
>> [   12.552121] Oops: 0000 [#1] PREEMPT SMP NOPTI
>> [   12.552124] CPU: 2 PID: 692 Comm: (udev-worker) Not tainted 6.5.0+ #1
>> [   12.552126] Hardware name: LENOVO 21CEZ9Q3US/21CEZ9Q3US, BIOS N3AET72W (1.37 ) 03/02/2023
>> [   12.552127] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552137] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>> [   12.552138] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>> [   12.552140] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>> [   12.552142] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>> [   12.552143] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>> [   12.552144] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>> [   12.552145] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>> [   12.552146] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>> [   12.552148] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   12.552150] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>> [   12.552152] PKRU: 55555554
>> [   12.552153] Call Trace:
>> [   12.552155]  <TASK>
>> [   12.552157]  ? __die+0x1f/0x70
>> [   12.552162]  ? page_fault_oops+0x13d/0x480
>> [   12.552167]  ? do_user_addr_fault+0x65/0x830
>> [   12.552170]  ? exc_page_fault+0x36/0x200
>> [   12.552174]  ? exc_page_fault+0x7b/0x200
>> [   12.552176]  ? asm_exc_page_fault+0x22/0x30
>> [   12.552180]  ? ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552186]  ? _raw_spin_unlock_irqrestore+0x35/0x60
>> [   12.552190]  free_irq+0x256/0x3a0
>> [   12.552194]  devres_release_all+0xa5/0xe0
>> [   12.552200]  device_unbind_cleanup+0xe/0x70
>> [   12.552203]  really_probe+0x145/0x3e0
>> [   12.552206]  ? __pfx___driver_attach+0x10/0x10
>> [   12.552209]  __driver_probe_device+0x78/0x160
>> [   12.552212]  driver_probe_device+0x1f/0x90
>> [   12.552215]  __driver_attach+0xd2/0x1c0
>> [   12.552218]  bus_for_each_dev+0x63/0xa0
>> [   12.552221]  bus_add_driver+0x115/0x210
>> [   12.552223]  driver_register+0x55/0x100
>> [   12.552226]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>> [   12.552234]  ipu6_init+0x20/0xff0 [intel_ipu6]
>> [   12.552241]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>> [   12.552247]  do_one_initcall+0x5a/0x360
>> [   12.552251]  ? rcu_is_watching+0xd/0x40
>> [   12.552254]  ? kmalloc_trace+0xa5/0xb0
>> [   12.552258]  do_init_module+0x60/0x230
>> [   12.552261]  init_module_from_file+0x75/0xa0
>> [   12.552265]  idempotent_init_module+0xf9/0x270
>> [   12.552268]  ? subflow_v6_conn_request+0xc0/0x120
>> [   12.552273]  __x64_sys_finit_module+0x5a/0xb0
>> [   12.552276]  do_syscall_64+0x59/0x90
>> [   12.552279]  ? do_syscall_64+0x68/0x90
>> [   12.552281]  ? lockdep_hardirqs_on+0x7d/0x100
>> [   12.552283]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>> [   12.552286] RIP: 0033:0x7fd5bff2cb5d
>> [   12.552288] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 7b 92 0c 00 f7 d8 64 89 01 48
>> [   12.552289] RSP: 002b:00007ffc50c03b38 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>> [   12.552291] RAX: ffffffffffffffda RBX: 000055e540797f00 RCX: 00007fd5bff2cb5d
>> [   12.552292] RDX: 0000000000000000 RSI: 00007fd5c052607d RDI: 000000000000000c
>> [   12.552293] RBP: 00007ffc50c03bf0 R08: 0000000000000000 R09: 00007ffc50c03b80
>> [   12.552294] R10: 000000000000000c R11: 0000000000000246 R12: 00007fd5c052607d
>> [   12.552296] R13: 0000000000020000 R14: 000055e540797030 R15: 000055e540796290
>> [   12.552301]  </TASK>
>> [   12.552302] Modules linked in: v4l2_async(+) processor_thermal_rfim industrialio_triggered_buffer ecdh_generic(+) processor_thermal_mbox kfifo_buf videodev snd processor_thermal_rapl intel_skl_int3472_tps68470 intel_ipu6(+) industrialio thunderbolt(+) soundcore tps68470_regulator rfkill mc intel_rapl_common ipu_bridge intel_hid int3403_thermal(+) idma64(+) soc_button_array intel_vsec igen6_edac int340x_thermal_zone clk_tps68470 joydev intel_skl_int3472_discrete sparse_keymap int3400_thermal acpi_thermal_rel acpi_pad acpi_tad loop zram hid_sensor_hub intel_ishtp_hid i915 i2c_algo_bit crct10dif_pclmul drm_buddy crc32_pclmul ttm crc32c_intel drm_display_helper intel_ish_ipc video nvme ghash_clmulni_intel ucsi_acpi wacom hid_multitouch sha512_ssse3 typec_ucsi nvme_core intel_ishtp cec typec i2c_hid_acpi i2c_hid wmi pinctrl_tigerlake serio_raw ip6_tables ip_tables fuse
>> [   12.552348] CR2: 0000000000000490
>> [   12.552351] ---[ end trace 0000000000000000 ]---
>> [   12.552352] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>> [   12.552361] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>> [   12.552363] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>> [   12.552365] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>> [   12.552366] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>> [   12.552367] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>> [   12.552368] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>> [   12.552369] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>> [   12.552370] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>> [   12.552371] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   12.552372] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>> [   12.552373] PKRU: 55555554
>>
>> Please fix this for the next version. Reproducing this is easy, just remove the firmware file from under /lib/firmware/intel .
> 
> Unfortunately, I did not reproduce this problem on my machine.

Ok.

> The interrupt
> should not be triggered until buttress authentication if need. 
> So could you help try to move the devm_request_threaded_irq() block before
> 
> ret = ipu6_buttress_authenticate(isp);
> 
> to check what will happen?

I'm not sure how that will help. You really should not rely on the hw not triggering an IRQ until a cerain point in time, instead you should modify the driver so that the IRQ is only registered once everything has been fully setup and the IRQ handler can safely run, since the IRQ handler can run as soon as you call request_irq(). E.g. the hw might be un an unexpected state causing the hw to immediately trigger the IRQ, which seems to be what happened during my testing.

Since I hit this when firmware loading failed, IMHO the request_irq() should be moved to after loading the fw. I really don't see how registering the IRQ before loading the fw is ready is useful.

Regards,

Hans
Bingbu Cao Sept. 20, 2023, 4:46 a.m. UTC | #15
Hans,

On 9/19/23 6:23 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/4/23 08:12, Bingbu Cao wrote:
>> Hans,
>>
>> On 9/3/23 10:32 PM, Hans de Goede wrote:
>>> Hi All,
>>>
>>> On 9/2/23 16:54, Hans de Goede wrote:
>>>> Attached is one more patch which fixes an oops when
>>>> using lockdep.
>>>>
>>>> With both patches applied this is:
>>>>
>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> I withdraw my Tested-by. After a fresh install the driver crashed,
>>> messing up the entire machine, due to the firmware not being
>>> installed.
>>>
>>> On missing firmware the driver should simply exit cleanly, not
>>> take down the entire machine.
>>>
>>> Here is a backtrace of the crash:
>>>
>>> [   12.549799] intel-ipu6 0000:00:05.0: cpd file name: intel/ipu6ep_fw.bin
>>> [   12.551859] intel-ipu6 0000:00:05.0: Direct firmware load for intel/ipu6ep_fw.bin failed with error -2
>>> [   12.551876] intel-ipu6 0000:00:05.0: Requesting signed firmware failed
>>> [   12.551880] intel-ipu6: probe of 0000:00:05.0 failed with error -2
>>> [   12.552112] BUG: kernel NULL pointer dereference, address: 0000000000000490
>>> [   12.552116] #PF: supervisor read access in kernel mode
>>> [   12.552118] #PF: error_code(0x0000) - not-present page
>>> [   12.552119] PGD 0 P4D 0 
>>> [   12.552121] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>> [   12.552124] CPU: 2 PID: 692 Comm: (udev-worker) Not tainted 6.5.0+ #1
>>> [   12.552126] Hardware name: LENOVO 21CEZ9Q3US/21CEZ9Q3US, BIOS N3AET72W (1.37 ) 03/02/2023
>>> [   12.552127] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>> [   12.552137] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>>> [   12.552138] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>>> [   12.552140] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>>> [   12.552142] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>>> [   12.552143] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>>> [   12.552144] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>>> [   12.552145] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>>> [   12.552146] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>>> [   12.552148] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [   12.552150] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>>> [   12.552152] PKRU: 55555554
>>> [   12.552153] Call Trace:
>>> [   12.552155]  <TASK>
>>> [   12.552157]  ? __die+0x1f/0x70
>>> [   12.552162]  ? page_fault_oops+0x13d/0x480
>>> [   12.552167]  ? do_user_addr_fault+0x65/0x830
>>> [   12.552170]  ? exc_page_fault+0x36/0x200
>>> [   12.552174]  ? exc_page_fault+0x7b/0x200
>>> [   12.552176]  ? asm_exc_page_fault+0x22/0x30
>>> [   12.552180]  ? ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>> [   12.552186]  ? _raw_spin_unlock_irqrestore+0x35/0x60
>>> [   12.552190]  free_irq+0x256/0x3a0
>>> [   12.552194]  devres_release_all+0xa5/0xe0
>>> [   12.552200]  device_unbind_cleanup+0xe/0x70
>>> [   12.552203]  really_probe+0x145/0x3e0
>>> [   12.552206]  ? __pfx___driver_attach+0x10/0x10
>>> [   12.552209]  __driver_probe_device+0x78/0x160
>>> [   12.552212]  driver_probe_device+0x1f/0x90
>>> [   12.552215]  __driver_attach+0xd2/0x1c0
>>> [   12.552218]  bus_for_each_dev+0x63/0xa0
>>> [   12.552221]  bus_add_driver+0x115/0x210
>>> [   12.552223]  driver_register+0x55/0x100
>>> [   12.552226]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>>> [   12.552234]  ipu6_init+0x20/0xff0 [intel_ipu6]
>>> [   12.552241]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>>> [   12.552247]  do_one_initcall+0x5a/0x360
>>> [   12.552251]  ? rcu_is_watching+0xd/0x40
>>> [   12.552254]  ? kmalloc_trace+0xa5/0xb0
>>> [   12.552258]  do_init_module+0x60/0x230
>>> [   12.552261]  init_module_from_file+0x75/0xa0
>>> [   12.552265]  idempotent_init_module+0xf9/0x270
>>> [   12.552268]  ? subflow_v6_conn_request+0xc0/0x120
>>> [   12.552273]  __x64_sys_finit_module+0x5a/0xb0
>>> [   12.552276]  do_syscall_64+0x59/0x90
>>> [   12.552279]  ? do_syscall_64+0x68/0x90
>>> [   12.552281]  ? lockdep_hardirqs_on+0x7d/0x100
>>> [   12.552283]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>>> [   12.552286] RIP: 0033:0x7fd5bff2cb5d
>>> [   12.552288] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 7b 92 0c 00 f7 d8 64 89 01 48
>>> [   12.552289] RSP: 002b:00007ffc50c03b38 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>>> [   12.552291] RAX: ffffffffffffffda RBX: 000055e540797f00 RCX: 00007fd5bff2cb5d
>>> [   12.552292] RDX: 0000000000000000 RSI: 00007fd5c052607d RDI: 000000000000000c
>>> [   12.552293] RBP: 00007ffc50c03bf0 R08: 0000000000000000 R09: 00007ffc50c03b80
>>> [   12.552294] R10: 000000000000000c R11: 0000000000000246 R12: 00007fd5c052607d
>>> [   12.552296] R13: 0000000000020000 R14: 000055e540797030 R15: 000055e540796290
>>> [   12.552301]  </TASK>
>>> [   12.552302] Modules linked in: v4l2_async(+) processor_thermal_rfim industrialio_triggered_buffer ecdh_generic(+) processor_thermal_mbox kfifo_buf videodev snd processor_thermal_rapl intel_skl_int3472_tps68470 intel_ipu6(+) industrialio thunderbolt(+) soundcore tps68470_regulator rfkill mc intel_rapl_common ipu_bridge intel_hid int3403_thermal(+) idma64(+) soc_button_array intel_vsec igen6_edac int340x_thermal_zone clk_tps68470 joydev intel_skl_int3472_discrete sparse_keymap int3400_thermal acpi_thermal_rel acpi_pad acpi_tad loop zram hid_sensor_hub intel_ishtp_hid i915 i2c_algo_bit crct10dif_pclmul drm_buddy crc32_pclmul ttm crc32c_intel drm_display_helper intel_ish_ipc video nvme ghash_clmulni_intel ucsi_acpi wacom hid_multitouch sha512_ssse3 typec_ucsi nvme_core intel_ishtp cec typec i2c_hid_acpi i2c_hid wmi pinctrl_tigerlake serio_raw ip6_tables ip_tables fuse
>>> [   12.552348] CR2: 0000000000000490
>>> [   12.552351] ---[ end trace 0000000000000000 ]---
>>> [   12.552352] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>> [   12.552361] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>>> [   12.552363] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>>> [   12.552365] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>>> [   12.552366] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>>> [   12.552367] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>>> [   12.552368] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>>> [   12.552369] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>>> [   12.552370] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>>> [   12.552371] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [   12.552372] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>>> [   12.552373] PKRU: 55555554
>>>
>>> Please fix this for the next version. Reproducing this is easy, just remove the firmware file from under /lib/firmware/intel .
>>
>> Unfortunately, I did not reproduce this problem on my machine.
> 
> Ok.
> 
>> The interrupt
>> should not be triggered until buttress authentication if need. 
>> So could you help try to move the devm_request_threaded_irq() block before
>>
>> ret = ipu6_buttress_authenticate(isp);
>>
>> to check what will happen?
> 
> I'm not sure how that will help. You really should not rely on the hw not triggering an IRQ until a cerain point in time, instead you should modify the driver so that the IRQ is only registered once everything has been fully setup and the IRQ handler can safely run, since the IRQ handler can run as soon as you call request_irq(). E.g. the hw might be un an unexpected state causing the hw to immediately trigger the IRQ, which seems to be what happened during my testing.
> 
> Since I hit this when firmware loading failed, IMHO the request_irq() should be moved to after loading the fw. I really don't see how registering the IRQ before loading the fw is ready is useful.

HW should not trigger any buttress IRQ until firmware authentication. So I think
it make sense to move the request_irq before buttress_authenticate(). So the
unexpected IRQ is not related to firmware load, firmware is not ready
before authentication, so there is no big difference between moving after
firmware load and before authentication.
> 
> Regards,
> 
> Hans
> 
>
Hans de Goede Sept. 20, 2023, 8:52 a.m. UTC | #16
Hi Bingbu,

On 9/20/23 06:46, Bingbu Cao wrote:
> Hans,
> 
> On 9/19/23 6:23 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 9/4/23 08:12, Bingbu Cao wrote:
>>> Hans,
>>>
>>> On 9/3/23 10:32 PM, Hans de Goede wrote:
>>>> Hi All,
>>>>
>>>> On 9/2/23 16:54, Hans de Goede wrote:
>>>>> Attached is one more patch which fixes an oops when
>>>>> using lockdep.
>>>>>
>>>>> With both patches applied this is:
>>>>>
>>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> I withdraw my Tested-by. After a fresh install the driver crashed,
>>>> messing up the entire machine, due to the firmware not being
>>>> installed.
>>>>
>>>> On missing firmware the driver should simply exit cleanly, not
>>>> take down the entire machine.
>>>>
>>>> Here is a backtrace of the crash:
>>>>
>>>> [   12.549799] intel-ipu6 0000:00:05.0: cpd file name: intel/ipu6ep_fw.bin
>>>> [   12.551859] intel-ipu6 0000:00:05.0: Direct firmware load for intel/ipu6ep_fw.bin failed with error -2
>>>> [   12.551876] intel-ipu6 0000:00:05.0: Requesting signed firmware failed
>>>> [   12.551880] intel-ipu6: probe of 0000:00:05.0 failed with error -2
>>>> [   12.552112] BUG: kernel NULL pointer dereference, address: 0000000000000490
>>>> [   12.552116] #PF: supervisor read access in kernel mode
>>>> [   12.552118] #PF: error_code(0x0000) - not-present page
>>>> [   12.552119] PGD 0 P4D 0 
>>>> [   12.552121] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>> [   12.552124] CPU: 2 PID: 692 Comm: (udev-worker) Not tainted 6.5.0+ #1
>>>> [   12.552126] Hardware name: LENOVO 21CEZ9Q3US/21CEZ9Q3US, BIOS N3AET72W (1.37 ) 03/02/2023
>>>> [   12.552127] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>>> [   12.552137] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>>>> [   12.552138] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>>>> [   12.552140] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>>>> [   12.552142] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>>>> [   12.552143] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>>>> [   12.552144] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>>>> [   12.552145] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>>>> [   12.552146] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>>>> [   12.552148] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [   12.552150] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>>>> [   12.552152] PKRU: 55555554
>>>> [   12.552153] Call Trace:
>>>> [   12.552155]  <TASK>
>>>> [   12.552157]  ? __die+0x1f/0x70
>>>> [   12.552162]  ? page_fault_oops+0x13d/0x480
>>>> [   12.552167]  ? do_user_addr_fault+0x65/0x830
>>>> [   12.552170]  ? exc_page_fault+0x36/0x200
>>>> [   12.552174]  ? exc_page_fault+0x7b/0x200
>>>> [   12.552176]  ? asm_exc_page_fault+0x22/0x30
>>>> [   12.552180]  ? ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>>> [   12.552186]  ? _raw_spin_unlock_irqrestore+0x35/0x60
>>>> [   12.552190]  free_irq+0x256/0x3a0
>>>> [   12.552194]  devres_release_all+0xa5/0xe0
>>>> [   12.552200]  device_unbind_cleanup+0xe/0x70
>>>> [   12.552203]  really_probe+0x145/0x3e0
>>>> [   12.552206]  ? __pfx___driver_attach+0x10/0x10
>>>> [   12.552209]  __driver_probe_device+0x78/0x160
>>>> [   12.552212]  driver_probe_device+0x1f/0x90
>>>> [   12.552215]  __driver_attach+0xd2/0x1c0
>>>> [   12.552218]  bus_for_each_dev+0x63/0xa0
>>>> [   12.552221]  bus_add_driver+0x115/0x210
>>>> [   12.552223]  driver_register+0x55/0x100
>>>> [   12.552226]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>>>> [   12.552234]  ipu6_init+0x20/0xff0 [intel_ipu6]
>>>> [   12.552241]  ? __pfx_ipu6_init+0x10/0x10 [intel_ipu6]
>>>> [   12.552247]  do_one_initcall+0x5a/0x360
>>>> [   12.552251]  ? rcu_is_watching+0xd/0x40
>>>> [   12.552254]  ? kmalloc_trace+0xa5/0xb0
>>>> [   12.552258]  do_init_module+0x60/0x230
>>>> [   12.552261]  init_module_from_file+0x75/0xa0
>>>> [   12.552265]  idempotent_init_module+0xf9/0x270
>>>> [   12.552268]  ? subflow_v6_conn_request+0xc0/0x120
>>>> [   12.552273]  __x64_sys_finit_module+0x5a/0xb0
>>>> [   12.552276]  do_syscall_64+0x59/0x90
>>>> [   12.552279]  ? do_syscall_64+0x68/0x90
>>>> [   12.552281]  ? lockdep_hardirqs_on+0x7d/0x100
>>>> [   12.552283]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>>>> [   12.552286] RIP: 0033:0x7fd5bff2cb5d
>>>> [   12.552288] Code: c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 7b 92 0c 00 f7 d8 64 89 01 48
>>>> [   12.552289] RSP: 002b:00007ffc50c03b38 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>>>> [   12.552291] RAX: ffffffffffffffda RBX: 000055e540797f00 RCX: 00007fd5bff2cb5d
>>>> [   12.552292] RDX: 0000000000000000 RSI: 00007fd5c052607d RDI: 000000000000000c
>>>> [   12.552293] RBP: 00007ffc50c03bf0 R08: 0000000000000000 R09: 00007ffc50c03b80
>>>> [   12.552294] R10: 000000000000000c R11: 0000000000000246 R12: 00007fd5c052607d
>>>> [   12.552296] R13: 0000000000020000 R14: 000055e540797030 R15: 000055e540796290
>>>> [   12.552301]  </TASK>
>>>> [   12.552302] Modules linked in: v4l2_async(+) processor_thermal_rfim industrialio_triggered_buffer ecdh_generic(+) processor_thermal_mbox kfifo_buf videodev snd processor_thermal_rapl intel_skl_int3472_tps68470 intel_ipu6(+) industrialio thunderbolt(+) soundcore tps68470_regulator rfkill mc intel_rapl_common ipu_bridge intel_hid int3403_thermal(+) idma64(+) soc_button_array intel_vsec igen6_edac int340x_thermal_zone clk_tps68470 joydev intel_skl_int3472_discrete sparse_keymap int3400_thermal acpi_thermal_rel acpi_pad acpi_tad loop zram hid_sensor_hub intel_ishtp_hid i915 i2c_algo_bit crct10dif_pclmul drm_buddy crc32_pclmul ttm crc32c_intel drm_display_helper intel_ish_ipc video nvme ghash_clmulni_intel ucsi_acpi wacom hid_multitouch sha512_ssse3 typec_ucsi nvme_core intel_ishtp cec typec i2c_hid_acpi i2c_hid wmi pinctrl_tigerlake serio_raw ip6_tables ip_tables fuse
>>>> [   12.552348] CR2: 0000000000000490
>>>> [   12.552351] ---[ end trace 0000000000000000 ]---
>>>> [   12.552352] RIP: 0010:ipu6_buttress_isr+0x6d/0x3a0 [intel_ipu6]
>>>> [   12.552361] Code: 34 03 00 00 c7 44 24 04 00 00 00 00 41 bc 64 00 00 00 45 31 ed 48 8b 85 50 04 00 00 89 98 9c 00 00 00 45 31 ff 4a 8b 7c fc 08 <4c> 8b b7 90 04 00 00 48 85 ff 74 46 48 83 bf 88 04 00 00 00 74 3c
>>>> [   12.552363] RSP: 0018:ffffb5928145fb08 EFLAGS: 00010046
>>>> [   12.552365] RAX: ffffb59289000000 RBX: 0000000000000040 RCX: 0000000000000001
>>>> [   12.552366] RDX: 0000000000000000 RSI: ffff90a2c67a2828 RDI: 0000000000000000
>>>> [   12.552367] RBP: ffff90a2c67a2828 R08: 0000000000000001 R09: 0000000000000001
>>>> [   12.552368] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000064
>>>> [   12.552369] R13: 0000000000000000 R14: ffff90a2c5b79400 R15: 0000000000000000
>>>> [   12.552370] FS:  00007fd5bf725940(0000) GS:ffff90a60f100000(0000) knlGS:0000000000000000
>>>> [   12.552371] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [   12.552372] CR2: 0000000000000490 CR3: 000000010c28c000 CR4: 0000000000750ee0
>>>> [   12.552373] PKRU: 55555554
>>>>
>>>> Please fix this for the next version. Reproducing this is easy, just remove the firmware file from under /lib/firmware/intel .
>>>
>>> Unfortunately, I did not reproduce this problem on my machine.
>>
>> Ok.
>>
>>> The interrupt
>>> should not be triggered until buttress authentication if need. 
>>> So could you help try to move the devm_request_threaded_irq() block before
>>>
>>> ret = ipu6_buttress_authenticate(isp);
>>>
>>> to check what will happen?
>>
>> I'm not sure how that will help. You really should not rely on the hw not triggering an IRQ until a cerain point in time, instead you should modify the driver so that the IRQ is only registered once everything has been fully setup and the IRQ handler can safely run, since the IRQ handler can run as soon as you call request_irq(). E.g. the hw might be un an unexpected state causing the hw to immediately trigger the IRQ, which seems to be what happened during my testing.
>>
>> Since I hit this when firmware loading failed, IMHO the request_irq() should be moved to after loading the fw. I really don't see how registering the IRQ before loading the fw is ready is useful.
> 
> HW should not trigger any buttress IRQ until firmware authentication. So I think
> it make sense to move the request_irq before buttress_authenticate(). So the
> unexpected IRQ is not related to firmware load, firmware is not ready
> before authentication, so there is no big difference between moving after
> firmware load and before authentication.

Ah I understand now. When you said "move the devm_request_threaded_irq() block before ipu6_buttress_authenticate()" I though this meant doing it even earlier then it was already being done. But I know see that this means moving the devm_request_threaded_irq() call to as late as possible.

Yes that sounds good to me, please it move there for the next version of this series.

Regards,

Hans
Claus Stovgaard Sept. 20, 2023, 12:32 p.m. UTC | #17
Hi Bingbu

On Wed, 2023-09-20 at 10:52 +0200, Hans de Goede wrote:
> Hi Bingbu,
> 
> 
> Yes that sounds good to me, please it move there for the next version
> of this series.
> 
> Regards,
> 
> Hans
> 

Now Hans mention it. When is the plan for the next version?
Would love to test my Dell on top of latest rc of 6.6, as the ivsc is
now merged upstream, and also with the updated v4l2 api changes.

Regards
Claus
Hans de Goede Oct. 2, 2023, 5:19 p.m. UTC | #18
Hi,

On 9/4/23 05:13, Cao, Bingbu wrote:
> Hans,
> 
> Thanks for your test and report.
> 
> I have made some changes locally which will support the latest
> v4l2-async APIs, I will also add the fix for this issue and merge
> them in v3.

I have been trying to make rawbayer capture with the mainline isys code
work in libcamera and I have hit several short comings in the ipu6-isys
code. I have attached 3 patches to fix various issues please integrate
these into the next version of this series.

Talking about the next version of this series, I think it would be
good to post a new version soon ?

Regards,

Hans
Laurent Pinchart Oct. 2, 2023, 5:38 p.m. UTC | #19
On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/4/23 05:13, Cao, Bingbu wrote:
> > Hans,
> > 
> > Thanks for your test and report.
> > 
> > I have made some changes locally which will support the latest
> > v4l2-async APIs, I will also add the fix for this issue and merge
> > them in v3.
> 
> I have been trying to make rawbayer capture with the mainline isys code
> work in libcamera and I have hit several short comings in the ipu6-isys
> code. I have attached 3 patches to fix various issues please integrate
> these into the next version of this series.

They look good to me.

> Talking about the next version of this series, I think it would be
> good to post a new version soon ?
> 

> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 16:37:11 +0200
> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>  /dev/video enum_fmt
> 
> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index dc1605491352..20fd03f6f204 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>  			      struct v4l2_fmtdesc *f)
>  {
> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
> -		return -EINVAL;
> +	unsigned int i, found = 0;
>  
> -	f->flags = 0;
> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
> +	if (!f->mbus_code) {
> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
> +			return -EINVAL;
>  
> -	return 0;
> +		f->flags = 0;
> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
> +			continue;
> +
> +		if (f->index == found) {
> +			f->flags = 0;
> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
> +			return 0;
> +		}
> +
> +		found++;
> +	}
> +
> +	return -EINVAL;
>  }
>  
>  static int vidioc_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> -- 
> 2.41.0
> 

> From 8250d2c3fd1c2ab83debcca80b4947d3b9d410f7 Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 17:02:06 +0200
> Subject: [PATCH 2/3] media: intel/ipu6: Implement g_enum_framesizes for isys
>  /dev/video# nodes
> 
> Implement g_enum_framesizes for isys /dev/video# nodes.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index 20fd03f6f204..ad74a19527b7 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -157,6 +157,23 @@ int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>  	return -EINVAL;
>  }
>  
> +static int ipu6_isys_vidioc_g_enum_framesizes(struct file *file, void *fh,
> +					      struct v4l2_frmsizeenum *fsize)
> +{
> +	if (fsize->index > 0)
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> +	fsize->stepwise.min_width = IPU6_ISYS_MIN_WIDTH;
> +	fsize->stepwise.max_width = IPU6_ISYS_MAX_WIDTH;
> +	fsize->stepwise.min_height = IPU6_ISYS_MIN_HEIGHT;
> +	fsize->stepwise.max_height = IPU6_ISYS_MAX_HEIGHT;
> +	fsize->stepwise.step_width = 1;
> +	fsize->stepwise.step_height = 1;
> +
> +	return 0;
> +}
> +
>  static int vidioc_g_fmt_vid_cap_mplane(struct file *file, void *fh,
>  				       struct v4l2_format *fmt)
>  {
> @@ -987,6 +1004,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
>  static const struct v4l2_ioctl_ops ioctl_ops_mplane = {
>  	.vidioc_querycap = ipu6_isys_vidioc_querycap,
>  	.vidioc_enum_fmt_vid_cap = ipu6_isys_vidioc_enum_fmt,
> +	.vidioc_enum_framesizes = ipu6_isys_vidioc_g_enum_framesizes,
>  	.vidioc_g_fmt_vid_cap_mplane = vidioc_g_fmt_vid_cap_mplane,
>  	.vidioc_s_fmt_vid_cap_mplane = vidioc_s_fmt_vid_cap_mplane,
>  	.vidioc_try_fmt_vid_cap_mplane = vidioc_try_fmt_vid_cap_mplane,
> -- 
> 2.41.0
> 

> From b5db94bbd147711885986c1f6e46f59fdca10d3c Mon Sep 17 00:00:00 2001
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 2 Oct 2023 16:05:35 +0200
> Subject: [PATCH 3/3] media: intel/ipu6: Set V4L2_CAP_IO_MC flag for isys
>  /dev/video# nodes
> 
> The IPU6 isys is a media-controller centric device which needs
> the pipeline to be configured using the media controller API before use.
> 
> Set the V4L2_CAP_IO_MC flag to reflect this.
> 
> This also allows dropping of the enum_input() g_input() and s_input()
> implementations, with V4L2_CAP_IO_MC set the v4l2-core will take care
> of those.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 ++-----------------
>  1 file changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index ad74a19527b7..e6fc32603c3f 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -262,29 +262,6 @@ static int vidioc_try_fmt_vid_cap_mplane(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static int vidioc_enum_input(struct file *file, void *fh,
> -			     struct v4l2_input *input)
> -{
> -	if (input->index > 0)
> -		return -EINVAL;
> -	strscpy(input->name, "camera", sizeof(input->name));
> -	input->type = V4L2_INPUT_TYPE_CAMERA;
> -
> -	return 0;
> -}
> -
> -static int vidioc_g_input(struct file *file, void *fh, unsigned int *input)
> -{
> -	*input = 0;
> -
> -	return 0;
> -}
> -
> -static int vidioc_s_input(struct file *file, void *fh, unsigned int input)
> -{
> -	return input == 0 ? 0 : -EINVAL;
> -}
> -
>  static int link_validate(struct media_link *link)
>  {
>  	struct ipu6_isys_video *av =
> @@ -1017,9 +994,6 @@ static const struct v4l2_ioctl_ops ioctl_ops_mplane = {
>  	.vidioc_streamon = vb2_ioctl_streamon,
>  	.vidioc_streamoff = vb2_ioctl_streamoff,
>  	.vidioc_expbuf = vb2_ioctl_expbuf,
> -	.vidioc_enum_input = vidioc_enum_input,
> -	.vidioc_g_input = vidioc_g_input,
> -	.vidioc_s_input = vidioc_s_input,
>  };
>  
>  static const struct media_entity_operations entity_ops = {
> @@ -1217,7 +1191,8 @@ int ipu6_isys_video_init(struct ipu6_isys_video *av)
>  
>  	mutex_init(&av->mutex);
>  	av->vdev.device_caps = V4L2_CAP_STREAMING |
> -			       V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +			       V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> +			       V4L2_CAP_IO_MC;
>  	av->vdev.vfl_dir = VFL_DIR_RX;
>  
>  	ret = ipu6_isys_queue_init(&av->aq);
> -- 
> 2.41.0
>
Bingbu Cao Oct. 9, 2023, 12:25 p.m. UTC | #20
Hans and Laurent,

On 10/3/23 1:41 AM, Hans de Goede wrote:
> Hi,
> 
> On 10/2/23 19:38, Laurent Pinchart wrote:
>> On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 9/4/23 05:13, Cao, Bingbu wrote:
>>>> Hans,
>>>>
>>>> Thanks for your test and report.
>>>>
>>>> I have made some changes locally which will support the latest
>>>> v4l2-async APIs, I will also add the fix for this issue and merge
>>>> them in v3.
>>>
>>> I have been trying to make rawbayer capture with the mainline isys code
>>> work in libcamera and I have hit several short comings in the ipu6-isys
>>> code. I have attached 3 patches to fix various issues please integrate
>>> these into the next version of this series.
>>
>> They look good to me.
> 
> Actually I just realized I forgot to commit + squash in a bug fix:
> 
>>> Talking about the next version of this series, I think it would be
>>> good to post a new version soon ?
>>>
>>
>>> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
>>> From: Hans de Goede <hdegoede@redhat.com>
>>> Date: Mon, 2 Oct 2023 16:37:11 +0200
>>> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>>>  /dev/video enum_fmt
>>>
>>> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>>>  1 file changed, 23 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> index dc1605491352..20fd03f6f204 100644
>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>>>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>>>  			      struct v4l2_fmtdesc *f)
>>>  {
>>> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>> -		return -EINVAL;
>>> +	unsigned int i, found = 0;
>>>  
>>> -	f->flags = 0;
>>> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>> +	if (!f->mbus_code) {
>>> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>> +			return -EINVAL;
>>>  
>>> -	return 0;
>>> +		f->flags = 0;
>>> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
> 
> There is a:
> 		return 0;
> 
> missing here.		
> 
>>> +	}
>>> +
> 
> Regards,
> 
> Hans
> 
> 
> 
>>> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
>>> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>> +			continue;
>>> +
>>> +		if (f->index == found) {
>>> +			f->flags = 0;
>>> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
>>> +			return 0;
>>> +		}
>>> +
>>> +		found++;
>>> +	}

A little confused here -

If the `mbus_code` argument here is not zero, does the user expect that
the first try (f->index == 0) should be found and return? `found` is
always 0 here?

My understanding is - user will try to enum again if return -EINVAL.

>>> +
>>> +	return -EINVAL;
>>>  }
>>>


<snip>
Hans de Goede Oct. 9, 2023, 12:53 p.m. UTC | #21
Hi Bingbu,

On 10/9/23 14:25, Bingbu Cao wrote:
> 
> Hans and Laurent,
> 
> On 10/3/23 1:41 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 10/2/23 19:38, Laurent Pinchart wrote:
>>> On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 9/4/23 05:13, Cao, Bingbu wrote:
>>>>> Hans,
>>>>>
>>>>> Thanks for your test and report.
>>>>>
>>>>> I have made some changes locally which will support the latest
>>>>> v4l2-async APIs, I will also add the fix for this issue and merge
>>>>> them in v3.
>>>>
>>>> I have been trying to make rawbayer capture with the mainline isys code
>>>> work in libcamera and I have hit several short comings in the ipu6-isys
>>>> code. I have attached 3 patches to fix various issues please integrate
>>>> these into the next version of this series.
>>>
>>> They look good to me.
>>
>> Actually I just realized I forgot to commit + squash in a bug fix:
>>
>>>> Talking about the next version of this series, I think it would be
>>>> good to post a new version soon ?
>>>>
>>>
>>>> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>> Date: Mon, 2 Oct 2023 16:37:11 +0200
>>>> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>>>>  /dev/video enum_fmt
>>>>
>>>> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>>>>  1 file changed, 23 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>> index dc1605491352..20fd03f6f204 100644
>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>>>>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>>>>  			      struct v4l2_fmtdesc *f)
>>>>  {
>>>> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>> -		return -EINVAL;
>>>> +	unsigned int i, found = 0;
>>>>  
>>>> -	f->flags = 0;
>>>> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>> +	if (!f->mbus_code) {
>>>> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>> +			return -EINVAL;
>>>>  
>>>> -	return 0;
>>>> +		f->flags = 0;
>>>> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>
>> There is a:
>> 		return 0;
>>
>> missing here.		
>>
>>>> +	}
>>>> +
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
>>>> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>>> +			continue;
>>>> +
>>>> +		if (f->index == found) {
>>>> +			f->flags = 0;
>>>> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
>>>> +			return 0;
>>>> +		}
>>>> +
>>>> +		found++;
>>>> +	}
> 
> A little confused here -
> 
> If the `mbus_code` argument here is not zero, does the user expect that
> the first try (f->index == 0) should be found and return? `found` is
> always 0 here?

Notice how formats where:

		if (f->mbus_code != ipu6_isys_pfmts[i].code)

Is true are skipped:

		if (f->mbus_code != ipu6_isys_pfmts[i].code)
			continue;

The idea is that for (f->index == 0) the first format
matching the passed in mbus_code is returned and then
for  (f->index == 1) the second format matching the passed
in mbus_code is returned, etc.

In practice this means that e.g. for a mbus_code for
a 10bit raw bayer format both the padded (one 10 bits
pixel in each 16bit integer) and packed formats are
returned.


> My understanding is - user will try to enum again if return -EINVAL.

No, -EINVAL means that the end of the list of
formats has been reached, so the user keeps
calling this function with higher
f->index values until -EINVAL is returned.

Regards,

Hans





> 
>>>> +
>>>> +	return -EINVAL;
>>>>  }
>>>>
> 
> 
> <snip>
Hans de Goede Oct. 10, 2023, 8:10 a.m. UTC | #22
Hi,

On 10/10/23 04:54, Bingbu Cao wrote:
> Hans,
> 
> On 10/9/23 8:53 PM, Hans de Goede wrote:
>> Hi Bingbu,
>>
>> On 10/9/23 14:25, Bingbu Cao wrote:
>>>
>>> Hans and Laurent,
>>>
>>> On 10/3/23 1:41 AM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 10/2/23 19:38, Laurent Pinchart wrote:
>>>>> On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 9/4/23 05:13, Cao, Bingbu wrote:
>>>>>>> Hans,
>>>>>>>
>>>>>>> Thanks for your test and report.
>>>>>>>
>>>>>>> I have made some changes locally which will support the latest
>>>>>>> v4l2-async APIs, I will also add the fix for this issue and merge
>>>>>>> them in v3.
>>>>>>
>>>>>> I have been trying to make rawbayer capture with the mainline isys code
>>>>>> work in libcamera and I have hit several short comings in the ipu6-isys
>>>>>> code. I have attached 3 patches to fix various issues please integrate
>>>>>> these into the next version of this series.
>>>>>
>>>>> They look good to me.
>>>>
>>>> Actually I just realized I forgot to commit + squash in a bug fix:
>>>>
>>>>>> Talking about the next version of this series, I think it would be
>>>>>> good to post a new version soon ?
>>>>>>
>>>>>
>>>>>> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
>>>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>>>> Date: Mon, 2 Oct 2023 16:37:11 +0200
>>>>>> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>>>>>>  /dev/video enum_fmt
>>>>>>
>>>>>> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>>>>>>  1 file changed, 23 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> index dc1605491352..20fd03f6f204 100644
>>>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>>>>>>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>>>>>>  			      struct v4l2_fmtdesc *f)
>>>>>>  {
>>>>>> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>> -		return -EINVAL;
>>>>>> +	unsigned int i, found = 0;
>>>>>>  
>>>>>> -	f->flags = 0;
>>>>>> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>>> +	if (!f->mbus_code) {
>>>>>> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>> +			return -EINVAL;
>>>>>>  
>>>>>> -	return 0;
>>>>>> +		f->flags = 0;
>>>>>> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>
>>>> There is a:
>>>> 		return 0;
>>>>
>>>> missing here.		
>>>>
>>>>>> +	}
>>>>>> +
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>>> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
>>>>>> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>>>>> +			continue;
>>>>>> +
>>>>>> +		if (f->index == found) {
>>>>>> +			f->flags = 0;
>>>>>> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
>>>>>> +			return 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		found++;
>>>>>> +	}
>>>
>>> A little confused here -
>>>
>>> If the `mbus_code` argument here is not zero, does the user expect that
>>> the first try (f->index == 0) should be found and return? `found` is
>>> always 0 here?
>>
>> Notice how formats where:
>>
>> 		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>
>> Is true are skipped:
>>
>> 		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>> 			continue;
>>
>> The idea is that for (f->index == 0) the first format
>> matching the passed in mbus_code is returned and then
>> for  (f->index == 1) the second format matching the passed
>> in mbus_code is returned, etc.
> 
> Ack. So for 1:1 match case, is (f->index == 0) expected for all formats?

If there is only 1 format which matches the mbus-code then yes
that fmt will only be returned for (f->index == 0).

But as mentioned already for raw bayer there are typically
2 formats matching the same mbus-code:

>> In practice this means that e.g. for a mbus_code for
>> a 10bit raw bayer format both the padded (one 10 bits
>> pixel in each 16bit integer) and packed formats are
>> returned.
>>
>>
>>> My understanding is - user will try to enum again if return -EINVAL.
>>
>> No, -EINVAL means that the end of the list of
>> formats has been reached, so the user keeps
>> calling this function with higher
>> f->index values until -EINVAL is returned.
> 
> So for libcamera, it's trying to enumerate all the pixel formats,
> Is it trying to enumerate from index 0 for each `mbus_code` or continue next
> format enumeration from higher index?

libcamera sets up the media-controller graph, so it already
knowns the mbus_code between the v4l2subdevs. AFAIK after setting
up the graph it uses mbus_code filtering to only get output formats
for /dev/video# which actually match with the configured mbus-code.

Regards,

Hans
Bingbu Cao Oct. 10, 2023, 8:35 a.m. UTC | #23
Hans,

On 10/10/23 4:10 PM, Hans de Goede wrote:
> Hi,
> 
> On 10/10/23 04:54, Bingbu Cao wrote:
>> Hans,
>>
>> On 10/9/23 8:53 PM, Hans de Goede wrote:
>>> Hi Bingbu,
>>>
>>> On 10/9/23 14:25, Bingbu Cao wrote:
>>>>
>>>> Hans and Laurent,
>>>>
>>>> On 10/3/23 1:41 AM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 10/2/23 19:38, Laurent Pinchart wrote:
>>>>>> On Mon, Oct 02, 2023 at 07:19:13PM +0200, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 9/4/23 05:13, Cao, Bingbu wrote:
>>>>>>>> Hans,
>>>>>>>>
>>>>>>>> Thanks for your test and report.
>>>>>>>>
>>>>>>>> I have made some changes locally which will support the latest
>>>>>>>> v4l2-async APIs, I will also add the fix for this issue and merge
>>>>>>>> them in v3.
>>>>>>>
>>>>>>> I have been trying to make rawbayer capture with the mainline isys code
>>>>>>> work in libcamera and I have hit several short comings in the ipu6-isys
>>>>>>> code. I have attached 3 patches to fix various issues please integrate
>>>>>>> these into the next version of this series.
>>>>>>
>>>>>> They look good to me.
>>>>>
>>>>> Actually I just realized I forgot to commit + squash in a bug fix:
>>>>>
>>>>>>> Talking about the next version of this series, I think it would be
>>>>>>> good to post a new version soon ?
>>>>>>>
>>>>>>
>>>>>>> From 14f42fd3071a7aba8b83b98802ca3b413794296d Mon Sep 17 00:00:00 2001
>>>>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Date: Mon, 2 Oct 2023 16:37:11 +0200
>>>>>>> Subject: [PATCH 1/3] media: intel/ipu6: Add mbus code filtering to isys
>>>>>>>  /dev/video enum_fmt
>>>>>>>
>>>>>>> Add mbus code filtering to ipu6_isys_vidioc_enum_fmt().
>>>>>>>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 29 +++++++++++++++----
>>>>>>>  1 file changed, 23 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>>> index dc1605491352..20fd03f6f204 100644
>>>>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>>>>>> @@ -130,14 +130,31 @@ int ipu6_isys_vidioc_querycap(struct file *file, void *fh,
>>>>>>>  int ipu6_isys_vidioc_enum_fmt(struct file *file, void *fh,
>>>>>>>  			      struct v4l2_fmtdesc *f)
>>>>>>>  {
>>>>>>> -	if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>>> -		return -EINVAL;
>>>>>>> +	unsigned int i, found = 0;
>>>>>>>  
>>>>>>> -	f->flags = 0;
>>>>>>> -	f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>>> -	f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>>>> +	if (!f->mbus_code) {
>>>>>>> +		if (f->index >= ARRAY_SIZE(ipu6_isys_pfmts))
>>>>>>> +			return -EINVAL;
>>>>>>>  
>>>>>>> -	return 0;
>>>>>>> +		f->flags = 0;
>>>>>>> +		f->pixelformat = ipu6_isys_pfmts[f->index].pixelformat;
>>>>>>> +		f->mbus_code = ipu6_isys_pfmts[f->index].code;
>>>>>
>>>>> There is a:
>>>>> 		return 0;
>>>>>
>>>>> missing here.		
>>>>>
>>>>>>> +	}
>>>>>>> +
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>
>>>>>>> +	for (i = 0; i < ARRAY_SIZE(ipu6_isys_pfmts); i++) {
>>>>>>> +		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>>>>>> +			continue;
>>>>>>> +
>>>>>>> +		if (f->index == found) {
>>>>>>> +			f->flags = 0;
>>>>>>> +			f->pixelformat = ipu6_isys_pfmts[i].pixelformat;
>>>>>>> +			return 0;
>>>>>>> +		}
>>>>>>> +
>>>>>>> +		found++;
>>>>>>> +	}
>>>>
>>>> A little confused here -
>>>>
>>>> If the `mbus_code` argument here is not zero, does the user expect that
>>>> the first try (f->index == 0) should be found and return? `found` is
>>>> always 0 here?
>>>
>>> Notice how formats where:
>>>
>>> 		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>>
>>> Is true are skipped:
>>>
>>> 		if (f->mbus_code != ipu6_isys_pfmts[i].code)
>>> 			continue;
>>>
>>> The idea is that for (f->index == 0) the first format
>>> matching the passed in mbus_code is returned and then
>>> for  (f->index == 1) the second format matching the passed
>>> in mbus_code is returned, etc.
>>
>> Ack. So for 1:1 match case, is (f->index == 0) expected for all formats?
> 
> If there is only 1 format which matches the mbus-code then yes
> that fmt will only be returned for (f->index == 0).
> 
> But as mentioned already for raw bayer there are typically
> 2 formats matching the same mbus-code:
> 
>>> In practice this means that e.g. for a mbus_code for
>>> a 10bit raw bayer format both the padded (one 10 bits
>>> pixel in each 16bit integer) and packed formats are
>>> returned.
>>>
>>>
>>>> My understanding is - user will try to enum again if return -EINVAL.
>>>
>>> No, -EINVAL means that the end of the list of
>>> formats has been reached, so the user keeps
>>> calling this function with higher
>>> f->index values until -EINVAL is returned.
>>
>> So for libcamera, it's trying to enumerate all the pixel formats,
>> Is it trying to enumerate from index 0 for each `mbus_code` or continue next
>> format enumeration from higher index?
> 
> libcamera sets up the media-controller graph, so it already
> knowns the mbus_code between the v4l2subdevs. AFAIK after setting
> up the graph it uses mbus_code filtering to only get output formats
> for /dev/video# which actually match with the configured mbus-code.

Ack, thanks.

> 
> Regards,
> 
> Hans
> 
> 
>