Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

Hi Benoit,

On Mon, Aug 07, 2017 at 02:24:24PM -0500, Benoit Parrot wrote:

quoted

+#define CSI2RX_STREAMS_MAX	4
+

Just to confirm here that "streams" in this case are equivalent to
"Virtual Channel", correct?

Kind of, yes, but it's a bit more complicated than that. The streams
are the output of the CSI2-RX block, so the interface to the capture
devices.

You can associate any CSI-2 virtual channel (in input) to any stream
(in output). However, as we don't have a way to change that routing at
runtime using v4l2 (yet [1]), the driver currently hardcode that
routing to have CSI-2 VC0 sent to stream 0, VC1 to stream 1, etc.

quoted

+static void csi2rx_reset(struct csi2rx_priv *csi2rx)
+{
+	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
+	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+	udelay(10);

Shouldn't we use usleep_range() instead?

We totally should.

quoted

+static int csi2rx_stop(struct csi2rx_priv *csi2rx)
+{
+	int i;
+
+	for (i = 0; i < csi2rx->max_streams; i++)
+		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+	return 0;
+}
+

Here, it is entirely possible that the "dma/buffer" engine which will
make use of this receiver creates separates video nodes for each streams.
In which case, you could theoretically have multiple user space capture
on-going.
But the "start" and "stop" method above would disrupt any of the other
stream. Unless you start and stop all 4 capture streams in lock step.

Eventually, the sub device might be a port aggregator which has up to
4 sensors on the source pad and feed each camera traffic on its own
Virtual Channel.

I know there isn't support in the framework for this currently but it
is something to think about.

I guess you could make the same argument if you have several engines,
each one connected to a stream.

One way to work around that could be to add some reference counting in
the start and stop functions, and keep enabling all the outputs.

This wouldn't solve the underlying issue that all the stream would be
enabled and we don't really have a way to tell which one we want to
enable, but at least we wouldn't disrupt active streams when the first
one goes away.

Maxime

1: https://www.mail-archive.com/linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg116955.html

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com