[PATCH 0/5] media: atomisp: stream start/stop error handling improvements

[PATCH 0/5] media: atomisp: stream start/stop error handling improvements

[Hans de Goede]

Hi All,

While working on the mt9m114 driver I introduced a problem where
the sensor's s_stream callback would fail, which turned out to be a good
test-case for the stream start/stop error handling in the atomisp driver.

This series is the result of fixing various error-handling issues which
popped up using this (and other) test-cases.

Regards,

Hans


Hans de Goede (5):
  media: atomisp: Move atomisp_stop_streaming() above
    atomisp_start_streaming()
  media: atomisp: Properly stop the ISP stream on sensor streamon errors
  media: atomisp: Stop pipeline on atomisp_css_start() failure
  media: atomisp: Always free MIPI / CSI-receiver buffers from
    ia_css_uninit()
  media: atomisp: Fix "stop stream timeout." error

 .../media/atomisp/pci/atomisp_compat_css20.c  |   2 +-
 .../staging/media/atomisp/pci/atomisp_fops.c  |   5 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 129 ++++++++----------
 .../media/atomisp/pci/atomisp_subdev.h        |   3 -
 .../staging/media/atomisp/pci/ia_css_pipe.h   |   2 -
 .../pipeline/interface/ia_css_pipeline.h      |   1 -
 .../pci/runtime/pipeline/src/pipeline.c       |   2 -
 drivers/staging/media/atomisp/pci/sh_css.c    |  27 ----
 .../staging/media/atomisp/pci/sh_css_mipi.c   |  11 --
 .../staging/media/atomisp/pci/sh_css_mipi.h   |   2 -
 10 files changed, 62 insertions(+), 122 deletions(-)

-- 
2.49.0

[PATCH 1/5] media: atomisp: Move atomisp_stop_streaming() above atomisp_start_streaming()

[Hans de Goede]

Move atomisp_stop_streaming() above atomisp_start_streaming(), this is
a preparation patch for making atomisp_start_streaming() properly cleanup
if starting the sensor stream fails.

No functional change, only moving a block of code up.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 148 +++++++++---------
 1 file changed, 74 insertions(+), 74 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 97d99bed1560..705f104a2147 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -856,6 +856,80 @@ static void atomisp_dma_burst_len_cfg(struct atomisp_sub_device *asd)
 		atomisp_css2_hw_store_32(DMA_BURST_SIZE_REG, 0x00);
 }
 
