[PATCH v4 00/14] IPU6 driver cleanups and fixes

[PATCH v4 00/14] 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 v3:

- Also drop redundant calls of ipu6_mmu_hw_cleanup() in MMU hardware init
  cleanup patch.

- Improved description of removing redundant streaming start mechanism.

- Always call close_streaming_firmware(), even if disabling streaming on
  upstream sub-device fails.

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 (14):
  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: Always close firmware stream
  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    | 88 ++++++-------------
 .../media/pci/intel/ipu6/ipu6-isys-video.h    |  6 +-
 drivers/media/pci/intel/ipu6/ipu6-isys.c      | 21 +----
 drivers/media/pci/intel/ipu6/ipu6-isys.h      |  2 -
 9 files changed, 73 insertions(+), 160 deletions(-)

-- 
2.47.3

[PATCH v4 01/14] 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 v4 02/14] media: ipu6: Drop MMU hardware initialisation in probe()

[Sakari Ailus]

The MMU hardware is initialised and cleaned up in the runtime PM resume
callbacks. Do not do this in probe().

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

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index fc0ec0a4b8f5..226bfae21c16 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;
 
@@ -1125,8 +1121,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
 	if (ret)
 		goto free_fw_msg_bufs;
 
-	ipu6_mmu_hw_cleanup(adev->mmu);
-
 	return 0;
 
 free_fw_msg_bufs:
@@ -1148,8 +1142,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
 	mutex_destroy(&isys->mutex);
 	mutex_destroy(&isys->stream_mutex);
 
-	ipu6_mmu_hw_cleanup(adev->mmu);
-
 	return ret;
 }
 
-- 
2.47.3

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

[Bingbu Cao]

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

On 1/8/26 4:16 PM, Sakari Ailus wrote:
> The MMU hardware is initialised and cleaned up in the runtime PM resume
> callbacks. Do not do this in probe().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> index fc0ec0a4b8f5..226bfae21c16 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;
>  
> @@ -1125,8 +1121,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  	if (ret)
>  		goto free_fw_msg_bufs;
>  
> -	ipu6_mmu_hw_cleanup(adev->mmu);
> -
>  	return 0;
>  
>  free_fw_msg_bufs:
> @@ -1148,8 +1142,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  	mutex_destroy(&isys->mutex);
>  	mutex_destroy(&isys->stream_mutex);
>  
> -	ipu6_mmu_hw_cleanup(adev->mmu);
> -
>  	return ret;
>  }
>  
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 03/14] 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>
Reviewed-by: Bingbu Cao <bingbu.cao@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 226bfae21c16..ef01dcb635fa 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

Re: [PATCH v4 03/14] media: ipu6: Remove redundant driver data checks

[Bingbu Cao]

Sakari,

Thanks for the patch.

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

On 1/8/26 4:17 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>
> Reviewed-by: Bingbu Cao <bingbu.cao@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 226bfae21c16..ef01dcb635fa 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);
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 04/14] 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>
Reviewed-by: Bingbu Cao <bingbu.cao@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 ef01dcb635fa..5a8becff65ae 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

Re: [PATCH v4 04/14] media: ipu6: Make symbols static

[Bingbu Cao]

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

On 1/8/26 4:17 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>
> Reviewed-by: Bingbu Cao <bingbu.cao@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 ef01dcb635fa..5a8becff65ae 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,
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 05/14] 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 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 v4 06/14] 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

Re: [PATCH v4 06/14] media: ipu6: Don't check pipeline in stream_start

[Bingbu Cao]

Sakari,

Thanks for the patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> 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;
>  	}
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 07/14] 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

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

[Bingbu Cao]

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

On 1/8/26 4:17 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);
>  
>  	return ret;
>  }
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 08/14] media: ipu6: Always close firmware stream

[Sakari Ailus]

Close the firmware stream even when disabling a stream on an upstream
sub-device fails. This allows the firmware to release resources related to
a stream that is stopped in any case.

Suggested-by: Bingbu Cao <bingbu.cao@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 5 ++---
 1 file changed, 2 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..54d861aca008 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1036,11 +1036,10 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
 			sd->name, r_pad->index, stream_mask);
 		ret = v4l2_subdev_disable_streams(sd, r_pad->index,
 						  stream_mask);
