[PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

For v4:
- fixes the warning reported by the kernel test robot
- tiny code change to enable the vc functionality with the partially-applied
  multistream patches on linux-next (tested on tag:next-20221010)

For v3:
- setting the sink pad format on the CSID entity will now propagate the
  format to the source pads to keep the subdev in a valid internal state.
- code syntax improvements

For v2:
- code syntax improvements
- The info print for the enabled VCs was demoted to a dbg print. Can be
  enabled with dynamic debug, e.g.:
echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control

NOTE: These changes depend on the multistream series, that as of yet
is still not merged upstream. However, part of the
multistream patches are merged in linux-next (tested on
tag:next-20221010), including the patch that introduces the
video_device_pipeline_alloc_start() functionality. This allows 
applying and using this series on linux-next without applying the
complete multistream set.

The CSID hardware on SM8250 can demux the input data stream into
maximum of 4 multiple streams depending on virtual channel (vc)
or data type (dt) configuration.

Situations in which demuxing is useful:
- HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
  (the setup we used for testing, with the imx412 sensor),
  or a 3-frame HDR output - light, medium-lit, dark frame.
- sensors with additional metadata that is streamed over a different
  virtual channel/datatype.
- sensors that produce frames with multiple resolutions in the same pixel
  data stream

With these changes, the CSID entity has, as it did previously, a single
sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
virtual channel configuration is determined by which of the source ports
are linked to an output VFE line. For example, the link below will
configure the CSID driver to enable vc 0 and vc 1:

media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'

which will be demuxed and propagated into /dev/video0
and /dev/video1 respectively. With this, the userspace can use
any normal V4L2 client app to start/stop/queue/dequeue from these
video nodes. Tested with the yavta app.

The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
msm_vfe0_rdi1,...) must also be configured explicitly.

Note that in order to keep a valid internal subdevice state,
setting the sink pad format of the CSID subdevice will propagate
this format to the source pads. However, since the CSID hardware
can demux the input stream into several streams each of which can 
be a different format, in that case each source pad's 
format must be set individually, e.g.:

media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'

Milen Mitkov (4):
  media: camss: sm8250: Virtual channels for CSID
  media: camss: vfe: Reserve VFE lines on stream start and link to CSID
  media: camss: vfe-480: Multiple outputs support for SM8250
  media: camss: sm8250: Pipeline starting and stopping for multiple
    virtual channels

 .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
 .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
 .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
 .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
 drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
 .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
 drivers/media/platform/qcom/camss/camss.c     |  2 +-
 7 files changed, 140 insertions(+), 60 deletions(-)

-- 
2.37.3

[PATCH v4 1/4] media: camss: sm8250: Virtual channels for CSID

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

CSID hardware on SM8250 can demux up to 4 simultaneous streams
based on virtual channel (vc) or datatype (dt).
The CSID subdevice entity now has 4 source ports that can be
enabled/disabled and thus can control which virtual channels
are enabled. Datatype demuxing not tested.

In order to keep a valid internal state of the subdevice,
implicit format propagation from the sink to the source pads
has been preserved. However, the format on each source pad
can be different and in that case it must be configured explicitly.

CSID's s_stream is called when any stream is started or stopped.
It will call configure_streams() that will rewrite IRQ settings to HW.
When multiple streams are running simultaneously there is an issue
when writing IRQ settings for one stream while another is still
running, thus avoid re-writing settings if they were not changed
in link setup, or by fully powering off the CSID hardware.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
---
 .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++++-------
 .../media/platform/qcom/camss/camss-csid.c    | 44 ++++++++++-----
 .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
 3 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 2031bde13a93..0f8ac29d038d 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -334,13 +334,14 @@ static const struct csid_format csid_formats[] = {
 	},
 };
 
-static void csid_configure_stream(struct csid_device *csid, u8 enable)
+static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
 {
 	struct csid_testgen_config *tg = &csid->testgen;
 	u32 val;
 	u32 phy_sel = 0;
 	u8 lane_cnt = csid->phy.lane_cnt;
-	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_SRC];
+	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
+	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
 	const struct csid_format *format = csid_get_fmt_entry(csid->formats, csid->nformats,
 							      input_format->code);
 
@@ -351,8 +352,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
 		phy_sel = csid->phy.csiphy_id;
 
 	if (enable) {
-		u8 vc = 0; /* Virtual Channel 0 */
-		u8 dt_id = vc * 4;
+		u8 dt_id = vc;
 
 		if (tg->enabled) {
 			/* Config Test Generator */
@@ -395,42 +395,42 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
 		val |= format->data_type << RDI_CFG0_DATA_TYPE;
 		val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
 		val |= dt_id << RDI_CFG0_DT_ID;
-		writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));
+		writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
 
 		/* CSID_TIMESTAMP_STB_POST_IRQ */
 		val = 2 << RDI_CFG1_TIMESTAMP_STB_SEL;
-		writel_relaxed(val, csid->base + CSID_RDI_CFG1(0));
+		writel_relaxed(val, csid->base + CSID_RDI_CFG1(vc));
 
 		val = 1;
-		writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(0));
+		writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(vc));
 
 		val = 0;
-		writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PATTERN(0));
+		writel_relaxed(val, csid->base + CSID_RDI_FRM_DROP_PATTERN(vc));
 
 		val = 1;
-		writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(0));
+		writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
 
 		val = 0;
-		writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(0));
+		writel_relaxed(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
 
 		val = 1;
-		writel_relaxed(val, csid->base + CSID_RDI_RPP_PIX_DROP_PERIOD(0));
+		writel_relaxed(val, csid->base + CSID_RDI_RPP_PIX_DROP_PERIOD(vc));
 
 		val = 0;
-		writel_relaxed(val, csid->base + CSID_RDI_RPP_PIX_DROP_PATTERN(0));
+		writel_relaxed(val, csid->base + CSID_RDI_RPP_PIX_DROP_PATTERN(vc));
 
 		val = 1;
-		writel_relaxed(val, csid->base + CSID_RDI_RPP_LINE_DROP_PERIOD(0));
+		writel_relaxed(val, csid->base + CSID_RDI_RPP_LINE_DROP_PERIOD(vc));
 
 		val = 0;
-		writel_relaxed(val, csid->base + CSID_RDI_RPP_LINE_DROP_PATTERN(0));
+		writel_relaxed(val, csid->base + CSID_RDI_RPP_LINE_DROP_PATTERN(vc));
 
 		val = 0;
-		writel_relaxed(val, csid->base + CSID_RDI_CTRL(0));
+		writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
 
-		val = readl_relaxed(csid->base + CSID_RDI_CFG0(0));
+		val = readl_relaxed(csid->base + CSID_RDI_CFG0(vc));
 		val |=  1 << RDI_CFG0_ENABLE;
-		writel_relaxed(val, csid->base + CSID_RDI_CFG0(0));
+		writel_relaxed(val, csid->base + CSID_RDI_CFG0(vc));
 	}
 
 	if (tg->enabled) {
@@ -456,7 +456,16 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
 		val = HALT_CMD_RESUME_AT_FRAME_BOUNDARY << RDI_CTRL_HALT_CMD;
 	else
 		val = HALT_CMD_HALT_AT_FRAME_BOUNDARY << RDI_CTRL_HALT_CMD;
-	writel_relaxed(val, csid->base + CSID_RDI_CTRL(0));
+	writel_relaxed(val, csid->base + CSID_RDI_CTRL(vc));
+}
+
+static void csid_configure_stream(struct csid_device *csid, u8 enable)
+{
+	u8 i;
+	/* Loop through all enabled VCs and configure stream for each */
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i))
+			__csid_configure_stream(csid, enable, i);
 }
 
 static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
@@ -502,6 +511,7 @@ static irqreturn_t csid_isr(int irq, void *dev)
 	struct csid_device *csid = dev;
 	u32 val;
 	u8 reset_done;
+	int i;
 
 	val = readl_relaxed(csid->base + CSID_TOP_IRQ_STATUS);
 	writel_relaxed(val, csid->base + CSID_TOP_IRQ_CLEAR);
@@ -510,8 +520,12 @@ static irqreturn_t csid_isr(int irq, void *dev)
 	val = readl_relaxed(csid->base + CSID_CSI2_RX_IRQ_STATUS);
 	writel_relaxed(val, csid->base + CSID_CSI2_RX_IRQ_CLEAR);
 
-	val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(0));
-	writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(0));
+	/* Read and clear IRQ status for each enabled RDI channel */
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			val = readl_relaxed(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
+			writel_relaxed(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
+		}
 
 	val = 1 << IRQ_CMD_CLEAR;
 	writel_relaxed(val, csid->base + CSID_IRQ_CMD);
diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 88f188e0f750..20c22564e0f9 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -209,6 +209,8 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 		}
 
 		csid->ops->hw_version(csid);
