[PATCH v3 00/13] IPU6 driver cleanups and fixes

[PATCH v3 00/13] IPU6 driver cleanups and fixes

[Sakari Ailus]

Hello all,

This small set contains cleanups and fixes for the IPU6 driver. I'm
preparing a larger set of improvements in the IPU6 driver on top of the
metadata series (I'll post an update soonish) so consider this to be
preparation for that.

since v2:

- Factor in Mehdi's comments:

  - Rework commit message of "media: ipu6: Remove redundant streaming
    start via buffer queueing" patch a little.

  - Also remove IPU6_ISYS_BUFFER_LIST_FL_SET_STATE flag (patch "media:
    ipu6: Drop error argument from ipu6_isys_stream_start()").

since v1:

- Fix some intra-set compilation breakage and remove a now-redundant
  variable.

- Fix missing assignment of ret in ipu6_isys_link_fmt_validate() (includes
  a cleanup, too).

Sakari Ailus (13):
  media: ipu6: Ensure stream_mutex is acquired when dealing with node
    list
  media: ipu6: Drop MMU hardware initialisation in probe()
  media: ipu6: Remove redundant driver data checks
  media: ipu6: Make symbols static
  media: ipu6: Remove redundant streaming start via buffer queueing
  media: ipu6: Don't check pipeline in stream_start
  media: ipu6: Close firmware streams on streaming enable failure
  media: ipu6: Drop error argument from ipu6_isys_stream_start()
  media: ipu6: Obtain remote pad using media_pad_remote_pad_unique()
  media: ipu6: Obtain unique source pad from remote sub-device
  media: ipu6: Remove source_entity from struct ipu6_isys_stream
  media: ipu6: Drop custom functions to obtain sd state information
  media: ipu6: Always call video_device_pipeline_alloc_start()

 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c |  2 +-
 .../media/pci/intel/ipu6/ipu6-isys-queue.c    | 73 ++++++++--------
 .../media/pci/intel/ipu6/ipu6-isys-queue.h    |  1 -
 .../media/pci/intel/ipu6/ipu6-isys-subdev.c   | 36 --------
 .../media/pci/intel/ipu6/ipu6-isys-subdev.h   |  4 -
 .../media/pci/intel/ipu6/ipu6-isys-video.c    | 83 ++++++-------------
 .../media/pci/intel/ipu6/ipu6-isys-video.h    |  6 +-
 drivers/media/pci/intel/ipu6/ipu6-isys.c      | 17 +---
 drivers/media/pci/intel/ipu6/ipu6-isys.h      |  2 -
 9 files changed, 71 insertions(+), 153 deletions(-)

-- 
2.47.3

[PATCH v3 01/13] media: ipu6: Ensure stream_mutex is acquired when dealing with node list

[Sakari Ailus]

The ipu6 isys driver maintains the list of video buffer queues related to
a stream (in ipu6 context streams on the same CSI-2 virtual channel) and
this list is modified through VIDIOC_STREAMON and VIDIOC_STREAMOFF IOCTLs.
Ensure the common mutex is acquired when accessing the linked list, i.e.
the isys device context's stream_mutex.

Add a lockdep assert to ipu6_isys_get_buffer_list() and switch to guard()
while at it as the error handling becomes more simple this way.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index aa2cf7287477..8f05987cdb4e 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2013--2024 Intel Corporation
  */
 #include <linux/atomic.h>
+#include <linux/cleanup.h>
 #include <linux/bug.h>
 #include <linux/device.h>
 #include <linux/list.h>
@@ -201,6 +202,8 @@ static int buffer_list_get(struct ipu6_isys_stream *stream,
 	unsigned long flags;
 	unsigned long buf_flag = IPU6_ISYS_BUFFER_LIST_FL_INCOMING;
 
+	lockdep_assert_held(&stream->mutex);
+
 	bl->nbufs = 0;
 	INIT_LIST_HEAD(&bl->head);
 
@@ -294,9 +297,8 @@ static int ipu6_isys_stream_start(struct ipu6_isys_video *av,
 	struct ipu6_isys_buffer_list __bl;
 	int ret;
 
-	mutex_lock(&stream->isys->stream_mutex);
+	guard(mutex)(&stream->isys->stream_mutex);
 	ret = ipu6_isys_video_set_streaming(av, 1, bl);
-	mutex_unlock(&stream->isys->stream_mutex);
 	if (ret)
 		goto out_requeue;
 
@@ -637,10 +639,10 @@ static void stop_streaming(struct vb2_queue *q)
 	mutex_lock(&av->isys->stream_mutex);
 	if (stream->nr_streaming == stream->nr_queues && stream->streaming)
 		ipu6_isys_video_set_streaming(av, 0, NULL);
+	list_del(&aq->node);
 	mutex_unlock(&av->isys->stream_mutex);
 
 	stream->nr_streaming--;
-	list_del(&aq->node);
 	stream->streaming = 0;
 	mutex_unlock(&stream->mutex);
 
-- 
2.47.3

[PATCH v3 02/13] media: ipu6: Drop MMU hardware initialisation in probe()

[Sakari Ailus]

The MMU hardware is initialised in the runtime PM resume callback. Do not
do it in probe().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index fc0ec0a4b8f5..eb45544a9f05 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -1070,10 +1070,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
 	if (!isys->csi2)
 		return -ENOMEM;
 
-	ret = ipu6_mmu_hw_init(adev->mmu);
-	if (ret)
-		return ret;
-
 	/* initial sensor type */
 	isys->sensor_type = isys->pdata->ipdata->sensor_type_start;
 
-- 
2.47.3

[PATCH v3 03/13] media: ipu6: Remove redundant driver data checks

[Sakari Ailus]

Both runtime PM resume and suspend callbacks check whether the driver's
data is set for the device. This is done in probe(); drop the redundant
checks.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index eb45544a9f05..4855eeb24980 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -857,9 +857,6 @@ static int isys_runtime_pm_resume(struct device *dev)
 	unsigned long flags;
 	int ret;
 
-	if (!isys)
-		return 0;
-
 	ret = ipu6_mmu_hw_init(adev->mmu);
 	if (ret)
 		return ret;
@@ -884,13 +881,9 @@ static int isys_runtime_pm_resume(struct device *dev)
 static int isys_runtime_pm_suspend(struct device *dev)
 {
 	struct ipu6_bus_device *adev = to_ipu6_bus_device(dev);
-	struct ipu6_isys *isys;
+	struct ipu6_isys *isys = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	isys = dev_get_drvdata(dev);
-	if (!isys)
-		return 0;
-
 	spin_lock_irqsave(&isys->power_lock, flags);
 	isys->power = 0;
 	spin_unlock_irqrestore(&isys->power_lock, flags);
-- 
2.47.3

[PATCH v3 04/13] media: ipu6: Make symbols static

[Sakari Ailus]

Make isys_setup_hw and isys_isr static as they're only used in a single
file.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys.c | 4 ++--
 drivers/media/pci/intel/ipu6/ipu6-isys.h | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index 4855eeb24980..1b527d9156e2 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -269,7 +269,7 @@ static int isys_register_video_devices(struct ipu6_isys *isys)
 	return ret;
 }
 
-void isys_setup_hw(struct ipu6_isys *isys)
+static void isys_setup_hw(struct ipu6_isys *isys)
 {
 	void __iomem *base = isys->pdata->base;
 	const u8 *thd = isys->pdata->ipdata->hw_variant.cdc_fifo_threshold;
@@ -341,7 +341,7 @@ static void ipu6_isys_csi2_isr(struct ipu6_isys_csi2 *csi2)
 	}
 }
 
-irqreturn_t isys_isr(struct ipu6_bus_device *adev)
+static irqreturn_t isys_isr(struct ipu6_bus_device *adev)
 {
 	struct ipu6_isys *isys = ipu6_bus_get_drvdata(adev);
 	void __iomem *base = isys->pdata->base;
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.h b/drivers/media/pci/intel/ipu6/ipu6-isys.h
index 0e2c8b71edfc..7fb8cb820912 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.h
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.h
@@ -181,8 +181,6 @@ void ipu6_cleanup_fw_msg_bufs(struct ipu6_isys *isys);
 
 extern const struct v4l2_ioctl_ops ipu6_isys_ioctl_ops;
 
-void isys_setup_hw(struct ipu6_isys *isys);
-irqreturn_t isys_isr(struct ipu6_bus_device *adev);
 void update_watermark_setting(struct ipu6_isys *isys);
 
 int ipu6_isys_mcd_phy_set_power(struct ipu6_isys *isys,
-- 
2.47.3

[PATCH v3 05/13] media: ipu6: Remove redundant streaming start via buffer queueing

[Sakari Ailus]

The videobuf2 framework will ensure buffers are queued before streaming is
started. Remove support for starting starting streaming via the
buf_queue() callback.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index 8f05987cdb4e..fdf41b3cf60e 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -408,13 +408,6 @@ static void buf_queue(struct vb2_buffer *vb)
 	ipu6_isys_buf_to_fw_frame_buf(buf, stream, &bl);
 	ipu6_fw_isys_dump_frame_buff_set(dev, buf, stream->nr_output_pins);
 
-	if (!stream->streaming) {
-		ret = ipu6_isys_stream_start(av, &bl, true);
-		if (ret)
-			dev_err(dev, "stream start failed.\n");
-		goto out;
-	}
-
 	/*
 	 * We must queue the buffers in the buffer list to the
 	 * appropriate video buffer queues BEFORE passing them to the
-- 
2.47.3

[PATCH v3 06/13] media: ipu6: Don't check pipeline in stream_start

[Sakari Ailus]

A pipeline exists when start_streaming has returned so the check for
start_streaming_called is equivalent to having media_pipeline. Use
vb2_start_streaming_called() to perform the check.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index fdf41b3cf60e..dcad6aafee29 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -355,8 +355,6 @@ static void buf_queue(struct vb2_buffer *vb)
 		vb2_buffer_to_ipu6_isys_video_buffer(vvb);
 	struct ipu6_isys_buffer *ib = &ivb->ib;
 	struct device *dev = &av->isys->adev->auxdev.dev;
-	struct media_pipeline *media_pipe =
-		media_entity_pipeline(&av->vdev.entity);
 	struct ipu6_fw_isys_frame_buff_set_abi *buf = NULL;
 	struct ipu6_isys_stream *stream = av->stream;
 	struct ipu6_isys_buffer_list bl;
@@ -374,8 +372,8 @@ static void buf_queue(struct vb2_buffer *vb)
 	list_add(&ib->head, &aq->incoming);
 	spin_unlock_irqrestore(&aq->lock, flags);
 
-	if (!media_pipe || !vb->vb2_queue->start_streaming_called) {
-		dev_dbg(dev, "media pipeline is not ready for %s\n",
+	if (!vb2_start_streaming_called(vb->vb2_queue)) {
+		dev_dbg(dev, "start_streaming hasn't been called yet on %s\n",
 			av->vdev.name);
 		return;
 	}
-- 
2.47.3

[PATCH v3 07/13] media: ipu6: Close firmware streams on streaming enable failure

[Sakari Ailus]

When enabling streaming fails, the stream is stopped in firmware but not
closed. Do this to release resources on firmware side.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
index dec8f5ffcfa5..919b77107cef 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1066,6 +1066,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
 
 out_media_entity_stop_streaming_firmware:
 	stop_streaming_firmware(av);
+	close_streaming_firmware(av);
 
 	return ret;
 }
-- 
2.47.3

[PATCH v3 08/13] media: ipu6: Drop error argument from ipu6_isys_stream_start()

[Sakari Ailus]

error argument for ipu6_isys_stream_start() is always false, remove the
argument. The IPU6_ISYS_BUFFER_LIST_FL_SET_STATE buffer flag also becomes
redundant as a result, remove it as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 12 +++---------
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.h |  1 -
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index dcad6aafee29..0e9f0025aeb3 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -132,9 +132,6 @@ void ipu6_isys_buffer_list_queue(struct ipu6_isys_buffer_list *bl,
 			list_add_tail(&ib->head, &aq->incoming);
 		spin_unlock_irqrestore(&aq->lock, flags);
 
-		if (op_flags & IPU6_ISYS_BUFFER_LIST_FL_SET_STATE)
-			vb2_buffer_done(vb, state);
-
 		if (first) {
 			dev_dbg(dev,
 				"queue buf list %p flags %lx, s %d, %d bufs\n",
@@ -290,7 +287,7 @@ ipu6_isys_buf_to_fw_frame_buf(struct ipu6_fw_isys_frame_buff_set_abi *set,
 
 /* Start streaming for real. The buffer list must be available. */
 static int ipu6_isys_stream_start(struct ipu6_isys_video *av,
-				  struct ipu6_isys_buffer_list *bl, bool error)
+				  struct ipu6_isys_buffer_list *bl)
 {
 	struct ipu6_isys_stream *stream = av->stream;
 	struct device *dev = &stream->isys->adev->auxdev.dev;
@@ -336,10 +333,7 @@ static int ipu6_isys_stream_start(struct ipu6_isys_video *av,
 out_requeue:
 	if (bl && bl->nbufs)
 		ipu6_isys_buffer_list_queue(bl,
-					    IPU6_ISYS_BUFFER_LIST_FL_INCOMING |
-					    (error ?
-					    IPU6_ISYS_BUFFER_LIST_FL_SET_STATE :
-					     0), error ? VB2_BUF_STATE_ERROR :
+					    IPU6_ISYS_BUFFER_LIST_FL_INCOMING,
 					    VB2_BUF_STATE_QUEUED);
 	flush_firmware_streamon_fail(stream);
 
@@ -590,7 +584,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 		goto out;
 	}
 
-	ret = ipu6_isys_stream_start(av, bl, false);
+	ret = ipu6_isys_stream_start(av, bl);
 	if (ret)
 		goto out_stream_start;
 
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
index 844dfda15ab6..dec1fed44dd2 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
@@ -39,7 +39,6 @@ struct ipu6_isys_video_buffer {
 
 #define IPU6_ISYS_BUFFER_LIST_FL_INCOMING	BIT(0)
 #define IPU6_ISYS_BUFFER_LIST_FL_ACTIVE	BIT(1)
-#define IPU6_ISYS_BUFFER_LIST_FL_SET_STATE	BIT(2)
 
 struct ipu6_isys_buffer_list {
 	struct list_head head;
-- 
2.47.3

[PATCH v3 09/13] media: ipu6: Obtain remote pad using media_pad_remote_pad_unique()

[Sakari Ailus]

There's no reason to use media_entity_remote_source_pad_unique() as we
know our pads. Use media_pad_remote_pad_unique() instead.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
index 43a2a16a3c2a..7e539a0c6c92 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
@@ -88,7 +88,7 @@ s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
 	if (!csi2)
 		return -EINVAL;
 
-	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
+	src_pad = media_pad_remote_pad_unique(&csi2->asd.sd.entity.pads[CSI2_PAD_SINK]);
 	if (IS_ERR(src_pad)) {
 		dev_err(&csi2->isys->adev->auxdev.dev,
 			"can't get source pad of %s (%pe)\n",
-- 
2.47.3

[PATCH v3 10/13] media: ipu6: Obtain unique source pad from remote sub-device

[Sakari Ailus]

Obtain unique source pad from a remote sub-device, instead of the first
one. This means that only one link may be active at stream start. There's
no functional change in practice, unless multiple CSI-2 transmitters are
directly connected to the receiver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
index 919b77107cef..fb319d623a11 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1205,10 +1205,10 @@ int ipu6_isys_setup_video(struct ipu6_isys_video *av,
 
 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 	asd = to_ipu6_isys_subdev(remote_sd);
-	source_pad = media_pad_remote_pad_first(&remote_pad->entity->pads[0]);
-	if (!source_pad) {
+	source_pad = media_pad_remote_pad_unique(&remote_pad->entity->pads[0]);
+	if (IS_ERR(source_pad)) {
 		dev_dbg(dev, "No external source entity\n");
-		return -ENODEV;
+		return PTR_ERR(source_pad);
 	}
 
 	*source_entity = source_pad->entity;
-- 
2.47.3

[PATCH v3 11/13] media: ipu6: Remove source_entity from struct ipu6_isys_stream

[Sakari Ailus]

Remove source_entity from struct ipu6_isys_stream and instead pass it on
in function arguments.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../media/pci/intel/ipu6/ipu6-isys-queue.c    | 22 ++++++++--
 .../media/pci/intel/ipu6/ipu6-isys-video.c    | 42 +++++--------------
 .../media/pci/intel/ipu6/ipu6-isys-video.h    |  6 +--
 3 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index 0e9f0025aeb3..651ddab9ef14 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -532,14 +532,28 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 		ipu6_isys_get_isys_format(ipu6_isys_get_format(av), 0);
 	struct ipu6_isys_buffer_list __bl, *bl = NULL;
 	struct ipu6_isys_stream *stream;
-	struct media_entity *source_entity = NULL;
+	struct media_pad *source_pad, *remote_pad;
 	int nr_queues, ret;
 
 	dev_dbg(dev, "stream: %s: width %u, height %u, css pixelformat %u\n",
 		av->vdev.name, ipu6_isys_get_frame_width(av),
 		ipu6_isys_get_frame_height(av), pfmt->css_pixelformat);
 
-	ret = ipu6_isys_setup_video(av, &source_entity, &nr_queues);
+	remote_pad = media_pad_remote_pad_unique(&av->pad);
+	if (IS_ERR(remote_pad)) {
+		dev_dbg(dev, "failed to get remote pad\n");
+		ret = PTR_ERR(remote_pad);
+		goto out_return_buffers;
+	}
+
+	source_pad = media_pad_remote_pad_unique(&remote_pad->entity->pads[0]);
+	if (IS_ERR(source_pad)) {
+		dev_dbg(dev, "No external source entity\n");
+		ret = PTR_ERR(source_pad);
+		goto out_return_buffers;
+	}
+
+	ret = ipu6_isys_setup_video(av, remote_pad, source_pad, &nr_queues);
 	if (ret < 0) {
 		dev_dbg(dev, "failed to setup video\n");
 		goto out_return_buffers;
@@ -560,7 +574,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 	stream = av->stream;
 	mutex_lock(&stream->mutex);
 	if (!stream->nr_streaming) {
-		ret = ipu6_isys_video_prepare_stream(av, source_entity,
+		ret = ipu6_isys_video_prepare_stream(av, source_pad->entity,
 						     nr_queues);
 		if (ret)
 			goto out_fw_close;
@@ -571,7 +585,7 @@ static int start_streaming(struct vb2_queue *q, unsigned int count)
 		stream->nr_queues);
 
 	list_add(&aq->node, &stream->queues);
-	ipu6_isys_configure_stream_watermark(av, true);
+	ipu6_isys_configure_stream_watermark(av, source_pad->entity);
 	ipu6_isys_update_stream_watermark(av, true);
 
 	if (stream->nr_streaming != stream->nr_queues)
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
index fb319d623a11..141f0e72c5c8 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -745,17 +745,16 @@ int ipu6_isys_video_prepare_stream(struct ipu6_isys_video *av,
 	stream->stream_source = stream->asd->source;
 	csi2 = ipu6_isys_subdev_to_csi2(stream->asd);
 	csi2->receiver_errors = 0;
-	stream->source_entity = source_entity;
 
 	dev_dbg(&av->isys->adev->auxdev.dev,
 		"prepare stream: external entity %s\n",
-		stream->source_entity->name);
+		source_entity->name);
 
 	return 0;
 }
 
 void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
-					  bool state)
+					  struct media_entity *source)
 {
 	struct ipu6_isys *isys = av->isys;
 	struct ipu6_isys_csi2 *csi2 = NULL;
@@ -769,10 +768,7 @@ void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
 	u64 pixel_rate = 0;
 	int ret;
 
-	if (!state)
-		return;
-
-	esd = media_entity_to_v4l2_subdev(av->stream->source_entity);
+	esd = media_entity_to_v4l2_subdev(source);
 
 	av->watermark.width = ipu6_isys_get_frame_width(av);
 	av->watermark.height = ipu6_isys_get_frame_height(av);
@@ -804,7 +800,7 @@ void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
 		iwake_watermark->force_iwake_disable = true;
 		mutex_unlock(&iwake_watermark->mutex);
 		dev_warn(dev, "unexpected pixel_rate from %s, disable iwake.\n",
-			 av->stream->source_entity->name);
+			 source->name);
 	}
 }
 
@@ -1011,9 +1007,6 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
 
 	dev_dbg(dev, "set stream: %d\n", state);
 
-	if (WARN(!stream->source_entity, "No source entity for stream\n"))
-		return -ENODEV;
-
 	sd = &stream->asd->sd;
 	r_pad = media_pad_remote_pad_first(&av->pad);
 	r_stream = ipu6_isys_get_src_stream_by_src_pad(sd, r_pad->index);
@@ -1180,7 +1173,8 @@ void ipu6_isys_fw_close(struct ipu6_isys *isys)
 }
 
 int ipu6_isys_setup_video(struct ipu6_isys_video *av,
-			  struct media_entity **source_entity, int *nr_queues)
+			  struct media_pad *remote_pad,
+			  struct media_pad *source_pad, int *nr_queues)
 {
 	const struct ipu6_isys_pixelformat *pfmt =
 		ipu6_isys_get_isys_format(ipu6_isys_get_format(av), 0);
@@ -1189,30 +1183,14 @@ int ipu6_isys_setup_video(struct ipu6_isys_video *av,
 	struct v4l2_subdev_route *route = NULL;
 	struct v4l2_subdev_route *r;
 	struct v4l2_subdev_state *state;
-	struct ipu6_isys_subdev *asd;
-	struct v4l2_subdev *remote_sd;
+	struct v4l2_subdev *remote_sd =
+		media_entity_to_v4l2_subdev(remote_pad->entity);
+	struct ipu6_isys_subdev *asd = to_ipu6_isys_subdev(remote_sd);
 	struct media_pipeline *pipeline;
-	struct media_pad *source_pad, *remote_pad;
 	int ret = -EINVAL;
 
 	*nr_queues = 0;
 
-	remote_pad = media_pad_remote_pad_unique(&av->pad);
-	if (IS_ERR(remote_pad)) {
-		dev_dbg(dev, "failed to get remote pad\n");
-		return PTR_ERR(remote_pad);
-	}
-
-	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
-	asd = to_ipu6_isys_subdev(remote_sd);
-	source_pad = media_pad_remote_pad_unique(&remote_pad->entity->pads[0]);
-	if (IS_ERR(source_pad)) {
-		dev_dbg(dev, "No external source entity\n");
-		return PTR_ERR(source_pad);
-	}
-
-	*source_entity = source_pad->entity;
-
 	/* Find the root */
 	state = v4l2_subdev_lock_and_get_active_state(remote_sd);
 	for_each_active_route(&state->routing, r) {
@@ -1232,7 +1210,7 @@ int ipu6_isys_setup_video(struct ipu6_isys_video *av,
 
 	ret = ipu6_isys_csi2_get_remote_desc(av->source_stream,
 					     to_ipu6_isys_csi2(asd),
-					     *source_entity, &entry);
+					     source_pad->entity, &entry);
 	if (ret == -ENOIOCTLCMD) {
 		av->vc = 0;
 		av->dt = ipu6_isys_mbus_code_to_mipi(pfmt->code);
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.h b/drivers/media/pci/intel/ipu6/ipu6-isys-video.h
index 1dd36f2a077e..2ff53315d7b9 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.h
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.h
@@ -43,7 +43,6 @@ struct sequence_info {
  */
 struct ipu6_isys_stream {
 	struct mutex mutex;
-	struct media_entity *source_entity;
 	atomic_t sequence;
 	unsigned int seq_index;
 	struct sequence_info seq[IPU6_ISYS_MAX_PARALLEL_SOF];
@@ -113,7 +112,8 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
 int ipu6_isys_fw_open(struct ipu6_isys *isys);
 void ipu6_isys_fw_close(struct ipu6_isys *isys);
 int ipu6_isys_setup_video(struct ipu6_isys_video *av,
-			  struct media_entity **source_entity, int *nr_queues);
+			  struct media_pad *remote_pad,
+			  struct media_pad *source_pad, int *nr_queues);
 int ipu6_isys_video_init(struct ipu6_isys_video *av);
 void ipu6_isys_video_cleanup(struct ipu6_isys_video *av);
 void ipu6_isys_put_stream(struct ipu6_isys_stream *stream);
@@ -123,7 +123,7 @@ struct ipu6_isys_stream *
 ipu6_isys_query_stream_by_source(struct ipu6_isys *isys, int source, u8 vc);
 
 void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
-					  bool state);
+					  struct media_entity *source);
 void ipu6_isys_update_stream_watermark(struct ipu6_isys_video *av, bool state);
 
 u32 ipu6_isys_get_format(struct ipu6_isys_video *av);
-- 
2.47.3

[PATCH v3 12/13] media: ipu6: Drop custom functions to obtain sd state information

[Sakari Ailus]

Drop the custom functions that are used to obtain information from the
sub-device state.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../media/pci/intel/ipu6/ipu6-isys-queue.c    | 17 ++++++---
 .../media/pci/intel/ipu6/ipu6-isys-subdev.c   | 36 -------------------
 .../media/pci/intel/ipu6/ipu6-isys-subdev.h   |  4 ---
 .../media/pci/intel/ipu6/ipu6-isys-video.c    | 33 +++++++----------
 4 files changed, 24 insertions(+), 66 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index 651ddab9ef14..c862de31af9c 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -420,7 +420,7 @@ static void buf_queue(struct vb2_buffer *vb)
 
 static int ipu6_isys_link_fmt_validate(struct ipu6_isys_queue *aq)
 {
-	struct v4l2_mbus_framefmt format;
+	struct v4l2_mbus_framefmt format, *__format;
 	struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
 	struct device *dev = &av->isys->adev->auxdev.dev;
 	struct media_pad *remote_pad =
@@ -435,13 +435,20 @@ static int ipu6_isys_link_fmt_validate(struct ipu6_isys_queue *aq)
 	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 	r_stream = ipu6_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
 
-	ret = ipu6_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
-					   &format);
+	struct v4l2_subdev_state *state =
+		v4l2_subdev_lock_and_get_active_state(sd);
 
-	if (ret) {
+	__format = v4l2_subdev_state_get_format(state, remote_pad->index,
+						r_stream);
+	if (__format)
+		format = *__format;
+
+	v4l2_subdev_unlock_state(state);
+
+	if (!__format) {
 		dev_dbg(dev, "failed to get %s: pad %d, stream:%d format\n",
 			sd->entity.name, remote_pad->index, r_stream);
-		return ret;
+		return -EPIPE;
 	}
 
 	if (format.width != ipu6_isys_get_frame_width(av) ||
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
index 869e7d4ba572..dbd6f76a066d 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
@@ -265,42 +265,6 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
 	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
 }
 
-int ipu6_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
-				 struct v4l2_mbus_framefmt *format)
-{
-	struct v4l2_mbus_framefmt *fmt;
-	struct v4l2_subdev_state *state;
-
-	if (!sd || !format)
-		return -EINVAL;
-
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-	fmt = v4l2_subdev_state_get_format(state, pad, stream);
-	if (fmt)
-		*format = *fmt;
-	v4l2_subdev_unlock_state(state);
-
-	return fmt ? 0 : -EINVAL;
-}
-
-int ipu6_isys_get_stream_pad_crop(struct v4l2_subdev *sd, u32 pad, u32 stream,
-				  struct v4l2_rect *crop)
-{
-	struct v4l2_subdev_state *state;
-	struct v4l2_rect *rect;
-
-	if (!sd || !crop)
-		return -EINVAL;
-
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-	rect = v4l2_subdev_state_get_crop(state, pad, stream);
-	if (rect)
-		*crop = *rect;
-	v4l2_subdev_unlock_state(state);
-
-	return rect ? 0 : -EINVAL;
-}
-
 u32 ipu6_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
 {
 	struct v4l2_subdev_state *state;
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
index 268dfa01e903..35069099c364 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
@@ -38,10 +38,6 @@ int ipu6_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_mbus_code_enum
 				    *code);
 u32 ipu6_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
-int ipu6_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
-				 struct v4l2_mbus_framefmt *format);
-int ipu6_isys_get_stream_pad_crop(struct v4l2_subdev *sd, u32 pad, u32 stream,
-				  struct v4l2_rect *crop);
 int ipu6_isys_subdev_set_routing(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 enum v4l2_subdev_format_whence which,
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
index 141f0e72c5c8..c7f9f888c46d 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -455,6 +455,7 @@ static int ipu6_isys_fw_pin_cfg(struct ipu6_isys_video *av,
 {
 	struct media_pad *src_pad = media_pad_remote_pad_first(&av->pad);
 	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(src_pad->entity);
+	struct v4l2_subdev_state *state = v4l2_subdev_get_locked_active_state(sd);
 	struct ipu6_fw_isys_input_pin_info_abi *input_pin;
 	struct ipu6_fw_isys_output_pin_info_abi *output_pin;
 	struct ipu6_isys_stream *stream = av->stream;
@@ -464,26 +465,13 @@ static int ipu6_isys_fw_pin_cfg(struct ipu6_isys_video *av,
 		ipu6_isys_get_isys_format(ipu6_isys_get_format(av), 0);
 	struct v4l2_rect v4l2_crop;
 	struct ipu6_isys *isys = av->isys;
-	struct device *dev = &isys->adev->auxdev.dev;
 	int input_pins = cfg->nof_input_pins++;
 	int output_pins;
 	u32 src_stream;
-	int ret;
 
 	src_stream = ipu6_isys_get_src_stream_by_src_pad(sd, src_pad->index);
-	ret = ipu6_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
-					   &fmt);
-	if (ret < 0) {
-		dev_err(dev, "can't get stream format (%d)\n", ret);
-		return ret;
-	}
-
-	ret = ipu6_isys_get_stream_pad_crop(sd, src_pad->index, src_stream,
-					    &v4l2_crop);
-	if (ret < 0) {
-		dev_err(dev, "can't get stream crop (%d)\n", ret);
-		return ret;
-	}
+	fmt = *v4l2_subdev_state_get_format(state, src_pad->index, src_stream);
+	v4l2_crop = *v4l2_subdev_state_get_crop(state, src_pad->index, src_stream);
 
 	input_pin = &cfg->input_pins[input_pins];
 	input_pin->input_res.width = fmt.width;
@@ -784,13 +772,16 @@ void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
 	csi2 = ipu6_isys_subdev_to_csi2(av->stream->asd);
 	link_freq = ipu6_isys_csi2_get_link_freq(csi2);
 	if (link_freq > 0) {
+		struct v4l2_subdev_state *state =
+			v4l2_subdev_lock_and_get_active_state(&csi2->asd.sd);
+
 		lanes = csi2->nlanes;
-		ret = ipu6_isys_get_stream_pad_fmt(&csi2->asd.sd, 0,
-						   av->source_stream, &format);
-		if (!ret) {
-			bpp = ipu6_isys_mbus_code_to_bpp(format.code);
-			pixel_rate = mul_u64_u32_div(link_freq, lanes * 2, bpp);
-		}
+		format = *v4l2_subdev_state_get_format(state, 0,
+						       av->source_stream);
+		bpp = ipu6_isys_mbus_code_to_bpp(format.code);
+		pixel_rate = mul_u64_u32_div(link_freq, lanes * 2, bpp);
+
+		v4l2_subdev_unlock_state(state);
 	}
 
 	av->watermark.pixel_rate = pixel_rate;
-- 
2.47.3

[PATCH v3 13/13] media: ipu6: Always call video_device_pipeline_alloc_start()

[Sakari Ailus]

Even if a video device is part of a pipeline already,
video_device_pipeline_alloc_start() handles that case gracefully. Don't
explicitly differentiate between video_device_pipeline_start() and
video_device_pipeline_alloc_start() based on the existence of a pipeline.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 1 -
 drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
index c862de31af9c..fabaed63df0c 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
@@ -427,7 +427,6 @@ static int ipu6_isys_link_fmt_validate(struct ipu6_isys_queue *aq)
 		media_pad_remote_pad_first(av->vdev.entity.pads);
 	struct v4l2_subdev *sd;
 	u32 r_stream, code;
-	int ret;
 
 	if (!remote_pad)
 		return -ENOTCONN;
diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
index c7f9f888c46d..9da7ac85e02e 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1177,7 +1177,6 @@ int ipu6_isys_setup_video(struct ipu6_isys_video *av,
 	struct v4l2_subdev *remote_sd =
 		media_entity_to_v4l2_subdev(remote_pad->entity);
 	struct ipu6_isys_subdev *asd = to_ipu6_isys_subdev(remote_sd);
-	struct media_pipeline *pipeline;
 	int ret = -EINVAL;
 
 	*nr_queues = 0;
@@ -1217,11 +1216,7 @@ int ipu6_isys_setup_video(struct ipu6_isys_video *av,
 		return ret;
 	}
 
-	pipeline = media_entity_pipeline(&av->vdev.entity);
-	if (!pipeline)
-		ret = video_device_pipeline_alloc_start(&av->vdev);
-	else
-		ret = video_device_pipeline_start(&av->vdev, pipeline);
+	ret = video_device_pipeline_alloc_start(&av->vdev);
 	if (ret < 0) {
 		dev_dbg(dev, "media pipeline start failed\n");
 		return ret;
-- 
2.47.3

Re: [PATCH v3 02/13] media: ipu6: Drop MMU hardware initialisation in probe()

[Bingbu Cao]

Sakari,

Thanks for the patch.

On 12/30/25 9:10 PM, Sakari Ailus wrote:
> The MMU hardware is initialised in the runtime PM resume callback. Do not
> do it in probe().

It's correct. The MMU hardware initialization was there as firmware
open happen during ISYS probe before, but now it was moved at stream
starting stage.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> index fc0ec0a4b8f5..eb45544a9f05 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> @@ -1070,10 +1070,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  	if (!isys->csi2)
>  		return -ENOMEM;
>  
> -	ret = ipu6_mmu_hw_init(adev->mmu);
> -	if (ret)
> -		return ret;
> -

So, also need to remove the ipu6_mmu_hw_cleanup() below. :)

>  	/* initial sensor type */
>  	isys->sensor_type = isys->pdata->ipdata->sensor_type_start;
>  
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 03/13] media: ipu6: Remove redundant driver data checks

[Bingbu Cao]

Sakari,

Thanks for this cleanup.

On 12/30/25 9:10 PM, Sakari Ailus wrote:
> Both runtime PM resume and suspend callbacks check whether the driver's
> data is set for the device. This is done in probe(); drop the redundant
> checks.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> index eb45544a9f05..4855eeb24980 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> @@ -857,9 +857,6 @@ static int isys_runtime_pm_resume(struct device *dev)
>  	unsigned long flags;
>  	int ret;
>  
> -	if (!isys)
> -		return 0;
> -
>  	ret = ipu6_mmu_hw_init(adev->mmu);
>  	if (ret)
>  		return ret;
> @@ -884,13 +881,9 @@ static int isys_runtime_pm_resume(struct device *dev)
>  static int isys_runtime_pm_suspend(struct device *dev)
>  {
>  	struct ipu6_bus_device *adev = to_ipu6_bus_device(dev);
> -	struct ipu6_isys *isys;
> +	struct ipu6_isys *isys = dev_get_drvdata(dev);
>  	unsigned long flags;
>  
> -	isys = dev_get_drvdata(dev);
> -	if (!isys)
> -		return 0;
> -
>  	spin_lock_irqsave(&isys->power_lock, flags);
>  	isys->power = 0;
>  	spin_unlock_irqrestore(&isys->power_lock, flags);
> 

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 04/13] media: ipu6: Make symbols static

[Bingbu Cao]

On 12/30/25 9:10 PM, Sakari Ailus wrote:
> Make isys_setup_hw and isys_isr static as they're only used in a single
> file.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 4 ++--
>  drivers/media/pci/intel/ipu6/ipu6-isys.h | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> index 4855eeb24980..1b527d9156e2 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> @@ -269,7 +269,7 @@ static int isys_register_video_devices(struct ipu6_isys *isys)
>  	return ret;
>  }
>  
> -void isys_setup_hw(struct ipu6_isys *isys)
> +static void isys_setup_hw(struct ipu6_isys *isys)
>  {
>  	void __iomem *base = isys->pdata->base;
>  	const u8 *thd = isys->pdata->ipdata->hw_variant.cdc_fifo_threshold;
> @@ -341,7 +341,7 @@ static void ipu6_isys_csi2_isr(struct ipu6_isys_csi2 *csi2)
>  	}
>  }
>  
> -irqreturn_t isys_isr(struct ipu6_bus_device *adev)
> +static irqreturn_t isys_isr(struct ipu6_bus_device *adev)
>  {
>  	struct ipu6_isys *isys = ipu6_bus_get_drvdata(adev);
>  	void __iomem *base = isys->pdata->base;
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.h b/drivers/media/pci/intel/ipu6/ipu6-isys.h
> index 0e2c8b71edfc..7fb8cb820912 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys.h
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.h
> @@ -181,8 +181,6 @@ void ipu6_cleanup_fw_msg_bufs(struct ipu6_isys *isys);
>  
>  extern const struct v4l2_ioctl_ops ipu6_isys_ioctl_ops;
>  
> -void isys_setup_hw(struct ipu6_isys *isys);
> -irqreturn_t isys_isr(struct ipu6_bus_device *adev);
>  void update_watermark_setting(struct ipu6_isys *isys);
>  
>  int ipu6_isys_mcd_phy_set_power(struct ipu6_isys *isys,
> 

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 05/13] media: ipu6: Remove redundant streaming start via buffer queueing

[Bingbu Cao]

Hi, Sakari,

On 12/30/25 9:10 PM, Sakari Ailus wrote:
> The videobuf2 framework will ensure buffers are queued before streaming is
> started.

The logic is only applicable for !start_streaming_called, right?
I am not sure it work for multiplex streams like metadata + frame
capture case.

>Remove support for starting starting streaming via the

double 'starting' here.

> buf_queue() callback.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> index 8f05987cdb4e..fdf41b3cf60e 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> @@ -408,13 +408,6 @@ static void buf_queue(struct vb2_buffer *vb)
>  	ipu6_isys_buf_to_fw_frame_buf(buf, stream, &bl);
>  	ipu6_fw_isys_dump_frame_buff_set(dev, buf, stream->nr_output_pins);
>  
> -	if (!stream->streaming) {
> -		ret = ipu6_isys_stream_start(av, &bl, true);
> -		if (ret)
> -			dev_err(dev, "stream start failed.\n");
> -		goto out;
> -	}
> -
>  	/*
>  	 * We must queue the buffers in the buffer list to the
>  	 * appropriate video buffer queues BEFORE passing them to the
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 07/13] media: ipu6: Close firmware streams on streaming enable failure

[Bingbu Cao]

Sakari,

Thanks for the patch.

On 12/30/25 9:10 PM, Sakari Ailus wrote:
> When enabling streaming fails, the stream is stopped in firmware but not
> closed. Do this to release resources on firmware side.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index dec8f5ffcfa5..919b77107cef 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -1066,6 +1066,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
>  
>  out_media_entity_stop_streaming_firmware:
>  	stop_streaming_firmware(av);
> +	close_streaming_firmware(av);

It looks the close_streaming_firmware() has no chance to run if
v4l2_subdev_disable_streams() above failed. Beside, the
stop_streaming_firmware() is better called after subdev
disable_streams().

>  
>  	return ret;
>  }
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 02/13] media: ipu6: Drop MMU hardware initialisation in probe()

[Sakari Ailus]

Hi Bingbu,

Thanks for the review.

On Wed, Dec 31, 2025 at 01:38:00PM +0800, Bingbu Cao wrote:
> Sakari,
> 
> Thanks for the patch.
> 
> On 12/30/25 9:10 PM, Sakari Ailus wrote:
> > The MMU hardware is initialised in the runtime PM resume callback. Do not
> > do it in probe().
> 
> It's correct. The MMU hardware initialization was there as firmware
> open happen during ISYS probe before, but now it was moved at stream
> starting stage.
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/pci/intel/ipu6/ipu6-isys.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> > index fc0ec0a4b8f5..eb45544a9f05 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> > @@ -1070,10 +1070,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
> >  	if (!isys->csi2)
> >  		return -ENOMEM;
> >  
> > -	ret = ipu6_mmu_hw_init(adev->mmu);
> > -	if (ret)
> > -		return ret;
> > -
> 
> So, also need to remove the ipu6_mmu_hw_cleanup() below. :)

I'll add that for v4.

> 
> >  	/* initial sensor type */
> >  	isys->sensor_type = isys->pdata->ipdata->sensor_type_start;
> >  
> > 
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 05/13] media: ipu6: Remove redundant streaming start via buffer queueing

[Sakari Ailus]

Hi Bingbu,

On Wed, Dec 31, 2025 at 02:01:14PM +0800, Bingbu Cao wrote:
> Hi, Sakari,
> 
> On 12/30/25 9:10 PM, Sakari Ailus wrote:
> > The videobuf2 framework will ensure buffers are queued before streaming is
> > started.
> 
> The logic is only applicable for !start_streaming_called, right?
> I am not sure it work for multiplex streams like metadata + frame
> capture case.

The start_streaming() callback is only called after the minimum number of
buffers specified for the queue (1 in this case) have been queued. So in
case of multiple streams from the same CSI-2 receiver, it's a
start_streaming() callback on one of the queues that triggers starting
streaming from the source -- not the buf_queue() callback.

FWIW, multiple streams don't work with the driver right now anyway -- this
is addressed by the streaming series I hope I'll be able to post soon,
after addressing some more review feedback.

> 
> >Remove support for starting starting streaming via the
> 
> double 'starting' here.

Fixed for v4.

> 
> > buf_queue() callback.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> > index 8f05987cdb4e..fdf41b3cf60e 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> > @@ -408,13 +408,6 @@ static void buf_queue(struct vb2_buffer *vb)
> >  	ipu6_isys_buf_to_fw_frame_buf(buf, stream, &bl);
> >  	ipu6_fw_isys_dump_frame_buff_set(dev, buf, stream->nr_output_pins);
> >  
> > -	if (!stream->streaming) {
> > -		ret = ipu6_isys_stream_start(av, &bl, true);
> > -		if (ret)
> > -			dev_err(dev, "stream start failed.\n");
> > -		goto out;
> > -	}
> > -
> >  	/*
> >  	 * We must queue the buffers in the buffer list to the
> >  	 * appropriate video buffer queues BEFORE passing them to the
> > 
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 07/13] media: ipu6: Close firmware streams on streaming enable failure

[Sakari Ailus]

Hi Bingbu,

On Wed, Dec 31, 2025 at 02:11:40PM +0800, Bingbu Cao wrote:
> Sakari,
> 
> Thanks for the patch.
> 
> On 12/30/25 9:10 PM, Sakari Ailus wrote:
> > When enabling streaming fails, the stream is stopped in firmware but not
> > closed. Do this to release resources on firmware side.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > index dec8f5ffcfa5..919b77107cef 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > @@ -1066,6 +1066,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> >  
> >  out_media_entity_stop_streaming_firmware:
> >  	stop_streaming_firmware(av);
> > +	close_streaming_firmware(av);
> 
> It looks the close_streaming_firmware() has no chance to run if
> v4l2_subdev_disable_streams() above failed. Beside, the
> stop_streaming_firmware() is better called after subdev
> disable_streams().

Do you mean we wouldn't need to call stop_streaming_firmware() at all here?
That would be actually aligned with start_streaming_firmware() error
handling.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 07/13] media: ipu6: Close firmware streams on streaming enable failure

[Bingbu Cao]

Sakari,

On 1/2/26 3:58 AM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Wed, Dec 31, 2025 at 02:11:40PM +0800, Bingbu Cao wrote:
>> Sakari,
>>
>> Thanks for the patch.
>>
>> On 12/30/25 9:10 PM, Sakari Ailus wrote:
>>> When enabling streaming fails, the stream is stopped in firmware but not
>>> closed. Do this to release resources on firmware side.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> index dec8f5ffcfa5..919b77107cef 100644
>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>>> @@ -1066,6 +1066,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
>>>  
>>>  out_media_entity_stop_streaming_firmware:
>>>  	stop_streaming_firmware(av);
>>> +	close_streaming_firmware(av);
>>
>> It looks the close_streaming_firmware() has no chance to run if
>> v4l2_subdev_disable_streams() above failed. Beside, the
>> stop_streaming_firmware() is better called after subdev
>> disable_streams().
> 
> Do you mean we wouldn't need to call stop_streaming_firmware() at all here?
> That would be actually aligned with start_streaming_firmware() error
> handling.
>

I mean the code above:
  	if (!state) {
		...
		ret = v4l2_subdev_disable_streams(sd, r_pad->index,
						  stream_mask);
		if (ret) {
			...
			return ret;
		}
		close_streaming_firmware(av);
	} else {

close_streaming_firmware() should be called anyway.

-- 
Best regards,
Bingbu Cao

Re: [PATCH v3 05/13] media: ipu6: Remove redundant streaming start via buffer queueing

[Bingbu Cao]

Sakari,

On 1/2/26 3:51 AM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Wed, Dec 31, 2025 at 02:01:14PM +0800, Bingbu Cao wrote:
>> Hi, Sakari,
>>
>> On 12/30/25 9:10 PM, Sakari Ailus wrote:
>>> The videobuf2 framework will ensure buffers are queued before streaming is
>>> started.
>>
>> The logic is only applicable for !start_streaming_called, right?
>> I am not sure it work for multiplex streams like metadata + frame
>> capture case.
> 
> The start_streaming() callback is only called after the minimum number of
> buffers specified for the queue (1 in this case) have been queued. So in
> case of multiple streams from the same CSI-2 receiver, it's a
> start_streaming() callback on one of the queues that triggers starting
> streaming from the source -- not the buf_queue() callback.
> 
> FWIW, multiple streams don't work with the driver right now anyway -- this
> is addressed by the streaming series I hope I'll be able to post soon,
> after addressing some more review feedback.

Ack.

> 
>>
>>> Remove support for starting starting streaming via the
>>
>> double 'starting' here.
> 
> Fixed for v4.
> 
>>
>>> buf_queue() callback.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>  drivers/media/pci/intel/ipu6/ipu6-isys-queue.c | 7 -------
>>>  1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
>>> index 8f05987cdb4e..fdf41b3cf60e 100644
>>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
>>> @@ -408,13 +408,6 @@ static void buf_queue(struct vb2_buffer *vb)
>>>  	ipu6_isys_buf_to_fw_frame_buf(buf, stream, &bl);
>>>  	ipu6_fw_isys_dump_frame_buff_set(dev, buf, stream->nr_output_pins);
>>>  
>>> -	if (!stream->streaming) {
>>> -		ret = ipu6_isys_stream_start(av, &bl, true);
>>> -		if (ret)
>>> -			dev_err(dev, "stream start failed.\n");
>>> -		goto out;
>>> -	}
>>> -
>>>  	/*
>>>  	 * We must queue the buffers in the buffer list to the
>>>  	 * appropriate video buffer queues BEFORE passing them to the
>>>
>>
> 

-- 
Best regards,
Bingbu Cao