* [PATCH 1/5] media: atomisp: Move atomisp_stop_streaming() above atomisp_start_streaming()
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
@ 2025-05-05 21:00 ` Hans de Goede
2025-05-05 21:00 ` [PATCH 2/5] media: atomisp: Properly stop the ISP stream on sensor streamon errors Hans de Goede
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-05-05 21:00 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/5] media: atomisp: Properly stop the ISP stream on sensor streamon errors
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
2025-05-05 21:00 ` [PATCH 1/5] media: atomisp: Move atomisp_stop_streaming() above atomisp_start_streaming() Hans de Goede
@ 2025-05-05 21:00 ` Hans de Goede
2025-05-05 21:00 ` [PATCH 3/5] media: atomisp: Stop pipeline on atomisp_css_start() failure Hans de Goede
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-05-05 21:00 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/5] media: atomisp: Stop pipeline on atomisp_css_start() failure
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
2025-05-05 21:00 ` [PATCH 1/5] media: atomisp: Move atomisp_stop_streaming() above atomisp_start_streaming() Hans de Goede
2025-05-05 21:00 ` [PATCH 2/5] media: atomisp: Properly stop the ISP stream on sensor streamon errors Hans de Goede
@ 2025-05-05 21:00 ` Hans de Goede
2025-05-05 21:00 ` [PATCH 4/5] media: atomisp: Always free MIPI / CSI-receiver buffers from ia_css_uninit() Hans de Goede
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-05-05 21:00 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/5] media: atomisp: Always free MIPI / CSI-receiver buffers from ia_css_uninit()
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
` (2 preceding siblings ...)
2025-05-05 21:00 ` [PATCH 3/5] media: atomisp: Stop pipeline on atomisp_css_start() failure Hans de Goede
@ 2025-05-05 21:00 ` Hans de Goede
2025-05-05 21:00 ` [PATCH 5/5] media: atomisp: Fix "stop stream timeout." error Hans de Goede
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-05-05 21:00 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 5/5] media: atomisp: Fix "stop stream timeout." error
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
` (3 preceding siblings ...)
2025-05-05 21:00 ` [PATCH 4/5] media: atomisp: Always free MIPI / CSI-receiver buffers from ia_css_uninit() Hans de Goede
@ 2025-05-05 21:00 ` Hans de Goede
2025-05-07 6:18 ` [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Andy Shevchenko
2025-07-04 9:35 ` Hans de Goede
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-05-05 21:00 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/5] media: atomisp: stream start/stop error handling improvements
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
` (4 preceding siblings ...)
2025-05-05 21:00 ` [PATCH 5/5] media: atomisp: Fix "stop stream timeout." error Hans de Goede
@ 2025-05-07 6:18 ` Andy Shevchenko
2025-07-04 9:35 ` Hans de Goede
6 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2025-05-07 6:18 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Andy Shevchenko, Mauro Carvalho Chehab, linux-media,
linux-staging
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/5] media: atomisp: stream start/stop error handling improvements
2025-05-05 21:00 [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Hans de Goede
` (5 preceding siblings ...)
2025-05-07 6:18 ` [PATCH 0/5] media: atomisp: stream start/stop error handling improvements Andy Shevchenko
@ 2025-07-04 9:35 ` Hans de Goede
6 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2025-07-04 9:35 UTC (permalink / raw)
To: Hans de Goede, Sakari Ailus, Andy Shevchenko
Cc: Mauro Carvalho Chehab, linux-media, linux-staging
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(-)
>
^ permalink raw reply [flat|nested] 8+ messages in thread