+
+		csid->phy.need_vc_update = true;
 	} else {
 		disable_irq(csid->irq);
 		camss_disable_clocks(csid->nclocks, csid->clock);
@@ -249,7 +251,10 @@ static int csid_set_stream(struct v4l2_subdev *sd, int enable)
 			return -ENOLINK;
 	}
 
-	csid->ops->configure_stream(csid, enable);
+	if (csid->phy.need_vc_update) {
+		csid->ops->configure_stream(csid, enable);
+		csid->phy.need_vc_update = false;
+	}
 
 	return 0;
 }
@@ -460,6 +465,7 @@ static int csid_set_format(struct v4l2_subdev *sd,
 {
 	struct csid_device *csid = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *format;
+	int i;
 
 	format = __csid_get_format(csid, sd_state, fmt->pad, fmt->which);
 	if (format == NULL)
@@ -468,14 +474,14 @@ static int csid_set_format(struct v4l2_subdev *sd,
 	csid_try_format(csid, sd_state, fmt->pad, &fmt->format, fmt->which);
 	*format = fmt->format;
 
-	/* Propagate the format from sink to source */
+	/* Propagate the format from sink to source pads */
 	if (fmt->pad == MSM_CSID_PAD_SINK) {
-		format = __csid_get_format(csid, sd_state, MSM_CSID_PAD_SRC,
-					   fmt->which);
+		for (i = MSM_CSID_PAD_FIRST_SRC; i < MSM_CSID_PADS_NUM; ++i) {
+			format = __csid_get_format(csid, sd_state, i, fmt->which);
 
-		*format = fmt->format;
-		csid_try_format(csid, sd_state, MSM_CSID_PAD_SRC, format,
-				fmt->which);
+			*format = fmt->format;
+			csid_try_format(csid, sd_state, i, format, fmt->which);
+		}
 	}
 
 	return 0;
@@ -738,7 +744,6 @@ static int csid_link_setup(struct media_entity *entity,
 		struct csid_device *csid;
 		struct csiphy_device *csiphy;
 		struct csiphy_lanes_cfg *lane_cfg;
-		struct v4l2_subdev_format format = { 0 };
 
 		sd = media_entity_to_v4l2_subdev(entity);
 		csid = v4l2_get_subdevdata(sd);
@@ -761,11 +766,22 @@ static int csid_link_setup(struct media_entity *entity,
 		lane_cfg = &csiphy->cfg.csi2->lane_cfg;
 		csid->phy.lane_cnt = lane_cfg->num_data;
 		csid->phy.lane_assign = csid_get_lane_assign(lane_cfg);
+	}
+	/* Decide which virtual channels to enable based on which source pads are enabled */
+	if (local->flags & MEDIA_PAD_FL_SOURCE) {
+		struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+		struct csid_device *csid = v4l2_get_subdevdata(sd);
+		struct device *dev = csid->camss->dev;
+
+		if (flags & MEDIA_LNK_FL_ENABLED)
+			csid->phy.en_vc |= BIT(local->index - 1);
+		else
+			csid->phy.en_vc &= ~BIT(local->index - 1);
 
-		/* Reset format on source pad to sink pad format */
-		format.pad = MSM_CSID_PAD_SRC;
-		format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-		csid_set_format(&csid->subdev, NULL, &format);
+		csid->phy.need_vc_update = true;
+
+		dev_dbg(dev, "%s: Enabled CSID virtual channels mask 0x%x\n",
+			__func__, csid->phy.en_vc);
 	}
 
 	return 0;
@@ -816,6 +832,7 @@ int msm_csid_register_entity(struct csid_device *csid,
 	struct v4l2_subdev *sd = &csid->subdev;
 	struct media_pad *pads = csid->pads;
 	struct device *dev = csid->camss->dev;
+	int i;
 	int ret;
 
 	v4l2_subdev_init(sd, &csid_v4l2_ops);
@@ -852,7 +869,8 @@ int msm_csid_register_entity(struct csid_device *csid,
 	}
 
 	pads[MSM_CSID_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
-	pads[MSM_CSID_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
+	for (i = MSM_CSID_PAD_FIRST_SRC; i < MSM_CSID_PADS_NUM; ++i)
+		pads[i].flags = MEDIA_PAD_FL_SOURCE;
 
 	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	sd->entity.ops = &csid_media_ops;
diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index f06040e44c51..d4b48432a097 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -19,8 +19,13 @@
 #include <media/v4l2-subdev.h>
 
 #define MSM_CSID_PAD_SINK 0
-#define MSM_CSID_PAD_SRC 1
-#define MSM_CSID_PADS_NUM 2
+#define MSM_CSID_PAD_FIRST_SRC 1
+#define MSM_CSID_PADS_NUM 5
+
+#define MSM_CSID_PAD_SRC (MSM_CSID_PAD_FIRST_SRC)
+
+/* CSID hardware can demultiplex up to 4 outputs */
+#define MSM_CSID_MAX_SRC_STREAMS	4
 
 #define DATA_TYPE_EMBEDDED_DATA_8BIT	0x12
 #define DATA_TYPE_YUV420_8BIT		0x18
@@ -81,6 +86,8 @@ struct csid_phy_config {
 	u8 csiphy_id;
 	u8 lane_cnt;
 	u32 lane_assign;
+	u32 en_vc;
+	u8 need_vc_update;
 };
 
 struct csid_device;
-- 
2.37.3

[PATCH v4 2/4] media: camss: vfe: Reserve VFE lines on stream start and link to CSID

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

For multiple virtual channels support, each VFE line can be in either
ON, RESERVED or OFF states. This allows the starting and stopping
of a VFE line independently of other active VFE lines.

Also, link the CSID entity's source ports to corresponding VFE lines.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe.c | 7 +++++++
 drivers/media/platform/qcom/camss/camss.c     | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index a26e4a5d87b6..cd8ac0478cf1 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -738,8 +738,10 @@ static int vfe_set_stream(struct v4l2_subdev *sd, int enable)
 	struct vfe_line *line = v4l2_get_subdevdata(sd);
 	struct vfe_device *vfe = to_vfe(line);
 	int ret;
+	int i;
 
 	if (enable) {
+		line->output.state = VFE_OUTPUT_RESERVED;
 		ret = vfe->ops->vfe_enable(line);
 		if (ret < 0)
 			dev_err(vfe->camss->dev,
@@ -749,6 +751,11 @@ static int vfe_set_stream(struct v4l2_subdev *sd, int enable)
 		if (ret < 0)
 			dev_err(vfe->camss->dev,
 				"Failed to disable vfe outputs\n");
+
+		/* At least one VFE line remains, return -EBUSY to avoid premature pipeline stop */
+		for (i = 0; i < vfe->line_num; i++)
+			if (vfe->line[i].output.state != VFE_OUTPUT_OFF)
+				return -EBUSY;
 	}
 
 	return ret;
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1118c40886d5..63653ac3e056 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1320,7 +1320,7 @@ static int camss_register_entities(struct camss *camss)
 					struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;
 
 					ret = media_create_pad_link(&csid->entity,
-								    MSM_CSID_PAD_SRC,
+								    MSM_CSID_PAD_FIRST_SRC + j,
 								    &vfe->entity,
 								    MSM_VFE_PAD_SINK,
 								    0);
-- 
2.37.3

[PATCH v4 3/4] media: camss: vfe-480: Multiple outputs support for SM8250

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

On SM8250 each VFE supports at least 3 RDI channels, or 4
in case of VFE-Lite, so add appropriate IRQ setup and handling.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index 129585110393..88e4dc71ec6c 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -94,6 +94,8 @@ static inline int bus_irq_mask_0_comp_done(struct vfe_device *vfe, int n)
 #define RDI_WM(n)			((IS_LITE ? 0 : 23) + (n))
 #define RDI_COMP_GROUP(n)		((IS_LITE ? 0 : 11) + (n))
 
+#define MAX_VFE_OUTPUT_LINES	4
+
 static u32 vfe_hw_version(struct vfe_device *vfe)
 {
 	u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
@@ -171,12 +173,26 @@ static inline void vfe_reg_update_clear(struct vfe_device *vfe,
 
 static void vfe_enable_irq_common(struct vfe_device *vfe)
 {
-	/* enable only the IRQs used: rup and comp_done irqs for RDI0 */
+	/* enable reset ack IRQ and top BUS status IRQ */
 	writel_relaxed(IRQ_MASK_0_RESET_ACK | IRQ_MASK_0_BUS_TOP_IRQ,
 		       vfe->base + VFE_IRQ_MASK(0));
-	writel_relaxed(BUS_IRQ_MASK_0_RDI_RUP(vfe, 0) |
-		       BUS_IRQ_MASK_0_COMP_DONE(vfe, RDI_COMP_GROUP(0)),
-		       vfe->base + VFE_BUS_IRQ_MASK(0));
+}
+
+static void vfe_enable_lines_irq(struct vfe_device *vfe)
+{
+	int i;
+	u32 bus_irq_mask = 0;
+
+	for (i = 0; i < MAX_VFE_OUTPUT_LINES; i++) {
+		/* Enable IRQ for newly added lines, but also keep already running lines's IRQ */
+		if (vfe->line[i].output.state == VFE_OUTPUT_RESERVED ||
+		    vfe->line[i].output.state == VFE_OUTPUT_ON) {
+			bus_irq_mask |= BUS_IRQ_MASK_0_RDI_RUP(vfe, i)
+					| BUS_IRQ_MASK_0_COMP_DONE(vfe, RDI_COMP_GROUP(i));
+			}
+	}
+
+	writel_relaxed(bus_irq_mask, vfe->base + VFE_BUS_IRQ_MASK(0));
 }
 
 static void vfe_isr_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id);
@@ -193,6 +209,7 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 {
 	struct vfe_device *vfe = dev;
 	u32 status;
+	int i;
 
 	status = readl_relaxed(vfe->base + VFE_IRQ_STATUS(0));
 	writel_relaxed(status, vfe->base + VFE_IRQ_CLEAR(0));
@@ -207,11 +224,14 @@ static irqreturn_t vfe_isr(int irq, void *dev)
 		writel_relaxed(status, vfe->base + VFE_BUS_IRQ_CLEAR(0));
 		writel_relaxed(1, vfe->base + VFE_BUS_IRQ_CLEAR_GLOBAL);
 
-		if (status & BUS_IRQ_MASK_0_RDI_RUP(vfe, 0))
-			vfe_isr_reg_update(vfe, 0);
+		/* Loop through all WMs IRQs */
+		for (i = 0; i < MSM_VFE_IMAGE_MASTERS_NUM; i++) {
+			if (status & BUS_IRQ_MASK_0_RDI_RUP(vfe, i))
+				vfe_isr_reg_update(vfe, i);
 
-		if (status & BUS_IRQ_MASK_0_COMP_DONE(vfe, RDI_COMP_GROUP(0)))
-			vfe_isr_wm_done(vfe, 0);
+			if (status & BUS_IRQ_MASK_0_COMP_DONE(vfe, RDI_COMP_GROUP(i)))
+				vfe_isr_wm_done(vfe, i);
+		}
 	}
 
 	return IRQ_HANDLED;
@@ -234,24 +254,23 @@ static int vfe_get_output(struct vfe_line *line)
 	struct vfe_device *vfe = to_vfe(line);
 	struct vfe_output *output;
 	unsigned long flags;
-	int wm_idx;
 
 	spin_lock_irqsave(&vfe->output_lock, flags);
 
 	output = &line->output;
-	if (output->state != VFE_OUTPUT_OFF) {
+	if (output->state > VFE_OUTPUT_RESERVED) {
 		dev_err(vfe->camss->dev, "Output is running\n");
 		goto error;
 	}
 
 	output->wm_num = 1;
 
-	wm_idx = vfe_reserve_wm(vfe, line->id);
-	if (wm_idx < 0) {
-		dev_err(vfe->camss->dev, "Can not reserve wm\n");
-		goto error_get_wm;
-	}
-	output->wm_idx[0] = wm_idx;
+	/* Correspondence between VFE line number and WM number.
+	 * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 -> PIX/RDI3
+	 * Note this 1:1 mapping will not work for PIX streams.
+	 */
+	output->wm_idx[0] = line->id;
+	vfe->wm_output_map[line->id] = line->id;
 
 	output->drop_update_idx = 0;
 
@@ -259,11 +278,9 @@ static int vfe_get_output(struct vfe_line *line)
 
 	return 0;
 
-error_get_wm:
-	vfe_release_wm(vfe, output->wm_idx[0]);
-	output->state = VFE_OUTPUT_OFF;
 error:
 	spin_unlock_irqrestore(&vfe->output_lock, flags);
+	output->state = VFE_OUTPUT_OFF;
 
 	return -EINVAL;
 }
@@ -279,7 +296,7 @@ static int vfe_enable_output(struct vfe_line *line)
 
 	vfe_reg_update_clear(vfe, line->id);
 
-	if (output->state != VFE_OUTPUT_OFF) {
+	if (output->state > VFE_OUTPUT_RESERVED) {
 		dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
 			output->state);
 		spin_unlock_irqrestore(&vfe->output_lock, flags);
@@ -360,6 +377,8 @@ static int vfe_enable(struct vfe_line *line)
 
 	vfe->stream_count++;
 
+	vfe_enable_lines_irq(vfe);
+
 	mutex_unlock(&vfe->stream_lock);
 
 	ret = vfe_get_output(line);
@@ -548,7 +567,7 @@ static const struct camss_video_ops vfe_video_ops_480 = {
 static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
 {
 	vfe->video_ops = vfe_video_ops_480;
-	vfe->line_num = 1;
+	vfe->line_num = MAX_VFE_OUTPUT_LINES;
 }
 
 const struct vfe_hw_ops vfe_ops_480 = {
-- 
2.37.3

[PATCH v4 4/4] media: camss: sm8250: Pipeline starting and stopping for multiple virtual channels

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

Use the multistream series function video_device_pipeline_alloc_start
to allows multiple clients of the same pipeline.

If any of the entities in the pipeline doesn't return success at stop
(e.g. if a VFE line remains running), the full pipeline won't be stopped.
This allows for stopping and starting streams at any point without
disrupting the other running streams.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
---
 .../media/platform/qcom/camss/camss-video.c   | 21 ++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 81fb3a5bc1d5..094971e2ff02 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -351,6 +351,7 @@ static int video_get_subdev_format(struct camss_video *video,
 	if (subdev == NULL)
 		return -EPIPE;
 
+	memset(&fmt, 0, sizeof(fmt));
 	fmt.pad = pad;
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 
@@ -493,9 +494,11 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct v4l2_subdev *subdev;
 	int ret;
 
-	ret = video_device_pipeline_start(vdev, &video->pipe);
-	if (ret < 0)
+	ret = video_device_pipeline_alloc_start(vdev);
+	if (ret < 0) {
+		dev_err(video->camss->dev, "Failed to start media pipeline: %d\n", ret);
 		return ret;
+	}
 
 	ret = video_check_format(video);
 	if (ret < 0)
@@ -536,6 +539,7 @@ static void video_stop_streaming(struct vb2_queue *q)
 	struct media_entity *entity;
 	struct media_pad *pad;
 	struct v4l2_subdev *subdev;
+	int ret;
 
 	entity = &vdev->entity;
 	while (1) {
@@ -550,7 +554,18 @@ static void video_stop_streaming(struct vb2_queue *q)
 		entity = pad->entity;
 		subdev = media_entity_to_v4l2_subdev(entity);
 
-		v4l2_subdev_call(subdev, video, s_stream, 0);
+		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
+
+		if (ret == -EBUSY) {
+			/* Don't stop if other instances of the pipeline are still running */
+			dev_dbg(video->camss->dev, "Video pipeline still used, don't stop streaming.\n");
+			return;
+		}
+
+		if (ret) {
+			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
+			return;
+		}
 	}
 
 	video_device_pipeline_stop(vdev);
-- 
2.37.3

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
> For v4:
> - fixes the warning reported by the kernel test robot
> - tiny code change to enable the vc functionality with the partially-applied
>    multistream patches on linux-next (tested on tag:next-20221010)

Ah.

I never groked I need to apply an alternative set on top to test this.
Doh !

Let me see if I can give you a tested-by

---
bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Laurent Pinchart]

Hello Milen,

Thank you for the patches.

I'll try to give this an initial review soon.

What hardware platform would you recommend, if any, if I wanted to test
this series ? Ideally that would be an off-the-shelf development board
that can easily be sourced, and that wouldn't require out-of-tree
drivers (beside perhaps for the camera sensor if it's not in mainline
yet) to test the camss driver (ethernet would be very appreciated, so
would display support).

On Thu, Oct 13, 2022 at 03:12:51PM +0300, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> For v4:
> - fixes the warning reported by the kernel test robot
> - tiny code change to enable the vc functionality with the partially-applied
>   multistream patches on linux-next (tested on tag:next-20221010)
> 
> For v3:
> - setting the sink pad format on the CSID entity will now propagate the
>   format to the source pads to keep the subdev in a valid internal state.
> - code syntax improvements
> 
> For v2:
> - code syntax improvements
> - The info print for the enabled VCs was demoted to a dbg print. Can be
>   enabled with dynamic debug, e.g.:
> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
> 
> NOTE: These changes depend on the multistream series, that as of yet
> is still not merged upstream. However, part of the
> multistream patches are merged in linux-next (tested on
> tag:next-20221010), including the patch that introduces the
> video_device_pipeline_alloc_start() functionality. This allows 
> applying and using this series on linux-next without applying the
> complete multistream set.
> 
> The CSID hardware on SM8250 can demux the input data stream into
> maximum of 4 multiple streams depending on virtual channel (vc)
> or data type (dt) configuration.
> 
> Situations in which demuxing is useful:
> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>   (the setup we used for testing, with the imx412 sensor),
>   or a 3-frame HDR output - light, medium-lit, dark frame.
> - sensors with additional metadata that is streamed over a different
>   virtual channel/datatype.
> - sensors that produce frames with multiple resolutions in the same pixel
>   data stream
> 
> With these changes, the CSID entity has, as it did previously, a single
> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
> virtual channel configuration is determined by which of the source ports
> are linked to an output VFE line. For example, the link below will
> configure the CSID driver to enable vc 0 and vc 1:
> 
> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
> 
> which will be demuxed and propagated into /dev/video0
> and /dev/video1 respectively. With this, the userspace can use
> any normal V4L2 client app to start/stop/queue/dequeue from these
> video nodes. Tested with the yavta app.
> 
> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
> msm_vfe0_rdi1,...) must also be configured explicitly.
> 
> Note that in order to keep a valid internal subdevice state,
> setting the sink pad format of the CSID subdevice will propagate
> this format to the source pads. However, since the CSID hardware
> can demux the input stream into several streams each of which can 
> be a different format, in that case each source pad's 
> format must be set individually, e.g.:
> 
> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
> 
> Milen Mitkov (4):
>   media: camss: sm8250: Virtual channels for CSID
>   media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>   media: camss: vfe-480: Multiple outputs support for SM8250
>   media: camss: sm8250: Pipeline starting and stopping for multiple
>     virtual channels
> 
>  .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>  .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>  .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>  .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>  drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>  .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>  drivers/media/platform/qcom/camss/camss.c     |  2 +-
>  7 files changed, 140 insertions(+), 60 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> For v4:
> - fixes the warning reported by the kernel test robot
> - tiny code change to enable the vc functionality with the partially-applied
>    multistream patches on linux-next (tested on tag:next-20221010)
> 
> For v3:
> - setting the sink pad format on the CSID entity will now propagate the
>    format to the source pads to keep the subdev in a valid internal state.
> - code syntax improvements
> 
> For v2:
> - code syntax improvements
> - The info print for the enabled VCs was demoted to a dbg print. Can be
>    enabled with dynamic debug, e.g.:
> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
> 
> NOTE: These changes depend on the multistream series, that as of yet
> is still not merged upstream. However, part of the
> multistream patches are merged in linux-next (tested on
> tag:next-20221010), including the patch that introduces the
> video_device_pipeline_alloc_start() functionality. This allows
> applying and using this series on linux-next without applying the
> complete multistream set.
> 
> The CSID hardware on SM8250 can demux the input data stream into
> maximum of 4 multiple streams depending on virtual channel (vc)
> or data type (dt) configuration.
> 
> Situations in which demuxing is useful:
> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>    (the setup we used for testing, with the imx412 sensor),
>    or a 3-frame HDR output - light, medium-lit, dark frame.
> - sensors with additional metadata that is streamed over a different
>    virtual channel/datatype.
> - sensors that produce frames with multiple resolutions in the same pixel
>    data stream
> 
> With these changes, the CSID entity has, as it did previously, a single
> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
> virtual channel configuration is determined by which of the source ports
> are linked to an output VFE line. For example, the link below will
> configure the CSID driver to enable vc 0 and vc 1:
> 
> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
> 
> which will be demuxed and propagated into /dev/video0
> and /dev/video1 respectively. With this, the userspace can use
> any normal V4L2 client app to start/stop/queue/dequeue from these
> video nodes. Tested with the yavta app.
> 
> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
> msm_vfe0_rdi1,...) must also be configured explicitly.
> 
> Note that in order to keep a valid internal subdevice state,
> setting the sink pad format of the CSID subdevice will propagate
> this format to the source pads. However, since the CSID hardware
> can demux the input stream into several streams each of which can
> be a different format, in that case each source pad's
> format must be set individually, e.g.:
> 
> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
> 
> Milen Mitkov (4):
>    media: camss: sm8250: Virtual channels for CSID
>    media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>    media: camss: vfe-480: Multiple outputs support for SM8250
>    media: camss: sm8250: Pipeline starting and stopping for multiple
>      virtual channels
> 
>   .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>   .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>   .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>   7 files changed, 140 insertions(+), 60 deletions(-)
> 

Hi Milen

The set applies to next-20221013 including patch#4.

I can confirm it doesn't break anything for me - though my sensor is a 
bog-standard imx577 so it doesn't have any VC support.

Before I give you a tested-by for the series .. is this normal ?

[   90.535909] qcom-camss ac6a000.camss: VFE HW Version = 2.0.1
[   90.545756] qcom-camss ac6a000.camss: CSIPHY 3PH HW Version = 0x40010000
[   90.546358] qcom-camss ac6a000.camss: CSID HW Version = 2.0.1
[   90.546365] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
virtual channels mask 0x1
[   90.547675] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
virtual channels mask 0x0

I have

- ov9282 on cam1
- imx577 on cam2

i.e. why do I see this message twice if I only have the one sensor 
active, with no VCs and one operable camera ?

---
bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Milen Mitkov (Consultant)]

On 17/10/2022 03:16, Bryan O'Donoghue wrote:
> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>
>> For v4:
>> - fixes the warning reported by the kernel test robot
>> - tiny code change to enable the vc functionality with the 
>> partially-applied
>>    multistream patches on linux-next (tested on tag:next-20221010)
>>
>> For v3:
>> - setting the sink pad format on the CSID entity will now propagate the
>>    format to the source pads to keep the subdev in a valid internal 
>> state.
>> - code syntax improvements
>>
>> For v2:
>> - code syntax improvements
>> - The info print for the enabled VCs was demoted to a dbg print. Can be
>>    enabled with dynamic debug, e.g.:
>> echo "file drivers/media/platform/qcom/camss/* +p" > 
>> /sys/kernel/debug/dynamic_debug/control
>>
>> NOTE: These changes depend on the multistream series, that as of yet
>> is still not merged upstream. However, part of the
>> multistream patches are merged in linux-next (tested on
>> tag:next-20221010), including the patch that introduces the
>> video_device_pipeline_alloc_start() functionality. This allows
>> applying and using this series on linux-next without applying the
>> complete multistream set.
>>
>> The CSID hardware on SM8250 can demux the input data stream into
>> maximum of 4 multiple streams depending on virtual channel (vc)
>> or data type (dt) configuration.
>>
>> Situations in which demuxing is useful:
>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a 
>> dark frame
>>    (the setup we used for testing, with the imx412 sensor),
>>    or a 3-frame HDR output - light, medium-lit, dark frame.
>> - sensors with additional metadata that is streamed over a different
>>    virtual channel/datatype.
>> - sensors that produce frames with multiple resolutions in the same 
>> pixel
>>    data stream
>>
>> With these changes, the CSID entity has, as it did previously, a single
>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
>> virtual channel configuration is determined by which of the source ports
>> are linked to an output VFE line. For example, the link below will
>> configure the CSID driver to enable vc 0 and vc 1:
>>
>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>>
>> which will be demuxed and propagated into /dev/video0
>> and /dev/video1 respectively. With this, the userspace can use
>> any normal V4L2 client app to start/stop/queue/dequeue from these
>> video nodes. Tested with the yavta app.
>>
>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
>> msm_vfe0_rdi1,...) must also be configured explicitly.
>>
>> Note that in order to keep a valid internal subdevice state,
>> setting the sink pad format of the CSID subdevice will propagate
>> this format to the source pads. However, since the CSID hardware
>> can demux the input stream into several streams each of which can
>> be a different format, in that case each source pad's
>> format must be set individually, e.g.:
>>
>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
>>
>> Milen Mitkov (4):
>>    media: camss: sm8250: Virtual channels for CSID
>>    media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>>    media: camss: vfe-480: Multiple outputs support for SM8250
>>    media: camss: sm8250: Pipeline starting and stopping for multiple
>>      virtual channels
>>
>>   .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>>   .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>>   .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>   7 files changed, 140 insertions(+), 60 deletions(-)
>>
>
> Hi Milen
>
> The set applies to next-20221013 including patch#4.
>
> I can confirm it doesn't break anything for me - though my sensor is a 
> bog-standard imx577 so it doesn't have any VC support.
>
> Before I give you a tested-by for the series .. is this normal ?
>
> [   90.535909] qcom-camss ac6a000.camss: VFE HW Version = 2.0.1
> [   90.545756] qcom-camss ac6a000.camss: CSIPHY 3PH HW Version = 
> 0x40010000
> [   90.546358] qcom-camss ac6a000.camss: CSID HW Version = 2.0.1
> [   90.546365] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
> virtual channels mask 0x1
> [   90.547675] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
> virtual channels mask 0x0
>
> I have
>
> - ov9282 on cam1
> - imx577 on cam2
>
> i.e. why do I see this message twice if I only have the one sensor 
> active, with no VCs and one operable camera ?
>
> ---
> bod


Hey Bryan,

I don't see the second print (..."virtual channels mask 0x0") on my side 
when testing with the standard imx577 driver. I also have imx577 on slot 
2 and ov9282 on slot 1. I am testing with:

media-ctl --reset
media-ctl -v -d /dev/media0 -V '"imx577 
'22-001a'":0[fmt:SRGGB10/4056x3040 field:none]'
media-ctl -V '"msm_csiphy2":0[fmt:SRGGB10/4056x3040]'
media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]'
media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]'
media-ctl -l '"msm_csiphy2":1->"msm_csid0":0[1]'
media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'

yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0


What is your testing procedure?


Regards,

Milen

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 17/10/2022 12:47, Milen Mitkov (Consultant) wrote:
> 
> Hey Bryan,
> 
> I don't see the second print (..."virtual channels mask 0x0") on my side 
> when testing with the standard imx577 driver. I also have imx577 on slot 
> 2 and ov9282 on slot 1. I am testing with:
> 
> media-ctl --reset
> media-ctl -v -d /dev/media0 -V '"imx577 
> '22-001a'":0[fmt:SRGGB10/4056x3040 field:none]'
> media-ctl -V '"msm_csiphy2":0[fmt:SRGGB10/4056x3040]'
> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]'
> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]'
> media-ctl -l '"msm_csiphy2":1->"msm_csid0":0[1]'
> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> 
> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0
> 
> 
> What is your testing procedure?
> 
> 
> Regards,

So for that I just do

https://libcamera.org/getting-started.html

apt install -y g++ meson libyaml-dev python3-yaml python3-ply 
python3-jinja2 libssl-dev libdw-dev libunwind-dev libudev-dev 
libgstreamer1.0-dev libgstreamer-plugins-base1.0-dev libevent-dev

git clone https://git.libcamera.org/libcamera/libcamera.git
cd libcamera
meson build
ninja -C build

cd build/src/cam

./cam -l

Available cameras:
1: 'ov9282' (/base/soc@0/cci@ac4f000/i2c-bus@1/camera@60)
2: 'imx577' (/base/soc@0/cci@ac50000/i2c-bus@0/camera@1a)

./cam -c 2 --capture=10 --file

Do you have the ov9282 probed ?

To ssh://git.linaro.org/people/bryan.odonoghue/kernel.git
  * [new branch]                HEAD -> next-20221013-sm8250-camss-vc

---
bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 17/10/2022 14:06, Kieran Bingham wrote:
> Quoting Bryan O'Donoghue (2022-10-17 01:16:05)
>> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v4:
>>> - fixes the warning reported by the kernel test robot
>>> - tiny code change to enable the vc functionality with the partially-applied
>>>     multistream patches on linux-next (tested on tag:next-20221010)
>>>
>>> For v3:
>>> - setting the sink pad format on the CSID entity will now propagate the
>>>     format to the source pads to keep the subdev in a valid internal state.
>>> - code syntax improvements
>>>
>>> For v2:
>>> - code syntax improvements
>>> - The info print for the enabled VCs was demoted to a dbg print. Can be
>>>     enabled with dynamic debug, e.g.:
>>> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
>>>
>>> NOTE: These changes depend on the multistream series, that as of yet
>>> is still not merged upstream. However, part of the
>>> multistream patches are merged in linux-next (tested on
>>> tag:next-20221010), including the patch that introduces the
>>> video_device_pipeline_alloc_start() functionality. This allows
>>> applying and using this series on linux-next without applying the
>>> complete multistream set.
>>>
>>> The CSID hardware on SM8250 can demux the input data stream into
>>> maximum of 4 multiple streams depending on virtual channel (vc)
>>> or data type (dt) configuration.
>>>
>>> Situations in which demuxing is useful:
>>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>>>     (the setup we used for testing, with the imx412 sensor),
>>>     or a 3-frame HDR output - light, medium-lit, dark frame.
>>> - sensors with additional metadata that is streamed over a different
>>>     virtual channel/datatype.
>>> - sensors that produce frames with multiple resolutions in the same pixel
>>>     data stream
>>>
>>> With these changes, the CSID entity has, as it did previously, a single
>>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
>>> virtual channel configuration is determined by which of the source ports
>>> are linked to an output VFE line. For example, the link below will
>>> configure the CSID driver to enable vc 0 and vc 1:
>>>
>>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>>>
>>> which will be demuxed and propagated into /dev/video0
>>> and /dev/video1 respectively. With this, the userspace can use
>>> any normal V4L2 client app to start/stop/queue/dequeue from these
>>> video nodes. Tested with the yavta app.
>>>
>>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
>>> msm_vfe0_rdi1,...) must also be configured explicitly.
>>>
>>> Note that in order to keep a valid internal subdevice state,
>>> setting the sink pad format of the CSID subdevice will propagate
>>> this format to the source pads. However, since the CSID hardware
>>> can demux the input stream into several streams each of which can
>>> be a different format, in that case each source pad's
>>> format must be set individually, e.g.:
>>>
>>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
>>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
>>>
>>> Milen Mitkov (4):
>>>     media: camss: sm8250: Virtual channels for CSID
>>>     media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>>>     media: camss: vfe-480: Multiple outputs support for SM8250
>>>     media: camss: sm8250: Pipeline starting and stopping for multiple
>>>       virtual channels
>>>
>>>    .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>>>    .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>>>    .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>>>    .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>>    drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>>>    .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>    drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>    7 files changed, 140 insertions(+), 60 deletions(-)
>>>
>>
>> Hi Milen
>>
>> The set applies to next-20221013 including patch#4.
>>
>> I can confirm it doesn't break anything for me - though my sensor is a
>> bog-standard imx577 so it doesn't have any VC support.
> 
> Interesting though - the IMX477 has the ability to convey metadata on a
> separate VC... That's actually the thing holding back the RPi IMX477
> driver from mainline, as the way it was anticipated to support multiple
> data streams is with the new multiplexed streams API.
> 
> And I think we inferred that the IMX577 and IMX477 are closely related,
> so should have similar capabilities for obtaining metadata channels?

Hmm I was not aware of that.

If we could import the rpi/imx477.c code into upstrea/imx412.c it might 
be possible

The core init is very similar

https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L167

https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx412.c#L160

Maybe it would be possible to apply the rest of the imx477 config on-top 
as a POC

https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L479

The similary is born out by the shared init code I can see in Leopard 
imaging's driver, I'm not sure if it supports virtual-channels - I'll 
have a look, though.

What's in the imx477 meta-data ?

@Milen if you have the imx577 datasheet - I don't - perhaps we could 
cherry-pick some of the code from imx477 and get the imx412.c->imx577 
dumping VC data out with the RB5 camera mezzanine.

---
bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Kieran Bingham]

Quoting Bryan O'Donoghue (2022-10-17 15:22:10)
> On 17/10/2022 14:06, Kieran Bingham wrote:
> > Quoting Bryan O'Donoghue (2022-10-17 01:16:05)
> >> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
> >>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> >>>
> >>> For v4:
> >>> - fixes the warning reported by the kernel test robot
> >>> - tiny code change to enable the vc functionality with the partially-applied
> >>>     multistream patches on linux-next (tested on tag:next-20221010)
> >>>
> >>> For v3:
> >>> - setting the sink pad format on the CSID entity will now propagate the
> >>>     format to the source pads to keep the subdev in a valid internal state.
> >>> - code syntax improvements
> >>>
> >>> For v2:
> >>> - code syntax improvements
> >>> - The info print for the enabled VCs was demoted to a dbg print. Can be
> >>>     enabled with dynamic debug, e.g.:
> >>> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
> >>>
> >>> NOTE: These changes depend on the multistream series, that as of yet
> >>> is still not merged upstream. However, part of the
> >>> multistream patches are merged in linux-next (tested on
> >>> tag:next-20221010), including the patch that introduces the
> >>> video_device_pipeline_alloc_start() functionality. This allows
> >>> applying and using this series on linux-next without applying the
> >>> complete multistream set.
> >>>
> >>> The CSID hardware on SM8250 can demux the input data stream into
> >>> maximum of 4 multiple streams depending on virtual channel (vc)
> >>> or data type (dt) configuration.
> >>>
> >>> Situations in which demuxing is useful:
> >>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
> >>>     (the setup we used for testing, with the imx412 sensor),
> >>>     or a 3-frame HDR output - light, medium-lit, dark frame.
> >>> - sensors with additional metadata that is streamed over a different
> >>>     virtual channel/datatype.
> >>> - sensors that produce frames with multiple resolutions in the same pixel
> >>>     data stream
> >>>
> >>> With these changes, the CSID entity has, as it did previously, a single
> >>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
> >>> virtual channel configuration is determined by which of the source ports
> >>> are linked to an output VFE line. For example, the link below will
> >>> configure the CSID driver to enable vc 0 and vc 1:
> >>>
> >>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> >>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
> >>>
> >>> which will be demuxed and propagated into /dev/video0
> >>> and /dev/video1 respectively. With this, the userspace can use
> >>> any normal V4L2 client app to start/stop/queue/dequeue from these
> >>> video nodes. Tested with the yavta app.
> >>>
> >>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
> >>> msm_vfe0_rdi1,...) must also be configured explicitly.
> >>>
> >>> Note that in order to keep a valid internal subdevice state,
> >>> setting the sink pad format of the CSID subdevice will propagate
> >>> this format to the source pads. However, since the CSID hardware
> >>> can demux the input stream into several streams each of which can
> >>> be a different format, in that case each source pad's
> >>> format must be set individually, e.g.:
> >>>
> >>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
> >>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
> >>>
> >>> Milen Mitkov (4):
> >>>     media: camss: sm8250: Virtual channels for CSID
> >>>     media: camss: vfe: Reserve VFE lines on stream start and link to CSID
> >>>     media: camss: vfe-480: Multiple outputs support for SM8250
> >>>     media: camss: sm8250: Pipeline starting and stopping for multiple
> >>>       virtual channels
> >>>
> >>>    .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
> >>>    .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
> >>>    .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
> >>>    .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
> >>>    drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
> >>>    .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
> >>>    drivers/media/platform/qcom/camss/camss.c     |  2 +-
> >>>    7 files changed, 140 insertions(+), 60 deletions(-)
> >>>
> >>
> >> Hi Milen
> >>
> >> The set applies to next-20221013 including patch#4.
> >>
> >> I can confirm it doesn't break anything for me - though my sensor is a
> >> bog-standard imx577 so it doesn't have any VC support.
> > 
> > Interesting though - the IMX477 has the ability to convey metadata on a
> > separate VC... That's actually the thing holding back the RPi IMX477
> > driver from mainline, as the way it was anticipated to support multiple
> > data streams is with the new multiplexed streams API.
> > 
> > And I think we inferred that the IMX577 and IMX477 are closely related,
> > so should have similar capabilities for obtaining metadata channels?
> 
> Hmm I was not aware of that.
> 
> If we could import the rpi/imx477.c code into upstrea/imx412.c it might 
> be possible
> 
> The core init is very similar
> 
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L167
> 
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx412.c#L160
> 
> Maybe it would be possible to apply the rest of the imx477 config on-top 
> as a POC
> 
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L479
> 
> The similary is born out by the shared init code I can see in Leopard 
> imaging's driver, I'm not sure if it supports virtual-channels - I'll 
> have a look, though.
> 
> What's in the imx477 meta-data ?

The exact exposure of the captured frame, exact gain, and frame length,
and even the temperature of the sensor at the time of capture (not sure
at /which/ time if this is a long exposure).


https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/raspberrypi/cam_helper_imx477.cpp#n168

"""
void CamHelperImx477::populateMetadata(const MdParser::RegisterMap &registers,
				       Metadata &metadata) const
{
	DeviceStatus deviceStatus;

	deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
	deviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
	deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);
	deviceStatus.sensorTemperature = std::clamp<int8_t>(registers.at(temperatureReg), -20, 80);

	metadata.set("device.status", deviceStatus);
}

"""

Having the embedded metadata from the sensor helps to ensure accurate
handling in the control loops, so I believe we would always prefer to
reference this when available, rather than what we 'think' we have
programmed. (Which due to timing, or any other error - might not be as
accurate as what the metadata will report).
--
Kieran


> 
> @Milen if you have the imx577 datasheet - I don't - perhaps we could 
> cherry-pick some of the code from imx477 and get the imx412.c->imx577 
> dumping VC data out with the RB5 camera mezzanine.
> 
> ---
> bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Milen Mitkov (Consultant)]

