From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AC94512839D; Thu, 4 Apr 2024 13:53:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712238841; cv=none; b=BCmO4Fug8+z7dNDd1IvPDnVJ1OqVhm2CKtcz5mWFrJMHelF1I3xG13CH44H8ex4kU/61WXiALTmhcSN0GVmSmUPcjLkcicDsXVYqm3lf7GtOOeWR2eOI8f51yiQBF4n7a56tjFbFChf/Ew1PhAEOl5x9o1xE8+EbvWO6gujvEbc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712238841; c=relaxed/simple; bh=rHGwoKG09JOiSfZHMXZ5zTOUqRGK+5ViCwByZmRhBC8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=VVQxe6VT2k2WxnsbMaZgKIz5CByQmqQTfiU7YvGzLV8jLslXzCoSwOJ3IVRKll+B+Dt+DnZDxELF5FWiNNH6gCTfJL890zBhS+QfdUiQkvrrCORMnRqzbBZ/SgOOc8nFJHOoAeqGZrbCliAWeVjeq3RZfmUrE8nNR6Zz9q3vxBY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=ukCnlCA1; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ukCnlCA1" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id BABE21A2; Thu, 4 Apr 2024 15:53:19 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1712238799; bh=rHGwoKG09JOiSfZHMXZ5zTOUqRGK+5ViCwByZmRhBC8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ukCnlCA18C9A3PJMUFiAZbCtNS7P877Mnm3qCnTQQVb5mx+hByjlkmdrXzE6pNYRR YJWIBWv9qBNriaI8oKqifnBGbwW4KaV7Fp8/G21dOYNyF7R+uUSBEnJIfIe7ZQ4vp0 y7VTHYAtom3sUwhVKvMbKofEci68GDK/AMbyGgzI= Date: Thu, 4 Apr 2024 16:53:46 +0300 From: Laurent Pinchart To: Hans Verkuil Cc: linux-media@vger.kernel.org, Dave Stevenson , David Plowman , Jean-Michel Hautbois , Naushir Patuck , Sakari Ailus , kernel-list@raspberrypi.com, linux-rpi-kernel@lists.infradead.org, Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list@broadcom.com, Conor Dooley , Krzysztof Kozlowski , Rob Herring , devicetree@vger.kernel.org Subject: Re: [PATCH v9 04/10] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface Message-ID: <20240404135346.GA12590@pendragon.ideasonboard.com> References: <20240402000424.4650-1-laurent.pinchart@ideasonboard.com> <20240402000424.4650-5-laurent.pinchart@ideasonboard.com> <11107704-46a7-4228-99da-55389e210553@xs4all.nl> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <11107704-46a7-4228-99da-55389e210553@xs4all.nl> 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 > > > > 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 > > Co-developed-by: Naushir Patuck > > Signed-off-by: Naushir Patuck > > Co-developed-by: Jean-Michel Hautbois > > Signed-off-by: Jean-Michel Hautbois > > Co-developed-by: Laurent Pinchart > > Signed-off-by: Laurent Pinchart > > --- > > 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 > > > > > > > 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 @@ > > > > > +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; > > +} > > > > > +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; > > + } > > + } > > +} > > -- Regards, Laurent Pinchart