+void atomisp_stop_streaming(struct vb2_queue *vq)
+{
+	struct atomisp_video_pipe *pipe = vq_to_pipe(vq);
+	struct atomisp_sub_device *asd = pipe->asd;
+	struct atomisp_device *isp = asd->isp;
+	struct pci_dev *pdev = to_pci_dev(isp->dev);
+	unsigned long flags;
+	int ret;
+
+	dev_dbg(isp->dev, "Stop stream\n");
+
+	mutex_lock(&isp->mutex);
+	/*
+	 * There is no guarantee that the buffers queued to / owned by the ISP
+	 * will properly be returned to the queue when stopping. Set a flag to
+	 * avoid new buffers getting queued and then wait for all the current
+	 * buffers to finish.
+	 */
+	pipe->stopping = true;
+	mutex_unlock(&isp->mutex);
+	/* wait max 1 second */
+	ret = wait_event_timeout(pipe->vb_queue.done_wq,
+				 atomisp_buffers_in_css(pipe) == 0, HZ);
+	mutex_lock(&isp->mutex);
+	pipe->stopping = false;
+	if (ret == 0)
+		dev_warn(isp->dev, "Warning timeout waiting for CSS to return buffers\n");
+
+	spin_lock_irqsave(&isp->lock, flags);
+	asd->streaming = false;
+	spin_unlock_irqrestore(&isp->lock, flags);
+
+	atomisp_clear_css_buffer_counters(asd);
+	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
+
+	atomisp_css_stop(asd, false);
+
+	atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_ERROR, true);
+
+	atomisp_subdev_cleanup_pending_events(asd);
+
+	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].csi_remote_source,
+			       video, s_stream, 0);
+	if (ret)
+		dev_warn(isp->dev, "Stopping sensor stream failed: %d\n", ret);
+
+	/* Disable the CSI interface on ANN B0/K0 */
+	if (isp->media_dev.hw_revision >= ((ATOMISP_HW_REVISION_ISP2401 <<
+					    ATOMISP_HW_REVISION_SHIFT) | ATOMISP_HW_STEPPING_B0)) {
+		pci_write_config_word(pdev, MRFLD_PCI_CSI_CONTROL,
+				      isp->saved_regs.csi_control & ~MRFLD_PCI_CSI_CONTROL_CSI_READY);
+	}
+
+	if (atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_LOW, false))
+		dev_warn(isp->dev, "DFS failed.\n");
+
+	/*
+	 * ISP work around, need to reset ISP to allow next stream on to work.
+	 * Streams have already been destroyed by atomisp_css_stop().
+	 * Disable PUNIT/ISP acknowledge/handshake - SRSE=3 and then reset.
+	 */
+	pci_write_config_dword(pdev, PCI_I_CONTROL,
+			       isp->saved_regs.i_control | MRFLD_PCI_I_CONTROL_SRSE_RESET_MASK);
+	atomisp_reset(isp);
+
+	/* Streams were destroyed by atomisp_css_stop(), recreate them. */
+	ret = atomisp_create_pipes_stream(&isp->asd);
+	if (ret)
+		dev_warn(isp->dev, "Recreating streams failed: %d\n", ret);
+
+	media_pipeline_stop(&asd->video_out.vdev.entity.pads[0]);
+	mutex_unlock(&isp->mutex);
+}
+
 int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct atomisp_video_pipe *pipe = vq_to_pipe(vq);
@@ -961,80 +1035,6 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return ret;
 }
 