On 17/10/2022 17:22, Bryan O'Donoghue wrote:
> On 17/10/2022 14:06, Kieran Bingham wrote:
>> Quoting Bryan O'Donoghue (2022-10-17 01:16:05)
>>> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
>>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>>
>>>> For v4:
>>>> - fixes the warning reported by the kernel test robot
>>>> - tiny code change to enable the vc functionality with the 
>>>> partially-applied
>>>>     multistream patches on linux-next (tested on tag:next-20221010)
>>>>
>>>> For v3:
>>>> - setting the sink pad format on the CSID entity will now propagate 
>>>> the
>>>>     format to the source pads to keep the subdev in a valid 
>>>> internal state.
>>>> - code syntax improvements
>>>>
>>>> For v2:
>>>> - code syntax improvements
>>>> - The info print for the enabled VCs was demoted to a dbg print. 
>>>> Can be
>>>>     enabled with dynamic debug, e.g.:
>>>> echo "file drivers/media/platform/qcom/camss/* +p" > 
>>>> /sys/kernel/debug/dynamic_debug/control
>>>>
>>>> NOTE: These changes depend on the multistream series, that as of yet
>>>> is still not merged upstream. However, part of the
>>>> multistream patches are merged in linux-next (tested on
>>>> tag:next-20221010), including the patch that introduces the
>>>> video_device_pipeline_alloc_start() functionality. This allows
>>>> applying and using this series on linux-next without applying the
>>>> complete multistream set.
>>>>
>>>> The CSID hardware on SM8250 can demux the input data stream into
>>>> maximum of 4 multiple streams depending on virtual channel (vc)
>>>> or data type (dt) configuration.
>>>>
>>>> Situations in which demuxing is useful:
>>>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a 
>>>> dark frame
>>>>     (the setup we used for testing, with the imx412 sensor),
>>>>     or a 3-frame HDR output - light, medium-lit, dark frame.
>>>> - sensors with additional metadata that is streamed over a different
>>>>     virtual channel/datatype.
>>>> - sensors that produce frames with multiple resolutions in the same 
>>>> pixel
>>>>     data stream
>>>>
>>>> With these changes, the CSID entity has, as it did previously, a 
>>>> single
>>>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
>>>> virtual channel configuration is determined by which of the source 
>>>> ports
>>>> are linked to an output VFE line. For example, the link below will
>>>> configure the CSID driver to enable vc 0 and vc 1:
>>>>
>>>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>>>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>>>>
>>>> which will be demuxed and propagated into /dev/video0
>>>> and /dev/video1 respectively. With this, the userspace can use
>>>> any normal V4L2 client app to start/stop/queue/dequeue from these
>>>> video nodes. Tested with the yavta app.
>>>>
>>>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
>>>> msm_vfe0_rdi1,...) must also be configured explicitly.
>>>>
>>>> Note that in order to keep a valid internal subdevice state,
>>>> setting the sink pad format of the CSID subdevice will propagate
>>>> this format to the source pads. However, since the CSID hardware
>>>> can demux the input stream into several streams each of which can
>>>> be a different format, in that case each source pad's
>>>> format must be set individually, e.g.:
>>>>
>>>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
>>>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
>>>>
>>>> Milen Mitkov (4):
>>>>     media: camss: sm8250: Virtual channels for CSID
>>>>     media: camss: vfe: Reserve VFE lines on stream start and link 
>>>> to CSID
>>>>     media: camss: vfe-480: Multiple outputs support for SM8250
>>>>     media: camss: sm8250: Pipeline starting and stopping for multiple
>>>>       virtual channels
>>>>
>>>>    .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>>>>    .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>>>>    .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>>>>    .../media/platform/qcom/camss/camss-vfe-480.c | 61 
>>>> ++++++++++++-------
>>>>    drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>>>>    .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>>    drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>>    7 files changed, 140 insertions(+), 60 deletions(-)
>>>>
>>>
>>> Hi Milen
>>>
>>> The set applies to next-20221013 including patch#4.
>>>
>>> I can confirm it doesn't break anything for me - though my sensor is a
>>> bog-standard imx577 so it doesn't have any VC support.
>>
>> Interesting though - the IMX477 has the ability to convey metadata on a
>> separate VC... That's actually the thing holding back the RPi IMX477
>> driver from mainline, as the way it was anticipated to support multiple
>> data streams is with the new multiplexed streams API.
>>
>> And I think we inferred that the IMX577 and IMX477 are closely related,
>> so should have similar capabilities for obtaining metadata channels?
>
> Hmm I was not aware of that.
>
> If we could import the rpi/imx477.c code into upstrea/imx412.c it 
> might be possible
>
> The core init is very similar
>
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L167 
>
>
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx412.c#L160 
>
>
> Maybe it would be possible to apply the rest of the imx477 config 
> on-top as a POC
>
> https://github.com/raspberrypi/linux/blob/rpi-5.19.y/drivers/media/i2c/imx477.c#L479 
>
>
> The similary is born out by the shared init code I can see in Leopard 
> imaging's driver, I'm not sure if it supports virtual-channels - I'll 
> have a look, though.
>
> What's in the imx477 meta-data ?
>
> @Milen if you have the imx577 datasheet - I don't - perhaps we could 
> cherry-pick some of the code from imx477 and get the imx412.c->imx577 
> dumping VC data out with the RB5 camera mezzanine.
>
> ---
> bod

