| Message ID | 20240627-multistream-v2-0-6ae96c54c1c3@ti.com |
|---|---|
| Headers | show |
| Series | media: cadence,ti: CSI2RX Multistream Support | expand |
Hi, On 27/06/2024 16:09, Jai Luthra wrote: > From: Jayshri Pawar <jpawar@cadence.com> > > Use runtime power management hooks to save power when CSI-RX is not in > use. Also stop/start any in-progress streams, which might happen during > a system suspend/resume cycle. > > Signed-off-by: Jayshri Pawar <jpawar@cadence.com> > Co-developed-by: Jai Luthra <j-luthra@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 43 +++++++++++++++++++++++++++- > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 6f7d27a48eff..751eadbe61ef 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -366,6 +366,12 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > int ret = 0; > > + if (enable) { > + ret = pm_runtime_resume_and_get(csi2rx->dev); > + if (ret < 0) > + return ret; > + } > + > mutex_lock(&csi2rx->lock); > > if (enable) { > @@ -375,8 +381,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > */ > if (!csi2rx->count) { > ret = csi2rx_start(csi2rx); > - if (ret) > + if (ret) { > + pm_runtime_put(csi2rx->dev); > goto out; > + } > } > > csi2rx->count++; > @@ -388,6 +396,8 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > */ > if (!csi2rx->count) > csi2rx_stop(csi2rx); > + > + pm_runtime_put(csi2rx->dev); > } > > out: > @@ -661,6 +671,29 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx) > return ret; > } > > +static int csi2rx_suspend(struct device *dev) > +{ > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > + > + mutex_lock(&csi2rx->lock); > + if (csi2rx->count) > + csi2rx_stop(csi2rx); > + mutex_unlock(&csi2rx->lock); > + > + return 0; > +} > + > +static int csi2rx_resume(struct device *dev) > +{ > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > + > + mutex_lock(&csi2rx->lock); > + if (csi2rx->count) > + csi2rx_start(csi2rx); > + mutex_unlock(&csi2rx->lock); > + return 0; > +} > + I don't think this looks correct. Afaik the runtime suspend/resume is not called on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to use the same callbacks for runtime and system suspend, but I think that's a bad idea. Runtime suspend is not supposed to turn off the streaming. The driver is supposed to turn off the streaming, then call runtime_put, which would result in runtime suspend callback getting called. Tomi > static int csi2rx_probe(struct platform_device *pdev) > { > struct csi2rx_priv *csi2rx; > @@ -707,6 +740,7 @@ static int csi2rx_probe(struct platform_device *pdev) > if (ret) > goto err_cleanup; > > + pm_runtime_enable(csi2rx->dev); > ret = v4l2_async_register_subdev(&csi2rx->subdev); > if (ret < 0) > goto err_free_state; > @@ -721,6 +755,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > err_free_state: > v4l2_subdev_cleanup(&csi2rx->subdev); > + pm_runtime_disable(csi2rx->dev); > err_cleanup: > v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > @@ -739,9 +774,14 @@ static void csi2rx_remove(struct platform_device *pdev) > v4l2_async_unregister_subdev(&csi2rx->subdev); > v4l2_subdev_cleanup(&csi2rx->subdev); > media_entity_cleanup(&csi2rx->subdev.entity); > + pm_runtime_disable(csi2rx->dev); > kfree(csi2rx); > } > > +static const struct dev_pm_ops csi2rx_pm_ops = { > + SET_RUNTIME_PM_OPS(csi2rx_suspend, csi2rx_resume, NULL) > +}; > + > static const struct of_device_id csi2rx_of_table[] = { > { .compatible = "starfive,jh7110-csi2rx" }, > { .compatible = "cdns,csi2rx" }, > @@ -756,6 +796,7 @@ static struct platform_driver csi2rx_driver = { > .driver = { > .name = "cdns-csi2rx", > .of_match_table = csi2rx_of_table, > + .pm = &csi2rx_pm_ops, > }, > }; > module_platform_driver(csi2rx_driver); >
Hi Tomi, [...] > > +static int csi2rx_suspend(struct device *dev) { > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_stop(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + > > + return 0; > > +} > > + > > +static int csi2rx_resume(struct device *dev) { > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > + > > + mutex_lock(&csi2rx->lock); > > + if (csi2rx->count) > > + csi2rx_start(csi2rx); > > + mutex_unlock(&csi2rx->lock); > > + return 0; > > +} > > + > > I don't think this looks correct. Afaik the runtime suspend/resume is not called > on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to > use the same callbacks for runtime and system suspend, but I think that's a > bad idea. Runtime suspend is not supposed to turn off the streaming. The > driver is supposed to turn off the streaming, then call runtime_put, which > would result in runtime suspend callback getting called. > I implemented system suspend/resume based on this patch, I feel good about it. https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@starfivetech.com/ Regards, Changhuang
Hi Changhuang, On Jun 28, 2024 at 08:45:06 +0000, Changhuang Liang wrote: > Hi Tomi, > > [...] > > > +static int csi2rx_suspend(struct device *dev) { > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > + > > > + mutex_lock(&csi2rx->lock); > > > + if (csi2rx->count) > > > + csi2rx_stop(csi2rx); > > > + mutex_unlock(&csi2rx->lock); > > > + > > > + return 0; > > > +} > > > + > > > +static int csi2rx_resume(struct device *dev) { > > > + struct csi2rx_priv *csi2rx = dev_get_drvdata(dev); > > > + > > > + mutex_lock(&csi2rx->lock); > > > + if (csi2rx->count) > > > + csi2rx_start(csi2rx); > > > + mutex_unlock(&csi2rx->lock); > > > + return 0; > > > +} > > > + > > > > I don't think this looks correct. Afaik the runtime suspend/resume is not called > > on system suspend/resume. You could change the SET_RUNTIME_PM_OPS to > > use the same callbacks for runtime and system suspend, but I think that's a > > bad idea. Runtime suspend is not supposed to turn off the streaming. The > > driver is supposed to turn off the streaming, then call runtime_put, which > > would result in runtime suspend callback getting called. > > > > I implemented system suspend/resume based on this patch, I feel good about it. > > https://lore.kernel.org/all/20240326031237.25331-1-changhuang.liang@starfivetech.com/ Thanks for carrying this patch in your series. I think Tomi's point still holds - only the system suspend hook should try to stop/start the CSI2RX device. Runtime PM hooks are usually only called when there are no users, so no active streams. > > Regards, > Changhuang
On 28/06/2024 12:35, Jai Luthra wrote: > Hi Tomi, > > On Jun 28, 2024 at 11:26:59 +0300, Tomi Valkeinen wrote: >> Hi Jai, >> >> On 27/06/2024 16:09, Jai Luthra wrote: >>> This series adds multi-stream support for Cadence CSI2RX and TI CSI2RX >>> Shim drivers. >>> >>> PATCH 1: Runtime Power Management for Cadence CSI2RX >>> PATCH 2-7: Support multiple DMA contexts/video nodes in TI CSI2RX >>> PATCH 8-9: Use get_frame_desc to propagate virtual channel information >>> across Cadence and TI CSI-RX subdevs >>> PATCH 10-12: Use new multi-stream APIs across the drivers to support >>> multiplexed cameras from sources like UB960 (FPDLink) >>> PATCH 13: Optimize stream on by submitting all queued buffers to DMA >>> >>> This applies on top of today's linux-next (next-20240626) > > This series is based on top of next-20240626 > >>> (also tested rebase with media_stage.git master) >>> >>> Signed-off-by: Jai Luthra <j-luthra@ti.com> >>> --- >>> Changes in v2: >>> >>> - Change the multi-camera capture architecture to be similar to that of >>> Tomi's RPi5 FE series, where the driver will wait for userspace to >>> start streaming on all "actively routed" video nodes before starting >>> streaming on the source. This simplifies things a lot from the HW >>> perspective, which might run into deadlocks due to a shared FIFO >>> between multiple DMA channels. >>> >>> - Drop a few fixes that were posted separately and are already merged >>> - Fix dtschema warnings reported by Rob on [02/13] >>> - Fix warnings for uninitialized `used_vc` variable in cdns-csi2rx.c >>> - Return -EBUSY if someone updates routes for j721e-csi2rx subdev while >>> streaming >>> - Only allow single-streams to be routed to the source pads (linked to >>> video nodes) of the j721e-csi2rx device >>> - Squash the patches marked "SQUASH" in the v1 RFC series >>> >>> - Link to RFC (v1): >>> https://lore.kernel.org/r/20240222-multistream-v1-0-1837ed916eeb@ti.com >>> >>> --- >>> Jai Luthra (8): >>> dt-bindings: media: ti,j721e-csi2rx-shim: Support 32 dma chans >>> media: ti: j721e-csi2rx: separate out device and context >>> media: ti: j721e-csi2rx: add a subdev for the core device >>> media: ti: j721e-csi2rx: add support for processing virtual channels >>> media: cadence: csi2rx: Use new enable stream APIs >>> media: cadence: csi2rx: Enable multi-stream support >>> media: ti: j721e-csi2rx: add multistream support >>> media: ti: j721e-csi2rx: Submit all available buffers >>> >>> Jayshri Pawar (1): >>> media: cadence: csi2rx: Support runtime PM >>> >>> Pratyush Yadav (4): >>> media: ti: j721e-csi2rx: prepare SHIM code for multiple contexts >>> media: ti: j721e-csi2rx: allocate DMA channel based on context index >>> media: ti: j721e-csi2rx: get number of contexts from device tree >>> media: cadence: csi2rx: add get_frame_desc wrapper >>> >>> .../bindings/media/ti,j721e-csi2rx-shim.yaml | 39 +- >>> drivers/media/platform/cadence/cdns-csi2rx.c | 440 +++++++++-- >>> .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 879 ++++++++++++++++----- >>> 3 files changed, 1071 insertions(+), 287 deletions(-) >>> --- >>> base-commit: df9574a57d02b265322e77fb8628d4d33641dda9 >>> change-id: 20240221-multistream-fbba6ffe47a3 >> >> You have based this series on top of your private branch. Don't do that. >> Base on top of a kernel tag, or a commonly known tree (linux-media-stage for >> example), and preferably mention the base in the cover letter. > > The base commit SHA populated by b4 is the same as next-20240626 as > mentioned above Ah, right, my bad. I took your branch https://github.com/jailuthra/linux/commits/b4/multistream/, and assumed it's the one you used to send these patches. In that branch these patches are not based on linux-next. > https://gitlab.com/linux-kernel/linux-next/-/commits/df9574a57d02b265322e77fb8628d4d33641dda9 > > I chose to not use media-stage as the base, but this series applies > cleanly (and compiles) on top of that as well. I'd still recommend media-stage, as that's where these patches would be merged (or just linux-media). linux-next is good for testing, but I wouldn't normally base patches on top of that, or at last send patches based on that. >> Your private branch contains e.g. dtsos needed for testing. If you have such >> a branch, you should point to it in the cover letter as it's valuable for >> reviewers/testers. > > Ah my bad, I missed mentioning my github branch that can be used for > testing the content of this series. It contains some DTSOs and defconfig > updates, along with support for FPDLink/V3Link sensors. > > https://github.com/jailuthra/linux/commits/b4/multistream/ Jfyi, I've tested this with am62a and arducam's fpdlink board with imx219 sensors, and works fine for me. I only tested with pixel streams, I'd like to also add all the patches needed for embedded data and test that (I think all of those have been posted to the lists), but I don't think I'll have time for that right now. Tomi >> Only base on top of a private branch if your patches compile-time depend on >> something from there, and in that case point to the branch and mention this >> dependency clearly in the cover letter. > > Makes sense, will take extra care to mention the dependencies and base > branch from next version. > >> >> Tomi >> >
On Jun 28, 2024 at 12:53:04 +0300, Tomi Valkeinen wrote: > On 28/06/2024 12:35, Jai Luthra wrote: > > Hi Tomi, > > > > On Jun 28, 2024 at 11:26:59 +0300, Tomi Valkeinen wrote: > > > Hi Jai, > > > > > > On 27/06/2024 16:09, Jai Luthra wrote: > > > > This series adds multi-stream support for Cadence CSI2RX and TI CSI2RX > > > > Shim drivers. > > > > > > > > PATCH 1: Runtime Power Management for Cadence CSI2RX > > > > PATCH 2-7: Support multiple DMA contexts/video nodes in TI CSI2RX > > > > PATCH 8-9: Use get_frame_desc to propagate virtual channel information > > > > across Cadence and TI CSI-RX subdevs > > > > PATCH 10-12: Use new multi-stream APIs across the drivers to support > > > > multiplexed cameras from sources like UB960 (FPDLink) > > > > PATCH 13: Optimize stream on by submitting all queued buffers to DMA > > > > > > > > This applies on top of today's linux-next (next-20240626) > > > > This series is based on top of next-20240626 > > > > > > (also tested rebase with media_stage.git master) > > > > > > > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > > > --- > > > > Changes in v2: > > > > > > > > - Change the multi-camera capture architecture to be similar to that of > > > > Tomi's RPi5 FE series, where the driver will wait for userspace to > > > > start streaming on all "actively routed" video nodes before starting > > > > streaming on the source. This simplifies things a lot from the HW > > > > perspective, which might run into deadlocks due to a shared FIFO > > > > between multiple DMA channels. > > > > > > > > - Drop a few fixes that were posted separately and are already merged > > > > - Fix dtschema warnings reported by Rob on [02/13] > > > > - Fix warnings for uninitialized `used_vc` variable in cdns-csi2rx.c > > > > - Return -EBUSY if someone updates routes for j721e-csi2rx subdev while > > > > streaming > > > > - Only allow single-streams to be routed to the source pads (linked to > > > > video nodes) of the j721e-csi2rx device > > > > - Squash the patches marked "SQUASH" in the v1 RFC series > > > > > > > > - Link to RFC (v1): > > > > https://lore.kernel.org/r/20240222-multistream-v1-0-1837ed916eeb@ti.com > > > > > > > > --- > > > > Jai Luthra (8): > > > > dt-bindings: media: ti,j721e-csi2rx-shim: Support 32 dma chans > > > > media: ti: j721e-csi2rx: separate out device and context > > > > media: ti: j721e-csi2rx: add a subdev for the core device > > > > media: ti: j721e-csi2rx: add support for processing virtual channels > > > > media: cadence: csi2rx: Use new enable stream APIs > > > > media: cadence: csi2rx: Enable multi-stream support > > > > media: ti: j721e-csi2rx: add multistream support > > > > media: ti: j721e-csi2rx: Submit all available buffers > > > > > > > > Jayshri Pawar (1): > > > > media: cadence: csi2rx: Support runtime PM > > > > > > > > Pratyush Yadav (4): > > > > media: ti: j721e-csi2rx: prepare SHIM code for multiple contexts > > > > media: ti: j721e-csi2rx: allocate DMA channel based on context index > > > > media: ti: j721e-csi2rx: get number of contexts from device tree > > > > media: cadence: csi2rx: add get_frame_desc wrapper > > > > > > > > .../bindings/media/ti,j721e-csi2rx-shim.yaml | 39 +- > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 440 +++++++++-- > > > > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 879 ++++++++++++++++----- > > > > 3 files changed, 1071 insertions(+), 287 deletions(-) > > > > --- > > > > base-commit: df9574a57d02b265322e77fb8628d4d33641dda9 > > > > change-id: 20240221-multistream-fbba6ffe47a3 > > > > > > You have based this series on top of your private branch. Don't do that. > > > Base on top of a kernel tag, or a commonly known tree (linux-media-stage for > > > example), and preferably mention the base in the cover letter. > > > > The base commit SHA populated by b4 is the same as next-20240626 as > > mentioned above > > Ah, right, my bad. I took your branch > https://github.com/jailuthra/linux/commits/b4/multistream/, and assumed it's > the one you used to send these patches. In that branch these patches are not > based on linux-next. Ah, yes I cherry-picked them in after posting. > > > https://gitlab.com/linux-kernel/linux-next/-/commits/df9574a57d02b265322e77fb8628d4d33641dda9 > > > > I chose to not use media-stage as the base, but this series applies > > cleanly (and compiles) on top of that as well. > > I'd still recommend media-stage, as that's where these patches would be > merged (or just linux-media). linux-next is good for testing, but I wouldn't > normally base patches on top of that, or at last send patches based on that. Understood, will use media-stage while posting future revisions. > > > > Your private branch contains e.g. dtsos needed for testing. If you have such > > > a branch, you should point to it in the cover letter as it's valuable for > > > reviewers/testers. > > > > Ah my bad, I missed mentioning my github branch that can be used for > > testing the content of this series. It contains some DTSOs and defconfig > > updates, along with support for FPDLink/V3Link sensors. > > > > https://github.com/jailuthra/linux/commits/b4/multistream/ > > Jfyi, I've tested this with am62a and arducam's fpdlink board with imx219 > sensors, and works fine for me. Neat! Thanks for testing it out. > > I only tested with pixel streams, I'd like to also add all the patches > needed for embedded data and test that (I think all of those have been > posted to the lists), but I don't think I'll have time for that right now. I see, I haven't been following the recent series adding support for internal pads and embedded data in IMX219. I will try to merge those in and see if I can get something working. > > Tomi > > > > Only base on top of a private branch if your patches compile-time depend on > > > something from there, and in that case point to the branch and mention this > > > dependency clearly in the cover letter. > > > > Makes sense, will take extra care to mention the dependencies and base > > branch from next version. > > > > > > > > Tomi > > > > > >
Thanks for the review. On Jun 28, 2024 at 13:42:01 +0300, Tomi Valkeinen wrote: > On 27/06/2024 16:09, Jai Luthra wrote: > > The CSI2RX SHIM IP can support a maximum of 32x DMA channels. > > > > These can be used to split incoming "streams" of data on the CSI-RX > > port, distinguished by MIPI Virtual Channel (or Data Type), into > > different locations in memory (/dev/videoX nodes). > > Usually you shouldn't talk about Linux specifics in DT bindings. The DT > bindings are only about the HW, and the OS doesn't matter. It doesn't really > matter much, but I'd just leave out the mention to /dev/videoX. My bad, will drop the reference to /dev/videoX in next revision. > > > Actual number of DMA channels reserved is different for each SoC > > integrating this IP, but a maximum of 32x channels are always available > > in this IP's register space, so set minimum as 1 and maximum as 32. > > So in the SoC's dts file you will set the number of channels to the maximum > supported by that SoC? I guess that's fine. > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > Tomi > > [...] >
On 27/06/2024 15:09, Jai Luthra wrote: > The CSI2RX SHIM IP can support a maximum of 32x DMA channels. > > These can be used to split incoming "streams" of data on the CSI-RX > port, distinguished by MIPI Virtual Channel (or Data Type), into > different locations in memory (/dev/videoX nodes). > > Actual number of DMA channels reserved is different for each SoC > integrating this IP, but a maximum of 32x channels are always available > in this IP's register space, so set minimum as 1 and maximum as 32. > > Link: https://www.ti.com/lit/pdf/spruiv7 > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > .../bindings/media/ti,j721e-csi2rx-shim.yaml | 39 ++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml b/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > index f762fdc05e4d..0e00533c7b68 100644 > --- a/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > +++ b/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > @@ -20,11 +20,44 @@ properties: > const: ti,j721e-csi2rx-shim > > dmas: > - maxItems: 1 > + minItems: 1 > + maxItems: 32 > > dma-names: > + minItems: 1 > items: > - const: rx0 So all variants now get total random number of DMAs? I don't understand why this is not constrained. Best regards, Krzysztof
Hi Krzysztof, Thanks for the review. On Jul 01, 2024 at 11:30:13 +0200, Krzysztof Kozlowski wrote: > On 27/06/2024 15:09, Jai Luthra wrote: > > The CSI2RX SHIM IP can support a maximum of 32x DMA channels. > > > > These can be used to split incoming "streams" of data on the CSI-RX > > port, distinguished by MIPI Virtual Channel (or Data Type), into > > different locations in memory (/dev/videoX nodes). > > > > Actual number of DMA channels reserved is different for each SoC > > integrating this IP, but a maximum of 32x channels are always available > > in this IP's register space, so set minimum as 1 and maximum as 32. Sorry, this above paragraph is incorrect. All variants of SoCs integrating this IP can in-fact support 32 channels. I will update the commit description in the next revision to make this clearer. > > > > Link: https://www.ti.com/lit/pdf/spruiv7 > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > .../bindings/media/ti,j721e-csi2rx-shim.yaml | 39 ++++++++++++++++++++-- > > 1 file changed, 36 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml b/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > > index f762fdc05e4d..0e00533c7b68 100644 > > --- a/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > > +++ b/Documentation/devicetree/bindings/media/ti,j721e-csi2rx-shim.yaml > > @@ -20,11 +20,44 @@ properties: > > const: ti,j721e-csi2rx-shim > > > > dmas: > > - maxItems: 1 > > + minItems: 1 > > + maxItems: 32 > > > > dma-names: > > + minItems: 1 > > items: > > - const: rx0 > > So all variants now get total random number of DMAs? I don't understand > why this is not constrained. The default system-level resource partitioning allocates < 32 channels for some platforms, though that can be changed by the user [1]. I don't think a separate compatible for constraints makes sense here, as this is not an IP or SoC-level constraint, and only depends on how the end-user allocates the channels across peripherals. > > Best regards, > Krzysztof > [1] https://software-dl.ti.com/processor-sdk-linux/esd/AM62X/latest/exports/docs/linux/How_to_Guides/Host/K3_Resource_Partitioning_Tool.html
Hi Jai On Thu, Jun 27, 2024 at 06:39:58PM GMT, Jai Luthra wrote: > The TI CSI2RX wrapper has two parts: the main device and the DMA > contexts. The driver was originally written with single camera capture > in mind, so only one DMA context was needed. For the sake of simplicity, > the context specific stuff was not modeled different to the main device. > > To enable multiplexed stream capture, the contexts need to be separated > out from the main device. Create a struct ti_csi2rx_ctx that holds the > DMA context specific things. Separate out functions handling the device > and context related functionality. > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 423 ++++++++++++--------- > 1 file changed, 242 insertions(+), 181 deletions(-) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index 22442fce7607..d8dfe0002b72 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -40,6 +40,8 @@ > #define SHIM_PSI_CFG0_DST_TAG GENMASK(31, 16) > > #define PSIL_WORD_SIZE_BYTES 16 > +#define TI_CSI2RX_NUM_CTX 1 > + > /* > * There are no hard limits on the width or height. The DMA engine can handle > * all sizes. The max width and height are arbitrary numbers for this driver. > @@ -64,7 +66,7 @@ struct ti_csi2rx_buffer { > /* Common v4l2 buffer. Must be first. */ > struct vb2_v4l2_buffer vb; > struct list_head list; > - struct ti_csi2rx_dev *csi; > + struct ti_csi2rx_ctx *ctx; > }; > > enum ti_csi2rx_dma_state { > @@ -84,29 +86,37 @@ struct ti_csi2rx_dma { > * Queue of buffers submitted to DMA engine. > */ > struct list_head submitted; > - /* Buffer to drain stale data from PSI-L endpoint */ > - struct { > - void *vaddr; > - dma_addr_t paddr; > - size_t len; > - } drain; > +}; > + > +struct ti_csi2rx_dev; > + > +struct ti_csi2rx_ctx { > + struct ti_csi2rx_dev *csi; > + struct video_device vdev; > + struct vb2_queue vidq; > + struct mutex mutex; /* To serialize ioctls. */ > + struct v4l2_format v_fmt; > + struct ti_csi2rx_dma dma; > + u32 sequence; > + u32 idx; > }; > > struct ti_csi2rx_dev { > struct device *dev; > void __iomem *shim; > struct v4l2_device v4l2_dev; > - struct video_device vdev; > struct media_device mdev; > struct media_pipeline pipe; > struct media_pad pad; > struct v4l2_async_notifier notifier; > struct v4l2_subdev *source; > - struct vb2_queue vidq; > - struct mutex mutex; /* To serialize ioctls. */ > - struct v4l2_format v_fmt; > - struct ti_csi2rx_dma dma; > - u32 sequence; > + struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX]; > + /* Buffer to drain stale data from PSI-L endpoint */ > + struct { > + void *vaddr; > + dma_addr_t paddr; > + size_t len; > + } drain; > }; > > static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = { > @@ -212,7 +222,7 @@ static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = { > }; > > /* Forward declaration needed by ti_csi2rx_dma_callback. */ > -static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > +static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx, > struct ti_csi2rx_buffer *buf); > > static const struct ti_csi2rx_fmt *find_format_by_fourcc(u32 pixelformat) > @@ -302,7 +312,7 @@ static int ti_csi2rx_enum_fmt_vid_cap(struct file *file, void *priv, > static int ti_csi2rx_g_fmt_vid_cap(struct file *file, void *prov, > struct v4l2_format *f) > { > - struct ti_csi2rx_dev *csi = video_drvdata(file); > + struct ti_csi2rx_ctx *csi = video_drvdata(file); > > *f = csi->v_fmt; > > @@ -333,7 +343,7 @@ static int ti_csi2rx_try_fmt_vid_cap(struct file *file, void *priv, > static int ti_csi2rx_s_fmt_vid_cap(struct file *file, void *priv, > struct v4l2_format *f) > { > - struct ti_csi2rx_dev *csi = video_drvdata(file); > + struct ti_csi2rx_ctx *csi = video_drvdata(file); > struct vb2_queue *q = &csi->vidq; > int ret; > > @@ -419,25 +429,33 @@ static int csi_async_notifier_bound(struct v4l2_async_notifier *notifier, > static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > { > struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev); > - struct video_device *vdev = &csi->vdev; > - int ret; > + int ret, i; > > - ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > - if (ret) > - return ret; > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > + struct ti_csi2rx_ctx *ctx = &csi->ctx[i]; > + struct video_device *vdev = &ctx->vdev; > > - ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > - MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED); > - > - if (ret) { > - video_unregister_device(vdev); > - return ret; > + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > + if (ret) > + goto unregister_dev; > } > > + ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > + MEDIA_LNK_FL_IMMUTABLE | > + MEDIA_LNK_FL_ENABLED); > + if (ret) > + goto unregister_dev; > + > ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev); > if (ret) > - video_unregister_device(vdev); > + goto unregister_dev; > + > + return 0; > > +unregister_dev: > + i--; > + for (; i >= 0; i--) > + video_unregister_device(&csi->ctx[i].vdev); > return ret; > } > > @@ -483,12 +501,13 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi) > return 0; > } > > -static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi) > +static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx) > { > + struct ti_csi2rx_dev *csi = ctx->csi; > const struct ti_csi2rx_fmt *fmt; > unsigned int reg; > > - fmt = find_format_by_fourcc(csi->v_fmt.fmt.pix.pixelformat); > + fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat); > > /* De-assert the pixel interface reset. */ > reg = SHIM_CNTL_PIX_RST; > @@ -555,8 +574,9 @@ static void ti_csi2rx_drain_callback(void *param) > * To prevent that stale data corrupting the subsequent transactions, it is > * required to issue DMA requests to drain it out. > */ > -static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > +static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx) > { > + struct ti_csi2rx_dev *csi = ctx->csi; > struct dma_async_tx_descriptor *desc; > struct completion drain_complete; > dma_cookie_t cookie; > @@ -564,8 +584,8 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > > init_completion(&drain_complete); > > - desc = dmaengine_prep_slave_single(csi->dma.chan, csi->dma.drain.paddr, > - csi->dma.drain.len, DMA_DEV_TO_MEM, > + desc = dmaengine_prep_slave_single(ctx->dma.chan, csi->drain.paddr, > + csi->drain.len, DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!desc) { > ret = -EIO; > @@ -580,11 +600,11 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > if (ret) > goto out; > > - dma_async_issue_pending(csi->dma.chan); > + dma_async_issue_pending(ctx->dma.chan); > > if (!wait_for_completion_timeout(&drain_complete, > msecs_to_jiffies(DRAIN_TIMEOUT_MS))) { > - dmaengine_terminate_sync(csi->dma.chan); > + dmaengine_terminate_sync(ctx->dma.chan); > dev_dbg(csi->dev, "DMA transfer timed out for drain buffer\n"); > ret = -ETIMEDOUT; > goto out; > @@ -596,8 +616,8 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > static void ti_csi2rx_dma_callback(void *param) > { > struct ti_csi2rx_buffer *buf = param; > - struct ti_csi2rx_dev *csi = buf->csi; > - struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_ctx *ctx = buf->ctx; > + struct ti_csi2rx_dma *dma = &ctx->dma; > unsigned long flags; > > /* > @@ -605,7 +625,7 @@ static void ti_csi2rx_dma_callback(void *param) > * hardware monitor registers. > */ > buf->vb.vb2_buf.timestamp = ktime_get_ns(); > - buf->vb.sequence = csi->sequence++; > + buf->vb.sequence = ctx->sequence++; > > spin_lock_irqsave(&dma->lock, flags); > > @@ -617,8 +637,9 @@ static void ti_csi2rx_dma_callback(void *param) > while (!list_empty(&dma->queue)) { > buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list); > > - if (ti_csi2rx_start_dma(csi, buf)) { > - dev_err(csi->dev, "Failed to queue the next buffer for DMA\n"); > + if (ti_csi2rx_start_dma(ctx, buf)) { > + dev_err(ctx->csi->dev, > + "Failed to queue the next buffer for DMA\n"); > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > } else { > list_move_tail(&buf->list, &dma->submitted); > @@ -631,17 +652,17 @@ static void ti_csi2rx_dma_callback(void *param) > spin_unlock_irqrestore(&dma->lock, flags); > } > > -static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > +static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx, > struct ti_csi2rx_buffer *buf) > { > unsigned long addr; > struct dma_async_tx_descriptor *desc; > - size_t len = csi->v_fmt.fmt.pix.sizeimage; > + size_t len = ctx->v_fmt.fmt.pix.sizeimage; > dma_cookie_t cookie; > int ret = 0; > > addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0); > - desc = dmaengine_prep_slave_single(csi->dma.chan, addr, len, > + desc = dmaengine_prep_slave_single(ctx->dma.chan, addr, len, > DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!desc) > @@ -655,20 +676,20 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > if (ret) > return ret; > > - dma_async_issue_pending(csi->dma.chan); > + dma_async_issue_pending(ctx->dma.chan); > > return 0; > } > > -static void ti_csi2rx_stop_dma(struct ti_csi2rx_dev *csi) > +static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx) > { > - struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_dma *dma = &ctx->dma; > enum ti_csi2rx_dma_state state; > unsigned long flags; > int ret; > > spin_lock_irqsave(&dma->lock, flags); > - state = csi->dma.state; > + state = ctx->dma.state; > dma->state = TI_CSI2RX_DMA_STOPPED; > spin_unlock_irqrestore(&dma->lock, flags); > > @@ -679,30 +700,30 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_dev *csi) > * is stopped, as the module-level pixel reset cannot be > * enforced before terminating DMA. > */ > - ret = ti_csi2rx_drain_dma(csi); > + ret = ti_csi2rx_drain_dma(ctx); > if (ret && ret != -ETIMEDOUT) > - dev_warn(csi->dev, > + dev_warn(ctx->csi->dev, > "Failed to drain DMA. Next frame might be bogus\n"); > } > > - ret = dmaengine_terminate_sync(csi->dma.chan); > + ret = dmaengine_terminate_sync(ctx->dma.chan); > if (ret) > - dev_err(csi->dev, "Failed to stop DMA: %d\n", ret); > + dev_err(ctx->csi->dev, "Failed to stop DMA: %d\n", ret); > } > > -static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_dev *csi, > +static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_ctx *ctx, > enum vb2_buffer_state state) > { > - struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_dma *dma = &ctx->dma; > struct ti_csi2rx_buffer *buf, *tmp; > unsigned long flags; > > spin_lock_irqsave(&dma->lock, flags); > - list_for_each_entry_safe(buf, tmp, &csi->dma.queue, list) { > + list_for_each_entry_safe(buf, tmp, &ctx->dma.queue, list) { > list_del(&buf->list); > vb2_buffer_done(&buf->vb.vb2_buf, state); > } > - list_for_each_entry_safe(buf, tmp, &csi->dma.submitted, list) { > + list_for_each_entry_safe(buf, tmp, &ctx->dma.submitted, list) { > list_del(&buf->list); > vb2_buffer_done(&buf->vb.vb2_buf, state); > } > @@ -713,8 +734,8 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers, > unsigned int *nplanes, unsigned int sizes[], > struct device *alloc_devs[]) > { > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(q); > - unsigned int size = csi->v_fmt.fmt.pix.sizeimage; > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(q); > + unsigned int size = ctx->v_fmt.fmt.pix.sizeimage; > > if (*nplanes) { > if (sizes[0] < size) > @@ -730,11 +751,11 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers, > > static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb) > { > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue); > - unsigned long size = csi->v_fmt.fmt.pix.sizeimage; > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); > + unsigned long size = ctx->v_fmt.fmt.pix.sizeimage; > > if (vb2_plane_size(vb, 0) < size) { > - dev_err(csi->dev, "Data will not fit into plane\n"); > + dev_err(ctx->csi->dev, "Data will not fit into plane\n"); > return -EINVAL; > } > > @@ -744,15 +765,15 @@ static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb) > > 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_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); > struct ti_csi2rx_buffer *buf; > - struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_dma *dma = &ctx->dma; > bool restart_dma = false; > unsigned long flags = 0; > int ret; > > buf = container_of(vb, struct ti_csi2rx_buffer, vb.vb2_buf); > - buf->csi = csi; > + buf->ctx = ctx; > > spin_lock_irqsave(&dma->lock, flags); > /* > @@ -781,18 +802,18 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) > * the application and will only confuse it. Issue a DMA > * transaction to drain that up. > */ > - ret = ti_csi2rx_drain_dma(csi); > + ret = ti_csi2rx_drain_dma(ctx); > if (ret && ret != -ETIMEDOUT) > - dev_warn(csi->dev, > + dev_warn(ctx->csi->dev, > "Failed to drain DMA. Next frame might be bogus\n"); > > spin_lock_irqsave(&dma->lock, flags); > - ret = ti_csi2rx_start_dma(csi, buf); > + ret = ti_csi2rx_start_dma(ctx, buf); > if (ret) { > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > dma->state = TI_CSI2RX_DMA_IDLE; > spin_unlock_irqrestore(&dma->lock, flags); > - dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > + dev_err(ctx->csi->dev, "Failed to start DMA: %d\n", ret); > } else { > list_add_tail(&buf->list, &dma->submitted); > spin_unlock_irqrestore(&dma->lock, flags); > @@ -802,8 +823,9 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) > > static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > { > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > - struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq); > + struct ti_csi2rx_dev *csi = ctx->csi; > + struct ti_csi2rx_dma *dma = &ctx->dma; > struct ti_csi2rx_buffer *buf; > unsigned long flags; > int ret = 0; > @@ -815,18 +837,18 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > if (ret) > return ret; > > - ret = video_device_pipeline_start(&csi->vdev, &csi->pipe); > + ret = video_device_pipeline_start(&ctx->vdev, &csi->pipe); > if (ret) > goto err; > > - ti_csi2rx_setup_shim(csi); > + ti_csi2rx_setup_shim(ctx); > > - csi->sequence = 0; > + ctx->sequence = 0; > > spin_lock_irqsave(&dma->lock, flags); > buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list); > > - ret = ti_csi2rx_start_dma(csi, buf); > + ret = ti_csi2rx_start_dma(ctx, buf); > if (ret) { > dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > spin_unlock_irqrestore(&dma->lock, flags); > @@ -844,22 +866,23 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > return 0; > > err_dma: > - ti_csi2rx_stop_dma(csi); > + ti_csi2rx_stop_dma(ctx); > err_pipeline: > - video_device_pipeline_stop(&csi->vdev); > + video_device_pipeline_stop(&ctx->vdev); > writel(0, csi->shim + SHIM_CNTL); > writel(0, csi->shim + SHIM_DMACNTX); > err: > - ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_QUEUED); > + ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_QUEUED); > return ret; > } > > static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > { > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq); > + struct ti_csi2rx_dev *csi = ctx->csi; > int ret; > > - video_device_pipeline_stop(&csi->vdev); > + video_device_pipeline_stop(&ctx->vdev); > > writel(0, csi->shim + SHIM_CNTL); > writel(0, csi->shim + SHIM_DMACNTX); > @@ -868,8 +891,8 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > if (ret) > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > - ti_csi2rx_stop_dma(csi); > - ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_ERROR); > + ti_csi2rx_stop_dma(ctx); > + ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_ERROR); > } > > static const struct vb2_ops csi_vb2_qops = { > @@ -882,27 +905,60 @@ static const struct vb2_ops csi_vb2_qops = { > .wait_finish = vb2_ops_wait_finish, > }; > > -static int ti_csi2rx_init_vb2q(struct ti_csi2rx_dev *csi) > +static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) Is it worth a wrapper function ? > { > - struct vb2_queue *q = &csi->vidq; > + dma_release_channel(ctx->dma.chan); > +} > + > +static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > +{ > + media_device_unregister(&csi->mdev); > + v4l2_device_unregister(&csi->v4l2_dev); > + media_device_cleanup(&csi->mdev); > +} > + > +static void ti_csi2rx_cleanup_notifier(struct ti_csi2rx_dev *csi) > +{ > + v4l2_async_nf_unregister(&csi->notifier); > + v4l2_async_nf_cleanup(&csi->notifier); > +} > + > +static void ti_csi2rx_cleanup_vb2q(struct ti_csi2rx_ctx *ctx) Has a single caller, can be inlined maybe ? > +{ > + vb2_queue_release(&ctx->vidq); > +} > + > +static void ti_csi2rx_cleanup_ctx(struct ti_csi2rx_ctx *ctx) > +{ > + ti_csi2rx_cleanup_dma(ctx); > + ti_csi2rx_cleanup_vb2q(ctx); So this would become dma_release_channel(ctx->dma.chan); vb2_queue_release(&ctx->vidq); > + > + video_unregister_device(&ctx->vdev); > + > + mutex_destroy(&ctx->mutex); > +} > + > +static int ti_csi2rx_init_vb2q(struct ti_csi2rx_ctx *ctx) > +{ > + struct vb2_queue *q = &ctx->vidq; > int ret; > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > q->io_modes = VB2_MMAP | VB2_DMABUF; > - q->drv_priv = csi; > + q->drv_priv = ctx; > q->buf_struct_size = sizeof(struct ti_csi2rx_buffer); > q->ops = &csi_vb2_qops; > q->mem_ops = &vb2_dma_contig_memops; > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > - q->dev = dmaengine_get_dma_device(csi->dma.chan); > - q->lock = &csi->mutex; > + q->dev = dmaengine_get_dma_device(ctx->dma.chan); > + q->lock = &ctx->mutex; > q->min_queued_buffers = 1; > > ret = vb2_queue_init(q); > if (ret) > return ret; > > - csi->vdev.queue = q; > + ctx->vdev.queue = q; > > return 0; > } > @@ -911,8 +967,9 @@ static int ti_csi2rx_link_validate(struct media_link *link) > { > struct media_entity *entity = link->sink->entity; > struct video_device *vdev = media_entity_to_video_device(entity); > - struct ti_csi2rx_dev *csi = container_of(vdev, struct ti_csi2rx_dev, vdev); > - struct v4l2_pix_format *csi_fmt = &csi->v_fmt.fmt.pix; > + struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev); > + struct ti_csi2rx_dev *csi = ctx->csi; > + struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix; > struct v4l2_subdev_format source_fmt = { > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > .pad = link->source->index, > @@ -965,47 +1022,69 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = { > .link_validate = ti_csi2rx_link_validate, > }; > > -static int ti_csi2rx_init_dma(struct ti_csi2rx_dev *csi) > +static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > { > struct dma_slave_config cfg = { > .src_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES, > }; > int ret; > > - INIT_LIST_HEAD(&csi->dma.queue); > - INIT_LIST_HEAD(&csi->dma.submitted); > - spin_lock_init(&csi->dma.lock); > + INIT_LIST_HEAD(&ctx->dma.queue); > + INIT_LIST_HEAD(&ctx->dma.submitted); > + spin_lock_init(&ctx->dma.lock); > > - csi->dma.state = TI_CSI2RX_DMA_STOPPED; > + ctx->dma.state = TI_CSI2RX_DMA_STOPPED; > > - csi->dma.chan = dma_request_chan(csi->dev, "rx0"); > - if (IS_ERR(csi->dma.chan)) > - return PTR_ERR(csi->dma.chan); > + ctx->dma.chan = dma_request_chan(ctx->csi->dev, "rx0"); The channel name should probably be parametrized, I presume it will happen next > + if (IS_ERR(ctx->dma.chan)) > + return PTR_ERR(ctx->dma.chan); > > - ret = dmaengine_slave_config(csi->dma.chan, &cfg); > + ret = dmaengine_slave_config(ctx->dma.chan, &cfg); > if (ret) { > - dma_release_channel(csi->dma.chan); > + dma_release_channel(ctx->dma.chan); > return ret; > } > > - csi->dma.drain.len = DRAIN_BUFFER_SIZE; > - csi->dma.drain.vaddr = dma_alloc_coherent(csi->dev, csi->dma.drain.len, > - &csi->dma.drain.paddr, > - GFP_KERNEL); > - if (!csi->dma.drain.vaddr) > - return -ENOMEM; > - > return 0; > } > > static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > { > struct media_device *mdev = &csi->mdev; > - struct video_device *vdev = &csi->vdev; > + int ret; > + > + mdev->dev = csi->dev; > + mdev->hw_revision = 1; > + strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model)); > + > + media_device_init(mdev); > + > + csi->v4l2_dev.mdev = mdev; > + > + ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > + if (ret) > + return ret; > + > + ret = media_device_register(mdev); > + if (ret) { > + v4l2_device_unregister(&csi->v4l2_dev); > + media_device_cleanup(mdev); > + return ret; > + } > + > + return 0; > +} > + > +static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > +{ > + struct ti_csi2rx_dev *csi = ctx->csi; > + struct video_device *vdev = &ctx->vdev; > const struct ti_csi2rx_fmt *fmt; > - struct v4l2_pix_format *pix_fmt = &csi->v_fmt.fmt.pix; > + struct v4l2_pix_format *pix_fmt = &ctx->v_fmt.fmt.pix; > int ret; > > + mutex_init(&ctx->mutex); > + > fmt = find_format_by_fourcc(V4L2_PIX_FMT_UYVY); > if (!fmt) > return -EINVAL; > @@ -1018,15 +1097,16 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE, > pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB, > > - ti_csi2rx_fill_fmt(fmt, &csi->v_fmt); > - > - mdev->dev = csi->dev; > - mdev->hw_revision = 1; > - strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model)); > + ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt); > > - media_device_init(mdev); > + csi->pad.flags = MEDIA_PAD_FL_SINK; > + vdev->entity.ops = &ti_csi2rx_video_entity_ops; > + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad); > + if (ret) > + return ret; > > - strscpy(vdev->name, TI_CSI2RX_MODULE_NAME, sizeof(vdev->name)); > + snprintf(vdev->name, sizeof(vdev->name), "%s context %u", > + dev_name(csi->dev), ctx->idx); > vdev->v4l2_dev = &csi->v4l2_dev; > vdev->vfl_dir = VFL_DIR_RX; > vdev->fops = &csi_fops; > @@ -1034,61 +1114,28 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > vdev->release = video_device_release_empty; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > V4L2_CAP_IO_MC; > - vdev->lock = &csi->mutex; > - video_set_drvdata(vdev, csi); > + vdev->lock = &ctx->mutex; > + video_set_drvdata(vdev, ctx); > > - csi->pad.flags = MEDIA_PAD_FL_SINK; > - vdev->entity.ops = &ti_csi2rx_video_entity_ops; > - ret = media_entity_pads_init(&csi->vdev.entity, 1, &csi->pad); > + ret = ti_csi2rx_init_dma(ctx); > if (ret) > return ret; > > - csi->v4l2_dev.mdev = mdev; > - > - ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > + ret = ti_csi2rx_init_vb2q(ctx); > if (ret) > - return ret; > - > - ret = media_device_register(mdev); > - if (ret) { > - v4l2_device_unregister(&csi->v4l2_dev); > - media_device_cleanup(mdev); > - return ret; > - } > + goto cleanup_dma; > > return 0; > -} > > -static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_dev *csi) > -{ > - dma_free_coherent(csi->dev, csi->dma.drain.len, > - csi->dma.drain.vaddr, csi->dma.drain.paddr); > - csi->dma.drain.vaddr = NULL; > - dma_release_channel(csi->dma.chan); > -} > - > -static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > -{ > - media_device_unregister(&csi->mdev); > - v4l2_device_unregister(&csi->v4l2_dev); > - media_device_cleanup(&csi->mdev); > -} > - > -static void ti_csi2rx_cleanup_subdev(struct ti_csi2rx_dev *csi) > -{ > - v4l2_async_nf_unregister(&csi->notifier); > - v4l2_async_nf_cleanup(&csi->notifier); > -} > - > -static void ti_csi2rx_cleanup_vb2q(struct ti_csi2rx_dev *csi) > -{ > - vb2_queue_release(&csi->vidq); > +cleanup_dma: > + ti_csi2rx_cleanup_dma(ctx); > + return ret; > } > > static int ti_csi2rx_probe(struct platform_device *pdev) > { > struct ti_csi2rx_dev *csi; > - int ret; > + int ret, i; > > csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL); > if (!csi) > @@ -1097,62 +1144,76 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > csi->dev = &pdev->dev; > platform_set_drvdata(pdev, csi); > > - mutex_init(&csi->mutex); > csi->shim = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(csi->shim)) { > ret = PTR_ERR(csi->shim); > - goto err_mutex; > + return ret; > } > > - ret = ti_csi2rx_init_dma(csi); > - if (ret) > - goto err_mutex; > + csi->drain.len = DRAIN_BUFFER_SIZE; > + csi->drain.vaddr = dma_alloc_coherent(csi->dev, csi->drain.len, > + &csi->drain.paddr, > + GFP_KERNEL); > + if (!csi->drain.vaddr) > + return -ENOMEM; > > ret = ti_csi2rx_v4l2_init(csi); > - if (ret) > - goto err_dma; > - > - ret = ti_csi2rx_init_vb2q(csi); > if (ret) > goto err_v4l2; > > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > + csi->ctx[i].idx = i; > + csi->ctx[i].csi = csi; > + ret = ti_csi2rx_init_ctx(&csi->ctx[i]); > + if (ret) > + goto err_ctx; > + } > + > ret = ti_csi2rx_notifier_register(csi); > if (ret) > - goto err_vb2q; > + goto err_ctx; > > ret = of_platform_populate(csi->dev->of_node, NULL, NULL, csi->dev); > if (ret) { > dev_err(csi->dev, "Failed to create children: %d\n", ret); > - goto err_subdev; > + goto err_notifier; > } > > return 0; > > -err_subdev: > - ti_csi2rx_cleanup_subdev(csi); > -err_vb2q: > - ti_csi2rx_cleanup_vb2q(csi); > -err_v4l2: > +err_notifier: > + ti_csi2rx_cleanup_notifier(csi); > +err_ctx: > + i--; > + for (; i >= 0; i--) > + ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > ti_csi2rx_cleanup_v4l2(csi); > -err_dma: > - ti_csi2rx_cleanup_dma(csi); > -err_mutex: > - mutex_destroy(&csi->mutex); > +err_v4l2: > + dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > + csi->drain.paddr); > return ret; > } > > static void ti_csi2rx_remove(struct platform_device *pdev) > { > struct ti_csi2rx_dev *csi = platform_get_drvdata(pdev); > + int i; unsigned > + > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > + if (vb2_is_busy(&csi->ctx[i].vidq)) Can this happen ? > + dev_err(csi->dev, > + "Failed to remove as queue busy for ctx %u\n", > + i); > + } > > - video_unregister_device(&csi->vdev); > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) > + ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > > - ti_csi2rx_cleanup_vb2q(csi); > - ti_csi2rx_cleanup_subdev(csi); > + ti_csi2rx_cleanup_notifier(csi); > ti_csi2rx_cleanup_v4l2(csi); > - ti_csi2rx_cleanup_dma(csi); > > - mutex_destroy(&csi->mutex); > + dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > + csi->drain.paddr); > } > > static const struct of_device_id ti_csi2rx_of_match[] = { > > -- > 2.43.0 > >
Hi Jai On Thu, Jun 27, 2024 at 06:40:00PM GMT, Jai Luthra wrote: > From: Pratyush Yadav <p.yadav@ti.com> > > With multiple contexts, there needs to be a different DMA channel for > each context. Earlier, the DMA channel name was hard coded to "rx0" for > the sake of simplicity. Generate the DMA channel name based on its index > and get the channel corresponding to the context. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index bffc8165fd33..361b0ea8e0d9 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -1027,6 +1027,7 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > struct dma_slave_config cfg = { > .src_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES, > }; > + char name[32]; > int ret; > > INIT_LIST_HEAD(&ctx->dma.queue); > @@ -1035,7 +1036,8 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > ctx->dma.state = TI_CSI2RX_DMA_STOPPED; > > - ctx->dma.chan = dma_request_chan(ctx->csi->dev, "rx0"); > + snprintf(name, sizeof(name), "rx%u", ctx->idx); > + ctx->dma.chan = dma_request_chan(ctx->csi->dev, name); > if (IS_ERR(ctx->dma.chan)) > return PTR_ERR(ctx->dma.chan); > > > -- > 2.43.0 > >
On Thu, Jun 27, 2024 at 06:40:01PM GMT, Jai Luthra wrote: > With single stream capture, it was simpler to use the video device as > the media entity representing the main TI CSI2RX device. Now with multi > stream capture coming into the picture, the model has shifted to each > video device having a link to the main device's subdev. The routing > would then be set on this subdev. > > Add this subdev, link each context to this subdev's entity and link the > subdev's entity to the source. Also add an array of media pads. It will > have one sink pad and source pads equal to the number of contexts. > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 237 ++++++++++++++++++--- > 1 file changed, 212 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index 361b0ea8e0d9..13d7426ab4ba 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -51,6 +51,11 @@ > #define MAX_WIDTH_BYTES SZ_16K > #define MAX_HEIGHT_LINES SZ_16K > > +#define TI_CSI2RX_PAD_SINK 0 > +#define TI_CSI2RX_PAD_FIRST_SOURCE 1 > +#define TI_CSI2RX_NUM_SOURCE_PADS 1 > +#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS) > + > #define DRAIN_TIMEOUT_MS 50 > #define DRAIN_BUFFER_SIZE SZ_32K > > @@ -97,6 +102,7 @@ struct ti_csi2rx_ctx { > struct mutex mutex; /* To serialize ioctls. */ > struct v4l2_format v_fmt; > struct ti_csi2rx_dma dma; > + struct media_pad pad; > u32 sequence; > u32 idx; > }; > @@ -104,12 +110,15 @@ struct ti_csi2rx_ctx { > struct ti_csi2rx_dev { > struct device *dev; > void __iomem *shim; > + struct mutex mutex; /* To serialize ioctls. */ > + unsigned int enable_count; > struct v4l2_device v4l2_dev; > struct media_device mdev; > struct media_pipeline pipe; > - struct media_pad pad; > + struct media_pad pads[TI_CSI2RX_NUM_PADS]; > struct v4l2_async_notifier notifier; > struct v4l2_subdev *source; > + struct v4l2_subdev subdev; > struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX]; > /* Buffer to drain stale data from PSI-L endpoint */ > struct { > @@ -431,6 +440,15 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev); > int ret, i; > > + /* Create link from source to subdev */ > + ret = v4l2_create_fwnode_links_to_pad(csi->source, Isn't this a single link from the image source to the RX's sink pad ? Wouldn't media_create_pad_link() do here ? > + &csi->pads[TI_CSI2RX_PAD_SINK], > + MEDIA_LNK_FL_IMMUTABLE | > + MEDIA_LNK_FL_ENABLED); > + if (ret) > + return ret; > + > + /* Create and link video nodes for all DMA contexts */ > for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > struct ti_csi2rx_ctx *ctx = &csi->ctx[i]; > struct video_device *vdev = &ctx->vdev; > @@ -438,13 +456,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > if (ret) > goto unregister_dev; > - } > > - ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > - MEDIA_LNK_FL_IMMUTABLE | > - MEDIA_LNK_FL_ENABLED); > - if (ret) > - goto unregister_dev; > + ret = media_create_pad_link(&csi->subdev.entity, > + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx, > + &vdev->entity, 0, > + MEDIA_LNK_FL_IMMUTABLE | > + MEDIA_LNK_FL_ENABLED); > + if (ret) { > + video_unregister_device(vdev); > + goto unregister_dev; Should you call media_entity_remove_links() on the csi2 entity on the error path ? > + } > + } > > ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev); > if (ret) > @@ -859,7 +881,7 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > dma->state = TI_CSI2RX_DMA_ACTIVE; > spin_unlock_irqrestore(&dma->lock, flags); > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 1); > if (ret) > goto err_dma; > > @@ -887,7 +909,7 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > writel(0, csi->shim + SHIM_CNTL); > writel(0, csi->shim + SHIM_DMACNTX(ctx->idx)); > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 0); > if (ret) > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > @@ -905,6 +927,119 @@ static const struct vb2_ops csi_vb2_qops = { > .wait_finish = vb2_ops_wait_finish, > }; > > +static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct ti_csi2rx_dev, subdev); > +} > + > +static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format) > +{ > + struct v4l2_mbus_framefmt *fmt; > + int ret = 0; > + > + /* No transcoding, don't allow setting source fmt */ > + if (format->pad >= TI_CSI2RX_PAD_FIRST_SOURCE) > TI_CSI2RX_PAD_SINK might be clearer > + return v4l2_subdev_get_fmt(sd, state, format); > + > + if (!find_format_by_code(format->format.code)) > + format->format.code = ti_csi2rx_formats[0].code; > + > + format->format.field = V4L2_FIELD_NONE; What about other colorspace related fields ? > + > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > + if (!fmt) { Can this happen ? > + ret = -EINVAL; > + goto out; > + } > + *fmt = format->format; > + > + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE, > + format->stream); > + if (!fmt) { > + ret = -EINVAL; ditto > + goto out; kind of missing the point of a jump here where you can simply return -EINVAL > + } > + *fmt = format->format; > + > +out: > + return ret; > +} > + > +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_format format = { > + .pad = TI_CSI2RX_PAD_SINK, > + .format = { > + .width = 640, > + .height = 480, > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .field = V4L2_FIELD_NONE, > + .colorspace = V4L2_COLORSPACE_SRGB, > + .ycbcr_enc = V4L2_YCBCR_ENC_601, > + .quantization = V4L2_QUANTIZATION_LIM_RANGE, > + .xfer_func = V4L2_XFER_FUNC_SRGB, > + }, > + }; > + > + return ti_csi2rx_sd_set_fmt(sd, state, &format); > +} > + > +static int ti_csi2rx_sd_s_stream(struct v4l2_subdev *sd, int enable) Even if I never used that API myself, but I have a feeling enable_streams() (which is meant to replace s_stream for multiplexed subdev) would help you avoiding the manual enable_count booking here below ? > +{ > + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd); > + int ret = 0; > + > + mutex_lock(&csi->mutex); > + > + if (enable) { > + if (csi->enable_count > 0) { > + csi->enable_count++; > + goto out; > + } > + > + ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > + if (ret) > + goto out; > + > + csi->enable_count++; > + } else { > + if (csi->enable_count == 0) { > + ret = -EINVAL; > + goto out; > + } > + > + if (--csi->enable_count > 0) > + goto out; > + > + ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > + } > + > +out: > + mutex_unlock(&csi->mutex); > + return ret; > +} > + > +static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = { > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = ti_csi2rx_sd_set_fmt, > +}; > + > +static const struct v4l2_subdev_video_ops ti_csi2rx_subdev_video_ops = { > + .s_stream = ti_csi2rx_sd_s_stream, > +}; > + > +static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = { > + .video = &ti_csi2rx_subdev_video_ops, > + .pad = &ti_csi2rx_subdev_pad_ops, > +}; > + > +static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = { > + .init_state = ti_csi2rx_sd_init_state, > +}; > + > static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > { > dma_release_channel(ctx->dma.chan); > @@ -912,6 +1047,7 @@ static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > { > + v4l2_subdev_cleanup(&csi->subdev); > media_device_unregister(&csi->mdev); > v4l2_device_unregister(&csi->v4l2_dev); > media_device_cleanup(&csi->mdev); > @@ -973,14 +1109,22 @@ static int ti_csi2rx_link_validate(struct media_link *link) > struct v4l2_subdev_format source_fmt = { > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > .pad = link->source->index, > + .stream = 0, I don't think it hurts, but this seems not to be strictly related to this patch ? > }; > + struct v4l2_subdev_state *state; > const struct ti_csi2rx_fmt *ti_fmt; > int ret; > > - ret = v4l2_subdev_call_state_active(csi->source, pad, > - get_fmt, &source_fmt); > - if (ret) > - return ret; > + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev); > + ret = v4l2_subdev_call(&csi->subdev, pad, get_fmt, state, &source_fmt); > + v4l2_subdev_unlock_state(state); > + > + if (ret) { > + dev_dbg(csi->dev, > + "Skipping validation as no format present on \"%s\":%u:0\n", > + link->source->entity->name, link->source->index); If get_fmt returns an error, should you simply continue or fail ? In which cases get_fmt on a subdev fails ? Only if the pad or the stream are not valid right ? > + return 0; > + } > > if (source_fmt.format.width != csi_fmt->width) { > dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n", > @@ -1010,8 +1154,9 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > if (ti_fmt->fourcc != csi_fmt->pixelformat) { > dev_dbg(csi->dev, > - "Cannot transform source fmt 0x%x to sink fmt 0x%x\n", > - ti_fmt->fourcc, csi_fmt->pixelformat); > + "Cannot transform \"%s\":%u format %p4cc to %p4cc\n", > + link->source->entity->name, link->source->index, > + &ti_fmt->fourcc, &csi_fmt->pixelformat); > return -EPIPE; > } > > @@ -1022,6 +1167,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = { > .link_validate = ti_csi2rx_link_validate, > }; > > +static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > { > struct dma_slave_config cfg = { > @@ -1053,7 +1202,8 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > { > struct media_device *mdev = &csi->mdev; > - int ret; > + struct v4l2_subdev *sd = &csi->subdev; > + int ret, i; > > mdev->dev = csi->dev; > mdev->hw_revision = 1; > @@ -1065,16 +1215,50 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > if (ret) > - return ret; > + goto cleanup_media; > > ret = media_device_register(mdev); > - if (ret) { > - v4l2_device_unregister(&csi->v4l2_dev); > - media_device_cleanup(mdev); > - return ret; > - } > + if (ret) > + goto unregister_v4l2; > + > + v4l2_subdev_init(sd, &ti_csi2rx_subdev_ops); > + sd->internal_ops = &ti_csi2rx_internal_ops; > + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name)); > + sd->dev = csi->dev; > + sd->entity.ops = &ti_csi2rx_subdev_entity_ops; > + > + csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > + > + for (i = TI_CSI2RX_PAD_FIRST_SOURCE; i < TI_CSI2RX_NUM_PADS; i++) for (unsigned int i = 0; > + csi->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads), > + csi->pads); > + if (ret) > + goto unregister_media; > + > + ret = v4l2_subdev_init_finalize(sd); > + if (ret) > + goto unregister_media; > + > + ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd); > + if (ret) > + goto cleanup_subdev; > > return 0; > + > +cleanup_subdev: > + v4l2_subdev_cleanup(sd); > +unregister_media: > + media_device_unregister(mdev); > +unregister_v4l2: > + v4l2_device_unregister(&csi->v4l2_dev); > +cleanup_media: > + media_device_cleanup(mdev); > + > + return ret; > } > > static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > @@ -1101,9 +1285,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt); > > - csi->pad.flags = MEDIA_PAD_FL_SINK; Ah was this re-initializing the same pad multiple times ? > + ctx->pad.flags = MEDIA_PAD_FL_SINK; > vdev->entity.ops = &ti_csi2rx_video_entity_ops; > - ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad); > + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad); > if (ret) > return ret; > > @@ -1159,6 +1343,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > if (!csi->drain.vaddr) > return -ENOMEM; > > + mutex_init(&csi->mutex); > + > ret = ti_csi2rx_v4l2_init(csi); > if (ret) > goto err_v4l2; > @@ -1191,6 +1377,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > ti_csi2rx_cleanup_v4l2(csi); > err_v4l2: > + mutex_destroy(&csi->mutex); > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > csi->drain.paddr); > return ret; > @@ -1213,7 +1400,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev) > > ti_csi2rx_cleanup_notifier(csi); > ti_csi2rx_cleanup_v4l2(csi); > - > + mutex_destroy(&csi->mutex); > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > csi->drain.paddr); > } > > -- > 2.43.0 > >
Hi Jacopo, Thanks for the review. On Jul 12, 2024 at 15:35:51 +0200, Jacopo Mondi wrote: > Hi Jai > > On Thu, Jun 27, 2024 at 06:39:58PM GMT, Jai Luthra wrote: > > The TI CSI2RX wrapper has two parts: the main device and the DMA > > contexts. The driver was originally written with single camera capture > > in mind, so only one DMA context was needed. For the sake of simplicity, > > the context specific stuff was not modeled different to the main device. > > > > To enable multiplexed stream capture, the contexts need to be separated > > out from the main device. Create a struct ti_csi2rx_ctx that holds the > > DMA context specific things. Separate out functions handling the device > > and context related functionality. > > > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 423 ++++++++++++--------- > > 1 file changed, 242 insertions(+), 181 deletions(-) > > > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > index 22442fce7607..d8dfe0002b72 100644 > > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > @@ -40,6 +40,8 @@ > > #define SHIM_PSI_CFG0_DST_TAG GENMASK(31, 16) > > > > #define PSIL_WORD_SIZE_BYTES 16 > > +#define TI_CSI2RX_NUM_CTX 1 > > + > > /* > > * There are no hard limits on the width or height. The DMA engine can handle > > * all sizes. The max width and height are arbitrary numbers for this driver. > > @@ -64,7 +66,7 @@ struct ti_csi2rx_buffer { > > /* Common v4l2 buffer. Must be first. */ > > struct vb2_v4l2_buffer vb; > > struct list_head list; > > - struct ti_csi2rx_dev *csi; > > + struct ti_csi2rx_ctx *ctx; > > }; > > > > enum ti_csi2rx_dma_state { > > @@ -84,29 +86,37 @@ struct ti_csi2rx_dma { > > * Queue of buffers submitted to DMA engine. > > */ > > struct list_head submitted; > > - /* Buffer to drain stale data from PSI-L endpoint */ > > - struct { > > - void *vaddr; > > - dma_addr_t paddr; > > - size_t len; > > - } drain; > > +}; > > + > > +struct ti_csi2rx_dev; > > + > > +struct ti_csi2rx_ctx { > > + struct ti_csi2rx_dev *csi; > > + struct video_device vdev; > > + struct vb2_queue vidq; > > + struct mutex mutex; /* To serialize ioctls. */ > > + struct v4l2_format v_fmt; > > + struct ti_csi2rx_dma dma; > > + u32 sequence; > > + u32 idx; > > }; > > > > struct ti_csi2rx_dev { > > struct device *dev; > > void __iomem *shim; > > struct v4l2_device v4l2_dev; > > - struct video_device vdev; > > struct media_device mdev; > > struct media_pipeline pipe; > > struct media_pad pad; > > struct v4l2_async_notifier notifier; > > struct v4l2_subdev *source; > > - struct vb2_queue vidq; > > - struct mutex mutex; /* To serialize ioctls. */ > > - struct v4l2_format v_fmt; > > - struct ti_csi2rx_dma dma; > > - u32 sequence; > > + struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX]; > > + /* Buffer to drain stale data from PSI-L endpoint */ > > + struct { > > + void *vaddr; > > + dma_addr_t paddr; > > + size_t len; > > + } drain; > > }; > > > > static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = { > > @@ -212,7 +222,7 @@ static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = { > > }; > > > > /* Forward declaration needed by ti_csi2rx_dma_callback. */ > > -static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > > +static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx, > > struct ti_csi2rx_buffer *buf); > > > > static const struct ti_csi2rx_fmt *find_format_by_fourcc(u32 pixelformat) > > @@ -302,7 +312,7 @@ static int ti_csi2rx_enum_fmt_vid_cap(struct file *file, void *priv, > > static int ti_csi2rx_g_fmt_vid_cap(struct file *file, void *prov, > > struct v4l2_format *f) > > { > > - struct ti_csi2rx_dev *csi = video_drvdata(file); > > + struct ti_csi2rx_ctx *csi = video_drvdata(file); > > > > *f = csi->v_fmt; > > > > @@ -333,7 +343,7 @@ static int ti_csi2rx_try_fmt_vid_cap(struct file *file, void *priv, > > static int ti_csi2rx_s_fmt_vid_cap(struct file *file, void *priv, > > struct v4l2_format *f) > > { > > - struct ti_csi2rx_dev *csi = video_drvdata(file); > > + struct ti_csi2rx_ctx *csi = video_drvdata(file); > > struct vb2_queue *q = &csi->vidq; > > int ret; > > > > @@ -419,25 +429,33 @@ static int csi_async_notifier_bound(struct v4l2_async_notifier *notifier, > > static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > > { > > struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev); > > - struct video_device *vdev = &csi->vdev; > > - int ret; > > + int ret, i; > > > > - ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > > - if (ret) > > - return ret; > > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > > + struct ti_csi2rx_ctx *ctx = &csi->ctx[i]; > > + struct video_device *vdev = &ctx->vdev; > > > > - ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > > - MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED); > > - > > - if (ret) { > > - video_unregister_device(vdev); > > - return ret; > > + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > > + if (ret) > > + goto unregister_dev; > > } > > > > + ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > > + MEDIA_LNK_FL_IMMUTABLE | > > + MEDIA_LNK_FL_ENABLED); > > + if (ret) > > + goto unregister_dev; > > + > > ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev); > > if (ret) > > - video_unregister_device(vdev); > > + goto unregister_dev; > > + > > + return 0; > > > > +unregister_dev: > > + i--; > > + for (; i >= 0; i--) > > + video_unregister_device(&csi->ctx[i].vdev); > > return ret; > > } > > > > @@ -483,12 +501,13 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi) > > return 0; > > } > > > > -static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi) > > +static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx) > > { > > + struct ti_csi2rx_dev *csi = ctx->csi; > > const struct ti_csi2rx_fmt *fmt; > > unsigned int reg; > > > > - fmt = find_format_by_fourcc(csi->v_fmt.fmt.pix.pixelformat); > > + fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat); > > > > /* De-assert the pixel interface reset. */ > > reg = SHIM_CNTL_PIX_RST; > > @@ -555,8 +574,9 @@ static void ti_csi2rx_drain_callback(void *param) > > * To prevent that stale data corrupting the subsequent transactions, it is > > * required to issue DMA requests to drain it out. > > */ > > -static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > > +static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx) > > { > > + struct ti_csi2rx_dev *csi = ctx->csi; > > struct dma_async_tx_descriptor *desc; > > struct completion drain_complete; > > dma_cookie_t cookie; > > @@ -564,8 +584,8 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > > > > init_completion(&drain_complete); > > > > - desc = dmaengine_prep_slave_single(csi->dma.chan, csi->dma.drain.paddr, > > - csi->dma.drain.len, DMA_DEV_TO_MEM, > > + desc = dmaengine_prep_slave_single(ctx->dma.chan, csi->drain.paddr, > > + csi->drain.len, DMA_DEV_TO_MEM, > > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > > if (!desc) { > > ret = -EIO; > > @@ -580,11 +600,11 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > > if (ret) > > goto out; > > > > - dma_async_issue_pending(csi->dma.chan); > > + dma_async_issue_pending(ctx->dma.chan); > > > > if (!wait_for_completion_timeout(&drain_complete, > > msecs_to_jiffies(DRAIN_TIMEOUT_MS))) { > > - dmaengine_terminate_sync(csi->dma.chan); > > + dmaengine_terminate_sync(ctx->dma.chan); > > dev_dbg(csi->dev, "DMA transfer timed out for drain buffer\n"); > > ret = -ETIMEDOUT; > > goto out; > > @@ -596,8 +616,8 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi) > > static void ti_csi2rx_dma_callback(void *param) > > { > > struct ti_csi2rx_buffer *buf = param; > > - struct ti_csi2rx_dev *csi = buf->csi; > > - struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_ctx *ctx = buf->ctx; > > + struct ti_csi2rx_dma *dma = &ctx->dma; > > unsigned long flags; > > > > /* > > @@ -605,7 +625,7 @@ static void ti_csi2rx_dma_callback(void *param) > > * hardware monitor registers. > > */ > > buf->vb.vb2_buf.timestamp = ktime_get_ns(); > > - buf->vb.sequence = csi->sequence++; > > + buf->vb.sequence = ctx->sequence++; > > > > spin_lock_irqsave(&dma->lock, flags); > > > > @@ -617,8 +637,9 @@ static void ti_csi2rx_dma_callback(void *param) > > while (!list_empty(&dma->queue)) { > > buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list); > > > > - if (ti_csi2rx_start_dma(csi, buf)) { > > - dev_err(csi->dev, "Failed to queue the next buffer for DMA\n"); > > + if (ti_csi2rx_start_dma(ctx, buf)) { > > + dev_err(ctx->csi->dev, > > + "Failed to queue the next buffer for DMA\n"); > > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > > } else { > > list_move_tail(&buf->list, &dma->submitted); > > @@ -631,17 +652,17 @@ static void ti_csi2rx_dma_callback(void *param) > > spin_unlock_irqrestore(&dma->lock, flags); > > } > > > > -static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > > +static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx, > > struct ti_csi2rx_buffer *buf) > > { > > unsigned long addr; > > struct dma_async_tx_descriptor *desc; > > - size_t len = csi->v_fmt.fmt.pix.sizeimage; > > + size_t len = ctx->v_fmt.fmt.pix.sizeimage; > > dma_cookie_t cookie; > > int ret = 0; > > > > addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0); > > - desc = dmaengine_prep_slave_single(csi->dma.chan, addr, len, > > + desc = dmaengine_prep_slave_single(ctx->dma.chan, addr, len, > > DMA_DEV_TO_MEM, > > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > > if (!desc) > > @@ -655,20 +676,20 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi, > > if (ret) > > return ret; > > > > - dma_async_issue_pending(csi->dma.chan); > > + dma_async_issue_pending(ctx->dma.chan); > > > > return 0; > > } > > > > -static void ti_csi2rx_stop_dma(struct ti_csi2rx_dev *csi) > > +static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx) > > { > > - struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_dma *dma = &ctx->dma; > > enum ti_csi2rx_dma_state state; > > unsigned long flags; > > int ret; > > > > spin_lock_irqsave(&dma->lock, flags); > > - state = csi->dma.state; > > + state = ctx->dma.state; > > dma->state = TI_CSI2RX_DMA_STOPPED; > > spin_unlock_irqrestore(&dma->lock, flags); > > > > @@ -679,30 +700,30 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_dev *csi) > > * is stopped, as the module-level pixel reset cannot be > > * enforced before terminating DMA. > > */ > > - ret = ti_csi2rx_drain_dma(csi); > > + ret = ti_csi2rx_drain_dma(ctx); > > if (ret && ret != -ETIMEDOUT) > > - dev_warn(csi->dev, > > + dev_warn(ctx->csi->dev, > > "Failed to drain DMA. Next frame might be bogus\n"); > > } > > > > - ret = dmaengine_terminate_sync(csi->dma.chan); > > + ret = dmaengine_terminate_sync(ctx->dma.chan); > > if (ret) > > - dev_err(csi->dev, "Failed to stop DMA: %d\n", ret); > > + dev_err(ctx->csi->dev, "Failed to stop DMA: %d\n", ret); > > } > > > > -static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_dev *csi, > > +static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_ctx *ctx, > > enum vb2_buffer_state state) > > { > > - struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_dma *dma = &ctx->dma; > > struct ti_csi2rx_buffer *buf, *tmp; > > unsigned long flags; > > > > spin_lock_irqsave(&dma->lock, flags); > > - list_for_each_entry_safe(buf, tmp, &csi->dma.queue, list) { > > + list_for_each_entry_safe(buf, tmp, &ctx->dma.queue, list) { > > list_del(&buf->list); > > vb2_buffer_done(&buf->vb.vb2_buf, state); > > } > > - list_for_each_entry_safe(buf, tmp, &csi->dma.submitted, list) { > > + list_for_each_entry_safe(buf, tmp, &ctx->dma.submitted, list) { > > list_del(&buf->list); > > vb2_buffer_done(&buf->vb.vb2_buf, state); > > } > > @@ -713,8 +734,8 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers, > > unsigned int *nplanes, unsigned int sizes[], > > struct device *alloc_devs[]) > > { > > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(q); > > - unsigned int size = csi->v_fmt.fmt.pix.sizeimage; > > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(q); > > + unsigned int size = ctx->v_fmt.fmt.pix.sizeimage; > > > > if (*nplanes) { > > if (sizes[0] < size) > > @@ -730,11 +751,11 @@ static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers, > > > > static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb) > > { > > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue); > > - unsigned long size = csi->v_fmt.fmt.pix.sizeimage; > > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); > > + unsigned long size = ctx->v_fmt.fmt.pix.sizeimage; > > > > if (vb2_plane_size(vb, 0) < size) { > > - dev_err(csi->dev, "Data will not fit into plane\n"); > > + dev_err(ctx->csi->dev, "Data will not fit into plane\n"); > > return -EINVAL; > > } > > > > @@ -744,15 +765,15 @@ static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb) > > > > 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_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue); > > struct ti_csi2rx_buffer *buf; > > - struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_dma *dma = &ctx->dma; > > bool restart_dma = false; > > unsigned long flags = 0; > > int ret; > > > > buf = container_of(vb, struct ti_csi2rx_buffer, vb.vb2_buf); > > - buf->csi = csi; > > + buf->ctx = ctx; > > > > spin_lock_irqsave(&dma->lock, flags); > > /* > > @@ -781,18 +802,18 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) > > * the application and will only confuse it. Issue a DMA > > * transaction to drain that up. > > */ > > - ret = ti_csi2rx_drain_dma(csi); > > + ret = ti_csi2rx_drain_dma(ctx); > > if (ret && ret != -ETIMEDOUT) > > - dev_warn(csi->dev, > > + dev_warn(ctx->csi->dev, > > "Failed to drain DMA. Next frame might be bogus\n"); > > > > spin_lock_irqsave(&dma->lock, flags); > > - ret = ti_csi2rx_start_dma(csi, buf); > > + ret = ti_csi2rx_start_dma(ctx, buf); > > if (ret) { > > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > > dma->state = TI_CSI2RX_DMA_IDLE; > > spin_unlock_irqrestore(&dma->lock, flags); > > - dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > > + dev_err(ctx->csi->dev, "Failed to start DMA: %d\n", ret); > > } else { > > list_add_tail(&buf->list, &dma->submitted); > > spin_unlock_irqrestore(&dma->lock, flags); > > @@ -802,8 +823,9 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb) > > > > static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > { > > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > > - struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq); > > + struct ti_csi2rx_dev *csi = ctx->csi; > > + struct ti_csi2rx_dma *dma = &ctx->dma; > > struct ti_csi2rx_buffer *buf; > > unsigned long flags; > > int ret = 0; > > @@ -815,18 +837,18 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > if (ret) > > return ret; > > > > - ret = video_device_pipeline_start(&csi->vdev, &csi->pipe); > > + ret = video_device_pipeline_start(&ctx->vdev, &csi->pipe); > > if (ret) > > goto err; > > > > - ti_csi2rx_setup_shim(csi); > > + ti_csi2rx_setup_shim(ctx); > > > > - csi->sequence = 0; > > + ctx->sequence = 0; > > > > spin_lock_irqsave(&dma->lock, flags); > > buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list); > > > > - ret = ti_csi2rx_start_dma(csi, buf); > > + ret = ti_csi2rx_start_dma(ctx, buf); > > if (ret) { > > dev_err(csi->dev, "Failed to start DMA: %d\n", ret); > > spin_unlock_irqrestore(&dma->lock, flags); > > @@ -844,22 +866,23 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > return 0; > > > > err_dma: > > - ti_csi2rx_stop_dma(csi); > > + ti_csi2rx_stop_dma(ctx); > > err_pipeline: > > - video_device_pipeline_stop(&csi->vdev); > > + video_device_pipeline_stop(&ctx->vdev); > > writel(0, csi->shim + SHIM_CNTL); > > writel(0, csi->shim + SHIM_DMACNTX); > > err: > > - ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_QUEUED); > > + ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_QUEUED); > > return ret; > > } > > > > static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > > { > > - struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > > + struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq); > > + struct ti_csi2rx_dev *csi = ctx->csi; > > int ret; > > > > - video_device_pipeline_stop(&csi->vdev); > > + video_device_pipeline_stop(&ctx->vdev); > > > > writel(0, csi->shim + SHIM_CNTL); > > writel(0, csi->shim + SHIM_DMACNTX); > > @@ -868,8 +891,8 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > > if (ret) > > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > > > - ti_csi2rx_stop_dma(csi); > > - ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_ERROR); > > + ti_csi2rx_stop_dma(ctx); > > + ti_csi2rx_cleanup_buffers(ctx, VB2_BUF_STATE_ERROR); > > } > > > > static const struct vb2_ops csi_vb2_qops = { > > @@ -882,27 +905,60 @@ static const struct vb2_ops csi_vb2_qops = { > > .wait_finish = vb2_ops_wait_finish, > > }; > > > > -static int ti_csi2rx_init_vb2q(struct ti_csi2rx_dev *csi) > > +static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > Is it worth a wrapper function ? > > > { > > - struct vb2_queue *q = &csi->vidq; > > + dma_release_channel(ctx->dma.chan); > > +} > > + > > +static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > > +{ > > + media_device_unregister(&csi->mdev); > > + v4l2_device_unregister(&csi->v4l2_dev); > > + media_device_cleanup(&csi->mdev); > > +} > > + > > +static void ti_csi2rx_cleanup_notifier(struct ti_csi2rx_dev *csi) > > +{ > > + v4l2_async_nf_unregister(&csi->notifier); > > + v4l2_async_nf_cleanup(&csi->notifier); > > +} > > + > > +static void ti_csi2rx_cleanup_vb2q(struct ti_csi2rx_ctx *ctx) > > Has a single caller, can be inlined maybe ? > > > +{ > > + vb2_queue_release(&ctx->vidq); > > +} > > + > > +static void ti_csi2rx_cleanup_ctx(struct ti_csi2rx_ctx *ctx) > > +{ > > + ti_csi2rx_cleanup_dma(ctx); > > + ti_csi2rx_cleanup_vb2q(ctx); > > So this would become > > dma_release_channel(ctx->dma.chan); > vb2_queue_release(&ctx->vidq); > Good catch, will update in v3. > > + > > + video_unregister_device(&ctx->vdev); > > + > > + mutex_destroy(&ctx->mutex); > > +} > > + > > +static int ti_csi2rx_init_vb2q(struct ti_csi2rx_ctx *ctx) > > +{ > > + struct vb2_queue *q = &ctx->vidq; > > int ret; > > > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > q->io_modes = VB2_MMAP | VB2_DMABUF; > > - q->drv_priv = csi; > > + q->drv_priv = ctx; > > q->buf_struct_size = sizeof(struct ti_csi2rx_buffer); > > q->ops = &csi_vb2_qops; > > q->mem_ops = &vb2_dma_contig_memops; > > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > > - q->dev = dmaengine_get_dma_device(csi->dma.chan); > > - q->lock = &csi->mutex; > > + q->dev = dmaengine_get_dma_device(ctx->dma.chan); > > + q->lock = &ctx->mutex; > > q->min_queued_buffers = 1; > > > > ret = vb2_queue_init(q); > > if (ret) > > return ret; > > > > - csi->vdev.queue = q; > > + ctx->vdev.queue = q; > > > > return 0; > > } > > @@ -911,8 +967,9 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > { > > struct media_entity *entity = link->sink->entity; > > struct video_device *vdev = media_entity_to_video_device(entity); > > - struct ti_csi2rx_dev *csi = container_of(vdev, struct ti_csi2rx_dev, vdev); > > - struct v4l2_pix_format *csi_fmt = &csi->v_fmt.fmt.pix; > > + struct ti_csi2rx_ctx *ctx = container_of(vdev, struct ti_csi2rx_ctx, vdev); > > + struct ti_csi2rx_dev *csi = ctx->csi; > > + struct v4l2_pix_format *csi_fmt = &ctx->v_fmt.fmt.pix; > > struct v4l2_subdev_format source_fmt = { > > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > .pad = link->source->index, > > @@ -965,47 +1022,69 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = { > > .link_validate = ti_csi2rx_link_validate, > > }; > > > > -static int ti_csi2rx_init_dma(struct ti_csi2rx_dev *csi) > > +static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > { > > struct dma_slave_config cfg = { > > .src_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES, > > }; > > int ret; > > > > - INIT_LIST_HEAD(&csi->dma.queue); > > - INIT_LIST_HEAD(&csi->dma.submitted); > > - spin_lock_init(&csi->dma.lock); > > + INIT_LIST_HEAD(&ctx->dma.queue); > > + INIT_LIST_HEAD(&ctx->dma.submitted); > > + spin_lock_init(&ctx->dma.lock); > > > > - csi->dma.state = TI_CSI2RX_DMA_STOPPED; > > + ctx->dma.state = TI_CSI2RX_DMA_STOPPED; > > > > - csi->dma.chan = dma_request_chan(csi->dev, "rx0"); > > - if (IS_ERR(csi->dma.chan)) > > - return PTR_ERR(csi->dma.chan); > > + ctx->dma.chan = dma_request_chan(ctx->csi->dev, "rx0"); > > The channel name should probably be parametrized, I presume it will > happen next Yes in subsequent patches. > > > + if (IS_ERR(ctx->dma.chan)) > > + return PTR_ERR(ctx->dma.chan); > > > > - ret = dmaengine_slave_config(csi->dma.chan, &cfg); > > + ret = dmaengine_slave_config(ctx->dma.chan, &cfg); > > if (ret) { > > - dma_release_channel(csi->dma.chan); > > + dma_release_channel(ctx->dma.chan); > > return ret; > > } > > > > - csi->dma.drain.len = DRAIN_BUFFER_SIZE; > > - csi->dma.drain.vaddr = dma_alloc_coherent(csi->dev, csi->dma.drain.len, > > - &csi->dma.drain.paddr, > > - GFP_KERNEL); > > - if (!csi->dma.drain.vaddr) > > - return -ENOMEM; > > - > > return 0; > > } > > > > static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > { > > struct media_device *mdev = &csi->mdev; > > - struct video_device *vdev = &csi->vdev; > > + int ret; > > + > > + mdev->dev = csi->dev; > > + mdev->hw_revision = 1; > > + strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model)); > > + > > + media_device_init(mdev); > > + > > + csi->v4l2_dev.mdev = mdev; > > + > > + ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > > + if (ret) > > + return ret; > > + > > + ret = media_device_register(mdev); > > + if (ret) { > > + v4l2_device_unregister(&csi->v4l2_dev); > > + media_device_cleanup(mdev); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > +{ > > + struct ti_csi2rx_dev *csi = ctx->csi; > > + struct video_device *vdev = &ctx->vdev; > > const struct ti_csi2rx_fmt *fmt; > > - struct v4l2_pix_format *pix_fmt = &csi->v_fmt.fmt.pix; > > + struct v4l2_pix_format *pix_fmt = &ctx->v_fmt.fmt.pix; > > int ret; > > > > + mutex_init(&ctx->mutex); > > + > > fmt = find_format_by_fourcc(V4L2_PIX_FMT_UYVY); > > if (!fmt) > > return -EINVAL; > > @@ -1018,15 +1097,16 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE, > > pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB, > > > > - ti_csi2rx_fill_fmt(fmt, &csi->v_fmt); > > - > > - mdev->dev = csi->dev; > > - mdev->hw_revision = 1; > > - strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model)); > > + ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt); > > > > - media_device_init(mdev); > > + csi->pad.flags = MEDIA_PAD_FL_SINK; > > + vdev->entity.ops = &ti_csi2rx_video_entity_ops; > > + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad); > > + if (ret) > > + return ret; > > > > - strscpy(vdev->name, TI_CSI2RX_MODULE_NAME, sizeof(vdev->name)); > > + snprintf(vdev->name, sizeof(vdev->name), "%s context %u", > > + dev_name(csi->dev), ctx->idx); > > vdev->v4l2_dev = &csi->v4l2_dev; > > vdev->vfl_dir = VFL_DIR_RX; > > vdev->fops = &csi_fops; > > @@ -1034,61 +1114,28 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > vdev->release = video_device_release_empty; > > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > > V4L2_CAP_IO_MC; > > - vdev->lock = &csi->mutex; > > - video_set_drvdata(vdev, csi); > > + vdev->lock = &ctx->mutex; > > + video_set_drvdata(vdev, ctx); > > > > - csi->pad.flags = MEDIA_PAD_FL_SINK; > > - vdev->entity.ops = &ti_csi2rx_video_entity_ops; > > - ret = media_entity_pads_init(&csi->vdev.entity, 1, &csi->pad); > > + ret = ti_csi2rx_init_dma(ctx); > > if (ret) > > return ret; > > > > - csi->v4l2_dev.mdev = mdev; > > - > > - ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > > + ret = ti_csi2rx_init_vb2q(ctx); > > if (ret) > > - return ret; > > - > > - ret = media_device_register(mdev); > > - if (ret) { > > - v4l2_device_unregister(&csi->v4l2_dev); > > - media_device_cleanup(mdev); > > - return ret; > > - } > > + goto cleanup_dma; > > > > return 0; > > -} > > > > -static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_dev *csi) > > -{ > > - dma_free_coherent(csi->dev, csi->dma.drain.len, > > - csi->dma.drain.vaddr, csi->dma.drain.paddr); > > - csi->dma.drain.vaddr = NULL; > > - dma_release_channel(csi->dma.chan); > > -} > > - > > -static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > > -{ > > - media_device_unregister(&csi->mdev); > > - v4l2_device_unregister(&csi->v4l2_dev); > > - media_device_cleanup(&csi->mdev); > > -} > > - > > -static void ti_csi2rx_cleanup_subdev(struct ti_csi2rx_dev *csi) > > -{ > > - v4l2_async_nf_unregister(&csi->notifier); > > - v4l2_async_nf_cleanup(&csi->notifier); > > -} > > - > > -static void ti_csi2rx_cleanup_vb2q(struct ti_csi2rx_dev *csi) > > -{ > > - vb2_queue_release(&csi->vidq); > > +cleanup_dma: > > + ti_csi2rx_cleanup_dma(ctx); > > + return ret; > > } > > > > static int ti_csi2rx_probe(struct platform_device *pdev) > > { > > struct ti_csi2rx_dev *csi; > > - int ret; > > + int ret, i; > > > > csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL); > > if (!csi) > > @@ -1097,62 +1144,76 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > > csi->dev = &pdev->dev; > > platform_set_drvdata(pdev, csi); > > > > - mutex_init(&csi->mutex); > > csi->shim = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(csi->shim)) { > > ret = PTR_ERR(csi->shim); > > - goto err_mutex; > > + return ret; > > } > > > > - ret = ti_csi2rx_init_dma(csi); > > - if (ret) > > - goto err_mutex; > > + csi->drain.len = DRAIN_BUFFER_SIZE; > > + csi->drain.vaddr = dma_alloc_coherent(csi->dev, csi->drain.len, > > + &csi->drain.paddr, > > + GFP_KERNEL); > > + if (!csi->drain.vaddr) > > + return -ENOMEM; > > > > ret = ti_csi2rx_v4l2_init(csi); > > - if (ret) > > - goto err_dma; > > - > > - ret = ti_csi2rx_init_vb2q(csi); > > if (ret) > > goto err_v4l2; > > > > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > > + csi->ctx[i].idx = i; > > + csi->ctx[i].csi = csi; > > + ret = ti_csi2rx_init_ctx(&csi->ctx[i]); > > + if (ret) > > + goto err_ctx; > > + } > > + > > ret = ti_csi2rx_notifier_register(csi); > > if (ret) > > - goto err_vb2q; > > + goto err_ctx; > > > > ret = of_platform_populate(csi->dev->of_node, NULL, NULL, csi->dev); > > if (ret) { > > dev_err(csi->dev, "Failed to create children: %d\n", ret); > > - goto err_subdev; > > + goto err_notifier; > > } > > > > return 0; > > > > -err_subdev: > > - ti_csi2rx_cleanup_subdev(csi); > > -err_vb2q: > > - ti_csi2rx_cleanup_vb2q(csi); > > -err_v4l2: > > +err_notifier: > > + ti_csi2rx_cleanup_notifier(csi); > > +err_ctx: > > + i--; > > + for (; i >= 0; i--) > > + ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > > ti_csi2rx_cleanup_v4l2(csi); > > -err_dma: > > - ti_csi2rx_cleanup_dma(csi); > > -err_mutex: > > - mutex_destroy(&csi->mutex); > > +err_v4l2: > > + dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > + csi->drain.paddr); > > return ret; > > } > > > > static void ti_csi2rx_remove(struct platform_device *pdev) > > { > > struct ti_csi2rx_dev *csi = platform_get_drvdata(pdev); > > + int i; > > unsigned > Will fix. > > + > > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > > + if (vb2_is_busy(&csi->ctx[i].vidq)) > > Can this happen ? > Probably not. While this driver is in use `modprobe -r j721e_csi2rx` would fail anyway because the module refcnt > 0. And when it's not in use the queue would never be busy... Thanks for the catch, will drop this check (and move to .remove_new API) > > + dev_err(csi->dev, > > + "Failed to remove as queue busy for ctx %u\n", > > + i); > > + } > > > > - video_unregister_device(&csi->vdev); > > + for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) > > + ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > > > > - ti_csi2rx_cleanup_vb2q(csi); > > - ti_csi2rx_cleanup_subdev(csi); > > + ti_csi2rx_cleanup_notifier(csi); > > ti_csi2rx_cleanup_v4l2(csi); > > - ti_csi2rx_cleanup_dma(csi); > > > > - mutex_destroy(&csi->mutex); > > + dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > + csi->drain.paddr); > > } > > > > static const struct of_device_id ti_csi2rx_of_match[] = { > > > > -- > > 2.43.0 > > > >
On Jul 12, 2024 at 16:18:36 +0200, Jacopo Mondi wrote: > On Thu, Jun 27, 2024 at 06:40:01PM GMT, Jai Luthra wrote: > > With single stream capture, it was simpler to use the video device as > > the media entity representing the main TI CSI2RX device. Now with multi > > stream capture coming into the picture, the model has shifted to each > > video device having a link to the main device's subdev. The routing > > would then be set on this subdev. > > > > Add this subdev, link each context to this subdev's entity and link the > > subdev's entity to the source. Also add an array of media pads. It will > > have one sink pad and source pads equal to the number of contexts. > > > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 237 ++++++++++++++++++--- > > 1 file changed, 212 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > index 361b0ea8e0d9..13d7426ab4ba 100644 > > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > @@ -51,6 +51,11 @@ > > #define MAX_WIDTH_BYTES SZ_16K > > #define MAX_HEIGHT_LINES SZ_16K > > > > +#define TI_CSI2RX_PAD_SINK 0 > > +#define TI_CSI2RX_PAD_FIRST_SOURCE 1 > > +#define TI_CSI2RX_NUM_SOURCE_PADS 1 > > +#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS) > > + > > #define DRAIN_TIMEOUT_MS 50 > > #define DRAIN_BUFFER_SIZE SZ_32K > > > > @@ -97,6 +102,7 @@ struct ti_csi2rx_ctx { > > struct mutex mutex; /* To serialize ioctls. */ > > struct v4l2_format v_fmt; > > struct ti_csi2rx_dma dma; > > + struct media_pad pad; > > u32 sequence; > > u32 idx; > > }; > > @@ -104,12 +110,15 @@ struct ti_csi2rx_ctx { > > struct ti_csi2rx_dev { > > struct device *dev; > > void __iomem *shim; > > + struct mutex mutex; /* To serialize ioctls. */ > > + unsigned int enable_count; > > struct v4l2_device v4l2_dev; > > struct media_device mdev; > > struct media_pipeline pipe; > > - struct media_pad pad; > > + struct media_pad pads[TI_CSI2RX_NUM_PADS]; > > struct v4l2_async_notifier notifier; > > struct v4l2_subdev *source; > > + struct v4l2_subdev subdev; > > struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX]; > > /* Buffer to drain stale data from PSI-L endpoint */ > > struct { > > @@ -431,6 +440,15 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > > struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev); > > int ret, i; > > > > + /* Create link from source to subdev */ > > + ret = v4l2_create_fwnode_links_to_pad(csi->source, > > Isn't this a single link from the image source to the RX's sink pad ? > Wouldn't media_create_pad_link() do here ? The source here is always cdns-csi2rx bridge (and not the sensor) which has multiple pads. If we use media_create_pad_link() here, we would need to figure out the correct source pad using fwnode APIs. v4l2_create_fwnode_links_to_pad() helper already does that for us. But because of your comment I realized that we are missing .get_fwnode_pad() callbacks in both cdns-csi2rx and this driver. It is working by sheer luck as media_entity_get_fwnode_pad() returns the first pad as fallback. I will fix this in v3. > > > > + &csi->pads[TI_CSI2RX_PAD_SINK], > > + MEDIA_LNK_FL_IMMUTABLE | > > + MEDIA_LNK_FL_ENABLED); > > + if (ret) > > + return ret; > > + > > + /* Create and link video nodes for all DMA contexts */ > > for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > > struct ti_csi2rx_ctx *ctx = &csi->ctx[i]; > > struct video_device *vdev = &ctx->vdev; > > @@ -438,13 +456,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > > if (ret) > > goto unregister_dev; > > - } > > > > - ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > > - MEDIA_LNK_FL_IMMUTABLE | > > - MEDIA_LNK_FL_ENABLED); > > - if (ret) > > - goto unregister_dev; > > + ret = media_create_pad_link(&csi->subdev.entity, > > + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx, > > + &vdev->entity, 0, > > + MEDIA_LNK_FL_IMMUTABLE | > > + MEDIA_LNK_FL_ENABLED); > > + if (ret) { > > + video_unregister_device(vdev); > > + goto unregister_dev; > > Should you call media_entity_remove_links() on the csi2 entity on the > error path ? > Will fix. > > + } > > + } > > > > ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev); > > if (ret) > > @@ -859,7 +881,7 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > dma->state = TI_CSI2RX_DMA_ACTIVE; > > spin_unlock_irqrestore(&dma->lock, flags); > > > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 1); > > if (ret) > > goto err_dma; > > > > @@ -887,7 +909,7 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > > writel(0, csi->shim + SHIM_CNTL); > > writel(0, csi->shim + SHIM_DMACNTX(ctx->idx)); > > > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 0); > > if (ret) > > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > > > @@ -905,6 +927,119 @@ static const struct vb2_ops csi_vb2_qops = { > > .wait_finish = vb2_ops_wait_finish, > > }; > > > > +static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct ti_csi2rx_dev, subdev); > > +} > > + > > +static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *format) > > +{ > > + struct v4l2_mbus_framefmt *fmt; > > + int ret = 0; > > + > > + /* No transcoding, don't allow setting source fmt */ > > + if (format->pad >= TI_CSI2RX_PAD_FIRST_SOURCE) > > > > TI_CSI2RX_PAD_SINK > > might be clearer Will fix. > > > + return v4l2_subdev_get_fmt(sd, state, format); > > + > > + if (!find_format_by_code(format->format.code)) > > + format->format.code = ti_csi2rx_formats[0].code; > > + > > + format->format.field = V4L2_FIELD_NONE; > > What about other colorspace related fields ? The HW does not care about colorspace/encoding etc. and acts as passthrough, so we accept whatever userspace sets for those. > > > + > > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > > + if (!fmt) { > > Can this happen ? Yes __v4l2_subdev_state_get_format() may return NULL if userspace tried to set format with an invalid `stream` (i.e. no routes exist for that stream) > > > + ret = -EINVAL; > > + goto out; > > + } > > + *fmt = format->format; > > + > > + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE, > > + format->stream); > > + if (!fmt) { > > + ret = -EINVAL; > > ditto > > > + goto out; > > kind of missing the point of a jump here where you can simply return > -EINVAL Will fix. > > > + } > > + *fmt = format->format; > > + > > +out: > > + return ret; > > +} > > + > > +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_subdev_format format = { > > + .pad = TI_CSI2RX_PAD_SINK, > > + .format = { > > + .width = 640, > > + .height = 480, > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > + .field = V4L2_FIELD_NONE, > > + .colorspace = V4L2_COLORSPACE_SRGB, > > + .ycbcr_enc = V4L2_YCBCR_ENC_601, > > + .quantization = V4L2_QUANTIZATION_LIM_RANGE, > > + .xfer_func = V4L2_XFER_FUNC_SRGB, > > + }, > > + }; > > + > > + return ti_csi2rx_sd_set_fmt(sd, state, &format); > > +} > > + > > +static int ti_csi2rx_sd_s_stream(struct v4l2_subdev *sd, int enable) > > Even if I never used that API myself, but I have a feeling > enable_streams() (which is meant to replace s_stream for multiplexed > subdev) would help you avoiding the manual enable_count booking here > below ? In PATCH 12/13 we switch to using enable_streams() APIs here. We still keep this enable_count bookkeeping, but I agree it can be dropped with minor changes. Will fix in v3. > > > +{ > > + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd); > > + int ret = 0; > > + > > + mutex_lock(&csi->mutex); > > + > > + if (enable) { > > + if (csi->enable_count > 0) { > > + csi->enable_count++; > > + goto out; > > + } > > + > > + ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > > + if (ret) > > + goto out; > > + > > + csi->enable_count++; > > + } else { > > + if (csi->enable_count == 0) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + if (--csi->enable_count > 0) > > + goto out; > > + > > + ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > > + } > > + > > +out: > > + mutex_unlock(&csi->mutex); > > + return ret; > > +} > > + > > +static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = { > > + .get_fmt = v4l2_subdev_get_fmt, > > + .set_fmt = ti_csi2rx_sd_set_fmt, > > +}; > > + > > +static const struct v4l2_subdev_video_ops ti_csi2rx_subdev_video_ops = { > > + .s_stream = ti_csi2rx_sd_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = { > > + .video = &ti_csi2rx_subdev_video_ops, > > + .pad = &ti_csi2rx_subdev_pad_ops, > > +}; > > + > > +static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = { > > + .init_state = ti_csi2rx_sd_init_state, > > +}; > > + > > static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > { > > dma_release_channel(ctx->dma.chan); > > @@ -912,6 +1047,7 @@ static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > > > static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > > { > > + v4l2_subdev_cleanup(&csi->subdev); > > media_device_unregister(&csi->mdev); > > v4l2_device_unregister(&csi->v4l2_dev); > > media_device_cleanup(&csi->mdev); > > @@ -973,14 +1109,22 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > struct v4l2_subdev_format source_fmt = { > > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > .pad = link->source->index, > > + .stream = 0, > > I don't think it hurts, but this seems not to be strictly related to > this patch ? > > > }; > > + struct v4l2_subdev_state *state; > > const struct ti_csi2rx_fmt *ti_fmt; > > int ret; > > > > - ret = v4l2_subdev_call_state_active(csi->source, pad, > > - get_fmt, &source_fmt); > > - if (ret) > > - return ret; > > + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev); > > + ret = v4l2_subdev_call(&csi->subdev, pad, get_fmt, state, &source_fmt); > > + v4l2_subdev_unlock_state(state); > > + > > + if (ret) { > > + dev_dbg(csi->dev, > > + "Skipping validation as no format present on \"%s\":%u:0\n", > > + link->source->entity->name, link->source->index); > > If get_fmt returns an error, should you simply continue or fail ? In > which cases get_fmt on a subdev fails ? Only if the pad or the stream > are not valid right ? The subdev introduced in this patch has multiple source pads, all actively linked to separate video nodes, one for each DMA channel. We continue (return 0) here as we don't want __media_pipeline_start to fail because of pads that are "unused" i.e. have no routes and no formats set. If the user actually routes a stream to any pad the pipeline validation will check that a format is also set (and thus the link will get validated here). I wonder if we can keep the links mutable, and expect userspace to disable links to unused video nodes. Will have to think more about that approach. > > > + return 0; > > + } > > > > if (source_fmt.format.width != csi_fmt->width) { > > dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n", > > @@ -1010,8 +1154,9 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > > > if (ti_fmt->fourcc != csi_fmt->pixelformat) { > > dev_dbg(csi->dev, > > - "Cannot transform source fmt 0x%x to sink fmt 0x%x\n", > > - ti_fmt->fourcc, csi_fmt->pixelformat); > > + "Cannot transform \"%s\":%u format %p4cc to %p4cc\n", > > + link->source->entity->name, link->source->index, > > + &ti_fmt->fourcc, &csi_fmt->pixelformat); > > return -EPIPE; > > } > > > > @@ -1022,6 +1167,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = { > > .link_validate = ti_csi2rx_link_validate, > > }; > > > > +static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > { > > struct dma_slave_config cfg = { > > @@ -1053,7 +1202,8 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > { > > struct media_device *mdev = &csi->mdev; > > - int ret; > > + struct v4l2_subdev *sd = &csi->subdev; > > + int ret, i; > > > > mdev->dev = csi->dev; > > mdev->hw_revision = 1; > > @@ -1065,16 +1215,50 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > > > ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > > if (ret) > > - return ret; > > + goto cleanup_media; > > > > ret = media_device_register(mdev); > > - if (ret) { > > - v4l2_device_unregister(&csi->v4l2_dev); > > - media_device_cleanup(mdev); > > - return ret; > > - } > > + if (ret) > > + goto unregister_v4l2; > > + > > + v4l2_subdev_init(sd, &ti_csi2rx_subdev_ops); > > + sd->internal_ops = &ti_csi2rx_internal_ops; > > + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name)); > > + sd->dev = csi->dev; > > + sd->entity.ops = &ti_csi2rx_subdev_entity_ops; > > + > > + csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > + > > + for (i = TI_CSI2RX_PAD_FIRST_SOURCE; i < TI_CSI2RX_NUM_PADS; i++) > > for (unsigned int i = 0; Will fix. > > > + csi->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads), > > + csi->pads); > > + if (ret) > > + goto unregister_media; > > + > > + ret = v4l2_subdev_init_finalize(sd); > > + if (ret) > > + goto unregister_media; > > + > > + ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd); > > + if (ret) > > + goto cleanup_subdev; > > > > return 0; > > + > > +cleanup_subdev: > > + v4l2_subdev_cleanup(sd); > > +unregister_media: > > + media_device_unregister(mdev); > > +unregister_v4l2: > > + v4l2_device_unregister(&csi->v4l2_dev); > > +cleanup_media: > > + media_device_cleanup(mdev); > > + > > + return ret; > > } > > > > static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > @@ -1101,9 +1285,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > > > ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt); > > > > - csi->pad.flags = MEDIA_PAD_FL_SINK; > > Ah was this re-initializing the same pad multiple times ? Not sure I understand the comment fully. We have only 1 video context here, more contexts will be introduced in subsequent patches. > > > + ctx->pad.flags = MEDIA_PAD_FL_SINK; > > vdev->entity.ops = &ti_csi2rx_video_entity_ops; > > - ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad); > > + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad); > > if (ret) > > return ret; > > > > @@ -1159,6 +1343,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > > if (!csi->drain.vaddr) > > return -ENOMEM; > > > > + mutex_init(&csi->mutex); > > + > > ret = ti_csi2rx_v4l2_init(csi); > > if (ret) > > goto err_v4l2; > > @@ -1191,6 +1377,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > > ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > > ti_csi2rx_cleanup_v4l2(csi); > > err_v4l2: > > + mutex_destroy(&csi->mutex); > > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > csi->drain.paddr); > > return ret; > > @@ -1213,7 +1400,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev) > > > > ti_csi2rx_cleanup_notifier(csi); > > ti_csi2rx_cleanup_v4l2(csi); > > - > > + mutex_destroy(&csi->mutex); > > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > csi->drain.paddr); > > } > > > > -- > > 2.43.0 > > > >
Hi Jai + Laurent + Sakari for a question below On Tue, Jul 16, 2024 at 03:02:48PM GMT, Jai Luthra wrote: > On Jul 12, 2024 at 16:18:36 +0200, Jacopo Mondi wrote: > > On Thu, Jun 27, 2024 at 06:40:01PM GMT, Jai Luthra wrote: > > > With single stream capture, it was simpler to use the video device as > > > the media entity representing the main TI CSI2RX device. Now with multi > > > stream capture coming into the picture, the model has shifted to each > > > video device having a link to the main device's subdev. The routing > > > would then be set on this subdev. > > > > > > Add this subdev, link each context to this subdev's entity and link the > > > subdev's entity to the source. Also add an array of media pads. It will > > > have one sink pad and source pads equal to the number of contexts. > > > > > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > > --- > > > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 237 ++++++++++++++++++--- > > > 1 file changed, 212 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > > index 361b0ea8e0d9..13d7426ab4ba 100644 > > > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > > > @@ -51,6 +51,11 @@ > > > #define MAX_WIDTH_BYTES SZ_16K > > > #define MAX_HEIGHT_LINES SZ_16K > > > > > > +#define TI_CSI2RX_PAD_SINK 0 > > > +#define TI_CSI2RX_PAD_FIRST_SOURCE 1 > > > +#define TI_CSI2RX_NUM_SOURCE_PADS 1 > > > +#define TI_CSI2RX_NUM_PADS (1 + TI_CSI2RX_NUM_SOURCE_PADS) > > > + > > > #define DRAIN_TIMEOUT_MS 50 > > > #define DRAIN_BUFFER_SIZE SZ_32K > > > > > > @@ -97,6 +102,7 @@ struct ti_csi2rx_ctx { > > > struct mutex mutex; /* To serialize ioctls. */ > > > struct v4l2_format v_fmt; > > > struct ti_csi2rx_dma dma; > > > + struct media_pad pad; > > > u32 sequence; > > > u32 idx; > > > }; > > > @@ -104,12 +110,15 @@ struct ti_csi2rx_ctx { > > > struct ti_csi2rx_dev { > > > struct device *dev; > > > void __iomem *shim; > > > + struct mutex mutex; /* To serialize ioctls. */ > > > + unsigned int enable_count; > > > struct v4l2_device v4l2_dev; > > > struct media_device mdev; > > > struct media_pipeline pipe; > > > - struct media_pad pad; > > > + struct media_pad pads[TI_CSI2RX_NUM_PADS]; > > > struct v4l2_async_notifier notifier; > > > struct v4l2_subdev *source; > > > + struct v4l2_subdev subdev; > > > struct ti_csi2rx_ctx ctx[TI_CSI2RX_NUM_CTX]; > > > /* Buffer to drain stale data from PSI-L endpoint */ > > > struct { > > > @@ -431,6 +440,15 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > > > struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev); > > > int ret, i; > > > > > > + /* Create link from source to subdev */ > > > + ret = v4l2_create_fwnode_links_to_pad(csi->source, > > > > Isn't this a single link from the image source to the RX's sink pad ? > > Wouldn't media_create_pad_link() do here ? > > The source here is always cdns-csi2rx bridge (and not the sensor) which > has multiple pads. > > If we use media_create_pad_link() here, we would need to figure out the > correct source pad using fwnode APIs. v4l2_create_fwnode_links_to_pad() > helper already does that for us. > > But because of your comment I realized that we are missing > .get_fwnode_pad() callbacks in both cdns-csi2rx and this driver. > > It is working by sheer luck as media_entity_get_fwnode_pad() returns the > first pad as fallback. I will fix this in v3. > Ack > > > > > > > + &csi->pads[TI_CSI2RX_PAD_SINK], > > > + MEDIA_LNK_FL_IMMUTABLE | > > > + MEDIA_LNK_FL_ENABLED); > > > + if (ret) > > > + return ret; > > > + > > > + /* Create and link video nodes for all DMA contexts */ > > > for (i = 0; i < TI_CSI2RX_NUM_CTX; i++) { > > > struct ti_csi2rx_ctx *ctx = &csi->ctx[i]; > > > struct video_device *vdev = &ctx->vdev; > > > @@ -438,13 +456,17 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier) > > > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1); > > > if (ret) > > > goto unregister_dev; > > > - } > > > > > > - ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad, > > > - MEDIA_LNK_FL_IMMUTABLE | > > > - MEDIA_LNK_FL_ENABLED); > > > - if (ret) > > > - goto unregister_dev; > > > + ret = media_create_pad_link(&csi->subdev.entity, > > > + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx, > > > + &vdev->entity, 0, > > > + MEDIA_LNK_FL_IMMUTABLE | > > > + MEDIA_LNK_FL_ENABLED); > > > + if (ret) { > > > + video_unregister_device(vdev); > > > + goto unregister_dev; > > > > Should you call media_entity_remove_links() on the csi2 entity on the > > error path ? > > > > Will fix. > > > > + } > > > + } > > > > > > ret = v4l2_device_register_subdev_nodes(&csi->v4l2_dev); > > > if (ret) > > > @@ -859,7 +881,7 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > > dma->state = TI_CSI2RX_DMA_ACTIVE; > > > spin_unlock_irqrestore(&dma->lock, flags); > > > > > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > > > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 1); > > > if (ret) > > > goto err_dma; > > > > > > @@ -887,7 +909,7 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > > > writel(0, csi->shim + SHIM_CNTL); > > > writel(0, csi->shim + SHIM_DMACNTX(ctx->idx)); > > > > > > - ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > > > + ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 0); > > > if (ret) > > > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > > > > > @@ -905,6 +927,119 @@ static const struct vb2_ops csi_vb2_qops = { > > > .wait_finish = vb2_ops_wait_finish, > > > }; > > > > > > +static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd) > > > +{ > > > + return container_of(sd, struct ti_csi2rx_dev, subdev); > > > +} > > > + > > > +static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state, > > > + struct v4l2_subdev_format *format) > > > +{ > > > + struct v4l2_mbus_framefmt *fmt; > > > + int ret = 0; > > > + > > > + /* No transcoding, don't allow setting source fmt */ > > > + if (format->pad >= TI_CSI2RX_PAD_FIRST_SOURCE) > > > > > > > TI_CSI2RX_PAD_SINK > > > > might be clearer > > Will fix. > > > > > > + return v4l2_subdev_get_fmt(sd, state, format); > > > + > > > + if (!find_format_by_code(format->format.code)) > > > + format->format.code = ti_csi2rx_formats[0].code; > > > + > > > + format->format.field = V4L2_FIELD_NONE; > > > > What about other colorspace related fields ? > > The HW does not care about colorspace/encoding etc. and acts as > passthrough, so we accept whatever userspace sets for those. > Or should it rather propagate to userspace what is set by the downstream entity ? > > > > > + > > > + fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream); > > > + if (!fmt) { > > > > Can this happen ? > > Yes __v4l2_subdev_state_get_format() may return NULL if userspace tried > to set format with an invalid `stream` (i.e. no routes exist for that > stream) > The core makes sure the stream is valid afaict See check_state() in v4l2-subdev.c > > > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + *fmt = format->format; > > > + > > > + fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE, > > > + format->stream); > > > + if (!fmt) { > > > + ret = -EINVAL; > > > > ditto > > > > > + goto out; > > > > kind of missing the point of a jump here where you can simply return > > -EINVAL > > Will fix. > > > > > > + } > > > + *fmt = format->format; > > > + > > > +out: > > > + return ret; > > > +} > > > + > > > +static int ti_csi2rx_sd_init_state(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + struct v4l2_subdev_format format = { > > > + .pad = TI_CSI2RX_PAD_SINK, > > > + .format = { > > > + .width = 640, > > > + .height = 480, > > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > > + .field = V4L2_FIELD_NONE, > > > + .colorspace = V4L2_COLORSPACE_SRGB, > > > + .ycbcr_enc = V4L2_YCBCR_ENC_601, > > > + .quantization = V4L2_QUANTIZATION_LIM_RANGE, > > > + .xfer_func = V4L2_XFER_FUNC_SRGB, > > > + }, > > > + }; > > > + > > > + return ti_csi2rx_sd_set_fmt(sd, state, &format); > > > +} > > > + > > > +static int ti_csi2rx_sd_s_stream(struct v4l2_subdev *sd, int enable) > > > > Even if I never used that API myself, but I have a feeling > > enable_streams() (which is meant to replace s_stream for multiplexed > > subdev) would help you avoiding the manual enable_count booking here > > below ? > > In PATCH 12/13 we switch to using enable_streams() APIs here. > > We still keep this enable_count bookkeeping, but I agree it can be > dropped with minor changes. Will fix in v3. > Thanks, if it gets troublesome to remove it and you later switch to the new API then don't bother > > > > > +{ > > > + struct ti_csi2rx_dev *csi = to_csi2rx_dev(sd); > > > + int ret = 0; > > > + > > > + mutex_lock(&csi->mutex); > > > + > > > + if (enable) { > > > + if (csi->enable_count > 0) { > > > + csi->enable_count++; > > > + goto out; > > > + } > > > + > > > + ret = v4l2_subdev_call(csi->source, video, s_stream, 1); > > > + if (ret) > > > + goto out; > > > + > > > + csi->enable_count++; > > > + } else { > > > + if (csi->enable_count == 0) { > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + > > > + if (--csi->enable_count > 0) > > > + goto out; > > > + > > > + ret = v4l2_subdev_call(csi->source, video, s_stream, 0); > > > + } > > > + > > > +out: > > > + mutex_unlock(&csi->mutex); > > > + return ret; > > > +} > > > + > > > +static const struct v4l2_subdev_pad_ops ti_csi2rx_subdev_pad_ops = { > > > + .get_fmt = v4l2_subdev_get_fmt, > > > + .set_fmt = ti_csi2rx_sd_set_fmt, > > > +}; > > > + > > > +static const struct v4l2_subdev_video_ops ti_csi2rx_subdev_video_ops = { > > > + .s_stream = ti_csi2rx_sd_s_stream, > > > +}; > > > + > > > +static const struct v4l2_subdev_ops ti_csi2rx_subdev_ops = { > > > + .video = &ti_csi2rx_subdev_video_ops, > > > + .pad = &ti_csi2rx_subdev_pad_ops, > > > +}; > > > + > > > +static const struct v4l2_subdev_internal_ops ti_csi2rx_internal_ops = { > > > + .init_state = ti_csi2rx_sd_init_state, > > > +}; > > > + > > > static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > > { > > > dma_release_channel(ctx->dma.chan); > > > @@ -912,6 +1047,7 @@ static void ti_csi2rx_cleanup_dma(struct ti_csi2rx_ctx *ctx) > > > > > > static void ti_csi2rx_cleanup_v4l2(struct ti_csi2rx_dev *csi) > > > { > > > + v4l2_subdev_cleanup(&csi->subdev); > > > media_device_unregister(&csi->mdev); > > > v4l2_device_unregister(&csi->v4l2_dev); > > > media_device_cleanup(&csi->mdev); > > > @@ -973,14 +1109,22 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > > struct v4l2_subdev_format source_fmt = { > > > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > > .pad = link->source->index, > > > + .stream = 0, > > > > I don't think it hurts, but this seems not to be strictly related to > > this patch ? > > > > > }; > > > + struct v4l2_subdev_state *state; > > > const struct ti_csi2rx_fmt *ti_fmt; > > > int ret; > > > > > > - ret = v4l2_subdev_call_state_active(csi->source, pad, > > > - get_fmt, &source_fmt); > > > - if (ret) > > > - return ret; > > > + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev); > > > + ret = v4l2_subdev_call(&csi->subdev, pad, get_fmt, state, &source_fmt); > > > + v4l2_subdev_unlock_state(state); > > > + > > > + if (ret) { > > > + dev_dbg(csi->dev, > > > + "Skipping validation as no format present on \"%s\":%u:0\n", > > > + link->source->entity->name, link->source->index); > > > > If get_fmt returns an error, should you simply continue or fail ? In > > which cases get_fmt on a subdev fails ? Only if the pad or the stream > > are not valid right ? > > The subdev introduced in this patch has multiple source pads, all > actively linked to separate video nodes, one for each DMA channel. > > We continue (return 0) here as we don't want __media_pipeline_start to > fail because of pads that are "unused" i.e. have no routes and no > formats set. > > If the user actually routes a stream to any pad the pipeline validation > will check that a format is also set (and thus the link will get > validated here). > > I wonder if we can keep the links mutable, and expect userspace to > disable links to unused video nodes. Will have to think more about that > approach. > We recently had a discussion about this (link enablement for 'active' dma paths) with Sakari and Laurent on the Mali-C55 ISP. Let's see if they have feedbacks. > > > > > + return 0; > > > + } > > > > > > if (source_fmt.format.width != csi_fmt->width) { > > > dev_dbg(csi->dev, "Width does not match (source %u, sink %u)\n", > > > @@ -1010,8 +1154,9 @@ static int ti_csi2rx_link_validate(struct media_link *link) > > > > > > if (ti_fmt->fourcc != csi_fmt->pixelformat) { > > > dev_dbg(csi->dev, > > > - "Cannot transform source fmt 0x%x to sink fmt 0x%x\n", > > > - ti_fmt->fourcc, csi_fmt->pixelformat); > > > + "Cannot transform \"%s\":%u format %p4cc to %p4cc\n", > > > + link->source->entity->name, link->source->index, > > > + &ti_fmt->fourcc, &csi_fmt->pixelformat); > > > return -EPIPE; > > > } > > > > > > @@ -1022,6 +1167,10 @@ static const struct media_entity_operations ti_csi2rx_video_entity_ops = { > > > .link_validate = ti_csi2rx_link_validate, > > > }; > > > > > > +static const struct media_entity_operations ti_csi2rx_subdev_entity_ops = { > > > + .link_validate = v4l2_subdev_link_validate, > > > +}; > > > + > > > static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > > { > > > struct dma_slave_config cfg = { > > > @@ -1053,7 +1202,8 @@ static int ti_csi2rx_init_dma(struct ti_csi2rx_ctx *ctx) > > > static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > > { > > > struct media_device *mdev = &csi->mdev; > > > - int ret; > > > + struct v4l2_subdev *sd = &csi->subdev; > > > + int ret, i; > > > > > > mdev->dev = csi->dev; > > > mdev->hw_revision = 1; > > > @@ -1065,16 +1215,50 @@ static int ti_csi2rx_v4l2_init(struct ti_csi2rx_dev *csi) > > > > > > ret = v4l2_device_register(csi->dev, &csi->v4l2_dev); > > > if (ret) > > > - return ret; > > > + goto cleanup_media; > > > > > > ret = media_device_register(mdev); > > > - if (ret) { > > > - v4l2_device_unregister(&csi->v4l2_dev); > > > - media_device_cleanup(mdev); > > > - return ret; > > > - } > > > + if (ret) > > > + goto unregister_v4l2; > > > + > > > + v4l2_subdev_init(sd, &ti_csi2rx_subdev_ops); > > > + sd->internal_ops = &ti_csi2rx_internal_ops; > > > + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > > + sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > > + strscpy(sd->name, dev_name(csi->dev), sizeof(sd->name)); > > > + sd->dev = csi->dev; > > > + sd->entity.ops = &ti_csi2rx_subdev_entity_ops; > > > + > > > + csi->pads[TI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > > > + > > > + for (i = TI_CSI2RX_PAD_FIRST_SOURCE; i < TI_CSI2RX_NUM_PADS; i++) > > > > for (unsigned int i = 0; > > Will fix. > > > > > > + csi->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > > + > > > + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(csi->pads), > > > + csi->pads); > > > + if (ret) > > > + goto unregister_media; > > > + > > > + ret = v4l2_subdev_init_finalize(sd); > > > + if (ret) > > > + goto unregister_media; > > > + > > > + ret = v4l2_device_register_subdev(&csi->v4l2_dev, sd); > > > + if (ret) > > > + goto cleanup_subdev; > > > > > > return 0; > > > + > > > +cleanup_subdev: > > > + v4l2_subdev_cleanup(sd); > > > +unregister_media: > > > + media_device_unregister(mdev); > > > +unregister_v4l2: > > > + v4l2_device_unregister(&csi->v4l2_dev); > > > +cleanup_media: > > > + media_device_cleanup(mdev); > > > + > > > + return ret; > > > } > > > > > > static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > > @@ -1101,9 +1285,9 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx) > > > > > > ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt); > > > > > > - csi->pad.flags = MEDIA_PAD_FL_SINK; > > > > Ah was this re-initializing the same pad multiple times ? > > Not sure I understand the comment fully. We have only 1 video context > here, more contexts will be introduced in subsequent patches. Yeah, I meant that before thsi patch there was a single csi->pad which I thought was re-initialized multiple times. But before this patch you had no contexts, so no re-initialization > > > > > > + ctx->pad.flags = MEDIA_PAD_FL_SINK; > > > vdev->entity.ops = &ti_csi2rx_video_entity_ops; > > > - ret = media_entity_pads_init(&ctx->vdev.entity, 1, &csi->pad); > > > + ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad); > > > if (ret) > > > return ret; > > > > > > @@ -1159,6 +1343,8 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > > > if (!csi->drain.vaddr) > > > return -ENOMEM; > > > > > > + mutex_init(&csi->mutex); > > > + > > > ret = ti_csi2rx_v4l2_init(csi); > > > if (ret) > > > goto err_v4l2; > > > @@ -1191,6 +1377,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev) > > > ti_csi2rx_cleanup_ctx(&csi->ctx[i]); > > > ti_csi2rx_cleanup_v4l2(csi); > > > err_v4l2: > > > + mutex_destroy(&csi->mutex); > > > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > > csi->drain.paddr); > > > return ret; > > > @@ -1213,7 +1400,7 @@ static void ti_csi2rx_remove(struct platform_device *pdev) > > > > > > ti_csi2rx_cleanup_notifier(csi); > > > ti_csi2rx_cleanup_v4l2(csi); > > > - > > > + mutex_destroy(&csi->mutex); > > > dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr, > > > csi->drain.paddr); > > > } > > > > > > -- > > > 2.43.0 > > > > > > > > -- > Thanks, > Jai > > GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145
On 27/06/2024 16:10, Jai Luthra wrote: > Each CSI2 stream can be multiplexed into 4 independent streams, each > identified by its virtual channel number. To capture this multiplexed > stream, the application needs to tell the driver how it wants to route > the data. It needs to specify which context should process which stream. > This is done via the new routing APIs. > > Add ioctls to accept routing information from the application and save > that in the driver. This can be used when starting streaming on a > context to determine which route and consequently which virtual channel > it should process. > > Support the new enable_stream()/disable_stream() APIs in the subdev > instead of s_stream() hook. We wait for the userspace to start capturing > on all video nodes that have active routes, and once all video nodes are > STREAMON, we request the source to enable_stream() for all the sink > streams. > > Co-developed-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 231 +++++++++++++++++---- > 1 file changed, 185 insertions(+), 46 deletions(-) > > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > index c0916ca1a6f8..84b972c251e8 100644 > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > @@ -124,6 +124,7 @@ struct ti_csi2rx_dev { > struct v4l2_subdev *source; > struct v4l2_subdev subdev; > struct ti_csi2rx_ctx ctx[TI_CSI2RX_MAX_CTX]; > + u64 streams_mask; /* Enabled sink streams */ > /* Buffer to drain stale data from PSI-L endpoint */ > struct { > void *vaddr; > @@ -535,10 +536,6 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx) > > fmt = find_format_by_fourcc(ctx->v_fmt.fmt.pix.pixelformat); > > - /* De-assert the pixel interface reset. */ > - reg = SHIM_CNTL_PIX_RST; > - writel(reg, csi->shim + SHIM_CNTL); > - > reg = SHIM_DMACNTX_EN; > reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt); > > @@ -881,8 +878,12 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > struct ti_csi2rx_dev *csi = ctx->csi; > struct ti_csi2rx_dma *dma = &ctx->dma; > struct ti_csi2rx_buffer *buf; > + struct v4l2_subdev_krouting *routing; > + struct v4l2_subdev_route *route = NULL; > + struct media_pad *remote_pad; > unsigned long flags; > - int ret = 0; > + int ret = 0, i; > + struct v4l2_subdev_state *state; > > spin_lock_irqsave(&dma->lock, flags); > if (list_empty(&dma->queue)) > @@ -895,6 +896,40 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > if (ret) > goto err; > > + remote_pad = media_entity_remote_source_pad_unique(ctx->pad.entity); > + if (!remote_pad) { > + ret = -ENODEV; > + goto err; > + } > + > + state = v4l2_subdev_lock_and_get_active_state(&csi->subdev); > + > + routing = &state->routing; > + > + /* Find the stream to process. */ > + for (i = 0; i < routing->num_routes; i++) { for_each_active_route() > + struct v4l2_subdev_route *r = &routing->routes[i]; > + > + if (!(r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)) > + continue; > + > + if (r->source_pad != remote_pad->index) > + continue; > + > + route = r; > + break; > + } > + > + if (!route) { > + ret = -ENODEV; > + v4l2_subdev_unlock_state(state); > + goto err; > + } > + > + ctx->stream = route->sink_stream; > + > + v4l2_subdev_unlock_state(state); > + > ret = ti_csi2rx_get_vc(ctx); > if (ret == -ENOIOCTLCMD) > ctx->vc = 0; > @@ -921,7 +956,10 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > dma->state = TI_CSI2RX_DMA_ACTIVE; > spin_unlock_irqrestore(&dma->lock, flags); > > - ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 1); > + /* Start stream 0, we don't allow multiple streams on the source pad */ > + ret = v4l2_subdev_enable_streams(&csi->subdev, > + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx, > + BIT(0)); > if (ret) > goto err_dma; > > @@ -944,12 +982,16 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq) > struct ti_csi2rx_dev *csi = ctx->csi; > int ret; > > - video_device_pipeline_stop(&ctx->vdev); > - > + /* assert pixel reset to prevent stale data */ > writel(0, csi->shim + SHIM_CNTL); > + > + video_device_pipeline_stop(&ctx->vdev); > writel(0, csi->shim + SHIM_DMACNTX(ctx->idx)); > > - ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 0); > + ret = v4l2_subdev_disable_streams(&csi->subdev, > + TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx, > + BIT(0)); > + > if (ret) > dev_err(csi->dev, "Failed to stop subdev stream\n"); > > @@ -995,8 +1037,8 @@ static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd, > } > *fmt = format->format; > > - fmt = v4l2_subdev_state_get_format(state, TI_CSI2RX_PAD_FIRST_SOURCE, > - format->stream); > + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > + format->stream); > if (!fmt) { > ret = -EINVAL; > goto out; > @@ -1007,72 +1049,169 @@ static int ti_csi2rx_sd_set_fmt(struct v4l2_subdev *sd, > return ret; > } > > +static int _ti_csi2rx_sd_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_krouting *routing) > +{ > + int ret; > + > + const struct v4l2_mbus_framefmt format = { static const Tomi
This series adds multi-stream support for Cadence CSI2RX and TI CSI2RX Shim drivers. PATCH 1: Runtime Power Management for Cadence CSI2RX PATCH 2-7: Support multiple DMA contexts/video nodes in TI CSI2RX PATCH 8-9: Use get_frame_desc to propagate virtual channel information across Cadence and TI CSI-RX subdevs PATCH 10-12: Use new multi-stream APIs across the drivers to support multiplexed cameras from sources like UB960 (FPDLink) PATCH 13: Optimize stream on by submitting all queued buffers to DMA This applies on top of today's linux-next (next-20240626) (also tested rebase with media_stage.git master) Signed-off-by: Jai Luthra <j-luthra@ti.com> --- Changes in v2: - Change the multi-camera capture architecture to be similar to that of Tomi's RPi5 FE series, where the driver will wait for userspace to start streaming on all "actively routed" video nodes before starting streaming on the source. This simplifies things a lot from the HW perspective, which might run into deadlocks due to a shared FIFO between multiple DMA channels. - Drop a few fixes that were posted separately and are already merged - Fix dtschema warnings reported by Rob on [02/13] - Fix warnings for uninitialized `used_vc` variable in cdns-csi2rx.c - Return -EBUSY if someone updates routes for j721e-csi2rx subdev while streaming - Only allow single-streams to be routed to the source pads (linked to video nodes) of the j721e-csi2rx device - Squash the patches marked "SQUASH" in the v1 RFC series - Link to RFC (v1): https://lore.kernel.org/r/20240222-multistream-v1-0-1837ed916eeb@ti.com --- Jai Luthra (8): dt-bindings: media: ti,j721e-csi2rx-shim: Support 32 dma chans media: ti: j721e-csi2rx: separate out device and context media: ti: j721e-csi2rx: add a subdev for the core device media: ti: j721e-csi2rx: add support for processing virtual channels media: cadence: csi2rx: Use new enable stream APIs media: cadence: csi2rx: Enable multi-stream support media: ti: j721e-csi2rx: add multistream support media: ti: j721e-csi2rx: Submit all available buffers Jayshri Pawar (1): media: cadence: csi2rx: Support runtime PM Pratyush Yadav (4): media: ti: j721e-csi2rx: prepare SHIM code for multiple contexts media: ti: j721e-csi2rx: allocate DMA channel based on context index media: ti: j721e-csi2rx: get number of contexts from device tree media: cadence: csi2rx: add get_frame_desc wrapper .../bindings/media/ti,j721e-csi2rx-shim.yaml | 39 +- drivers/media/platform/cadence/cdns-csi2rx.c | 440 +++++++++-- .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 879 ++++++++++++++++----- 3 files changed, 1071 insertions(+), 287 deletions(-) --- base-commit: df9574a57d02b265322e77fb8628d4d33641dda9 change-id: 20240221-multistream-fbba6ffe47a3 Best regards,