mbox series

[v2,00/13] media: cadence,ti: CSI2RX Multistream Support

Message ID 20240627-multistream-v2-0-6ae96c54c1c3@ti.com
Headers show
Series media: cadence,ti: CSI2RX Multistream Support | expand

Message

Jai Luthra June 27, 2024, 1:09 p.m. UTC
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,

Comments

Tomi Valkeinen June 28, 2024, 8:14 a.m. UTC | #1
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);
>
Changhuang Liang June 28, 2024, 8:45 a.m. UTC | #2
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
Jai Luthra June 28, 2024, 9:39 a.m. UTC | #3
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
Tomi Valkeinen June 28, 2024, 9:53 a.m. UTC | #4
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
>>
>
Jai Luthra June 28, 2024, 10:29 a.m. UTC | #5
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
> > > 
> > 
>
Jai Luthra July 1, 2024, 7:49 a.m. UTC | #6
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
> 
> [...] 
>
Jai Luthra July 1, 2024, 12:33 p.m. UTC | #7
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