-void atomisp_stop_streaming(struct vb2_queue *vq)
-{
-	struct atomisp_video_pipe *pipe = vq_to_pipe(vq);
-	struct atomisp_sub_device *asd = pipe->asd;
-	struct atomisp_device *isp = asd->isp;
-	struct pci_dev *pdev = to_pci_dev(isp->dev);
-	unsigned long flags;
-	int ret;
-
-	dev_dbg(isp->dev, "Stop stream\n");
-
-	mutex_lock(&isp->mutex);
-	/*
-	 * There is no guarantee that the buffers queued to / owned by the ISP
-	 * will properly be returned to the queue when stopping. Set a flag to
-	 * avoid new buffers getting queued and then wait for all the current
-	 * buffers to finish.
-	 */
-	pipe->stopping = true;
-	mutex_unlock(&isp->mutex);
-	/* wait max 1 second */
-	ret = wait_event_timeout(pipe->vb_queue.done_wq,
-				 atomisp_buffers_in_css(pipe) == 0, HZ);
-	mutex_lock(&isp->mutex);
-	pipe->stopping = false;
-	if (ret == 0)
-		dev_warn(isp->dev, "Warning timeout waiting for CSS to return buffers\n");
-
-	spin_lock_irqsave(&isp->lock, flags);
-	asd->streaming = false;
-	spin_unlock_irqrestore(&isp->lock, flags);
-
-	atomisp_clear_css_buffer_counters(asd);
-	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
-
-	atomisp_css_stop(asd, false);
-
-	atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_ERROR, true);
-
-	atomisp_subdev_cleanup_pending_events(asd);
-
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].csi_remote_source,
-			       video, s_stream, 0);
-	if (ret)
-		dev_warn(isp->dev, "Stopping sensor stream failed: %d\n", ret);
-
-	/* Disable the CSI interface on ANN B0/K0 */
-	if (isp->media_dev.hw_revision >= ((ATOMISP_HW_REVISION_ISP2401 <<
-					    ATOMISP_HW_REVISION_SHIFT) | ATOMISP_HW_STEPPING_B0)) {
-		pci_write_config_word(pdev, MRFLD_PCI_CSI_CONTROL,
-				      isp->saved_regs.csi_control & ~MRFLD_PCI_CSI_CONTROL_CSI_READY);
-	}
-
-	if (atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_LOW, false))
-		dev_warn(isp->dev, "DFS failed.\n");
-
-	/*
-	 * ISP work around, need to reset ISP to allow next stream on to work.
-	 * Streams have already been destroyed by atomisp_css_stop().
-	 * Disable PUNIT/ISP acknowledge/handshake - SRSE=3 and then reset.
-	 */
-	pci_write_config_dword(pdev, PCI_I_CONTROL,
-			       isp->saved_regs.i_control | MRFLD_PCI_I_CONTROL_SRSE_RESET_MASK);
-	atomisp_reset(isp);
-
-	/* Streams were destroyed by atomisp_css_stop(), recreate them. */
-	ret = atomisp_create_pipes_stream(&isp->asd);
-	if (ret)
-		dev_warn(isp->dev, "Recreating streams failed: %d\n", ret);
-
-	media_pipeline_stop(&asd->video_out.vdev.entity.pads[0]);
-	mutex_unlock(&isp->mutex);
-}
-
 /*
  * To get the current value of a control.
  * applications initialize the id field of a struct v4l2_control and
-- 
2.49.0

[PATCH 2/5] media: atomisp: Properly stop the ISP stream on sensor streamon errors

[Hans de Goede]

When v4l2_subdev_call(sensor, s_stream, 1) fails atomisp_start_streaming()
was not properly returning the buffer ownership back to the videobuf2-core
code, resulting in:

[ 1318.153447] ------------[ cut here ]------------
[ 1318.153499] WARNING: CPU: 0 PID: 4856 at drivers/media/common/videobuf2/videobuf2-core.c:1803 vb2_start_streaming+0xcb/0x160 [videobuf2_common]
...
[ 1318.154551] Call Trace:
[ 1318.154560]  <TASK>
[ 1318.154571]  ? __warn.cold+0xb7/0x14a
[ 1318.154591]  ? vb2_start_streaming+0xcb/0x160 [videobuf2_common]
[ 1318.154617]  ? report_bug+0xe0/0x180
[ 1318.154640]  ? handle_bug+0x5e/0xa0
[ 1318.154652]  ? exc_invalid_op+0x14/0x70
[ 1318.154665]  ? asm_exc_invalid_op+0x16/0x20
[ 1318.154697]  ? vb2_start_streaming+0xcb/0x160 [videobuf2_common]
[ 1318.154723]  ? vb2_start_streaming+0x70/0x160 [videobuf2_common]
[ 1318.154748]  vb2_core_streamon+0xa2/0x100 [videobuf2_common]

The sensor streamon call is the last thing that atomisp_start_streaming()
does and it was failing to undo all of the previous steps in general.

Refactor atomisp_stop_streaming() into an atomisp_stop_stream() helper and
call that on sensor streamon failure to properly clean things up.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 38 +++++++++++--------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 705f104a2147..491af67cc7a8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -856,18 +856,15 @@ static void atomisp_dma_burst_len_cfg(struct atomisp_sub_device *asd)
 		atomisp_css2_hw_store_32(DMA_BURST_SIZE_REG, 0x00);
 }
 
-void atomisp_stop_streaming(struct vb2_queue *vq)
+static void atomisp_stop_stream(struct atomisp_video_pipe *pipe,
+				bool stop_sensor, enum vb2_buffer_state state)
 {
-	struct atomisp_video_pipe *pipe = vq_to_pipe(vq);
 	struct atomisp_sub_device *asd = pipe->asd;
 	struct atomisp_device *isp = asd->isp;
 	struct pci_dev *pdev = to_pci_dev(isp->dev);
 	unsigned long flags;
 	int ret;
 
-	dev_dbg(isp->dev, "Stop stream\n");
-
-	mutex_lock(&isp->mutex);
 	/*
 	 * There is no guarantee that the buffers queued to / owned by the ISP
 	 * will properly be returned to the queue when stopping. Set a flag to
@@ -893,14 +890,16 @@ void atomisp_stop_streaming(struct vb2_queue *vq)
 
 	atomisp_css_stop(asd, false);
 
-	atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_ERROR, true);
+	atomisp_flush_video_pipe(pipe, state, true);
 
 	atomisp_subdev_cleanup_pending_events(asd);
 
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].csi_remote_source,
-			       video, s_stream, 0);
-	if (ret)
-		dev_warn(isp->dev, "Stopping sensor stream failed: %d\n", ret);
+	if (stop_sensor) {
+		ret = v4l2_subdev_call(isp->inputs[asd->input_curr].csi_remote_source,
+				       video, s_stream, 0);
+		if (ret)
+			dev_warn(isp->dev, "Stopping sensor stream failed: %d\n", ret);
+	}
 
 	/* Disable the CSI interface on ANN B0/K0 */
 	if (isp->media_dev.hw_revision >= ((ATOMISP_HW_REVISION_ISP2401 <<
@@ -927,7 +926,6 @@ void atomisp_stop_streaming(struct vb2_queue *vq)
 		dev_warn(isp->dev, "Recreating streams failed: %d\n", ret);
 
 	media_pipeline_stop(&asd->video_out.vdev.entity.pads[0]);
-	mutex_unlock(&isp->mutex);
 }
 
 int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
@@ -1023,11 +1021,7 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 			       video, s_stream, 1);
 	if (ret) {
 		dev_err(isp->dev, "Starting sensor stream failed: %d\n", ret);
-		spin_lock_irqsave(&isp->lock, irqflags);
-		asd->streaming = false;
-		spin_unlock_irqrestore(&isp->lock, irqflags);
-		ret = -EINVAL;
-		goto out_unlock;
+		atomisp_stop_stream(pipe, false, VB2_BUF_STATE_QUEUED);
 	}
 
 out_unlock:
@@ -1035,6 +1029,18 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return ret;
 }
 
