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

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

[quic_mmitkov]

From: Milen Mitkov <quic_mmitkov@quicinc.com>

For v7:
- Fix an issue with output state for different versions of the IFE
  hardware (for platforms different from QRB5, e.g. QRB3).

For v6:
- Fix for a potential race condition in csid
- More detailed description on how to use/test this feature in
  user-space in the last patch.

For v5:
- Use entity->use_count instead of s_stream subdev call ret code to
  check if another instance of the pipeline is running. Prevents an
  error on 6.1 and up, when stopping one of several simultaneous
  instances.
- flush buffers instead of just returning if the pipeline didn't start.

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-170.c |  4 +-
 .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
 .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
 drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
 .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
 drivers/media/platform/qcom/camss/camss.c     |  2 +-
 9 files changed, 138 insertions(+), 64 deletions(-)

-- 
2.37.3

[PATCH v7 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>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-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..6360314f04a6 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -196,6 +196,8 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
 			return ret;
 		}
 
+		csid->phy.need_vc_update = true;
+
 		enable_irq(csid->irq);
 
 		ret = csid->ops->reset(csid);
@@ -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 v7 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 (e.g. already-
running lines stay in ON state, and newly-added lines are RESERVED)

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>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Robert Foss <robert.foss@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe-170.c  | 4 ++--
 drivers/media/platform/qcom/camss/camss-vfe-480.c  | 4 ++--
 drivers/media/platform/qcom/camss/camss-vfe-gen1.c | 4 ++--
 drivers/media/platform/qcom/camss/camss-vfe.c      | 1 +
 drivers/media/platform/qcom/camss/camss.c          | 2 +-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index 8e506a805d11..02494c89da91 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -409,7 +409,7 @@ static int vfe_get_output(struct vfe_line *line)
 	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;
 	}
@@ -462,7 +462,7 @@ static int vfe_enable_output(struct vfe_line *line)
 
 	ops->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);
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index 3aa962b5663b..f03a84daafbe 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -239,7 +239,7 @@ static int vfe_get_output(struct vfe_line *line)
 	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;
 	}
@@ -279,7 +279,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);
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
index 4fd265d01883..239d3d4ac666 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
@@ -194,7 +194,7 @@ static int vfe_enable_output(struct vfe_line *line)
 
 	ops->reg_update_clear(vfe, line->id);
 
-	if (output->state != VFE_OUTPUT_RESERVED) {
+	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);
 		return -EINVAL;
@@ -289,7 +289,7 @@ static int vfe_get_output(struct vfe_line *line)
 	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;
 	}
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index a26e4a5d87b6..e0832f3f4f25 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -740,6 +740,7 @@ static int vfe_set_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		line->output.state = VFE_OUTPUT_RESERVED;
 		ret = vfe->ops->vfe_enable(line);
 		if (ret < 0)
 			dev_err(vfe->camss->dev,
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 9cda284f1e71..547099f8ed14 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 v7 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>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Robert Foss <robert.foss@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-480.c | 57 ++++++++++++-------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index f03a84daafbe..f70aad2e8c23 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,7 +254,6 @@ 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);
 
@@ -246,12 +265,12 @@ static int vfe_get_output(struct vfe_line *line)
 
 	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;
 }
@@ -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);
@@ -566,7 +585,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 v7 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 the VFE entity is used by another instance of the pipeline,
the pipeline won't be stopped. This allows for stopping and starting
streams at any point without disrupting the other running streams.

To prepare and start multiple virtual channels each CSID source pad
corresponding to a virtual channel must be linked to the corresponding
IFE entity. CSID pad 1 (1st source pad) corresponds to virtual
channel 0, CSID pad 2 corresponds to virtual channel 1 and so on.
Each of these must be linked to corresponding IFE RDI port.
E.g. to enable vc 0 on CSID0:

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

To enable vc1 on CSID0:

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

And so on. Note that on SM8250 each CSID is connected, at the
hardware level, to only one IFE. Thus, you must link CSID0
with IFE0, you can't link it with IFE1.

Example: the following media controller setup expects multiplexed
sensor data on CSIPHY2. Data will be passed on to CSID0, which will
demux it to 2 streams - for RDI0 and RD1 ports of IFE0:

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

Note: CSID's entity pad 0 is a sink pad, pads 1..4 are source pads
To start streaming a v4l2 client must open the corresponding
/dev/videoN node. For example, with yavta:

yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 3840x2160 -F /dev/video0
yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 3840x2160 -F /dev/video1

Note that IFEs (vfe0, vfe1) on SM8250 have 3 RDI ports and a single
PIX port and IFELites (vfe2, vfe3) have 4 RDI ports and no PIX port.

Signed-off-by: Milen Mitkov <quic_mmitkov@quicinc.com>
Reviewed-by: Robert Foss <robert.foss@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-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 41deda232e4a..12ac7d4d755e 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);
 		goto flush_buffers;
+	}
 
 	ret = video_check_format(video);
 	if (ret < 0)
