Re: [PATCH v9 04/10] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux