RE: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver

Hi Luca,

Thanks for reviewing!

-----Original Message-----
From: Luca Ceresoli <luca@lucaceresoli.net>
Sent: Monday, May 25, 2020 6:44 PM
To: Vishal Sagar <redacted>; Hyun Kwon <redacted>;
laurent.pinchart@ideasonboard.com; mchehab@kernel.org;
robh+dt@kernel.org; mark.rutland@arm.com; Michal Simek
[off-list ref]; linux-media@vger.kernel.org;
devicetree@vger.kernel.org; hans.verkuil@cisco.com; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Dinesh Kumar
[off-list ref]; Sandip Kothari [off-list ref]; Jacopo Mondi
[off-list ref]
Cc: Hyun Kwon <redacted>
Subject: Re: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
Subsystem driver

Hi Vishal,

thanks. I have only a few minor nitpicking comments.

On 12/05/20 17:19, Vishal Sagar wrote:

quoted

The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
for image processing. Please refer to PG232 for details.

The CSI2 Rx controller filters out all packets except for the packets
with data type fixed in hardware. RAW8 packets are always allowed to
pass through.

It is also used to setup and handle interrupts and enable the core. It
logs all the events in respective counters between streaming on and off.

The driver supports only the video format bridge enabled configuration.
Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
the CSI v2.0 feature is enabled in design. When the VCX feature is
enabled, the maximum number of virtual channels becomes 16 from 4.

Signed-off-by: Vishal Sagar <redacted>
Reviewed-by: Hyun Kwon <redacted>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[...]

quoted