+void atomisp_stop_streaming(struct vb2_queue *vq)
+{
+	struct atomisp_video_pipe *pipe = vq_to_pipe(vq);
+	struct atomisp_device *isp = pipe->asd->isp;
+
+	dev_dbg(isp->dev, "Stop stream\n");
+
+	mutex_lock(&isp->mutex);
+	atomisp_stop_stream(pipe, true, VB2_BUF_STATE_ERROR);
+	mutex_unlock(&isp->mutex);
+}
+
 /*
  * To get the current value of a control.
  * applications initialize the id field of a struct v4l2_control and
-- 
2.49.0

[PATCH 3/5] media: atomisp: Stop pipeline on atomisp_css_start() failure

[Hans de Goede]

atomisp_start_streaming() starts the media pipeline before calling
atomisp_css_start(). On atomisp_css_start() failures stop the pipeline
before returning the error.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 491af67cc7a8..fecba2588e38 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -982,6 +982,7 @@ int atomisp_start_streaming(struct vb2_queue *vq, unsigned int count)
 	ret = atomisp_css_start(asd);
 	if (ret) {
 		atomisp_flush_video_pipe(pipe, VB2_BUF_STATE_QUEUED, true);
+		media_pipeline_stop(&asd->video_out.vdev.entity.pads[0]);
 		goto out_unlock;
 	}
 
-- 
2.49.0

[PATCH 4/5] media: atomisp: Always free MIPI / CSI-receiver buffers from ia_css_uninit()

[Hans de Goede]

The atomisp interrupt handling will free the MIPI / CSI-receiver buffers
when processing a frame-completion event if the stop_requested flag is set,
but only in the ISP2400 / BYT, not in the ISP2401 / CHT case.

There are 2 problems with this:

1. Since this is only done in the BYT case the "mipi frames are not freed."
   warning always triggers on CHT devices.

2. There are 2 stop_requested flags, ia_css_pipe.stop_requested and
   ia_css_pipeline.stop_requested. The ISR checks the ia_css_pipe flag,
   but atomisp_css_stop() sets the ia_css_pipeline.stop_requested flag.
   So even on BYT freeing the buffers from the ISR never happens.

   This likely is a good thing since the buffers get freed on the first
   frame completion event and there might be multiple frames queued up.

Fix things by completely dropping the freeing of the MIPI buffers from
the ISR as well as the stop_requested flag always freeing the buffers
from ia_css_uninit().

Also drop the warning since this now always is expected behavior.

Note that ia_css_uninit() get called whenever streaming is stopped
through atomisp_stop_stream() calling atomisp_reset() so the buffers
are still freed whenever streaming is stopped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/ia_css_pipe.h   |  2 --
 .../pipeline/interface/ia_css_pipeline.h      |  1 -
 .../pci/runtime/pipeline/src/pipeline.c       |  2 --
 drivers/staging/media/atomisp/pci/sh_css.c    | 27 -------------------
 .../staging/media/atomisp/pci/sh_css_mipi.c   | 11 --------
 .../staging/media/atomisp/pci/sh_css_mipi.h   |  2 --
 6 files changed, 45 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/ia_css_pipe.h b/drivers/staging/media/atomisp/pci/ia_css_pipe.h
index c97d2ae356fd..77072694eb42 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_pipe.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_pipe.h
@@ -102,8 +102,6 @@ struct ia_css_yuvpp_settings {
 struct osys_object;
 
 struct ia_css_pipe {
-	/* TODO: Remove stop_requested and use stop_requested in the pipeline */
-	bool                            stop_requested;
 	struct ia_css_pipe_config       config;
 	struct ia_css_pipe_extra_config extra_config;
 	struct ia_css_pipe_info         info;
