Hi Hans, On Thu, Apr 04, 2024 at 03:36:12PM +0200, Hans Verkuil wrote: > Just two minor comments: For a new large driver, I'll take this as a sign we're very close to completion :-) > 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. It looks like I can, I'll give it a try for the next version. > > + > > + /* 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. I had forgotten about that function. I'll do so. > > + node->registered = false; > > + } > > + } > > +} > > <snip> -- Regards, Laurent Pinchart