Bryan, contact Nicolas Dechesne (from Linaro) with regards to the imx577 
datasheet. I don't have permissions.


Regards,

Milen

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Tomi Valkeinen]

On 17/10/2022 16:06, Kieran Bingham wrote:
> Quoting Bryan O'Donoghue (2022-10-17 01:16:05)
>> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v4:
>>> - fixes the warning reported by the kernel test robot
>>> - tiny code change to enable the vc functionality with the partially-applied
>>>     multistream patches on linux-next (tested on tag:next-20221010)
>>>
>>> For v3:
>>> - setting the sink pad format on the CSID entity will now propagate the
>>>     format to the source pads to keep the subdev in a valid internal state.
>>> - code syntax improvements
>>>
>>> For v2:
>>> - code syntax improvements
>>> - The info print for the enabled VCs was demoted to a dbg print. Can be
>>>     enabled with dynamic debug, e.g.:
>>> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
>>>
>>> NOTE: These changes depend on the multistream series, that as of yet
>>> is still not merged upstream. However, part of the
>>> multistream patches are merged in linux-next (tested on
>>> tag:next-20221010), including the patch that introduces the
>>> video_device_pipeline_alloc_start() functionality. This allows
>>> applying and using this series on linux-next without applying the
>>> complete multistream set.
>>>
>>> The CSID hardware on SM8250 can demux the input data stream into
>>> maximum of 4 multiple streams depending on virtual channel (vc)
>>> or data type (dt) configuration.
>>>
>>> Situations in which demuxing is useful:
>>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>>>     (the setup we used for testing, with the imx412 sensor),
>>>     or a 3-frame HDR output - light, medium-lit, dark frame.
>>> - sensors with additional metadata that is streamed over a different
>>>     virtual channel/datatype.
>>> - sensors that produce frames with multiple resolutions in the same pixel
>>>     data stream
>>>
>>> With these changes, the CSID entity has, as it did previously, a single
>>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
>>> virtual channel configuration is determined by which of the source ports
>>> are linked to an output VFE line. For example, the link below will
>>> configure the CSID driver to enable vc 0 and vc 1:
>>>
>>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>>>
>>> which will be demuxed and propagated into /dev/video0
>>> and /dev/video1 respectively. With this, the userspace can use
>>> any normal V4L2 client app to start/stop/queue/dequeue from these
>>> video nodes. Tested with the yavta app.
>>>
>>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
>>> msm_vfe0_rdi1,...) must also be configured explicitly.
>>>
>>> Note that in order to keep a valid internal subdevice state,
>>> setting the sink pad format of the CSID subdevice will propagate
>>> this format to the source pads. However, since the CSID hardware
>>> can demux the input stream into several streams each of which can
>>> be a different format, in that case each source pad's
>>> format must be set individually, e.g.:
>>>
>>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
>>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
>>>
>>> Milen Mitkov (4):
>>>     media: camss: sm8250: Virtual channels for CSID
>>>     media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>>>     media: camss: vfe-480: Multiple outputs support for SM8250
>>>     media: camss: sm8250: Pipeline starting and stopping for multiple
>>>       virtual channels
>>>
>>>    .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>>>    .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>>>    .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>>>    .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>>    drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>>>    .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>    drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>    7 files changed, 140 insertions(+), 60 deletions(-)
>>>
>>
>> Hi Milen
>>
>> The set applies to next-20221013 including patch#4.
>>
>> I can confirm it doesn't break anything for me - though my sensor is a
>> bog-standard imx577 so it doesn't have any VC support.
> 
> Interesting though - the IMX477 has the ability to convey metadata on a
> separate VC... That's actually the thing holding back the RPi IMX477
> driver from mainline, as the way it was anticipated to support multiple
> data streams is with the new multiplexed streams API.

