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