-		if (ret) {
+		if (ret)
 			dev_err(dev, "stream off %s failed with %d\n", sd->name,
 				ret);
-			return ret;
-		}
+
 		close_streaming_firmware(av);
 	} else {
 		ret = start_stream_firmware(av, bl);
-- 
2.47.3

Re: [PATCH v4 08/14] media: ipu6: Always close firmware stream

[Bingbu Cao]

Sakari,

Thanks for the patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> Close the firmware stream even when disabling a stream on an upstream
> sub-device fails. This allows the firmware to release resources related to
> a stream that is stopped in any case.
> 
> Suggested-by: Bingbu Cao <bingbu.cao@linux.intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 5 ++---
>  1 file changed, 2 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..54d861aca008 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -1036,11 +1036,10 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
>  			sd->name, r_pad->index, stream_mask);
>  		ret = v4l2_subdev_disable_streams(sd, r_pad->index,
>  						  stream_mask);
> -		if (ret) {
> +		if (ret)
>  			dev_err(dev, "stream off %s failed with %d\n", sd->name,
>  				ret);
> -			return ret;
> -		}
> +
>  		close_streaming_firmware(av);
>  	} else {
>  		ret = start_stream_firmware(av, bl);
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 09/14] 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

Re: [PATCH v4 09/14] media: ipu6: Drop error argument from ipu6_isys_stream_start()

[Bingbu Cao]

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

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> 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;
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 10/14] 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

Re: [PATCH v4 10/14] media: ipu6: Obtain remote pad using media_pad_remote_pad_unique()

[Bingbu Cao]

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

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> 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",
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 11/14] 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 54d861aca008..1d58e321bda7 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1204,10 +1204,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

Re: [PATCH v4 11/14] media: ipu6: Obtain unique source pad from remote sub-device

[Bingbu Cao]

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

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> 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 54d861aca008..1d58e321bda7 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -1204,10 +1204,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;
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v4 11/14] media: ipu6: Obtain unique source pad from remote sub-device

[Mehdi Djait]

Hi Sakari,

On Thu, Jan 08, 2026 at 10:17:08AM +0200, Sakari Ailus wrote:
> 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 54d861aca008..1d58e321bda7 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -1204,10 +1204,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");

Don't you think this warrants a dev_err() like elsewhere in the driver ?

--
Kind Regards
Mehdi Djait

Re: [PATCH v4 11/14] media: ipu6: Obtain unique source pad from remote sub-device

[Sakari Ailus]

Hi Mehdi,

Thanks for the review.

On Thu, Jan 08, 2026 at 01:28:57PM +0100, Mehdi Djait wrote:
> Hi Sakari,
> 
> On Thu, Jan 08, 2026 at 10:17:08AM +0200, Sakari Ailus wrote:
> > 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 54d861aca008..1d58e321bda7 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > @@ -1204,10 +1204,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");
> 
> Don't you think this warrants a dev_err() like elsewhere in the driver ?

This isn't really supposed to happen. But if it does, it allows the user to
fill logs with it. I'd keep it on debug level.

-- 
Kind regards,

Sakari Ailus

[PATCH v4 12/14] 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 1d58e321bda7..b035c7342a91 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);
@@ -1179,7 +1172,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);
@@ -1188,30 +1182,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) {
@@ -1231,7 +1209,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 v4 13/14] 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 b035c7342a91..e9dab8a709ef 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

Re: [PATCH v4 13/14] media: ipu6: Drop custom functions to obtain sd state information

[Bingbu Cao]

Sakari,

Thanks for the patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> 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 b035c7342a91..e9dab8a709ef 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;
> 

-- 
Best regards,
Bingbu Cao

[PATCH v4 14/14] 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 e9dab8a709ef..3ac48d2076da 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
@@ -1176,7 +1176,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;
@@ -1216,11 +1215,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 v4 00/14] IPU6 driver cleanups and fixes

[Mehdi Djait]

Hi Sakari,

On Thu, Jan 08, 2026 at 10:16:57AM +0200, Sakari Ailus wrote:
> 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.

[...]

> Sakari Ailus (14):
>   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: Always close firmware stream
>   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    | 88 ++++++-------------
>  .../media/pci/intel/ipu6/ipu6-isys-video.h    |  6 +-
>  drivers/media/pci/intel/ipu6/ipu6-isys.c      | 21 +----
>  drivers/media/pci/intel/ipu6/ipu6-isys.h      |  2 -
>  9 files changed, 73 insertions(+), 160 deletions(-)

Thank you for the patches!

Tested-by: Mehdi Djait <mehdi.djait@linux.intel.com> # Dell XPS 9315
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

--
Kind Regards
Mehdi Djait