Note that while the streams series enables the support for multiple 
streams, metadata support is not included. This mainly means the 
metadata mbus/v4l2 formats.

We've had some discussions about this with Laurent and Sakari, and it 
sounds like a rather complex issue (although you can skip the problems 
by just adding a custom metadata format, which I'm doing in my work 
branch to test metadata).

  Tomi

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> For v4:
> - fixes the warning reported by the kernel test robot
> - tiny code change to enable the vc functionality with the partially-applied
>    multistream patches on linux-next (tested on tag:next-20221010)
> 
> For v3:
> - setting the sink pad format on the CSID entity will now propagate the
>    format to the source pads to keep the subdev in a valid internal state.
> - code syntax improvements
> 
> For v2:
> - code syntax improvements
> - The info print for the enabled VCs was demoted to a dbg print. Can be
>    enabled with dynamic debug, e.g.:
> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
> 
> NOTE: These changes depend on the multistream series, that as of yet
> is still not merged upstream. However, part of the
> multistream patches are merged in linux-next (tested on
> tag:next-20221010), including the patch that introduces the
> video_device_pipeline_alloc_start() functionality. This allows
> applying and using this series on linux-next without applying the
> complete multistream set.
> 
> The CSID hardware on SM8250 can demux the input data stream into
> maximum of 4 multiple streams depending on virtual channel (vc)
> or data type (dt) configuration.
> 
> Situations in which demuxing is useful:
> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>    (the setup we used for testing, with the imx412 sensor),
>    or a 3-frame HDR output - light, medium-lit, dark frame.
> - sensors with additional metadata that is streamed over a different
>    virtual channel/datatype.
> - sensors that produce frames with multiple resolutions in the same pixel
>    data stream
> 
> With these changes, the CSID entity has, as it did previously, a single
> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
> virtual channel configuration is determined by which of the source ports
> are linked to an output VFE line. For example, the link below will
> configure the CSID driver to enable vc 0 and vc 1:
> 
> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
> 
> which will be demuxed and propagated into /dev/video0
> and /dev/video1 respectively. With this, the userspace can use
> any normal V4L2 client app to start/stop/queue/dequeue from these
> video nodes. Tested with the yavta app.
> 
> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
> msm_vfe0_rdi1,...) must also be configured explicitly.
> 
> Note that in order to keep a valid internal subdevice state,
> setting the sink pad format of the CSID subdevice will propagate
> this format to the source pads. However, since the CSID hardware
> can demux the input stream into several streams each of which can
> be a different format, in that case each source pad's
> format must be set individually, e.g.:
> 
> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
> 
> Milen Mitkov (4):
>    media: camss: sm8250: Virtual channels for CSID
>    media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>    media: camss: vfe-480: Multiple outputs support for SM8250
>    media: camss: sm8250: Pipeline starting and stopping for multiple
>      virtual channels
> 
>   .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>   .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>   .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>   7 files changed, 140 insertions(+), 60 deletions(-)
> 

