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