Just two minor comments:
On 02/04/2024 02:04, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>
> Add a driver for the Unicam camera receiver block on BCM283x processors.
> It is represented as two video device nodes: unicam-image and
> unicam-embedded which are connected to an internal subdev (named
> unicam-subdev) in order to manage streams routing.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Co-developed-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> Changes since v8:
>
> - Use MIPI_CSI2_DT_* macros
> - Disable image stream on start error
> - Move hardware configuration to unicam_sd_enable_streams()
> - Get VC and DT from frame descriptor
> - Don't cache fmtinfo in unicam_node
> - Calculate line_int_freq based on subdev format
> - Fix try_fmt_meta regression from v5
>
> Changes since v7:
>
> - Indentation, line wrap and white space fixes
> - Add copyright notice for Ideas on Board
> - Use unsigned values for shifts in macro
> - Replace condition and assignment with max()
> - Don't set the serial to an empty string manually
> - Don't use loop for lane clocks setting
> - Store PUM value in struct unicam_format_info
>
> Changes since v6:
>
> - Fix typos in comments
> - Drop outdated comment
> - Indentation fixes
> - Turn UNICAM_SD_PAD_* into an enum
> - Drop unicam_format_info.metadata_fmt
> - Remove unneeded dev_dbg()
> - Report meta frame sizes as V4L2_FRMSIZE_TYPE_STEPWISE
> - Drop stray semicolons
> - Set V4L2_FMT_FLAG_META_LINE_BASED for metadata format
> - Rename error label
> - Use .get_mbus_config() to get number of data lanes
> - Drop minimum of 3 buffers in .queue_setup()
> - Merge locks for the two video nodes
> - Rework start/stop to avoid race conditions
>
> Changes since v5:
>
> - Move to drivers/media/platform/broadcom/
> - Port to the upstream V4L2 streams API
> - Rebase on latest metadata API proposal
> - Add missing error message
> - Drop unneeded documentation block for unicam_isr()
> - Drop unneeded dev_dbg() and dev_err() messages
> - Drop unneeded streams_mask and fmt checks
> - Drop unused unicam_sd_pad_is_sink()
> - Drop unneeded includes
> - Drop v4l2_ctrl_subscribe_event() call
> - Use pm_runtime_resume_and_get()
> - Indentation and line wrap fixes
> - Let the framework set bus_info
> - Use v4l2_fwnode_endpoint_parse()
> - Fix media device cleanup
> - Drop lane reordering checks
> - Fix subdev state locking
> - Drop extra debug messages
> - Move clock handling to runtime PM handlers
> - Reorder functions
> - Rename init functions for more clarity
> - Initialize runtime PM earlier
> - Clarify error messages
> - Simplify subdev init with local variable
> - Fix subdev cleanup
> - Fix typos and indentation
> - Don't initialize local variables needlessly
> - Simplify num lanes check
> - Fix metadata handling in subdev set_fmt
> - Drop manual fallback to .s_stream()
> - Pass v4l2_pix_format to unicam_calc_format_size_bpl()
> - Simplify unicam_set_default_format()
> - Fix default format settings
> - Add busy check in unicam_s_fmt_meta()
> - Add missing \n at end of format strings
> - Fix metadata handling in subdev set_fmt
> - Fix locking when starting streaming
> - Return buffers from start streaming fails
> - Fix format validation for metadata node
> - Use video_device_pipeline_{start,stop}() helpers
> - Simplify format enumeration
> - Drop unset variable
> - Update MAINTAINERS entry
> - Update to the upstream v4l2_async_nf API
> - Update to the latest subdev routing API
> - Update to the latest subdev state API
> - Move from subdev .init_cfg() to .init_state()
> - Update to the latest videobuf2 API
> - Fix v4l2_subdev_enable_streams() error check
> - Use correct pad for the connected subdev
> - Return buffers to vb2 when start streaming fails
> - Improve debugging in start streaming handler
> - Simplify DMA address management
> - Drop comment about bcm2835-camera driver
> - Clarify comments that explain min/max sizes
> - Pass v4l2_pix_format to unicam_try_fmt()
> - Drop unneeded local variables
> - Rename image-related constants and functions
> - Turn unicam_fmt.metadata_fmt into bool
> - Rename unicam_fmt to unicam_format_info
> - Rename unicam_format_info variables to fmtinfo
> - Rename unicam_node.v_fmt to fmt
> - Add metadata formats for RAW10, RAW12 and RAW14
> - Make metadata formats line-based
> - Validate format on metadata video device
> - Add Co-devlopped-by tags
>
> Changes since v3:
>
> - Add the vendor prefix for DT name
> - Use the reg-names in DT parsing
> - Remove MAINTAINERS entry
>
> Changes since v2:
>
> - Change code organization
> - Remove unused variables
> - Correct the fmt_meta functions
> - Rewrite the start/stop streaming
> - You can now start the image node alone, but not the metadata one
> - The buffers are allocated per-node
> - only the required stream is started, if the route exists and is
> enabled
> - Prefix the macros with UNICAM_ to not have too generic names
> - Drop colorspace support
>
> Changes since v1:
>
> - Replace the unicam_{info,debug,error} macros with dev_*()
> ---
> MAINTAINERS | 1 +
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/broadcom/Kconfig | 23 +
> drivers/media/platform/broadcom/Makefile | 3 +
> .../platform/broadcom/bcm2835-unicam-regs.h | 246 ++
> .../media/platform/broadcom/bcm2835-unicam.c | 2745 +++++++++++++++++
> 7 files changed, 3020 insertions(+)
> create mode 100644 drivers/media/platform/broadcom/Kconfig
> create mode 100644 drivers/media/platform/broadcom/Makefile
> create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c
>
<snip>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> new file mode 100644
> index 000000000000..1418f209d6ad
> --- /dev/null
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -0,0 +1,2745 @@
<snip>
> +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct unicam_node *node = vb2_get_drv_priv(vq);
> + struct unicam_device *unicam = node->dev;
> + struct unicam_buffer *buf;
> + struct media_pipeline_pad_iter iter;
> + struct media_pad *pad;
> + unsigned long flags;
> + int ret;
> +
> + dev_dbg(unicam->dev, "Starting stream on %s device\n",
> + is_metadata_node(node) ? "metadata" : "image");
> +
> + /*
> + * Start the pipeline. This validates all links, and populates the
> + * pipeline structure.
> + */
> + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe.pipe);
> + if (ret < 0) {
> + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret);
> + goto err_buffers;
> + }
> +
> + /*
> + * Determine which video nodes are included in the pipeline, and get the
> + * number of data lanes.
> + */
> + if (unicam->pipe.pipe.start_count == 1) {
> + unicam->pipe.nodes = 0;
> +
> + media_pipeline_for_each_pad(&unicam->pipe.pipe, &iter, pad) {
> + if (pad->entity != &unicam->subdev.sd.entity)
> + continue;
> +
> + if (pad->index == UNICAM_SD_PAD_SOURCE_IMAGE)
> + unicam->pipe.nodes |= BIT(UNICAM_IMAGE_NODE);
> + else if (pad->index == UNICAM_SD_PAD_SOURCE_METADATA)
> + unicam->pipe.nodes |= BIT(UNICAM_METADATA_NODE);
> + }
> +
> + if (!(unicam->pipe.nodes & BIT(UNICAM_IMAGE_NODE))) {
> + dev_dbg(unicam->dev,
> + "Pipeline does not include image node\n");
> + ret = -EPIPE;
> + goto err_pipeline;
> + }
> +
> + ret = unicam_num_data_lanes(unicam);
> + if (ret < 0)
> + goto err_pipeline;
> +
> + unicam->pipe.num_data_lanes = ret;
> +
> + dev_dbg(unicam->dev, "Running with %u data lanes, nodes %u\n",
> + unicam->pipe.num_data_lanes, unicam->pipe.nodes);
> + }
> +
> + node->streaming = true;
Hmm, do you need to keep track of this here? Can't you use vb2_start_streaming_called()?
Generally I dislike keeping track of the same information in two places.
> +
> + /* Arm the node with the first buffer from the DMA queue. */
> + spin_lock_irqsave(&node->dma_queue_lock, flags);
> + buf = list_first_entry(&node->dma_queue, struct unicam_buffer, list);
> + node->cur_frm = buf;
> + node->next_frm = buf;
> + list_del(&buf->list);
> + spin_unlock_irqrestore(&node->dma_queue_lock, flags);
> +
> + /*
> + * Wait for all the video devices in the pipeline to have been started
> + * before starting the hardware. In the general case, this would
> + * prevent capturing multiple streams independently. However, the
> + * Unicam DMA engines are not generic, they have been designed to
> + * capture image data and embedded data from the same camera sensor.
> + * Not only does the main use case not benefit from independent
> + * capture, it requires proper synchronization of the streams at start
> + * time.
> + */
> + if (unicam->pipe.pipe.start_count < hweight32(unicam->pipe.nodes))
> + return 0;
> +
> + ret = pm_runtime_resume_and_get(unicam->dev);
> + if (ret < 0) {
> + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret);
> + goto err_pipeline;
> + }
> +
> + /* Enable the streams on the source. */
> + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_IMAGE,
> + BIT(0));
> + if (ret < 0) {
> + dev_err(unicam->dev, "stream on failed in subdev\n");
> + goto err_pm_put;
> + }
> +
> + if (unicam->pipe.nodes & BIT(UNICAM_METADATA_NODE)) {
> + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_METADATA,
> + BIT(0));
> + if (ret < 0) {
> + dev_err(unicam->dev, "stream on failed in subdev\n");
> + goto err_disable_streams;
> + }
> + }
> +
> + return 0;
> +
> +err_disable_streams:
> + v4l2_subdev_disable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_IMAGE, BIT(0));
> +err_pm_put:
> + pm_runtime_put_sync(unicam->dev);
> +err_pipeline:
> + video_device_pipeline_stop(&node->video_dev);
> +err_buffers:
> + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED);
> + node->streaming = false;
> + return ret;
> +}
<snip>
> +static void unicam_unregister_nodes(struct unicam_device *unicam)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> + struct unicam_node *node = &unicam->node[i];
> +
> + if (node->dummy_buf_cpu_addr)
> + dma_free_coherent(unicam->dev, node->dummy_buf.size,
> + node->dummy_buf_cpu_addr,
> + node->dummy_buf.dma_addr);
> +
> + if (node->registered) {
> + video_unregister_device(&node->video_dev);
Call vb2_video_unregister_device instead of video_unregister_device.
That ensures that unregistering the device will also stop streaming.
See comments in include/media/videobuf2-v4l2.h.
> + node->registered = false;
> + }
> + }
> +}
<snip>
Regards,
Hans