I've done some offline work with Milen on this.

I'm happy enough to add my

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

to the series. I don't have - currently a VC enabled setup but for the 
simple case this set doesn't break anything on RB5 for me.

---
bod

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Robert Foss]

On Tue, 8 Nov 2022 at 01:12, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
> > From: Milen Mitkov <quic_mmitkov@quicinc.com>
> >
> > For v4:
> > - fixes the warning reported by the kernel test robot
> > - tiny code change to enable the vc functionality with the partially-applied
> >    multistream patches on linux-next (tested on tag:next-20221010)
> >
> > For v3:
> > - setting the sink pad format on the CSID entity will now propagate the
> >    format to the source pads to keep the subdev in a valid internal state.
> > - code syntax improvements
> >
> > For v2:
> > - code syntax improvements
> > - The info print for the enabled VCs was demoted to a dbg print. Can be
> >    enabled with dynamic debug, e.g.:
> > echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
> >
> > NOTE: These changes depend on the multistream series, that as of yet
> > is still not merged upstream. However, part of the
> > multistream patches are merged in linux-next (tested on
> > tag:next-20221010), including the patch that introduces the
> > video_device_pipeline_alloc_start() functionality. This allows
> > applying and using this series on linux-next without applying the
> > complete multistream set.
> >
> > The CSID hardware on SM8250 can demux the input data stream into
> > maximum of 4 multiple streams depending on virtual channel (vc)
> > or data type (dt) configuration.
> >
> > Situations in which demuxing is useful:
> > - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
> >    (the setup we used for testing, with the imx412 sensor),
> >    or a 3-frame HDR output - light, medium-lit, dark frame.
> > - sensors with additional metadata that is streamed over a different
> >    virtual channel/datatype.
> > - sensors that produce frames with multiple resolutions in the same pixel
> >    data stream
> >
> > With these changes, the CSID entity has, as it did previously, a single
> > sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
> > virtual channel configuration is determined by which of the source ports
> > are linked to an output VFE line. For example, the link below will
> > configure the CSID driver to enable vc 0 and vc 1:
> >
> > media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> > media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
> >
> > which will be demuxed and propagated into /dev/video0
> > and /dev/video1 respectively. With this, the userspace can use
> > any normal V4L2 client app to start/stop/queue/dequeue from these
> > video nodes. Tested with the yavta app.
> >
> > The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
> > msm_vfe0_rdi1,...) must also be configured explicitly.
> >
> > Note that in order to keep a valid internal subdevice state,
> > setting the sink pad format of the CSID subdevice will propagate
> > this format to the source pads. However, since the CSID hardware
> > can demux the input stream into several streams each of which can
> > be a different format, in that case each source pad's
> > format must be set individually, e.g.:
> >
> > media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
> > media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
> >
> > Milen Mitkov (4):
> >    media: camss: sm8250: Virtual channels for CSID
> >    media: camss: vfe: Reserve VFE lines on stream start and link to CSID
> >    media: camss: vfe-480: Multiple outputs support for SM8250
> >    media: camss: sm8250: Pipeline starting and stopping for multiple
> >      virtual channels
> >
> >   .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
> >   .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
> >   .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
> >   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
> >   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
> >   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
> >   drivers/media/platform/qcom/camss/camss.c     |  2 +-
> >   7 files changed, 140 insertions(+), 60 deletions(-)
> >
>
> I've done some offline work with Milen on this.
>
> I'm happy enough to add my
>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>
> to the series. I don't have - currently a VC enabled setup but for the
> simple case this set doesn't break anything on RB5 for me.
>
> ---
> bod