@@ -537,6 +540,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) {
@@ -551,7 +555,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 (entity->use_count > 1) {
+			/* 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 v7 0/4] media: camss: sm8250: Virtual channels support for SM8250

[Bryan O'Donoghue]

On 09/12/2022 09:40, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> For v7:
> - Fix an issue with output state for different versions of the IFE
>    hardware (for platforms different from QRB5, e.g. QRB3).
> 

Yep.

Working for me on rb3 now and thank you for updating the git commit in 
patch #4.

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

for the series.

---
bod

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

[Milen Mitkov (Consultant)]

On 09/12/2022 18:17, Bryan O'Donoghue wrote:
> On 09/12/2022 09:40, quic_mmitkov@quicinc.com wrote:
>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>
>> For v7:
>> - Fix an issue with output state for different versions of the IFE
>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>
>
> Yep.
>
> Working for me on rb3 now and thank you for updating the git commit in 
> patch #4.
>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>
> for the series.
>
> ---
> bod


Hi Bryan, Robert, Hans and others,


Happy New Year!

Is there anything else I can/need to do to speed up the merging process 
of this series?


Thanks,

Milen

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

[Bryan O'Donoghue]

On 05/01/2023 08:37, Milen Mitkov (Consultant) wrote:
> On 09/12/2022 18:17, Bryan O'Donoghue wrote:
>> On 09/12/2022 09:40, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v7:
>>> - Fix an issue with output state for different versions of the IFE
>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>
>>
>> Yep.
>>
>> Working for me on rb3 now and thank you for updating the git commit in 
>> patch #4.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> for the series.
>>
>> ---
>> bod
> 
> 
> Hi Bryan, Robert, Hans and others,
> 
> 
> Happy New Year!
> 
> Is there anything else I can/need to do to speed up the merging process 
> of this series?
> 
> 
> Thanks,
> 
> Milen
> 

I don't think so.

Is everything still working on linux-next ?

e45fb347b630e...cc3c08b41a9c9 master        -> linux-next/master

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

[Milen Mitkov (Consultant)]

On 05/01/2023 20:43, Bryan O'Donoghue wrote:
> On 05/01/2023 08:37, Milen Mitkov (Consultant) wrote:
>> On 09/12/2022 18:17, Bryan O'Donoghue wrote:
>>> On 09/12/2022 09:40, quic_mmitkov@quicinc.com wrote:
>>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>>
>>>> For v7:
>>>> - Fix an issue with output state for different versions of the IFE
>>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>>
>>>
>>> Yep.
>>>
>>> Working for me on rb3 now and thank you for updating the git commit 
>>> in patch #4.
>>>
>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>
>>> for the series.
>>>
>>> ---
>>> bod
>>
>>
>> Hi Bryan, Robert, Hans and others,
>>
>>
>> Happy New Year!
>>
>> Is there anything else I can/need to do to speed up the merging 
>> process of this series?
>>
>>
>> Thanks,
>>
>> Milen
>>
>
> I don't think so.
>
> Is everything still working on linux-next ?
>
> e45fb347b630e...cc3c08b41a9c9 master        -> linux-next/master
>
Hi Bryan,

Yes, I took the sm8250_config from 
git.linaro.org/people/bryan.odonoghue/kernel.git, put it on most recent 
master of git.linaro.org/kernel-org/linux-next.git and build with it, 
virtual channels work as expected.


Regard,

Milen

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

[Laurent Pinchart]

Hi Milen,

On Fri, Dec 09, 2022 at 11:40:33AM +0200, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> 
> For v7:
> - Fix an issue with output state for different versions of the IFE
>   hardware (for platforms different from QRB5, e.g. QRB3).
> 
> For v6:
> - Fix for a potential race condition in csid
> - More detailed description on how to use/test this feature in
>   user-space in the last patch.
> 
> For v5:
> - Use entity->use_count instead of s_stream subdev call ret code to
>   check if another instance of the pipeline is running. Prevents an
>   error on 6.1 and up, when stopping one of several simultaneous
>   instances.
> - flush buffers instead of just returning if the pipeline didn't start.
> 
> 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.

I'd like to get a high-level view of the result. Could you provide the
media graph with this series applied (both -p and --print-dot) ?

> 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-170.c |  4 +-
>  .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>  .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>  drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>  .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>  drivers/media/platform/qcom/camss/camss.c     |  2 +-
>  9 files changed, 138 insertions(+), 64 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Milen,

On Fri, Jan 06, 2023 at 11:49:23AM +0200, Milen Mitkov (Consultant) wrote:
> On 05/01/2023 20:43, Bryan O'Donoghue wrote:
> > On 05/01/2023 08:37, Milen Mitkov (Consultant) wrote:
> >> On 09/12/2022 18:17, Bryan O'Donoghue wrote:
> >>> On 09/12/2022 09:40, quic_mmitkov@quicinc.com wrote:
> >>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
> >>>>
> >>>> For v7:
> >>>> - Fix an issue with output state for different versions of the IFE
> >>>>    hardware (for platforms different from QRB5, e.g. QRB3).
> >>>>
> >>>
> >>> Yep.
> >>>
> >>> Working for me on rb3 now and thank you for updating the git commit 
> >>> in patch #4.
> >>>
> >>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>>
> >>> for the series.
> >>>
> >>> ---
> >>> bod
> >>
> >> Hi Bryan, Robert, Hans and others,
> >>
> >> Happy New Year!
> >>
> >> Is there anything else I can/need to do to speed up the merging 
> >> process of this series?
> >>
> >> Thanks,
> >>
> >> Milen
> >
> > I don't think so.
> >
> > Is everything still working on linux-next ?
> >
> > e45fb347b630e...cc3c08b41a9c9 master        -> linux-next/master
>
> Hi Bryan,
> 
> Yes, I took the sm8250_config from 
> git.linaro.org/people/bryan.odonoghue/kernel.git, put it on most recent 
> master of git.linaro.org/kernel-org/linux-next.git and build with it, 
> virtual channels work as expected.

I haven't had much time to review the series so far, sorry about that.
Spare review time is limited, and I've recently focussed on the streams
API to get it merged in v6.3. I'll try to review your patches in that
context.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Sun, Jan 08, 2023 at 04:51:22AM +0200, Laurent Pinchart wrote:
> Hi Milen,
> 
> On Fri, Dec 09, 2022 at 11:40:33AM +0200, quic_mmitkov@quicinc.com wrote:
> > From: Milen Mitkov <quic_mmitkov@quicinc.com>
> > 
> > For v7:
> > - Fix an issue with output state for different versions of the IFE
> >   hardware (for platforms different from QRB5, e.g. QRB3).
> > 
> > For v6:
> > - Fix for a potential race condition in csid
> > - More detailed description on how to use/test this feature in
> >   user-space in the last patch.
> > 
> > For v5:
> > - Use entity->use_count instead of s_stream subdev call ret code to
> >   check if another instance of the pipeline is running. Prevents an
> >   error on 6.1 and up, when stopping one of several simultaneous
> >   instances.
> > - flush buffers instead of just returning if the pipeline didn't start.
> > 
> > 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.

Another question, how does this work when demultiplexing different data
types for the same virtual channel ?

> > 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.
> 
> I'd like to get a high-level view of the result. Could you provide the
> media graph with this series applied (both -p and --print-dot) ?
> 
> > 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-170.c |  4 +-
> >  .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
> >  .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
> >  drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
> >  .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
> >  drivers/media/platform/qcom/camss/camss.c     |  2 +-
> >  9 files changed, 138 insertions(+), 64 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Milen Mitkov (Consultant)]

On 08/01/2023 04:54, Laurent Pinchart wrote:
> On Sun, Jan 08, 2023 at 04:51:22AM +0200, Laurent Pinchart wrote:
>> Hi Milen,
>>
>> On Fri, Dec 09, 2022 at 11:40:33AM +0200, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v7:
>>> - Fix an issue with output state for different versions of the IFE
>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>
>>> For v6:
>>> - Fix for a potential race condition in csid
>>> - More detailed description on how to use/test this feature in
>>>    user-space in the last patch.
>>>
>>> For v5:
>>> - Use entity->use_count instead of s_stream subdev call ret code to
>>>    check if another instance of the pipeline is running. Prevents an
>>>    error on 6.1 and up, when stopping one of several simultaneous
>>>    instances.
>>> - flush buffers instead of just returning if the pipeline didn't start.
>>>
>>> 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.
> Another question, how does this work when demultiplexing different data
> types for the same virtual channel ?
>
>>> 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.
>> I'd like to get a high-level view of the result. Could you provide the
>> media graph with this series applied (both -p and --print-dot) ?
>>
>>> 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-170.c |  4 +-
>>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>>>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>   9 files changed, 138 insertions(+), 64 deletions(-)

Hi Laurent,

Happy New Year!

Would be good if you can help us merge this series in the upcoming merge 
window. I will answer your 3 questions here.

Q: I'd like to get a high-level view of the result. Could you provide 
the media graph with this series applied (both -p and --print-dot) ?

A: As I am not sure what is the policy with regards to mail attachments 
on the kernel mailing lists I've base64 encoded a small archive that has 
4 different media-ctl dumps: -p before and after media links setup (for 
2 virtual channels demuxing on CSID0) and --print-dot before and after 
media links setup. It can be found here:

https://pastes.io/sp0pkgrgfh

Copy/paste the content to a file and: base64 -d <file> | tar -gz

p.s. if there's another way to share files here please mention it :)


Q: How does this work when demultiplexing different data types for the 
same virtual channel ?

A: Our open-source customers defined the scope of work and they needed 
virtual channels demuxing support only. Demuxing via datatype is not 
supported currently.


Q: I was wondering if Qualcomm had a spare RB5 vision development kit 
that I could use for testing, both for this series, and for future 
development (including the libcamera side).

A: We have to check internally as there's a limited supply of RB5 
boards. We'll get back to you.


Thanks,

Milen

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

[Milen Mitkov (Consultant)]

On 09/12/2022 11:40, quic_mmitkov@quicinc.com wrote:
> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>
> For v7:
> - Fix an issue with output state for different versions of the IFE
>    hardware (for platforms different from QRB5, e.g. QRB3).
>
> For v6:
> - Fix for a potential race condition in csid
> - More detailed description on how to use/test this feature in
>    user-space in the last patch.
>
> For v5:
> - Use entity->use_count instead of s_stream subdev call ret code to
>    check if another instance of the pipeline is running. Prevents an
>    error on 6.1 and up, when stopping one of several simultaneous
>    instances.
> - flush buffers instead of just returning if the pipeline didn't start.
>
> 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-170.c |  4 +-
>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>   9 files changed, 138 insertions(+), 64 deletions(-)

Hi guys,

just a ping for this series.

Laurent, I sent you an email with answers to the questions you 
requested. I read your reply that you'll review these changes in the 
context of the multi-stream API, but this series doesn't really use the 
multi-stream API, just a note.

Cheers,

Milen

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

[Milen Mitkov (Consultant)]

On 31/01/2023 11:00, Milen Mitkov (Consultant) wrote:
> On 09/12/2022 11:40, quic_mmitkov@quicinc.com wrote:
>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>
>> For v7:
>> - Fix an issue with output state for different versions of the IFE
>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>
>> For v6:
>> - Fix for a potential race condition in csid
>> - More detailed description on how to use/test this feature in
>>    user-space in the last patch.
>>
>> For v5:
>> - Use entity->use_count instead of s_stream subdev call ret code to
>>    check if another instance of the pipeline is running. Prevents an
>>    error on 6.1 and up, when stopping one of several simultaneous
>>    instances.
>> - flush buffers instead of just returning if the pipeline didn't start.
>>
>> 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-170.c |  4 +-
>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>   9 files changed, 138 insertions(+), 64 deletions(-)
>
> Hi guys,
>
> just a ping for this series.
>
> Laurent, I sent you an email with answers to the questions you 
> requested. I read your reply that you'll review these changes in the 
> context of the multi-stream API, but this series doesn't really use 
> the multi-stream API, just a note.
>
> Cheers,
>
> Milen
>
Hi there,

Just another ping..:)

Please let me know if there's anything I could do (improve/fix/code 
differently/etc.) to help get this series merged.


Best Regards,

Milen

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

[Bryan O'Donoghue]

On 20/02/2023 12:18, Milen Mitkov (Consultant) wrote:
> On 31/01/2023 11:00, Milen Mitkov (Consultant) wrote:
>> On 09/12/2022 11:40, quic_mmitkov@quicinc.com wrote:
>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>
>>> For v7:
>>> - Fix an issue with output state for different versions of the IFE
>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>
>>> For v6:
>>> - Fix for a potential race condition in csid
>>> - More detailed description on how to use/test this feature in
>>>    user-space in the last patch.
>>>
>>> For v5:
>>> - Use entity->use_count instead of s_stream subdev call ret code to
>>>    check if another instance of the pipeline is running. Prevents an
>>>    error on 6.1 and up, when stopping one of several simultaneous
>>>    instances.
>>> - flush buffers instead of just returning if the pipeline didn't start.
>>>
>>> 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-170.c |  4 +-
>>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 ++++++++++++-------
>>>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>>>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>   9 files changed, 138 insertions(+), 64 deletions(-)
>>
>> Hi guys,
>>
>> just a ping for this series.
>>
>> Laurent, I sent you an email with answers to the questions you 
>> requested. I read your reply that you'll review these changes in the 
>> context of the multi-stream API, but this series doesn't really use 
>> the multi-stream API, just a note.
>>
>> Cheers,
>>
>> Milen
>>
> Hi there,
> 
> Just another ping..:)
> 
> Please let me know if there's anything I could do (improve/fix/code 
> differently/etc.) to help get this series merged.
> 
> 
> Best Regards,
> 
> Milen
> 
> 
> 

Well, we need to re-verify it works on linux-next.

Other than that it seems OK to me.

---
bod

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

[Milen Mitkov (Consultant)]

On 20/02/2023 14:26, Bryan O'Donoghue wrote:
> On 20/02/2023 12:18, Milen Mitkov (Consultant) wrote:
>> On 31/01/2023 11:00, Milen Mitkov (Consultant) wrote:
>>> On 09/12/2022 11:40, quic_mmitkov@quicinc.com wrote:
>>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>>
>>>> For v7:
>>>> - Fix an issue with output state for different versions of the IFE
>>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>>
>>>> For v6:
>>>> - Fix for a potential race condition in csid
>>>> - More detailed description on how to use/test this feature in
>>>>    user-space in the last patch.
>>>>
>>>> For v5:
>>>> - Use entity->use_count instead of s_stream subdev call ret code to
>>>>    check if another instance of the pipeline is running. Prevents an
>>>>    error on 6.1 and up, when stopping one of several simultaneous
>>>>    instances.
>>>> - flush buffers instead of just returning if the pipeline didn't 
>>>> start.
>>>>
>>>> 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-170.c |  4 +-
>>>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61 
>>>> ++++++++++++-------
>>>>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>>>>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>>>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>>   9 files changed, 138 insertions(+), 64 deletions(-)
>>>
>>> Hi guys,
>>>
>>> just a ping for this series.
>>>
>>> Laurent, I sent you an email with answers to the questions you 
>>> requested. I read your reply that you'll review these changes in the 
>>> context of the multi-stream API, but this series doesn't really use 
>>> the multi-stream API, just a note.
>>>
>>> Cheers,
>>>
>>> Milen
>>>
>> Hi there,
>>
>> Just another ping..:)
>>
>> Please let me know if there's anything I could do (improve/fix/code 
>> differently/etc.) to help get this series merged.
>>
>>
>> Best Regards,
>>
>> Milen
>>
>>
>>
>
> Well, we need to re-verify it works on linux-next.
>
> Other than that it seems OK to me.
>
> ---
> bod

Thanks Bryan,

I just re-tested on latest linux-next (next-20230221 tag) and the set 
applies and, judging by the standard set of tests I did, works as 
expected and doesn't break anything.


Best Regards,

Milen

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

[Azam Sadiq Pasha Kapatrala Syed]

+ Nico (Linaro)
Hi Team

Would like to know if anything is pending form our end as we want to get the patches mainlined?

Thanks,
Azam

-----Original Message-----
From: Milen Mitkov (Consultant) (QUIC) <quic_mmitkov@quicinc.com> 
Sent: Tuesday, February 21, 2023 12:44 AM
To: bryan.odonoghue@linaro.org; linux-media@vger.kernel.org; linux-arm-msm@vger.kernel.org; Azam Sadiq Pasha Kapatrala Syed <akapatra@quicinc.com>; Jigarkumar Zala <jzala@quicinc.com>; todor.too@gmail.com
Cc: agross@kernel.org; konrad.dybcio@somainline.org; mchehab@kernel.org; Chandan Gera <cgera@qti.qualcomm.com>; Guru Chinnabhandar <gchinnab@quicinc.com>; Alireza Yasan <ayasan@qti.qualcomm.com>; laurent.pinchart@ideasonboard.com
Subject: Re: [PATCH v7 0/4] media: camss: sm8250: Virtual channels support for SM8250


On 20/02/2023 14:26, Bryan O'Donoghue wrote:
> On 20/02/2023 12:18, Milen Mitkov (Consultant) wrote:
>> On 31/01/2023 11:00, Milen Mitkov (Consultant) wrote:
>>> On 09/12/2022 11:40, quic_mmitkov@quicinc.com wrote:
>>>> From: Milen Mitkov <quic_mmitkov@quicinc.com>
>>>>
>>>> For v7:
>>>> - Fix an issue with output state for different versions of the IFE
>>>>    hardware (for platforms different from QRB5, e.g. QRB3).
>>>>
>>>> For v6:
>>>> - Fix for a potential race condition in csid
>>>> - More detailed description on how to use/test this feature in
>>>>    user-space in the last patch.
>>>>
>>>> For v5:
>>>> - Use entity->use_count instead of s_stream subdev call ret code to
>>>>    check if another instance of the pipeline is running. Prevents 
>>>> an
>>>>    error on 6.1 and up, when stopping one of several simultaneous
>>>>    instances.
>>>> - flush buffers instead of just returning if the pipeline didn't 
>>>> start.
>>>>
>>>> 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-170.c |  4 +-
>>>>   .../media/platform/qcom/camss/camss-vfe-480.c | 61
>>>> ++++++++++++-------
>>>>   .../platform/qcom/camss/camss-vfe-gen1.c      |  4 +-
>>>>   drivers/media/platform/qcom/camss/camss-vfe.c |  1 +
>>>>   .../media/platform/qcom/camss/camss-video.c   | 21 ++++++-
>>>>   drivers/media/platform/qcom/camss/camss.c     |  2 +-
>>>>   9 files changed, 138 insertions(+), 64 deletions(-)
>>>
>>> Hi guys,
>>>
>>> just a ping for this series.
>>>
>>> Laurent, I sent you an email with answers to the questions you 
>>> requested. I read your reply that you'll review these changes in the 
>>> context of the multi-stream API, but this series doesn't really use 
>>> the multi-stream API, just a note.
>>>
>>> Cheers,
>>>
>>> Milen
>>>
>> Hi there,
>>
>> Just another ping..:)
>>
>> Please let me know if there's anything I could do (improve/fix/code
>> differently/etc.) to help get this series merged.
>>
>>
>> Best Regards,
>>
>> Milen
>>
>>
>>
>
> Well, we need to re-verify it works on linux-next.
>
> Other than that it seems OK to me.
>
> ---
> bod

Thanks Bryan,

I just re-tested on latest linux-next (next-20230221 tag) and the set applies and, judging by the standard set of tests I did, works as expected and doesn't break anything.


Best Regards,

Milen

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

[Bryan O'Donoghue]

On 31/03/2023 07:20, Azam Sadiq Pasha Kapatrala Syed wrote:
> + Nico (Linaro)
> Hi Team
> 
> Would like to know if anything is pending form our end as we want to get the patches mainlined?
> 
> Thanks,
> Azam

I'd like to get a clearer picture on this

[   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

Using the IMX577 sensor on the RB5 we get his pretty odd virtual 
channels mask.

If userspace is sending this in, the question I have is why. Surely with 
a sensor that doesn't have a VC there should be no impact on user-space.

---
bod

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

[Bryan O'Donoghue]

On 03/04/2023 09:38, Milen Mitkov (Consultant) wrote:
> On 31/03/2023 11:07, Bryan O'Donoghue wrote:
>> On 31/03/2023 07:20, Azam Sadiq Pasha Kapatrala Syed wrote:
>>> + Nico (Linaro)
>>> Hi Team
>>>
>>> Would like to know if anything is pending form our end as we want to 
>>> get the patches mainlined?
>>>
>>> Thanks,
>>> Azam
>>
>> I'd like to get a clearer picture on this
>>
>> [   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
>>
>> Using the IMX577 sensor on the RB5 we get his pretty odd virtual 
>> channels mask.
>>
>> If userspace is sending this in, the question I have is why. Surely 
>> with a sensor that doesn't have a VC there should be no impact on 
>> user-space.
>>
>> ---
>> bod
> 
> Hey Bryan,
> 
> what media-ctl commands are you using? I can't recreate this on my side. 
> I am using this set of commands to test (with the default imx577 driver 
> without any multiple virtual channels outputs) and am getting only the 
> first message (virtual channels mask 0x1):
> 
> 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
> 
> 
> Thanks,
> 
> Milen

Its a dev_dbg() so "#define DEBUG 1" in 
drivers/media/platform/qcom/camss/camss-csid.c

---
bod

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

[Milen Mitkov (Consultant)]

On 03/04/2023 12:16, Bryan O'Donoghue wrote:
> On 03/04/2023 09:38, Milen Mitkov (Consultant) wrote:
>> On 31/03/2023 11:07, Bryan O'Donoghue wrote:
>>> On 31/03/2023 07:20, Azam Sadiq Pasha Kapatrala Syed wrote:
>>>> + Nico (Linaro)
>>>> Hi Team
>>>>
>>>> Would like to know if anything is pending form our end as we want 
>>>> to get the patches mainlined?
>>>>
>>>> Thanks,
>>>> Azam
>>>
>>> I'd like to get a clearer picture on this
>>>
>>> [   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
>>>
>>> Using the IMX577 sensor on the RB5 we get his pretty odd virtual 
>>> channels mask.
>>>
>>> If userspace is sending this in, the question I have is why. Surely 
>>> with a sensor that doesn't have a VC there should be no impact on 
>>> user-space.
>>>
>>> ---
>>> bod
>>
>> Hey Bryan,
>>
>> what media-ctl commands are you using? I can't recreate this on my 
>> side. I am using this set of commands to test (with the default 
>> imx577 driver without any multiple virtual channels outputs) and am 
>> getting only the first message (virtual channels mask 0x1):
>>
>> 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
>>
>>
>> Thanks,
>>
>> Milen
>
> Its a dev_dbg() so "#define DEBUG 1" in 
> drivers/media/platform/qcom/camss/camss-csid.c
>
> ---
> bod
>
Yes, I enabled the dev_dbg(). I just see only one message with regards 
to the channels mask. Just this one:

[   90.546365] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
virtual channels mask 0x1

so I wonder if you're testing with a different set of media-ctl/yavta 
commands.


Regards,

Milen

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

[Bryan O'Donoghue]

On 03/04/2023 10:20, Milen Mitkov (Consultant) wrote:
> 
> On 03/04/2023 12:16, Bryan O'Donoghue wrote:
>> On 03/04/2023 09:38, Milen Mitkov (Consultant) wrote:
>>> On 31/03/2023 11:07, Bryan O'Donoghue wrote:
>>>> On 31/03/2023 07:20, Azam Sadiq Pasha Kapatrala Syed wrote:
>>>>> + Nico (Linaro)
>>>>> Hi Team
>>>>>
>>>>> Would like to know if anything is pending form our end as we want 
>>>>> to get the patches mainlined?
>>>>>
>>>>> Thanks,
>>>>> Azam
>>>>
>>>> I'd like to get a clearer picture on this
>>>>
>>>> [   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
>>>>
>>>> Using the IMX577 sensor on the RB5 we get his pretty odd virtual 
>>>> channels mask.
>>>>
>>>> If userspace is sending this in, the question I have is why. Surely 
>>>> with a sensor that doesn't have a VC there should be no impact on 
>>>> user-space.
>>>>
>>>> ---
>>>> bod
>>>
>>> Hey Bryan,
>>>
>>> what media-ctl commands are you using? I can't recreate this on my 
>>> side. I am using this set of commands to test (with the default 
>>> imx577 driver without any multiple virtual channels outputs) and am 
>>> getting only the first message (virtual channels mask 0x1):
>>>
>>> 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
>>>
>>>
>>> Thanks,
>>>
>>> Milen
>>
>> Its a dev_dbg() so "#define DEBUG 1" in 
>> drivers/media/platform/qcom/camss/camss-csid.c
>>
>> ---
>> bod
>>
> Yes, I enabled the dev_dbg(). I just see only one message with regards 
> to the channels mask. Just this one:
> 
> [   90.546365] qcom-camss ac6a000.camss: csid_link_setup: Enabled CSID 
> virtual channels mask 0x1
> 
> so I wonder if you're testing with a different set of media-ctl/yavta 
> commands.

No. I'm asking about how this _used_ to be.

If the iteration through the masks went away - then why did it go away ? 
libcamera version, a change you have made since - V5 I think it was when 
we discussed this last ?

---
bod

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

[Milen Mitkov (Consultant)]

On 03/04/2023 12:36, Bryan O'Donoghue wrote:
> On 03/04/2023 10:20, Milen Mitkov (Consultant) wrote:
>>
>> On 03/04/2023 12:16, Bryan O'Donoghue wrote:
>>> On 03/04/2023 09:38, Milen Mitkov (Consultant) wrote:
>>>> On 31/03/2023 11:07, Bryan O'Donoghue wrote:
>>>>> On 31/03/2023 07:20, Azam Sadiq Pasha Kapatrala Syed wrote:
>>>>>> + Nico (Linaro)
>>>>>> Hi Team
>>>>>>
>>>>>> Would like to know if anything is pending form our end as we want 
>>>>>> to get the patches mainlined?
>>>>>>
>>>>>> Thanks,
>>>>>> Azam
>>>>>
>>>>> I'd like to get a clearer picture on this
>>>>>
>>>>> [   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
>>>>>
>>>>> Using the IMX577 sensor on the RB5 we get his pretty odd virtual 
>>>>> channels mask.
>>>>>
>>>>> If userspace is sending this in, the question I have is why. 
>>>>> Surely with a sensor that doesn't have a VC there should be no 
>>>>> impact on user-space.
>>>>>
>>>>> ---
>>>>> bod
>>>>
>>>> Hey Bryan,
>>>>
>>>> what media-ctl commands are you using? I can't recreate this on my 
>>>> side. I am using this set of commands to test (with the default 
>>>> imx577 driver without any multiple virtual channels outputs) and am 
>>>> getting only the first message (virtual channels mask 0x1):
>>>>
>>>> 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
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Milen
>>>
>>> Its a dev_dbg() so "#define DEBUG 1" in 
>>> drivers/media/platform/qcom/camss/camss-csid.c
>>>
>>> ---
>>> bod
>>>
>> Yes, I enabled the dev_dbg(). I just see only one message with 
>> regards to the channels mask. Just this one:
>>
>> [   90.546365] qcom-camss ac6a000.camss: csid_link_setup: Enabled 
>> CSID virtual channels mask 0x1
>>
>> so I wonder if you're testing with a different set of media-ctl/yavta 
>> commands.
>
> No. I'm asking about how this _used_ to be.
>
> If the iteration through the masks went away - then why did it go away 
> ? libcamera version, a change you have made since - V5 I think it was 
> when we discussed this last ?
>
> ---
> bod
>
Hi Bryan,

no, the iteration through the mask didn't go away? The print shows up 
when the csid entity's source pad(s) enables the link to the ife sink 
pad(s). Maybe the client (libcamera?) decides to disable this link for 
some reason?

Regards,

Milen

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

[Bryan O'Donoghue]

On 03/04/2023 12:01, Milen Mitkov (Consultant) wrote:
> Hi Bryan,
> 
> no, the iteration through the mask didn't go away? The print shows up 
> when the csid entity's source pad(s) enables the link to the ife sink 
> pad(s). Maybe the client (libcamera?) decides to disable this link for 
> some reason?
> 
> Regards,
> 
> Milen

So previously we had one CSI device in user-space and after your change 
we have one CSI device per VC, correct ?

---
bod

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

[Milen Mitkov (Consultant)]

On 03/04/2023 15:10, Bryan O'Donoghue wrote:
> On 03/04/2023 12:01, Milen Mitkov (Consultant) wrote:
>> Hi Bryan,
>>
>> no, the iteration through the mask didn't go away? The print shows up 
>> when the csid entity's source pad(s) enables the link to the ife sink 
>> pad(s). Maybe the client (libcamera?) decides to disable this link 
>> for some reason?
>>
>> Regards,
>>
>> Milen
>
> So previously we had one CSI device in user-space and after your 
> change we have one CSI device per VC, correct ?
>
> ---
> bod

With these changes there's still one CSID device/media entity, but it 
has more source pads (4 vs 1 previously).

Regards,

Milen

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

[Bryan O'Donoghue]

On 03/04/2023 13:16, Milen Mitkov (Consultant) wrote:
> 
> On 03/04/2023 15:10, Bryan O'Donoghue wrote:
>> On 03/04/2023 12:01, Milen Mitkov (Consultant) wrote:
>>> Hi Bryan,
>>>
>>> no, the iteration through the mask didn't go away? The print shows up 
>>> when the csid entity's source pad(s) enables the link to the ife sink 
>>> pad(s). Maybe the client (libcamera?) decides to disable this link 
>>> for some reason?
>>>
>>> Regards,
>>>
>>> Milen
>>
>> So previously we had one CSI device in user-space and after your 
>> change we have one CSI device per VC, correct ?
>>
>> ---
>> bod
> 
> With these changes there's still one CSID device/media entity, but it 
> has more source pads (4 vs 1 previously).
> 
> Regards,
> 
> Milen
> 

OK.

I took the time to apply this series to my development tree.

Before:
- entity 19: msm_csid0 (2 pads, 10 links)
              type V4L2 subdev subtype Unknown flags 0
              device node name /dev/v4l-subdev6
         pad0: Sink
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 <- "msm_csiphy0":1 []
                 <- "msm_csiphy1":1 []
                 <- "msm_csiphy2":1 []
                 <- "msm_csiphy3":1 []
                 <- "msm_csiphy4":1 []
                 <- "msm_csiphy5":1 []
         pad1: Source
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 -> "msm_vfe0_rdi0":0 []
                 -> "msm_vfe1_rdi0":0 []
                 -> "msm_vfe2_rdi0":0 []
                 -> "msm_vfe3_rdi0":0 []

After:
- entity 19: msm_csid0 (5 pads, 22 links)
              type V4L2 subdev subtype Unknown flags 0
              device node name /dev/v4l-subdev6
         pad0: Sink
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 <- "msm_csiphy0":1 []
                 <- "msm_csiphy1":1 []
                 <- "msm_csiphy2":1 [ENABLED]
                 <- "msm_csiphy3":1 []
                 <- "msm_csiphy4":1 []
                 <- "msm_csiphy5":1 []
         pad1: Source
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 -> "msm_vfe0_rdi0":0 [ENABLED]
                 -> "msm_vfe1_rdi0":0 []
                 -> "msm_vfe2_rdi0":0 []
                 -> "msm_vfe3_rdi0":0 []
         pad2: Source
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 -> "msm_vfe0_rdi1":0 []
                 -> "msm_vfe1_rdi1":0 []
                 -> "msm_vfe2_rdi1":0 []
                 -> "msm_vfe3_rdi1":0 []
         pad3: Source
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 -> "msm_vfe0_rdi2":0 []
                 -> "msm_vfe1_rdi2":0 []
                 -> "msm_vfe2_rdi2":0 []
                 -> "msm_vfe3_rdi2":0 []
         pad4: Source
                 [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
                 -> "msm_vfe0_pix":0 []
                 -> "msm_vfe1_pix":0 []
                 -> "msm_vfe2_pix":0 []
                 -> "msm_vfe3_pix":0 []

So that's consistent and this worked for me.

media-ctl --reset
media-ctl -v -d /dev/media0 -V '"imx577 
'20-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]'
build/yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F 
/dev/video0

For the series.

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

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

[Azam Sadiq Pasha Kapatrala Syed]

-----Original Message-----
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org> 
Sent: Tuesday, April 4, 2023 12:26 AM
To: Azam Sadiq Pasha Kapatrala Syed <akapatra@quicinc.com>; nicolas.dechesne@linaro.org
Subject: Fwd: [PATCH v7 0/4] media: camss: sm8250: Virtual channels support for SM8250

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

-------- Forwarded Message --------
Subject: Re: [PATCH v7 0/4] media: camss: sm8250: Virtual channels support for SM8250
Date: Tue, 4 Apr 2023 08:25:17 +0100
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: Milen Mitkov (Consultant) <quic_mmitkov@quicinc.com>, laurent.pinchart@ideasonboard.com, hverkuil-cisco@xs4all.nl

On 04/04/2023 01:06, Bryan O'Donoghue wrote:
> On 03/04/2023 13:16, Milen Mitkov (Consultant) wrote:
>>
>> On 03/04/2023 15:10, Bryan O'Donoghue wrote:
>>> On 03/04/2023 12:01, Milen Mitkov (Consultant) wrote:
>>>> Hi Bryan,
>>>>
>>>> no, the iteration through the mask didn't go away? The print shows 
>>>> up when the csid entity's source pad(s) enables the link to the ife 
>>>> sink pad(s). Maybe the client (libcamera?) decides to disable this 
>>>> link for some reason?
>>>>
>>>> Regards,
>>>>
>>>> Milen
>>>
>>> So previously we had one CSI device in user-space and after your 
>>> change we have one CSI device per VC, correct ?
>>>
>>> ---
>>> bod
>>
>> With these changes there's still one CSID device/media entity, but it 
>> has more source pads (4 vs 1 previously).
>>
>> Regards,
>>
>> Milen
>>
>
> OK.
>
> I took the time to apply this series to my development tree.
>
> Before:
> - entity 19: msm_csid0 (2 pads, 10 links)
>               type V4L2 subdev subtype Unknown flags 0
>               device node name /dev/v4l-subdev6
>          pad0: Sink
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  <- "msm_csiphy0":1 []
>                  <- "msm_csiphy1":1 []
>                  <- "msm_csiphy2":1 []
>                  <- "msm_csiphy3":1 []
>                  <- "msm_csiphy4":1 []
>                  <- "msm_csiphy5":1 []
>          pad1: Source
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  -> "msm_vfe0_rdi0":0 []
>                  -> "msm_vfe1_rdi0":0 []
>                  -> "msm_vfe2_rdi0":0 []
>                  -> "msm_vfe3_rdi0":0 []
>
> After:
> - entity 19: msm_csid0 (5 pads, 22 links)
>               type V4L2 subdev subtype Unknown flags 0
>               device node name /dev/v4l-subdev6
>          pad0: Sink
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  <- "msm_csiphy0":1 []
>                  <- "msm_csiphy1":1 []
>                  <- "msm_csiphy2":1 [ENABLED]
>                  <- "msm_csiphy3":1 []
>                  <- "msm_csiphy4":1 []
>                  <- "msm_csiphy5":1 []
>          pad1: Source
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  -> "msm_vfe0_rdi0":0 [ENABLED]
>                  -> "msm_vfe1_rdi0":0 []
>                  -> "msm_vfe2_rdi0":0 []
>                  -> "msm_vfe3_rdi0":0 []
>          pad2: Source
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  -> "msm_vfe0_rdi1":0 []
>                  -> "msm_vfe1_rdi1":0 []
>                  -> "msm_vfe2_rdi1":0 []
>                  -> "msm_vfe3_rdi1":0 []
>          pad3: Source
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  -> "msm_vfe0_rdi2":0 []
>                  -> "msm_vfe1_rdi2":0 []
>                  -> "msm_vfe2_rdi2":0 []
>                  -> "msm_vfe3_rdi2":0 []
>          pad4: Source
>                  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:srgb]
>                  -> "msm_vfe0_pix":0 []
>                  -> "msm_vfe1_pix":0 []
>                  -> "msm_vfe2_pix":0 []
>                  -> "msm_vfe3_pix":0 []
>
> So that's consistent and this worked for me.
>
> media-ctl --reset
> media-ctl -v -d /dev/media0 -V '"imx577
> '20-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]'
> build/yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F
> /dev/video0
>
> For the series.
>
> Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

> Hey Hans, Laurent.

> Do you guys have any thoughts on picking Milen's series up ?

 > From my perspective it works well enough on the SM8250 hardware.

> ---
> bod

Hi Team - Appreciate for any feedback on when we can get this series picked up. As this will help customers to directly pick the latest Kernel instead of applying patches to each kernel version.

Thanks,
Azam