+static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state) {
+	int ret = 0;
+
+	/* enable core */
+	xcsi2rxss_set(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
+
+	ret = xcsi2rxss_soft_reset(state);
+	if (ret < 0) {

'if (ret)' is enough, it's a classic nonzero-on-error return value.

Agreed. I will fix it in next version.

quoted

+/**
+ * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
+ * @irq: IRQ number
+ * @data: Pointer to device state
+ *
+ * In the interrupt handler, a list of event counters are updated for
+ * corresponding interrupts. This is useful to get status / debug.
+ *
+ * Return: IRQ_HANDLED after handling interrupts  */ static
+irqreturn_t xcsi2rxss_irq_handler(int irq, void *data) {
+	struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)data;
+	struct device *dev = state->dev;
+	u32 status;
+
+	status = xcsi2rxss_read(state, XCSI_ISR_OFFSET) &

XCSI_ISR_ALLINTR_MASK;

quoted

+	xcsi2rxss_write(state, XCSI_ISR_OFFSET, status);
+
+	/* Received a short packet */
+	if (status & XCSI_ISR_SPFIFONE) {
+		u32 count = 0;
+
+		/*
+		 * Drain generic short packet FIFO by reading max 31
+		 * (fifo depth) short packets from fifo or till fifo is empty.
+		 */
+		for (count = 0; count < XCSI_SPKT_FIFO_DEPTH; ++count) {
+			u32 spfifostat, spkt;
+
+			spkt = xcsi2rxss_read(state, XCSI_SPKTR_OFFSET);
+			dev_dbg(dev, "Short packet = 0x%08x\n", spkt);
+			spfifostat = xcsi2rxss_read(state, XCSI_ISR_OFFSET);
+			spfifostat &= XCSI_ISR_SPFIFONE;
+			if (!spfifostat)
+				break;
+			xcsi2rxss_write(state, XCSI_ISR_OFFSET, spfifostat);
+		}
+	}
+
+	/* Short packet FIFO overflow */
+	if (status & XCSI_ISR_SPFIFOF)
+		dev_dbg_ratelimited(dev, "Short packet FIFO overflowed\n");
+
+	/*
+	 * Stream line buffer full
+	 * This means there is a backpressure from downstream IP
+	 */
+	if (status & XCSI_ISR_SLBF) {
+		dev_alert_ratelimited(dev, "Stream Line Buffer Full!\n");
+
+		/* disable interrupts */
+		xcsi2rxss_clr(state, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
+		xcsi2rxss_clr(state, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
+
+		/* disable core */
+		xcsi2rxss_clr(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
+		state->streaming = false;
+
+		/*
+		 * The IP needs to be hard reset before it can be used now.
+		 * This will be done in streamoff.
+		 */
+
+		/*
+		 * TODO: Notify the whole pipeline with v4l2_subdev_notify()

to

quoted

+		 * inform userspace.
+		 */
+	}
+
+	/* Increment event counters */
+	if (status & XCSI_ISR_ALLINTR_MASK) {
+		unsigned int i;
+
+		for (i = 0; i < XCSI_NUM_EVENTS; i++) {
+			if (!(status & xcsi2rxss_events[i].mask))
+				continue;
+			state->events[i]++;
+			dev_dbg_ratelimited(dev, "%s: %u\n",
+					    xcsi2rxss_events[i].name,
+					    state->events[i]);
+		}
+
+		if (status & XCSI_ISR_VCXFE && state->en_vcx) {
+			u32 vcxstatus;
+
+			vcxstatus = xcsi2rxss_read(state, XCSI_VCXR_OFFSET);
+			vcxstatus &= XCSI_VCXR_VCERR;
+			for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
+				if (!(vcxstatus & (1 << i)))

You can use BIT(i) instead of (1 << i).

Yep that is a good alternative.

quoted

+/**
+ * xcsi2rxss_set_format - This is used to set the pad format
+ * @sd: Pointer to V4L2 Sub device structure
+ * @cfg: Pointer to sub device pad information structure
+ * @fmt: Pointer to pad level media bus format
+ *
+ * This function is used to set the pad format. Since the pad format
+is fixed
+ * in hardware, it can't be modified on run time. So when a format
+set is
+ * requested by application, all parameters except the format type is
+saved
+ * for the pad and the original pad format is sent back to the application.
+ *
+ * Return: 0 on success
+ */
+static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_format *fmt)
+{
+	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
+	struct v4l2_mbus_framefmt *__format;
+	u32 dt;
+
+	/* only sink pad format can be updated */

This comment should be placed...

quoted

+	mutex_lock(&xcsi2rxss->lock);
+
+	/*
+	 * Only the format->code parameter matters for CSI as the
+	 * CSI format cannot be changed at runtime.
+	 * Ensure that format to set is copied to over to CSI pad format
+	 */
+	__format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
+					      fmt->pad, fmt->which);
+

...here.

Ok will move the comment here.

quoted

+	if (fmt->pad == XVIP_PAD_SOURCE) {
+		fmt->format = *__format;
+		mutex_unlock(&xcsi2rxss->lock);
+		return 0;
+	}
+
+	/*
+	 * RAW8 is supported in all datatypes. So if requested media bus

format

quoted

+	 * is of RAW8 type, then allow to be set. In case core is configured to
+	 * other RAW, YUV422 8/10 or RGB888, set appropriate media bus

format.

quoted

+	 */
+	dt = xcsi2rxss_get_dt(fmt->format.code);
+	if (dt != xcsi2rxss->datatype && dt != XCSI_DT_RAW8) {
+		dev_dbg(xcsi2rxss->dev, "Unsupported media bus format");
+		/* set the default format for the data type */
+		fmt->format.code = xcsi2rxss_get_nth_mbus(xcsi2rxss-
datatype,
+							  0);
+	}
+
+	*__format = fmt->format;
+	mutex_unlock(&xcsi2rxss->lock);
+
+	return 0;
+}
+
+/*
+ * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
+ * @sd : pointer to v4l2 subdev structure
+ * @cfg: V4L2 subdev pad configuration
+ * @code : pointer to v4l2_subdev_mbus_code_enum structure

Remove space before colon here.

Looks good otherwise, and my comments are minor details so:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

That’s great. Thanks for reviewing this again.

I tried to runtime test this driver as well replacing the v10 driver that I'm using
at the moment, but ran into many problems, apparently in the media entity
navigation. The diff between v10 and v13 does not justify these problems, so
I'm assuming v13 needs a more recent kernel than the 4.19 I'm currentl stuck
on.

--
Luca Ceresoli

Regards
Vishal Sagar
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel