Message ID | 20230314115516.667-1-vaishnav.a@ti.com (mailing list archive) |
---|---|
Headers | show |
Series | CSI2RX support on J721E | expand |
On Tue, 2023-03-14 at 17:25 +0530, Vaishnav Achath wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper > driver. > > This is a V7 of the below V6 series, > https://lore.kernel.org/all/20220121142904.4091481-1-p.yadav@ti.com/ > > Since Pratyush moved out of TI, I will be working on upstreaming the > TI CSI2RX wrapper support. > > Tested on TI's J721E EVM with LI OV5640 sensor module. > https://gist.github.com/vaishnavachath/f030a257d5b6569817bc9deab1c4fa77 > > Also, Tested on TI AM62-SK with Pcam5C OV5640 module. > https://gist.github.com/vaishnavachath/ff2605faa92f1a6ab5670426da28ccee > Hi Vaishnav, I assume I'm doing something wrong. I have a TI AM62-SK and the Pcam5C OV5640 module. I've been trying to test this with gstreamer using the following command: gst-launch-1.0 -v v4l2src device=/dev/video0 num-buffers=10 ! video/x- raw, width=640, height=480, format=UYVY, framerate=30/1 ! jpegenc ! multifilesink location=test%d.jpg However I've not been able to get this working, failing with this failure unless I patch in some changes I found in the TI BSP: [ 28.083635] cdns-mipi-dphy-rx 30110000.phy: DPHY wait for lane ready timeout [ 28.090905] cdns-csi2rx 30101000.csi-bridge: Failed to configure external DPHY: -110 The changes (and the device tree nodes I added, which might be wrong...) can be found here: https://gitlab.collabora.com/martyn/linux/-/commits/am625-sk-ov5640 Any ideas what I'm doing wrong? Martyn > For all newer TI platforms that TI J721E Silicon Revision 1.0, below > update > to DPHY RX driver is needed: > https://lore.kernel.org/all/20230314073137.2153-1-vaishnav.a@ti.com/ > > Changes in v7: > - For patch 10/13 ("Add CSI2RX upport for J721E"): > - Fix incorrect value written in SHIM_PSI_CFG0_DST_TAG > - Drop support for 2X8 formats. > - Update maintainer to Vaishnav as Pratyush moved out of TI. > - Address Sakari's review comments: > - Update MAX_HEIGHT_LINES, MAX_WIDTH_BYTES to prevent overflow. > - Assign dma_slave_config during declaration, drop memset(). > - dma_release_channel() on slave_config failure. > - provide entity ops for the vdev entity with link_validate(). > - mutex_destroy() on ti_csi2rx_probe failure path. > - Drop busy check in remove(). > - mutex_destroy() in ti_csi2rx_remove(). > - Address Laurent's review comments: > - Update entries in Makefile in alphabetical order. > - include headers in alphabetical order. > - Drop redundant CSI DT defines and use from media/mipi-csi2.h. > - Rename csi_df to csi_dt. > - Drop v4l2_colorspace from ti_csi2rx_fmt and set default in > ti_csi2rx_v4l2_init() > - Adjust field and not return EINVAL in ti_csi2rx_try_fmt_vid_cap(). > - inline ti_csi2rx_video_register(). > - start DMA before starting source subdev. > - move buffer cleanup to separate function > ti_csi2rx_cleanup_buffers() > to be used in ti_csi2rx_stop_streaming() and > ti_csi2rx_start_streaming() > failure path. > - Drop VB2_USERPTR, VB2_READ and V4L2_CAP_READWRITE. > - For patch 4/13 ("media: cadence: csi2rx: Add external DPHY > support"): > - Fix multiplier and divider in v4l2_get_link_freq() which caused > failures during streaming. > > Changes in v6: > - Move the lock around the dereference for framefmt in > csi2rx_{get,set}_fmt() instead of when we get the pointer. > - Do not return an error when an unsupported format is set. Instead > adjust the code to the first format in the list. > - Drop variable bpp and use fmt->bpp directly. > - Drop variable got_pm. Call phy_pm_runtime_put() unconditionally > since > it will just return an error if runtime PM is not enabled. > - Drop transcoding from the commit message. > - Make csi2rx_media_ops const. > > Changes in v5: > - Cleanup notifier in csi2rx_parse_dt() after the call to > v4l2_async_nf_add_fwnode_remote(). > - Use YUV 1X16 formats instead of 2X8. > - Only error out when phy_pm_runtime_get_sync() returns a negative > value. A positive value can be returned if the phy was already > resumed. > - Do not query the source subdev for format. Use the newly added > internal format instead. > - Make i unsigned. > - Change %d to %u > - Add dependency on PHY_CADENCE_DPHY_RX instead of PHY_CADENCE_DPHY > since the Rx mode DPHY now has a separate driver. > - Drop ti_csi2rx_validate_pipeline(). Pipeline validation should be > done > at media_pipeline_start(). > - Do not assign flags. > - Fix error handling in ti_csi2rx_start_streaming(). Free up vb2 > buffers > when media_pipeline_start() fails. > - Move clock description in comments under the clocks property. > - Make ports required. > - Add link validation to cdns-csi2rx driver. > > Changes in v4: > - Drop the call to set PHY submode. It is now being done via > compatible > on the DPHY side. > - Acquire the media device's graph_mutex before starting the graph > walk. > - Call media_graph_walk_init() and media_graph_walk_cleanup() when > starting and ending the graph walk respectively. > - Reduce max frame height and width in enum_framesizes. Currently > they > are set to UINT_MAX but they must be a multiple of step_width, so > they > need to be rounded down. Also, these values are absurdly large > which > causes some userspace applications like gstreamer to trip up. While > it > is not generally right to change the kernel for an application bug, > it > is not such a big deal here. This change is replacing one set of > absurdly large arbitrary values with another set of smaller but > still > absurdly large arbitrary values. Both limits are unlikely to be hit > in > practice. > - Add power-domains property. > - Drop maxItems from clock-names. > - Drop the type for data-lanes. > - Drop uniqueItems from data-lanes. Move it to video-interfaces.yaml > instead. > - Drop OV5640 runtime pm patch. It seems to be a bit complicated and > it > is not exactly necessary for this series. Any CSI-2 camera will > work > just fine, OV5640 just happens to be the one I tested with. I don't > want it to block this series. I will submit it as a separate patch > later. > > Changes in v3: > - Use v4l2_get_link_freq() to calculate pixel clock. > - Move DMA related fields in struct ti_csi2rx_dma. > - Protect DMA buffer queue with a spinlock to make sure the queue > buffer > and DMA callback don't race on it. > - Track the current DMA state. It might go idle because of a lack of > buffers. This state can be used to restart it if needed. > - Do not include the current buffer in the pending queue. It is > slightly > better modelling than leaving it at the head of the pending queue. > - Use the buffer as the callback argument, and add a reference to csi > in it. > - If queueing a buffer to DMA fails, the buffer gets leaked and DMA > gets > stalled with. Instead, report the error to vb2 and queue the next > buffer in the pending queue. > - DMA gets stalled if we run out of buffers since the callback is the > only one that fires subsequent transfers and it is no longer being > called. Check for that when queueing buffers and restart DMA if > needed. > - Do not put of node until we are done using the fwnode. > - Set inital format to UYVY 640x480. > - Add compatible: contains: const: cdns,csi2rx to allow SoC specific > compatible. > - Add more constraints for data-lanes property. > > 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. > - 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. > - 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 > > Pratyush Yadav (13): > media: cadence: csi2rx: Unregister v4l2 async notifier > media: cadence: csi2rx: Cleanup media entity properly > media: cadence: csi2rx: Add get_fmt and set_fmt pad ops > 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 > media: cadence: csi2rx: Add link validation > media: ti: Add CSI2RX support for J721E > media: dt-bindings: Make sure items in data-lanes are unique > media: dt-bindings: Add DT bindings for TI J721E CSI2RX driver > media: dt-bindings: Convert Cadence CSI2RX binding to YAML > > .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -- > .../bindings/media/cdns,csi2rx.yaml | 176 +++ > .../bindings/media/ti,j721e-csi2rx.yaml | 101 ++ > .../bindings/media/video-interfaces.yaml | 1 + > MAINTAINERS | 7 + > drivers/media/platform/cadence/cdns-csi2rx.c | 273 ++++- > drivers/media/platform/ti/Kconfig | 12 + > drivers/media/platform/ti/Makefile | 1 + > .../media/platform/ti/j721e-csi2rx/Makefile | 2 + > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 1022 > +++++++++++++++++ > 10 files changed, 1580 insertions(+), 115 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 > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/Makefile > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/j721e- > csi2rx.c >
Hi Martyn, Thanks for the tests. On Mar 23, 2023 at 19:36:18 +0000, Martyn Welch wrote: > On Tue, 2023-03-14 at 17:25 +0530, Vaishnav Achath wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper > > driver. > > > > This is a V7 of the below V6 series, > > https://lore.kernel.org/all/20220121142904.4091481-1-p.yadav@ti.com/ > > > > Since Pratyush moved out of TI, I will be working on upstreaming the > > TI CSI2RX wrapper support. > > > > Tested on TI's J721E EVM with LI OV5640 sensor module. > > https://gist.github.com/vaishnavachath/f030a257d5b6569817bc9deab1c4fa77 > > > > Also, Tested on TI AM62-SK with Pcam5C OV5640 module. > > https://gist.github.com/vaishnavachath/ff2605faa92f1a6ab5670426da28ccee > > > > Hi Vaishnav, > > I assume I'm doing something wrong. I have a TI AM62-SK and the Pcam5C > OV5640 module. I've been trying to test this with gstreamer using the > following command: > > gst-launch-1.0 -v v4l2src device=/dev/video0 num-buffers=10 ! video/x- > raw, width=640, height=480, format=UYVY, framerate=30/1 ! jpegenc ! > multifilesink location=test%d.jpg > > However I've not been able to get this working, failing with this > failure unless I patch in some changes I found in the TI BSP: > > [ 28.083635] cdns-mipi-dphy-rx 30110000.phy: DPHY wait for lane ready > timeout > [ 28.090905] cdns-csi2rx 30101000.csi-bridge: Failed to configure > external DPHY: -110 That is expected as currently the DPHY driver does not release the SW reset which all SoCs since J721E SR1.0 expect. See the patch linked by Vaishnav below. > > The changes (and the device tree nodes I added, which might be > wrong...) can be found here: > > https://gitlab.collabora.com/martyn/linux/-/commits/am625-sk-ov5640 > > Any ideas what I'm doing wrong? > > Martyn > > > For all newer TI platforms that TI J721E Silicon Revision 1.0, below > > update > > to DPHY RX driver is needed: > > https://lore.kernel.org/all/20230314073137.2153-1-vaishnav.a@ti.com/ ^ With that patch and this series on top of linux-next, I was able to get Pcam5C capturing with SK-AM62. Although I did face issues with the sensor framerate and had to revert a few recent sensor commits, not sure why exactly yet. But here is the working branch with all the changes: https://github.com/jailuthra/linux/commits/b4/csi_single Let us know if you face any other issues during your tests. > > > > Changes in v7: > > - For patch 10/13 ("Add CSI2RX upport for J721E"): > > - Fix incorrect value written in SHIM_PSI_CFG0_DST_TAG > > - Drop support for 2X8 formats. > > - Update maintainer to Vaishnav as Pratyush moved out of TI. > > - Address Sakari's review comments: > > - Update MAX_HEIGHT_LINES, MAX_WIDTH_BYTES to prevent overflow. > > - Assign dma_slave_config during declaration, drop memset(). > > - dma_release_channel() on slave_config failure. > > - provide entity ops for the vdev entity with link_validate(). > > - mutex_destroy() on ti_csi2rx_probe failure path. > > - Drop busy check in remove(). > > - mutex_destroy() in ti_csi2rx_remove(). > > - Address Laurent's review comments: > > - Update entries in Makefile in alphabetical order. > > - include headers in alphabetical order. > > - Drop redundant CSI DT defines and use from media/mipi-csi2.h. > > - Rename csi_df to csi_dt. > > - Drop v4l2_colorspace from ti_csi2rx_fmt and set default in > > ti_csi2rx_v4l2_init() > > - Adjust field and not return EINVAL in ti_csi2rx_try_fmt_vid_cap(). > > - inline ti_csi2rx_video_register(). > > - start DMA before starting source subdev. > > - move buffer cleanup to separate function > > ti_csi2rx_cleanup_buffers() > > to be used in ti_csi2rx_stop_streaming() and > > ti_csi2rx_start_streaming() > > failure path. > > - Drop VB2_USERPTR, VB2_READ and V4L2_CAP_READWRITE. > > - For patch 4/13 ("media: cadence: csi2rx: Add external DPHY > > support"): > > - Fix multiplier and divider in v4l2_get_link_freq() which caused > > failures during streaming. > > > > Changes in v6: > > - Move the lock around the dereference for framefmt in > > csi2rx_{get,set}_fmt() instead of when we get the pointer. > > - Do not return an error when an unsupported format is set. Instead > > adjust the code to the first format in the list. > > - Drop variable bpp and use fmt->bpp directly. > > - Drop variable got_pm. Call phy_pm_runtime_put() unconditionally > > since > > it will just return an error if runtime PM is not enabled. > > - Drop transcoding from the commit message. > > - Make csi2rx_media_ops const. > > > > Changes in v5: > > - Cleanup notifier in csi2rx_parse_dt() after the call to > > v4l2_async_nf_add_fwnode_remote(). > > - Use YUV 1X16 formats instead of 2X8. > > - Only error out when phy_pm_runtime_get_sync() returns a negative > > value. A positive value can be returned if the phy was already > > resumed. > > - Do not query the source subdev for format. Use the newly added > > internal format instead. > > - Make i unsigned. > > - Change %d to %u > > - Add dependency on PHY_CADENCE_DPHY_RX instead of PHY_CADENCE_DPHY > > since the Rx mode DPHY now has a separate driver. > > - Drop ti_csi2rx_validate_pipeline(). Pipeline validation should be > > done > > at media_pipeline_start(). > > - Do not assign flags. > > - Fix error handling in ti_csi2rx_start_streaming(). Free up vb2 > > buffers > > when media_pipeline_start() fails. > > - Move clock description in comments under the clocks property. > > - Make ports required. > > - Add link validation to cdns-csi2rx driver. > > > > Changes in v4: > > - Drop the call to set PHY submode. It is now being done via > > compatible > > on the DPHY side. > > - Acquire the media device's graph_mutex before starting the graph > > walk. > > - Call media_graph_walk_init() and media_graph_walk_cleanup() when > > starting and ending the graph walk respectively. > > - Reduce max frame height and width in enum_framesizes. Currently > > they > > are set to UINT_MAX but they must be a multiple of step_width, so > > they > > need to be rounded down. Also, these values are absurdly large > > which > > causes some userspace applications like gstreamer to trip up. While > > it > > is not generally right to change the kernel for an application bug, > > it > > is not such a big deal here. This change is replacing one set of > > absurdly large arbitrary values with another set of smaller but > > still > > absurdly large arbitrary values. Both limits are unlikely to be hit > > in > > practice. > > - Add power-domains property. > > - Drop maxItems from clock-names. > > - Drop the type for data-lanes. > > - Drop uniqueItems from data-lanes. Move it to video-interfaces.yaml > > instead. > > - Drop OV5640 runtime pm patch. It seems to be a bit complicated and > > it > > is not exactly necessary for this series. Any CSI-2 camera will > > work > > just fine, OV5640 just happens to be the one I tested with. I don't > > want it to block this series. I will submit it as a separate patch > > later. > > > > Changes in v3: > > - Use v4l2_get_link_freq() to calculate pixel clock. > > - Move DMA related fields in struct ti_csi2rx_dma. > > - Protect DMA buffer queue with a spinlock to make sure the queue > > buffer > > and DMA callback don't race on it. > > - Track the current DMA state. It might go idle because of a lack of > > buffers. This state can be used to restart it if needed. > > - Do not include the current buffer in the pending queue. It is > > slightly > > better modelling than leaving it at the head of the pending queue. > > - Use the buffer as the callback argument, and add a reference to csi > > in it. > > - If queueing a buffer to DMA fails, the buffer gets leaked and DMA > > gets > > stalled with. Instead, report the error to vb2 and queue the next > > buffer in the pending queue. > > - DMA gets stalled if we run out of buffers since the callback is the > > only one that fires subsequent transfers and it is no longer being > > called. Check for that when queueing buffers and restart DMA if > > needed. > > - Do not put of node until we are done using the fwnode. > > - Set inital format to UYVY 640x480. > > - Add compatible: contains: const: cdns,csi2rx to allow SoC specific > > compatible. > > - Add more constraints for data-lanes property. > > > > 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. > > - 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. > > - 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 > > > > Pratyush Yadav (13): > > media: cadence: csi2rx: Unregister v4l2 async notifier > > media: cadence: csi2rx: Cleanup media entity properly > > media: cadence: csi2rx: Add get_fmt and set_fmt pad ops > > 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 > > media: cadence: csi2rx: Add link validation > > media: ti: Add CSI2RX support for J721E > > media: dt-bindings: Make sure items in data-lanes are unique > > media: dt-bindings: Add DT bindings for TI J721E CSI2RX driver > > media: dt-bindings: Convert Cadence CSI2RX binding to YAML > > > > .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -- > > .../bindings/media/cdns,csi2rx.yaml | 176 +++ > > .../bindings/media/ti,j721e-csi2rx.yaml | 101 ++ > > .../bindings/media/video-interfaces.yaml | 1 + > > MAINTAINERS | 7 + > > drivers/media/platform/cadence/cdns-csi2rx.c | 273 ++++- > > drivers/media/platform/ti/Kconfig | 12 + > > drivers/media/platform/ti/Makefile | 1 + > > .../media/platform/ti/j721e-csi2rx/Makefile | 2 + > > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 1022 > > +++++++++++++++++ > > 10 files changed, 1580 insertions(+), 115 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 > > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/Makefile > > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/j721e- > > csi2rx.c > > > Thanks, Jai
On Tue, 2023-03-28 at 11:31 +0530, Jai Luthra wrote: > Hi Martyn, > > Thanks for the tests. > > On Mar 23, 2023 at 19:36:18 +0000, Martyn Welch wrote: > > On Tue, 2023-03-14 at 17:25 +0530, Vaishnav Achath wrote: > > > Hi, > > > > > > This series adds support for CSI2 capture on J721E. It includes > > > some > > > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX > > > wrapper > > > driver. > > > > > > This is a V7 of the below V6 series, > > > https://lore.kernel.org/all/20220121142904.4091481-1-p.yadav@ti.com/ > > > > > > Since Pratyush moved out of TI, I will be working on upstreaming > > > the > > > TI CSI2RX wrapper support. > > > > > > Tested on TI's J721E EVM with LI OV5640 sensor module. > > > https://gist.github.com/vaishnavachath/f030a257d5b6569817bc9deab1c4fa77 > > > > > > Also, Tested on TI AM62-SK with Pcam5C OV5640 module. > > > https://gist.github.com/vaishnavachath/ff2605faa92f1a6ab5670426da28ccee > > > > > > > Hi Vaishnav, > > > > I assume I'm doing something wrong. I have a TI AM62-SK and the > > Pcam5C > > OV5640 module. I've been trying to test this with gstreamer using > > the > > following command: > > > > gst-launch-1.0 -v v4l2src device=/dev/video0 num-buffers=10 ! > > video/x- > > raw, width=640, height=480, format=UYVY, framerate=30/1 ! jpegenc ! > > multifilesink location=test%d.jpg > > > > However I've not been able to get this working, failing with this > > failure unless I patch in some changes I found in the TI BSP: > > > > [ 28.083635] cdns-mipi-dphy-rx 30110000.phy: DPHY wait for lane > > ready > > timeout > > [ 28.090905] cdns-csi2rx 30101000.csi-bridge: Failed to configure > > external DPHY: -110 > > That is expected as currently the DPHY driver does not release the SW > reset which all SoCs since J721E SR1.0 expect. See the patch linked > by > Vaishnav below. > Ah! Sorry - somehow missed that... Yep, with those changes it's working for me, thanks! Tested-by: Martyn Welch <martyn.welch@collabora.com> > > > > The changes (and the device tree nodes I added, which might be > > wrong...) can be found here: > > > > https://gitlab.collabora.com/martyn/linux/-/commits/am625-sk-ov5640 > > > > Any ideas what I'm doing wrong? > > > > Martyn > > > > > For all newer TI platforms that TI J721E Silicon Revision 1.0, > > > below > > > update > > > to DPHY RX driver is needed: > > > https://lore.kernel.org/all/20230314073137.2153-1-vaishnav.a@ti.com/ > > ^ > > With that patch and this series on top of linux-next, I was able to > get > Pcam5C capturing with SK-AM62. Although I did face issues with the > sensor framerate and had to revert a few recent sensor commits, not > sure > why exactly yet. But here is the working branch with all the changes: > https://github.com/jailuthra/linux/commits/b4/csi_single > > Let us know if you face any other issues during your tests. > > > > > > > Changes in v7: > > > - For patch 10/13 ("Add CSI2RX upport for J721E"): > > > - Fix incorrect value written in SHIM_PSI_CFG0_DST_TAG > > > - Drop support for 2X8 formats. > > > - Update maintainer to Vaishnav as Pratyush moved out of TI. > > > - Address Sakari's review comments: > > > - Update MAX_HEIGHT_LINES, MAX_WIDTH_BYTES to prevent overflow. > > > - Assign dma_slave_config during declaration, drop memset(). > > > - dma_release_channel() on slave_config failure. > > > - provide entity ops for the vdev entity with link_validate(). > > > - mutex_destroy() on ti_csi2rx_probe failure path. > > > - Drop busy check in remove(). > > > - mutex_destroy() in ti_csi2rx_remove(). > > > - Address Laurent's review comments: > > > - Update entries in Makefile in alphabetical order. > > > - include headers in alphabetical order. > > > - Drop redundant CSI DT defines and use from media/mipi-csi2.h. > > > - Rename csi_df to csi_dt. > > > - Drop v4l2_colorspace from ti_csi2rx_fmt and set default in > > > ti_csi2rx_v4l2_init() > > > - Adjust field and not return EINVAL in > > > ti_csi2rx_try_fmt_vid_cap(). > > > - inline ti_csi2rx_video_register(). > > > - start DMA before starting source subdev. > > > - move buffer cleanup to separate function > > > ti_csi2rx_cleanup_buffers() > > > to be used in ti_csi2rx_stop_streaming() and > > > ti_csi2rx_start_streaming() > > > failure path. > > > - Drop VB2_USERPTR, VB2_READ and V4L2_CAP_READWRITE. > > > - For patch 4/13 ("media: cadence: csi2rx: Add external DPHY > > > support"): > > > - Fix multiplier and divider in v4l2_get_link_freq() which caused > > > failures during streaming. > > > > > > Changes in v6: > > > - Move the lock around the dereference for framefmt in > > > csi2rx_{get,set}_fmt() instead of when we get the pointer. > > > - Do not return an error when an unsupported format is set. > > > Instead > > > adjust the code to the first format in the list. > > > - Drop variable bpp and use fmt->bpp directly. > > > - Drop variable got_pm. Call phy_pm_runtime_put() unconditionally > > > since > > > it will just return an error if runtime PM is not enabled. > > > - Drop transcoding from the commit message. > > > - Make csi2rx_media_ops const. > > > > > > Changes in v5: > > > - Cleanup notifier in csi2rx_parse_dt() after the call to > > > v4l2_async_nf_add_fwnode_remote(). > > > - Use YUV 1X16 formats instead of 2X8. > > > - Only error out when phy_pm_runtime_get_sync() returns a > > > negative > > > value. A positive value can be returned if the phy was already > > > resumed. > > > - Do not query the source subdev for format. Use the newly added > > > internal format instead. > > > - Make i unsigned. > > > - Change %d to %u > > > - Add dependency on PHY_CADENCE_DPHY_RX instead of > > > PHY_CADENCE_DPHY > > > since the Rx mode DPHY now has a separate driver. > > > - Drop ti_csi2rx_validate_pipeline(). Pipeline validation should > > > be > > > done > > > at media_pipeline_start(). > > > - Do not assign flags. > > > - Fix error handling in ti_csi2rx_start_streaming(). Free up vb2 > > > buffers > > > when media_pipeline_start() fails. > > > - Move clock description in comments under the clocks property. > > > - Make ports required. > > > - Add link validation to cdns-csi2rx driver. > > > > > > Changes in v4: > > > - Drop the call to set PHY submode. It is now being done via > > > compatible > > > on the DPHY side. > > > - Acquire the media device's graph_mutex before starting the > > > graph > > > walk. > > > - Call media_graph_walk_init() and media_graph_walk_cleanup() > > > when > > > starting and ending the graph walk respectively. > > > - Reduce max frame height and width in enum_framesizes. Currently > > > they > > > are set to UINT_MAX but they must be a multiple of step_width, > > > so > > > they > > > need to be rounded down. Also, these values are absurdly large > > > which > > > causes some userspace applications like gstreamer to trip up. > > > While > > > it > > > is not generally right to change the kernel for an application > > > bug, > > > it > > > is not such a big deal here. This change is replacing one set > > > of > > > absurdly large arbitrary values with another set of smaller but > > > still > > > absurdly large arbitrary values. Both limits are unlikely to be > > > hit > > > in > > > practice. > > > - Add power-domains property. > > > - Drop maxItems from clock-names. > > > - Drop the type for data-lanes. > > > - Drop uniqueItems from data-lanes. Move it to video- > > > interfaces.yaml > > > instead. > > > - Drop OV5640 runtime pm patch. It seems to be a bit complicated > > > and > > > it > > > is not exactly necessary for this series. Any CSI-2 camera will > > > work > > > just fine, OV5640 just happens to be the one I tested with. I > > > don't > > > want it to block this series. I will submit it as a separate > > > patch > > > later. > > > > > > Changes in v3: > > > - Use v4l2_get_link_freq() to calculate pixel clock. > > > - Move DMA related fields in struct ti_csi2rx_dma. > > > - Protect DMA buffer queue with a spinlock to make sure the queue > > > buffer > > > and DMA callback don't race on it. > > > - Track the current DMA state. It might go idle because of a lack > > > of > > > buffers. This state can be used to restart it if needed. > > > - Do not include the current buffer in the pending queue. It is > > > slightly > > > better modelling than leaving it at the head of the pending > > > queue. > > > - Use the buffer as the callback argument, and add a reference to > > > csi > > > in it. > > > - If queueing a buffer to DMA fails, the buffer gets leaked and > > > DMA > > > gets > > > stalled with. Instead, report the error to vb2 and queue the > > > next > > > buffer in the pending queue. > > > - DMA gets stalled if we run out of buffers since the callback is > > > the > > > only one that fires subsequent transfers and it is no longer > > > being > > > called. Check for that when queueing buffers and restart DMA if > > > needed. > > > - Do not put of node until we are done using the fwnode. > > > - Set inital format to UYVY 640x480. > > > - Add compatible: contains: const: cdns,csi2rx to allow SoC > > > specific > > > compatible. > > > - Add more constraints for data-lanes property. > > > > > > 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. > > > - 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. > > > - 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 > > > > > > Pratyush Yadav (13): > > > media: cadence: csi2rx: Unregister v4l2 async notifier > > > media: cadence: csi2rx: Cleanup media entity properly > > > media: cadence: csi2rx: Add get_fmt and set_fmt pad ops > > > 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 > > > media: cadence: csi2rx: Add link validation > > > media: ti: Add CSI2RX support for J721E > > > media: dt-bindings: Make sure items in data-lanes are unique > > > media: dt-bindings: Add DT bindings for TI J721E CSI2RX driver > > > media: dt-bindings: Convert Cadence CSI2RX binding to YAML > > > > > > .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -- > > > .../bindings/media/cdns,csi2rx.yaml | 176 +++ > > > .../bindings/media/ti,j721e-csi2rx.yaml | 101 ++ > > > .../bindings/media/video-interfaces.yaml | 1 + > > > MAINTAINERS | 7 + > > > drivers/media/platform/cadence/cdns-csi2rx.c | 273 ++++- > > > drivers/media/platform/ti/Kconfig | 12 + > > > drivers/media/platform/ti/Makefile | 1 + > > > .../media/platform/ti/j721e-csi2rx/Makefile | 2 + > > > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 1022 > > > +++++++++++++++++ > > > 10 files changed, 1580 insertions(+), 115 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 > > > create mode 100644 drivers/media/platform/ti/j721e- > > > csi2rx/Makefile > > > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/j721e- > > > csi2rx.c > > > > > > > Thanks, > Jai
On 14/03/2023 13:55, Vaishnav Achath wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper driver. I get some errors with 'v4l2-compliance -m /dev/media0', which also causes: [ 51.185172] ------------[ cut here ]------------ [ 51.189786] WARNING: CPU: 1 PID: 174 at mm/page_alloc.c:5568 __alloc_pages+0x684/0xd24 [ 51.197698] Modules linked in: [ 51.200743] CPU: 1 PID: 174 Comm: v4l2-compliance Not tainted 6.3.0-rc5+ #12 [ 51.207772] Hardware name: Texas Instruments J721e EVM (DT) [ 51.213326] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 51.220269] pc : __alloc_pages+0x684/0xd24 [ 51.224353] lr : __dma_direct_alloc_pages.constprop.0+0x1d4/0x2a0 [ 51.230431] sp : ffff800009fd37b0 [ 51.233732] x29: ffff800009fd37b0 x28: ffff000801ad18e0 x27: 0000000000000001 [ 51.240852] x26: 0000000000000010 x25: ffff800009fd3b18 x24: ffff800008112c60 [ 51.247971] x23: 0000000000000010 x22: 00000000ffffffff x21: 0000000000000cc0 [ 51.255089] x20: 0000000010000000 x19: 0000000000000000 x18: 0000000000000000 [ 51.262208] x17: 0000000000000000 x16: 0000000000000000 x15: 00000000fff7a238 [ 51.269326] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [ 51.276444] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 [ 51.283562] x8 : ffff000802c4a200 x7 : 0000000000000000 x6 : 0000000000000000 [ 51.290680] x5 : 0000000000000000 x4 : ffff0008022a0000 x3 : 0000000000000000 [ 51.297798] x2 : 0000000000000000 x1 : 0000000000000001 x0 : ffff80000937a000 [ 51.304916] Call trace: [ 51.307350] __alloc_pages+0x684/0xd24 [ 51.311086] __dma_direct_alloc_pages.constprop.0+0x1d4/0x2a0 [ 51.316814] dma_direct_alloc+0x210/0x33c [ 51.320809] dma_alloc_attrs+0x80/0xf4 [ 51.324548] vb2_dc_alloc+0xa0/0x184 [ 51.328114] __vb2_queue_alloc+0x19c/0x490 [ 51.332197] vb2_core_reqbufs+0x250/0x45c [ 51.336192] vb2_ioctl_reqbufs+0xb0/0xe8 [ 51.340103] v4l_reqbufs+0x50/0x64 [ 51.343494] __video_do_ioctl+0x18c/0x3ec [ 51.347489] video_usercopy+0x214/0x6c4 [ 51.351312] video_ioctl2+0x18/0x24 [ 51.354788] v4l2_ioctl+0x40/0x60 [ 51.358088] v4l2_compat_ioctl32+0x90/0xb4 [ 51.362171] __arm64_compat_sys_ioctl+0x14c/0x170 [ 51.366860] invoke_syscall+0x48/0x114 [ 51.370599] el0_svc_common.constprop.0+0x44/0xf4 [ 51.375288] do_el0_svc_compat+0x20/0x44 [ 51.379198] el0_svc_compat+0x2c/0x84 [ 51.382848] el0t_32_sync_handler+0x98/0x148 [ 51.387102] el0t_32_sync+0x194/0x198 [ 51.390752] ---[ end trace 0000000000000000 ]--- [ 51.395408] ti-udma 31150000.dma-controller: dma alloc of size 268435456 failed Tomi
Hi Vaishnav, On 14/03/2023 13:55, Vaishnav Achath wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper > driver. We are testing this patch series and experienced some strange behaviour, with the same sequence of 5-10 frames repeated over and over. (Almost the same sequence since frames have different md5sum) To solve this issue we had to forward port some functions from the TI BSP Kernel[1] such as ti_csi2rx_restart_dma, and ti_csi2rx_drain_dma. Can you consider this issue for the next patchset version? Thank you, Julien Here are the modifications we made for information only. --- .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 138 +++++++++++++++--- 1 file changed, 117 insertions(+), 21 deletions(-) diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c index 1af7b0b09cfc..e8579dbf07b4 100644 --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c @@ -46,6 +46,8 @@ #define MAX_WIDTH_BYTES SZ_16K #define MAX_HEIGHT_LINES SZ_16K +#define DRAIN_TIMEOUT_MS 50 + struct ti_csi2rx_fmt { u32 fourcc; /* Four character code. */ u32 code; /* Mbus code. */ @@ -498,6 +500,59 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi) writel(reg, csi->shim + SHIM_PSI_CFG0); } +static void ti_csi2rx_drain_callback(void *param) +{ + struct completion *drain_complete = param; + + complete(drain_complete); +} + +static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) +{ + struct dma_async_tx_descriptor *desc; + struct device *dev = csi->dma.chan->device->dev; + struct completion drain_complete; + void *buf; + size_t len = csi->v_fmt.fmt.pix.sizeimage; + dma_addr_t addr; + dma_cookie_t cookie; + int ret; + + init_completion(&drain_complete); + + buf = dma_alloc_coherent(dev, len, &addr, GFP_KERNEL | GFP_ATOMIC); + if (!buf) + return -ENOMEM; + + desc = dmaengine_prep_slave_single(csi->dma.chan, addr, len, + DMA_DEV_TO_MEM, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc) { + ret = -EIO; + goto out; + } + + desc->callback = ti_csi2rx_drain_callback; + desc->callback_param = &drain_complete; + + cookie = dmaengine_submit(desc); + ret = dma_submit_error(cookie); + if (ret) + goto out; + + dma_async_issue_pending(csi->dma.chan); + + if (!wait_for_completion_timeout(&drain_complete, + msecs_to_jiffies(DRAIN_TIMEOUT_MS))) { + dmaengine_terminate_sync(csi->dma.chan); + ret = -ETIMEDOUT; + goto out; + } +out: + dma_free_coherent(dev, len, buf, addr); + return ret; +} + static void ti_csi2rx_dma_callback(void *param) { struct ti_csi2rx_buffer *buf = param; @@ -564,24 +619,61 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, } static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_dev *csi, - enum vb2_buffer_state state) + enum vb2_buffer_state buf_state) { struct ti_csi2rx_dma *dma = &csi->dma; - struct ti_csi2rx_buffer *buf = NULL, *tmp; + struct ti_csi2rx_buffer *buf = NULL, *tmp, *curr;; + enum ti_csi2rx_dma_state state; unsigned long flags; + int ret; spin_lock_irqsave(&dma->lock, flags); list_for_each_entry_safe(buf, tmp, &csi->dma.queue, list) { list_del(&buf->list); - vb2_buffer_done(&buf->vb.vb2_buf, state); + vb2_buffer_done(&buf->vb.vb2_buf, buf_state); } - if (dma->curr) - vb2_buffer_done(&dma->curr->vb.vb2_buf, state); - + curr = csi->dma.curr; + state = csi->dma.state; dma->curr = NULL; dma->state = TI_CSI2RX_DMA_STOPPED; spin_unlock_irqrestore(&dma->lock, flags); + + if (state != TI_CSI2RX_DMA_STOPPED) { + ret = ti_csi2rx_drain_dma(csi); + if (ret) + dev_dbg(csi->dev, + "Failed to drain DMA. Next frame might be bogus\n"); + dmaengine_terminate_sync(csi->dma.chan); + } + + if (curr) + vb2_buffer_done(&curr->vb.vb2_buf, buf_state); +} + +static int ti_csi2rx_restart_dma(struct ti_csi2rx_dev *csi, + struct ti_csi2rx_buffer *buf) +{ + struct ti_csi2rx_dma *dma = &csi->dma; + unsigned long flags = 0; + int ret = 0; + + ret = ti_csi2rx_drain_dma(csi); + if (ret) + dev_warn(csi->dev, + "Failed to drain DMA. Next frame might be bogus\n"); + + ret = ti_csi2rx_start_dma(csi, buf); + if (ret) { + dev_err(csi->dev, "Failed to start DMA: %d\n", ret); + spin_lock_irqsave(&dma->lock, flags); + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); + dma->curr = NULL; + dma->state = TI_CSI2RX_DMA_IDLE; + spin_unlock_irqrestore(&dma->lock, flags); + } + + return ret; } static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers, @@ -622,6 +714,7 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue); struct ti_csi2rx_buffer *buf; struct ti_csi2rx_dma *dma = &csi->dma; + bool restart_dma = false; unsigned long flags = 0; int ret; @@ -634,21 +727,30 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) * But if DMA has stalled due to lack of buffers, restart it now. */ if (dma->state == TI_CSI2RX_DMA_IDLE) { - ret = ti_csi2rx_start_dma(csi, buf); - if (ret) { - dev_err(csi->dev, "Failed to start DMA: %d\n", ret); - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED); - goto unlock; - } - + /* + * Do not restart DMA with the lock held because + * ti_csi2rx_drain_dma() might block when allocating a buffer. + * There won't be a race on queueing DMA anyway since the + * callback is not being fired. + */ + restart_dma = true; dma->curr = buf; dma->state = TI_CSI2RX_DMA_ACTIVE; } else { list_add_tail(&buf->list, &dma->queue); } - -unlock: spin_unlock_irqrestore(&dma->lock, flags); + + if (restart_dma) { + /* + * Once frames start dropping, some data gets stuck in the DMA + * pipeline somewhere. So the first DMA transfer after frame + * drops gives a partial frame. This is obviously not useful to + * the application and will only confuse it. Issue a DMA + * transaction to drain that up. + */ + ti_csi2rx_restart_dma(csi, buf); + } } static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) @@ -718,12 +820,6 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) writel(0, csi->shim + SHIM_CNTL); - ret = dmaengine_terminate_sync(csi->dma.chan); - if (ret) - dev_err(csi->dev, "Failed to stop DMA: %d\n", ret); - - writel(0, csi->shim + SHIM_DMACNTX); - ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_ERROR); }
Hi Julien, On Jun 22, 2023 at 11:18:28 +0200, Julien Massot wrote: > Hi Vaishnav, > > On 14/03/2023 13:55, Vaishnav Achath wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper > > driver. > We are testing this patch series and experienced some strange behaviour, > with the same sequence of 5-10 frames repeated over and over. > (Almost the same sequence since frames have different md5sum) > > To solve this issue we had to forward port some functions from the TI BSP > Kernel[1] such as ti_csi2rx_restart_dma, and ti_csi2rx_drain_dma. > > Can you consider this issue for the next patchset version? While we haven't seen this particular issue (of repeating frames) due to a lack of draining, I agree that it is a useful fix. Will include it in v8 of this series along with some other features and fixes around stream stop sequence. Thanks, > > Thank you, > Julien > > > Here are the modifications we made for information only. > > --- > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 138 +++++++++++++++--- > 1 file changed, 117 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index 1af7b0b09cfc..e8579dbf07b4 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -46,6 +46,8 @@ > #define MAX_WIDTH_BYTES SZ_16K > #define MAX_HEIGHT_LINES SZ_16K > > +#define DRAIN_TIMEOUT_MS 50 > + > struct ti_csi2rx_fmt { > u32 fourcc; /* Four character code. */ > u32 code; /* Mbus code. */ > @@ -498,6 +500,59 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev > *csi) > writel(reg, csi->shim + SHIM_PSI_CFG0); > } > > +static void ti_csi2rx_drain_callback(void *param) > +{ > + struct completion *drain_complete = param; > + > + complete(drain_complete); > +} > + > +static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > +{ > + struct dma_async_tx_descriptor *desc; > + struct device *dev = csi->dma.chan->device->dev; > + struct completion drain_complete; > + void *buf; > + size_t len = csi->v_fmt.fmt.pix.sizeimage; > + dma_addr_t addr; > + dma_cookie_t cookie; > + int ret; > + > + init_completion(&drain_complete); > + > + buf = dma_alloc_coherent(dev, len, &addr, GFP_KERNEL | GFP_ATOMIC); > + if (!buf) > + return -ENOMEM; > + > + desc = dmaengine_prep_slave_single(csi->dma.chan, addr, len, > + DMA_DEV_TO_MEM, > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > + if (!desc) { > + ret = -EIO; > + goto out; > + } > + > + desc->callback = ti_csi2rx_drain_callback; > + desc->callback_param = &drain_complete; > + > + cookie = dmaengine_submit(desc); > + ret = dma_submit_error(cookie); > + if (ret) > + goto out; > + > + dma_async_issue_pending(csi->dma.chan); > + > + if (!wait_for_completion_timeout(&drain_complete, > + msecs_to_jiffies(DRAIN_TIMEOUT_MS))) { > + dmaengine_terminate_sync(csi->dma.chan); > + ret = -ETIMEDOUT; > + goto out; > + } > +out: > + dma_free_coherent(dev, len, buf, addr); > + return ret; > +} > + > static void ti_csi2rx_dma_callback(void *param) > { > struct ti_csi2rx_buffer *buf = param; > @@ -564,24 +619,61 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev > *csi, > } > > static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_dev *csi, > - enum vb2_buffer_state state) > + enum vb2_buffer_state buf_state) > { > struct ti_csi2rx_dma *dma = &csi->dma; > - struct ti_csi2rx_buffer *buf = NULL, *tmp; > + struct ti_csi2rx_buffer *buf = NULL, *tmp, *curr;; > + enum ti_csi2rx_dma_state state; > unsigned long flags; > + int ret; > > spin_lock_irqsave(&dma->lock, flags); > list_for_each_entry_safe(buf, tmp, &csi->dma.queue, list) { > list_del(&buf->list); > - vb2_buffer_done(&buf->vb.vb2_buf, state); > + vb2_buffer_done(&buf->vb.vb2_buf, buf_state); > } > > - if (dma->curr) > - vb2_buffer_done(&dma->curr->vb.vb2_buf, state); > - > + curr = csi->dma.curr; > + state = csi->dma.state; > dma->curr = NULL; > dma->state = TI_CSI2RX_DMA_STOPPED; > spin_unlock_irqrestore(&dma->lock, flags); > + > + if (state != TI_CSI2RX_DMA_STOPPED) { > + ret = ti_csi2rx_drain_dma(csi); > + if (ret) > + dev_dbg(csi->dev, > + "Failed to drain DMA. Next frame might be bogus\n"); > + dmaengine_terminate_sync(csi->dma.chan); > + } > + > + if (curr) > + vb2_buffer_done(&curr->vb.vb2_buf, buf_state); > +} > + > +static int ti_csi2rx_restart_dma(struct ti_csi2rx_dev *csi, > + struct ti_csi2rx_buffer *buf) > +{ > + struct ti_csi2rx_dma *dma = &csi->dma; > + unsigned long flags = 0; > + int ret = 0; > + > + ret = ti_csi2rx_drain_dma(csi); > + if (ret) > + dev_warn(csi->dev, > + "Failed to drain DMA. Next frame might be bogus\n"); > + > + ret = ti_csi2rx_start_dma(csi, buf); > + if (ret) { > + dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > + spin_lock_irqsave(&dma->lock, flags); > + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > + dma->curr = NULL; > + dma->state = TI_CSI2RX_DMA_IDLE; > + spin_unlock_irqrestore(&dma->lock, flags); > + } > + > + return ret; > } > > static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int > *nbuffers, > @@ -622,6 +714,7 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer > *vb) > struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue); > struct ti_csi2rx_buffer *buf; > struct ti_csi2rx_dma *dma = &csi->dma; > + bool restart_dma = false; > unsigned long flags = 0; > int ret; > > @@ -634,21 +727,30 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer > *vb) > * But if DMA has stalled due to lack of buffers, restart it now. > */ > if (dma->state == TI_CSI2RX_DMA_IDLE) { > - ret = ti_csi2rx_start_dma(csi, buf); > - if (ret) { > - dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED); > - goto unlock; > - } > - > + /* > + * Do not restart DMA with the lock held because > + * ti_csi2rx_drain_dma() might block when allocating a buffer. > + * There won't be a race on queueing DMA anyway since the > + * callback is not being fired. > + */ > + restart_dma = true; > dma->curr = buf; > dma->state = TI_CSI2RX_DMA_ACTIVE; > } else { > list_add_tail(&buf->list, &dma->queue); > } > - > -unlock: > spin_unlock_irqrestore(&dma->lock, flags); > + > + if (restart_dma) { > + /* > + * Once frames start dropping, some data gets stuck in the DMA > + * pipeline somewhere. So the first DMA transfer after frame > + * drops gives a partial frame. This is obviously not useful to > + * the application and will only confuse it. Issue a DMA > + * transaction to drain that up. > + */ > + ti_csi2rx_restart_dma(csi, buf); > + } > } > > static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int > count) > @@ -718,12 +820,6 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue > *vq) > > writel(0, csi->shim + SHIM_CNTL); > > - ret = dmaengine_terminate_sync(csi->dma.chan); > - if (ret) > - dev_err(csi->dev, "Failed to stop DMA: %d\n", ret); > - > - writel(0, csi->shim + SHIM_DMACNTX); > - > ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_ERROR); > } > > -- > 2.41.0 > > > [1] TI BSP kernel : https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c?h=ti-linux-6.1.y-cicd > > -- > Julien Massot > Senior Software Engineer > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 -- Jai GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145