* [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H
@ 2025-07-04 11:20 Daniel Scally
2025-07-04 11:20 ` [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops Daniel Scally
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
Hello all
This series adds a driver for the Input Video Control Block in the
RZ/V2H SoC. The IVC block transmits input image data from memory to
the ISP core (on this SoC, a Mali-C55 ISP). The driver registers an
output video device for userspace to queue image buffers to. One
noteworthy feature is that - because it is not a part of the main ISP
drive - the IVC driver also registers a subdevice, which connects to
the media device created by the ISP driver through the usual v4l2
async framework. This requires delaying the registration of the video
device until the .registered() callback of the subdevice, so that the
struct v4l2_dev pointer the subdevice connected to can be set to the
video device.
This version of the driver drops the reliance on the new media
framework that was posted recently [1], so can be merged without it
and updated later. One patch from that series is retained here though
(since it's used independently of the new framework) and another new
patch added to add a new helper in V4L2. The series is also based on
top of the latest version of the Mali-C55 driver [2] and some updates
to rzg2l-cru [3].
Thanks
Dan
[1] https://lore.kernel.org/linux-media/20250624-media-jobs-v2-0-8e649b069a96@ideasonboard.com/T/#t
[2] https://lore.kernel.org/linux-media/20250624-c55-v10-0-54f3d4196990@ideasonboard.com/T/#t
[3] https://lore.kernel.org/linux-media/20250625-rzg2l-cru-v6-0-a9099ed26c14@ideasonboard.com/T
---
Changes in v3:
- Added two new patches that create helpers in V4L2 and mc core that
the driver then consumes.
- Link to v2: https://lore.kernel.org/r/20250624-ivc-v2-0-e4ecdddb0a96@ideasonboard.com
---
Daniel Scally (5):
media: mc: entity: Add pipeline_started/stopped ops
media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
dt-bindings: media: Add bindings for the RZ/V2H IVC block
media: platform: Add Renesas Input Video Control block driver
MAINTAINERS: Add entry for rzv2h-ivc driver
.../bindings/media/renesas,r9a09g057-ivc.yaml | 103 ++++
MAINTAINERS | 7 +
drivers/media/mc/mc-entity.c | 46 ++
drivers/media/platform/renesas/Kconfig | 2 +
drivers/media/platform/renesas/Makefile | 1 +
drivers/media/platform/renesas/rzv2h-ivc/Kconfig | 16 +
drivers/media/platform/renesas/rzv2h-ivc/Makefile | 5 +
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c | 228 +++++++++
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c | 376 ++++++++++++++
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c | 568 +++++++++++++++++++++
.../media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h | 131 +++++
drivers/media/v4l2-core/v4l2-dev.c | 57 +++
include/media/media-entity.h | 29 ++
include/media/v4l2-dev.h | 36 ++
14 files changed, 1605 insertions(+)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250624-ivc-833d24376167
prerequisite-patch-id: ae1f5045379f5944df15d0f62eaca9803654ae33
prerequisite-patch-id: ff9cfd027783e4943f1281d793dcaf447964c724
prerequisite-patch-id: a8de5362397c6a4b1d8f43acb123ebbdc4102192
prerequisite-patch-id: df748b118c52fd426b2701079da8fc0001a71807
prerequisite-patch-id: a2e584a5b189b97973aab601073c6af0e760ca18
prerequisite-patch-id: 256864ec0ddbfc3a6e7eb5aec15ad3cbe2dbe477
prerequisite-patch-id: ecc5483454fc52289c093e711d5423e1cdd8bc3b
prerequisite-patch-id: 1aea6316a2a4a7b56316dbef3ca6034de6ec1672
Best regards,
--
Daniel Scally <dan.scally@ideasonboard.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
@ 2025-07-04 11:20 ` Daniel Scally
2025-07-08 13:29 ` Jacopo Mondi
2025-07-04 11:20 ` [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]() Daniel Scally
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
Add two new members to struct media_entity_operations, along with new
functions in media-entity.c to traverse a media pipeline and call the
new operations. The new functions are intended to be used to signal
to a media pipeline that it has fully started, with the entity ops
allowing drivers to define some action to be taken when those
conditions are met.
The combination of the new functions and operations allows drivers
which are part of a multi-driver pipeline to delay actually starting
streaming until all of the conditions for streaming succcessfully are
met across all drivers.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v4:
- Reverted to having the iter variable
Changes in v3:
- Dropped the iter variable now that the pipeline entity
iterator functions don't need it.
- Updated documentation to specify Optional and return
values
Changes in v2:
- Refactored media_pipeline_started() such that the cleanup
function for media_pipeline_entity_iter is unconditionally
called
- Avoided using media_entity_call() helper for operation that
has return type void to avoid compiler warnings
---
drivers/media/mc/mc-entity.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
include/media/media-entity.h | 29 ++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 045590905582054c46656e20463271b1f93fa6b4..d3443537d4304e12cb015630101efba22375c011 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1053,6 +1053,52 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
}
EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
+int media_pipeline_started(struct media_pipeline *pipe)
+{
+ struct media_pipeline_entity_iter iter;
+ struct media_entity *entity;
+ int ret;
+
+ ret = media_pipeline_entity_iter_init(pipe, &iter);
+ if (ret)
+ return ret;
+
+ media_pipeline_for_each_entity(pipe, &iter, entity) {
+ ret = media_entity_call(entity, pipeline_started);
+ if (ret && ret != -ENOIOCTLCMD)
+ break;
+ }
+
+ media_pipeline_entity_iter_cleanup(&iter);
+
+ ret = ret == -ENOIOCTLCMD ? 0 : ret;
+ if (ret)
+ media_pipeline_stopped(pipe);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(media_pipeline_started);
+
+int media_pipeline_stopped(struct media_pipeline *pipe)
+{
+ struct media_pipeline_entity_iter iter;
+ struct media_entity *entity;
+ int ret;
+
+ ret = media_pipeline_entity_iter_init(pipe, &iter);
+ if (ret)
+ return ret;
+
+ media_pipeline_for_each_entity(pipe, &iter, entity)
+ if (entity->ops && entity->ops->pipeline_stopped)
+ entity->ops->pipeline_stopped(entity);
+
+ media_pipeline_entity_iter_cleanup(&iter);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_pipeline_stopped);
+
/* -----------------------------------------------------------------------------
* Links management
*/
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 64cf590b11343f68a456c5870ca2f32917c122f9..ad658f42357ec505c84d9479bbbf18494da7f939 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -269,6 +269,10 @@ struct media_pad {
* media_entity_has_pad_interdep().
* Optional: If the operation isn't implemented all pads
* will be considered as interdependent.
+ * @pipeline_started: Notify this entity that the pipeline it is a part of has
+ * been started
+ * @pipeline_stopped: Notify this entity that the pipeline it is a part of has
+ * been stopped
*
* .. note::
*
@@ -284,6 +288,8 @@ struct media_entity_operations {
int (*link_validate)(struct media_link *link);
bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
unsigned int pad1);
+ int (*pipeline_started)(struct media_entity *entity);
+ void (*pipeline_stopped)(struct media_entity *entity);
};
/**
@@ -1261,6 +1267,29 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
entity != NULL; \
entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
+/**
+ * media_pipeline_started - Inform entities in a pipeline that it has started
+ * @pipe: The pipeline
+ *
+ * Iterate on all entities in a media pipeline and call their pipeline_started
+ * member of media_entity_operations. Optional.
+ *
+ * Return: zero on success, or a negative error code passed through from an
+ * entity's .pipeline_started() operation.
+ */
+int media_pipeline_started(struct media_pipeline *pipe);
+
+/**
+ * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
+ * @pipe: The pipeline
+ *
+ * Iterate on all entities in a media pipeline and call their pipeline_stopped
+ * member of media_entity_operations. Optional.
+ *
+ * Return: zero on success, or -ENOMEM if the iterator initialisation failed.
+ */
+int media_pipeline_stopped(struct media_pipeline *pipe);
+
/**
* media_pipeline_alloc_start - Mark a pipeline as streaming
* @pad: Starting pad
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
2025-07-04 11:20 ` [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops Daniel Scally
@ 2025-07-04 11:20 ` Daniel Scally
2025-07-08 13:10 ` Jacopo Mondi
2025-07-04 11:20 ` [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block Daniel Scally
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
Add helpers to run the new media_pipeline_started() and
media_pipeline_stopped() functions. The helpers iterate over the
entities in the pipeline and count the number of video devices and
compare that to the pipeline's start_count() before acting. This
allows us to only run the media pipeline callbacks in the event that
the pipeline has had video_pipeline_start() called for each video
device.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
We could take this further perhaps and include the equivalent routine
in video_device_pipeline[_alloc]_start()...if none of the entities
involved have .pipeline_started() or .pipeline_stopped() operations it
should be harmless, but I'm a bit reluctant to force the choice to run
those operations on users.
Changes in v2:
- Adapted now media_pipeline_for_each_entity() takes an iter
variable
- Fixed the Return: section of the kerneldoc comments
---
drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
2 files changed, 93 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
}
EXPORT_SYMBOL_GPL(video_device_pipeline);
+static int __video_device_pipeline_started(struct media_pipeline *pipe)
+{
+ struct media_pipeline_entity_iter iter;
+ unsigned int n_video_devices = 0;
+ struct media_entity *entity;
+ int ret;
+
+ ret = media_pipeline_entity_iter_init(pipe, &iter);
+ if (ret)
+ return ret;
+
+ media_pipeline_for_each_entity(pipe, &iter, entity) {
+ if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
+ n_video_devices++;
+ }
+
+ media_pipeline_entity_iter_cleanup(&iter);
+
+ return n_video_devices - pipe->start_count;
+}
+
+int video_device_pipeline_started(struct video_device *vdev)
+{
+ struct media_pipeline *pipe;
+ int ret;
+
+ pipe = video_device_pipeline(vdev);
+ if (!pipe)
+ return -ENODEV;
+
+ ret = __video_device_pipeline_started(pipe);
+ if (ret)
+ return ret;
+
+ return media_pipeline_started(pipe);
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_started);
+
+int video_device_pipeline_stopped(struct video_device *vdev)
+{
+ struct media_pipeline *pipe;
+ int ret;
+
+ pipe = video_device_pipeline(vdev);
+ if (!pipe)
+ return -ENODEV;
+
+ ret = __video_device_pipeline_started(pipe);
+ if (ret)
+ return ret;
+
+ media_pipeline_stopped(pipe);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
+
#endif /* CONFIG_MEDIA_CONTROLLER */
/*
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
*/
struct media_pipeline *video_device_pipeline(struct video_device *vdev);
+/**
+ * video_device_pipeline_started - Run the pipeline_started() entity operation
+ * for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
+ * streaming through the use of video_device_pipeline_start() or one of its
+ * equivalent functions. If so, media_pipeline_started() is called to inform
+ * entities in the pipeline of that fact. The intention is to provide drivers
+ * with a shortcut for checking whether their pipeline is fully ready to start
+ * processing data.
+ *
+ * Return: The number of video devices in the pipeline remaining to be started,
+ * or a negative error number on failure.
+ */
+int video_device_pipeline_started(struct video_device *vdev);
+
+/**
+ * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
+ * for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
+ * streaming through the use of video_device_pipeline_start() or one of its
+ * equivalent functions. If so, media_pipeline_stopped() is called for each
+ * entity in the pipeline. The intention is to provide drivers with a shortcut
+ * for checking whether this video device is the first device in the pipeline
+ * to be stopped.
+ *
+ * Return: The number of video devices in the pipeline remaining to be started, or a
+ * negative error number on failure.
+ */
+int video_device_pipeline_stopped(struct video_device *vdev);
+
#endif /* CONFIG_MEDIA_CONTROLLER */
#endif /* _V4L2_DEV_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
2025-07-04 11:20 ` [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops Daniel Scally
2025-07-04 11:20 ` [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]() Daniel Scally
@ 2025-07-04 11:20 ` Daniel Scally
2025-07-07 6:39 ` Krzysztof Kozlowski
2025-07-04 11:20 ` [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver Daniel Scally
2025-07-04 11:20 ` [PATCH v3 5/5] MAINTAINERS: Add entry for rzv2h-ivc driver Daniel Scally
4 siblings, 1 reply; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
The RZ/V2H SoC has a block called the Input Video Control block which
feeds image data into the Image Signal Processor. Add dt bindings to
describe the IVC.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v3:
- Rename from rzv2h-ivc.yaml to r9a09g057-ivc.yaml
- Update clock and reset names
Changes in v2:
- compatible matches filename
- Added power-domains
- Aligned clock and reset entries on opening "<"
- Removed status = "okay"; from example
---
.../bindings/media/renesas,r9a09g057-ivc.yaml | 103 +++++++++++++++++++++
1 file changed, 103 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/renesas,r9a09g057-ivc.yaml b/Documentation/devicetree/bindings/media/renesas,r9a09g057-ivc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..3ecccf0a4b05ffcf90c130924bfe50065b06f87e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,r9a09g057-ivc.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/renesas,r9a09g057-ivc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/V2H Input Video Control Block
+
+maintainers:
+ - Daniel Scally <dan.scally@ideasonboard.com>
+
+description:
+ The IVC block is a module that takes video frames from memory and feeds them
+ to the Image Signal Processor for processing.
+
+properties:
+ compatible:
+ const: renesas,r9a09g057-ivc
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: Input Video Control block register access clock
+ - description: Video input data AXI bus clock
+ - description: ISP system clock
+
+ clock-names:
+ items:
+ - const: reg
+ - const: axi
+ - const: isp
+
+ power-domains:
+ maxItems: 1
+
+ resets:
+ items:
+ - description: Input Video Control block register access reset
+ - description: Video input data AXI bus reset
+ - description: ISP core reset
+
+ reset-names:
+ items:
+ - const: reg
+ - const: axi
+ - const: isp
+
+ port:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: Output parallel video bus
+
+ properties:
+ endpoint:
+ $ref: /schemas/graph.yaml#/properties/endpoint
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - power-domains
+ - resets
+ - reset-names
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/renesas,r9a09g057-cpg.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ isp-input@16040000 {
+ compatible = "renesas,r9a09g057-ivc";
+ reg = <0x16040000 0x230>;
+
+ clocks = <&cpg CPG_MOD 0xe3>,
+ <&cpg CPG_MOD 0xe4>,
+ <&cpg CPG_MOD 0xe5>;
+ clock-names = "reg", "axi", "isp";
+
+ power-domains = <&cpg>;
+
+ resets = <&cpg 0xd4>,
+ <&cpg 0xd1>,
+ <&cpg 0xd3>;
+ reset-names = "reg", "axi", "isp";
+
+ interrupts = <GIC_SPI 861 IRQ_TYPE_EDGE_RISING>;
+
+ port {
+ ivc_out: endpoint {
+ remote-endpoint = <&isp_in>;
+ };
+ };
+ };
+...
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
` (2 preceding siblings ...)
2025-07-04 11:20 ` [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block Daniel Scally
@ 2025-07-04 11:20 ` Daniel Scally
2025-07-08 14:49 ` Jacopo Mondi
2025-07-04 11:20 ` [PATCH v3 5/5] MAINTAINERS: Add entry for rzv2h-ivc driver Daniel Scally
4 siblings, 1 reply; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
Add a driver for the Input Video Control block in an RZ/V2H SoC which
feeds data into the Arm Mali-C55 ISP.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v4:
- Update the compatible to renesas,r9a09g057-ivc
- Dropped the media jobs / scheduler functionality, and re
worked the driver to have its own workqueue pushing frames
- Fix .enum_mbus_code() to return 20-bit output for source
pad.
- Fix some alignment issues
- Make the forwarding of sink to source pad format a more
explicit operation.
- Rename rzv2h_initialise_video_device_and_queue()
- Reversed order of v4l2_subdev_init_finalize() and
v4l2_async_register_subdev() to make sure everything is
finished initialising before registering the subdev.
- Change function to MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
- Use a parametised macro for min vblank
- Minor formatting
- Use the DEFAULT macros for quantization / ycbcr_enc values
- Switch to using the mplane API
- Dropped select RESET_CONTROLLER
- Used the new helpers for starting a media pipeline
- Switch from threaded irq to normal with driver workqueue
and revised startup routine
Changes in v3:
- Account for the renamed CRU pixel formats
Changes in v2:
- Added selects and depends statements to Kconfig entry
- Fixed copyright year
- Stopped including in .c files headers already included in .h
- Fixed uninitialized variable in iterator
- Only check vvalid member in interrupt function and wait
unconditionally elsewhere
- __maybe_unused for the PM ops
- Initialise the subdevice after setting up PM
- Fixed the remove function for the driver to actually do
something.
- Some minor formatting changes
- Fixed the quantization member for the format
- Changes accounting for the v2 of the media jobs framework
- Change min_queued_buffers to 0
---
drivers/media/platform/renesas/Kconfig | 2 +
drivers/media/platform/renesas/Makefile | 1 +
drivers/media/platform/renesas/rzv2h-ivc/Kconfig | 16 +
drivers/media/platform/renesas/rzv2h-ivc/Makefile | 5 +
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c | 228 +++++++++
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c | 376 ++++++++++++++
.../platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c | 568 +++++++++++++++++++++
.../media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h | 131 +++++
8 files changed, 1327 insertions(+)
diff --git a/drivers/media/platform/renesas/Kconfig b/drivers/media/platform/renesas/Kconfig
index 27a54fa7908384f2e8200f0f7283a82b0ae8435c..5462e524c3708be87a50dd80d4b4017a2466aa99 100644
--- a/drivers/media/platform/renesas/Kconfig
+++ b/drivers/media/platform/renesas/Kconfig
@@ -42,6 +42,8 @@ config VIDEO_SH_VOU
source "drivers/media/platform/renesas/rcar-isp/Kconfig"
source "drivers/media/platform/renesas/rcar-vin/Kconfig"
source "drivers/media/platform/renesas/rzg2l-cru/Kconfig"
+source "drivers/media/platform/renesas/rzv2h-ivc/Kconfig"
+
# Mem2mem drivers
diff --git a/drivers/media/platform/renesas/Makefile b/drivers/media/platform/renesas/Makefile
index 1127259c09d6a51b70803e76c495918e06777f67..b6b4abf01db246aaf8269b8027efee9b0b32083a 100644
--- a/drivers/media/platform/renesas/Makefile
+++ b/drivers/media/platform/renesas/Makefile
@@ -6,6 +6,7 @@
obj-y += rcar-isp/
obj-y += rcar-vin/
obj-y += rzg2l-cru/
+obj-y += rzv2h-ivc/
obj-y += vsp1/
obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Kconfig b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
new file mode 100644
index 0000000000000000000000000000000000000000..bde2ba52329b522cccd63667f376edb2f4a0608f
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config VIDEO_RZV2H_IVC
+ tristate "Renesas RZ/V2H Input Video Control block driver"
+ depends on V4L_PLATFORM_DRIVERS
+ depends on VIDEO_DEV
+ depends on ARCH_RENESAS || COMPILE_TEST
+ depends on OF
+ select VIDEOBUF2_DMA_CONTIG
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ help
+ Support for the Video Input Block found in the RZ/V2H SoC. The IVC
+ block is used to read data from memory and forward it to the ISP that
+ is integrated to the SoC. Enable this to support the block, and by
+ extension the ISP to which it feeds data.
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Makefile b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..080ee3570f09c236d87abeaea5d8dd578fefb6d3
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+rzv2h-ivc-y := rzv2h-ivc-dev.o rzv2h-ivc-subdev.o rzv2h-ivc-video.o
+
+obj-$(CONFIG_VIDEO_RZV2H_IVC) += rzv2h-ivc.o
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
new file mode 100644
index 0000000000000000000000000000000000000000..841fa2d17df24ffe982a1d3237ca7c0491c290c4
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H Input Video Control Block driver
+ *
+ * Copyright (C) 2025 Ideas on Board Oy
+ */
+
+#include "rzv2h-ivc.h"
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+inline void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val)
+{
+ writel(val, ivc->base + addr);
+}
+
+void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
+ u32 mask, u32 val)
+{
+ u32 orig, new;
+
+ orig = readl(ivc->base + addr);
+
+ new = orig & ~mask;
+ new |= val & mask;
+
+ if (new != orig)
+ writel(new, ivc->base + addr);
+}
+
+static int rzv2h_ivc_get_hardware_resources(struct rzv2h_ivc *ivc,
+ struct platform_device *pdev)
+{
+ const char * const resource_names[RZV2H_IVC_NUM_HW_RESOURCES] = {
+ "reg",
+ "axi",
+ "isp",
+ };
+ struct resource *res;
+ int ret;
+
+ ivc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+ if (IS_ERR(ivc->base))
+ return dev_err_probe(ivc->dev, PTR_ERR(ivc->base),
+ "failed to map IO memory\n");
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
+ ivc->clks[i].id = resource_names[i];
+
+ ret = devm_clk_bulk_get(ivc->dev, ARRAY_SIZE(resource_names), ivc->clks);
+ if (ret)
+ return dev_err_probe(ivc->dev, ret, "failed to acquire clks\n");
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
+ ivc->resets[i].id = resource_names[i];
+
+ ret = devm_reset_control_bulk_get_optional_shared(
+ ivc->dev, ARRAY_SIZE(resource_names), ivc->resets);
+ if (ret)
+ return dev_err_probe(ivc->dev, ret, "failed to acquire resets\n");
+
+ return 0;
+}
+
+static void rzv2h_ivc_global_config(struct rzv2h_ivc *ivc)
+{
+ /* Currently we only support single-exposure input */
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PLNUM, RZV2H_IVC_ONE_EXPOSURE);
+
+ /*
+ * Datasheet says we should disable the interrupts before changing mode
+ * to avoid spurious IFP interrupt.
+ */
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, 0x0);
+
+ /*
+ * RZ/V2H documentation says software controlled configuration is not
+ * supported, and currently neither is multi-context mode. That being so
+ * we just set single context sw-hw mode.
+ */
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_CONTEXT,
+ RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG);
+
+ /*
+ * We enable the frame end interrupt so that we know when we should send
+ * follow-up frames.
+ */
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, RZV2H_IVC_VVAL_IFPE);
+}
+
+static irqreturn_t rzv2h_ivc_isr(int irq, void *context)
+{
+ struct device *dev = context;
+ struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
+
+ guard(spinlock)(&ivc->spinlock);
+
+ if (!--ivc->vvalid_ifp)
+ queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
+
+ return IRQ_HANDLED;
+}
+
+static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
+{
+ struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
+ int ret;
+
+ ret = request_irq(ivc->irqnum, rzv2h_ivc_isr, 0, dev_driver_string(dev),
+ dev);
+ if (ret) {
+ dev_err(dev, "failed to request irq\n");
+ return ret;
+ }
+
+ ret = clk_bulk_prepare_enable(ARRAY_SIZE(ivc->clks), ivc->clks);
+ if (ret) {
+ dev_err(ivc->dev, "failed to enable clocks\n");
+ goto err_free_irqnum;
+ }
+
+ ret = reset_control_bulk_deassert(ARRAY_SIZE(ivc->resets), ivc->resets);
+ if (ret) {
+ dev_err(ivc->dev, "failed to deassert resets\n");
+ goto err_disable_clks;
+ }
+
+ rzv2h_ivc_global_config(ivc);
+
+ return 0;
+
+err_disable_clks:
+ clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
+err_free_irqnum:
+ free_irq(ivc->irqnum, dev);
+
+ return ret;
+}
+
+static int __maybe_unused rzv2h_ivc_runtime_suspend(struct device *dev)
+{
+ struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
+
+ reset_control_bulk_assert(ARRAY_SIZE(ivc->resets), ivc->resets);
+ clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
+ free_irq(ivc->irqnum, dev);
+
+ return 0;
+}
+
+static const struct dev_pm_ops rzv2h_ivc_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+ SET_RUNTIME_PM_OPS(rzv2h_ivc_runtime_suspend, rzv2h_ivc_runtime_resume,
+ NULL)
+};
+
+static int rzv2h_ivc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rzv2h_ivc *ivc;
+ int ret;
+
+ ivc = devm_kzalloc(dev, sizeof(*ivc), GFP_KERNEL);
+ if (!ivc)
+ return -ENOMEM;
+
+ ivc->dev = dev;
+ platform_set_drvdata(pdev, ivc);
+ mutex_init(&ivc->lock);
+ spin_lock_init(&ivc->spinlock);
+
+ ret = rzv2h_ivc_get_hardware_resources(ivc, pdev);
+ if (ret)
+ return ret;
+
+ pm_runtime_set_autosuspend_delay(dev, 2000);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_enable(dev);
+
+ ivc->irqnum = platform_get_irq(pdev, 0);
+ if (ivc->irqnum < 0) {
+ dev_err(dev, "failed to get interrupt\n");
+ return ret;
+ }
+
+ ret = rzv2h_ivc_initialise_subdevice(ivc);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void rzv2h_ivc_remove(struct platform_device *pdev)
+{
+ struct rzv2h_ivc *ivc = platform_get_drvdata(pdev);
+
+ rzv2h_deinit_video_dev_and_queue(ivc);
+ rzv2h_ivc_deinit_subdevice(ivc);
+ mutex_destroy(&ivc->lock);
+}
+
+static const struct of_device_id rzv2h_ivc_of_match[] = {
+ { .compatible = "renesas,r9a09g057-ivc", },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rzv2h_ivc_of_match);
+
+static struct platform_driver rzv2h_ivc_driver = {
+ .driver = {
+ .name = "rzv2h-ivc",
+ .of_match_table = rzv2h_ivc_of_match,
+ .pm = &rzv2h_ivc_pm_ops,
+ },
+ .probe = rzv2h_ivc_probe,
+ .remove = rzv2h_ivc_remove,
+};
+
+module_platform_driver(rzv2h_ivc_driver);
+
+MODULE_AUTHOR("Daniel Scally <dan.scally@ideasonboard.com>");
+MODULE_DESCRIPTION("Renesas RZ/V2H Input Video Control Block driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
new file mode 100644
index 0000000000000000000000000000000000000000..ff3eac313994b0c109004ca7ba960f7edf1d2112
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H Input Video Control Block driver
+ *
+ * Copyright (C) 2025 Ideas on Board Oy
+ */
+
+#include "rzv2h-ivc.h"
+
+#include <linux/media.h>
+#include <linux/media-bus-format.h>
+#include <linux/v4l2-mediabus.h>
+
+#include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-event.h>
+
+#define RZV2H_IVC_N_INPUTS_PER_OUTPUT 6
+
+/*
+ * We support 8/10/12/14/16/20 bit input in any bayer order, but the output
+ * format is fixed at 20-bits with the same order as the input.
+ */
+static const struct {
+ u32 inputs[RZV2H_IVC_N_INPUTS_PER_OUTPUT];
+ u32 output;
+} rzv2h_ivc_formats[] = {
+ {
+ .inputs = {
+ MEDIA_BUS_FMT_SBGGR8_1X8,
+ MEDIA_BUS_FMT_SBGGR10_1X10,
+ MEDIA_BUS_FMT_SBGGR12_1X12,
+ MEDIA_BUS_FMT_SBGGR14_1X14,
+ MEDIA_BUS_FMT_SBGGR16_1X16,
+ MEDIA_BUS_FMT_SBGGR20_1X20,
+ },
+ .output = MEDIA_BUS_FMT_SBGGR20_1X20
+ },
+ {
+ .inputs = {
+ MEDIA_BUS_FMT_SGBRG8_1X8,
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+ MEDIA_BUS_FMT_SGBRG12_1X12,
+ MEDIA_BUS_FMT_SGBRG14_1X14,
+ MEDIA_BUS_FMT_SGBRG16_1X16,
+ MEDIA_BUS_FMT_SGBRG20_1X20,
+ },
+ .output = MEDIA_BUS_FMT_SGBRG20_1X20
+ },
+ {
+ .inputs = {
+ MEDIA_BUS_FMT_SGRBG8_1X8,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SGRBG12_1X12,
+ MEDIA_BUS_FMT_SGRBG14_1X14,
+ MEDIA_BUS_FMT_SGRBG16_1X16,
+ MEDIA_BUS_FMT_SGRBG20_1X20,
+ },
+ .output = MEDIA_BUS_FMT_SGRBG20_1X20
+ },
+ {
+ .inputs = {
+ MEDIA_BUS_FMT_SRGGB8_1X8,
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SRGGB12_1X12,
+ MEDIA_BUS_FMT_SRGGB14_1X14,
+ MEDIA_BUS_FMT_SRGGB16_1X16,
+ MEDIA_BUS_FMT_SRGGB20_1X20,
+ },
+ .output = MEDIA_BUS_FMT_SRGGB20_1X20
+ },
+};
+
+static u32 rzv2h_ivc_get_mbus_output_from_input(u32 mbus_code)
+{
+ unsigned int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++) {
+ for (j = 0; j < RZV2H_IVC_N_INPUTS_PER_OUTPUT; j++) {
+ if (rzv2h_ivc_formats[i].inputs[j] == mbus_code)
+ return rzv2h_ivc_formats[i].output;
+ }
+ }
+
+ return 0;
+}
+
+static int rzv2h_ivc_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ const struct v4l2_mbus_framefmt *fmt;
+ unsigned int order_index;
+ unsigned int index;
+
+ /*
+ * On the source pad, only the 20-bit format corresponding to the sink
+ * pad format's bayer order is supported.
+ */
+ if (code->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
+ if (code->index)
+ return -EINVAL;
+
+ fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SINK_PAD);
+ code->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code);
+
+ return 0;
+ }
+
+ if (code->index >= ARRAY_SIZE(rzv2h_ivc_formats) *
+ RZV2H_IVC_N_INPUTS_PER_OUTPUT)
+ return -EINVAL;
+
+ order_index = code->index / RZV2H_IVC_N_INPUTS_PER_OUTPUT;
+ index = code->index % RZV2H_IVC_N_INPUTS_PER_OUTPUT;
+
+ code->code = rzv2h_ivc_formats[order_index].inputs[index];
+
+ return 0;
+}
+
+static int rzv2h_ivc_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ const struct v4l2_mbus_framefmt *fmt;
+
+ if (fse->index > 0)
+ return -EINVAL;
+
+ if (fse->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
+ fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SINK_PAD);
+
+ if (fse->code != fmt->code)
+ return -EINVAL;
+
+ fse->min_width = fmt->width;
+ fse->max_width = fmt->width;
+ fse->min_height = fmt->height;
+ fse->max_height = fmt->height;
+
+ return 0;
+ }
+
+ if (!rzv2h_ivc_get_mbus_output_from_input(fse->code))
+ return -EINVAL;
+
+ fse->min_width = RZV2H_IVC_MIN_WIDTH;
+ fse->max_width = RZV2H_IVC_MAX_WIDTH;
+ fse->min_height = RZV2H_IVC_MIN_HEIGHT;
+ fse->max_height = RZV2H_IVC_MAX_HEIGHT;
+
+ return 0;
+}
+
+static int rzv2h_ivc_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_format *format)
+{
+ struct v4l2_mbus_framefmt *fmt = &format->format;
+ struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
+
+ if (format->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD)
+ return v4l2_subdev_get_fmt(sd, state, format);
+
+ sink_fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SINK_PAD);
+
+ sink_fmt->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code) ?
+ fmt->code : rzv2h_ivc_formats[0].inputs[0];
+
+ sink_fmt->width = clamp(fmt->width, RZV2H_IVC_MIN_WIDTH,
+ RZV2H_IVC_MAX_WIDTH);
+ sink_fmt->height = clamp(fmt->height, RZV2H_IVC_MIN_HEIGHT,
+ RZV2H_IVC_MAX_HEIGHT);
+
+ *fmt = *sink_fmt;
+
+ src_fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SOURCE_PAD);
+ *src_fmt = *sink_fmt;
+ src_fmt->code = rzv2h_ivc_get_mbus_output_from_input(sink_fmt->code);
+
+ return 0;
+}
+
+static int rzv2h_ivc_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ /*
+ * We have a single source pad, which has a single stream. V4L2 core has
+ * already validated those things. The actual power-on and programming
+ * of registers will be done through the video device's .vidioc_streamon
+ * so there's nothing to actually do here...
+ */
+
+ return 0;
+}
+
+static int rzv2h_ivc_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ return 0;
+}
+
+static const struct v4l2_subdev_pad_ops rzv2h_ivc_pad_ops = {
+ .enum_mbus_code = rzv2h_ivc_enum_mbus_code,
+ .enum_frame_size = rzv2h_ivc_enum_frame_size,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = rzv2h_ivc_set_fmt,
+ .enable_streams = rzv2h_ivc_enable_streams,
+ .disable_streams = rzv2h_ivc_disable_streams,
+};
+
+static const struct v4l2_subdev_core_ops rzv2h_ivc_core_ops = {
+ .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops rzv2h_ivc_subdev_ops = {
+ .core = &rzv2h_ivc_core_ops,
+ .pad = &rzv2h_ivc_pad_ops,
+};
+
+static int rzv2h_ivc_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state)
+{
+ struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
+
+ sink_fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SINK_PAD);
+ sink_fmt->width = RZV2H_IVC_DEFAULT_WIDTH;
+ sink_fmt->height = RZV2H_IVC_DEFAULT_HEIGHT;
+ sink_fmt->field = V4L2_FIELD_NONE;
+ sink_fmt->code = MEDIA_BUS_FMT_SRGGB16_1X16;
+ sink_fmt->colorspace = V4L2_COLORSPACE_RAW;
+ sink_fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
+ sink_fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
+ sink_fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(
+ true, sink_fmt->colorspace, sink_fmt->ycbcr_enc);
+
+ src_fmt = v4l2_subdev_state_get_format(state,
+ RZV2H_IVC_SUBDEV_SOURCE_PAD);
+
+ *src_fmt = *sink_fmt;
+ src_fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20;
+
+ return 0;
+}
+
+static int rzv2h_ivc_registered(struct v4l2_subdev *sd)
+{
+ struct rzv2h_ivc *ivc = container_of(sd, struct rzv2h_ivc, subdev.sd);
+
+ return rzv2h_ivc_init_vdev(ivc, sd->v4l2_dev);
+}
+
+static const struct v4l2_subdev_internal_ops rzv2h_ivc_subdev_internal_ops = {
+ .init_state = rzv2h_ivc_init_state,
+ .registered = rzv2h_ivc_registered,
+};
+
+static int rzv2h_ivc_link_validate(struct media_link *link)
+{
+ struct video_device *vdev =
+ media_entity_to_video_device(link->source->entity);
+ struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
+ struct v4l2_subdev *sd =
+ media_entity_to_v4l2_subdev(link->sink->entity);
+ const struct rzv2h_ivc_format *fmt;
+ const struct v4l2_pix_format_mplane *pix;
+ struct v4l2_subdev_state *state;
+ struct v4l2_mbus_framefmt *mf;
+ unsigned int i;
+ int ret = 0;
+
+ state = v4l2_subdev_lock_and_get_active_state(sd);
+ mf = v4l2_subdev_state_get_format(state, link->sink->index);
+
+ pix = &ivc->format.pix;
+ fmt = ivc->format.fmt;
+
+ if (mf->width != pix->width || mf->height != pix->height) {
+ dev_dbg(ivc->dev,
+ "link '%s':%u -> '%s':%u not valid: %ux%u != %ux%u\n",
+ link->source->entity->name, link->source->index,
+ link->sink->entity->name, link->sink->index,
+ mf->width, mf->height,
+ pix->width, pix->height);
+ ret = -EPIPE;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(fmt->mbus_codes); i++)
+ if (mf->code == fmt->mbus_codes[i])
+ break;
+
+ if (i == ARRAY_SIZE(fmt->mbus_codes)) {
+ dev_dbg(ivc->dev,
+ "link '%s':%u -> '%s':%u not valid: pixel format %p4cc cannot produce mbus_code 0x%04x\n",
+ link->source->entity->name, link->source->index,
+ link->sink->entity->name, link->sink->index,
+ &pix->pixelformat, mf->code);
+ ret = -EPIPE;
+ }
+
+ v4l2_subdev_unlock_state(state);
+
+ return ret;
+}
+
+static const struct media_entity_operations rzv2h_ivc_media_ops = {
+ .link_validate = rzv2h_ivc_link_validate,
+};
+
+int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc)
+{
+ struct v4l2_subdev *sd;
+ int ret;
+
+ /* Initialise subdevice */
+ sd = &ivc->subdev.sd;
+ sd->dev = ivc->dev;
+ v4l2_subdev_init(sd, &rzv2h_ivc_subdev_ops);
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+ sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+ sd->internal_ops = &rzv2h_ivc_subdev_internal_ops;
+ sd->entity.ops = &rzv2h_ivc_media_ops;
+
+ ivc->subdev.pads[RZV2H_IVC_SUBDEV_SINK_PAD].flags = MEDIA_PAD_FL_SINK;
+ ivc->subdev.pads[RZV2H_IVC_SUBDEV_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE;
+
+ snprintf(sd->name, sizeof(sd->name), "rzv2h ivc block");
+
+ ret = media_entity_pads_init(&sd->entity, RZV2H_IVC_NUM_SUBDEV_PADS,
+ ivc->subdev.pads);
+ if (ret) {
+ dev_err(ivc->dev, "failed to initialise media entity\n");
+ return ret;
+ }
+
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret) {
+ dev_err(ivc->dev, "failed to finalize subdev init\n");
+ goto err_cleanup_subdev_entity;
+ }
+
+ ret = v4l2_async_register_subdev(sd);
+ if (ret) {
+ dev_err(ivc->dev, "failed to register subdevice\n");
+ goto err_cleanup_subdev;
+ }
+
+ return 0;
+
+err_cleanup_subdev:
+ v4l2_subdev_cleanup(sd);
+err_cleanup_subdev_entity:
+ media_entity_cleanup(&sd->entity);
+
+ return ret;
+}
+
+void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc)
+{
+ struct v4l2_subdev *sd = &ivc->subdev.sd;
+
+ v4l2_subdev_cleanup(sd);
+ media_entity_remove_links(&sd->entity);
+ v4l2_async_unregister_subdev(sd);
+ media_entity_cleanup(&sd->entity);
+}
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
new file mode 100644
index 0000000000000000000000000000000000000000..63e20b83d596aff8a9534720b70fe75aa6c0c0ae
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
@@ -0,0 +1,568 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/V2H Input Video Control Block driver
+ *
+ * Copyright (C) 2025 Ideas on Board Oy
+ */
+
+#include "rzv2h-ivc.h"
+
+#include <linux/cleanup.h>
+#include <linux/iopoll.h>
+#include <linux/media-bus-format.h>
+#include <linux/minmax.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+
+#include <media/mipi-csi2.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-dma-contig.h>
+
+#define RZV2H_IVC_FIXED_HBLANK 0x20
+#define RZV2H_IVC_MIN_VBLANK(hts) max(0x1b, 15 + (120501 / (hts)))
+
+struct rzv2h_ivc_buf {
+ struct vb2_v4l2_buffer vb;
+ struct list_head queue;
+ dma_addr_t addr;
+};
+
+#define to_rzv2h_ivc_buf(vbuf) \
+ container_of(vbuf, struct rzv2h_ivc_buf, vb)
+
+static const struct rzv2h_ivc_format rzv2h_ivc_formats[] = {
+ {
+ .fourcc = V4L2_PIX_FMT_SBGGR8,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SBGGR8_1X8,
+ },
+ .dtype = MIPI_CSI2_DT_RAW8,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SGBRG8,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SGBRG8_1X8,
+ },
+ .dtype = MIPI_CSI2_DT_RAW8,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SGRBG8,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SGRBG8_1X8,
+ },
+ .dtype = MIPI_CSI2_DT_RAW8,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SRGGB8,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SRGGB8_1X8,
+ },
+ .dtype = MIPI_CSI2_DT_RAW8,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_RAW_CRU10,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SBGGR10_1X10,
+ MEDIA_BUS_FMT_SGBRG10_1X10,
+ MEDIA_BUS_FMT_SGRBG10_1X10,
+ MEDIA_BUS_FMT_SRGGB10_1X10
+ },
+ .dtype = MIPI_CSI2_DT_RAW10,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_RAW_CRU12,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SBGGR12_1X12,
+ MEDIA_BUS_FMT_SGBRG12_1X12,
+ MEDIA_BUS_FMT_SGRBG12_1X12,
+ MEDIA_BUS_FMT_SRGGB12_1X12
+ },
+ .dtype = MIPI_CSI2_DT_RAW12,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_RAW_CRU14,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SBGGR14_1X14,
+ MEDIA_BUS_FMT_SGBRG14_1X14,
+ MEDIA_BUS_FMT_SGRBG14_1X14,
+ MEDIA_BUS_FMT_SRGGB14_1X14
+ },
+ .dtype = MIPI_CSI2_DT_RAW14,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SBGGR16,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SBGGR16_1X16,
+ },
+ .dtype = MIPI_CSI2_DT_RAW16,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SGBRG16,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SGBRG16_1X16,
+ },
+ .dtype = MIPI_CSI2_DT_RAW16,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SGRBG16,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SGRBG16_1X16,
+ },
+ .dtype = MIPI_CSI2_DT_RAW16,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_SRGGB16,
+ .mbus_codes = {
+ MEDIA_BUS_FMT_SRGGB16_1X16,
+ },
+ .dtype = MIPI_CSI2_DT_RAW16,
+ },
+};
+
+static void rzv2h_ivc_transfer_buffer(struct work_struct *work)
+{
+ struct rzv2h_ivc *ivc = container_of(work, struct rzv2h_ivc,
+ buffers.work);
+ struct rzv2h_ivc_buf *buf;
+
+ scoped_guard(spinlock, &ivc->buffers.lock) {
+ if (ivc->buffers.curr) {
+ ivc->buffers.curr->vb.sequence = ivc->buffers.sequence++;
+ vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf,
+ VB2_BUF_STATE_DONE);
+ ivc->buffers.curr = NULL;
+ }
+
+ buf = list_first_entry_or_null(&ivc->buffers.queue,
+ struct rzv2h_ivc_buf, queue);
+ }
+
+ if (buf)
+ list_del(&buf->queue);
+ else
+ return;
+
+ ivc->buffers.curr = buf;
+ buf->addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_SADDL_P0, buf->addr);
+
+ scoped_guard(spinlock_irqsave, &ivc->spinlock) {
+ ivc->vvalid_ifp = 2;
+ }
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_FRCON, 0x1);
+}
+
+static int rzv2h_ivc_pipeline_started(struct media_entity *entity)
+{
+ struct video_device *vdev = media_entity_to_video_device(entity);
+ struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
+
+ guard(spinlock)(&ivc->buffers.lock);
+
+ if (list_empty(&ivc->buffers.queue)) {
+ /*
+ * The driver waits for interrupts to send a new frame and
+ * tracks their receipt in the vvalid_ifp variable. .buf_queue()
+ * will queue work if vvalid_ifp == 0 to trigger a new frame (an
+ * event that normally would only occur if no buffer was ready
+ * when the interrupt arrived). If there are no buffers in the
+ * queue yet, we set vvalid_ifp to zero so that the next queue
+ * will trigger the work.
+ */
+ scoped_guard(spinlock_irqsave, &ivc->spinlock) {
+ ivc->vvalid_ifp = 0;
+ }
+ } else {
+ queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
+ }
+
+ return 0;
+}
+
+static void rzv2h_ivc_pipeline_stopped(struct media_entity *entity)
+{
+ struct video_device *vdev = media_entity_to_video_device(entity);
+ struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
+ u32 val = 0;
+
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_STOP, 0x1);
+ readl_poll_timeout(ivc->base + RZV2H_IVC_REG_FM_STOP,
+ val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
+}
+
+static const struct media_entity_operations rzv2h_ivc_media_ops = {
+ .pipeline_started = rzv2h_ivc_pipeline_started,
+ .pipeline_stopped = rzv2h_ivc_pipeline_stopped,
+};
+
+static int rzv2h_ivc_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
+ unsigned int *num_planes, unsigned int sizes[],
+ struct device *alloc_devs[])
+{
+ struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
+
+ if (*num_planes && *num_planes > 1)
+ return -EINVAL;
+
+ if (sizes[0] && sizes[0] < ivc->format.pix.plane_fmt[0].sizeimage)
+ return -EINVAL;
+
+ *num_planes = 1;
+
+ if (!sizes[0])
+ sizes[0] = ivc->format.pix.plane_fmt[0].sizeimage;
+
+ return 0;
+}
+
+static void rzv2h_ivc_buf_queue(struct vb2_buffer *vb)
+{
+ struct rzv2h_ivc *ivc = vb2_get_drv_priv(vb->vb2_queue);
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct rzv2h_ivc_buf *buf = to_rzv2h_ivc_buf(vbuf);
+
+ scoped_guard(spinlock, &ivc->buffers.lock) {
+ list_add_tail(&buf->queue, &ivc->buffers.queue);
+ }
+
+ scoped_guard(spinlock_irqsave, &ivc->spinlock) {
+ if (vb2_is_streaming(vb->vb2_queue) && !ivc->vvalid_ifp)
+ queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
+ }
+}
+
+static void rzv2h_ivc_format_configure(struct rzv2h_ivc *ivc)
+{
+ const struct rzv2h_ivc_format *fmt = ivc->format.fmt;
+ struct v4l2_pix_format_mplane *pix = &ivc->format.pix;
+ unsigned int vblank;
+ unsigned int hts;
+
+ /* Currently only CRU packed pixel formats are supported */
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
+ RZV2H_IVC_INPUT_FMT_CRU_PACKED);
+
+ rzv2h_ivc_update_bits(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
+ RZV2H_IVC_PXFMT_DTYPE, fmt->dtype);
+
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_HSIZE, pix->width);
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_VSIZE, pix->height);
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_STRD,
+ pix->plane_fmt[0].bytesperline);
+
+ /*
+ * The ISP has minimum vertical blanking requirements that must be
+ * adhered to by the IVC. The minimum is a function of the Iridix blocks
+ * clocking requirements and the width of the image and horizontal
+ * blanking, but if we assume the worst case then it boils down to the
+ * below (plus one to the numerator to ensure the answer is rounded up)
+ */
+
+ hts = pix->width + RZV2H_IVC_FIXED_HBLANK;
+ vblank = RZV2H_IVC_MIN_VBLANK(hts);
+
+ rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_BLANK,
+ RZV2H_IVC_VBLANK(vblank));
+}
+
+static void rzv2h_ivc_return_buffers(struct rzv2h_ivc *ivc,
+ enum vb2_buffer_state state)
+{
+ struct rzv2h_ivc_buf *buf, *tmp;
+
+ guard(spinlock)(&ivc->buffers.lock);
+
+ if (ivc->buffers.curr) {
+ vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf, state);
+ ivc->buffers.curr = NULL;
+ }
+
+ list_for_each_entry_safe(buf, tmp, &ivc->buffers.queue, queue) {
+ list_del(&buf->queue);
+ vb2_buffer_done(&buf->vb.vb2_buf, state);
+ }
+}
+
+static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+ struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
+ int ret;
+
+ ivc->buffers.sequence = 0;
+ ivc->vvalid_ifp = 2;
+
+ ret = pm_runtime_resume_and_get(ivc->dev);
+ if (ret)
+ goto err_return_buffers;
+
+ ret = video_device_pipeline_alloc_start(&ivc->vdev.dev);
+ if (ret) {
+ dev_err(ivc->dev, "failed to start media pipeline\n");
+ goto err_pm_runtime_put;
+ }
+
+ rzv2h_ivc_format_configure(ivc);
+
+ ret = video_device_pipeline_started(&ivc->vdev.dev);
+ if (ret < 0)
+ goto err_stop_pipeline;
+
+ return 0;
+
+err_stop_pipeline:
+ video_device_pipeline_stop(&ivc->vdev.dev);
+err_pm_runtime_put:
+ pm_runtime_put(ivc->dev);
+err_return_buffers:
+ rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_QUEUED);
+
+ return ret;
+}
+
+static void rzv2h_ivc_stop_streaming(struct vb2_queue *q)
+{
+ struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
+
+ video_device_pipeline_stopped(&ivc->vdev.dev);
+ rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_ERROR);
+ video_device_pipeline_stop(&ivc->vdev.dev);
+ pm_runtime_mark_last_busy(ivc->dev);
+ pm_runtime_put_autosuspend(ivc->dev);
+}
+
+static const struct vb2_ops rzv2h_ivc_vb2_ops = {
+ .queue_setup = &rzv2h_ivc_queue_setup,
+ .buf_queue = &rzv2h_ivc_buf_queue,
+ .wait_prepare = vb2_ops_wait_prepare,
+ .wait_finish = vb2_ops_wait_finish,
+ .start_streaming = &rzv2h_ivc_start_streaming,
+ .stop_streaming = &rzv2h_ivc_stop_streaming,
+};
+
+static const struct rzv2h_ivc_format *
+rzv2h_ivc_format_from_pixelformat(u32 fourcc)
+{
+ for (unsigned int i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++)
+ if (fourcc == rzv2h_ivc_formats[i].fourcc)
+ return &rzv2h_ivc_formats[i];
+
+ return &rzv2h_ivc_formats[0];
+}
+
+static int rzv2h_ivc_enum_fmt_vid_out(struct file *file, void *fh,
+ struct v4l2_fmtdesc *f)
+{
+ if (f->index >= ARRAY_SIZE(rzv2h_ivc_formats))
+ return -EINVAL;
+
+ f->pixelformat = rzv2h_ivc_formats[f->index].fourcc;
+ return 0;
+}
+
+static int rzv2h_ivc_g_fmt_vid_out(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct rzv2h_ivc *ivc = video_drvdata(file);
+
+ f->fmt.pix_mp = ivc->format.pix;
+
+ return 0;
+}
+
+static void rzv2h_ivc_try_fmt(struct v4l2_pix_format_mplane *pix,
+ const struct rzv2h_ivc_format *fmt)
+{
+ pix->pixelformat = fmt->fourcc;
+
+ pix->width = clamp(pix->width, RZV2H_IVC_MIN_WIDTH,
+ RZV2H_IVC_MAX_WIDTH);
+ pix->height = clamp(pix->height, RZV2H_IVC_MIN_HEIGHT,
+ RZV2H_IVC_MAX_HEIGHT);
+
+ pix->field = V4L2_FIELD_NONE;
+ pix->colorspace = V4L2_COLORSPACE_RAW;
+ pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
+ pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+ pix->colorspace,
+ pix->ycbcr_enc);
+
+ v4l2_fill_pixfmt_mp(pix, pix->pixelformat, pix->width, pix->height);
+}
+
+static void rzv2h_ivc_set_format(struct rzv2h_ivc *ivc,
+ struct v4l2_pix_format_mplane *pix)
+{
+ const struct rzv2h_ivc_format *fmt;
+
+ fmt = rzv2h_ivc_format_from_pixelformat(pix->pixelformat);
+
+ rzv2h_ivc_try_fmt(pix, fmt);
+ ivc->format.pix = *pix;
+ ivc->format.fmt = fmt;
+}
+
+static int rzv2h_ivc_s_fmt_vid_out(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct rzv2h_ivc *ivc = video_drvdata(file);
+ struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
+
+ if (vb2_is_busy(&ivc->vdev.vb2q))
+ return -EBUSY;
+
+ rzv2h_ivc_set_format(ivc, pix);
+
+ return 0;
+}
+
+static int rzv2h_ivc_try_fmt_vid_out(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ const struct rzv2h_ivc_format *fmt;
+
+ fmt = rzv2h_ivc_format_from_pixelformat(f->fmt.pix.pixelformat);
+ rzv2h_ivc_try_fmt(&f->fmt.pix_mp, fmt);
+
+ return 0;
+}
+
+static int rzv2h_ivc_querycap(struct file *file, void *fh,
+ struct v4l2_capability *cap)
+{
+ strscpy(cap->driver, "rzv2h-ivc", sizeof(cap->driver));
+ strscpy(cap->card, "Renesas Input Video Control", sizeof(cap->card));
+
+ return 0;
+}
+
+static const struct v4l2_ioctl_ops rzv2h_ivc_v4l2_ioctl_ops = {
+ .vidioc_reqbufs = vb2_ioctl_reqbufs,
+ .vidioc_querybuf = vb2_ioctl_querybuf,
+ .vidioc_create_bufs = vb2_ioctl_create_bufs,
+ .vidioc_qbuf = vb2_ioctl_qbuf,
+ .vidioc_expbuf = vb2_ioctl_expbuf,
+ .vidioc_dqbuf = vb2_ioctl_dqbuf,
+ .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
+ .vidioc_enum_fmt_vid_out = rzv2h_ivc_enum_fmt_vid_out,
+ .vidioc_g_fmt_vid_out_mplane = rzv2h_ivc_g_fmt_vid_out,
+ .vidioc_s_fmt_vid_out_mplane = rzv2h_ivc_s_fmt_vid_out,
+ .vidioc_try_fmt_vid_out_mplane = rzv2h_ivc_try_fmt_vid_out,
+ .vidioc_querycap = rzv2h_ivc_querycap,
+ .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
+ .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
+};
+
+static const struct v4l2_file_operations rzv2h_ivc_v4l2_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = video_ioctl2,
+ .open = v4l2_fh_open,
+ .release = vb2_fop_release,
+ .poll = vb2_fop_poll,
+ .mmap = vb2_fop_mmap,
+};
+
+int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev)
+{
+ struct v4l2_pix_format_mplane pix = { };
+ struct video_device *vdev;
+ struct vb2_queue *vb2q;
+ int ret;
+
+ spin_lock_init(&ivc->buffers.lock);
+ INIT_LIST_HEAD(&ivc->buffers.queue);
+ INIT_WORK(&ivc->buffers.work, rzv2h_ivc_transfer_buffer);
+
+ ivc->buffers.async_wq = alloc_workqueue("rzv2h-ivc", 0, 0);
+ if (!ivc->buffers.async_wq)
+ return -EINVAL;
+
+ /* Initialise vb2 queue */
+ vb2q = &ivc->vdev.vb2q;
+ vb2q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+ vb2q->io_modes = VB2_MMAP | VB2_DMABUF;
+ vb2q->drv_priv = ivc;
+ vb2q->mem_ops = &vb2_dma_contig_memops;
+ vb2q->ops = &rzv2h_ivc_vb2_ops;
+ vb2q->buf_struct_size = sizeof(struct rzv2h_ivc_buf);
+ vb2q->min_queued_buffers = 0;
+ vb2q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+ vb2q->lock = &ivc->lock;
+ vb2q->dev = ivc->dev;
+
+ ret = vb2_queue_init(vb2q);
+ if (ret) {
+ dev_err(ivc->dev, "vb2 queue init failed\n");
+ goto err_destroy_workqueue;
+ }
+
+ /* Initialise Video Device */
+ vdev = &ivc->vdev.dev;
+ strscpy(vdev->name, "rzv2h-ivc", sizeof(vdev->name));
+ vdev->release = video_device_release_empty;
+ vdev->fops = &rzv2h_ivc_v4l2_fops;
+ vdev->ioctl_ops = &rzv2h_ivc_v4l2_ioctl_ops;
+ vdev->lock = &ivc->lock;
+ vdev->v4l2_dev = v4l2_dev;
+ vdev->queue = vb2q;
+ vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_STREAMING;
+ vdev->vfl_dir = VFL_DIR_TX;
+ video_set_drvdata(vdev, ivc);
+
+ pix.pixelformat = V4L2_PIX_FMT_SRGGB16;
+ pix.width = RZV2H_IVC_DEFAULT_WIDTH;
+ pix.height = RZV2H_IVC_DEFAULT_HEIGHT;
+ rzv2h_ivc_set_format(ivc, &pix);
+
+ ivc->vdev.pad.flags = MEDIA_PAD_FL_SOURCE;
+ ivc->vdev.dev.entity.ops = &rzv2h_ivc_media_ops;
+ ret = media_entity_pads_init(&ivc->vdev.dev.entity, 1, &ivc->vdev.pad);
+ if (ret)
+ goto err_release_vb2q;
+
+ ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
+ if (ret) {
+ dev_err(ivc->dev, "failed to register IVC video device\n");
+ goto err_cleanup_vdev_entity;
+ }
+
+ ret = media_create_pad_link(&vdev->entity, 0, &ivc->subdev.sd.entity,
+ RZV2H_IVC_SUBDEV_SINK_PAD,
+ MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+ if (ret) {
+ dev_err(ivc->dev, "failed to create media link\n");
+ goto err_unregister_vdev;
+ }
+
+ return 0;
+
+err_unregister_vdev:
+ video_unregister_device(vdev);
+err_cleanup_vdev_entity:
+ media_entity_cleanup(&vdev->entity);
+err_release_vb2q:
+ vb2_queue_release(vb2q);
+err_destroy_workqueue:
+ destroy_workqueue(ivc->buffers.async_wq);
+
+ return ret;
+}
+
+void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc)
+{
+ struct video_device *vdev = &ivc->vdev.dev;
+ struct vb2_queue *vb2q = &ivc->vdev.vb2q;
+
+ if (!ivc->sched)
+ return;
+
+ vb2_video_unregister_device(vdev);
+ media_entity_cleanup(&vdev->entity);
+ vb2_queue_release(vb2q);
+}
diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
new file mode 100644
index 0000000000000000000000000000000000000000..709c6a9398fe2484c2acb03d443d58ea4e153a66
--- /dev/null
+++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
@@ -0,0 +1,131 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Renesas RZ/V2H Input Video Control Block driver
+ *
+ * Copyright (C) 2025 Ideas on Board Oy
+ */
+
+#include <linux/clk.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/videodev2.h>
+#include <linux/workqueue.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-core.h>
+#include <media/videobuf2-v4l2.h>
+
+#define RZV2H_IVC_REG_AXIRX_PLNUM 0x0000
+#define RZV2H_IVC_ONE_EXPOSURE 0x00
+#define RZV2H_IVC_TWO_EXPOSURE 0x01
+#define RZV2H_IVC_REG_AXIRX_PXFMT 0x0004
+#define RZV2H_IVC_INPUT_FMT_MIPI (0 << 16)
+#define RZV2H_IVC_INPUT_FMT_CRU_PACKED (1 << 16)
+#define RZV2H_IVC_PXFMT_DTYPE GENMASK(7, 0)
+#define RZV2H_IVC_REG_AXIRX_SADDL_P0 0x0010
+#define RZV2H_IVC_REG_AXIRX_SADDH_P0 0x0014
+#define RZV2H_IVC_REG_AXIRX_SADDL_P1 0x0018
+#define RZV2H_IVC_REG_AXIRX_SADDH_P1 0x001c
+#define RZV2H_IVC_REG_AXIRX_HSIZE 0x0020
+#define RZV2H_IVC_REG_AXIRX_VSIZE 0x0024
+#define RZV2H_IVC_REG_AXIRX_BLANK 0x0028
+#define RZV2H_IVC_VBLANK(x) ((x) << 16)
+#define RZV2H_IVC_REG_AXIRX_STRD 0x0030
+#define RZV2H_IVC_REG_AXIRX_ISSU 0x0040
+#define RZV2H_IVC_REG_AXIRX_ERACT 0x0048
+#define RZV2H_IVC_REG_FM_CONTEXT 0x0100
+#define RZV2H_IVC_SOFTWARE_CFG 0x00
+#define RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG BIT(0)
+#define RZV2H_IVC_MULTI_CONTEXT_SW_HW_CFG BIT(1)
+#define RZV2H_IVC_REG_FM_MCON 0x0104
+#define RZV2H_IVC_REG_FM_FRCON 0x0108
+#define RZV2H_IVC_REG_FM_STOP 0x010c
+#define RZV2H_IVC_REG_FM_INT_EN 0x0120
+#define RZV2H_IVC_VVAL_IFPE BIT(0)
+#define RZV2H_IVC_REG_FM_INT_STA 0x0124
+#define RZV2H_IVC_REG_AXIRX_FIFOCAP0 0x0208
+#define RZV2H_IVC_REG_CORE_CAPCON 0x020c
+#define RZV2H_IVC_REG_CORE_FIFOCAP0 0x0228
+#define RZV2H_IVC_REG_CORE_FIFOCAP1 0x022c
+
+#define RZV2H_IVC_MIN_WIDTH 640
+#define RZV2H_IVC_MAX_WIDTH 4096
+#define RZV2H_IVC_MIN_HEIGHT 480
+#define RZV2H_IVC_MAX_HEIGHT 4096
+#define RZV2H_IVC_DEFAULT_WIDTH 1920
+#define RZV2H_IVC_DEFAULT_HEIGHT 1080
+
+#define RZV2H_IVC_NUM_HW_RESOURCES 3
+
+struct device;
+
+enum rzv2h_ivc_subdev_pads {
+ RZV2H_IVC_SUBDEV_SINK_PAD,
+ RZV2H_IVC_SUBDEV_SOURCE_PAD,
+ RZV2H_IVC_NUM_SUBDEV_PADS
+};
+
+struct rzv2h_ivc_format {
+ u32 fourcc;
+ /*
+ * The CRU packed pixel formats are bayer-order agnostic, so each could
+ * support any one of the 4 possible media bus formats.
+ */
+ u32 mbus_codes[4];
+ u8 dtype;
+};
+
+struct rzv2h_ivc {
+ struct device *dev;
+ void __iomem *base;
+ struct clk_bulk_data clks[RZV2H_IVC_NUM_HW_RESOURCES];
+ struct reset_control_bulk_data resets[RZV2H_IVC_NUM_HW_RESOURCES];
+ int irqnum;
+ u8 vvalid_ifp;
+
+ struct {
+ struct video_device dev;
+ struct vb2_queue vb2q;
+ struct media_pad pad;
+ } vdev;
+
+ struct {
+ struct v4l2_subdev sd;
+ struct media_pad pads[RZV2H_IVC_NUM_SUBDEV_PADS];
+ } subdev;
+
+ struct {
+ /* Spinlock to guard buffer queue */
+ spinlock_t lock;
+ struct workqueue_struct *async_wq;
+ struct work_struct work;
+ struct list_head queue;
+ struct rzv2h_ivc_buf *curr;
+ unsigned int sequence;
+ } buffers;
+
+ struct media_job_scheduler *sched;
+
+ struct {
+ struct v4l2_pix_format_mplane pix;
+ const struct rzv2h_ivc_format *fmt;
+ } format;
+
+ /* Mutex to provide to vb2 */
+ struct mutex lock;
+ /* Lock to protect the interrupt counter */
+ spinlock_t spinlock;
+};
+
+int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev);
+void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc);
+int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc);
+void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc);
+void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val);
+void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
+ u32 mask, u32 val);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] MAINTAINERS: Add entry for rzv2h-ivc driver
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
` (3 preceding siblings ...)
2025-07-04 11:20 ` [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver Daniel Scally
@ 2025-07-04 11:20 ` Daniel Scally
4 siblings, 0 replies; 21+ messages in thread
From: Daniel Scally @ 2025-07-04 11:20 UTC (permalink / raw)
To: linux-media, devicetree, linux-renesas-soc
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, jacopo.mondi,
biju.das.jz, Daniel Scally
Add an entry to the MAINTAINERS file for the rzv2h-ivc driver
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:
- None
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 276c5a0b2dc58589b6bc5cc9ea549156ad088bdf..7dbfc61a4b18d478c67a4d939ddd0ec7e08b96ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21166,6 +21166,13 @@ S: Maintained
F: Documentation/devicetree/bindings/net/renesas,r9a09g057-gbeth.yaml
F: drivers/net/ethernet/stmicro/stmmac/dwmac-renesas-gbeth.c
+RENESAS RZ/V2H(P) INPUT VIDEO CONTROL BLOCK DRIVER
+M: Daniel Scally <dan.scally@ideasonboard.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/renesas,r9a09g057-ivc.yaml
+F: drivers/media/platform/renesas/rzv2h-ivc/
+
RENESAS RZ/V2H(P) USB2PHY PORT RESET DRIVER
M: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
M: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block
2025-07-04 11:20 ` [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block Daniel Scally
@ 2025-07-07 6:39 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-07 6:39 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, jacopo.mondi, biju.das.jz
On Fri, Jul 04, 2025 at 12:20:20PM +0100, Daniel Scally wrote:
> + resets:
> + items:
> + - description: Input Video Control block register access reset
> + - description: Video input data AXI bus reset
> + - description: ISP core reset
> +
> + reset-names:
> + items:
> + - const: reg
> + - const: axi
> + - const: isp
> +
> + port:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: Output parallel video bus
> +
> + properties:
> + endpoint:
> + $ref: /schemas/graph.yaml#/properties/endpoint
This endpoint here should not be needed, because it is already in the
port schema and nothing more can be added to the 'port'.
Anyway,
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-04 11:20 ` [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]() Daniel Scally
@ 2025-07-08 13:10 ` Jacopo Mondi
2025-07-11 11:51 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-08 13:10 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, jacopo.mondi, biju.das.jz
Hi Dan
On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
> Add helpers to run the new media_pipeline_started() and
> media_pipeline_stopped() functions. The helpers iterate over the
> entities in the pipeline and count the number of video devices and
> compare that to the pipeline's start_count() before acting. This
> allows us to only run the media pipeline callbacks in the event that
> the pipeline has had video_pipeline_start() called for each video
> device.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>
> ---
>
> We could take this further perhaps and include the equivalent routine
> in video_device_pipeline[_alloc]_start()...if none of the entities
> involved have .pipeline_started() or .pipeline_stopped() operations it
> should be harmless, but I'm a bit reluctant to force the choice to run
> those operations on users.
I know I've kind of suggested that, but after all I don't think it's a
very good idea, having this in two steps is probably better. And I
like the fact the v4l2-dev layer operates on the video device counting
and only relies on the mc layer for the callbacks notification.
>
> Changes in v2:
>
> - Adapted now media_pipeline_for_each_entity() takes an iter
> variable
> - Fixed the Return: section of the kerneldoc comments
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
> include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
> }
> EXPORT_SYMBOL_GPL(video_device_pipeline);
>
> +static int __video_device_pipeline_started(struct media_pipeline *pipe)
__function_name() is usually reserved for the non-locking version of
function_name().
This seems to be an helper only used internally by
video_device_pipeline_started() so I would use a different name
something like video_device_has_pipeline_started() ?
> +{
> + struct media_pipeline_entity_iter iter;
> + unsigned int n_video_devices = 0;
> + struct media_entity *entity;
> + int ret;
> +
> + ret = media_pipeline_entity_iter_init(pipe, &iter);
> + if (ret)
> + return ret;
> +
> + media_pipeline_for_each_entity(pipe, &iter, entity) {
> + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
> + n_video_devices++;
> + }
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + return n_video_devices - pipe->start_count;
> +}
> +
> +int video_device_pipeline_started(struct video_device *vdev)
> +{
> + struct media_pipeline *pipe;
> + int ret;
> +
> + pipe = video_device_pipeline(vdev);
> + if (!pipe)
> + return -ENODEV;
> +
> + ret = __video_device_pipeline_started(pipe);
> + if (ret)
> + return ret;
I would not return ret, as it might take random values betwen
n_video_devices and 1. See below on the return value documentation
> +
> + return media_pipeline_started(pipe);
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
> +
> +int video_device_pipeline_stopped(struct video_device *vdev)
> +{
> + struct media_pipeline *pipe;
> + int ret;
> +
> + pipe = video_device_pipeline(vdev);
> + if (!pipe)
> + return -ENODEV;
> +
> + ret = __video_device_pipeline_started(pipe);
> + if (ret)
> + return ret;
ditto
> +
> + media_pipeline_stopped(pipe);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> /*
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
> */
> struct media_pipeline *video_device_pipeline(struct video_device *vdev);
>
> +/**
> + * video_device_pipeline_started - Run the pipeline_started() entity operation
> + * for a fully-started media pipeline
> + * @vdev: A video device that's part of the pipeline
> + *
> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> + * connected to a given video device through enabled links have been marked as
I would use the same text as the one from video_device_pipeline_start()
" connected to a given video device through enabled links, either
directly or indirectly,"
> + * streaming through the use of video_device_pipeline_start() or one of its
> + * equivalent functions. If so, media_pipeline_started() is called to inform
> + * entities in the pipeline of that fact. The intention is to provide drivers
> + * with a shortcut for checking whether their pipeline is fully ready to start
> + * processing data.
Not really a shortcut, I would use "mechanism" instead.
I would also specify that:
* entities in the pipeline of that fact. The intention is to provide drivers
* with a mechanism for checking whether their pipeline is fully ready to start
* processing data and call the .pipeline_started() media entity operation
* on all the entities in the pipeline.
> + *
> + * Return: The number of video devices in the pipeline remaining to be started,
> + * or a negative error number on failure.
0 for success as well
I would anyway return 0 for success and a specific error code for the
three failure cases:
-ENOMEM if allocating the iterator fails
-ENODEV if not all video devices have started
-EINVAL if media_pipeline_started() fails
You can document them as (copying from iommu.h)
* Return:
* * 0 - success
* * EINVAL - call to pipeline_started() failed
* * ENOMEM - failed to allocate pipe iterator
* * ENODEV - pipeline not yet fully started
> + */
> +int video_device_pipeline_started(struct video_device *vdev);
> +
> +/**
> + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
> + * for a fully-started media pipeline
> + * @vdev: A video device that's part of the pipeline
> + *
> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> + * connected to a given video device through enabled links have been marked as
> + * streaming through the use of video_device_pipeline_start() or one of its
What is the intended semantic here ? The first video device to receive
a streamoff() will trigger media_pipeline_stopped() or should the last
one do that ?
> + * equivalent functions. If so, media_pipeline_stopped() is called for each
> + * entity in the pipeline. The intention is to provide drivers with a shortcut
> + * for checking whether this video device is the first device in the pipeline
> + * to be stopped.
> + *
> + * Return: The number of video devices in the pipeline remaining to be started, or a
> + * negative error number on failure.
> + */
> +int video_device_pipeline_stopped(struct video_device *vdev);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> #endif /* _V4L2_DEV_H */
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops
2025-07-04 11:20 ` [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops Daniel Scally
@ 2025-07-08 13:29 ` Jacopo Mondi
2025-07-11 13:24 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-08 13:29 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, jacopo.mondi, biju.das.jz
Hi Dan
On Fri, Jul 04, 2025 at 12:20:18PM +0100, Daniel Scally wrote:
> Add two new members to struct media_entity_operations, along with new
> functions in media-entity.c to traverse a media pipeline and call the
> new operations. The new functions are intended to be used to signal
> to a media pipeline that it has fully started, with the entity ops
> allowing drivers to define some action to be taken when those
> conditions are met.
>
> The combination of the new functions and operations allows drivers
> which are part of a multi-driver pipeline to delay actually starting
> streaming until all of the conditions for streaming succcessfully are
> met across all drivers.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v4:
>
> - Reverted to having the iter variable
>
> Changes in v3:
>
> - Dropped the iter variable now that the pipeline entity
> iterator functions don't need it.
> - Updated documentation to specify Optional and return
> values
>
> Changes in v2:
>
> - Refactored media_pipeline_started() such that the cleanup
> function for media_pipeline_entity_iter is unconditionally
> called
> - Avoided using media_entity_call() helper for operation that
> has return type void to avoid compiler warnings
> ---
> drivers/media/mc/mc-entity.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> include/media/media-entity.h | 29 ++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 045590905582054c46656e20463271b1f93fa6b4..d3443537d4304e12cb015630101efba22375c011 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1053,6 +1053,52 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> }
> EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
>
> +int media_pipeline_started(struct media_pipeline *pipe)
> +{
> + struct media_pipeline_entity_iter iter;
> + struct media_entity *entity;
> + int ret;
> +
> + ret = media_pipeline_entity_iter_init(pipe, &iter);
> + if (ret)
> + return ret;
> +
> + media_pipeline_for_each_entity(pipe, &iter, entity) {
> + ret = media_entity_call(entity, pipeline_started);
> + if (ret && ret != -ENOIOCTLCMD)
> + break;
> + }
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + ret = ret == -ENOIOCTLCMD ? 0 : ret;
> + if (ret)
> + media_pipeline_stopped(pipe);
If you take my suggestion to limit the return value of
video_device_pipeline_started() to three possible error codes, you
could return -EINVAL here
> +
> + return ret;
and 0 here
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_started);
> +
> +int media_pipeline_stopped(struct media_pipeline *pipe)
> +{
> + struct media_pipeline_entity_iter iter;
> + struct media_entity *entity;
> + int ret;
> +
> + ret = media_pipeline_entity_iter_init(pipe, &iter);
> + if (ret)
> + return ret;
> +
> + media_pipeline_for_each_entity(pipe, &iter, entity)
> + if (entity->ops && entity->ops->pipeline_stopped)
> + entity->ops->pipeline_stopped(entity);
> +
> + media_pipeline_entity_iter_cleanup(&iter);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_pipeline_stopped);
> +
> /* -----------------------------------------------------------------------------
> * Links management
> */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 64cf590b11343f68a456c5870ca2f32917c122f9..ad658f42357ec505c84d9479bbbf18494da7f939 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -269,6 +269,10 @@ struct media_pad {
> * media_entity_has_pad_interdep().
> * Optional: If the operation isn't implemented all pads
> * will be considered as interdependent.
> + * @pipeline_started: Notify this entity that the pipeline it is a part of has
> + * been started
> + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has
> + * been stopped
The documentation of the other functions end with a full stop.
If the operation is optional, I would specify it here like it's done
for other operations
> *
> * .. note::
> *
> @@ -284,6 +288,8 @@ struct media_entity_operations {
> int (*link_validate)(struct media_link *link);
> bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
> unsigned int pad1);
> + int (*pipeline_started)(struct media_entity *entity);
> + void (*pipeline_stopped)(struct media_entity *entity);
> };
>
> /**
> @@ -1261,6 +1267,29 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
> entity != NULL; \
> entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
>
> +/**
> + * media_pipeline_started - Inform entities in a pipeline that it has started
> + * @pipe: The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_started
> + * member of media_entity_operations. Optional.
I would move "Optional" to the documentation of the media entity
> + *
> + * Return: zero on success, or a negative error code passed through from an
> + * entity's .pipeline_started() operation.
> + */
> +int media_pipeline_started(struct media_pipeline *pipe);
> +
> +/**
> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
> + * @pipe: The pipeline
> + *
> + * Iterate on all entities in a media pipeline and call their pipeline_stopped
> + * member of media_entity_operations. Optional.
> + *
> + * Return: zero on success, or -ENOMEM if the iterator initialisation failed.
> + */
> +int media_pipeline_stopped(struct media_pipeline *pipe);
> +
> /**
> * media_pipeline_alloc_start - Mark a pipeline as streaming
> * @pad: Starting pad
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-04 11:20 ` [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver Daniel Scally
@ 2025-07-08 14:49 ` Jacopo Mondi
2025-07-08 14:57 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-08 14:49 UTC (permalink / raw)
To: Daniel Scally
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, jacopo.mondi, biju.das.jz
On Fri, Jul 04, 2025 at 12:20:21PM +0100, Daniel Scally wrote:
> Add a driver for the Input Video Control block in an RZ/V2H SoC which
> feeds data into the Arm Mali-C55 ISP.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v4:
>
> - Update the compatible to renesas,r9a09g057-ivc
> - Dropped the media jobs / scheduler functionality, and re
> worked the driver to have its own workqueue pushing frames
> - Fix .enum_mbus_code() to return 20-bit output for source
> pad.
> - Fix some alignment issues
> - Make the forwarding of sink to source pad format a more
> explicit operation.
> - Rename rzv2h_initialise_video_device_and_queue()
> - Reversed order of v4l2_subdev_init_finalize() and
> v4l2_async_register_subdev() to make sure everything is
> finished initialising before registering the subdev.
> - Change function to MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
> - Use a parametised macro for min vblank
> - Minor formatting
> - Use the DEFAULT macros for quantization / ycbcr_enc values
> - Switch to using the mplane API
> - Dropped select RESET_CONTROLLER
> - Used the new helpers for starting a media pipeline
> - Switch from threaded irq to normal with driver workqueue
> and revised startup routine
>
> Changes in v3:
>
> - Account for the renamed CRU pixel formats
>
> Changes in v2:
>
> - Added selects and depends statements to Kconfig entry
> - Fixed copyright year
> - Stopped including in .c files headers already included in .h
> - Fixed uninitialized variable in iterator
> - Only check vvalid member in interrupt function and wait
> unconditionally elsewhere
> - __maybe_unused for the PM ops
> - Initialise the subdevice after setting up PM
> - Fixed the remove function for the driver to actually do
> something.
> - Some minor formatting changes
> - Fixed the quantization member for the format
> - Changes accounting for the v2 of the media jobs framework
> - Change min_queued_buffers to 0
> ---
> drivers/media/platform/renesas/Kconfig | 2 +
> drivers/media/platform/renesas/Makefile | 1 +
> drivers/media/platform/renesas/rzv2h-ivc/Kconfig | 16 +
> drivers/media/platform/renesas/rzv2h-ivc/Makefile | 5 +
> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c | 228 +++++++++
> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c | 376 ++++++++++++++
> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c | 568 +++++++++++++++++++++
> .../media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h | 131 +++++
> 8 files changed, 1327 insertions(+)
>
> diff --git a/drivers/media/platform/renesas/Kconfig b/drivers/media/platform/renesas/Kconfig
> index 27a54fa7908384f2e8200f0f7283a82b0ae8435c..5462e524c3708be87a50dd80d4b4017a2466aa99 100644
> --- a/drivers/media/platform/renesas/Kconfig
> +++ b/drivers/media/platform/renesas/Kconfig
> @@ -42,6 +42,8 @@ config VIDEO_SH_VOU
> source "drivers/media/platform/renesas/rcar-isp/Kconfig"
> source "drivers/media/platform/renesas/rcar-vin/Kconfig"
> source "drivers/media/platform/renesas/rzg2l-cru/Kconfig"
> +source "drivers/media/platform/renesas/rzv2h-ivc/Kconfig"
> +
Additional empty line
>
> # Mem2mem drivers
>
> diff --git a/drivers/media/platform/renesas/Makefile b/drivers/media/platform/renesas/Makefile
> index 1127259c09d6a51b70803e76c495918e06777f67..b6b4abf01db246aaf8269b8027efee9b0b32083a 100644
> --- a/drivers/media/platform/renesas/Makefile
> +++ b/drivers/media/platform/renesas/Makefile
> @@ -6,6 +6,7 @@
> obj-y += rcar-isp/
> obj-y += rcar-vin/
> obj-y += rzg2l-cru/
> +obj-y += rzv2h-ivc/
> obj-y += vsp1/
>
> obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Kconfig b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..bde2ba52329b522cccd63667f376edb2f4a0608f
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config VIDEO_RZV2H_IVC
> + tristate "Renesas RZ/V2H Input Video Control block driver"
> + depends on V4L_PLATFORM_DRIVERS
> + depends on VIDEO_DEV
> + depends on ARCH_RENESAS || COMPILE_TEST
> + depends on OF
> + select VIDEOBUF2_DMA_CONTIG
> + select MEDIA_CONTROLLER
> + select VIDEO_V4L2_SUBDEV_API
> + help
> + Support for the Video Input Block found in the RZ/V2H SoC. The IVC
I would stick to a more canonical description
Support for the Renesas RZ/V2H Input Video Control Block
(IVC).
To compile this driver as a module, choose M here: the
module will be called rzv2h-ivc.
> + block is used to read data from memory and forward it to the ISP that
> + is integrated to the SoC. Enable this to support the block, and by
> + extension the ISP to which it feeds data.
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Makefile b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..080ee3570f09c236d87abeaea5d8dd578fefb6d3
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +rzv2h-ivc-y := rzv2h-ivc-dev.o rzv2h-ivc-subdev.o rzv2h-ivc-video.o
> +
> +obj-$(CONFIG_VIDEO_RZV2H_IVC) += rzv2h-ivc.o
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..841fa2d17df24ffe982a1d3237ca7c0491c290c4
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2H Input Video Control Block driver
> + *
> + * Copyright (C) 2025 Ideas on Board Oy
> + */
> +
> +#include "rzv2h-ivc.h"
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +inline void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val)
I would not try to outsmart the compiler and let it decide what ot
inline or not
> +{
> + writel(val, ivc->base + addr);
> +}
> +
> +void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
> + u32 mask, u32 val)
> +{
> + u32 orig, new;
> +
> + orig = readl(ivc->base + addr);
> +
> + new = orig & ~mask;
> + new |= val & mask;
> +
> + if (new != orig)
> + writel(new, ivc->base + addr);
> +}
> +
> +static int rzv2h_ivc_get_hardware_resources(struct rzv2h_ivc *ivc,
> + struct platform_device *pdev)
> +{
> + const char * const resource_names[RZV2H_IVC_NUM_HW_RESOURCES] = {
> + "reg",
> + "axi",
> + "isp",
> + };
> + struct resource *res;
> + int ret;
> +
> + ivc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(ivc->base))
> + return dev_err_probe(ivc->dev, PTR_ERR(ivc->base),
> + "failed to map IO memory\n");
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
> + ivc->clks[i].id = resource_names[i];
> +
> + ret = devm_clk_bulk_get(ivc->dev, ARRAY_SIZE(resource_names), ivc->clks);
> + if (ret)
> + return dev_err_probe(ivc->dev, ret, "failed to acquire clks\n");
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
> + ivc->resets[i].id = resource_names[i];
> +
> + ret = devm_reset_control_bulk_get_optional_shared(
> + ivc->dev, ARRAY_SIZE(resource_names), ivc->resets);
> + if (ret)
> + return dev_err_probe(ivc->dev, ret, "failed to acquire resets\n");
> +
> + return 0;
> +}
> +
> +static void rzv2h_ivc_global_config(struct rzv2h_ivc *ivc)
> +{
> + /* Currently we only support single-exposure input */
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PLNUM, RZV2H_IVC_ONE_EXPOSURE);
> +
> + /*
> + * Datasheet says we should disable the interrupts before changing mode
> + * to avoid spurious IFP interrupt.
> + */
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, 0x0);
> +
> + /*
> + * RZ/V2H documentation says software controlled configuration is not
> + * supported, and currently neither is multi-context mode. That being so
> + * we just set single context sw-hw mode.
nit: the register documentation doesn't present multi-context SW-HW
mode as prohibited
> + */
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_CONTEXT,
> + RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG);
> +
> + /*
> + * We enable the frame end interrupt so that we know when we should send
> + * follow-up frames.
> + */
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, RZV2H_IVC_VVAL_IFPE);
> +}
> +
> +static irqreturn_t rzv2h_ivc_isr(int irq, void *context)
> +{
> + struct device *dev = context;
> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
> +
> + guard(spinlock)(&ivc->spinlock);
> +
> + if (!--ivc->vvalid_ifp)
> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
The driver doesn't depend or select CONFIG_PM, so this is rightfully
marked as __maybe_unused.
However, it doesn't seem to me that the probe() routine manually
enable the peripheral, so in case of !CONFIG_PM am I wrong or the
device won't operate at all ?
I would select CONFIG_PM, or otherwise call this function from the probe()
routine and then call pm_runtime_set_active() to inform runtime_pm
that the peripheral is active, and at the end of the probe routine
call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
reduces to a nop leaving the peripheral enabled.
I would just select CONFIG_PM tbh
> +{
> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = request_irq(ivc->irqnum, rzv2h_ivc_isr, 0, dev_driver_string(dev),
> + dev);
> + if (ret) {
> + dev_err(dev, "failed to request irq\n");
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(ivc->clks), ivc->clks);
> + if (ret) {
> + dev_err(ivc->dev, "failed to enable clocks\n");
> + goto err_free_irqnum;
> + }
> +
> + ret = reset_control_bulk_deassert(ARRAY_SIZE(ivc->resets), ivc->resets);
> + if (ret) {
> + dev_err(ivc->dev, "failed to deassert resets\n");
> + goto err_disable_clks;
> + }
> +
> + rzv2h_ivc_global_config(ivc);
> +
> + return 0;
> +
> +err_disable_clks:
> + clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
> +err_free_irqnum:
> + free_irq(ivc->irqnum, dev);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused rzv2h_ivc_runtime_suspend(struct device *dev)
> +{
> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
> +
> + reset_control_bulk_assert(ARRAY_SIZE(ivc->resets), ivc->resets);
> + clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
> + free_irq(ivc->irqnum, dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rzv2h_ivc_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(rzv2h_ivc_runtime_suspend, rzv2h_ivc_runtime_resume,
> + NULL)
> +};
> +
> +static int rzv2h_ivc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rzv2h_ivc *ivc;
> + int ret;
> +
> + ivc = devm_kzalloc(dev, sizeof(*ivc), GFP_KERNEL);
> + if (!ivc)
> + return -ENOMEM;
> +
> + ivc->dev = dev;
> + platform_set_drvdata(pdev, ivc);
> + mutex_init(&ivc->lock);
> + spin_lock_init(&ivc->spinlock);
> +
> + ret = rzv2h_ivc_get_hardware_resources(ivc, pdev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(dev, 2000);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_enable(dev);
> +
> + ivc->irqnum = platform_get_irq(pdev, 0);
> + if (ivc->irqnum < 0) {
> + dev_err(dev, "failed to get interrupt\n");
> + return ret;
> + }
> +
> + ret = rzv2h_ivc_initialise_subdevice(ivc);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void rzv2h_ivc_remove(struct platform_device *pdev)
> +{
> + struct rzv2h_ivc *ivc = platform_get_drvdata(pdev);
> +
> + rzv2h_deinit_video_dev_and_queue(ivc);
> + rzv2h_ivc_deinit_subdevice(ivc);
> + mutex_destroy(&ivc->lock);
> +}
> +
> +static const struct of_device_id rzv2h_ivc_of_match[] = {
> + { .compatible = "renesas,r9a09g057-ivc", },
> + { /* Sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rzv2h_ivc_of_match);
> +
> +static struct platform_driver rzv2h_ivc_driver = {
> + .driver = {
> + .name = "rzv2h-ivc",
> + .of_match_table = rzv2h_ivc_of_match,
> + .pm = &rzv2h_ivc_pm_ops,
> + },
> + .probe = rzv2h_ivc_probe,
> + .remove = rzv2h_ivc_remove,
> +};
> +
> +module_platform_driver(rzv2h_ivc_driver);
> +
> +MODULE_AUTHOR("Daniel Scally <dan.scally@ideasonboard.com>");
> +MODULE_DESCRIPTION("Renesas RZ/V2H Input Video Control Block driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ff3eac313994b0c109004ca7ba960f7edf1d2112
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2H Input Video Control Block driver
> + *
> + * Copyright (C) 2025 Ideas on Board Oy
> + */
> +
> +#include "rzv2h-ivc.h"
> +
> +#include <linux/media.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/v4l2-mediabus.h>
> +
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +
> +#define RZV2H_IVC_N_INPUTS_PER_OUTPUT 6
> +
> +/*
> + * We support 8/10/12/14/16/20 bit input in any bayer order, but the output
> + * format is fixed at 20-bits with the same order as the input.
> + */
> +static const struct {
> + u32 inputs[RZV2H_IVC_N_INPUTS_PER_OUTPUT];
> + u32 output;
> +} rzv2h_ivc_formats[] = {
> + {
> + .inputs = {
> + MEDIA_BUS_FMT_SBGGR8_1X8,
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SBGGR12_1X12,
> + MEDIA_BUS_FMT_SBGGR14_1X14,
> + MEDIA_BUS_FMT_SBGGR16_1X16,
> + MEDIA_BUS_FMT_SBGGR20_1X20,
> + },
> + .output = MEDIA_BUS_FMT_SBGGR20_1X20
> + },
> + {
> + .inputs = {
> + MEDIA_BUS_FMT_SGBRG8_1X8,
> + MEDIA_BUS_FMT_SGBRG10_1X10,
> + MEDIA_BUS_FMT_SGBRG12_1X12,
> + MEDIA_BUS_FMT_SGBRG14_1X14,
> + MEDIA_BUS_FMT_SGBRG16_1X16,
> + MEDIA_BUS_FMT_SGBRG20_1X20,
> + },
> + .output = MEDIA_BUS_FMT_SGBRG20_1X20
> + },
> + {
> + .inputs = {
> + MEDIA_BUS_FMT_SGRBG8_1X8,
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SGRBG12_1X12,
> + MEDIA_BUS_FMT_SGRBG14_1X14,
> + MEDIA_BUS_FMT_SGRBG16_1X16,
> + MEDIA_BUS_FMT_SGRBG20_1X20,
> + },
> + .output = MEDIA_BUS_FMT_SGRBG20_1X20
> + },
> + {
> + .inputs = {
> + MEDIA_BUS_FMT_SRGGB8_1X8,
> + MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SRGGB12_1X12,
> + MEDIA_BUS_FMT_SRGGB14_1X14,
> + MEDIA_BUS_FMT_SRGGB16_1X16,
> + MEDIA_BUS_FMT_SRGGB20_1X20,
> + },
> + .output = MEDIA_BUS_FMT_SRGGB20_1X20
> + },
> +};
> +
> +static u32 rzv2h_ivc_get_mbus_output_from_input(u32 mbus_code)
> +{
> + unsigned int i, j;
> +
> + for (i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++) {
> + for (j = 0; j < RZV2H_IVC_N_INPUTS_PER_OUTPUT; j++) {
> + if (rzv2h_ivc_formats[i].inputs[j] == mbus_code)
> + return rzv2h_ivc_formats[i].output;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + const struct v4l2_mbus_framefmt *fmt;
> + unsigned int order_index;
> + unsigned int index;
> +
> + /*
> + * On the source pad, only the 20-bit format corresponding to the sink
> + * pad format's bayer order is supported.
> + */
> + if (code->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
> + if (code->index)
> + return -EINVAL;
> +
> + fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SINK_PAD);
> + code->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code);
> +
> + return 0;
> + }
> +
> + if (code->index >= ARRAY_SIZE(rzv2h_ivc_formats) *
> + RZV2H_IVC_N_INPUTS_PER_OUTPUT)
> + return -EINVAL;
> +
> + order_index = code->index / RZV2H_IVC_N_INPUTS_PER_OUTPUT;
> + index = code->index % RZV2H_IVC_N_INPUTS_PER_OUTPUT;
> +
> + code->code = rzv2h_ivc_formats[order_index].inputs[index];
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + const struct v4l2_mbus_framefmt *fmt;
> +
> + if (fse->index > 0)
> + return -EINVAL;
> +
> + if (fse->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
> + fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SINK_PAD);
> +
> + if (fse->code != fmt->code)
> + return -EINVAL;
> +
> + fse->min_width = fmt->width;
> + fse->max_width = fmt->width;
> + fse->min_height = fmt->height;
> + fse->max_height = fmt->height;
> +
> + return 0;
> + }
> +
> + if (!rzv2h_ivc_get_mbus_output_from_input(fse->code))
> + return -EINVAL;
> +
> + fse->min_width = RZV2H_IVC_MIN_WIDTH;
> + fse->max_width = RZV2H_IVC_MAX_WIDTH;
> + fse->min_height = RZV2H_IVC_MIN_HEIGHT;
> + fse->max_height = RZV2H_IVC_MAX_HEIGHT;
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_format *format)
> +{
> + struct v4l2_mbus_framefmt *fmt = &format->format;
> + struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
> +
> + if (format->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD)
> + return v4l2_subdev_get_fmt(sd, state, format);
> +
> + sink_fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SINK_PAD);
> +
> + sink_fmt->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code) ?
> + fmt->code : rzv2h_ivc_formats[0].inputs[0];
> +
> + sink_fmt->width = clamp(fmt->width, RZV2H_IVC_MIN_WIDTH,
> + RZV2H_IVC_MAX_WIDTH);
> + sink_fmt->height = clamp(fmt->height, RZV2H_IVC_MIN_HEIGHT,
> + RZV2H_IVC_MAX_HEIGHT);
> +
> + *fmt = *sink_fmt;
> +
> + src_fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SOURCE_PAD);
> + *src_fmt = *sink_fmt;
> + src_fmt->code = rzv2h_ivc_get_mbus_output_from_input(sink_fmt->code);
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + /*
> + * We have a single source pad, which has a single stream. V4L2 core has
> + * already validated those things. The actual power-on and programming
> + * of registers will be done through the video device's .vidioc_streamon
> + * so there's nothing to actually do here...
> + */
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops rzv2h_ivc_pad_ops = {
> + .enum_mbus_code = rzv2h_ivc_enum_mbus_code,
> + .enum_frame_size = rzv2h_ivc_enum_frame_size,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = rzv2h_ivc_set_fmt,
> + .enable_streams = rzv2h_ivc_enable_streams,
> + .disable_streams = rzv2h_ivc_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_core_ops rzv2h_ivc_core_ops = {
> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_ops rzv2h_ivc_subdev_ops = {
> + .core = &rzv2h_ivc_core_ops,
> + .pad = &rzv2h_ivc_pad_ops,
> +};
> +
> +static int rzv2h_ivc_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
> +
> + sink_fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SINK_PAD);
> + sink_fmt->width = RZV2H_IVC_DEFAULT_WIDTH;
> + sink_fmt->height = RZV2H_IVC_DEFAULT_HEIGHT;
> + sink_fmt->field = V4L2_FIELD_NONE;
> + sink_fmt->code = MEDIA_BUS_FMT_SRGGB16_1X16;
> + sink_fmt->colorspace = V4L2_COLORSPACE_RAW;
> + sink_fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
> + sink_fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
> + sink_fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(
> + true, sink_fmt->colorspace, sink_fmt->ycbcr_enc);
> +
> + src_fmt = v4l2_subdev_state_get_format(state,
> + RZV2H_IVC_SUBDEV_SOURCE_PAD);
> +
> + *src_fmt = *sink_fmt;
> + src_fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20;
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_registered(struct v4l2_subdev *sd)
> +{
> + struct rzv2h_ivc *ivc = container_of(sd, struct rzv2h_ivc, subdev.sd);
> +
> + return rzv2h_ivc_init_vdev(ivc, sd->v4l2_dev);
> +}
> +
> +static const struct v4l2_subdev_internal_ops rzv2h_ivc_subdev_internal_ops = {
> + .init_state = rzv2h_ivc_init_state,
> + .registered = rzv2h_ivc_registered,
> +};
> +
> +static int rzv2h_ivc_link_validate(struct media_link *link)
> +{
> + struct video_device *vdev =
> + media_entity_to_video_device(link->source->entity);
> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
> + struct v4l2_subdev *sd =
> + media_entity_to_v4l2_subdev(link->sink->entity);
> + const struct rzv2h_ivc_format *fmt;
> + const struct v4l2_pix_format_mplane *pix;
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *mf;
> + unsigned int i;
> + int ret = 0;
> +
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + mf = v4l2_subdev_state_get_format(state, link->sink->index);
> +
> + pix = &ivc->format.pix;
> + fmt = ivc->format.fmt;
> +
> + if (mf->width != pix->width || mf->height != pix->height) {
> + dev_dbg(ivc->dev,
> + "link '%s':%u -> '%s':%u not valid: %ux%u != %ux%u\n",
> + link->source->entity->name, link->source->index,
> + link->sink->entity->name, link->sink->index,
> + mf->width, mf->height,
> + pix->width, pix->height);
> + ret = -EPIPE;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(fmt->mbus_codes); i++)
> + if (mf->code == fmt->mbus_codes[i])
> + break;
> +
> + if (i == ARRAY_SIZE(fmt->mbus_codes)) {
> + dev_dbg(ivc->dev,
> + "link '%s':%u -> '%s':%u not valid: pixel format %p4cc cannot produce mbus_code 0x%04x\n",
> + link->source->entity->name, link->source->index,
> + link->sink->entity->name, link->sink->index,
> + &pix->pixelformat, mf->code);
> + ret = -EPIPE;
> + }
> +
> + v4l2_subdev_unlock_state(state);
> +
> + return ret;
> +}
> +
> +static const struct media_entity_operations rzv2h_ivc_media_ops = {
> + .link_validate = rzv2h_ivc_link_validate,
> +};
> +
> +int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc)
> +{
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + /* Initialise subdevice */
> + sd = &ivc->subdev.sd;
> + sd->dev = ivc->dev;
> + v4l2_subdev_init(sd, &rzv2h_ivc_subdev_ops);
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + sd->internal_ops = &rzv2h_ivc_subdev_internal_ops;
> + sd->entity.ops = &rzv2h_ivc_media_ops;
> +
> + ivc->subdev.pads[RZV2H_IVC_SUBDEV_SINK_PAD].flags = MEDIA_PAD_FL_SINK;
> + ivc->subdev.pads[RZV2H_IVC_SUBDEV_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE;
> +
> + snprintf(sd->name, sizeof(sd->name), "rzv2h ivc block");
> +
> + ret = media_entity_pads_init(&sd->entity, RZV2H_IVC_NUM_SUBDEV_PADS,
> + ivc->subdev.pads);
> + if (ret) {
> + dev_err(ivc->dev, "failed to initialise media entity\n");
> + return ret;
> + }
> +
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret) {
> + dev_err(ivc->dev, "failed to finalize subdev init\n");
> + goto err_cleanup_subdev_entity;
> + }
> +
> + ret = v4l2_async_register_subdev(sd);
> + if (ret) {
> + dev_err(ivc->dev, "failed to register subdevice\n");
> + goto err_cleanup_subdev;
> + }
> +
> + return 0;
> +
> +err_cleanup_subdev:
> + v4l2_subdev_cleanup(sd);
> +err_cleanup_subdev_entity:
> + media_entity_cleanup(&sd->entity);
> +
> + return ret;
> +}
> +
> +void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc)
> +{
> + struct v4l2_subdev *sd = &ivc->subdev.sd;
> +
> + v4l2_subdev_cleanup(sd);
> + media_entity_remove_links(&sd->entity);
> + v4l2_async_unregister_subdev(sd);
> + media_entity_cleanup(&sd->entity);
> +}
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..63e20b83d596aff8a9534720b70fe75aa6c0c0ae
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
> @@ -0,0 +1,568 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/V2H Input Video Control Block driver
> + *
> + * Copyright (C) 2025 Ideas on Board Oy
> + */
> +
> +#include "rzv2h-ivc.h"
> +
> +#include <linux/cleanup.h>
> +#include <linux/iopoll.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/minmax.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <media/mipi-csi2.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#define RZV2H_IVC_FIXED_HBLANK 0x20
> +#define RZV2H_IVC_MIN_VBLANK(hts) max(0x1b, 15 + (120501 / (hts)))
> +
> +struct rzv2h_ivc_buf {
> + struct vb2_v4l2_buffer vb;
> + struct list_head queue;
> + dma_addr_t addr;
> +};
> +
> +#define to_rzv2h_ivc_buf(vbuf) \
> + container_of(vbuf, struct rzv2h_ivc_buf, vb)
> +
> +static const struct rzv2h_ivc_format rzv2h_ivc_formats[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_SBGGR8,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SBGGR8_1X8,
> + },
> + .dtype = MIPI_CSI2_DT_RAW8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SGBRG8,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SGBRG8_1X8,
> + },
> + .dtype = MIPI_CSI2_DT_RAW8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SGRBG8,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SGRBG8_1X8,
> + },
> + .dtype = MIPI_CSI2_DT_RAW8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SRGGB8,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SRGGB8_1X8,
> + },
> + .dtype = MIPI_CSI2_DT_RAW8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_RAW_CRU10,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SGBRG10_1X10,
> + MEDIA_BUS_FMT_SGRBG10_1X10,
> + MEDIA_BUS_FMT_SRGGB10_1X10
> + },
> + .dtype = MIPI_CSI2_DT_RAW10,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_RAW_CRU12,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SBGGR12_1X12,
> + MEDIA_BUS_FMT_SGBRG12_1X12,
> + MEDIA_BUS_FMT_SGRBG12_1X12,
> + MEDIA_BUS_FMT_SRGGB12_1X12
> + },
> + .dtype = MIPI_CSI2_DT_RAW12,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_RAW_CRU14,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SBGGR14_1X14,
> + MEDIA_BUS_FMT_SGBRG14_1X14,
> + MEDIA_BUS_FMT_SGRBG14_1X14,
> + MEDIA_BUS_FMT_SRGGB14_1X14
> + },
> + .dtype = MIPI_CSI2_DT_RAW14,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SBGGR16,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SBGGR16_1X16,
> + },
> + .dtype = MIPI_CSI2_DT_RAW16,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SGBRG16,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SGBRG16_1X16,
> + },
> + .dtype = MIPI_CSI2_DT_RAW16,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SGRBG16,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SGRBG16_1X16,
> + },
> + .dtype = MIPI_CSI2_DT_RAW16,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_SRGGB16,
> + .mbus_codes = {
> + MEDIA_BUS_FMT_SRGGB16_1X16,
> + },
> + .dtype = MIPI_CSI2_DT_RAW16,
> + },
> +};
> +
> +static void rzv2h_ivc_transfer_buffer(struct work_struct *work)
> +{
> + struct rzv2h_ivc *ivc = container_of(work, struct rzv2h_ivc,
> + buffers.work);
> + struct rzv2h_ivc_buf *buf;
> +
> + scoped_guard(spinlock, &ivc->buffers.lock) {
> + if (ivc->buffers.curr) {
> + ivc->buffers.curr->vb.sequence = ivc->buffers.sequence++;
> + vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf,
> + VB2_BUF_STATE_DONE);
> + ivc->buffers.curr = NULL;
> + }
> +
> + buf = list_first_entry_or_null(&ivc->buffers.queue,
> + struct rzv2h_ivc_buf, queue);
> + }
> +
> + if (buf)
> + list_del(&buf->queue);
> + else
> + return;
if (!buf)
return;
list_del(&buf->queue);
> +
> + ivc->buffers.curr = buf;
> + buf->addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_SADDL_P0, buf->addr);
> +
> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
> + ivc->vvalid_ifp = 2;
> + }
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_FRCON, 0x1);
> +}
> +
> +static int rzv2h_ivc_pipeline_started(struct media_entity *entity)
> +{
> + struct video_device *vdev = media_entity_to_video_device(entity);
> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
> +
> + guard(spinlock)(&ivc->buffers.lock);
> +
> + if (list_empty(&ivc->buffers.queue)) {
> + /*
> + * The driver waits for interrupts to send a new frame and
> + * tracks their receipt in the vvalid_ifp variable. .buf_queue()
> + * will queue work if vvalid_ifp == 0 to trigger a new frame (an
> + * event that normally would only occur if no buffer was ready
> + * when the interrupt arrived). If there are no buffers in the
> + * queue yet, we set vvalid_ifp to zero so that the next queue
> + * will trigger the work.
> + */
> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
> + ivc->vvalid_ifp = 0;
> + }
return 0;
}
> + } else {
and drop the else branch
I'm not 100% sure I got this part, will follow-up offline to get some
clarification
> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
> + }
> +
> + return 0;
> +}
> +
> +static void rzv2h_ivc_pipeline_stopped(struct media_entity *entity)
> +{
> + struct video_device *vdev = media_entity_to_video_device(entity);
> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
> + u32 val = 0;
> +
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_STOP, 0x1);
> + readl_poll_timeout(ivc->base + RZV2H_IVC_REG_FM_STOP,
> + val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
> +}
> +
> +static const struct media_entity_operations rzv2h_ivc_media_ops = {
> + .pipeline_started = rzv2h_ivc_pipeline_started,
> + .pipeline_stopped = rzv2h_ivc_pipeline_stopped,
> +};
> +
> +static int rzv2h_ivc_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
> + unsigned int *num_planes, unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
> +
> + if (*num_planes && *num_planes > 1)
> + return -EINVAL;
> +
> + if (sizes[0] && sizes[0] < ivc->format.pix.plane_fmt[0].sizeimage)
> + return -EINVAL;
> +
> + *num_planes = 1;
> +
> + if (!sizes[0])
> + sizes[0] = ivc->format.pix.plane_fmt[0].sizeimage;
> +
> + return 0;
> +}
> +
> +static void rzv2h_ivc_buf_queue(struct vb2_buffer *vb)
> +{
> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(vb->vb2_queue);
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct rzv2h_ivc_buf *buf = to_rzv2h_ivc_buf(vbuf);
> +
> + scoped_guard(spinlock, &ivc->buffers.lock) {
> + list_add_tail(&buf->queue, &ivc->buffers.queue);
> + }
> +
> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
> + if (vb2_is_streaming(vb->vb2_queue) && !ivc->vvalid_ifp)
> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
> + }
> +}
> +
> +static void rzv2h_ivc_format_configure(struct rzv2h_ivc *ivc)
> +{
> + const struct rzv2h_ivc_format *fmt = ivc->format.fmt;
> + struct v4l2_pix_format_mplane *pix = &ivc->format.pix;
> + unsigned int vblank;
> + unsigned int hts;
> +
> + /* Currently only CRU packed pixel formats are supported */
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
> + RZV2H_IVC_INPUT_FMT_CRU_PACKED);
> +
> + rzv2h_ivc_update_bits(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
> + RZV2H_IVC_PXFMT_DTYPE, fmt->dtype);
> +
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_HSIZE, pix->width);
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_VSIZE, pix->height);
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_STRD,
> + pix->plane_fmt[0].bytesperline);
> +
> + /*
> + * The ISP has minimum vertical blanking requirements that must be
> + * adhered to by the IVC. The minimum is a function of the Iridix blocks
> + * clocking requirements and the width of the image and horizontal
> + * blanking, but if we assume the worst case then it boils down to the
> + * below (plus one to the numerator to ensure the answer is rounded up)
> + */
> +
> + hts = pix->width + RZV2H_IVC_FIXED_HBLANK;
> + vblank = RZV2H_IVC_MIN_VBLANK(hts);
> +
> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_BLANK,
> + RZV2H_IVC_VBLANK(vblank));
> +}
> +
> +static void rzv2h_ivc_return_buffers(struct rzv2h_ivc *ivc,
> + enum vb2_buffer_state state)
> +{
> + struct rzv2h_ivc_buf *buf, *tmp;
> +
> + guard(spinlock)(&ivc->buffers.lock);
> +
> + if (ivc->buffers.curr) {
> + vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf, state);
> + ivc->buffers.curr = NULL;
> + }
> +
> + list_for_each_entry_safe(buf, tmp, &ivc->buffers.queue, queue) {
> + list_del(&buf->queue);
> + vb2_buffer_done(&buf->vb.vb2_buf, state);
> + }
> +}
> +
> +static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
> + int ret;
> +
> + ivc->buffers.sequence = 0;
> + ivc->vvalid_ifp = 2;
> +
> + ret = pm_runtime_resume_and_get(ivc->dev);
> + if (ret)
> + goto err_return_buffers;
> +
> + ret = video_device_pipeline_alloc_start(&ivc->vdev.dev);
> + if (ret) {
> + dev_err(ivc->dev, "failed to start media pipeline\n");
> + goto err_pm_runtime_put;
> + }
> +
> + rzv2h_ivc_format_configure(ivc);
> +
> + ret = video_device_pipeline_started(&ivc->vdev.dev);
> + if (ret < 0)
> + goto err_stop_pipeline;
> +
> + return 0;
> +
> +err_stop_pipeline:
> + video_device_pipeline_stop(&ivc->vdev.dev);
> +err_pm_runtime_put:
> + pm_runtime_put(ivc->dev);
> +err_return_buffers:
> + rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_QUEUED);
> +
> + return ret;
> +}
> +
> +static void rzv2h_ivc_stop_streaming(struct vb2_queue *q)
> +{
> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
> +
> + video_device_pipeline_stopped(&ivc->vdev.dev);
> + rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_ERROR);
> + video_device_pipeline_stop(&ivc->vdev.dev);
> + pm_runtime_mark_last_busy(ivc->dev);
> + pm_runtime_put_autosuspend(ivc->dev);
> +}
> +
> +static const struct vb2_ops rzv2h_ivc_vb2_ops = {
> + .queue_setup = &rzv2h_ivc_queue_setup,
> + .buf_queue = &rzv2h_ivc_buf_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .start_streaming = &rzv2h_ivc_start_streaming,
> + .stop_streaming = &rzv2h_ivc_stop_streaming,
> +};
> +
> +static const struct rzv2h_ivc_format *
> +rzv2h_ivc_format_from_pixelformat(u32 fourcc)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++)
> + if (fourcc == rzv2h_ivc_formats[i].fourcc)
> + return &rzv2h_ivc_formats[i];
> +
> + return &rzv2h_ivc_formats[0];
> +}
> +
> +static int rzv2h_ivc_enum_fmt_vid_out(struct file *file, void *fh,
> + struct v4l2_fmtdesc *f)
> +{
> + if (f->index >= ARRAY_SIZE(rzv2h_ivc_formats))
> + return -EINVAL;
> +
> + f->pixelformat = rzv2h_ivc_formats[f->index].fourcc;
> + return 0;
> +}
> +
> +static int rzv2h_ivc_g_fmt_vid_out(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rzv2h_ivc *ivc = video_drvdata(file);
> +
> + f->fmt.pix_mp = ivc->format.pix;
> +
> + return 0;
> +}
> +
> +static void rzv2h_ivc_try_fmt(struct v4l2_pix_format_mplane *pix,
> + const struct rzv2h_ivc_format *fmt)
> +{
> + pix->pixelformat = fmt->fourcc;
> +
> + pix->width = clamp(pix->width, RZV2H_IVC_MIN_WIDTH,
> + RZV2H_IVC_MAX_WIDTH);
> + pix->height = clamp(pix->height, RZV2H_IVC_MIN_HEIGHT,
> + RZV2H_IVC_MAX_HEIGHT);
> +
> + pix->field = V4L2_FIELD_NONE;
> + pix->colorspace = V4L2_COLORSPACE_RAW;
> + pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
> + pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> + pix->colorspace,
> + pix->ycbcr_enc);
> +
> + v4l2_fill_pixfmt_mp(pix, pix->pixelformat, pix->width, pix->height);
> +}
> +
> +static void rzv2h_ivc_set_format(struct rzv2h_ivc *ivc,
> + struct v4l2_pix_format_mplane *pix)
> +{
> + const struct rzv2h_ivc_format *fmt;
> +
> + fmt = rzv2h_ivc_format_from_pixelformat(pix->pixelformat);
> +
> + rzv2h_ivc_try_fmt(pix, fmt);
> + ivc->format.pix = *pix;
> + ivc->format.fmt = fmt;
> +}
> +
> +static int rzv2h_ivc_s_fmt_vid_out(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rzv2h_ivc *ivc = video_drvdata(file);
> + struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
> +
> + if (vb2_is_busy(&ivc->vdev.vb2q))
> + return -EBUSY;
> +
> + rzv2h_ivc_set_format(ivc, pix);
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_try_fmt_vid_out(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + const struct rzv2h_ivc_format *fmt;
> +
> + fmt = rzv2h_ivc_format_from_pixelformat(f->fmt.pix.pixelformat);
> + rzv2h_ivc_try_fmt(&f->fmt.pix_mp, fmt);
> +
> + return 0;
> +}
> +
> +static int rzv2h_ivc_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
> +{
> + strscpy(cap->driver, "rzv2h-ivc", sizeof(cap->driver));
> + strscpy(cap->card, "Renesas Input Video Control", sizeof(cap->card));
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops rzv2h_ivc_v4l2_ioctl_ops = {
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> + .vidioc_enum_fmt_vid_out = rzv2h_ivc_enum_fmt_vid_out,
> + .vidioc_g_fmt_vid_out_mplane = rzv2h_ivc_g_fmt_vid_out,
> + .vidioc_s_fmt_vid_out_mplane = rzv2h_ivc_s_fmt_vid_out,
> + .vidioc_try_fmt_vid_out_mplane = rzv2h_ivc_try_fmt_vid_out,
> + .vidioc_querycap = rzv2h_ivc_querycap,
> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +};
> +
> +static const struct v4l2_file_operations rzv2h_ivc_v4l2_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = video_ioctl2,
> + .open = v4l2_fh_open,
> + .release = vb2_fop_release,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev)
> +{
> + struct v4l2_pix_format_mplane pix = { };
> + struct video_device *vdev;
> + struct vb2_queue *vb2q;
> + int ret;
> +
> + spin_lock_init(&ivc->buffers.lock);
> + INIT_LIST_HEAD(&ivc->buffers.queue);
> + INIT_WORK(&ivc->buffers.work, rzv2h_ivc_transfer_buffer);
> +
> + ivc->buffers.async_wq = alloc_workqueue("rzv2h-ivc", 0, 0);
> + if (!ivc->buffers.async_wq)
> + return -EINVAL;
> +
> + /* Initialise vb2 queue */
> + vb2q = &ivc->vdev.vb2q;
> + vb2q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> + vb2q->io_modes = VB2_MMAP | VB2_DMABUF;
> + vb2q->drv_priv = ivc;
> + vb2q->mem_ops = &vb2_dma_contig_memops;
> + vb2q->ops = &rzv2h_ivc_vb2_ops;
> + vb2q->buf_struct_size = sizeof(struct rzv2h_ivc_buf);
> + vb2q->min_queued_buffers = 0;
> + vb2q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + vb2q->lock = &ivc->lock;
> + vb2q->dev = ivc->dev;
> +
> + ret = vb2_queue_init(vb2q);
> + if (ret) {
> + dev_err(ivc->dev, "vb2 queue init failed\n");
> + goto err_destroy_workqueue;
> + }
> +
> + /* Initialise Video Device */
> + vdev = &ivc->vdev.dev;
> + strscpy(vdev->name, "rzv2h-ivc", sizeof(vdev->name));
> + vdev->release = video_device_release_empty;
> + vdev->fops = &rzv2h_ivc_v4l2_fops;
> + vdev->ioctl_ops = &rzv2h_ivc_v4l2_ioctl_ops;
> + vdev->lock = &ivc->lock;
> + vdev->v4l2_dev = v4l2_dev;
> + vdev->queue = vb2q;
> + vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_STREAMING;
> + vdev->vfl_dir = VFL_DIR_TX;
> + video_set_drvdata(vdev, ivc);
> +
> + pix.pixelformat = V4L2_PIX_FMT_SRGGB16;
> + pix.width = RZV2H_IVC_DEFAULT_WIDTH;
> + pix.height = RZV2H_IVC_DEFAULT_HEIGHT;
> + rzv2h_ivc_set_format(ivc, &pix);
> +
> + ivc->vdev.pad.flags = MEDIA_PAD_FL_SOURCE;
> + ivc->vdev.dev.entity.ops = &rzv2h_ivc_media_ops;
> + ret = media_entity_pads_init(&ivc->vdev.dev.entity, 1, &ivc->vdev.pad);
> + if (ret)
> + goto err_release_vb2q;
> +
> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> + if (ret) {
> + dev_err(ivc->dev, "failed to register IVC video device\n");
> + goto err_cleanup_vdev_entity;
> + }
> +
> + ret = media_create_pad_link(&vdev->entity, 0, &ivc->subdev.sd.entity,
> + RZV2H_IVC_SUBDEV_SINK_PAD,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> + if (ret) {
> + dev_err(ivc->dev, "failed to create media link\n");
> + goto err_unregister_vdev;
> + }
> +
> + return 0;
> +
> +err_unregister_vdev:
> + video_unregister_device(vdev);
> +err_cleanup_vdev_entity:
> + media_entity_cleanup(&vdev->entity);
> +err_release_vb2q:
> + vb2_queue_release(vb2q);
> +err_destroy_workqueue:
> + destroy_workqueue(ivc->buffers.async_wq);
> +
> + return ret;
> +}
> +
> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc)
> +{
> + struct video_device *vdev = &ivc->vdev.dev;
> + struct vb2_queue *vb2q = &ivc->vdev.vb2q;
> +
> + if (!ivc->sched)
> + return;
> +
> + vb2_video_unregister_device(vdev);
> + media_entity_cleanup(&vdev->entity);
> + vb2_queue_release(vb2q);
> +}
> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..709c6a9398fe2484c2acb03d443d58ea4e153a66
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Renesas RZ/V2H Input Video Control Block driver
> + *
> + * Copyright (C) 2025 Ideas on Board Oy
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/reset.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +#include <linux/workqueue.h>
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#define RZV2H_IVC_REG_AXIRX_PLNUM 0x0000
> +#define RZV2H_IVC_ONE_EXPOSURE 0x00
> +#define RZV2H_IVC_TWO_EXPOSURE 0x01
> +#define RZV2H_IVC_REG_AXIRX_PXFMT 0x0004
> +#define RZV2H_IVC_INPUT_FMT_MIPI (0 << 16)
> +#define RZV2H_IVC_INPUT_FMT_CRU_PACKED (1 << 16)
> +#define RZV2H_IVC_PXFMT_DTYPE GENMASK(7, 0)
> +#define RZV2H_IVC_REG_AXIRX_SADDL_P0 0x0010
> +#define RZV2H_IVC_REG_AXIRX_SADDH_P0 0x0014
> +#define RZV2H_IVC_REG_AXIRX_SADDL_P1 0x0018
> +#define RZV2H_IVC_REG_AXIRX_SADDH_P1 0x001c
> +#define RZV2H_IVC_REG_AXIRX_HSIZE 0x0020
> +#define RZV2H_IVC_REG_AXIRX_VSIZE 0x0024
> +#define RZV2H_IVC_REG_AXIRX_BLANK 0x0028
> +#define RZV2H_IVC_VBLANK(x) ((x) << 16)
> +#define RZV2H_IVC_REG_AXIRX_STRD 0x0030
> +#define RZV2H_IVC_REG_AXIRX_ISSU 0x0040
> +#define RZV2H_IVC_REG_AXIRX_ERACT 0x0048
> +#define RZV2H_IVC_REG_FM_CONTEXT 0x0100
> +#define RZV2H_IVC_SOFTWARE_CFG 0x00
> +#define RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG BIT(0)
> +#define RZV2H_IVC_MULTI_CONTEXT_SW_HW_CFG BIT(1)
> +#define RZV2H_IVC_REG_FM_MCON 0x0104
> +#define RZV2H_IVC_REG_FM_FRCON 0x0108
> +#define RZV2H_IVC_REG_FM_STOP 0x010c
> +#define RZV2H_IVC_REG_FM_INT_EN 0x0120
> +#define RZV2H_IVC_VVAL_IFPE BIT(0)
> +#define RZV2H_IVC_REG_FM_INT_STA 0x0124
> +#define RZV2H_IVC_REG_AXIRX_FIFOCAP0 0x0208
> +#define RZV2H_IVC_REG_CORE_CAPCON 0x020c
> +#define RZV2H_IVC_REG_CORE_FIFOCAP0 0x0228
> +#define RZV2H_IVC_REG_CORE_FIFOCAP1 0x022c
> +
> +#define RZV2H_IVC_MIN_WIDTH 640
> +#define RZV2H_IVC_MAX_WIDTH 4096
> +#define RZV2H_IVC_MIN_HEIGHT 480
> +#define RZV2H_IVC_MAX_HEIGHT 4096
> +#define RZV2H_IVC_DEFAULT_WIDTH 1920
> +#define RZV2H_IVC_DEFAULT_HEIGHT 1080
> +
> +#define RZV2H_IVC_NUM_HW_RESOURCES 3
> +
> +struct device;
> +
> +enum rzv2h_ivc_subdev_pads {
> + RZV2H_IVC_SUBDEV_SINK_PAD,
> + RZV2H_IVC_SUBDEV_SOURCE_PAD,
> + RZV2H_IVC_NUM_SUBDEV_PADS
> +};
> +
> +struct rzv2h_ivc_format {
> + u32 fourcc;
> + /*
> + * The CRU packed pixel formats are bayer-order agnostic, so each could
> + * support any one of the 4 possible media bus formats.
> + */
> + u32 mbus_codes[4];
> + u8 dtype;
> +};
> +
> +struct rzv2h_ivc {
> + struct device *dev;
> + void __iomem *base;
> + struct clk_bulk_data clks[RZV2H_IVC_NUM_HW_RESOURCES];
> + struct reset_control_bulk_data resets[RZV2H_IVC_NUM_HW_RESOURCES];
> + int irqnum;
> + u8 vvalid_ifp;
> +
> + struct {
> + struct video_device dev;
> + struct vb2_queue vb2q;
> + struct media_pad pad;
> + } vdev;
> +
> + struct {
> + struct v4l2_subdev sd;
> + struct media_pad pads[RZV2H_IVC_NUM_SUBDEV_PADS];
> + } subdev;
> +
> + struct {
> + /* Spinlock to guard buffer queue */
> + spinlock_t lock;
> + struct workqueue_struct *async_wq;
> + struct work_struct work;
> + struct list_head queue;
> + struct rzv2h_ivc_buf *curr;
> + unsigned int sequence;
> + } buffers;
> +
> + struct media_job_scheduler *sched;
> +
> + struct {
> + struct v4l2_pix_format_mplane pix;
> + const struct rzv2h_ivc_format *fmt;
> + } format;
> +
> + /* Mutex to provide to vb2 */
> + struct mutex lock;
> + /* Lock to protect the interrupt counter */
> + spinlock_t spinlock;
> +};
> +
> +int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev);
> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc);
> +int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc);
> +void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc);
> +void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val);
> +void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
> + u32 mask, u32 val);
Mostly minor nits, I guess next version will be the good one.
Could you please run v4l2-compliance and attach the test summary to
the next cover letter version ?
Thanks
j
>
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-08 14:49 ` Jacopo Mondi
@ 2025-07-08 14:57 ` Dan Scally
2025-07-08 15:51 ` Jacopo Mondi
0 siblings, 1 reply; 21+ messages in thread
From: Dan Scally @ 2025-07-08 14:57 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
Hi Jacopo
On 08/07/2025 15:49, Jacopo Mondi wrote:
> On Fri, Jul 04, 2025 at 12:20:21PM +0100, Daniel Scally wrote:
>> Add a driver for the Input Video Control block in an RZ/V2H SoC which
>> feeds data into the Arm Mali-C55 ISP.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v4:
>>
>> - Update the compatible to renesas,r9a09g057-ivc
>> - Dropped the media jobs / scheduler functionality, and re
>> worked the driver to have its own workqueue pushing frames
>> - Fix .enum_mbus_code() to return 20-bit output for source
>> pad.
>> - Fix some alignment issues
>> - Make the forwarding of sink to source pad format a more
>> explicit operation.
>> - Rename rzv2h_initialise_video_device_and_queue()
>> - Reversed order of v4l2_subdev_init_finalize() and
>> v4l2_async_register_subdev() to make sure everything is
>> finished initialising before registering the subdev.
>> - Change function to MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER
>> - Use a parametised macro for min vblank
>> - Minor formatting
>> - Use the DEFAULT macros for quantization / ycbcr_enc values
>> - Switch to using the mplane API
>> - Dropped select RESET_CONTROLLER
>> - Used the new helpers for starting a media pipeline
>> - Switch from threaded irq to normal with driver workqueue
>> and revised startup routine
>>
>> Changes in v3:
>>
>> - Account for the renamed CRU pixel formats
>>
>> Changes in v2:
>>
>> - Added selects and depends statements to Kconfig entry
>> - Fixed copyright year
>> - Stopped including in .c files headers already included in .h
>> - Fixed uninitialized variable in iterator
>> - Only check vvalid member in interrupt function and wait
>> unconditionally elsewhere
>> - __maybe_unused for the PM ops
>> - Initialise the subdevice after setting up PM
>> - Fixed the remove function for the driver to actually do
>> something.
>> - Some minor formatting changes
>> - Fixed the quantization member for the format
>> - Changes accounting for the v2 of the media jobs framework
>> - Change min_queued_buffers to 0
>> ---
>> drivers/media/platform/renesas/Kconfig | 2 +
>> drivers/media/platform/renesas/Makefile | 1 +
>> drivers/media/platform/renesas/rzv2h-ivc/Kconfig | 16 +
>> drivers/media/platform/renesas/rzv2h-ivc/Makefile | 5 +
>> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c | 228 +++++++++
>> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c | 376 ++++++++++++++
>> .../platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c | 568 +++++++++++++++++++++
>> .../media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h | 131 +++++
>> 8 files changed, 1327 insertions(+)
>>
>> diff --git a/drivers/media/platform/renesas/Kconfig b/drivers/media/platform/renesas/Kconfig
>> index 27a54fa7908384f2e8200f0f7283a82b0ae8435c..5462e524c3708be87a50dd80d4b4017a2466aa99 100644
>> --- a/drivers/media/platform/renesas/Kconfig
>> +++ b/drivers/media/platform/renesas/Kconfig
>> @@ -42,6 +42,8 @@ config VIDEO_SH_VOU
>> source "drivers/media/platform/renesas/rcar-isp/Kconfig"
>> source "drivers/media/platform/renesas/rcar-vin/Kconfig"
>> source "drivers/media/platform/renesas/rzg2l-cru/Kconfig"
>> +source "drivers/media/platform/renesas/rzv2h-ivc/Kconfig"
>> +
> Additional empty line
>
>> # Mem2mem drivers
>>
>> diff --git a/drivers/media/platform/renesas/Makefile b/drivers/media/platform/renesas/Makefile
>> index 1127259c09d6a51b70803e76c495918e06777f67..b6b4abf01db246aaf8269b8027efee9b0b32083a 100644
>> --- a/drivers/media/platform/renesas/Makefile
>> +++ b/drivers/media/platform/renesas/Makefile
>> @@ -6,6 +6,7 @@
>> obj-y += rcar-isp/
>> obj-y += rcar-vin/
>> obj-y += rzg2l-cru/
>> +obj-y += rzv2h-ivc/
>> obj-y += vsp1/
>>
>> obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Kconfig b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..bde2ba52329b522cccd63667f376edb2f4a0608f
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/Kconfig
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config VIDEO_RZV2H_IVC
>> + tristate "Renesas RZ/V2H Input Video Control block driver"
>> + depends on V4L_PLATFORM_DRIVERS
>> + depends on VIDEO_DEV
>> + depends on ARCH_RENESAS || COMPILE_TEST
>> + depends on OF
>> + select VIDEOBUF2_DMA_CONTIG
>> + select MEDIA_CONTROLLER
>> + select VIDEO_V4L2_SUBDEV_API
>> + help
>> + Support for the Video Input Block found in the RZ/V2H SoC. The IVC
> I would stick to a more canonical description
I agree, I was fleshing out to avoid checkstyle complaining about descriptions being too thin...but
yours should just pass
>
> Support for the Renesas RZ/V2H Input Video Control Block
> (IVC).
>
> To compile this driver as a module, choose M here: the
> module will be called rzv2h-ivc.
>
>> + block is used to read data from memory and forward it to the ISP that
>> + is integrated to the SoC. Enable this to support the block, and by
>> + extension the ISP to which it feeds data.
>
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/Makefile b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..080ee3570f09c236d87abeaea5d8dd578fefb6d3
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +rzv2h-ivc-y := rzv2h-ivc-dev.o rzv2h-ivc-subdev.o rzv2h-ivc-video.o
>> +
>> +obj-$(CONFIG_VIDEO_RZV2H_IVC) += rzv2h-ivc.o
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..841fa2d17df24ffe982a1d3237ca7c0491c290c4
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-dev.c
>> @@ -0,0 +1,228 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Renesas RZ/V2H Input Video Control Block driver
>> + *
>> + * Copyright (C) 2025 Ideas on Board Oy
>> + */
>> +
>> +#include "rzv2h-ivc.h"
>> +
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +inline void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val)
> I would not try to outsmart the compiler and let it decide what ot
> inline or not
Okedokey...not sure what got me into that habit, I'll check it on the C55 too.
>
>> +{
>> + writel(val, ivc->base + addr);
>> +}
>> +
>> +void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
>> + u32 mask, u32 val)
>> +{
>> + u32 orig, new;
>> +
>> + orig = readl(ivc->base + addr);
>> +
>> + new = orig & ~mask;
>> + new |= val & mask;
>> +
>> + if (new != orig)
>> + writel(new, ivc->base + addr);
>> +}
>> +
>> +static int rzv2h_ivc_get_hardware_resources(struct rzv2h_ivc *ivc,
>> + struct platform_device *pdev)
>> +{
>> + const char * const resource_names[RZV2H_IVC_NUM_HW_RESOURCES] = {
>> + "reg",
>> + "axi",
>> + "isp",
>> + };
>> + struct resource *res;
>> + int ret;
>> +
>> + ivc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> + if (IS_ERR(ivc->base))
>> + return dev_err_probe(ivc->dev, PTR_ERR(ivc->base),
>> + "failed to map IO memory\n");
>> +
>> + for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
>> + ivc->clks[i].id = resource_names[i];
>> +
>> + ret = devm_clk_bulk_get(ivc->dev, ARRAY_SIZE(resource_names), ivc->clks);
>> + if (ret)
>> + return dev_err_probe(ivc->dev, ret, "failed to acquire clks\n");
>> +
>> + for (unsigned int i = 0; i < ARRAY_SIZE(resource_names); i++)
>> + ivc->resets[i].id = resource_names[i];
>> +
>> + ret = devm_reset_control_bulk_get_optional_shared(
>> + ivc->dev, ARRAY_SIZE(resource_names), ivc->resets);
>> + if (ret)
>> + return dev_err_probe(ivc->dev, ret, "failed to acquire resets\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void rzv2h_ivc_global_config(struct rzv2h_ivc *ivc)
>> +{
>> + /* Currently we only support single-exposure input */
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PLNUM, RZV2H_IVC_ONE_EXPOSURE);
>> +
>> + /*
>> + * Datasheet says we should disable the interrupts before changing mode
>> + * to avoid spurious IFP interrupt.
>> + */
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, 0x0);
>> +
>> + /*
>> + * RZ/V2H documentation says software controlled configuration is not
>> + * supported, and currently neither is multi-context mode. That being so
>> + * we just set single context sw-hw mode.
> nit: the register documentation doesn't present multi-context SW-HW
> mode as prohibited
No indeed, I meant by that the Single Context SW-HW mode, but I'll clarify it here.
>
>> + */
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_CONTEXT,
>> + RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG);
>> +
>> + /*
>> + * We enable the frame end interrupt so that we know when we should send
>> + * follow-up frames.
>> + */
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_INT_EN, RZV2H_IVC_VVAL_IFPE);
>> +}
>> +
>> +static irqreturn_t rzv2h_ivc_isr(int irq, void *context)
>> +{
>> + struct device *dev = context;
>> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
>> +
>> + guard(spinlock)(&ivc->spinlock);
>> +
>> + if (!--ivc->vvalid_ifp)
>> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
> The driver doesn't depend or select CONFIG_PM, so this is rightfully
> marked as __maybe_unused.
>
> However, it doesn't seem to me that the probe() routine manually
> enable the peripheral, so in case of !CONFIG_PM am I wrong or the
> device won't operate at all ?
>
> I would select CONFIG_PM, or otherwise call this function from the probe()
> routine and then call pm_runtime_set_active() to inform runtime_pm
> that the peripheral is active, and at the end of the probe routine
> call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
> will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
> reduces to a nop leaving the peripheral enabled.
Ack
>
> I would just select CONFIG_PM tbh
I dropped it on Philipp's suggestion in the last review; I have no strong feelings to be honest, I
would expect it to be enabled in any configuration that was intending to use this...but I suppose
there's no harm accounting for the possibility that it won't be
>
>> +{
>> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + ret = request_irq(ivc->irqnum, rzv2h_ivc_isr, 0, dev_driver_string(dev),
>> + dev);
>> + if (ret) {
>> + dev_err(dev, "failed to request irq\n");
>> + return ret;
>> + }
>> +
>> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(ivc->clks), ivc->clks);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to enable clocks\n");
>> + goto err_free_irqnum;
>> + }
>> +
>> + ret = reset_control_bulk_deassert(ARRAY_SIZE(ivc->resets), ivc->resets);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to deassert resets\n");
>> + goto err_disable_clks;
>> + }
>> +
>> + rzv2h_ivc_global_config(ivc);
>> +
>> + return 0;
>> +
>> +err_disable_clks:
>> + clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
>> +err_free_irqnum:
>> + free_irq(ivc->irqnum, dev);
>> +
>> + return ret;
>> +}
>> +
>> +static int __maybe_unused rzv2h_ivc_runtime_suspend(struct device *dev)
>> +{
>> + struct rzv2h_ivc *ivc = dev_get_drvdata(dev);
>> +
>> + reset_control_bulk_assert(ARRAY_SIZE(ivc->resets), ivc->resets);
>> + clk_bulk_disable_unprepare(ARRAY_SIZE(ivc->clks), ivc->clks);
>> + free_irq(ivc->irqnum, dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops rzv2h_ivc_pm_ops = {
>> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> + pm_runtime_force_resume)
>> + SET_RUNTIME_PM_OPS(rzv2h_ivc_runtime_suspend, rzv2h_ivc_runtime_resume,
>> + NULL)
>> +};
>> +
>> +static int rzv2h_ivc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct rzv2h_ivc *ivc;
>> + int ret;
>> +
>> + ivc = devm_kzalloc(dev, sizeof(*ivc), GFP_KERNEL);
>> + if (!ivc)
>> + return -ENOMEM;
>> +
>> + ivc->dev = dev;
>> + platform_set_drvdata(pdev, ivc);
>> + mutex_init(&ivc->lock);
>> + spin_lock_init(&ivc->spinlock);
>> +
>> + ret = rzv2h_ivc_get_hardware_resources(ivc, pdev);
>> + if (ret)
>> + return ret;
>> +
>> + pm_runtime_set_autosuspend_delay(dev, 2000);
>> + pm_runtime_use_autosuspend(dev);
>> + pm_runtime_enable(dev);
>> +
>> + ivc->irqnum = platform_get_irq(pdev, 0);
>> + if (ivc->irqnum < 0) {
>> + dev_err(dev, "failed to get interrupt\n");
>> + return ret;
>> + }
>> +
>> + ret = rzv2h_ivc_initialise_subdevice(ivc);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static void rzv2h_ivc_remove(struct platform_device *pdev)
>> +{
>> + struct rzv2h_ivc *ivc = platform_get_drvdata(pdev);
>> +
>> + rzv2h_deinit_video_dev_and_queue(ivc);
>> + rzv2h_ivc_deinit_subdevice(ivc);
>> + mutex_destroy(&ivc->lock);
>> +}
>> +
>> +static const struct of_device_id rzv2h_ivc_of_match[] = {
>> + { .compatible = "renesas,r9a09g057-ivc", },
>> + { /* Sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, rzv2h_ivc_of_match);
>> +
>> +static struct platform_driver rzv2h_ivc_driver = {
>> + .driver = {
>> + .name = "rzv2h-ivc",
>> + .of_match_table = rzv2h_ivc_of_match,
>> + .pm = &rzv2h_ivc_pm_ops,
>> + },
>> + .probe = rzv2h_ivc_probe,
>> + .remove = rzv2h_ivc_remove,
>> +};
>> +
>> +module_platform_driver(rzv2h_ivc_driver);
>> +
>> +MODULE_AUTHOR("Daniel Scally <dan.scally@ideasonboard.com>");
>> +MODULE_DESCRIPTION("Renesas RZ/V2H Input Video Control Block driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..ff3eac313994b0c109004ca7ba960f7edf1d2112
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-subdev.c
>> @@ -0,0 +1,376 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Renesas RZ/V2H Input Video Control Block driver
>> + *
>> + * Copyright (C) 2025 Ideas on Board Oy
>> + */
>> +
>> +#include "rzv2h-ivc.h"
>> +
>> +#include <linux/media.h>
>> +#include <linux/media-bus-format.h>
>> +#include <linux/v4l2-mediabus.h>
>> +
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-dev.h>
>> +#include <media/v4l2-event.h>
>> +
>> +#define RZV2H_IVC_N_INPUTS_PER_OUTPUT 6
>> +
>> +/*
>> + * We support 8/10/12/14/16/20 bit input in any bayer order, but the output
>> + * format is fixed at 20-bits with the same order as the input.
>> + */
>> +static const struct {
>> + u32 inputs[RZV2H_IVC_N_INPUTS_PER_OUTPUT];
>> + u32 output;
>> +} rzv2h_ivc_formats[] = {
>> + {
>> + .inputs = {
>> + MEDIA_BUS_FMT_SBGGR8_1X8,
>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>> + MEDIA_BUS_FMT_SBGGR12_1X12,
>> + MEDIA_BUS_FMT_SBGGR14_1X14,
>> + MEDIA_BUS_FMT_SBGGR16_1X16,
>> + MEDIA_BUS_FMT_SBGGR20_1X20,
>> + },
>> + .output = MEDIA_BUS_FMT_SBGGR20_1X20
>> + },
>> + {
>> + .inputs = {
>> + MEDIA_BUS_FMT_SGBRG8_1X8,
>> + MEDIA_BUS_FMT_SGBRG10_1X10,
>> + MEDIA_BUS_FMT_SGBRG12_1X12,
>> + MEDIA_BUS_FMT_SGBRG14_1X14,
>> + MEDIA_BUS_FMT_SGBRG16_1X16,
>> + MEDIA_BUS_FMT_SGBRG20_1X20,
>> + },
>> + .output = MEDIA_BUS_FMT_SGBRG20_1X20
>> + },
>> + {
>> + .inputs = {
>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>> + MEDIA_BUS_FMT_SGRBG14_1X14,
>> + MEDIA_BUS_FMT_SGRBG16_1X16,
>> + MEDIA_BUS_FMT_SGRBG20_1X20,
>> + },
>> + .output = MEDIA_BUS_FMT_SGRBG20_1X20
>> + },
>> + {
>> + .inputs = {
>> + MEDIA_BUS_FMT_SRGGB8_1X8,
>> + MEDIA_BUS_FMT_SRGGB10_1X10,
>> + MEDIA_BUS_FMT_SRGGB12_1X12,
>> + MEDIA_BUS_FMT_SRGGB14_1X14,
>> + MEDIA_BUS_FMT_SRGGB16_1X16,
>> + MEDIA_BUS_FMT_SRGGB20_1X20,
>> + },
>> + .output = MEDIA_BUS_FMT_SRGGB20_1X20
>> + },
>> +};
>> +
>> +static u32 rzv2h_ivc_get_mbus_output_from_input(u32 mbus_code)
>> +{
>> + unsigned int i, j;
>> +
>> + for (i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++) {
>> + for (j = 0; j < RZV2H_IVC_N_INPUTS_PER_OUTPUT; j++) {
>> + if (rzv2h_ivc_formats[i].inputs[j] == mbus_code)
>> + return rzv2h_ivc_formats[i].output;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + const struct v4l2_mbus_framefmt *fmt;
>> + unsigned int order_index;
>> + unsigned int index;
>> +
>> + /*
>> + * On the source pad, only the 20-bit format corresponding to the sink
>> + * pad format's bayer order is supported.
>> + */
>> + if (code->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
>> + if (code->index)
>> + return -EINVAL;
>> +
>> + fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SINK_PAD);
>> + code->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code);
>> +
>> + return 0;
>> + }
>> +
>> + if (code->index >= ARRAY_SIZE(rzv2h_ivc_formats) *
>> + RZV2H_IVC_N_INPUTS_PER_OUTPUT)
>> + return -EINVAL;
>> +
>> + order_index = code->index / RZV2H_IVC_N_INPUTS_PER_OUTPUT;
>> + index = code->index % RZV2H_IVC_N_INPUTS_PER_OUTPUT;
>> +
>> + code->code = rzv2h_ivc_formats[order_index].inputs[index];
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_enum_frame_size(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + const struct v4l2_mbus_framefmt *fmt;
>> +
>> + if (fse->index > 0)
>> + return -EINVAL;
>> +
>> + if (fse->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD) {
>> + fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SINK_PAD);
>> +
>> + if (fse->code != fmt->code)
>> + return -EINVAL;
>> +
>> + fse->min_width = fmt->width;
>> + fse->max_width = fmt->width;
>> + fse->min_height = fmt->height;
>> + fse->max_height = fmt->height;
>> +
>> + return 0;
>> + }
>> +
>> + if (!rzv2h_ivc_get_mbus_output_from_input(fse->code))
>> + return -EINVAL;
>> +
>> + fse->min_width = RZV2H_IVC_MIN_WIDTH;
>> + fse->max_width = RZV2H_IVC_MAX_WIDTH;
>> + fse->min_height = RZV2H_IVC_MIN_HEIGHT;
>> + fse->max_height = RZV2H_IVC_MAX_HEIGHT;
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_set_fmt(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state,
>> + struct v4l2_subdev_format *format)
>> +{
>> + struct v4l2_mbus_framefmt *fmt = &format->format;
>> + struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
>> +
>> + if (format->pad == RZV2H_IVC_SUBDEV_SOURCE_PAD)
>> + return v4l2_subdev_get_fmt(sd, state, format);
>> +
>> + sink_fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SINK_PAD);
>> +
>> + sink_fmt->code = rzv2h_ivc_get_mbus_output_from_input(fmt->code) ?
>> + fmt->code : rzv2h_ivc_formats[0].inputs[0];
>> +
>> + sink_fmt->width = clamp(fmt->width, RZV2H_IVC_MIN_WIDTH,
>> + RZV2H_IVC_MAX_WIDTH);
>> + sink_fmt->height = clamp(fmt->height, RZV2H_IVC_MIN_HEIGHT,
>> + RZV2H_IVC_MAX_HEIGHT);
>> +
>> + *fmt = *sink_fmt;
>> +
>> + src_fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SOURCE_PAD);
>> + *src_fmt = *sink_fmt;
>> + src_fmt->code = rzv2h_ivc_get_mbus_output_from_input(sink_fmt->code);
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_enable_streams(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state, u32 pad,
>> + u64 streams_mask)
>> +{
>> + /*
>> + * We have a single source pad, which has a single stream. V4L2 core has
>> + * already validated those things. The actual power-on and programming
>> + * of registers will be done through the video device's .vidioc_streamon
>> + * so there's nothing to actually do here...
>> + */
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_disable_streams(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state, u32 pad,
>> + u64 streams_mask)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_pad_ops rzv2h_ivc_pad_ops = {
>> + .enum_mbus_code = rzv2h_ivc_enum_mbus_code,
>> + .enum_frame_size = rzv2h_ivc_enum_frame_size,
>> + .get_fmt = v4l2_subdev_get_fmt,
>> + .set_fmt = rzv2h_ivc_set_fmt,
>> + .enable_streams = rzv2h_ivc_enable_streams,
>> + .disable_streams = rzv2h_ivc_disable_streams,
>> +};
>> +
>> +static const struct v4l2_subdev_core_ops rzv2h_ivc_core_ops = {
>> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>> +};
>> +
>> +static const struct v4l2_subdev_ops rzv2h_ivc_subdev_ops = {
>> + .core = &rzv2h_ivc_core_ops,
>> + .pad = &rzv2h_ivc_pad_ops,
>> +};
>> +
>> +static int rzv2h_ivc_init_state(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *state)
>> +{
>> + struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
>> +
>> + sink_fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SINK_PAD);
>> + sink_fmt->width = RZV2H_IVC_DEFAULT_WIDTH;
>> + sink_fmt->height = RZV2H_IVC_DEFAULT_HEIGHT;
>> + sink_fmt->field = V4L2_FIELD_NONE;
>> + sink_fmt->code = MEDIA_BUS_FMT_SRGGB16_1X16;
>> + sink_fmt->colorspace = V4L2_COLORSPACE_RAW;
>> + sink_fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(sink_fmt->colorspace);
>> + sink_fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(sink_fmt->colorspace);
>> + sink_fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(
>> + true, sink_fmt->colorspace, sink_fmt->ycbcr_enc);
>> +
>> + src_fmt = v4l2_subdev_state_get_format(state,
>> + RZV2H_IVC_SUBDEV_SOURCE_PAD);
>> +
>> + *src_fmt = *sink_fmt;
>> + src_fmt->code = MEDIA_BUS_FMT_SRGGB20_1X20;
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_registered(struct v4l2_subdev *sd)
>> +{
>> + struct rzv2h_ivc *ivc = container_of(sd, struct rzv2h_ivc, subdev.sd);
>> +
>> + return rzv2h_ivc_init_vdev(ivc, sd->v4l2_dev);
>> +}
>> +
>> +static const struct v4l2_subdev_internal_ops rzv2h_ivc_subdev_internal_ops = {
>> + .init_state = rzv2h_ivc_init_state,
>> + .registered = rzv2h_ivc_registered,
>> +};
>> +
>> +static int rzv2h_ivc_link_validate(struct media_link *link)
>> +{
>> + struct video_device *vdev =
>> + media_entity_to_video_device(link->source->entity);
>> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
>> + struct v4l2_subdev *sd =
>> + media_entity_to_v4l2_subdev(link->sink->entity);
>> + const struct rzv2h_ivc_format *fmt;
>> + const struct v4l2_pix_format_mplane *pix;
>> + struct v4l2_subdev_state *state;
>> + struct v4l2_mbus_framefmt *mf;
>> + unsigned int i;
>> + int ret = 0;
>> +
>> + state = v4l2_subdev_lock_and_get_active_state(sd);
>> + mf = v4l2_subdev_state_get_format(state, link->sink->index);
>> +
>> + pix = &ivc->format.pix;
>> + fmt = ivc->format.fmt;
>> +
>> + if (mf->width != pix->width || mf->height != pix->height) {
>> + dev_dbg(ivc->dev,
>> + "link '%s':%u -> '%s':%u not valid: %ux%u != %ux%u\n",
>> + link->source->entity->name, link->source->index,
>> + link->sink->entity->name, link->sink->index,
>> + mf->width, mf->height,
>> + pix->width, pix->height);
>> + ret = -EPIPE;
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(fmt->mbus_codes); i++)
>> + if (mf->code == fmt->mbus_codes[i])
>> + break;
>> +
>> + if (i == ARRAY_SIZE(fmt->mbus_codes)) {
>> + dev_dbg(ivc->dev,
>> + "link '%s':%u -> '%s':%u not valid: pixel format %p4cc cannot produce mbus_code 0x%04x\n",
>> + link->source->entity->name, link->source->index,
>> + link->sink->entity->name, link->sink->index,
>> + &pix->pixelformat, mf->code);
>> + ret = -EPIPE;
>> + }
>> +
>> + v4l2_subdev_unlock_state(state);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct media_entity_operations rzv2h_ivc_media_ops = {
>> + .link_validate = rzv2h_ivc_link_validate,
>> +};
>> +
>> +int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc)
>> +{
>> + struct v4l2_subdev *sd;
>> + int ret;
>> +
>> + /* Initialise subdevice */
>> + sd = &ivc->subdev.sd;
>> + sd->dev = ivc->dev;
>> + v4l2_subdev_init(sd, &rzv2h_ivc_subdev_ops);
>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>> + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>> + sd->internal_ops = &rzv2h_ivc_subdev_internal_ops;
>> + sd->entity.ops = &rzv2h_ivc_media_ops;
>> +
>> + ivc->subdev.pads[RZV2H_IVC_SUBDEV_SINK_PAD].flags = MEDIA_PAD_FL_SINK;
>> + ivc->subdev.pads[RZV2H_IVC_SUBDEV_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> + snprintf(sd->name, sizeof(sd->name), "rzv2h ivc block");
>> +
>> + ret = media_entity_pads_init(&sd->entity, RZV2H_IVC_NUM_SUBDEV_PADS,
>> + ivc->subdev.pads);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to initialise media entity\n");
>> + return ret;
>> + }
>> +
>> + ret = v4l2_subdev_init_finalize(sd);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to finalize subdev init\n");
>> + goto err_cleanup_subdev_entity;
>> + }
>> +
>> + ret = v4l2_async_register_subdev(sd);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to register subdevice\n");
>> + goto err_cleanup_subdev;
>> + }
>> +
>> + return 0;
>> +
>> +err_cleanup_subdev:
>> + v4l2_subdev_cleanup(sd);
>> +err_cleanup_subdev_entity:
>> + media_entity_cleanup(&sd->entity);
>> +
>> + return ret;
>> +}
>> +
>> +void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc)
>> +{
>> + struct v4l2_subdev *sd = &ivc->subdev.sd;
>> +
>> + v4l2_subdev_cleanup(sd);
>> + media_entity_remove_links(&sd->entity);
>> + v4l2_async_unregister_subdev(sd);
>> + media_entity_cleanup(&sd->entity);
>> +}
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..63e20b83d596aff8a9534720b70fe75aa6c0c0ae
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc-video.c
>> @@ -0,0 +1,568 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Renesas RZ/V2H Input Video Control Block driver
>> + *
>> + * Copyright (C) 2025 Ideas on Board Oy
>> + */
>> +
>> +#include "rzv2h-ivc.h"
>> +
>> +#include <linux/cleanup.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/media-bus-format.h>
>> +#include <linux/minmax.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include <media/mipi-csi2.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-dev.h>
>> +#include <media/v4l2-event.h>
>> +#include <media/v4l2-fh.h>
>> +#include <media/v4l2-ioctl.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +
>> +#define RZV2H_IVC_FIXED_HBLANK 0x20
>> +#define RZV2H_IVC_MIN_VBLANK(hts) max(0x1b, 15 + (120501 / (hts)))
>> +
>> +struct rzv2h_ivc_buf {
>> + struct vb2_v4l2_buffer vb;
>> + struct list_head queue;
>> + dma_addr_t addr;
>> +};
>> +
>> +#define to_rzv2h_ivc_buf(vbuf) \
>> + container_of(vbuf, struct rzv2h_ivc_buf, vb)
>> +
>> +static const struct rzv2h_ivc_format rzv2h_ivc_formats[] = {
>> + {
>> + .fourcc = V4L2_PIX_FMT_SBGGR8,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SBGGR8_1X8,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW8,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SGBRG8,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SGBRG8_1X8,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW8,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SGRBG8,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW8,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SRGGB8,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SRGGB8_1X8,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW8,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_RAW_CRU10,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>> + MEDIA_BUS_FMT_SGBRG10_1X10,
>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>> + MEDIA_BUS_FMT_SRGGB10_1X10
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW10,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_RAW_CRU12,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SBGGR12_1X12,
>> + MEDIA_BUS_FMT_SGBRG12_1X12,
>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>> + MEDIA_BUS_FMT_SRGGB12_1X12
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW12,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_RAW_CRU14,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SBGGR14_1X14,
>> + MEDIA_BUS_FMT_SGBRG14_1X14,
>> + MEDIA_BUS_FMT_SGRBG14_1X14,
>> + MEDIA_BUS_FMT_SRGGB14_1X14
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW14,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SBGGR16,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SBGGR16_1X16,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW16,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SGBRG16,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SGBRG16_1X16,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW16,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SGRBG16,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SGRBG16_1X16,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW16,
>> + },
>> + {
>> + .fourcc = V4L2_PIX_FMT_SRGGB16,
>> + .mbus_codes = {
>> + MEDIA_BUS_FMT_SRGGB16_1X16,
>> + },
>> + .dtype = MIPI_CSI2_DT_RAW16,
>> + },
>> +};
>> +
>> +static void rzv2h_ivc_transfer_buffer(struct work_struct *work)
>> +{
>> + struct rzv2h_ivc *ivc = container_of(work, struct rzv2h_ivc,
>> + buffers.work);
>> + struct rzv2h_ivc_buf *buf;
>> +
>> + scoped_guard(spinlock, &ivc->buffers.lock) {
>> + if (ivc->buffers.curr) {
>> + ivc->buffers.curr->vb.sequence = ivc->buffers.sequence++;
>> + vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf,
>> + VB2_BUF_STATE_DONE);
>> + ivc->buffers.curr = NULL;
>> + }
>> +
>> + buf = list_first_entry_or_null(&ivc->buffers.queue,
>> + struct rzv2h_ivc_buf, queue);
>> + }
>> +
>> + if (buf)
>> + list_del(&buf->queue);
>> + else
>> + return;
> if (!buf)
> return;
>
> list_del(&buf->queue);
Hah; of course. Much cleaner, thanks
>> +
>> + ivc->buffers.curr = buf;
>> + buf->addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_SADDL_P0, buf->addr);
>> +
>> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
>> + ivc->vvalid_ifp = 2;
>> + }
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_FRCON, 0x1);
>> +}
>> +
>> +static int rzv2h_ivc_pipeline_started(struct media_entity *entity)
>> +{
>> + struct video_device *vdev = media_entity_to_video_device(entity);
>> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
>> +
>> + guard(spinlock)(&ivc->buffers.lock);
>> +
>> + if (list_empty(&ivc->buffers.queue)) {
>> + /*
>> + * The driver waits for interrupts to send a new frame and
>> + * tracks their receipt in the vvalid_ifp variable. .buf_queue()
>> + * will queue work if vvalid_ifp == 0 to trigger a new frame (an
>> + * event that normally would only occur if no buffer was ready
>> + * when the interrupt arrived). If there are no buffers in the
>> + * queue yet, we set vvalid_ifp to zero so that the next queue
>> + * will trigger the work.
>> + */
>> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
>> + ivc->vvalid_ifp = 0;
>> + }
> return 0;
> }
>
>> + } else {
> and drop the else branch
>
> I'm not 100% sure I got this part, will follow-up offline to get some
> clarification
Okedokey
>> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void rzv2h_ivc_pipeline_stopped(struct media_entity *entity)
>> +{
>> + struct video_device *vdev = media_entity_to_video_device(entity);
>> + struct rzv2h_ivc *ivc = video_get_drvdata(vdev);
>> + u32 val = 0;
>> +
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_FM_STOP, 0x1);
>> + readl_poll_timeout(ivc->base + RZV2H_IVC_REG_FM_STOP,
>> + val, !val, 10 * USEC_PER_MSEC, 250 * USEC_PER_MSEC);
>> +}
>> +
>> +static const struct media_entity_operations rzv2h_ivc_media_ops = {
>> + .pipeline_started = rzv2h_ivc_pipeline_started,
>> + .pipeline_stopped = rzv2h_ivc_pipeline_stopped,
>> +};
>> +
>> +static int rzv2h_ivc_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
>> + unsigned int *num_planes, unsigned int sizes[],
>> + struct device *alloc_devs[])
>> +{
>> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>> +
>> + if (*num_planes && *num_planes > 1)
>> + return -EINVAL;
>> +
>> + if (sizes[0] && sizes[0] < ivc->format.pix.plane_fmt[0].sizeimage)
>> + return -EINVAL;
>> +
>> + *num_planes = 1;
>> +
>> + if (!sizes[0])
>> + sizes[0] = ivc->format.pix.plane_fmt[0].sizeimage;
>> +
>> + return 0;
>> +}
>> +
>> +static void rzv2h_ivc_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct rzv2h_ivc_buf *buf = to_rzv2h_ivc_buf(vbuf);
>> +
>> + scoped_guard(spinlock, &ivc->buffers.lock) {
>> + list_add_tail(&buf->queue, &ivc->buffers.queue);
>> + }
>> +
>> + scoped_guard(spinlock_irqsave, &ivc->spinlock) {
>> + if (vb2_is_streaming(vb->vb2_queue) && !ivc->vvalid_ifp)
>> + queue_work(ivc->buffers.async_wq, &ivc->buffers.work);
>> + }
>> +}
>> +
>> +static void rzv2h_ivc_format_configure(struct rzv2h_ivc *ivc)
>> +{
>> + const struct rzv2h_ivc_format *fmt = ivc->format.fmt;
>> + struct v4l2_pix_format_mplane *pix = &ivc->format.pix;
>> + unsigned int vblank;
>> + unsigned int hts;
>> +
>> + /* Currently only CRU packed pixel formats are supported */
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
>> + RZV2H_IVC_INPUT_FMT_CRU_PACKED);
>> +
>> + rzv2h_ivc_update_bits(ivc, RZV2H_IVC_REG_AXIRX_PXFMT,
>> + RZV2H_IVC_PXFMT_DTYPE, fmt->dtype);
>> +
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_HSIZE, pix->width);
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_VSIZE, pix->height);
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_STRD,
>> + pix->plane_fmt[0].bytesperline);
>> +
>> + /*
>> + * The ISP has minimum vertical blanking requirements that must be
>> + * adhered to by the IVC. The minimum is a function of the Iridix blocks
>> + * clocking requirements and the width of the image and horizontal
>> + * blanking, but if we assume the worst case then it boils down to the
>> + * below (plus one to the numerator to ensure the answer is rounded up)
>> + */
>> +
>> + hts = pix->width + RZV2H_IVC_FIXED_HBLANK;
>> + vblank = RZV2H_IVC_MIN_VBLANK(hts);
>> +
>> + rzv2h_ivc_write(ivc, RZV2H_IVC_REG_AXIRX_BLANK,
>> + RZV2H_IVC_VBLANK(vblank));
>> +}
>> +
>> +static void rzv2h_ivc_return_buffers(struct rzv2h_ivc *ivc,
>> + enum vb2_buffer_state state)
>> +{
>> + struct rzv2h_ivc_buf *buf, *tmp;
>> +
>> + guard(spinlock)(&ivc->buffers.lock);
>> +
>> + if (ivc->buffers.curr) {
>> + vb2_buffer_done(&ivc->buffers.curr->vb.vb2_buf, state);
>> + ivc->buffers.curr = NULL;
>> + }
>> +
>> + list_for_each_entry_safe(buf, tmp, &ivc->buffers.queue, queue) {
>> + list_del(&buf->queue);
>> + vb2_buffer_done(&buf->vb.vb2_buf, state);
>> + }
>> +}
>> +
>> +static int rzv2h_ivc_start_streaming(struct vb2_queue *q, unsigned int count)
>> +{
>> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>> + int ret;
>> +
>> + ivc->buffers.sequence = 0;
>> + ivc->vvalid_ifp = 2;
>> +
>> + ret = pm_runtime_resume_and_get(ivc->dev);
>> + if (ret)
>> + goto err_return_buffers;
>> +
>> + ret = video_device_pipeline_alloc_start(&ivc->vdev.dev);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to start media pipeline\n");
>> + goto err_pm_runtime_put;
>> + }
>> +
>> + rzv2h_ivc_format_configure(ivc);
>> +
>> + ret = video_device_pipeline_started(&ivc->vdev.dev);
>> + if (ret < 0)
>> + goto err_stop_pipeline;
>> +
>> + return 0;
>> +
>> +err_stop_pipeline:
>> + video_device_pipeline_stop(&ivc->vdev.dev);
>> +err_pm_runtime_put:
>> + pm_runtime_put(ivc->dev);
>> +err_return_buffers:
>> + rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_QUEUED);
>> +
>> + return ret;
>> +}
>> +
>> +static void rzv2h_ivc_stop_streaming(struct vb2_queue *q)
>> +{
>> + struct rzv2h_ivc *ivc = vb2_get_drv_priv(q);
>> +
>> + video_device_pipeline_stopped(&ivc->vdev.dev);
>> + rzv2h_ivc_return_buffers(ivc, VB2_BUF_STATE_ERROR);
>> + video_device_pipeline_stop(&ivc->vdev.dev);
>> + pm_runtime_mark_last_busy(ivc->dev);
>> + pm_runtime_put_autosuspend(ivc->dev);
>> +}
>> +
>> +static const struct vb2_ops rzv2h_ivc_vb2_ops = {
>> + .queue_setup = &rzv2h_ivc_queue_setup,
>> + .buf_queue = &rzv2h_ivc_buf_queue,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> + .start_streaming = &rzv2h_ivc_start_streaming,
>> + .stop_streaming = &rzv2h_ivc_stop_streaming,
>> +};
>> +
>> +static const struct rzv2h_ivc_format *
>> +rzv2h_ivc_format_from_pixelformat(u32 fourcc)
>> +{
>> + for (unsigned int i = 0; i < ARRAY_SIZE(rzv2h_ivc_formats); i++)
>> + if (fourcc == rzv2h_ivc_formats[i].fourcc)
>> + return &rzv2h_ivc_formats[i];
>> +
>> + return &rzv2h_ivc_formats[0];
>> +}
>> +
>> +static int rzv2h_ivc_enum_fmt_vid_out(struct file *file, void *fh,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + if (f->index >= ARRAY_SIZE(rzv2h_ivc_formats))
>> + return -EINVAL;
>> +
>> + f->pixelformat = rzv2h_ivc_formats[f->index].fourcc;
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_g_fmt_vid_out(struct file *file, void *fh,
>> + struct v4l2_format *f)
>> +{
>> + struct rzv2h_ivc *ivc = video_drvdata(file);
>> +
>> + f->fmt.pix_mp = ivc->format.pix;
>> +
>> + return 0;
>> +}
>> +
>> +static void rzv2h_ivc_try_fmt(struct v4l2_pix_format_mplane *pix,
>> + const struct rzv2h_ivc_format *fmt)
>> +{
>> + pix->pixelformat = fmt->fourcc;
>> +
>> + pix->width = clamp(pix->width, RZV2H_IVC_MIN_WIDTH,
>> + RZV2H_IVC_MAX_WIDTH);
>> + pix->height = clamp(pix->height, RZV2H_IVC_MIN_HEIGHT,
>> + RZV2H_IVC_MAX_HEIGHT);
>> +
>> + pix->field = V4L2_FIELD_NONE;
>> + pix->colorspace = V4L2_COLORSPACE_RAW;
>> + pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
>> + pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
>> + pix->colorspace,
>> + pix->ycbcr_enc);
>> +
>> + v4l2_fill_pixfmt_mp(pix, pix->pixelformat, pix->width, pix->height);
>> +}
>> +
>> +static void rzv2h_ivc_set_format(struct rzv2h_ivc *ivc,
>> + struct v4l2_pix_format_mplane *pix)
>> +{
>> + const struct rzv2h_ivc_format *fmt;
>> +
>> + fmt = rzv2h_ivc_format_from_pixelformat(pix->pixelformat);
>> +
>> + rzv2h_ivc_try_fmt(pix, fmt);
>> + ivc->format.pix = *pix;
>> + ivc->format.fmt = fmt;
>> +}
>> +
>> +static int rzv2h_ivc_s_fmt_vid_out(struct file *file, void *fh,
>> + struct v4l2_format *f)
>> +{
>> + struct rzv2h_ivc *ivc = video_drvdata(file);
>> + struct v4l2_pix_format_mplane *pix = &f->fmt.pix_mp;
>> +
>> + if (vb2_is_busy(&ivc->vdev.vb2q))
>> + return -EBUSY;
>> +
>> + rzv2h_ivc_set_format(ivc, pix);
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_try_fmt_vid_out(struct file *file, void *fh,
>> + struct v4l2_format *f)
>> +{
>> + const struct rzv2h_ivc_format *fmt;
>> +
>> + fmt = rzv2h_ivc_format_from_pixelformat(f->fmt.pix.pixelformat);
>> + rzv2h_ivc_try_fmt(&f->fmt.pix_mp, fmt);
>> +
>> + return 0;
>> +}
>> +
>> +static int rzv2h_ivc_querycap(struct file *file, void *fh,
>> + struct v4l2_capability *cap)
>> +{
>> + strscpy(cap->driver, "rzv2h-ivc", sizeof(cap->driver));
>> + strscpy(cap->card, "Renesas Input Video Control", sizeof(cap->card));
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_ioctl_ops rzv2h_ivc_v4l2_ioctl_ops = {
>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>> + .vidioc_expbuf = vb2_ioctl_expbuf,
>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>> + .vidioc_streamon = vb2_ioctl_streamon,
>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>> + .vidioc_enum_fmt_vid_out = rzv2h_ivc_enum_fmt_vid_out,
>> + .vidioc_g_fmt_vid_out_mplane = rzv2h_ivc_g_fmt_vid_out,
>> + .vidioc_s_fmt_vid_out_mplane = rzv2h_ivc_s_fmt_vid_out,
>> + .vidioc_try_fmt_vid_out_mplane = rzv2h_ivc_try_fmt_vid_out,
>> + .vidioc_querycap = rzv2h_ivc_querycap,
>> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>> +};
>> +
>> +static const struct v4l2_file_operations rzv2h_ivc_v4l2_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = video_ioctl2,
>> + .open = v4l2_fh_open,
>> + .release = vb2_fop_release,
>> + .poll = vb2_fop_poll,
>> + .mmap = vb2_fop_mmap,
>> +};
>> +
>> +int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev)
>> +{
>> + struct v4l2_pix_format_mplane pix = { };
>> + struct video_device *vdev;
>> + struct vb2_queue *vb2q;
>> + int ret;
>> +
>> + spin_lock_init(&ivc->buffers.lock);
>> + INIT_LIST_HEAD(&ivc->buffers.queue);
>> + INIT_WORK(&ivc->buffers.work, rzv2h_ivc_transfer_buffer);
>> +
>> + ivc->buffers.async_wq = alloc_workqueue("rzv2h-ivc", 0, 0);
>> + if (!ivc->buffers.async_wq)
>> + return -EINVAL;
>> +
>> + /* Initialise vb2 queue */
>> + vb2q = &ivc->vdev.vb2q;
>> + vb2q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>> + vb2q->io_modes = VB2_MMAP | VB2_DMABUF;
>> + vb2q->drv_priv = ivc;
>> + vb2q->mem_ops = &vb2_dma_contig_memops;
>> + vb2q->ops = &rzv2h_ivc_vb2_ops;
>> + vb2q->buf_struct_size = sizeof(struct rzv2h_ivc_buf);
>> + vb2q->min_queued_buffers = 0;
>> + vb2q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + vb2q->lock = &ivc->lock;
>> + vb2q->dev = ivc->dev;
>> +
>> + ret = vb2_queue_init(vb2q);
>> + if (ret) {
>> + dev_err(ivc->dev, "vb2 queue init failed\n");
>> + goto err_destroy_workqueue;
>> + }
>> +
>> + /* Initialise Video Device */
>> + vdev = &ivc->vdev.dev;
>> + strscpy(vdev->name, "rzv2h-ivc", sizeof(vdev->name));
>> + vdev->release = video_device_release_empty;
>> + vdev->fops = &rzv2h_ivc_v4l2_fops;
>> + vdev->ioctl_ops = &rzv2h_ivc_v4l2_ioctl_ops;
>> + vdev->lock = &ivc->lock;
>> + vdev->v4l2_dev = v4l2_dev;
>> + vdev->queue = vb2q;
>> + vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_STREAMING;
>> + vdev->vfl_dir = VFL_DIR_TX;
>> + video_set_drvdata(vdev, ivc);
>> +
>> + pix.pixelformat = V4L2_PIX_FMT_SRGGB16;
>> + pix.width = RZV2H_IVC_DEFAULT_WIDTH;
>> + pix.height = RZV2H_IVC_DEFAULT_HEIGHT;
>> + rzv2h_ivc_set_format(ivc, &pix);
>> +
>> + ivc->vdev.pad.flags = MEDIA_PAD_FL_SOURCE;
>> + ivc->vdev.dev.entity.ops = &rzv2h_ivc_media_ops;
>> + ret = media_entity_pads_init(&ivc->vdev.dev.entity, 1, &ivc->vdev.pad);
>> + if (ret)
>> + goto err_release_vb2q;
>> +
>> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to register IVC video device\n");
>> + goto err_cleanup_vdev_entity;
>> + }
>> +
>> + ret = media_create_pad_link(&vdev->entity, 0, &ivc->subdev.sd.entity,
>> + RZV2H_IVC_SUBDEV_SINK_PAD,
>> + MEDIA_LNK_FL_ENABLED |
>> + MEDIA_LNK_FL_IMMUTABLE);
>> + if (ret) {
>> + dev_err(ivc->dev, "failed to create media link\n");
>> + goto err_unregister_vdev;
>> + }
>> +
>> + return 0;
>> +
>> +err_unregister_vdev:
>> + video_unregister_device(vdev);
>> +err_cleanup_vdev_entity:
>> + media_entity_cleanup(&vdev->entity);
>> +err_release_vb2q:
>> + vb2_queue_release(vb2q);
>> +err_destroy_workqueue:
>> + destroy_workqueue(ivc->buffers.async_wq);
>> +
>> + return ret;
>> +}
>> +
>> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc)
>> +{
>> + struct video_device *vdev = &ivc->vdev.dev;
>> + struct vb2_queue *vb2q = &ivc->vdev.vb2q;
>> +
>> + if (!ivc->sched)
>> + return;
>> +
>> + vb2_video_unregister_device(vdev);
>> + media_entity_cleanup(&vdev->entity);
>> + vb2_queue_release(vb2q);
>> +}
>> diff --git a/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..709c6a9398fe2484c2acb03d443d58ea4e153a66
>> --- /dev/null
>> +++ b/drivers/media/platform/renesas/rzv2h-ivc/rzv2h-ivc.h
>> @@ -0,0 +1,131 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Renesas RZ/V2H Input Video Control Block driver
>> + *
>> + * Copyright (C) 2025 Ideas on Board Oy
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/list.h>
>> +#include <linux/mutex.h>
>> +#include <linux/reset.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +#include <linux/videodev2.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include <media/media-entity.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/videobuf2-core.h>
>> +#include <media/videobuf2-v4l2.h>
>> +
>> +#define RZV2H_IVC_REG_AXIRX_PLNUM 0x0000
>> +#define RZV2H_IVC_ONE_EXPOSURE 0x00
>> +#define RZV2H_IVC_TWO_EXPOSURE 0x01
>> +#define RZV2H_IVC_REG_AXIRX_PXFMT 0x0004
>> +#define RZV2H_IVC_INPUT_FMT_MIPI (0 << 16)
>> +#define RZV2H_IVC_INPUT_FMT_CRU_PACKED (1 << 16)
>> +#define RZV2H_IVC_PXFMT_DTYPE GENMASK(7, 0)
>> +#define RZV2H_IVC_REG_AXIRX_SADDL_P0 0x0010
>> +#define RZV2H_IVC_REG_AXIRX_SADDH_P0 0x0014
>> +#define RZV2H_IVC_REG_AXIRX_SADDL_P1 0x0018
>> +#define RZV2H_IVC_REG_AXIRX_SADDH_P1 0x001c
>> +#define RZV2H_IVC_REG_AXIRX_HSIZE 0x0020
>> +#define RZV2H_IVC_REG_AXIRX_VSIZE 0x0024
>> +#define RZV2H_IVC_REG_AXIRX_BLANK 0x0028
>> +#define RZV2H_IVC_VBLANK(x) ((x) << 16)
>> +#define RZV2H_IVC_REG_AXIRX_STRD 0x0030
>> +#define RZV2H_IVC_REG_AXIRX_ISSU 0x0040
>> +#define RZV2H_IVC_REG_AXIRX_ERACT 0x0048
>> +#define RZV2H_IVC_REG_FM_CONTEXT 0x0100
>> +#define RZV2H_IVC_SOFTWARE_CFG 0x00
>> +#define RZV2H_IVC_SINGLE_CONTEXT_SW_HW_CFG BIT(0)
>> +#define RZV2H_IVC_MULTI_CONTEXT_SW_HW_CFG BIT(1)
>> +#define RZV2H_IVC_REG_FM_MCON 0x0104
>> +#define RZV2H_IVC_REG_FM_FRCON 0x0108
>> +#define RZV2H_IVC_REG_FM_STOP 0x010c
>> +#define RZV2H_IVC_REG_FM_INT_EN 0x0120
>> +#define RZV2H_IVC_VVAL_IFPE BIT(0)
>> +#define RZV2H_IVC_REG_FM_INT_STA 0x0124
>> +#define RZV2H_IVC_REG_AXIRX_FIFOCAP0 0x0208
>> +#define RZV2H_IVC_REG_CORE_CAPCON 0x020c
>> +#define RZV2H_IVC_REG_CORE_FIFOCAP0 0x0228
>> +#define RZV2H_IVC_REG_CORE_FIFOCAP1 0x022c
>> +
>> +#define RZV2H_IVC_MIN_WIDTH 640
>> +#define RZV2H_IVC_MAX_WIDTH 4096
>> +#define RZV2H_IVC_MIN_HEIGHT 480
>> +#define RZV2H_IVC_MAX_HEIGHT 4096
>> +#define RZV2H_IVC_DEFAULT_WIDTH 1920
>> +#define RZV2H_IVC_DEFAULT_HEIGHT 1080
>> +
>> +#define RZV2H_IVC_NUM_HW_RESOURCES 3
>> +
>> +struct device;
>> +
>> +enum rzv2h_ivc_subdev_pads {
>> + RZV2H_IVC_SUBDEV_SINK_PAD,
>> + RZV2H_IVC_SUBDEV_SOURCE_PAD,
>> + RZV2H_IVC_NUM_SUBDEV_PADS
>> +};
>> +
>> +struct rzv2h_ivc_format {
>> + u32 fourcc;
>> + /*
>> + * The CRU packed pixel formats are bayer-order agnostic, so each could
>> + * support any one of the 4 possible media bus formats.
>> + */
>> + u32 mbus_codes[4];
>> + u8 dtype;
>> +};
>> +
>> +struct rzv2h_ivc {
>> + struct device *dev;
>> + void __iomem *base;
>> + struct clk_bulk_data clks[RZV2H_IVC_NUM_HW_RESOURCES];
>> + struct reset_control_bulk_data resets[RZV2H_IVC_NUM_HW_RESOURCES];
>> + int irqnum;
>> + u8 vvalid_ifp;
>> +
>> + struct {
>> + struct video_device dev;
>> + struct vb2_queue vb2q;
>> + struct media_pad pad;
>> + } vdev;
>> +
>> + struct {
>> + struct v4l2_subdev sd;
>> + struct media_pad pads[RZV2H_IVC_NUM_SUBDEV_PADS];
>> + } subdev;
>> +
>> + struct {
>> + /* Spinlock to guard buffer queue */
>> + spinlock_t lock;
>> + struct workqueue_struct *async_wq;
>> + struct work_struct work;
>> + struct list_head queue;
>> + struct rzv2h_ivc_buf *curr;
>> + unsigned int sequence;
>> + } buffers;
>> +
>> + struct media_job_scheduler *sched;
>> +
>> + struct {
>> + struct v4l2_pix_format_mplane pix;
>> + const struct rzv2h_ivc_format *fmt;
>> + } format;
>> +
>> + /* Mutex to provide to vb2 */
>> + struct mutex lock;
>> + /* Lock to protect the interrupt counter */
>> + spinlock_t spinlock;
>> +};
>> +
>> +int rzv2h_ivc_init_vdev(struct rzv2h_ivc *ivc, struct v4l2_device *v4l2_dev);
>> +void rzv2h_deinit_video_dev_and_queue(struct rzv2h_ivc *ivc);
>> +int rzv2h_ivc_initialise_subdevice(struct rzv2h_ivc *ivc);
>> +void rzv2h_ivc_deinit_subdevice(struct rzv2h_ivc *ivc);
>> +void rzv2h_ivc_write(struct rzv2h_ivc *ivc, u32 addr, u32 val);
>> +void rzv2h_ivc_update_bits(struct rzv2h_ivc *ivc, unsigned int addr,
>> + u32 mask, u32 val);
> Mostly minor nits, I guess next version will be the good one.
> Could you please run v4l2-compliance and attach the test summary to
> the next cover letter version ?
Sure thing
Thanks
Dan
>
> Thanks
> j
>
>> --
>> 2.34.1
>>
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-08 14:57 ` Dan Scally
@ 2025-07-08 15:51 ` Jacopo Mondi
2025-07-09 8:13 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-08 15:51 UTC (permalink / raw)
To: Dan Scally
Cc: Jacopo Mondi, linux-media, devicetree, linux-renesas-soc,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, biju.das.jz
Hi Dan
On Tue, Jul 08, 2025 at 03:57:46PM +0100, Dan Scally wrote:
> Hi Jacopo
>
[snip]
> > > +
> > > +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
> > The driver doesn't depend or select CONFIG_PM, so this is rightfully
> > marked as __maybe_unused.
> >
> > However, it doesn't seem to me that the probe() routine manually
> > enable the peripheral, so in case of !CONFIG_PM am I wrong or the
> > device won't operate at all ?
> >
> > I would select CONFIG_PM, or otherwise call this function from the probe()
> > routine and then call pm_runtime_set_active() to inform runtime_pm
> > that the peripheral is active, and at the end of the probe routine
> > call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
> > will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
> > reduces to a nop leaving the peripheral enabled.
> Ack
> >
> > I would just select CONFIG_PM tbh
> I dropped it on Philipp's suggestion in the last review; I have no strong
I only see a comment from Philipp here
https://lore.kernel.org/all/8301d2862546507303e2dba1dd61906b848552c2.camel@pengutronix.de/
about the RESET_CONTROLLER. Have I missed other comments maybe ?
> feelings to be honest, I would expect it to be enabled in any configuration
> that was intending to use this...but I suppose there's no harm accounting
> for the possibility that it won't be
no harm no, but a bit more complex handling of the device power up
sequences.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-08 15:51 ` Jacopo Mondi
@ 2025-07-09 8:13 ` Dan Scally
2025-07-09 8:22 ` Jacopo Mondi
0 siblings, 1 reply; 21+ messages in thread
From: Dan Scally @ 2025-07-09 8:13 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
Morning
On 08/07/2025 16:51, Jacopo Mondi wrote:
> Hi Dan
>
> On Tue, Jul 08, 2025 at 03:57:46PM +0100, Dan Scally wrote:
>> Hi Jacopo
>>
> [snip]
>
>>>> +
>>>> +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
>>> The driver doesn't depend or select CONFIG_PM, so this is rightfully
>>> marked as __maybe_unused.
>>>
>>> However, it doesn't seem to me that the probe() routine manually
>>> enable the peripheral, so in case of !CONFIG_PM am I wrong or the
>>> device won't operate at all ?
>>>
>>> I would select CONFIG_PM, or otherwise call this function from the probe()
>>> routine and then call pm_runtime_set_active() to inform runtime_pm
>>> that the peripheral is active, and at the end of the probe routine
>>> call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
>>> will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
>>> reduces to a nop leaving the peripheral enabled.
>> Ack
>>> I would just select CONFIG_PM tbh
>> I dropped it on Philipp's suggestion in the last review; I have no strong
> I only see a comment from Philipp here
> https://lore.kernel.org/all/8301d2862546507303e2dba1dd61906b848552c2.camel@pengutronix.de/
> about the RESET_CONTROLLER. Have I missed other comments maybe ?
Oh no you're right; I misremembered. Sorry for the noise!
>
>> feelings to be honest, I would expect it to be enabled in any configuration
>> that was intending to use this...but I suppose there's no harm accounting
>> for the possibility that it won't be
> no harm no, but a bit more complex handling of the device power up
> sequences.
No problem; I'll just select CONFIG_PM.
Thanks
Dan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-09 8:13 ` Dan Scally
@ 2025-07-09 8:22 ` Jacopo Mondi
2025-07-09 9:12 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-09 8:22 UTC (permalink / raw)
To: Dan Scally, Sakari Ailus
Cc: Jacopo Mondi, linux-media, devicetree, linux-renesas-soc,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, biju.das.jz
Hi Dan
I recall Sakari in the past had opinions on platform drivers selecting
CONFIG_PM, let's cc him
On Wed, Jul 09, 2025 at 09:13:51AM +0100, Dan Scally wrote:
> Morning
>
> On 08/07/2025 16:51, Jacopo Mondi wrote:
> > Hi Dan
> >
> > On Tue, Jul 08, 2025 at 03:57:46PM +0100, Dan Scally wrote:
> > > Hi Jacopo
> > >
> > [snip]
> >
> > > > > +
> > > > > +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
> > > > The driver doesn't depend or select CONFIG_PM, so this is rightfully
> > > > marked as __maybe_unused.
> > > >
> > > > However, it doesn't seem to me that the probe() routine manually
> > > > enable the peripheral, so in case of !CONFIG_PM am I wrong or the
> > > > device won't operate at all ?
> > > >
> > > > I would select CONFIG_PM, or otherwise call this function from the probe()
> > > > routine and then call pm_runtime_set_active() to inform runtime_pm
> > > > that the peripheral is active, and at the end of the probe routine
> > > > call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
> > > > will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
> > > > reduces to a nop leaving the peripheral enabled.
> > > Ack
> > > > I would just select CONFIG_PM tbh
> > > I dropped it on Philipp's suggestion in the last review; I have no strong
> > I only see a comment from Philipp here
> > https://lore.kernel.org/all/8301d2862546507303e2dba1dd61906b848552c2.camel@pengutronix.de/
> > about the RESET_CONTROLLER. Have I missed other comments maybe ?
> Oh no you're right; I misremembered. Sorry for the noise!
> >
> > > feelings to be honest, I would expect it to be enabled in any configuration
> > > that was intending to use this...but I suppose there's no harm accounting
> > > for the possibility that it won't be
> > no harm no, but a bit more complex handling of the device power up
> > sequences.
>
> No problem; I'll just select CONFIG_PM.
You can then remove __maybe_unused from the function declaration.
>
>
> Thanks
>
> Dan
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-09 8:22 ` Jacopo Mondi
@ 2025-07-09 9:12 ` Geert Uytterhoeven
2025-07-11 11:17 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2025-07-09 9:12 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Dan Scally, Sakari Ailus, linux-media, devicetree,
linux-renesas-soc, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, biju.das.jz
On Wed, 9 Jul 2025 at 10:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> I recall Sakari in the past had opinions on platform drivers selecting
> On Wed, Jul 09, 2025 at 09:13:51AM +0100, Dan Scally wrote:
> > On 08/07/2025 16:51, Jacopo Mondi wrote:
> > > On Tue, Jul 08, 2025 at 03:57:46PM +0100, Dan Scally wrote:
> > > > > > +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
> > > > > The driver doesn't depend or select CONFIG_PM, so this is rightfully
> > > > > marked as __maybe_unused.
> > > > >
> > > > > However, it doesn't seem to me that the probe() routine manually
> > > > > enable the peripheral, so in case of !CONFIG_PM am I wrong or the
> > > > > device won't operate at all ?
> > > > >
> > > > > I would select CONFIG_PM, or otherwise call this function from the probe()
> > > > > routine and then call pm_runtime_set_active() to inform runtime_pm
> > > > > that the peripheral is active, and at the end of the probe routine
> > > > > call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
> > > > > will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
> > > > > reduces to a nop leaving the peripheral enabled.
> > > > Ack
> > > > > I would just select CONFIG_PM tbh
> > > > I dropped it on Philipp's suggestion in the last review; I have no strong
> > > I only see a comment from Philipp here
> > > https://lore.kernel.org/all/8301d2862546507303e2dba1dd61906b848552c2.camel@pengutronix.de/
> > > about the RESET_CONTROLLER. Have I missed other comments maybe ?
> > Oh no you're right; I misremembered. Sorry for the noise!
> > >
> > > > feelings to be honest, I would expect it to be enabled in any configuration
> > > > that was intending to use this...but I suppose there's no harm accounting
> > > > for the possibility that it won't be
> > > no harm no, but a bit more complex handling of the device power up
> > > sequences.
> >
> > No problem; I'll just select CONFIG_PM.
>
> You can then remove __maybe_unused from the function declaration.
You could just remove them anyway, iff you would use the newer
*_PM_OPS() instead of the old SET_*_PM_OPS().
P.S. I already converted all Renesas drivers locally, so no need to
start working on the conversion to *_PM_OPS() for existing
Renesas drivers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver
2025-07-09 9:12 ` Geert Uytterhoeven
@ 2025-07-11 11:17 ` Dan Scally
0 siblings, 0 replies; 21+ messages in thread
From: Dan Scally @ 2025-07-11 11:17 UTC (permalink / raw)
To: Geert Uytterhoeven, Jacopo Mondi
Cc: Sakari Ailus, linux-media, devicetree, linux-renesas-soc,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, biju.das.jz
Hi Geert
On 09/07/2025 10:12, Geert Uytterhoeven wrote:
> On Wed, 9 Jul 2025 at 10:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>> I recall Sakari in the past had opinions on platform drivers selecting
>> On Wed, Jul 09, 2025 at 09:13:51AM +0100, Dan Scally wrote:
>>> On 08/07/2025 16:51, Jacopo Mondi wrote:
>>>> On Tue, Jul 08, 2025 at 03:57:46PM +0100, Dan Scally wrote:
>>>>>>> +static int __maybe_unused rzv2h_ivc_runtime_resume(struct device *dev)
>>>>>> The driver doesn't depend or select CONFIG_PM, so this is rightfully
>>>>>> marked as __maybe_unused.
>>>>>>
>>>>>> However, it doesn't seem to me that the probe() routine manually
>>>>>> enable the peripheral, so in case of !CONFIG_PM am I wrong or the
>>>>>> device won't operate at all ?
>>>>>>
>>>>>> I would select CONFIG_PM, or otherwise call this function from the probe()
>>>>>> routine and then call pm_runtime_set_active() to inform runtime_pm
>>>>>> that the peripheral is active, and at the end of the probe routine
>>>>>> call pm_runtime_put_autosuspend(): in case of CONFIG_PM the peripheral
>>>>>> will suspend, in case of !CONFIG_PM the pm_runtime_put_autosuspend()
>>>>>> reduces to a nop leaving the peripheral enabled.
>>>>> Ack
>>>>>> I would just select CONFIG_PM tbh
>>>>> I dropped it on Philipp's suggestion in the last review; I have no strong
>>>> I only see a comment from Philipp here
>>>> https://lore.kernel.org/all/8301d2862546507303e2dba1dd61906b848552c2.camel@pengutronix.de/
>>>> about the RESET_CONTROLLER. Have I missed other comments maybe ?
>>> Oh no you're right; I misremembered. Sorry for the noise!
>>>>> feelings to be honest, I would expect it to be enabled in any configuration
>>>>> that was intending to use this...but I suppose there's no harm accounting
>>>>> for the possibility that it won't be
>>>> no harm no, but a bit more complex handling of the device power up
>>>> sequences.
>>> No problem; I'll just select CONFIG_PM.
>> You can then remove __maybe_unused from the function declaration.
> You could just remove them anyway, iff you would use the newer
> *_PM_OPS() instead of the old SET_*_PM_OPS().
Thanks! I went with this.
> P.S. I already converted all Renesas drivers locally, so no need to
> start working on the conversion to *_PM_OPS() for existing
> Renesas drivers.
>
> Gr{oetje,eeting}s,
>
> Geert
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-08 13:10 ` Jacopo Mondi
@ 2025-07-11 11:51 ` Dan Scally
2025-07-11 12:43 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Dan Scally @ 2025-07-11 11:51 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
Hi Jacopo - thanks for the comments
On 08/07/2025 14:10, Jacopo Mondi wrote:
> Hi Dan
>
> On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
>> Add helpers to run the new media_pipeline_started() and
>> media_pipeline_stopped() functions. The helpers iterate over the
>> entities in the pipeline and count the number of video devices and
>> compare that to the pipeline's start_count() before acting. This
>> allows us to only run the media pipeline callbacks in the event that
>> the pipeline has had video_pipeline_start() called for each video
>> device.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>> ---
>>
>> We could take this further perhaps and include the equivalent routine
>> in video_device_pipeline[_alloc]_start()...if none of the entities
>> involved have .pipeline_started() or .pipeline_stopped() operations it
>> should be harmless, but I'm a bit reluctant to force the choice to run
>> those operations on users.
> I know I've kind of suggested that, but after all I don't think it's a
> very good idea, having this in two steps is probably better. And I
> like the fact the v4l2-dev layer operates on the video device counting
> and only relies on the mc layer for the callbacks notification.
Yeah me too. Let's stick to this
>
>> Changes in v2:
>>
>> - Adapted now media_pipeline_for_each_entity() takes an iter
>> variable
>> - Fixed the Return: section of the kerneldoc comments
>> ---
>> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
>> include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
>> 2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
>> }
>> EXPORT_SYMBOL_GPL(video_device_pipeline);
>>
>> +static int __video_device_pipeline_started(struct media_pipeline *pipe)
> __function_name() is usually reserved for the non-locking version of
> function_name().
>
> This seems to be an helper only used internally by
> video_device_pipeline_started() so I would use a different name
> something like video_device_has_pipeline_started() ?
What it does is count the number of _unstarted_ video
devices..."video_device_pipeline_unstarted_vdevs()"?
>
>
>> +{
>> + struct media_pipeline_entity_iter iter;
>> + unsigned int n_video_devices = 0;
>> + struct media_entity *entity;
>> + int ret;
>> +
>> + ret = media_pipeline_entity_iter_init(pipe, &iter);
>> + if (ret)
>> + return ret;
>> +
>> + media_pipeline_for_each_entity(pipe, &iter, entity) {
>> + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>> + n_video_devices++;
>> + }
>> +
>> + media_pipeline_entity_iter_cleanup(&iter);
>> +
>> + return n_video_devices - pipe->start_count;
>> +}
>> +
>> +int video_device_pipeline_started(struct video_device *vdev)
>> +{
>> + struct media_pipeline *pipe;
>> + int ret;
>> +
>> + pipe = video_device_pipeline(vdev);
>> + if (!pipe)
>> + return -ENODEV;
>> +
>> + ret = __video_device_pipeline_started(pipe);
>> + if (ret)
>> + return ret;
> I would not return ret, as it might take random values betwen
> n_video_devices and 1. See below on the return value documentation
But we need to be able to signal to the driver three states:
1. No errors, but there are still unstarted video devices
2. No errors and there are no unstarted video devices
3. An error
So I expect a driver to do a two stage check:
ret = video_device_pipeline_started(vdev);
if (ret < 0)
goto err_out;
if (ret == 0)
// something appropriate here like run the media jobs scheduler
>
>> +
>> + return media_pipeline_started(pipe);
>> +}
>> +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
>> +
>> +int video_device_pipeline_stopped(struct video_device *vdev)
>> +{
>> + struct media_pipeline *pipe;
>> + int ret;
>> +
>> + pipe = video_device_pipeline(vdev);
>> + if (!pipe)
>> + return -ENODEV;
>> +
>> + ret = __video_device_pipeline_started(pipe);
>> + if (ret)
>> + return ret;
> ditto
>
>> +
>> + media_pipeline_stopped(pipe);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
>> +
>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>
>> /*
>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>> index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
>> --- a/include/media/v4l2-dev.h
>> +++ b/include/media/v4l2-dev.h
>> @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
>> */
>> struct media_pipeline *video_device_pipeline(struct video_device *vdev);
>>
>> +/**
>> + * video_device_pipeline_started - Run the pipeline_started() entity operation
>> + * for a fully-started media pipeline
>> + * @vdev: A video device that's part of the pipeline
>> + *
>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>> + * connected to a given video device through enabled links have been marked as
> I would use the same text as the one from video_device_pipeline_start()
>
> " connected to a given video device through enabled links, either
> directly or indirectly,"
Ack
>
>> + * streaming through the use of video_device_pipeline_start() or one of its
>> + * equivalent functions. If so, media_pipeline_started() is called to inform
>> + * entities in the pipeline of that fact. The intention is to provide drivers
>> + * with a shortcut for checking whether their pipeline is fully ready to start
>> + * processing data.
> Not really a shortcut, I would use "mechanism" instead.
>
> I would also specify that:
>
> * entities in the pipeline of that fact. The intention is to provide drivers
> * with a mechanism for checking whether their pipeline is fully ready to start
> * processing data and call the .pipeline_started() media entity operation
> * on all the entities in the pipeline.
Ack!
>
>> + *
>> + * Return: The number of video devices in the pipeline remaining to be started,
>> + * or a negative error number on failure.
> 0 for success as well
>
> I would anyway return 0 for success and a specific error code for the
> three failure cases:
> -ENOMEM if allocating the iterator fails
> -ENODEV if not all video devices have started
> -EINVAL if media_pipeline_started() fails
>
> You can document them as (copying from iommu.h)
>
> * Return:
> * * 0 - success
> * * EINVAL - call to pipeline_started() failed
> * * ENOMEM - failed to allocate pipe iterator
> * * ENODEV - pipeline not yet fully started
>
>> + */
>> +int video_device_pipeline_started(struct video_device *vdev);
>> +
>> +/**
>> + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
>> + * for a fully-started media pipeline
>> + * @vdev: A video device that's part of the pipeline
>> + *
>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>> + * connected to a given video device through enabled links have been marked as
>> + * streaming through the use of video_device_pipeline_start() or one of its
> What is the intended semantic here ? The first video device to receive
> a streamoff() will trigger media_pipeline_stopped() or should the last
> one do that ?
The first one should do it, so the first device caling stop should trigger actual stop in all
involved hardware.
>
>> + * equivalent functions. If so, media_pipeline_stopped() is called for each
>> + * entity in the pipeline. The intention is to provide drivers with a shortcut
>> + * for checking whether this video device is the first device in the pipeline
>> + * to be stopped.
>> + *
>> + * Return: The number of video devices in the pipeline remaining to be started, or a
>> + * negative error number on failure.
>> + */
>> +int video_device_pipeline_stopped(struct video_device *vdev);
>> +
>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>
>> #endif /* _V4L2_DEV_H */
>>
>> --
>> 2.34.1
>>
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-11 11:51 ` Dan Scally
@ 2025-07-11 12:43 ` Dan Scally
2025-07-11 13:39 ` Jacopo Mondi
0 siblings, 1 reply; 21+ messages in thread
From: Dan Scally @ 2025-07-11 12:43 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
On 11/07/2025 12:51, Dan Scally wrote:
> Hi Jacopo - thanks for the comments
>
> On 08/07/2025 14:10, Jacopo Mondi wrote:
>> Hi Dan
>>
>> On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
>>> Add helpers to run the new media_pipeline_started() and
>>> media_pipeline_stopped() functions. The helpers iterate over the
>>> entities in the pipeline and count the number of video devices and
>>> compare that to the pipeline's start_count() before acting. This
>>> allows us to only run the media pipeline callbacks in the event that
>>> the pipeline has had video_pipeline_start() called for each video
>>> device.
>>>
>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>
>>> ---
>>>
>>> We could take this further perhaps and include the equivalent routine
>>> in video_device_pipeline[_alloc]_start()...if none of the entities
>>> involved have .pipeline_started() or .pipeline_stopped() operations it
>>> should be harmless, but I'm a bit reluctant to force the choice to run
>>> those operations on users.
>> I know I've kind of suggested that, but after all I don't think it's a
>> very good idea, having this in two steps is probably better. And I
>> like the fact the v4l2-dev layer operates on the video device counting
>> and only relies on the mc layer for the callbacks notification.
>
>
> Yeah me too. Let's stick to this
>
>>
>>> Changes in v2:
>>>
>>> - Adapted now media_pipeline_for_each_entity() takes an iter
>>> variable
>>> - Fixed the Return: section of the kerneldoc comments
>>> ---
>>> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
>>> include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
>>> 2 files changed, 93 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
>>> }
>>> EXPORT_SYMBOL_GPL(video_device_pipeline);
>>>
>>> +static int __video_device_pipeline_started(struct media_pipeline *pipe)
>> __function_name() is usually reserved for the non-locking version of
>> function_name().
>>
>> This seems to be an helper only used internally by
>> video_device_pipeline_started() so I would use a different name
>> something like video_device_has_pipeline_started() ?
>
>
> What it does is count the number of _unstarted_ video
> devices..."video_device_pipeline_unstarted_vdevs()"?
>
>>
>>
>>> +{
>>> + struct media_pipeline_entity_iter iter;
>>> + unsigned int n_video_devices = 0;
>>> + struct media_entity *entity;
>>> + int ret;
>>> +
>>> + ret = media_pipeline_entity_iter_init(pipe, &iter);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + media_pipeline_for_each_entity(pipe, &iter, entity) {
>>> + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>>> + n_video_devices++;
>>> + }
>>> +
>>> + media_pipeline_entity_iter_cleanup(&iter);
>>> +
>>> + return n_video_devices - pipe->start_count;
>>> +}
>>> +
>>> +int video_device_pipeline_started(struct video_device *vdev)
>>> +{
>>> + struct media_pipeline *pipe;
>>> + int ret;
>>> +
>>> + pipe = video_device_pipeline(vdev);
>>> + if (!pipe)
>>> + return -ENODEV;
>>> +
>>> + ret = __video_device_pipeline_started(pipe);
>>> + if (ret)
>>> + return ret;
>> I would not return ret, as it might take random values betwen
>> n_video_devices and 1. See below on the return value documentation
>
> But we need to be able to signal to the driver three states:
>
>
> 1. No errors, but there are still unstarted video devices
>
> 2. No errors and there are no unstarted video devices
>
> 3. An error
>
>
> So I expect a driver to do a two stage check:
>
>
> ret = video_device_pipeline_started(vdev);
>
> if (ret < 0)
>
> goto err_out;
>
> if (ret == 0)
>
> // something appropriate here like run the media jobs scheduler
>
Sorry: I had a reading comprehension failure. You were suggesting to use -ENODEV to signal that
there are unstarted video devices remaining. I understand now, but I'm still not sure about it,
because then instead of the "if (ret == 0) check here we'd have "if (ret == -ENODEV)", which I don't
especially like...or am I missing some way to avoid having that check here?
Thanks
Dan
>
>>
>>> +
>>> + return media_pipeline_started(pipe);
>>> +}
>>> +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
>>> +
>>> +int video_device_pipeline_stopped(struct video_device *vdev)
>>> +{
>>> + struct media_pipeline *pipe;
>>> + int ret;
>>> +
>>> + pipe = video_device_pipeline(vdev);
>>> + if (!pipe)
>>> + return -ENODEV;
>>> +
>>> + ret = __video_device_pipeline_started(pipe);
>>> + if (ret)
>>> + return ret;
>> ditto
>>
>>> +
>>> + media_pipeline_stopped(pipe);
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
>>> +
>>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>>
>>> /*
>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>> index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
>>> --- a/include/media/v4l2-dev.h
>>> +++ b/include/media/v4l2-dev.h
>>> @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
>>> */
>>> struct media_pipeline *video_device_pipeline(struct video_device *vdev);
>>>
>>> +/**
>>> + * video_device_pipeline_started - Run the pipeline_started() entity operation
>>> + * for a fully-started media pipeline
>>> + * @vdev: A video device that's part of the pipeline
>>> + *
>>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>>> + * connected to a given video device through enabled links have been marked as
>> I would use the same text as the one from video_device_pipeline_start()
>>
>> " connected to a given video device through enabled links, either
>> directly or indirectly,"
>
>
> Ack
>
>>
>>> + * streaming through the use of video_device_pipeline_start() or one of its
>>> + * equivalent functions. If so, media_pipeline_started() is called to inform
>>> + * entities in the pipeline of that fact. The intention is to provide drivers
>>> + * with a shortcut for checking whether their pipeline is fully ready to start
>>> + * processing data.
>> Not really a shortcut, I would use "mechanism" instead.
>>
>> I would also specify that:
>>
>> * entities in the pipeline of that fact. The intention is to provide drivers
>> * with a mechanism for checking whether their pipeline is fully ready to start
>> * processing data and call the .pipeline_started() media entity operation
>> * on all the entities in the pipeline.
> Ack!
>>
>>> + *
>>> + * Return: The number of video devices in the pipeline remaining to be started,
>>> + * or a negative error number on failure.
>> 0 for success as well
>>
>> I would anyway return 0 for success and a specific error code for the
>> three failure cases:
>> -ENOMEM if allocating the iterator fails
>> -ENODEV if not all video devices have started
>> -EINVAL if media_pipeline_started() fails
>>
>> You can document them as (copying from iommu.h)
>>
>> * Return:
>> * * 0 - success
>> * * EINVAL - call to pipeline_started() failed
>> * * ENOMEM - failed to allocate pipe iterator
>> * * ENODEV - pipeline not yet fully started
>>
>>> + */
>>> +int video_device_pipeline_started(struct video_device *vdev);
>>> +
>>> +/**
>>> + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
>>> + * for a fully-started media pipeline
>>> + * @vdev: A video device that's part of the pipeline
>>> + *
>>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>>> + * connected to a given video device through enabled links have been marked as
>>> + * streaming through the use of video_device_pipeline_start() or one of its
>> What is the intended semantic here ? The first video device to receive
>> a streamoff() will trigger media_pipeline_stopped() or should the last
>> one do that ?
> The first one should do it, so the first device caling stop should trigger actual stop in all
> involved hardware.
>>
>>> + * equivalent functions. If so, media_pipeline_stopped() is called for each
>>> + * entity in the pipeline. The intention is to provide drivers with a shortcut
>>> + * for checking whether this video device is the first device in the pipeline
>>> + * to be stopped.
>>> + *
>>> + * Return: The number of video devices in the pipeline remaining to be started, or a
>>> + * negative error number on failure.
>>> + */
>>> +int video_device_pipeline_stopped(struct video_device *vdev);
>>> +
>>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>>
>>> #endif /* _V4L2_DEV_H */
>>>
>>> --
>>> 2.34.1
>>>
>>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops
2025-07-08 13:29 ` Jacopo Mondi
@ 2025-07-11 13:24 ` Dan Scally
0 siblings, 0 replies; 21+ messages in thread
From: Dan Scally @ 2025-07-11 13:24 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
Hi Jacopo
On 08/07/2025 14:29, Jacopo Mondi wrote:
> Hi Dan
>
> On Fri, Jul 04, 2025 at 12:20:18PM +0100, Daniel Scally wrote:
>> Add two new members to struct media_entity_operations, along with new
>> functions in media-entity.c to traverse a media pipeline and call the
>> new operations. The new functions are intended to be used to signal
>> to a media pipeline that it has fully started, with the entity ops
>> allowing drivers to define some action to be taken when those
>> conditions are met.
>>
>> The combination of the new functions and operations allows drivers
>> which are part of a multi-driver pipeline to delay actually starting
>> streaming until all of the conditions for streaming succcessfully are
>> met across all drivers.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v4:
>>
>> - Reverted to having the iter variable
>>
>> Changes in v3:
>>
>> - Dropped the iter variable now that the pipeline entity
>> iterator functions don't need it.
>> - Updated documentation to specify Optional and return
>> values
>>
>> Changes in v2:
>>
>> - Refactored media_pipeline_started() such that the cleanup
>> function for media_pipeline_entity_iter is unconditionally
>> called
>> - Avoided using media_entity_call() helper for operation that
>> has return type void to avoid compiler warnings
>> ---
>> drivers/media/mc/mc-entity.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>> include/media/media-entity.h | 29 ++++++++++++++++++++++++++++
>> 2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index 045590905582054c46656e20463271b1f93fa6b4..d3443537d4304e12cb015630101efba22375c011 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -1053,6 +1053,52 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>> }
>> EXPORT_SYMBOL_GPL(__media_pipeline_entity_iter_next);
>>
>> +int media_pipeline_started(struct media_pipeline *pipe)
>> +{
>> + struct media_pipeline_entity_iter iter;
>> + struct media_entity *entity;
>> + int ret;
>> +
>> + ret = media_pipeline_entity_iter_init(pipe, &iter);
>> + if (ret)
>> + return ret;
>> +
>> + media_pipeline_for_each_entity(pipe, &iter, entity) {
>> + ret = media_entity_call(entity, pipeline_started);
>> + if (ret && ret != -ENOIOCTLCMD)
>> + break;
>> + }
>> +
>> + media_pipeline_entity_iter_cleanup(&iter);
>> +
>> + ret = ret == -ENOIOCTLCMD ? 0 : ret;
>> + if (ret)
>> + media_pipeline_stopped(pipe);
> If you take my suggestion to limit the return value of
> video_device_pipeline_started() to three possible error codes, you
> could return -EINVAL here
>
>> +
>> + return ret;
> and 0 here
>
>> +}
>> +EXPORT_SYMBOL_GPL(media_pipeline_started);
>> +
>> +int media_pipeline_stopped(struct media_pipeline *pipe)
>> +{
>> + struct media_pipeline_entity_iter iter;
>> + struct media_entity *entity;
>> + int ret;
>> +
>> + ret = media_pipeline_entity_iter_init(pipe, &iter);
>> + if (ret)
>> + return ret;
>> +
>> + media_pipeline_for_each_entity(pipe, &iter, entity)
>> + if (entity->ops && entity->ops->pipeline_stopped)
>> + entity->ops->pipeline_stopped(entity);
>> +
>> + media_pipeline_entity_iter_cleanup(&iter);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(media_pipeline_stopped);
>> +
>> /* -----------------------------------------------------------------------------
>> * Links management
>> */
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index 64cf590b11343f68a456c5870ca2f32917c122f9..ad658f42357ec505c84d9479bbbf18494da7f939 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -269,6 +269,10 @@ struct media_pad {
>> * media_entity_has_pad_interdep().
>> * Optional: If the operation isn't implemented all pads
>> * will be considered as interdependent.
>> + * @pipeline_started: Notify this entity that the pipeline it is a part of has
>> + * been started
>> + * @pipeline_stopped: Notify this entity that the pipeline it is a part of has
>> + * been stopped
> The documentation of the other functions end with a full stop.
> If the operation is optional, I would specify it here like it's done
> for other operations
>
>> *
>> * .. note::
>> *
>> @@ -284,6 +288,8 @@ struct media_entity_operations {
>> int (*link_validate)(struct media_link *link);
>> bool (*has_pad_interdep)(struct media_entity *entity, unsigned int pad0,
>> unsigned int pad1);
>> + int (*pipeline_started)(struct media_entity *entity);
>> + void (*pipeline_stopped)(struct media_entity *entity);
>> };
>>
>> /**
>> @@ -1261,6 +1267,29 @@ __media_pipeline_entity_iter_next(struct media_pipeline *pipe,
>> entity != NULL; \
>> entity = __media_pipeline_entity_iter_next((pipe), iter, entity))
>>
>> +/**
>> + * media_pipeline_started - Inform entities in a pipeline that it has started
>> + * @pipe: The pipeline
>> + *
>> + * Iterate on all entities in a media pipeline and call their pipeline_started
>> + * member of media_entity_operations. Optional.
> I would move "Optional" to the documentation of the media entity
I don't know what I was thinking putting it here...
Thanks!
Dan
>
>> + *
>> + * Return: zero on success, or a negative error code passed through from an
>> + * entity's .pipeline_started() operation.
>> + */
>> +int media_pipeline_started(struct media_pipeline *pipe);
>> +
>> +/**
>> + * media_pipeline_stopped - Inform entities in a pipeline that it has stopped
>> + * @pipe: The pipeline
>> + *
>> + * Iterate on all entities in a media pipeline and call their pipeline_stopped
>> + * member of media_entity_operations. Optional.
>> + *
>> + * Return: zero on success, or -ENOMEM if the iterator initialisation failed.
>> + */
>> +int media_pipeline_stopped(struct media_pipeline *pipe);
>> +
>> /**
>> * media_pipeline_alloc_start - Mark a pipeline as streaming
>> * @pad: Starting pad
>>
>> --
>> 2.34.1
>>
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-11 12:43 ` Dan Scally
@ 2025-07-11 13:39 ` Jacopo Mondi
2025-07-14 6:37 ` Dan Scally
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2025-07-11 13:39 UTC (permalink / raw)
To: Dan Scally
Cc: Jacopo Mondi, linux-media, devicetree, linux-renesas-soc,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Philipp Zabel, biju.das.jz
Hi Dan
On Fri, Jul 11, 2025 at 01:43:16PM +0100, Dan Scally wrote:
>
> On 11/07/2025 12:51, Dan Scally wrote:
> > Hi Jacopo - thanks for the comments
> >
> > On 08/07/2025 14:10, Jacopo Mondi wrote:
> > > Hi Dan
> > >
> > > On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
> > > > Add helpers to run the new media_pipeline_started() and
> > > > media_pipeline_stopped() functions. The helpers iterate over the
> > > > entities in the pipeline and count the number of video devices and
> > > > compare that to the pipeline's start_count() before acting. This
> > > > allows us to only run the media pipeline callbacks in the event that
> > > > the pipeline has had video_pipeline_start() called for each video
> > > > device.
> > > >
> > > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > >
> > > > ---
> > > >
> > > > We could take this further perhaps and include the equivalent routine
> > > > in video_device_pipeline[_alloc]_start()...if none of the entities
> > > > involved have .pipeline_started() or .pipeline_stopped() operations it
> > > > should be harmless, but I'm a bit reluctant to force the choice to run
> > > > those operations on users.
> > > I know I've kind of suggested that, but after all I don't think it's a
> > > very good idea, having this in two steps is probably better. And I
> > > like the fact the v4l2-dev layer operates on the video device counting
> > > and only relies on the mc layer for the callbacks notification.
> >
> >
> > Yeah me too. Let's stick to this
> >
> > >
> > > > Changes in v2:
> > > >
> > > > - Adapted now media_pipeline_for_each_entity() takes an iter
> > > > variable
> > > > - Fixed the Return: section of the kerneldoc comments
> > > > ---
> > > > drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
> > > > include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
> > > > 2 files changed, 93 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
> > > > }
> > > > EXPORT_SYMBOL_GPL(video_device_pipeline);
> > > >
> > > > +static int __video_device_pipeline_started(struct media_pipeline *pipe)
> > > __function_name() is usually reserved for the non-locking version of
> > > function_name().
> > >
> > > This seems to be an helper only used internally by
> > > video_device_pipeline_started() so I would use a different name
> > > something like video_device_has_pipeline_started() ?
> >
> >
> > What it does is count the number of _unstarted_ video
> > devices..."video_device_pipeline_unstarted_vdevs()"?
> >
> > >
> > >
> > > > +{
> > > > + struct media_pipeline_entity_iter iter;
> > > > + unsigned int n_video_devices = 0;
> > > > + struct media_entity *entity;
> > > > + int ret;
> > > > +
> > > > + ret = media_pipeline_entity_iter_init(pipe, &iter);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + media_pipeline_for_each_entity(pipe, &iter, entity) {
> > > > + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
> > > > + n_video_devices++;
> > > > + }
> > > > +
> > > > + media_pipeline_entity_iter_cleanup(&iter);
> > > > +
> > > > + return n_video_devices - pipe->start_count;
> > > > +}
> > > > +
> > > > +int video_device_pipeline_started(struct video_device *vdev)
> > > > +{
> > > > + struct media_pipeline *pipe;
> > > > + int ret;
> > > > +
> > > > + pipe = video_device_pipeline(vdev);
> > > > + if (!pipe)
> > > > + return -ENODEV;
> > > > +
> > > > + ret = __video_device_pipeline_started(pipe);
> > > > + if (ret)
> > > > + return ret;
> > > I would not return ret, as it might take random values betwen
> > > n_video_devices and 1. See below on the return value documentation
> >
> > But we need to be able to signal to the driver three states:
> >
> >
> > 1. No errors, but there are still unstarted video devices
> >
> > 2. No errors and there are no unstarted video devices
> >
> > 3. An error
> >
> >
> > So I expect a driver to do a two stage check:
> >
> >
> > ret = video_device_pipeline_started(vdev);
> >
> > if (ret < 0)
> >
> > goto err_out;
> >
> > if (ret == 0)
> >
> > // something appropriate here like run the media jobs scheduler
> >
> Sorry: I had a reading comprehension failure. You were suggesting to use
> -ENODEV to signal that there are unstarted video devices remaining. I
> understand now, but I'm still not sure about it, because then instead of the
> "if (ret == 0) check here we'd have "if (ret == -ENODEV)", which I don't
> especially like...or am I missing some way to avoid having that check here?
Yes, that would require drivers to check for -ENODEV to identify a
non-failure case when not all devices have started..
You're right it might not be optimal. With you implementation we
should then have
0: success
< 0: error
> 0: not all devices started
?
I might be actually ok with this, but could you please document it as
I've below suggested for clarity ?
Thanks
j
>
>
> Thanks
>
> Dan
>
> >
> > >
> > > > +
> > > > + return media_pipeline_started(pipe);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
> > > > +
> > > > +int video_device_pipeline_stopped(struct video_device *vdev)
> > > > +{
> > > > + struct media_pipeline *pipe;
> > > > + int ret;
> > > > +
> > > > + pipe = video_device_pipeline(vdev);
> > > > + if (!pipe)
> > > > + return -ENODEV;
> > > > +
> > > > + ret = __video_device_pipeline_started(pipe);
> > > > + if (ret)
> > > > + return ret;
> > > ditto
> > >
> > > > +
> > > > + media_pipeline_stopped(pipe);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
> > > > +
> > > > #endif /* CONFIG_MEDIA_CONTROLLER */
> > > >
> > > > /*
> > > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > > index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
> > > > --- a/include/media/v4l2-dev.h
> > > > +++ b/include/media/v4l2-dev.h
> > > > @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
> > > > */
> > > > struct media_pipeline *video_device_pipeline(struct video_device *vdev);
> > > >
> > > > +/**
> > > > + * video_device_pipeline_started - Run the pipeline_started() entity operation
> > > > + * for a fully-started media pipeline
> > > > + * @vdev: A video device that's part of the pipeline
> > > > + *
> > > > + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> > > > + * connected to a given video device through enabled links have been marked as
> > > I would use the same text as the one from video_device_pipeline_start()
> > >
> > > " connected to a given video device through enabled links, either
> > > directly or indirectly,"
> >
> >
> > Ack
> >
> > >
> > > > + * streaming through the use of video_device_pipeline_start() or one of its
> > > > + * equivalent functions. If so, media_pipeline_started() is called to inform
> > > > + * entities in the pipeline of that fact. The intention is to provide drivers
> > > > + * with a shortcut for checking whether their pipeline is fully ready to start
> > > > + * processing data.
> > > Not really a shortcut, I would use "mechanism" instead.
> > >
> > > I would also specify that:
> > >
> > > * entities in the pipeline of that fact. The intention is to provide drivers
> > > * with a mechanism for checking whether their pipeline is fully ready to start
> > > * processing data and call the .pipeline_started() media entity operation
> > > * on all the entities in the pipeline.
> > Ack!
> > >
> > > > + *
> > > > + * Return: The number of video devices in the pipeline remaining to be started,
> > > > + * or a negative error number on failure.
> > > 0 for success as well
> > >
> > > I would anyway return 0 for success and a specific error code for the
> > > three failure cases:
> > > -ENOMEM if allocating the iterator fails
> > > -ENODEV if not all video devices have started
> > > -EINVAL if media_pipeline_started() fails
> > >
> > > You can document them as (copying from iommu.h)
> > >
> > > * Return:
> > > * * 0 - success
> > > * * EINVAL - call to pipeline_started() failed
> > > * * ENOMEM - failed to allocate pipe iterator
> > > * * ENODEV - pipeline not yet fully started
> > >
> > > > + */
> > > > +int video_device_pipeline_started(struct video_device *vdev);
> > > > +
> > > > +/**
> > > > + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
> > > > + * for a fully-started media pipeline
> > > > + * @vdev: A video device that's part of the pipeline
> > > > + *
> > > > + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
> > > > + * connected to a given video device through enabled links have been marked as
> > > > + * streaming through the use of video_device_pipeline_start() or one of its
> > > What is the intended semantic here ? The first video device to receive
> > > a streamoff() will trigger media_pipeline_stopped() or should the last
> > > one do that ?
> > The first one should do it, so the first device caling stop should
> > trigger actual stop in all involved hardware.
> > >
> > > > + * equivalent functions. If so, media_pipeline_stopped() is called for each
> > > > + * entity in the pipeline. The intention is to provide drivers with a shortcut
> > > > + * for checking whether this video device is the first device in the pipeline
> > > > + * to be stopped.
> > > > + *
> > > > + * Return: The number of video devices in the pipeline remaining to be started, or a
> > > > + * negative error number on failure.
> > > > + */
> > > > +int video_device_pipeline_stopped(struct video_device *vdev);
> > > > +
> > > > #endif /* CONFIG_MEDIA_CONTROLLER */
> > > >
> > > > #endif /* _V4L2_DEV_H */
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]()
2025-07-11 13:39 ` Jacopo Mondi
@ 2025-07-14 6:37 ` Dan Scally
0 siblings, 0 replies; 21+ messages in thread
From: Dan Scally @ 2025-07-14 6:37 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, devicetree, linux-renesas-soc, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, Philipp Zabel, biju.das.jz
Hi Jacopo
On 11/07/2025 14:39, Jacopo Mondi wrote:
> Hi Dan
>
> On Fri, Jul 11, 2025 at 01:43:16PM +0100, Dan Scally wrote:
>> On 11/07/2025 12:51, Dan Scally wrote:
>>> Hi Jacopo - thanks for the comments
>>>
>>> On 08/07/2025 14:10, Jacopo Mondi wrote:
>>>> Hi Dan
>>>>
>>>> On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
>>>>> Add helpers to run the new media_pipeline_started() and
>>>>> media_pipeline_stopped() functions. The helpers iterate over the
>>>>> entities in the pipeline and count the number of video devices and
>>>>> compare that to the pipeline's start_count() before acting. This
>>>>> allows us to only run the media pipeline callbacks in the event that
>>>>> the pipeline has had video_pipeline_start() called for each video
>>>>> device.
>>>>>
>>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>>>
>>>>> ---
>>>>>
>>>>> We could take this further perhaps and include the equivalent routine
>>>>> in video_device_pipeline[_alloc]_start()...if none of the entities
>>>>> involved have .pipeline_started() or .pipeline_stopped() operations it
>>>>> should be harmless, but I'm a bit reluctant to force the choice to run
>>>>> those operations on users.
>>>> I know I've kind of suggested that, but after all I don't think it's a
>>>> very good idea, having this in two steps is probably better. And I
>>>> like the fact the v4l2-dev layer operates on the video device counting
>>>> and only relies on the mc layer for the callbacks notification.
>>>
>>> Yeah me too. Let's stick to this
>>>
>>>>> Changes in v2:
>>>>>
>>>>> - Adapted now media_pipeline_for_each_entity() takes an iter
>>>>> variable
>>>>> - Fixed the Return: section of the kerneldoc comments
>>>>> ---
>>>>> drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
>>>>> include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
>>>>> 2 files changed, 93 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>>> index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>>> @@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(video_device_pipeline);
>>>>>
>>>>> +static int __video_device_pipeline_started(struct media_pipeline *pipe)
>>>> __function_name() is usually reserved for the non-locking version of
>>>> function_name().
>>>>
>>>> This seems to be an helper only used internally by
>>>> video_device_pipeline_started() so I would use a different name
>>>> something like video_device_has_pipeline_started() ?
>>>
>>> What it does is count the number of _unstarted_ video
>>> devices..."video_device_pipeline_unstarted_vdevs()"?
>>>
>>>>
>>>>> +{
>>>>> + struct media_pipeline_entity_iter iter;
>>>>> + unsigned int n_video_devices = 0;
>>>>> + struct media_entity *entity;
>>>>> + int ret;
>>>>> +
>>>>> + ret = media_pipeline_entity_iter_init(pipe, &iter);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + media_pipeline_for_each_entity(pipe, &iter, entity) {
>>>>> + if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
>>>>> + n_video_devices++;
>>>>> + }
>>>>> +
>>>>> + media_pipeline_entity_iter_cleanup(&iter);
>>>>> +
>>>>> + return n_video_devices - pipe->start_count;
>>>>> +}
>>>>> +
>>>>> +int video_device_pipeline_started(struct video_device *vdev)
>>>>> +{
>>>>> + struct media_pipeline *pipe;
>>>>> + int ret;
>>>>> +
>>>>> + pipe = video_device_pipeline(vdev);
>>>>> + if (!pipe)
>>>>> + return -ENODEV;
>>>>> +
>>>>> + ret = __video_device_pipeline_started(pipe);
>>>>> + if (ret)
>>>>> + return ret;
>>>> I would not return ret, as it might take random values betwen
>>>> n_video_devices and 1. See below on the return value documentation
>>> But we need to be able to signal to the driver three states:
>>>
>>>
>>> 1. No errors, but there are still unstarted video devices
>>>
>>> 2. No errors and there are no unstarted video devices
>>>
>>> 3. An error
>>>
>>>
>>> So I expect a driver to do a two stage check:
>>>
>>>
>>> ret = video_device_pipeline_started(vdev);
>>>
>>> if (ret < 0)
>>>
>>> goto err_out;
>>>
>>> if (ret == 0)
>>>
>>> // something appropriate here like run the media jobs scheduler
>>>
>> Sorry: I had a reading comprehension failure. You were suggesting to use
>> -ENODEV to signal that there are unstarted video devices remaining. I
>> understand now, but I'm still not sure about it, because then instead of the
>> "if (ret == 0) check here we'd have "if (ret == -ENODEV)", which I don't
>> especially like...or am I missing some way to avoid having that check here?
> Yes, that would require drivers to check for -ENODEV to identify a
> non-failure case when not all devices have started..
>
> You're right it might not be optimal. With you implementation we
> should then have
>
> 0: success
> < 0: error
> > 0: not all devices started
>
> ?
Yes
>
> I might be actually ok with this, but could you please document it as
> I've below suggested for clarity ?
Yep, will do
>
> Thanks
> j
>
>>
>> Thanks
>>
>> Dan
>>
>>>>> +
>>>>> + return media_pipeline_started(pipe);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(video_device_pipeline_started);
>>>>> +
>>>>> +int video_device_pipeline_stopped(struct video_device *vdev)
>>>>> +{
>>>>> + struct media_pipeline *pipe;
>>>>> + int ret;
>>>>> +
>>>>> + pipe = video_device_pipeline(vdev);
>>>>> + if (!pipe)
>>>>> + return -ENODEV;
>>>>> +
>>>>> + ret = __video_device_pipeline_started(pipe);
>>>>> + if (ret)
>>>>> + return ret;
>>>> ditto
>>>>
>>>>> +
>>>>> + media_pipeline_stopped(pipe);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
>>>>> +
>>>>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>>>>
>>>>> /*
>>>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>>>> index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
>>>>> --- a/include/media/v4l2-dev.h
>>>>> +++ b/include/media/v4l2-dev.h
>>>>> @@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
>>>>> */
>>>>> struct media_pipeline *video_device_pipeline(struct video_device *vdev);
>>>>>
>>>>> +/**
>>>>> + * video_device_pipeline_started - Run the pipeline_started() entity operation
>>>>> + * for a fully-started media pipeline
>>>>> + * @vdev: A video device that's part of the pipeline
>>>>> + *
>>>>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>>>>> + * connected to a given video device through enabled links have been marked as
>>>> I would use the same text as the one from video_device_pipeline_start()
>>>>
>>>> " connected to a given video device through enabled links, either
>>>> directly or indirectly,"
>>>
>>> Ack
>>>
>>>>> + * streaming through the use of video_device_pipeline_start() or one of its
>>>>> + * equivalent functions. If so, media_pipeline_started() is called to inform
>>>>> + * entities in the pipeline of that fact. The intention is to provide drivers
>>>>> + * with a shortcut for checking whether their pipeline is fully ready to start
>>>>> + * processing data.
>>>> Not really a shortcut, I would use "mechanism" instead.
>>>>
>>>> I would also specify that:
>>>>
>>>> * entities in the pipeline of that fact. The intention is to provide drivers
>>>> * with a mechanism for checking whether their pipeline is fully ready to start
>>>> * processing data and call the .pipeline_started() media entity operation
>>>> * on all the entities in the pipeline.
>>> Ack!
>>>>> + *
>>>>> + * Return: The number of video devices in the pipeline remaining to be started,
>>>>> + * or a negative error number on failure.
>>>> 0 for success as well
>>>>
>>>> I would anyway return 0 for success and a specific error code for the
>>>> three failure cases:
>>>> -ENOMEM if allocating the iterator fails
>>>> -ENODEV if not all video devices have started
>>>> -EINVAL if media_pipeline_started() fails
>>>>
>>>> You can document them as (copying from iommu.h)
>>>>
>>>> * Return:
>>>> * * 0 - success
>>>> * * EINVAL - call to pipeline_started() failed
>>>> * * ENOMEM - failed to allocate pipe iterator
>>>> * * ENODEV - pipeline not yet fully started
>>>>
>>>>> + */
>>>>> +int video_device_pipeline_started(struct video_device *vdev);
>>>>> +
>>>>> +/**
>>>>> + * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
>>>>> + * for a fully-started media pipeline
>>>>> + * @vdev: A video device that's part of the pipeline
>>>>> + *
>>>>> + * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
>>>>> + * connected to a given video device through enabled links have been marked as
>>>>> + * streaming through the use of video_device_pipeline_start() or one of its
>>>> What is the intended semantic here ? The first video device to receive
>>>> a streamoff() will trigger media_pipeline_stopped() or should the last
>>>> one do that ?
>>> The first one should do it, so the first device caling stop should
>>> trigger actual stop in all involved hardware.
>>>>> + * equivalent functions. If so, media_pipeline_stopped() is called for each
>>>>> + * entity in the pipeline. The intention is to provide drivers with a shortcut
>>>>> + * for checking whether this video device is the first device in the pipeline
>>>>> + * to be stopped.
>>>>> + *
>>>>> + * Return: The number of video devices in the pipeline remaining to be started, or a
>>>>> + * negative error number on failure.
>>>>> + */
>>>>> +int video_device_pipeline_stopped(struct video_device *vdev);
>>>>> +
>>>>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>>>>
>>>>> #endif /* _V4L2_DEV_H */
>>>>>
>>>>> --
>>>>> 2.34.1
>>>>>
>>>>>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-07-14 6:37 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-04 11:20 [PATCH v3 0/5] Add Input Video Control Block driver for RZ/V2H Daniel Scally
2025-07-04 11:20 ` [PATCH v3 1/5] media: mc: entity: Add pipeline_started/stopped ops Daniel Scally
2025-07-08 13:29 ` Jacopo Mondi
2025-07-11 13:24 ` Dan Scally
2025-07-04 11:20 ` [PATCH v3 2/5] media: v4l2-dev: Add helpers to run media_pipeline_[started|stopped]() Daniel Scally
2025-07-08 13:10 ` Jacopo Mondi
2025-07-11 11:51 ` Dan Scally
2025-07-11 12:43 ` Dan Scally
2025-07-11 13:39 ` Jacopo Mondi
2025-07-14 6:37 ` Dan Scally
2025-07-04 11:20 ` [PATCH v3 3/5] dt-bindings: media: Add bindings for the RZ/V2H IVC block Daniel Scally
2025-07-07 6:39 ` Krzysztof Kozlowski
2025-07-04 11:20 ` [PATCH v3 4/5] media: platform: Add Renesas Input Video Control block driver Daniel Scally
2025-07-08 14:49 ` Jacopo Mondi
2025-07-08 14:57 ` Dan Scally
2025-07-08 15:51 ` Jacopo Mondi
2025-07-09 8:13 ` Dan Scally
2025-07-09 8:22 ` Jacopo Mondi
2025-07-09 9:12 ` Geert Uytterhoeven
2025-07-11 11:17 ` Dan Scally
2025-07-04 11:20 ` [PATCH v3 5/5] MAINTAINERS: Add entry for rzv2h-ivc driver Daniel Scally
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).