This series has my ack.

Acked-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Milen Mitkov (Consultant)]

On 08/11/2022 12:35, Robert Foss wrote:
> On Tue, 8 Nov 2022 at 01:12, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>> On 13/10/2022 13:12, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v4:
>>> - fixes the warning reported by the kernel test robot
>>> - tiny code change to enable the vc functionality with the partially-applied
>>>     multistream patches on linux-next (tested on tag:next-20221010)
>>>
>>> For v3:
>>> - setting the sink pad format on the CSID entity will now propagate the
>>>     format to the source pads to keep the subdev in a valid internal state.
>>> - code syntax improvements
>>>
>>> For v2:
>>> - code syntax improvements
>>> - The info print for the enabled VCs was demoted to a dbg print. Can be
>>>     enabled with dynamic debug, e.g.:
>>> echo "file drivers/media/platform/qcom/camss/* +p" > /sys/kernel/debug/dynamic_debug/control
>>>
>>> NOTE: These changes depend on the multistream series, that as of yet
>>> is still not merged upstream. However, part of the
>>> multistream patches are merged in linux-next (tested on
>>> tag:next-20221010), including the patch that introduces the
>>> video_device_pipeline_alloc_start() functionality. This allows
>>> applying and using this series on linux-next without applying the
>>> complete multistream set.
>>>
>>> The CSID hardware on SM8250 can demux the input data stream into
>>> maximum of 4 multiple streams depending on virtual channel (vc)
>>> or data type (dt) configuration.
>>>
>>> Situations in which demuxing is useful:
>>> - HDR sensors that produce a 2-frame HDR output, e.g. a light and a dark frame
>>>     (the setup we used for testing, with the imx412 sensor),
>>>     or a 3-frame HDR output - light, medium-lit, dark frame.
>>> - sensors with additional metadata that is streamed over a different
>>>     virtual channel/datatype.
>>> - sensors that produce frames with multiple resolutions in the same pixel
>>>     data stream
>>>
>>> With these changes, the CSID entity has, as it did previously, a single
>>> sink port (0), and always exposes 4 source ports (1, 2,3, 4). The
>>> virtual channel configuration is determined by which of the source ports
>>> are linked to an output VFE line. For example, the link below will
>>> configure the CSID driver to enable vc 0 and vc 1:
>>>
>>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>>> media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>>>
>>> which will be demuxed and propagated into /dev/video0
>>> and /dev/video1 respectively. With this, the userspace can use
>>> any normal V4L2 client app to start/stop/queue/dequeue from these
>>> video nodes. Tested with the yavta app.
>>>
>>> The format of each RDI channel of the used VFE(s) (msm_vfe0_rdi0,
>>> msm_vfe0_rdi1,...) must also be configured explicitly.
>>>
>>> Note that in order to keep a valid internal subdevice state,
>>> setting the sink pad format of the CSID subdevice will propagate
>>> this format to the source pads. However, since the CSID hardware
>>> can demux the input stream into several streams each of which can
>>> be a different format, in that case each source pad's
>>> format must be set individually, e.g.:
>>>
>>> media-ctl -V '"msm_csid0":1[fmt:SRGGB10/3840x2160]'
>>> media-ctl -V '"msm_csid0":2[fmt:SRGGB10/960x540]'
>>>
>>> Milen Mitkov (4):
>>>     media: camss: sm8250: Virtual channels for CSID
>>>     media: camss: vfe: Reserve VFE lines on stream start and link to CSID
>>>     media: camss: vfe-480: Multiple outputs support for SM8250
>>>     media: camss: sm8250: Pipeline starting and stopping for multiple
>>>       virtual channels
>>>
>>>    .../platform/qcom/camss/camss-csid-gen2.c     | 54 ++++++++++------
>>>    .../media/platform/qcom/camss/camss-csid.c    | 44 +++++++++----
>>>    .../media/platform/qcom/camss/camss-csid.h    | 11 +++-
>>>    .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>>    drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++
>>>    .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>    drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>    7 files changed, 140 insertions(+), 60 deletions(-)
>>>
>> I've done some offline work with Milen on this.
>>
>> I'm happy enough to add my
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> to the series. I don't have - currently a VC enabled setup but for the
>> simple case this set doesn't break anything on RB5 for me.
>>
>> ---
>> bod
> This series has my ack.
>
> Acked-by: Robert Foss <robert.foss@linaro.org>

