| Message ID | 20230727071558.1148653-1-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Intel IPU6 and IPU6 input system drivers | expand |
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
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. > > Regards > Claus Stovgaaard >
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 >> >
Bingbu On Mon, 2023-08-21 at 14:22 +0800, Bingbu Cao wrote: > > 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. > > > > > > ... > > > > > > 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" 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 :) /Claus > >
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 /Claus
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, On 8/21/23 6:07 PM, 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). 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. 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. :) > > > 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 >
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
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 :-)
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
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
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
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
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, 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. ------------------------------------------------------------------------ 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 >
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. 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? Thanks! > > Regards, > > Hans > >
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 >> >
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.
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
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 > >
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
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
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
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 >
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++; >> + } >> + >> + 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 >> > >
Hans, Thanks for your patch. 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. > I will squash them into v2. >>> + } >>> + > > Regards, > > Hans > > <snip>
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>
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, 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? > > 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? > > Regards, > > Hans > > > > > >> >>>>> + >>>>> + return -EINVAL; >>>>> } >>>>> >> >> >> <snip> >
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
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 > > >
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