| Message ID | 20210526152308.16525-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | CSI2RX support on J721E | expand |
Hi Pratyush, On 26/05/2021 18:22, Pratyush Yadav wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > driver, and finally adds the TI CSI2RX wrapper driver. > > Tested on TI's J721E with OV5640 sensor. I'm having some trouble unloading and reloading the modules: rmmod ti_cal rmmod j721e_csi2rx rmmod cdns_csi2rx rmmod cdns_dphy rmmod ov5640 [ 37.943128] ------------[ cut here ]------------ [ 37.947752] WARNING: CPU: 1 PID: 628 at drivers/media/v4l2-core/v4l2-ctrls-core.c:1807 __v4l2_ctrl_handler_setup+0x15c/0x170 [ 37.958963] Modules linked in: ov5640(-) v4l2_fwnode tidss ti_tfp410 tc358767 display_connector cdns_mhdp8546 panel_simple drm_kms_helper drm drm_panel_orientation_quirks cfbfill rect cfbimgblt cfbcopyarea phy_j721e_wiz phy_cadence_torrent [last unloaded: cdns_dphy] [ 37.982455] CPU: 1 PID: 628 Comm: rmmod Not tainted 5.13.0-rc1-00205-g93acc23badc8 #3 [ 37.990271] Hardware name: Texas Instruments K3 J721E SoC (DT) [ 37.996090] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) [ 38.002085] pc : __v4l2_ctrl_handler_setup+0x15c/0x170 [ 38.007214] lr : __v4l2_ctrl_handler_setup+0x158/0x170 [ 38.012343] sp : ffff80001476fae0 [ 38.015648] x29: ffff80001476fae0 x28: ffff000806780000 x27: 0000000000000000 [ 38.022781] x26: ffff00080300b448 x25: ffff000804ad4ac0 x24: 0000000000000001 [ 38.029912] x23: 0000000000000000 x22: 0000000000000005 x21: ffff000801fa7880 [ 38.037043] x20: ffff000801fa7888 x19: ffff000801fa7ba8 x18: 0000000000000000 [ 38.044173] x17: 0000000000000000 x16: 0000000000000010 x15: 0000000000000001 [ 38.051305] x14: 000000000000003b x13: 00000000aaaaaaab x12: ffff800011a915b8 [ 38.058436] x11: 00000000000c001c x10: 000000008260a2b7 x9 : ffff800009362d9c [ 38.065566] x8 : ffff800011887100 x7 : 0000000000000000 x6 : 0000000000000001 [ 38.072698] x5 : 0000000000000001 x4 : 0000000000000001 x3 : ffff800011260000 [ 38.079829] x2 : 00000000000000c0 x1 : 00000000000000c0 x0 : 0000000000000000 [ 38.086960] Call trace: [ 38.089399] __v4l2_ctrl_handler_setup+0x15c/0x170 [ 38.094181] ov5640_resume+0x1fc/0x270 [ov5640] [ 38.098709] __rpm_callback+0x98/0x160 [ 38.102452] rpm_callback+0x2c/0x90 [ 38.105934] rpm_resume+0x45c/0x6f4 [ 38.109415] __pm_runtime_resume+0x54/0xc0 [ 38.113503] __device_release_driver+0x40/0x240 [ 38.118025] driver_detach+0xd0/0x160 [ 38.121680] bus_remove_driver+0x68/0xe0 [ 38.125595] driver_unregister+0x3c/0x6c [ 38.129509] i2c_del_driver+0x64/0xb0 [ 38.133166] ov5640_i2c_driver_exit+0x1c/0xc948 [ov5640] [ 38.138469] __arm64_sys_delete_module+0x1b0/0x27c [ 38.143251] invoke_syscall+0x50/0x120 [ 38.146995] el0_svc_common.constprop.0+0x68/0x104 [ 38.151777] do_el0_svc+0x30/0x9c [ 38.155086] el0_svc+0x2c/0x54 [ 38.158135] el0_sync_handler+0x1a8/0x1ac [ 38.162136] el0_sync+0x198/0x1c0 [ 38.165444] irq event stamp: 11302 [ 38.168837] hardirqs last enabled at (11301): [<ffff800010bf4e40>] _raw_spin_unlock_irq+0x50/0xa0 [ 38.177781] hardirqs last disabled at (11302): [<ffff800010be7a64>] el1_dbg+0x24/0xa0 [ 38.185595] softirqs last enabled at (10378): [<ffff800010010ba0>] __do_softirq+0x500/0x6bc [ 38.194017] softirqs last disabled at (10373): [<ffff80001005d4c4>] __irq_exit_rcu+0x1d4/0x1e0 [ 38.202614] ---[ end trace 7037324a951cb149 ]--- rmmod v4l2_fwnode insmod /root/nfs/work/linux/drivers/media/v4l2-core/v4l2-fwnode.ko insmod /root/nfs/work/linux/drivers/phy/cadence/cdns-dphy.ko insmod /root/nfs/work/linux/drivers/media/platform/cadence/cdns-csi2rx.ko ERROR: Unhandled External Abort received on 0x80000001 from S-EL1 ERROR: exception reason=0 syndrome=0xbf000000 Unhandled Exception from EL1 x0 = 0x0000000000000000 x1 = 0xffff000804d59800 x2 = 0xffff8000146c4000 x3 = 0xffff800011260000 x4 = 0x0000000000000001 x5 = 0x0000000000000001 x6 = 0x0000000000000001 x7 = 0x0000000000000000 x8 = 0xffff800011887100 x9 = 0xffff800010bf5190 x10 = 0x000000008260a2b7 x11 = 0x00000000000c821d x12 = 0xffff800011a915b8 x13 = 0x0000000000000001 x14 = 0x0000000000000000 x15 = 0x0000000000000020 x16 = 0x0000000000000000 x17 = 0x0000000000000000 x18 = 0x00000000fffffffb x19 = 0xffff000806d44c00 x20 = 0x0000000000000000 x21 = 0xffff800009280058 x22 = 0xffff00080583c810 x23 = 0xffff00080583c800 x24 = 0xffff800009280058 x25 = 0x0000000000000047 x26 = 0xffff8000116d71d8 x27 = 0xffff800009280350 x28 = 0xffff800009280148 x29 = 0xffff80001432f850 x30 = 0xffff8000092506b8 scr_el3 = 0x000000000000073d sctlr_el3 = 0x0000000030cd183f cptr_el3 = 0x0000000000000000 tcr_el3 = 0x0000000080803520 daif = 0x00000000000002c0 mair_el3 = 0x00000000004404ff spsr_el3 = 0x0000000000000005 elr_el3 = 0xffff80000925043c ttbr0_el3 = 0x0000000070010b00 esr_el3 = 0x00000000bf000000 far_el3 = 0x0000000000000000 spsr_el1 = 0x0000000060000005 elr_el1 = 0xffff800010be8cb0 spsr_abt = 0x0000000000000000 spsr_und = 0x0000000000000000 spsr_irq = 0x0000000000000000 spsr_fiq = 0x0000000000000000 sctlr_el1 = 0x0000000034d4d91d actlr_el1 = 0x0000000000000000 cpacr_el1 = 0x0000000000300000 csselr_el1 = 0x0000000000000000 sp_el1 = 0xffff80001432f850 esr_el1 = 0x0000000056000000 ttbr0_el1 = 0x0000000882773200 ttbr1_el1 = 0x06d8000083180000 mair_el1 = 0x000c0400bb44ffff amair_el1 = 0x0000000000000000 tcr_el1 = 0x00000034f5d07590 tpidr_el1 = 0xffff80086e790000 tpidr_el0 = 0x0000ffff895c6910 tpidrro_el0 = 0x0000000000000000 par_el1 = 0x0000000000000000 mpidr_el1 = 0x0000000080000001 afsr0_el1 = 0x0000000000000000 afsr1_el1 = 0x0000000000000000 contextidr_el1 = 0x0000000000000000 vbar_el1 = 0xffff800010011000 cntp_ctl_el0 = 0x0000000000000005 cntp_cval_el0 = 0x000000023f77b7a1 cntv_ctl_el0 = 0x0000000000000000 cntv_cval_el0 = 0x0000000000000000 cntkctl_el1 = 0x00000000000000d6 sp_el0 = 0x000000007000abd0 isr_el1 = 0x0000000000000040 dacr32_el2 = 0x0000000000000000 ifsr32_el2 = 0x0000000000000000 cpuectlr_el1 = 0x0000001b00000040 cpumerrsr_el1 = 0x0000000000000000 l2merrsr_el1 = 0x0000000000000000
Hi Pratyush, On 26/05/2021 18:22, Pratyush Yadav wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > driver, and finally adds the TI CSI2RX wrapper driver. > > Tested on TI's J721E with OV5640 sensor. I also see this after a few captures: [ 84.115503] ------------[ cut here ]------------ [ 84.120144] DMA-API: ti-udma 31150000.dma-controller: mapping sg segment longer than device claims to support [len=1900544] [max=65536] [ 84.132376] WARNING: CPU: 1 PID: 594 at kernel/dma/debug.c:1172 debug_dma_map_sg+0x304/0x390 [ 84.140804] Modules linked in: ov5640 ti_cal j721e_csi2rx cdns_csi2rx cdns_dphy v4l2_fwnode tidss ti_tfp410 tc358767 display_connector cdns_mhdp8546 panel_simple drm_kms_helper d rm drm_panel_orientation_quirks cfbfillrect cfbimgblt cfbcopyarea phy_j721e_wiz phy_cadence_torrent [ 84.165298] CPU: 1 PID: 594 Comm: cam-mplex.py Not tainted 5.13.0-rc1-00206-g98bb91e95a28-dirty #5 [ 84.174236] Hardware name: Texas Instruments K3 J721E SoC (DT) [ 84.180051] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) [ 84.186041] pc : debug_dma_map_sg+0x304/0x390 [ 84.190383] lr : debug_dma_map_sg+0x304/0x390 [ 84.194725] sp : ffff800014d0f730 [ 84.198026] x29: ffff800014d0f730 x28: ffff000801544680 x27: ffffffffffffffff [ 84.205148] x26: 0000000000000000 x25: 0000000000000002 x24: 0000000000000001 [ 84.212269] x23: ffff80001163abe0 x22: 0000000000000000 x21: 0000000000000001 [ 84.219390] x20: ffff000801fa3010 x19: ffff0008075c7580 x18: 0000000000000000 [ 84.226510] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000030 [ 84.233630] x14: 6e61687420726567 x13: 6e6f6c20746e656d x12: 7420736d69616c63 [ 84.240751] x11: 2065636976656420 x10: ffff8000116b18f8 x9 : ffff8000100eb920 [ 84.247871] x8 : ffff8000116598f8 x7 : ffff8000116b18f8 x6 : 0000000000000001 [ 84.254991] x5 : 0000000000000001 x4 : 0000000000000001 x3 : 0000000000000000 [ 84.262111] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0008066f8000 [ 84.269230] Call trace: [ 84.271665] debug_dma_map_sg+0x304/0x390 [ 84.275660] dma_map_sg_attrs+0x70/0xb0 [ 84.279486] drm_gem_map_dma_buf+0x6c/0xf0 [drm] [ 84.284177] __map_dma_buf+0x28/0x80 [ 84.287742] dma_buf_map_attachment+0xe4/0x200 [ 84.292172] vb2_dc_map_dmabuf+0x3c/0x150 [ 84.296171] __prepare_dmabuf+0x1dc/0x514 [ 84.300168] __buf_prepare+0x1a0/0x25c [ 84.303903] vb2_core_qbuf+0x3d4/0x72c [ 84.307638] vb2_qbuf+0x9c/0xf4 [ 84.310767] vb2_ioctl_qbuf+0x68/0x7c [ 84.314416] v4l_qbuf+0x54/0x70 [ 84.317545] __video_do_ioctl+0x194/0x400 [ 84.321541] video_usercopy+0x19c/0x910 [ 84.325362] video_ioctl2+0x24/0x40 [ 84.328837] v4l2_ioctl+0x4c/0x70 [ 84.332141] __arm64_sys_ioctl+0xb4/0xfc [ 84.336053] invoke_syscall+0x50/0x120 [ 84.339791] el0_svc_common.constprop.0+0x68/0x104 [ 84.344569] do_el0_svc+0x30/0x9c [ 84.347872] el0_svc+0x2c/0x54 [ 84.350916] el0_sync_handler+0x1a8/0x1ac [ 84.354911] el0_sync+0x198/0x1c0 [ 84.358215] irq event stamp: 0 [ 84.361256] hardirqs last enabled at (0): [<0000000000000000>] 0x0 [ 84.367507] hardirqs last disabled at (0): [<ffff80001004fe2c>] copy_process+0x44c/0x1800 [ 84.375667] softirqs last enabled at (0): [<ffff80001004fe2c>] copy_process+0x44c/0x1800 [ 84.383824] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 84.390073] ---[ end trace af3448c784059129 ]---
Hi Pratyush, On 26/05/2021 18:23, Pratyush Yadav wrote: > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > capture over a CSI-2 bus. > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > called the SHIM layer. It takes in data from stream 0, repacks it, and > sends it to memory over PSI-L DMA. > > This driver acts as the "front end" to V4L2 client applications. It > implements the required ioctls and buffer operations, passes the > necessary calls on to the bridge, programs the SHIM layer, and performs > DMA via the dmaengine API to finally return the data to a buffer > supplied by the application. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> I noticed that my test app didn't work at all with this, and I also wasn't able to use v4l2-ctl to set the format. At least for my test app the problem was that this driver doesn't initialize the format at all. My app first calls VIDIOC_G_FMT with v4l2_format.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, then after the call modifies the fields it wants to change and calls VIDIOC_S_FMT. This failed, as G_FMT returned uninitialized fmt, i.e. type was 0, which my app didn't set again. I believe the driver should have an initial format, something that it will accept if an app calls G_FMT and then S_FMT. Tomi
On 26/05/2021 18:22, Pratyush Yadav wrote: > The Cadence DPHY can be used to receive image data over the CSI-2 > protocol. Add support for Rx mode. The programming sequence differs from > the Tx mode so it is added as a separate set of hooks to isolate the two > paths. > > The PHY is in Tx mode by default and it needs to be set in Rx mode by > setting the submode to PHY_MIPI_DPHY_SUBMODE_RX in the set_mode() > callback. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > (no changes since v1) > > drivers/phy/cadence/cdns-dphy.c | 237 ++++++++++++++++++++++++++++++++ > 1 file changed, 237 insertions(+) > > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c > index 7d5f7b333893..7bbca679e2bb 100644 > --- a/drivers/phy/cadence/cdns-dphy.c > +++ b/drivers/phy/cadence/cdns-dphy.c > @@ -1,11 +1,14 @@ > // SPDX-License-Identifier: GPL-2.0+ > /* > * Copyright: 2017-2018 Cadence Design Systems, Inc. > + * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com/ > */ > > #include <linux/bitops.h> > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/of_address.h> > #include <linux/of_device.h> > @@ -25,10 +28,14 @@ > #define DPHY_PMA_RCLK(reg) (0x600 + (reg)) > #define DPHY_PMA_RDATA(lane, reg) (0x700 + ((lane) * 0x100) + (reg)) > #define DPHY_PCS(reg) (0xb00 + (reg)) > +#define DPHY_ISO(reg) (0xc00 + (reg)) > > #define DPHY_CMN_SSM DPHY_PMA_CMN(0x20) > #define DPHY_CMN_SSM_EN BIT(0) > +#define DPHY_CMN_RX_BANDGAP_TIMER_MASK GENMASK(8, 1) > #define DPHY_CMN_TX_MODE_EN BIT(9) > +#define DPHY_CMN_RX_MODE_EN BIT(10) > +#define DPHY_CMN_RX_BANDGAP_TIMER 0x14 > > #define DPHY_CMN_PWM DPHY_PMA_CMN(0x40) > #define DPHY_CMN_PWM_DIV(x) ((x) << 20) > @@ -45,10 +52,27 @@ > #define DPHY_CMN_OPDIV_FROM_REG BIT(6) > #define DPHY_CMN_OPDIV(x) ((x) << 7) > > +#define DPHY_BAND_CFG DPHY_PCS(0x0) > +#define DPHY_BAND_CFG_LEFT_BAND GENMASK(4, 0) > +#define DPHY_BAND_CFG_RIGHT_BAND GENMASK(9, 5) > + > #define DPHY_PSM_CFG DPHY_PCS(0x4) > #define DPHY_PSM_CFG_FROM_REG BIT(0) > #define DPHY_PSM_CLK_DIV(x) ((x) << 1) > > +#define DPHY_POWER_ISLAND_EN_DATA DPHY_PCS(0x8) > +#define DPHY_POWER_ISLAND_EN_DATA_VAL 0xaaaaaaaa > +#define DPHY_POWER_ISLAND_EN_CLK DPHY_PCS(0xc) > +#define DPHY_POWER_ISLAND_EN_CLK_VAL 0xaa > + > +#define DPHY_ISO_CL_CTRL_L DPHY_ISO(0x10) > +#define DPHY_ISO_DL_CTRL_L0 DPHY_ISO(0x14) > +#define DPHY_ISO_DL_CTRL_L1 DPHY_ISO(0x20) > +#define DPHY_ISO_DL_CTRL_L2 DPHY_ISO(0x30) > +#define DPHY_ISO_DL_CTRL_L3 DPHY_ISO(0x3c) > +#define DPHY_ISO_LANE_READY_BIT 0 > +#define DPHY_ISO_LANE_READY_TIMEOUT_MS 100UL > + > #define DSI_HBP_FRAME_OVERHEAD 12 > #define DSI_HSA_FRAME_OVERHEAD 14 > #define DSI_HFP_FRAME_OVERHEAD 6 > @@ -57,6 +81,9 @@ > #define DSI_NULL_FRAME_OVERHEAD 6 > #define DSI_EOT_PKT_SIZE 4 > > +#define DPHY_LANES_MIN 1 > +#define DPHY_LANES_MAX 4 > + > struct cdns_dphy_cfg { > u8 pll_ipdiv; > u8 pll_opdiv; > @@ -312,6 +339,214 @@ static const struct cdns_dphy_ops tx_ref_dphy_ops = { > .set_psm_div = cdns_dphy_ref_set_psm_div, > }; > > +static int cdns_dphy_rx_power_on(struct cdns_dphy *dphy) > +{ > + /* Start RX state machine. */ > + writel(DPHY_CMN_SSM_EN | DPHY_CMN_RX_MODE_EN | > + FIELD_PREP(DPHY_CMN_RX_BANDGAP_TIMER_MASK, > + DPHY_CMN_RX_BANDGAP_TIMER), > + dphy->regs + DPHY_CMN_SSM); > + > + return 0; > +} > + > +static int cdns_dphy_rx_power_off(struct cdns_dphy *dphy) > +{ > + writel(0, dphy->regs + DPHY_CMN_SSM); > + > + return 0; > +} > + > +static int cdns_dphy_rx_get_band_ctrl(unsigned long hs_clk_rate) > +{ > + unsigned int rate = hs_clk_rate / 1000000UL; > + > + if (rate < 80 || rate >= 2500) > + return -EOPNOTSUPP; > + > + if (rate >= 80 && rate < 100) > + return 0; > + > + if (rate >= 100 && rate < 120) > + return 1; > + > + if (rate >= 120 && rate < 160) > + return 2; > + > + if (rate >= 160 && rate < 200) > + return 3; > + > + if (rate >= 200 && rate < 240) > + return 4; > + > + if (rate >= 240 && rate < 280) > + return 5; > + > + if (rate >= 280 && rate < 320) > + return 6; > + > + if (rate >= 320 && rate < 360) > + return 7; > + > + if (rate >= 360 && rate < 400) > + return 8; > + > + if (rate >= 400 && rate < 480) > + return 9; > + > + if (rate >= 480 && rate < 560) > + return 10; > + > + if (rate >= 560 && rate < 640) > + return 11; > + > + if (rate >= 640 && rate < 720) > + return 12; > + > + if (rate >= 720 && rate < 800) > + return 13; > + > + if (rate >= 800 && rate < 880) > + return 14; > + > + if (rate >= 880 && rate < 1040) > + return 15; > + > + if (rate >= 1040 && rate < 1200) > + return 16; > + > + if (rate >= 1200 && rate < 1350) > + return 17; > + > + if (rate >= 1350 && rate < 1500) > + return 18; > + > + if (rate >= 1500 && rate < 1750) > + return 19; > + > + if (rate >= 1750 && rate < 2000) > + return 20; > + > + if (rate >= 2000 && rate < 2250) > + return 21; > + > + if (rate >= 2250 && rate < 2500) > + return 22; > + All the above could be handled with a simple table and a for loop. > + /* Unreachable. */ > + WARN(1, "Reached unreachable code."); > + return -EINVAL; > +} > + > +static int cdns_dphy_rx_wait_for_bit(void __iomem *addr, unsigned int bit) > +{ > + u32 val; > + > + return readl_relaxed_poll_timeout(addr, val, val & BIT(bit), 10, > + DPHY_ISO_LANE_READY_TIMEOUT_MS * 1000); > +} > + > +static int cdns_dphy_rx_wait_lane_ready(struct cdns_dphy *dphy, int lanes) > +{ > + void __iomem *reg = dphy->regs; > + int ret; > + > + if (lanes < DPHY_LANES_MIN || lanes > DPHY_LANES_MAX) > + return -EINVAL; > + > + /* Clock lane */ > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_CL_CTRL_L, > + DPHY_ISO_LANE_READY_BIT); > + if (ret) > + return ret; > + > + /* Data lanes. Minimum one lane is mandatory. */ > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L0, > + DPHY_ISO_LANE_READY_BIT); > + if (ret) > + return ret; > + > + if (lanes < 2) > + return 0; > + > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L1, > + DPHY_ISO_LANE_READY_BIT); > + if (ret) > + return ret; > + > + if (lanes < 3) > + return 0; > + > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L2, > + DPHY_ISO_LANE_READY_BIT); > + if (ret) > + return ret; > + > + if (lanes < 4) > + return 0; > + > + ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L3, > + DPHY_ISO_LANE_READY_BIT); > + if (ret) > + return ret; > + This, too, could be handled with an array (for the regs) and a for loop. Tomi
On 26/05/2021 18:22, Pratyush Yadav wrote: > Calling s_power subdev callback is discouraged. Instead, the subdevs > should use runtime PM to control its power. Use runtime PM callbacks to > control sensor power. The pm counter is incremented when the stream is > started and decremented when the stream is stopped. > > Refactor s_stream() a bit to make this new control flow easier. Add a > helper to choose whether mipi or dvp set_stream needs to be called. The > logic flow is also changed to make it a bit clearer. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v2: > - New in v2. > > drivers/media/i2c/Kconfig | 2 +- > drivers/media/i2c/ov5640.c | 124 +++++++++++++++++++++++-------------- > 2 files changed, 77 insertions(+), 49 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 462c0e059754..5588fc1cc14a 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -914,7 +914,7 @@ config VIDEO_OV2740 > > config VIDEO_OV5640 > tristate "OmniVision OV5640 sensor support" > - depends on OF > + depends on OF && PM > depends on GPIOLIB && VIDEO_V4L2 && I2C > select MEDIA_CONTROLLER > select VIDEO_V4L2_SUBDEV_API > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5b9cc71df473..4ed5758e2398 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -15,6 +15,7 @@ > #include <linux/init.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/types.h> > @@ -238,8 +239,6 @@ struct ov5640_dev { > /* lock to protect all members below */ > struct mutex lock; > > - int power_count; > - > struct v4l2_mbus_framefmt fmt; > bool pending_fmt_change; > > @@ -1277,6 +1276,14 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > on ? 0x00 : 0x0f); > } > > +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) > +{ > + if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > + return ov5640_set_stream_mipi(sensor, on); > + else > + return ov5640_set_stream_dvp(sensor, on); > +} > + > static int ov5640_get_sysclk(struct ov5640_dev *sensor) > { > /* calculate sysclk */ > @@ -2155,37 +2162,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > > /* --------------- Subdev Operations --------------- */ > > -static int ov5640_s_power(struct v4l2_subdev *sd, int on) > -{ > - struct ov5640_dev *sensor = to_ov5640_dev(sd); > - int ret = 0; > - > - mutex_lock(&sensor->lock); > - > - /* > - * If the power count is modified from 0 to != 0 or from != 0 to 0, > - * update the power state. > - */ > - if (sensor->power_count == !on) { > - ret = ov5640_set_power(sensor, !!on); > - if (ret) > - goto out; > - } > - > - /* Update the power count. */ > - sensor->power_count += on ? 1 : -1; > - WARN_ON(sensor->power_count < 0); > -out: > - mutex_unlock(&sensor->lock); > - > - if (on && !ret && sensor->power_count == 1) { > - /* restore controls */ > - ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > - } > - > - return ret; > -} > - > static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > struct v4l2_fract *fi, > u32 width, u32 height) > @@ -2681,6 +2657,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + struct device *dev = &sensor->i2c_client->dev; > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > @@ -2690,7 +2667,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > * not apply any controls to H/W at this time. Instead > * the controls will be restored right after power-up. > */ > - if (sensor->power_count == 0) > + if (pm_runtime_suspended(dev)) > return 0; > > switch (ctrl->id) { > @@ -2939,39 +2916,56 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + struct device *dev = &sensor->i2c_client->dev; > int ret = 0; > > mutex_lock(&sensor->lock); > > - if (sensor->streaming == !enable) { > - if (enable && sensor->pending_mode_change) { > + if (sensor->streaming == enable) > + goto out; > + > + if (enable) { > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) { > + pm_runtime_put_noidle(dev); > + goto out; > + } There now seems to be a function to do the above steps: pm_runtime_resume_and_get. > + > + if (sensor->pending_mode_change) { > ret = ov5640_set_mode(sensor); > if (ret) > - goto out; > + goto put_pm; > } > > - if (enable && sensor->pending_fmt_change) { > + if (sensor->pending_fmt_change) { > ret = ov5640_set_framefmt(sensor, &sensor->fmt); > if (ret) > - goto out; > + goto put_pm; > sensor->pending_fmt_change = false; > } > > - if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > - ret = ov5640_set_stream_mipi(sensor, enable); > - else > - ret = ov5640_set_stream_dvp(sensor, enable); > + ret = ov5640_set_stream(sensor, enable); > + if (ret) > + goto put_pm; > + } else { > + ret = ov5640_set_stream(sensor, enable); Instead of using "enable" here (and in the enable path above), just use true/false. It'll be more readable. > + if (ret) > + goto out; > > - if (!ret) > - sensor->streaming = enable; > + pm_runtime_put(dev); > } > + > + sensor->streaming = enable; > + goto out; > + > +put_pm: > + pm_runtime_put(dev); > out: > mutex_unlock(&sensor->lock); > return ret; > } The flow in the above function is quite confusing. I think you should either 1) have a separate error paths via gotos and a return 0 before the error labels, or 2) common error and success path, without that final "goto out" you have above. Maybe if you move the code in the "if (enable) {} else {}" to the ov5640_set_stream(), the flow will be easier to manage. > > static const struct v4l2_subdev_core_ops ov5640_core_ops = { > - .s_power = ov5640_s_power, > .log_status = v4l2_ctrl_subdev_log_status, > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > @@ -3037,6 +3031,29 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor) > return ret; > } > > +static int ov5640_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > + > + return ov5640_set_power(sensor, false); > +} > + > +static int ov5640_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > + int ret = 0; > + > + ret = ov5640_set_power(sensor, true); > + if (ret) > + return ret; > + > + return __v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > +} > + > static int ov5640_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > @@ -3162,13 +3179,17 @@ static int ov5640_probe(struct i2c_client *client) > if (ret) > goto entity_cleanup; > > + pm_runtime_enable(dev); > + pm_runtime_set_suspended(dev); > + > ret = v4l2_async_register_subdev_sensor(&sensor->sd); > if (ret) > - goto free_ctrls; > + goto error_pm; > > return 0; > > -free_ctrls: > +error_pm: > + pm_runtime_disable(dev); The label style used here seems to be the "label-tells-what-will-be-done", so I think instead of "error_pm", it should be, perhaps, "pm_disable". > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > entity_cleanup: > media_entity_cleanup(&sensor->sd.entity); > @@ -3178,17 +3199,23 @@ static int ov5640_probe(struct i2c_client *client) > > static int ov5640_remove(struct i2c_client *client) > { > + struct device *dev = &client->dev; > struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > v4l2_async_unregister_subdev(&sensor->sd); > media_entity_cleanup(&sensor->sd.entity); > + pm_runtime_disable(dev); > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > mutex_destroy(&sensor->lock); > > return 0; > } > > +static const struct dev_pm_ops ov5640_pm_ops = { > + SET_RUNTIME_PM_OPS(ov5640_suspend, ov5640_resume, NULL) > +}; > + > static const struct i2c_device_id ov5640_id[] = { > {"ov5640", 0}, > {}, > @@ -3205,6 +3232,7 @@ static struct i2c_driver ov5640_i2c_driver = { > .driver = { > .name = "ov5640", > .of_match_table = ov5640_dt_ids, > + .pm = &ov5640_pm_ops, > }, > .id_table = ov5640_id, > .probe_new = ov5640_probe, >
On 26/05/2021 18:23, Pratyush Yadav wrote: > The devnode can be used by media-ctl and other userspace tools to > perform configurations on the subdev. Without it, media-ctl returns > ENOENT when setting format on the sensor subdev. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > (no changes since v1) > > drivers/media/platform/cadence/cdns-csi2rx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 1df21f462f3c..49bed63d5faa 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev) > csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) > csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, > csi2rx->pads); > I don't understand this one. There's nothing to configure in cdns-csi2rx from userspace, as far as I can see, so why is the dev node needed? And why would the lack of csi2rx dev node cause sensor subdev config to fail? Tomi
On 26/05/2021 18:22, Pratyush Yadav wrote: > Some platforms like TI's J721E can have the CSI2RX paired with an > external DPHY. Add support to enable and configure the DPHY using the > generic PHY framework. > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > along with the number of lanes. All other settings are left to their > default values. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v2: > - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making > calls to set PHY mode, etc. to make sure it is ready. > > drivers/media/platform/cadence/cdns-csi2rx.c | 158 +++++++++++++++++-- > 1 file changed, 148 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index c68a3eac62cd..459326de2eff 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -30,6 +30,12 @@ > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > +#define CSI2RX_DPHY_CL_RST BIT(16) > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > +#define CSI2RX_DPHY_CL_EN BIT(4) > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > + > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > @@ -54,6 +60,11 @@ enum csi2rx_pads { > CSI2RX_PAD_MAX, > }; > > +struct csi2rx_fmt { > + u32 code; > + u8 bpp; > +}; > + > struct csi2rx_priv { > struct device *dev; > unsigned int count; > @@ -85,6 +96,52 @@ struct csi2rx_priv { > int source_pad; > }; > > +static const struct csi2rx_fmt formats[] = { > + { > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > + .bpp = 16, > + }, > +}; > + > +static u8 csi2rx_get_bpp(u32 code) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].code == code) > + return formats[i].bpp; > + } > + > + return 0; > +} > + > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > +{ > + struct v4l2_ctrl *ctrl; > + > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > + V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > + csi2rx->source_subdev->name); > + return -EINVAL; > + } > + > + return v4l2_ctrl_g_ctrl_int64(ctrl); > +} > + > static inline > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > { > @@ -101,6 +158,66 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > } > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > +{ > + union phy_configure_opts opts = { }; > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > + struct v4l2_subdev_format sd_fmt; > + s64 pixel_rate; > + int ret; > + u8 bpp; > + bool got_pm = true; > + > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sd_fmt.pad = 0; > + > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > + &sd_fmt); > + if (ret) > + return ret; > + > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > + if (!bpp) > + return -EINVAL; > + > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > + if (pixel_rate < 0) > + return pixel_rate; > + > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > + csi2rx->num_lanes, cfg); > + if (ret) > + return ret; I guess the above code works for now, but with multiple streams it won't. There's no (single) pixel rate or bpp with multiple streams, and the link freq is the one that needs to be used. There's v4l2_get_link_freq() helper which makes it easier to support both pixel rate and link freq. Tomi
On 28/05/21 10:16AM, Tomi Valkeinen wrote: > On 26/05/2021 18:23, Pratyush Yadav wrote: > > The devnode can be used by media-ctl and other userspace tools to > > perform configurations on the subdev. Without it, media-ctl returns > > ENOENT when setting format on the sensor subdev. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > > > (no changes since v1) > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 1df21f462f3c..49bed63d5faa 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) > > csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, > > csi2rx->pads); > > > > I don't understand this one. There's nothing to configure in cdns-csi2rx > from userspace, as far as I can see, so why is the dev node needed? And why > would the lack of csi2rx dev node cause sensor subdev config to fail? Sensor config does not fail. But when I run media-ctl to set the format on /dev/media0, I get an error message that comes because the devnode for the bridge does not exist. I was not 100% sure about this patch but I figured if media-ctl expects it then it should be exposed. I don't mind dropping this patch. Just want to make sure what the right thing to do here is. Should every element of the pipeline have a devnode or not?
On 28/05/21 09:44AM, Tomi Valkeinen wrote: > On 26/05/2021 18:22, Pratyush Yadav wrote: > > Calling s_power subdev callback is discouraged. Instead, the subdevs > > should use runtime PM to control its power. Use runtime PM callbacks to > > control sensor power. The pm counter is incremented when the stream is > > started and decremented when the stream is stopped. > > > > Refactor s_stream() a bit to make this new control flow easier. Add a > > helper to choose whether mipi or dvp set_stream needs to be called. The > > logic flow is also changed to make it a bit clearer. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v2: > > - New in v2. > > > > drivers/media/i2c/Kconfig | 2 +- > > drivers/media/i2c/ov5640.c | 124 +++++++++++++++++++++++-------------- > > 2 files changed, 77 insertions(+), 49 deletions(-) > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 462c0e059754..5588fc1cc14a 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -914,7 +914,7 @@ config VIDEO_OV2740 > > config VIDEO_OV5640 > > tristate "OmniVision OV5640 sensor support" > > - depends on OF > > + depends on OF && PM > > depends on GPIOLIB && VIDEO_V4L2 && I2C > > select MEDIA_CONTROLLER > > select VIDEO_V4L2_SUBDEV_API > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 5b9cc71df473..4ed5758e2398 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -15,6 +15,7 @@ > > #include <linux/init.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > +#include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > @@ -238,8 +239,6 @@ struct ov5640_dev { > > /* lock to protect all members below */ > > struct mutex lock; > > - int power_count; > > - > > struct v4l2_mbus_framefmt fmt; > > bool pending_fmt_change; > > @@ -1277,6 +1276,14 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > > on ? 0x00 : 0x0f); > > } > > +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) > > +{ > > + if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > > + return ov5640_set_stream_mipi(sensor, on); > > + else > > + return ov5640_set_stream_dvp(sensor, on); > > +} > > + > > static int ov5640_get_sysclk(struct ov5640_dev *sensor) > > { > > /* calculate sysclk */ > > @@ -2155,37 +2162,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > > /* --------------- Subdev Operations --------------- */ > > -static int ov5640_s_power(struct v4l2_subdev *sd, int on) > > -{ > > - struct ov5640_dev *sensor = to_ov5640_dev(sd); > > - int ret = 0; > > - > > - mutex_lock(&sensor->lock); > > - > > - /* > > - * If the power count is modified from 0 to != 0 or from != 0 to 0, > > - * update the power state. > > - */ > > - if (sensor->power_count == !on) { > > - ret = ov5640_set_power(sensor, !!on); > > - if (ret) > > - goto out; > > - } > > - > > - /* Update the power count. */ > > - sensor->power_count += on ? 1 : -1; > > - WARN_ON(sensor->power_count < 0); > > -out: > > - mutex_unlock(&sensor->lock); > > - > > - if (on && !ret && sensor->power_count == 1) { > > - /* restore controls */ > > - ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > > - } > > - > > - return ret; > > -} > > - > > static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > > struct v4l2_fract *fi, > > u32 width, u32 height) > > @@ -2681,6 +2657,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + struct device *dev = &sensor->i2c_client->dev; > > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > > @@ -2690,7 +2667,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > * not apply any controls to H/W at this time. Instead > > * the controls will be restored right after power-up. > > */ > > - if (sensor->power_count == 0) > > + if (pm_runtime_suspended(dev)) > > return 0; > > switch (ctrl->id) { > > @@ -2939,39 +2916,56 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + struct device *dev = &sensor->i2c_client->dev; > > int ret = 0; > > mutex_lock(&sensor->lock); > > - if (sensor->streaming == !enable) { > > - if (enable && sensor->pending_mode_change) { > > + if (sensor->streaming == enable) > > + goto out; > > + > > + if (enable) { > > + ret = pm_runtime_get_sync(dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(dev); > > + goto out; > > + } > > There now seems to be a function to do the above steps: > pm_runtime_resume_and_get. Ok. > > > + > > + if (sensor->pending_mode_change) { > > ret = ov5640_set_mode(sensor); > > if (ret) > > - goto out; > > + goto put_pm; > > } > > - if (enable && sensor->pending_fmt_change) { > > + if (sensor->pending_fmt_change) { > > ret = ov5640_set_framefmt(sensor, &sensor->fmt); > > if (ret) > > - goto out; > > + goto put_pm; > > sensor->pending_fmt_change = false; > > } > > - if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > > - ret = ov5640_set_stream_mipi(sensor, enable); > > - else > > - ret = ov5640_set_stream_dvp(sensor, enable); > > + ret = ov5640_set_stream(sensor, enable); > > + if (ret) > > + goto put_pm; > > + } else { > > + ret = ov5640_set_stream(sensor, enable); > > Instead of using "enable" here (and in the enable path above), just use > true/false. It'll be more readable. Ok. > > > + if (ret) > > + goto out; > > - if (!ret) > > - sensor->streaming = enable; > > + pm_runtime_put(dev); > > } > > + > > + sensor->streaming = enable; > > + goto out; > > + > > +put_pm: > > + pm_runtime_put(dev); > > out: > > mutex_unlock(&sensor->lock); > > return ret; > > } > > The flow in the above function is quite confusing. I think you should either > 1) have a separate error paths via gotos and a return 0 before the error > labels, or 2) common error and success path, without that final "goto out" > you have above. > > Maybe if you move the code in the "if (enable) {} else {}" to the > ov5640_set_stream(), the flow will be easier to manage. Ok. I'll take a look and see what works better. > > > static const struct v4l2_subdev_core_ops ov5640_core_ops = { > > - .s_power = ov5640_s_power, > > .log_status = v4l2_ctrl_subdev_log_status, > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > @@ -3037,6 +3031,29 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor) > > return ret; > > } > > +static int ov5640_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > > + > > + return ov5640_set_power(sensor, false); > > +} > > + > > +static int ov5640_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > > + int ret = 0; > > + > > + ret = ov5640_set_power(sensor, true); > > + if (ret) > > + return ret; > > + > > + return __v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > > +} > > + > > static int ov5640_probe(struct i2c_client *client) > > { > > struct device *dev = &client->dev; > > @@ -3162,13 +3179,17 @@ static int ov5640_probe(struct i2c_client *client) > > if (ret) > > goto entity_cleanup; > > + pm_runtime_enable(dev); > > + pm_runtime_set_suspended(dev); > > + > > ret = v4l2_async_register_subdev_sensor(&sensor->sd); > > if (ret) > > - goto free_ctrls; > > + goto error_pm; > > return 0; > > -free_ctrls: > > +error_pm: > > + pm_runtime_disable(dev); > > The label style used here seems to be the "label-tells-what-will-be-done", > so I think instead of "error_pm", it should be, perhaps, "pm_disable". Ok. > > > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > > entity_cleanup: > > media_entity_cleanup(&sensor->sd.entity); > > @@ -3178,17 +3199,23 @@ static int ov5640_probe(struct i2c_client *client) > > static int ov5640_remove(struct i2c_client *client) > > { > > + struct device *dev = &client->dev; > > struct v4l2_subdev *sd = i2c_get_clientdata(client); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > v4l2_async_unregister_subdev(&sensor->sd); > > media_entity_cleanup(&sensor->sd.entity); > > + pm_runtime_disable(dev); > > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > > mutex_destroy(&sensor->lock); > > return 0; > > } > > +static const struct dev_pm_ops ov5640_pm_ops = { > > + SET_RUNTIME_PM_OPS(ov5640_suspend, ov5640_resume, NULL) > > +}; > > + > > static const struct i2c_device_id ov5640_id[] = { > > {"ov5640", 0}, > > {}, > > @@ -3205,6 +3232,7 @@ static struct i2c_driver ov5640_i2c_driver = { > > .driver = { > > .name = "ov5640", > > .of_match_table = ov5640_dt_ids, > > + .pm = &ov5640_pm_ops, > > }, > > .id_table = ov5640_id, > > .probe_new = ov5640_probe, > > >
On 28/05/21 10:23AM, Tomi Valkeinen wrote: > On 26/05/2021 18:22, Pratyush Yadav wrote: > > Some platforms like TI's J721E can have the CSI2RX paired with an > > external DPHY. Add support to enable and configure the DPHY using the > > generic PHY framework. > > > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > > along with the number of lanes. All other settings are left to their > > default values. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v2: > > - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making > > calls to set PHY mode, etc. to make sure it is ready. > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 158 +++++++++++++++++-- > > 1 file changed, 148 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index c68a3eac62cd..459326de2eff 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -30,6 +30,12 @@ > > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > > +#define CSI2RX_DPHY_CL_RST BIT(16) > > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > > +#define CSI2RX_DPHY_CL_EN BIT(4) > > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > > + > > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > > @@ -54,6 +60,11 @@ enum csi2rx_pads { > > CSI2RX_PAD_MAX, > > }; > > +struct csi2rx_fmt { > > + u32 code; > > + u8 bpp; > > +}; > > + > > struct csi2rx_priv { > > struct device *dev; > > unsigned int count; > > @@ -85,6 +96,52 @@ struct csi2rx_priv { > > int source_pad; > > }; > > +static const struct csi2rx_fmt formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + .bpp = 16, > > + }, > > +}; > > + > > +static u8 csi2rx_get_bpp(u32 code) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > + if (formats[i].code == code) > > + return formats[i].bpp; > > + } > > + > > + return 0; > > +} > > + > > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > > +{ > > + struct v4l2_ctrl *ctrl; > > + > > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > > + V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > > + csi2rx->source_subdev->name); > > + return -EINVAL; > > + } > > + > > + return v4l2_ctrl_g_ctrl_int64(ctrl); > > +} > > + > > static inline > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > { > > @@ -101,6 +158,66 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > > } > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > > +{ > > + union phy_configure_opts opts = { }; > > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > > + struct v4l2_subdev_format sd_fmt; > > + s64 pixel_rate; > > + int ret; > > + u8 bpp; > > + bool got_pm = true; > > + > > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + sd_fmt.pad = 0; > > + > > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > > + &sd_fmt); > > + if (ret) > > + return ret; > > + > > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > > + if (!bpp) > > + return -EINVAL; > > + > > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > > + if (pixel_rate < 0) > > + return pixel_rate; > > + > > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > > + csi2rx->num_lanes, cfg); > > + if (ret) > > + return ret; > > I guess the above code works for now, but with multiple streams it won't. > There's no (single) pixel rate or bpp with multiple streams, and the link > freq is the one that needs to be used. There's v4l2_get_link_freq() helper > which makes it easier to support both pixel rate and link freq. Ok. I will use it instead. The less changes needed when adding multistream support the better.
On 28/05/2021 10:24, Pratyush Yadav wrote: > On 28/05/21 10:16AM, Tomi Valkeinen wrote: >> On 26/05/2021 18:23, Pratyush Yadav wrote: >>> The devnode can be used by media-ctl and other userspace tools to >>> perform configurations on the subdev. Without it, media-ctl returns >>> ENOENT when setting format on the sensor subdev. >>> >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >>> --- >>> >>> (no changes since v1) >>> >>> drivers/media/platform/cadence/cdns-csi2rx.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c >>> index 1df21f462f3c..49bed63d5faa 100644 >>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c >>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c >>> @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev) >>> csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; >>> for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) >>> csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; >>> + csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>> ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, >>> csi2rx->pads); >>> >> >> I don't understand this one. There's nothing to configure in cdns-csi2rx >> from userspace, as far as I can see, so why is the dev node needed? And why >> would the lack of csi2rx dev node cause sensor subdev config to fail? > > Sensor config does not fail. But when I run media-ctl to set the format > on /dev/media0, I get an error message that comes because the devnode > for the bridge does not exist. I was not 100% sure about this patch but > I figured if media-ctl expects it then it should be exposed. > > I don't mind dropping this patch. Just want to make sure what the right > thing to do here is. Should every element of the pipeline have a devnode > or not? Tbh, I don't know. But I don't see why they should have. Also, my test works fine if I remove the devnode here. What media-ctl parameters did you use which fails? Tomi
On 26/05/2021 18:23, Pratyush Yadav wrote: > The ti-vpe/ sub-directory does not only contain the VPE-specific things. > It also contains the CAL driver, which is a completely different > subsystem. This is also not a good place to add new drivers for other TI > platforms since they will all get mixed up. > > Separate the VPE and CAL parts into different sub-directories and rename > the ti-vpe/ sub-directory to ti/. This is now the place where new TI > platform drivers can be added. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > Compile tested only. There should be no functional change. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
On 26/05/2021 18:23, Pratyush Yadav wrote: > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > have up to 32 threads but the current driver only supports using one. So > add an entry for that one thread. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v2: > - Add all 64 threads, instead of having only the one thread being > currently used by the driver. How many threads CSI2RX have? 32 (as per commit message) or 64? If I recall right, it is 32. > > drivers/dma/ti/k3-psil-j721e.c | 73 ++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > index 7580870ed746..34e3fc565a37 100644 > --- a/drivers/dma/ti/k3-psil-j721e.c > +++ b/drivers/dma/ti/k3-psil-j721e.c > @@ -58,6 +58,14 @@ > }, \ > } > > +#define PSIL_CSI2RX(x) \ > + { \ > + .thread_id = x, \ > + .ep_config = { \ > + .ep_type = PSIL_EP_NATIVE, \ > + }, \ > + } > + > /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > static struct psil_ep j721e_src_ep_map[] = { > /* SA2UL */ > @@ -138,6 +146,71 @@ static struct psil_ep j721e_src_ep_map[] = { > PSIL_PDMA_XY_PKT(0x4707), > PSIL_PDMA_XY_PKT(0x4708), > PSIL_PDMA_XY_PKT(0x4709), > + /* CSI2RX */ > + PSIL_CSI2RX(0x4940), > + PSIL_CSI2RX(0x4941), > + PSIL_CSI2RX(0x4942), > + PSIL_CSI2RX(0x4943), > + PSIL_CSI2RX(0x4944), > + PSIL_CSI2RX(0x4945), > + PSIL_CSI2RX(0x4946), > + PSIL_CSI2RX(0x4947), > + PSIL_CSI2RX(0x4948), > + PSIL_CSI2RX(0x4949), > + PSIL_CSI2RX(0x494a), > + PSIL_CSI2RX(0x494b), > + PSIL_CSI2RX(0x494c), > + PSIL_CSI2RX(0x494d), > + PSIL_CSI2RX(0x494e), > + PSIL_CSI2RX(0x494f), > + PSIL_CSI2RX(0x4950), > + PSIL_CSI2RX(0x4951), > + PSIL_CSI2RX(0x4952), > + PSIL_CSI2RX(0x4953), > + PSIL_CSI2RX(0x4954), > + PSIL_CSI2RX(0x4955), > + PSIL_CSI2RX(0x4956), > + PSIL_CSI2RX(0x4957), > + PSIL_CSI2RX(0x4958), > + PSIL_CSI2RX(0x4959), > + PSIL_CSI2RX(0x495a), > + PSIL_CSI2RX(0x495b), > + PSIL_CSI2RX(0x495c), > + PSIL_CSI2RX(0x495d), > + PSIL_CSI2RX(0x495e), > + PSIL_CSI2RX(0x495f), > + PSIL_CSI2RX(0x4960), > + PSIL_CSI2RX(0x4961), > + PSIL_CSI2RX(0x4962), > + PSIL_CSI2RX(0x4963), > + PSIL_CSI2RX(0x4964), > + PSIL_CSI2RX(0x4965), > + PSIL_CSI2RX(0x4966), > + PSIL_CSI2RX(0x4967), > + PSIL_CSI2RX(0x4968), > + PSIL_CSI2RX(0x4969), > + PSIL_CSI2RX(0x496a), > + PSIL_CSI2RX(0x496b), > + PSIL_CSI2RX(0x496c), > + PSIL_CSI2RX(0x496d), > + PSIL_CSI2RX(0x496e), > + PSIL_CSI2RX(0x496f), > + PSIL_CSI2RX(0x4970), > + PSIL_CSI2RX(0x4971), > + PSIL_CSI2RX(0x4972), > + PSIL_CSI2RX(0x4973), > + PSIL_CSI2RX(0x4974), > + PSIL_CSI2RX(0x4975), > + PSIL_CSI2RX(0x4976), > + PSIL_CSI2RX(0x4977), > + PSIL_CSI2RX(0x4978), > + PSIL_CSI2RX(0x4979), > + PSIL_CSI2RX(0x497a), > + PSIL_CSI2RX(0x497b), > + PSIL_CSI2RX(0x497c), > + PSIL_CSI2RX(0x497d), > + PSIL_CSI2RX(0x497e), > + PSIL_CSI2RX(0x497f), > /* CPSW9 */ > PSIL_ETHERNET(0x4a00), > /* CPSW0 */ >
On 31/05/21 09:51AM, Péter Ujfalusi wrote: > > > On 26/05/2021 18:23, Pratyush Yadav wrote: > > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > have up to 32 threads but the current driver only supports using one. So > > add an entry for that one thread. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v2: > > - Add all 64 threads, instead of having only the one thread being > > currently used by the driver. > > How many threads CSI2RX have? 32 (as per commit message) or 64? If I > recall right, it is 32. Ah, sorry I forgot to update the commit message. Each instance of CSI2RX has 32 threads, and J721E has 2 instances. So 64 threads total.
On 28/05/21 10:35AM, Tomi Valkeinen wrote: > On 28/05/2021 10:24, Pratyush Yadav wrote: > > On 28/05/21 10:16AM, Tomi Valkeinen wrote: > > > On 26/05/2021 18:23, Pratyush Yadav wrote: > > > > The devnode can be used by media-ctl and other userspace tools to > > > > perform configurations on the subdev. Without it, media-ctl returns > > > > ENOENT when setting format on the sensor subdev. > > > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > > > > > (no changes since v1) > > > > > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > > > index 1df21f462f3c..49bed63d5faa 100644 > > > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > > > @@ -613,6 +613,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > > > csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > > > for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) > > > > csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > > > + csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > > > ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, > > > > csi2rx->pads); > > > > > > > > > > I don't understand this one. There's nothing to configure in cdns-csi2rx > > > from userspace, as far as I can see, so why is the dev node needed? And why > > > would the lack of csi2rx dev node cause sensor subdev config to fail? > > > > Sensor config does not fail. But when I run media-ctl to set the format > > on /dev/media0, I get an error message that comes because the devnode > > for the bridge does not exist. I was not 100% sure about this patch but > > I figured if media-ctl expects it then it should be exposed. > > > > I don't mind dropping this patch. Just want to make sure what the right > > thing to do here is. Should every element of the pipeline have a devnode > > or not? > > Tbh, I don't know. But I don't see why they should have. Also, my test works > fine if I remove the devnode here. > > What media-ctl parameters did you use which fails? Media controller topology: root@j7-evm:~# media-ctl -p Media controller API version 5.13.0 Media device information ------------------------ driver j721e-csi2rx model TI-CSI2RX serial bus info platform:4500000.ticsi2rx hw revision 0x1 driver version 5.13.0 Device topology - entity 1: cdns_csi2rx.4504000.csi-bridge (5 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 pad0: Sink <- "ov5640 9-003c":0 [ENABLED,IMMUTABLE] pad1: Source -> "j721e-csi2rx":0 [ENABLED,IMMUTABLE] pad2: Source pad3: Source pad4: Source - entity 7: ov5640 9-003c (1 pad, 1 link) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev1 pad0: Source [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] -> "cdns_csi2rx.4504000.csi-bridge":0 [ENABLED,IMMUTABLE] - entity 11: j721e-csi2rx (1 pad, 1 link) type Node subtype V4L flags 0 device node name /dev/video0 pad0: Sink <- "cdns_csi2rx.4504000.csi-bridge":1 [ENABLED,IMMUTABLE] Setting resolution on the sensor (with cdns-csi2rx devnode present): root@j7-evm:~# media-ctl --set-v4l2 '"ov5640 9-003c":0 [fmt:UYVY8_2X8/1920x1080@1/30]' The above command works. Setting resolution on the sensor (without the devnode present): root@j7-evm:~# media-ctl --set-v4l2 '"ov5640 9-003c":0 [fmt:UYVY8_2X8/1920x1080@1/30]' Unable to setup formats: No such file or directory (2) This sets the format correctly on the sensor but fails when it tried to set it on the bridge.
On 27/05/21 04:29PM, Tomi Valkeinen wrote: > Hi Pratyush, > > On 26/05/2021 18:23, Pratyush Yadav wrote: > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > capture over a CSI-2 bus. > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > sends it to memory over PSI-L DMA. > > > > This driver acts as the "front end" to V4L2 client applications. It > > implements the required ioctls and buffer operations, passes the > > necessary calls on to the bridge, programs the SHIM layer, and performs > > DMA via the dmaengine API to finally return the data to a buffer > > supplied by the application. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > I noticed that my test app didn't work at all with this, and I also wasn't > able to use v4l2-ctl to set the format. I have not used v4l2-ctl, but I can see yavta works fine. What command did you use for setting format via v4l2-ctl? > > At least for my test app the problem was that this driver doesn't initialize > the format at all. My app first calls VIDIOC_G_FMT with v4l2_format.type == > V4L2_BUF_TYPE_VIDEO_CAPTURE, then after the call modifies the fields it > wants to change and calls VIDIOC_S_FMT. This failed, as G_FMT returned > uninitialized fmt, i.e. type was 0, which my app didn't set again. > > I believe the driver should have an initial format, something that it will > accept if an app calls G_FMT and then S_FMT. Right. This is a bug. The question is what should the initial format be? It is more or less arbitrary since there is no configuration made yet and we don't know what the camera can or will send. So for example, what if I use UYVY 640x480? The camera might not support it at all. Is it still OK to have it as the default?
On 27/05/21 04:23PM, Tomi Valkeinen wrote: > Hi Pratyush, > > On 26/05/2021 18:22, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > I also see this after a few captures: Can you share the application/command you are using to test? I used yavta to test and didn't see any problems after leaving the stream on for around 10 minutes. > > [ 84.115503] ------------[ cut here ]------------ > [ 84.120144] DMA-API: ti-udma 31150000.dma-controller: mapping sg segment longer than device claims to support [len=1900544] [max=65536] > [ 84.132376] WARNING: CPU: 1 PID: 594 at kernel/dma/debug.c:1172 debug_dma_map_sg+0x304/0x390 > [ 84.140804] Modules linked in: ov5640 ti_cal j721e_csi2rx cdns_csi2rx cdns_dphy v4l2_fwnode tidss ti_tfp410 tc358767 display_connector cdns_mhdp8546 panel_simple drm_kms_helper d > rm drm_panel_orientation_quirks cfbfillrect cfbimgblt cfbcopyarea phy_j721e_wiz phy_cadence_torrent > [ 84.165298] CPU: 1 PID: 594 Comm: cam-mplex.py Not tainted 5.13.0-rc1-00206-g98bb91e95a28-dirty #5 > [ 84.174236] Hardware name: Texas Instruments K3 J721E SoC (DT) > [ 84.180051] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > [ 84.186041] pc : debug_dma_map_sg+0x304/0x390 > [ 84.190383] lr : debug_dma_map_sg+0x304/0x390 > [ 84.194725] sp : ffff800014d0f730 > [ 84.198026] x29: ffff800014d0f730 x28: ffff000801544680 x27: ffffffffffffffff > [ 84.205148] x26: 0000000000000000 x25: 0000000000000002 x24: 0000000000000001 > [ 84.212269] x23: ffff80001163abe0 x22: 0000000000000000 x21: 0000000000000001 > [ 84.219390] x20: ffff000801fa3010 x19: ffff0008075c7580 x18: 0000000000000000 > [ 84.226510] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000030 > [ 84.233630] x14: 6e61687420726567 x13: 6e6f6c20746e656d x12: 7420736d69616c63 > [ 84.240751] x11: 2065636976656420 x10: ffff8000116b18f8 x9 : ffff8000100eb920 > [ 84.247871] x8 : ffff8000116598f8 x7 : ffff8000116b18f8 x6 : 0000000000000001 > [ 84.254991] x5 : 0000000000000001 x4 : 0000000000000001 x3 : 0000000000000000 > [ 84.262111] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0008066f8000 > [ 84.269230] Call trace: > [ 84.271665] debug_dma_map_sg+0x304/0x390 > [ 84.275660] dma_map_sg_attrs+0x70/0xb0 > [ 84.279486] drm_gem_map_dma_buf+0x6c/0xf0 [drm] > [ 84.284177] __map_dma_buf+0x28/0x80 > [ 84.287742] dma_buf_map_attachment+0xe4/0x200 > [ 84.292172] vb2_dc_map_dmabuf+0x3c/0x150 > [ 84.296171] __prepare_dmabuf+0x1dc/0x514 > [ 84.300168] __buf_prepare+0x1a0/0x25c > [ 84.303903] vb2_core_qbuf+0x3d4/0x72c > [ 84.307638] vb2_qbuf+0x9c/0xf4 > [ 84.310767] vb2_ioctl_qbuf+0x68/0x7c > [ 84.314416] v4l_qbuf+0x54/0x70 > [ 84.317545] __video_do_ioctl+0x194/0x400 > [ 84.321541] video_usercopy+0x19c/0x910 > [ 84.325362] video_ioctl2+0x24/0x40 > [ 84.328837] v4l2_ioctl+0x4c/0x70 > [ 84.332141] __arm64_sys_ioctl+0xb4/0xfc > [ 84.336053] invoke_syscall+0x50/0x120 > [ 84.339791] el0_svc_common.constprop.0+0x68/0x104 > [ 84.344569] do_el0_svc+0x30/0x9c > [ 84.347872] el0_svc+0x2c/0x54 > [ 84.350916] el0_sync_handler+0x1a8/0x1ac > [ 84.354911] el0_sync+0x198/0x1c0 > [ 84.358215] irq event stamp: 0 > [ 84.361256] hardirqs last enabled at (0): [<0000000000000000>] 0x0 > [ 84.367507] hardirqs last disabled at (0): [<ffff80001004fe2c>] copy_process+0x44c/0x1800 > [ 84.375667] softirqs last enabled at (0): [<ffff80001004fe2c>] copy_process+0x44c/0x1800 > [ 84.383824] softirqs last disabled at (0): [<0000000000000000>] 0x0 > [ 84.390073] ---[ end trace af3448c784059129 ]---
On 27/05/21 03:42PM, Tomi Valkeinen wrote: > Hi Pratyush, > > On 26/05/2021 18:22, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > I'm having some trouble unloading and reloading the modules: > > rmmod ti_cal > rmmod j721e_csi2rx > rmmod cdns_csi2rx > rmmod cdns_dphy > rmmod ov5640 I did some basic module insertion/removal testing but I didn't try removing the sensor module. I will check and see what the problem is. > [ 37.943128] ------------[ cut here ]------------ > [ 37.947752] WARNING: CPU: 1 PID: 628 at drivers/media/v4l2-core/v4l2-ctrls-core.c:1807 __v4l2_ctrl_handler_setup+0x15c/0x170 > [ 37.958963] Modules linked in: ov5640(-) v4l2_fwnode tidss ti_tfp410 tc358767 display_connector cdns_mhdp8546 panel_simple drm_kms_helper drm drm_panel_orientation_quirks cfbfill > rect cfbimgblt cfbcopyarea phy_j721e_wiz phy_cadence_torrent [last unloaded: cdns_dphy] > [ 37.982455] CPU: 1 PID: 628 Comm: rmmod Not tainted 5.13.0-rc1-00205-g93acc23badc8 #3 > [ 37.990271] Hardware name: Texas Instruments K3 J721E SoC (DT) > [ 37.996090] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > [ 38.002085] pc : __v4l2_ctrl_handler_setup+0x15c/0x170 > [ 38.007214] lr : __v4l2_ctrl_handler_setup+0x158/0x170 > [ 38.012343] sp : ffff80001476fae0 > [ 38.015648] x29: ffff80001476fae0 x28: ffff000806780000 x27: 0000000000000000 > [ 38.022781] x26: ffff00080300b448 x25: ffff000804ad4ac0 x24: 0000000000000001 > [ 38.029912] x23: 0000000000000000 x22: 0000000000000005 x21: ffff000801fa7880 > [ 38.037043] x20: ffff000801fa7888 x19: ffff000801fa7ba8 x18: 0000000000000000 > [ 38.044173] x17: 0000000000000000 x16: 0000000000000010 x15: 0000000000000001 > [ 38.051305] x14: 000000000000003b x13: 00000000aaaaaaab x12: ffff800011a915b8 > [ 38.058436] x11: 00000000000c001c x10: 000000008260a2b7 x9 : ffff800009362d9c > [ 38.065566] x8 : ffff800011887100 x7 : 0000000000000000 x6 : 0000000000000001 > [ 38.072698] x5 : 0000000000000001 x4 : 0000000000000001 x3 : ffff800011260000 > [ 38.079829] x2 : 00000000000000c0 x1 : 00000000000000c0 x0 : 0000000000000000 > [ 38.086960] Call trace: > [ 38.089399] __v4l2_ctrl_handler_setup+0x15c/0x170 > [ 38.094181] ov5640_resume+0x1fc/0x270 [ov5640] > [ 38.098709] __rpm_callback+0x98/0x160 > [ 38.102452] rpm_callback+0x2c/0x90 > [ 38.105934] rpm_resume+0x45c/0x6f4 > [ 38.109415] __pm_runtime_resume+0x54/0xc0 > [ 38.113503] __device_release_driver+0x40/0x240 > [ 38.118025] driver_detach+0xd0/0x160 > [ 38.121680] bus_remove_driver+0x68/0xe0 > [ 38.125595] driver_unregister+0x3c/0x6c > [ 38.129509] i2c_del_driver+0x64/0xb0 > [ 38.133166] ov5640_i2c_driver_exit+0x1c/0xc948 [ov5640] > [ 38.138469] __arm64_sys_delete_module+0x1b0/0x27c > [ 38.143251] invoke_syscall+0x50/0x120 > [ 38.146995] el0_svc_common.constprop.0+0x68/0x104 > [ 38.151777] do_el0_svc+0x30/0x9c > [ 38.155086] el0_svc+0x2c/0x54 > [ 38.158135] el0_sync_handler+0x1a8/0x1ac > [ 38.162136] el0_sync+0x198/0x1c0 > [ 38.165444] irq event stamp: 11302 > [ 38.168837] hardirqs last enabled at (11301): [<ffff800010bf4e40>] _raw_spin_unlock_irq+0x50/0xa0 > [ 38.177781] hardirqs last disabled at (11302): [<ffff800010be7a64>] el1_dbg+0x24/0xa0 > [ 38.185595] softirqs last enabled at (10378): [<ffff800010010ba0>] __do_softirq+0x500/0x6bc > [ 38.194017] softirqs last disabled at (10373): [<ffff80001005d4c4>] __irq_exit_rcu+0x1d4/0x1e0 > [ 38.202614] ---[ end trace 7037324a951cb149 ]--- > rmmod v4l2_fwnode > insmod /root/nfs/work/linux/drivers/media/v4l2-core/v4l2-fwnode.ko > insmod /root/nfs/work/linux/drivers/phy/cadence/cdns-dphy.ko > insmod /root/nfs/work/linux/drivers/media/platform/cadence/cdns-csi2rx.ko > ERROR: Unhandled External Abort received on 0x80000001 from S-EL1 > ERROR: exception reason=0 syndrome=0xbf000000 > Unhandled Exception from EL1 > x0 = 0x0000000000000000 > x1 = 0xffff000804d59800 > x2 = 0xffff8000146c4000 > x3 = 0xffff800011260000 > x4 = 0x0000000000000001 > x5 = 0x0000000000000001 > x6 = 0x0000000000000001 > x7 = 0x0000000000000000 > x8 = 0xffff800011887100 > x9 = 0xffff800010bf5190 > x10 = 0x000000008260a2b7 > x11 = 0x00000000000c821d > x12 = 0xffff800011a915b8 > x13 = 0x0000000000000001 > x14 = 0x0000000000000000 > x15 = 0x0000000000000020 > x16 = 0x0000000000000000 > x17 = 0x0000000000000000 > x18 = 0x00000000fffffffb > x19 = 0xffff000806d44c00 > x20 = 0x0000000000000000 > x21 = 0xffff800009280058 > x22 = 0xffff00080583c810 > x23 = 0xffff00080583c800 > x24 = 0xffff800009280058 > x25 = 0x0000000000000047 > x26 = 0xffff8000116d71d8 > x27 = 0xffff800009280350 > x28 = 0xffff800009280148 > x29 = 0xffff80001432f850 > x30 = 0xffff8000092506b8 > scr_el3 = 0x000000000000073d > sctlr_el3 = 0x0000000030cd183f > cptr_el3 = 0x0000000000000000 > tcr_el3 = 0x0000000080803520 > daif = 0x00000000000002c0 > mair_el3 = 0x00000000004404ff > spsr_el3 = 0x0000000000000005 > elr_el3 = 0xffff80000925043c > ttbr0_el3 = 0x0000000070010b00 > esr_el3 = 0x00000000bf000000 > far_el3 = 0x0000000000000000 > spsr_el1 = 0x0000000060000005 > elr_el1 = 0xffff800010be8cb0 > spsr_abt = 0x0000000000000000 > spsr_und = 0x0000000000000000 > spsr_irq = 0x0000000000000000 > spsr_fiq = 0x0000000000000000 > sctlr_el1 = 0x0000000034d4d91d > actlr_el1 = 0x0000000000000000 > cpacr_el1 = 0x0000000000300000 > csselr_el1 = 0x0000000000000000 > sp_el1 = 0xffff80001432f850 > esr_el1 = 0x0000000056000000 > ttbr0_el1 = 0x0000000882773200 > ttbr1_el1 = 0x06d8000083180000 > mair_el1 = 0x000c0400bb44ffff > amair_el1 = 0x0000000000000000 > tcr_el1 = 0x00000034f5d07590 > tpidr_el1 = 0xffff80086e790000 > tpidr_el0 = 0x0000ffff895c6910 > tpidrro_el0 = 0x0000000000000000 > par_el1 = 0x0000000000000000 > mpidr_el1 = 0x0000000080000001 > afsr0_el1 = 0x0000000000000000 > afsr1_el1 = 0x0000000000000000 > contextidr_el1 = 0x0000000000000000 > vbar_el1 = 0xffff800010011000 > cntp_ctl_el0 = 0x0000000000000005 > cntp_cval_el0 = 0x000000023f77b7a1 > cntv_ctl_el0 = 0x0000000000000000 > cntv_cval_el0 = 0x0000000000000000 > cntkctl_el1 = 0x00000000000000d6 > sp_el0 = 0x000000007000abd0 > isr_el1 = 0x0000000000000040 > dacr32_el2 = 0x0000000000000000 > ifsr32_el2 = 0x0000000000000000 > cpuectlr_el1 = 0x0000001b00000040 > cpumerrsr_el1 = 0x0000000000000000 > l2merrsr_el1 = 0x0000000000000000
On 03/06/2021 15:52, Pratyush Yadav wrote: > On 27/05/21 04:23PM, Tomi Valkeinen wrote: >> Hi Pratyush, >> >> On 26/05/2021 18:22, Pratyush Yadav wrote: >>> Hi, >>> >>> This series adds support for CSI2 capture on J721E. It includes some >>> fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY >>> driver, and finally adds the TI CSI2RX wrapper driver. >>> >>> Tested on TI's J721E with OV5640 sensor. >> >> I also see this after a few captures: > > Can you share the application/command you are using to test? I used > yavta to test and didn't see any problems after leaving the stream on > for around 10 minutes. You need to have CONFIG_DMA_API_DEBUG enabled. I think that's not enabled by default on TI configs. Tomi
On 03/06/2021 15:49, Pratyush Yadav wrote: > On 27/05/21 04:29PM, Tomi Valkeinen wrote: >> Hi Pratyush, >> >> On 26/05/2021 18:23, Pratyush Yadav wrote: >>> TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate >>> capture over a CSI-2 bus. >>> >>> The Cadence CSI2RX IP acts as a bridge between the TI specific parts and >>> the CSI-2 protocol parts. TI then has a wrapper on top of this bridge >>> called the SHIM layer. It takes in data from stream 0, repacks it, and >>> sends it to memory over PSI-L DMA. >>> >>> This driver acts as the "front end" to V4L2 client applications. It >>> implements the required ioctls and buffer operations, passes the >>> necessary calls on to the bridge, programs the SHIM layer, and performs >>> DMA via the dmaengine API to finally return the data to a buffer >>> supplied by the application. >>> >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >> >> I noticed that my test app didn't work at all with this, and I also wasn't >> able to use v4l2-ctl to set the format. > > I have not used v4l2-ctl, but I can see yavta works fine. What command > did you use for setting format via v4l2-ctl? > >> >> At least for my test app the problem was that this driver doesn't initialize >> the format at all. My app first calls VIDIOC_G_FMT with v4l2_format.type == >> V4L2_BUF_TYPE_VIDEO_CAPTURE, then after the call modifies the fields it >> wants to change and calls VIDIOC_S_FMT. This failed, as G_FMT returned >> uninitialized fmt, i.e. type was 0, which my app didn't set again. >> >> I believe the driver should have an initial format, something that it will >> accept if an app calls G_FMT and then S_FMT. > > Right. This is a bug. The question is what should the initial format be? > It is more or less arbitrary since there is no configuration made yet > and we don't know what the camera can or will send. So for example, what > if I use UYVY 640x480? The camera might not support it at all. Is it > still OK to have it as the default? I think it doesn't really matter what the initial format is, as long as it's valid for the subdev itself. There are two separate things: 1) Subdev config, where the subdev is considered independently from the other subdevs. E.g. the formats in the input pads and output pads should be valid, taking into account the capabilities of the subdev. The subdev driver has to take care of these, i.e. if the user sets a format on a pad, the driver must adjust the other pads (if needed) to keep the subdev config valid. 2) pipeline validation, where all the subdevs in the pipeline are looked at and validated that the settings are compatible. We're talking about 1) here, and it doesn't matter if the camera supports the csirx initial format or not. Tomi
Hi, This series adds support for CSI2 capture on J721E. It includes some fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY driver, and finally adds the TI CSI2RX wrapper driver. Tested on TI's J721E with OV5640 sensor. Changes in v2: - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making calls to set PHY mode, etc. to make sure it is ready. - Add all 64 threads, instead of having only the one thread being currently used by the driver. - Use dmaengine_get_dma_device() instead of directly accessing dma->device->dev. - Do not set dst_addr_width when configuring slave DMA. - Move to a separate subdir and rename to j721e-csi2rx.c - Convert compatible to ti,j721e-csi2rx. - Move to use Media Controller centric APIs. - Improve cleanup in probe when one of the steps fails. - Add colorspace to formats database. - Set hw_revision on media_device. - Move video device initialization to probe time instead of register time. - Rename to ti,j721e-csi2rx.yaml - Add an entry in MAINTAINERS. - Add a description for the binding. - Change compatible to ti,j721e-csi2rx to make it SoC specific. - Remove description from dmas, reg, power-domains. - Remove a limit of 2 from #address-cells and #size-cells. - Fix add ^ to csi-bridge subnode regex. - Make ranges mandatory. - Add unit address in example. - Add a reference to cdns,csi2rx in csi-bridge subnode. - Expand the example to include the csi-bridge subnode as well. - Re-order subject prefixes. - Drop reg description. - Add a description for each DPHY clock. - Rename dphy@... to phy@... in example. - Add Laurent's R-by. - Re-order subject prefixes. - Re-order subject prefixes. - Add power-domain to the example. - Add Laurent's R-by. - Re-order subject prefixes. - Convert OV5640 to use runtime PM and drop Cadence CSI2RX s_power patch. - Drop subdev call wrappers from cdns-csi2rx. - Move VPE and CAL to a separate subdir. - Rename ti-csi2rx.c to j721e-csi2rx.c Paul Kocialkowski (1): phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes Pratyush Yadav (17): phy: cdns-dphy: Prepare for Rx support phy: cdns-dphy: Allow setting mode phy: cdns-dphy: Add Rx support media: ov5640: Use runtime PM to control sensor power media: cadence: csi2rx: Add external DPHY support media: cadence: csi2rx: Soft reset the streams before starting capture media: cadence: csi2rx: Set the STOP bit when stopping a stream media: cadence: csi2rx: Fix stream data configuration media: cadence: csi2rx: Populate subdev devnode dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX media: Re-structure TI platform drivers media: ti: Add CSI2RX support for J721E media: dt-bindings: Add DT bindings for TI J721E CSI2RX driver media: dt-bindings: Convert Cadence CSI2RX binding to YAML phy: dt-bindings: Convert Cadence DPHY binding to YAML phy: dt-bindings: cdns,dphy: make clocks optional phy: dt-bindings: cdns,dphy: add power-domains property .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -- .../bindings/media/cdns,csi2rx.yaml | 164 ++++ .../bindings/media/ti,j721e-csi2rx.yaml | 101 ++ .../devicetree/bindings/phy/cdns,dphy.txt | 20 - .../devicetree/bindings/phy/cdns,dphy.yaml | 54 ++ MAINTAINERS | 10 +- drivers/dma/ti/k3-psil-j721e.c | 73 ++ drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ov5640.c | 124 ++- drivers/media/platform/Kconfig | 12 + drivers/media/platform/Makefile | 2 +- drivers/media/platform/cadence/cdns-csi2rx.c | 196 +++- drivers/media/platform/ti/Makefile | 4 + drivers/media/platform/ti/cal/Makefile | 3 + .../{ti-vpe => ti/cal}/cal-camerarx.c | 0 .../platform/{ti-vpe => ti/cal}/cal-video.c | 0 .../media/platform/{ti-vpe => ti/cal}/cal.c | 0 .../media/platform/{ti-vpe => ti/cal}/cal.h | 0 .../platform/{ti-vpe => ti/cal}/cal_regs.h | 0 .../media/platform/ti/j721e-csi2rx/Makefile | 2 + .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 884 ++++++++++++++++++ .../platform/{ti-vpe => ti/vpe}/Makefile | 4 - .../media/platform/{ti-vpe => ti/vpe}/csc.c | 0 .../media/platform/{ti-vpe => ti/vpe}/csc.h | 0 .../media/platform/{ti-vpe => ti/vpe}/sc.c | 0 .../media/platform/{ti-vpe => ti/vpe}/sc.h | 0 .../platform/{ti-vpe => ti/vpe}/sc_coeff.h | 0 .../media/platform/{ti-vpe => ti/vpe}/vpdma.c | 0 .../media/platform/{ti-vpe => ti/vpe}/vpdma.h | 0 .../platform/{ti-vpe => ti/vpe}/vpdma_priv.h | 0 .../media/platform/{ti-vpe => ti/vpe}/vpe.c | 0 .../platform/{ti-vpe => ti/vpe}/vpe_regs.h | 0 drivers/phy/cadence/cdns-dphy.c | 407 +++++++- include/linux/phy/phy-mipi-dphy.h | 13 + 34 files changed, 1950 insertions(+), 225 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.yaml create mode 100644 Documentation/devicetree/bindings/media/ti,j721e-csi2rx.yaml delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml create mode 100644 drivers/media/platform/ti/Makefile create mode 100644 drivers/media/platform/ti/cal/Makefile rename drivers/media/platform/{ti-vpe => ti/cal}/cal-camerarx.c (100%) rename drivers/media/platform/{ti-vpe => ti/cal}/cal-video.c (100%) rename drivers/media/platform/{ti-vpe => ti/cal}/cal.c (100%) rename drivers/media/platform/{ti-vpe => ti/cal}/cal.h (100%) rename drivers/media/platform/{ti-vpe => ti/cal}/cal_regs.h (100%) create mode 100644 drivers/media/platform/ti/j721e-csi2rx/Makefile create mode 100644 drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c rename drivers/media/platform/{ti-vpe => ti/vpe}/Makefile (78%) rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.c (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.h (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.c (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.h (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/sc_coeff.h (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.c (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.h (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma_priv.h (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe.c (100%) rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe_regs.h (100%)