Hi Robert, Bryan et al,

Thanks for the Tested-by and Acked-by.

Do I need to resubmit the patches with the added new tags?

If this is not needed, is there something else I can do to help the 
merging process?


Thank you,

Milen

Re: [PATCH v4 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 14/11/2022 17:18, Milen Mitkov (Consultant) wrote:
> 
> Do I need to resubmit the patches with the added new tags?

Not the committer can do that for you.

> If this is not needed, is there something else I can do to help the 
> merging process?

It looks like Hans has been merging most of this stuff on the 
linux-media side.

+ Hans

---
bod

Re: [PATCH v4 4/4] media: camss: sm8250: Pipeline starting and stopping for multiple virtual channels

[Hans Verkuil]

Hi Milen,

Just a small comment:

On 13/10/2022 14:12, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> Use the multistream series function video_device_pipeline_alloc_start
> to allows multiple clients of the same pipeline.
> 
> If any of the entities in the pipeline doesn't return success at stop
> (e.g. if a VFE line remains running), the full pipeline won't be stopped.
> This allows for stopping and starting streams at any point without
> disrupting the other running streams.
> 
> Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-video.c   | 21 ++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
> index 81fb3a5bc1d5..094971e2ff02 100644
> --- a/drivers/media/platform/qcom/camss/camss-video.c
> +++ b/drivers/media/platform/qcom/camss/camss-video.c
> @@ -351,6 +351,7 @@ static int video_get_subdev_format(struct camss_video *video,
>  	if (subdev == NULL)
>  		return -EPIPE;
>  
> +	memset(&fmt, 0, sizeof(fmt));
>  	fmt.pad = pad;
>  	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  
> @@ -493,9 +494,11 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
>  	struct v4l2_subdev *subdev;
>  	int ret;
>  
> -	ret = video_device_pipeline_start(vdev, &video->pipe);
> -	if (ret < 0)
> +	ret = video_device_pipeline_alloc_start(vdev);
> +	if (ret < 0) {
> +		dev_err(video->camss->dev, "Failed to start media pipeline: %d\n", ret);
>  		return ret;

This should be a 'goto flush_buffers;'.

Regards,

	Hans

> +	}
>  
>  	ret = video_check_format(video);
>  	if (ret < 0)
> @@ -536,6 +539,7 @@ static void video_stop_streaming(struct vb2_queue *q)
>  	struct media_entity *entity;
>  	struct media_pad *pad;
>  	struct v4l2_subdev *subdev;
> +	int ret;
>  
>  	entity = &vdev->entity;
>  	while (1) {
> @@ -550,7 +554,18 @@ static void video_stop_streaming(struct vb2_queue *q)
>  		entity = pad->entity;
>  		subdev = media_entity_to_v4l2_subdev(entity);
>  
> -		v4l2_subdev_call(subdev, video, s_stream, 0);
> +		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
> +
> +		if (ret == -EBUSY) {
> +			/* Don't stop if other instances of the pipeline are still running */
> +			dev_dbg(video->camss->dev, "Video pipeline still used, don't stop streaming.\n");
> +			return;
> +		}
> +
> +		if (ret) {
> +			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
> +			return;
> +		}
>  	}
>  
>  	video_device_pipeline_stop(vdev);