diff --git a/drivers/staging/media/atomisp/pci/runtime/pipeline/interface/ia_css_pipeline.h b/drivers/staging/media/atomisp/pci/runtime/pipeline/interface/ia_css_pipeline.h
index 316eaa2070ea..8b7cbf31a1a2 100644
--- a/drivers/staging/media/atomisp/pci/runtime/pipeline/interface/ia_css_pipeline.h
+++ b/drivers/staging/media/atomisp/pci/runtime/pipeline/interface/ia_css_pipeline.h
@@ -34,7 +34,6 @@ struct ia_css_pipeline_stage {
 struct ia_css_pipeline {
 	enum ia_css_pipe_id pipe_id;
 	u8 pipe_num;
-	bool stop_requested;
 	struct ia_css_pipeline_stage *stages;
 	struct ia_css_pipeline_stage *current_stage;
 	unsigned int num_stages;
diff --git a/drivers/staging/media/atomisp/pci/runtime/pipeline/src/pipeline.c b/drivers/staging/media/atomisp/pci/runtime/pipeline/src/pipeline.c
index aabebe61ec77..cb8d652227a7 100644
--- a/drivers/staging/media/atomisp/pci/runtime/pipeline/src/pipeline.c
+++ b/drivers/staging/media/atomisp/pci/runtime/pipeline/src/pipeline.c
@@ -198,7 +198,6 @@ int ia_css_pipeline_request_stop(struct ia_css_pipeline *pipeline)
 	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
 			    "ia_css_pipeline_request_stop() enter: pipeline=%p\n",
 			    pipeline);
-	pipeline->stop_requested = true;
 
 	/* Send stop event to the sp*/
 	/* This needs improvement, stop on all the pipes available
@@ -663,7 +662,6 @@ static void pipeline_init_defaults(
 
 	pipeline->pipe_id = pipe_id;
 	pipeline->stages = NULL;
-	pipeline->stop_requested = false;
 	pipeline->current_stage = NULL;
 
 	memcpy(&pipeline->in_frame, &ia_css_default_frame,
diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 5a8e8e67aa13..73bd87f43a8c 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -2002,10 +2002,6 @@ ia_css_uninit(void)
 
 	sh_css_params_free_default_gdc_lut();
 
-	/* TODO: JB: implement decent check and handling of freeing mipi frames */
-	if (!mipi_is_free())
-		dev_warn(atomisp_dev, "mipi frames are not freed.\n");
-
 	/* cleanup generic data */
 	sh_css_params_uninit();
 	ia_css_refcount_uninit();
@@ -3743,23 +3739,6 @@ ia_css_pipe_dequeue_buffer(struct ia_css_pipe *pipe,
 			case IA_CSS_BUFFER_TYPE_INPUT_FRAME:
 			case IA_CSS_BUFFER_TYPE_OUTPUT_FRAME:
 			case IA_CSS_BUFFER_TYPE_SEC_OUTPUT_FRAME:
-				if (pipe && pipe->stop_requested) {
-					if (!IS_ISP2401) {
-						/*
-						 * free mipi frames only for old input
-						 * system for 2401 it is done in
-						 * ia_css_stream_destroy call
-						 */
-						return_err = free_mipi_frames(pipe);
-						if (return_err) {
-							IA_CSS_LOG("free_mipi_frames() failed");
-							IA_CSS_LEAVE_ERR(return_err);
-							return return_err;
-						}
-					}
-					pipe->stop_requested = false;
-				}
-				fallthrough;
 			case IA_CSS_BUFFER_TYPE_VF_OUTPUT_FRAME:
 			case IA_CSS_BUFFER_TYPE_SEC_VF_OUTPUT_FRAME:
 				frame = (struct ia_css_frame *)HOST_ADDRESS(ddr_buffer.kernel_ptr);
@@ -4095,8 +4074,6 @@ sh_css_pipe_start(struct ia_css_stream *stream)
 		return err;
 	}
 
-	pipe->stop_requested = false;
-
 	switch (pipe_id) {
 	case IA_CSS_PIPE_ID_PREVIEW:
 		err = preview_start(pipe);
@@ -4120,19 +4097,15 @@ sh_css_pipe_start(struct ia_css_stream *stream)
 		for (i = 1; i < stream->num_pipes && 0 == err ; i++) {
 			switch (stream->pipes[i]->mode) {
 			case IA_CSS_PIPE_ID_PREVIEW:
-				stream->pipes[i]->stop_requested = false;
 				err = preview_start(stream->pipes[i]);
 				break;
 			case IA_CSS_PIPE_ID_VIDEO:
-				stream->pipes[i]->stop_requested = false;
 				err = video_start(stream->pipes[i]);
 				break;
 			case IA_CSS_PIPE_ID_CAPTURE:
-				stream->pipes[i]->stop_requested = false;
 				err = capture_start(stream->pipes[i]);
 				break;
 			case IA_CSS_PIPE_ID_YUVPP:
-				stream->pipes[i]->stop_requested = false;
 				err = yuvpp_start(stream->pipes[i]);
 				break;
 			default:
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index 42f14ed853e1..971b685cdb58 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -185,17 +185,6 @@ mipi_init(void)
 		ref_count_mipi_allocation[i] = 0;
 }
 
-bool mipi_is_free(void)
-{
-	unsigned int i;
-
-	for (i = 0; i < N_CSI_PORTS; i++)
-		if (ref_count_mipi_allocation[i])
-			return false;
-
-	return true;
-}
-
 /*
  * @brief Calculate the required MIPI buffer sizes.
  * Based on the stream configuration, calculate the
diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.h b/drivers/staging/media/atomisp/pci/sh_css_mipi.h
index 6f7389f44baa..b3887ee3c75a 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.h
@@ -14,8 +14,6 @@
 void
 mipi_init(void);
 
-bool mipi_is_free(void);
-
 int
 allocate_mipi_frames(struct ia_css_pipe *pipe, struct ia_css_stream_info *info);
 
-- 
2.49.0

[PATCH 5/5] media: atomisp: Fix "stop stream timeout." error

[Hans de Goede]

Commit c7194b21809e ("media: atomisp: On streamoff wait for buffers owned
by the CSS to be given back") added draining of the CSS buffer queue to
the beginning of atomisp_stop_stream().

But it turns out that when telling the CSS to stop streaming it needs at
least 1 buffer queued, because the CSS firmware waits for a frame to be
completed before stopping and without buffers it cannot complete a frame.

At the end of atomisp_stop_stream() it is always safe to return buffer
ownership to the videobuf2-core. Either atomisp_css_stop() has successfully
stopped the stream; or the atomisp_reset() later on which power-cycles
the ISP will definitely have stopped the stream.

Drop the draining of the CSS buffer queue to fix the "stop stream timeout."
error and move the atomisp_flush_video_pipe() call after atomisp_reset(),
passing false for the warn_on_css_frames flag since some buffers still
being marked as owned by the CSS expected on stream off.

Also increase the timeout in destroy_stream(), since this waits for
the last frame to be completed this can take longer then 40 ms. When e.g.
using a framerate of 15 fps, this could take 66ms, make the timeout 200 ms
to be on the safe side.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note no fixes tag since the code has changed a lot since the original
commit introducing the draining of the buffer queue
---
 .../media/atomisp/pci/atomisp_compat_css20.c  |  2 +-
 .../staging/media/atomisp/pci/atomisp_fops.c  |  5 +----
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 20 ++-----------------
 .../media/atomisp/pci/atomisp_subdev.h        |  3 ---
 4 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index bc97fa2c374c..be5f37f4a6fd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -387,7 +387,7 @@ static int __destroy_stream(struct atomisp_sub_device *asd,
 	}
 
 	if (stream_env->stream_state == CSS_STREAM_STARTED) {
-		timeout = jiffies + msecs_to_jiffies(40);
+		timeout = jiffies + msecs_to_jiffies(200);
 		while (1) {
 			if (ia_css_stream_has_stopped(stream_env->stream))
 				break;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 57da7ddb1503..c7aef066f209 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -234,9 +234,6 @@ static int atomisp_q_video_buffers_to_css(struct atomisp_sub_device *asd,
 	if (WARN_ON(css_pipe_id >= IA_CSS_PIPE_ID_NUM))
 		return -EINVAL;
 
-	if (pipe->stopping)
-		return -EINVAL;
-
 	space = ATOMISP_CSS_Q_DEPTH - atomisp_buffers_in_css(pipe);
 	while (space--) {
 		struct ia_css_frame *frame;
@@ -367,7 +364,7 @@ static void atomisp_buf_queue(struct vb2_buffer *vb)
 	mutex_lock(&asd->isp->mutex);
 
 	ret = atomisp_pipe_check(pipe, false);
-	if (ret || pipe->stopping) {
+	if (ret) {
 		spin_lock_irqsave(&pipe->irq_lock, irqflags);
 		atomisp_buffer_done(frame, VB2_BUF_STATE_ERROR);
 		spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index fecba2588e38..bb8b2f2213b0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -865,22 +865,6 @@ static void atomisp_stop_stream(struct atomisp_video_pipe *pipe,
 	unsigned long flags;
 	int ret;
 
-	/*
-	 * There is no guarantee that the buffers queued to / owned by the ISP
-	 * will properly be returned to the queue when stopping. Set a flag to
-	 * avoid new buffers getting queued and then wait for all the current
-	 * buffers to finish.
-	 */
-	pipe->stopping = true;
-	mutex_unlock(&isp->mutex);
-	/* wait max 1 second */
-	ret = wait_event_timeout(pipe->vb_queue.done_wq,
-				 atomisp_buffers_in_css(pipe) == 0, HZ);
-	mutex_lock(&isp->mutex);
-	pipe->stopping = false;
-	if (ret == 0)
-		dev_warn(isp->dev, "Warning timeout waiting for CSS to return buffers\n");
-
 	spin_lock_irqsave(&isp->lock, flags);
 	asd->streaming = false;
 	spin_unlock_irqrestore(&isp->lock, flags);
@@ -890,8 +874,6 @@ static void atomisp_stop_stream(struct atomisp_video_pipe *pipe,
 
 	atomisp_css_stop(asd, false);
 
-	atomisp_flush_video_pipe(pipe, state, true);
-
 	atomisp_subdev_cleanup_pending_events(asd);
 
 	if (stop_sensor) {
@@ -920,6 +902,8 @@ static void atomisp_stop_stream(struct atomisp_video_pipe *pipe,
 			       isp->saved_regs.i_control | MRFLD_PCI_I_CONTROL_SRSE_RESET_MASK);
 	atomisp_reset(isp);
 
+	atomisp_flush_video_pipe(pipe, state, false);
+
 	/* Streams were destroyed by atomisp_css_stop(), recreate them. */
 	ret = atomisp_create_pipes_stream(&isp->asd);
 	if (ret)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index bd1a198cda30..e1d0168cb91d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -57,9 +57,6 @@ struct atomisp_video_pipe {
 	/* Filled through atomisp_get_css_frame_info() on queue setup */
 	struct ia_css_frame_info frame_info;
 
-	/* Set from streamoff to disallow queuing further buffers in CSS */
-	bool stopping;
-
 	/*
 	 * irq_lock is used to protect video buffer state change operations and
 	 * also to make activeq and capq operations atomic.
-- 
2.49.0

Re: [PATCH 0/5] media: atomisp: stream start/stop error handling improvements

[Andy Shevchenko]

On Tue, May 6, 2025 at 12:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> While working on the mt9m114 driver I introduced a problem where
> the sensor's s_stream callback would fail, which turned out to be a good
> test-case for the stream start/stop error handling in the atomisp driver.
>
> This series is the result of fixing various error-handling issues which
> popped up using this (and other) test-cases.

LGTM,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] media: atomisp: stream start/stop error handling improvements

[Hans de Goede]

Hi All,

On 5-May-25 11:00 PM, Hans de Goede wrote:
> Hi All,
> 
> While working on the mt9m114 driver I introduced a problem where
> the sensor's s_stream callback would fail, which turned out to be a good
> test-case for the stream start/stop error handling in the atomisp driver.
> 
> This series is the result of fixing various error-handling issues which
> popped up using this (and other) test-cases.
> 
> Regards,
> 
> Hans

I have merged this in my media-atomisp branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

And this series will be included in my next
pull-request to Mauro (to media subsystem maintainer)

Regards,

Hans



> Hans de Goede (5):
>   media: atomisp: Move atomisp_stop_streaming() above
>     atomisp_start_streaming()
>   media: atomisp: Properly stop the ISP stream on sensor streamon errors
>   media: atomisp: Stop pipeline on atomisp_css_start() failure
>   media: atomisp: Always free MIPI / CSI-receiver buffers from
>     ia_css_uninit()
>   media: atomisp: Fix "stop stream timeout." error
> 
>  .../media/atomisp/pci/atomisp_compat_css20.c  |   2 +-
>  .../staging/media/atomisp/pci/atomisp_fops.c  |   5 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 129 ++++++++----------
>  .../media/atomisp/pci/atomisp_subdev.h        |   3 -
>  .../staging/media/atomisp/pci/ia_css_pipe.h   |   2 -
>  .../pipeline/interface/ia_css_pipeline.h      |   1 -
>  .../pci/runtime/pipeline/src/pipeline.c       |   2 -
>  drivers/staging/media/atomisp/pci/sh_css.c    |  27 ----
>  .../staging/media/atomisp/pci/sh_css_mipi.c   |  11 --
>  .../staging/media/atomisp/pci/sh_css_mipi.h   |   2 -
>  10 files changed, 62 insertions(+), 122 deletions(-)
>